search

openedx / edx-cookiecutters

Open edx public templates for apps, libraries and services.
Apache License 2.0
26 stars 22 forks source link

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

Open pshiu opened 1 year ago

pshiu commented 1 year ago

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 https://github.com/edx/program-intent-engagement/commit/4de0f0db375fd5ce47ea568a0c7c3157193c8ccf 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.
robrap commented 1 year ago

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

robrap commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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.