-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add LTI utilities app (Bad user fix) #652
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
Conversation
8d5774d to
f8d68a8
Compare
3e94a09 to
3a4deee
Compare
abddeff to
624c7fe
Compare
e9082e8 to
bfd54c3
Compare
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
| try: | ||
| create_retirement_request_and_deactivate_account(user) | ||
| except Exception as e: # noqa: BLE001 | ||
| log.error("Error retiring and deactivating user: %s", e) # noqa: TRY400 |
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.
If we get an error here, shouldn't we respond with an error?
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.
So I added it as an additional step and I explicitly didn't return error to MITx Online if retirement fails. That's because we would have already updated the email of the old user at this point so MITx Online registration can be retried. Let me know if we should link it to the error response we send to MITx Online.
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.
Ok, we can leave it as is and if we need more information passed back to mitxonline later we can circle back on it.
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.
LGTM
bfd54c3 to
2c686f6
Compare
ac0be76 to
2c686f6
Compare
What are the relevant tickets?
https://github.com/mitodl/hq/issues/8853
Description (What does it do?)
emailaddress as a POST parameter and tries to identify if the user was created via LTI and is in a bad stateusernamein the edX users table. As long as both of these values are the same, we are sure that the user was created automatically via Canvas/LTIHow can this be tested?
FEATURES["ENABLE_LTI_PROVIDER"] = True'lms.djangoapps.lti_provider') in INSTALLED_APPS inlms/envs/common.pye.gSCRIPT TO SET UP DUMMY LTI USER
In your LMS Django admin, use this script to create a dummy LTI user:
Additional Context
Should be tested along with mitodl/mitxonline#3098 and mitodl/edx-api-client#133