Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions openedx/core/djangoapps/external_user_ids/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,58 @@ def add_new_user_id(cls, user, type_name):
)
)
return external_id, created

@classmethod
def batch_get_or_create_user_ids(cls, users, type_name):
"""
Creates ExternalIds in batch.

Given a list of users and a type_name, this method creates new external ids for
users that do not already have one. Then returns a dictionary mapping user id with
corresponding external id.

Arguments:
users: List of User to create the IDs for
type_name (str): Name of the type of ExternalId
Returns:
dict: Returns ExternalIds mapped by User.id
{
user_id: ExternalId
}
"""
try:
type_obj = ExternalIdType.objects.get(name=type_name)
except ExternalIdType.DoesNotExist:
LOGGER.info(
'Batch ID Creation failed, no external id type of {type}'.format(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Batch ID Creation failed, no external id type of {type}'.format(
'Batch ID Creation failed, no external id type of {type!r}'.format(

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

type=type_name
)
)
return None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be {} instead to stick to the signature of the function?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or would it be better to raise an exception? I don't know about ExternalIdType, and what callers expect.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or at least update the docstring)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning behind sending None is to keep it similar to add_new_user_id which also returns None if fails. I've updated the docstring.


# get user ids in a set
user_ids = {user.id for user in users}

# find users for those external ids needs to be created
externalid_count = models.Count('externalid', filter=models.Q(externalid__external_id_type=type_obj))
users_wo_externalid = User.objects.annotate(externalid_count=externalid_count).filter(
externalid_count=0,
id__in=user_ids,
)

# get external ids that already exists
existing_externalids = cls.objects.filter(user__in=users, external_id_type=type_obj)

# prepare result dict with existing externalids.
result = {eid.user_id: eid for eid in existing_externalids}

# if there are users with no external id, create external ids for them
if len(users_wo_externalid) > 0:
new_externalids = cls.objects.bulk_create([
cls(user=user, external_id_type=type_obj) for user in users_wo_externalid
])

# append newly created externalids to result dict
result.update({eid.user_id: eid for eid in new_externalids})

return result

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is returning ExternalId model, when ideally it should return a dict mapping id to external_user_ids and signaling if they where created.

Can you make this function return something like the reponse shown below?

{
    "id_1": ("external_id_1", True),  # (external_id, created)
    "id_2": ("external_id_2", False),
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we keep the external user ID API purely using data without passing classes around.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giovannicimolin, Now the method returns a dictionary as per the suggested structure.

@giovannicimolin giovannicimolin Dec 18, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shimulch I think we can get rid of the created argument here. To simplify the implementation, a "user_id": ExternalId map is enough here.

Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
"""
Test batch_get_or_create in ExternalId model
"""

from django.test import TransactionTestCase
from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangoapps.external_user_ids.models import ExternalId
from openedx.core.djangoapps.external_user_ids.tests.factories import ExternalIDTypeFactory


class TestBatchGenerateExternalIds(TransactionTestCase):
"""
Test ExternalId.batch_get_or_create_user_ids
"""

# Following are the queries
# 1 - Get ExternalIdType
# 2 - Find users for those external ids needs to be created
# 3 - Get external ids that already exists
# 4 - BEGIN (from bulk_create)
# 5 - Create new external ids
EXPECTED_NUM_OF_QUERIES = 5

def test_batch_get_or_create_user_ids(self):
"""
Test if batch_get_or_create creates ExternalIds in batch
"""
id_type = ExternalIDTypeFactory.create(name='test')
users = [UserFactory() for _ in range(10)]

with self.assertNumQueries(self.EXPECTED_NUM_OF_QUERIES):
result = ExternalId.batch_get_or_create_user_ids(users, id_type)

assert len(result) == len(result)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This surely is not what you meant!

Suggested change
assert len(result) == len(result)
assert len(result) == len(users)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that and thanks for catching it.


for user in users:
assert result[user.id].external_id_type.name == 'test'
assert result[user.id].user == user

def test_batch_get_or_create_user_ids_existing_ids(self):
"""
Test batch creation output when there are existing ids for some user
"""
id_type = ExternalIDTypeFactory.create(name='test')

# first let's create some user and externalids for them
users = [UserFactory() for _ in range(10)]

with self.assertNumQueries(self.EXPECTED_NUM_OF_QUERIES):
result = ExternalId.batch_get_or_create_user_ids(users, id_type)

# now create some new user and try to create externalids for all user
new_users = [UserFactory() for _ in range(5)]
all_users = users + new_users

with self.assertNumQueries(self.EXPECTED_NUM_OF_QUERIES):
result = ExternalId.batch_get_or_create_user_ids(all_users, id_type)

assert len(result) == len(all_users)

def test_batch_get_or_create_user_ids_wrong_type(self):
"""
Test if batch_get_or_create returns None if wrong type given
"""
users = [UserFactory() for _ in range(2)]
external_ids = ExternalId.batch_get_or_create_user_ids(users, 'invalid')
assert external_ids is None