[BD-24] [BB-2726] [TNL-7330] Added batch_get_or_create class method for ExternalId by shimulch · Pull Request #25844 · openedx/openedx-platform

shimulch · 2020-12-10T19:21:21Z

openedx/xblock-lti-consumer#124 depends on this PR.

This PR adds a new class method batch_get_or_create_user_ids to ExternalId. This enables a faster way to create ExternalId for a list of User.

When batch_get_or_create_user_ids gets called with a list of User instances and a type_name (Name of the type of ExternalId) it will return a dict mapping user id with corresponding external id.

{
   user_id: ExternalId
}

JIRA tickets: This PR completes a part of the functionality required in the following tickets -

Dependencies: None

Merge deadline: None

Testing instructions:

Pull this PR on your local devstack.
Run your local devstack
Open LMS shell by running make lms-shell
Open Django shell for LMS python manage.py lms shell
Create couple of users -

from django.contrib.auth.models import User
users = [User.objects.create_user(username=f'test{i}', email=f'test{i}@beatles.com', password='test') for i in range(5)]

Import ExternalId model

from openedx.core.djangoapps.external_user_ids.models import ExternalId

Create external ids in batch

result = ExternalId.batch_get_or_create_user_ids(users, 'lti')

Check the result is in the above mentioned format. For example -

{10: <ExternalId: ExternalId object (None)>, 11: <ExternalId: ExternalId object (None)>, 12: <ExternalId: ExternalId object (None)>, 13: <ExternalId: ExternalId object (None)>, 14: <ExternalId: ExternalId object (None)>}

Author notes and concerns:
N/A

Reviewers

@giovannicimolin
edX reviewer[s] TBD

openedx-webhooks · 2020-12-10T19:21:26Z

Thanks for the pull request, @shimulch! I've created BLENDED-789 to keep track of it in Jira. More details are on the BD-24 project page.

When this pull request is ready, tag your edX technical lead.

giovannicimolin

Left a few comments.

giovannicimolin · 2020-12-14T19:26:10Z

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), }

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

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

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

giovannicimolin · 2020-12-14T19:36:47Z

Suggested change

def batch_get_or_create(cls, users, type_name):

def batch_get_or_create_user_ids(cls, users, type_name):

giovannicimolin · 2020-12-14T20:09:16Z

I think this is not the optimal solution, given that a potentially big user dataset might come in.
Forcing a queryset evaluation and then subtracting two user objects sets might be an expensive operation.

@giovannicimolin, I've optimized it to use a query instead of set operation.

natabene · 2020-12-17T02:46:07Z

@shimulch Thank you for your contribution. Please ping me once it is ready for edX review.

giovannicimolin

Aside a minor change in the data returned and a comment about a test, this is good to go.

giovannicimolin · 2020-12-18T13:01:07Z

Since this is a batch operation, we should ideally add some query count test here, to make sure more users != more queries.

This can be done with a simple assertion in the test_batch_get_or_create_user_ids (see assertNumQueries, might require a change to TransactionTestCase instead of TestCase).

Tests with the expected number of queries added. Thanks for the tip 👍🏽

giovannicimolin · 2020-12-18T13:20:51Z

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

bradenmacdonald · 2021-01-08T02:35:56Z

Can you please provide an example of when it's useful to create these in a batch, instead of creating them for each user as needed?

@bradenmacdonald, In the case of LTI Consumer, External ID's are created when a student actually launches LTI from LMS. But when providing course member information to a third party tool, not every member has External ID's available. To create one by one would be a performance issue. Thus this method has been created.

@shimulch Oops, I meant to specify that I'd like that added to the docstring. Dosctrings should generally say both what the code does and why it exists, because without the why it's hard to know when/how it can be refactored or removed in the future.

natabene · 2021-03-10T19:36:19Z

@shimulch Just checking if you are still planning on pursuing this PR.

pomegranited · 2021-03-11T00:35:13Z

@natabene CC @bradenmacdonald @shimulch This PR is related to openedx/xblock-lti-consumer#124, which is for a Blended project. Could someone tag it so we know it's not an ordinary OSPR?

giovannicimolin · 2021-03-11T12:41:21Z

@natabene As @pomegranited mentioned, this is related to BD-24. We ended up having discussions related to the implementation on this ADR before continuing the work. We're now unblocked and will start moving this forward in the coming week.

giovannicimolin

@shimulch Code is working perfectly, but I have a few nits and changes:

Python APIs should not return internal classes/objects whenever possible. Please change the ExternalId object on the response with the plain text external id.
Improve this PR test instructions and proper testing instructions for verifying this works on the devstack.
Clean up the unused description fields.
Rebase this PR on the latest master.

giovannicimolin · 2021-03-15T18:07:41Z

ExternalIds have no relation to LTI. This explanation should be in the lti_consumer code that uses this API.
Here you should put a description of what this API does, its limitations and return format.

Removed, LTI specific doc, and added generic doc which explains what this method does.

Update doc string

shimulch · 2021-03-18T07:19:00Z

@giovannicimolin,

Python APIs should not return internal classes/objects whenever possible. Please change the ExternalId object on the response with the plain text external id.

This method is just a batch version of the existing add_new_user_id method which returns the ExternalId object as well.

Improve this PR test instructions and proper testing instructions for verifying this works on the devstack.

Thanks for pointing that out. I've added proper test instruction on how to verify the new method.

Clean up the unused description fields.

If you are talking about the PR description, then yes, I've cleaned up. If that's not the case I am probably not sure what you meant.

Rebase this PR on the latest master.

Rebased.

giovannicimolin

👍

I tested this:

Followed the testing instructions on the devstack.
Checked that the query count didn't increase when increasing the number of users created (tested with 2, 20, and 200 users in the test script above).

I read through the code
~~I checked for accessibility issues~~ NA
Includes documentation: In docstrings

@shimulch This is good to go. Great work.
@nedbat This is ready for your review.

nedbat · 2021-03-22T19:56:19Z

+            type_obj = ExternalIdType.objects.get(name=type_name)
+        except ExternalIdType.DoesNotExist:
+            LOGGER.info(
+                'Batch ID Creation failed, no external id type of {type}'.format(


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(

nedbat · 2021-03-22T19:59:10Z

+                    type=type_name
+                )
+            )
+            return None


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

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

(or at least update the docstring)

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.

nedbat · 2021-03-22T20:19:30Z

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


This surely is not what you meant!

Suggested change

assert len(result) == len(result)

assert len(result) == len(users)

Sorry for that and thanks for catching it.

nedbat

GitHub says I have to leave a comment explaining the changes, so I will point you to all of the comments I left explaining changes :)

edx-status-bot · 2021-03-23T06:48:34Z

Your PR has finished running tests. There were no failures.

shimulch · 2021-03-23T07:32:06Z

@nedbat Thanks for the review. I've addressed your comments. :-)

edx-pipeline-bot · 2021-03-25T18:24:02Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-03-25T18:50:43Z

EdX Release Notice: This PR has been deployed to the production environment.

edx-pipeline-bot · 2021-03-25T19:06:35Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-03-25T19:31:38Z

EdX Release Notice: This PR has been deployed to the production environment.

edx-pipeline-bot · 2021-03-25T19:31:39Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Dec 10, 2020

shimulch mentioned this pull request Dec 12, 2020

[BD-24] [TNL-7330] [BB-2726] LTI Improvements - Implement Context Membership Service openedx/xblock-lti-consumer#124

Closed

2 tasks

giovannicimolin suggested changes Dec 14, 2020

View reviewed changes

giovannicimolin suggested changes Dec 18, 2020

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Dec 20, 2020

bradenmacdonald reviewed Jan 8, 2021

View reviewed changes

shimulch force-pushed the shimulch/bb-2726-batch-external-id branch from 489a9fc to b7951e3 Compare January 12, 2021 06:50

giovannicimolin mentioned this pull request Feb 17, 2021

[BD-24][TNL-7919] LTI Advantage Names and Roles provisioning service implementation direction openedx/xblock-lti-consumer#142

Merged

natabene changed the title ~~[BB-2726] [TNL-7330] Added batch_get_or_create class method for ExternalId~~ [BD-24] [BB-2726] [TNL-7330] Added batch_get_or_create class method for ExternalId Mar 11, 2021

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage and removed open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 11, 2021

shimulch force-pushed the shimulch/bb-2726-batch-external-id branch from b7951e3 to 8654d80 Compare March 11, 2021 14:33

shimulch mentioned this pull request Mar 12, 2021

[BD-24] [BB-2726] [TNL-7330] Added Course Membership API function #25843

Merged

2 tasks

shimulch force-pushed the shimulch/bb-2726-batch-external-id branch from 4556a0d to e0785b0 Compare March 12, 2021 05:46

giovannicimolin suggested changes Mar 15, 2021

View reviewed changes

Added batch_get_or_create_user_ids method for ExternalId Model

8db49de

Update doc string

shimulch force-pushed the shimulch/bb-2726-batch-external-id branch from 6c6b020 to 8db49de Compare March 18, 2021 06:45

giovannicimolin approved these changes Mar 18, 2021

View reviewed changes

nedbat reviewed Mar 22, 2021

View reviewed changes

nedbat suggested changes Mar 22, 2021

View reviewed changes

Update docstring & fix bug on test

f386c52

nedbat approved these changes Mar 25, 2021

View reviewed changes

nedbat merged commit eba710c into openedx:master Mar 25, 2021

openedx-webhooks added merged and removed needs triage labels Mar 25, 2021

giovannicimolin deleted the shimulch/bb-2726-batch-external-id branch March 25, 2021 16:53

	def batch_get_or_create(cls, users, type_name):
	def batch_get_or_create_user_ids(cls, users, type_name):

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

	assert len(result) == len(result)
	assert len(result) == len(users)

Uh oh!

Conversation

shimulch commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giovannicimolin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giovannicimolin Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natabene commented Dec 17, 2020

Uh oh!

giovannicimolin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giovannicimolin Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natabene commented Mar 10, 2021

Uh oh!

pomegranited commented Mar 11, 2021

Uh oh!

giovannicimolin commented Mar 11, 2021

Uh oh!

giovannicimolin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimulch commented Mar 18, 2021

Uh oh!

giovannicimolin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

shimulch commented Dec 10, 2020 •

edited

Loading

openedx-webhooks commented Dec 10, 2020 •

edited

Loading

giovannicimolin Dec 18, 2020 •

edited

Loading

giovannicimolin Dec 18, 2020 •

edited

Loading

giovannicimolin left a comment •

edited

Loading