-
Notifications
You must be signed in to change notification settings - Fork 2
fix: Fix LTI based duplicate email users #3098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
OpenAPI ChangesShow/hide ## Changes for v0.yaml:Unexpected changes? Ensure your branch is up-to-date with |
04f78d7 to
bbb9f0c
Compare
0cae285 to
4dabaaf
Compare
| drf-extensions = "^0.8.0" | ||
| drf-spectacular = "^0.28.0" | ||
| edx-api-client = "^1.13.0" | ||
| edx-api-client = { git = "https://github.com/mitodl/edx-api-client", branch = "arslan/8853-fix-lti-dup-users" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be replaced by an actual pin once mitodl/edx-api-client#133 is merged and released.
rhysyngsun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment on the order of operations
openedx/api.py
Outdated
| else: | ||
|
|
||
| # Only try for LTI user duplicate email error fix if the response error was duplicate-email | ||
| if _is_duplicate_email_error(resp, data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go earlier before the username collision handling so we can ideally still try to use the original username
openedx/api.py
Outdated
| ) | ||
| # If the LTI API fixed the duplicate email issue, Retry account creation | ||
| if dup_email_fix_resp.status_code == status.HTTP_200_OK: | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the potential to loop infinitely if the API responds with a 200 response but for some reason the next request still fails with the same error, we should set a flag here that we're retrying and if we hit this block again it should stop trying to create the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the operation order change have been incorporated in 4152ba6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unable to run this locally due to this error with the dependency definition:
40.49 - Installing edx-api-client (1.13.0 624c7fe)
41.37
41.37 | Failed to clone https://github.com/mitodl/edx-api-client at '624c7fe864d7f2df487ef6a0d670881ead44e113', verify ref exists on remote.
41.37 |
41.37 | Note: This error arises from interacting with the specified vcs source and is likely not a Poetry issue.
41.37 | This issue could be caused by any of the following;
41.37 |
41.37 | - there are network issues in this environment
41.37 | - the revision (624c7fe864d7f2df487ef6a0d670881ead44e113) you have specified
41.37 | - was misspelled
41.37 | - is invalid (must be a sha or symref)
41.37 | - is not present on remote
41.37 |
41.37 | You can also run your poetry command with -v to see more information.
I tried running poetry lock which usually fixes this kind of thing. I see that the github actions python tests are hitting it too so it's not just my environment.
3231441 to
b75efd1
Compare
b75efd1 to
4152ba6
Compare
I squashed commits in mitodl/edx-api-client#133 and the commit id mismatched, This should be fixed as well. |
What are the relevant tickets?
https://github.com/mitodl/hq/issues/8853
Description (What does it do?)
Screenshots (if appropriate):
How can this be tested?
Pre-Reqs
Testing Steps
something@lti_example.comand the user is moved to the retirement pipeline. The previous social auth entry should be deleted if existeddup-emailerror stateAdditional Context
This is part of a Multi PR fix and you will need to set up all three to test this end-to-end with mitodl/open-edx-plugins#652, mitodl/edx-api-client#133