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

Add lms_user_id by default to the user model of new cookiecutter IDAs #281

Open
pshiu opened this issue Dec 21, 2022 · 4 comments
Open

Add lms_user_id by default to the user model of new cookiecutter IDAs #281

pshiu opened this issue Dec 21, 2022 · 4 comments
Assignees

Comments

@pshiu
Copy link
Contributor

pshiu commented Dec 21, 2022

Description

OEP-32 standardizes the lms_user_id as the canonical identifier of a user in Open edX.

However, the lms_user_id is currently inaccessible from the default edx-cookiecutter IDA.

This issue is to add something like edx/program-intent-engagement@4de0f0d to edx-cookiecutter so JWT_PAYLOAD_USER_ATTRIBUTE_MAPPING loads the lms_user_id of incoming users into the Django User model of new IDAs built from edx-cookiecutters.

Acceptance Criteria

Additional Information

Notes for ADR from 2022-12-21 Arch Hour
  • Consensus:
    • Let’s add the lms_user_id in by default: PR + ADR
    • Let’s consider in the future how to reduce the number of identifiers, especially considering future efforts of unifying identity at 2U
      • Enterprise may have a model for this in how they stub users if they are added to subscriptions before they exist in the LMS.
  • Raw discussion notes:
    • Confusion about canonical user identifiers - LMS user ID
    • Pie or Exams do this thing about auto-adding LMS user ID - should we add this to the cookie cutter? Should new services automatically have the LMS user ID in their user model?
    • Well, maybe not all of them need it… but many may eventually need it?
    • Side note: Maybe we could set the id of the user in the new service to be the same as the lms_user_id?
      • What about conflicts?
    • We should have docs in the cookiecutter about this information
    • On the older services we didn’t have this for a long time. We were re-using an assorted variety of user identifiers across services. Users were and many times still are being created in LMS by different services.
    • Ecommerce was one of the first repos where we were trying to get the lms_user_id holistically added to all calls to/from the repo & LMS
    • Does Enterprise has any use cases of user imports?
      • Enterprise has a stub record we create if a user doesn’t pre-exist in LMS
    • Makes sense to have lms_user_id in the user model. Maybe a future thought is to reduce our total number of ids.
    • In the LMS, we do have the concept of external IDs.
    • We have global identity as well.
    • Maybe we have options to map it in the future.
@pshiu pshiu self-assigned this Dec 21, 2022
@robrap
Copy link
Contributor

robrap commented Dec 21, 2022

Thanks @pshiu. Looks great. I'm going to make one edit to the ADR note.

@robrap
Copy link
Contributor

robrap commented Dec 21, 2022

I made a small modification to the ADR note, but will add more thoughts here instead:

  1. The reason we aren't going further right now is because it would be large effort with unknown consequences. Are there still cases where we create the new user and don't yet have the lms user id?
  2. I like using "Deferred/Rejected Alternatives" as a section title for ideas that were temporarily rejected, but might be considered in the future.

Also, I'm wondering if the cookiecutter needs a doc that summarizes decisions, or "what you get with the cookiecutter", and potentially explains alternatives? May not be necessary for this, but just a thought.

@robrap
Copy link
Contributor

robrap commented Dec 21, 2022

@pshiu: Have you seen this ADR? https://github.com/openedx/ecommerce/blob/master/docs/decisions/0004-unique-identifier-for-users.rst. It is also relying on social auth to get LMS user ids in certain cases. I'm not sure if that still applies, and how it would relate to cookie-cutter. I also remember that code being messier than it probably needs to be.

@pshiu
Copy link
Contributor Author

pshiu commented Dec 21, 2022

@robrap Very interesting! I should have been aware, I suppose, as a maintainer of ecommerce. Looks like we use add_lms_user_id() when ecommerce receives requests on behalf of other users but doesn't know their lms_user_id. A good case to add in the ADR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants