Skip to content
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

Resolve issue when repairing user's edx synchronised records #1496

Conversation

collinpreston
Copy link
Contributor

@collinpreston collinpreston commented Mar 17, 2023

Pre-Flight checklist

  • Migrations
    • Migration is backwards-compatible with current production code
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

#1493

What's this PR do?

An issue occurs when we attempt to repair mitxonline User's edx OAuth accounts. This issue occurs when a user meets the following criteria:

  • User has an OpenEdxUser record in mitxonline.
  • No user records exist for the user in Edx.

How should this be manually tested?

  1. Have mitxonline and devstack up and running locally, and connected.
  2. You will need to run a makemigrations in mitxonline.
  3. Create a user through the mitxonline UI.
  4. Delete the user's record, created during step 2, from devstack: http://localhost:18000/admin/auth/user/
  5. From the Django shell, run:
from users.models import User
from openedx import api
user = User.objects.get(id=<USER_ID_FROM_STEP_2>)
result = api.repair_faulty_edx_user(user)
  1. Verify that a user record is created in devstack: http://localhost:18000/admin/auth/user/
  2. Verify that the OpenEdxUser record in mitxonline is marked as synced: http://mitxonline.odl.local:8013/admin/openedx/openedxuser/

Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

This seems to work if I remove the OpenEdxUser record but it doesn't if it's still there. Line 314 in openedx/api.py looks for that record so it seems like it it's skipping the call to regen the auth tokens?

@collinpreston
Copy link
Contributor Author

This seems to work if I remove the OpenEdxUser record but it doesn't if it's still there. Line 314 in openedx/api.py looks for that record so it seems like it it's skipping the call to regen the auth tokens?

Nice find. I updated the PR to now make an API call after retrieving the existing token: client.user_info.get_user_info() . An Exception should be thrown in the try if the user either doesn't have a valid OpenEdxApiAuth record and edx responds to the creation of a new OpenEdxApiAuth record with an error since the user does not exist, or if the user has a valid OpenEdxApiAuth then edx will respond to a call to get_user_info() with an Exception.

@collinpreston collinpreston requested a review from jkachel March 20, 2023 18:13
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2023

Codecov Report

Merging #1496 (fe1146f) into main (5bf0db9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1496      +/-   ##
==========================================
+ Coverage   82.92%   82.93%   +0.01%     
==========================================
  Files         313      313              
  Lines       13443    13451       +8     
  Branches      954      953       -1     
==========================================
+ Hits        11147    11155       +8     
- Misses       2027     2029       +2     
+ Partials      269      267       -2     
Impacted Files Coverage Δ
openedx/models.py 90.47% <ø> (ø)
openedx/api.py 82.05% <100.00%> (+0.14%) ⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

Tried repairing in the way that I was and was successful. LGTM! 👍

@collinpreston collinpreston merged commit 8a52e71 into main Mar 20, 2023
@collinpreston collinpreston deleted the 1493-openedxoauth2error-redirected-to-httpscoursesmitxonlinemiteduregister-expected-httpsmitxonlinemited branch March 20, 2023 18:44
This was referenced Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants