From cfbfe359046f530241a498921836e8fd6a73476d Mon Sep 17 00:00:00 2001 From: Matt Hughes Date: Tue, 29 Oct 2019 14:56:49 -0400 Subject: [PATCH] Link program enrollments on social auth updates in add. to creation See lms/djangoapps/program_enrollments/docs/decisions/0002-matriculation-on-user_social_auth_change.rst for more details JIRA:EDUCATOR-4770 --- ...triculation-on-user_social_auth_change.rst | 60 +++++++++++++++++++ lms/djangoapps/program_enrollments/signals.py | 3 - .../program_enrollments/tests/test_signals.py | 24 ++++++++ 3 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 lms/djangoapps/program_enrollments/docs/decisions/0002-matriculation-on-user_social_auth_change.rst diff --git a/lms/djangoapps/program_enrollments/docs/decisions/0002-matriculation-on-user_social_auth_change.rst b/lms/djangoapps/program_enrollments/docs/decisions/0002-matriculation-on-user_social_auth_change.rst new file mode 100644 index 0000000000..98dd66e793 --- /dev/null +++ b/lms/djangoapps/program_enrollments/docs/decisions/0002-matriculation-on-user_social_auth_change.rst @@ -0,0 +1,60 @@ +Matriculation on UserSocialAuth Changes +--------------------------------------- + +Status +====== + +Accepted (circa October 2019) + + +Context +======= + +Georgia Tech has been enrolling learners into its Master's programs +for a long time, using enterprise-oriented APIs for the purpose. To +transition them to using our Master's-intended workflow, we've worked +out a plan with them to convert existing ``UserSocialAuth`` records to use +a different SAML-provided field as the canonical UID used elsewhere in +our system to identify the learner for program enrollment +purposes. Thus, we've already decided we'll need a one-time management +command to update these learners' User SocialAuth records to use the +new UID. + +The scope of this decision is a particular snag in how program +enrollments are linked to learner accounts. Namely, this linking only +occurs on the creation of a ``UserSocialAuth`` record, and not in +response to the kind of update we're proposing to perform with our +management command. + +Decisions +========= + +To address this concern and to achieve greater flexibility of program +enrollments into the future, we will attempt to link program +enrollments to learner accounts upon any change to a +``UserSocialAuth`` record, rather than solely on creation. + +Consequences +============ + +This may have a performance impact, since updates to +``UserSocialAuth`` records happen often at times which would not +benefit us. We anticipate this performance impact will be fairly +minimal given the current indices on the table, however. + +This does enable us to keep the management command simple. It also +enables us to re-trigger program enrollment linking on demand by +making trivial changes to the ``UserSocialAuth`` record, which may be +helpful for support purposes. + +Alternatives +============ + +We also considered, but rejected the following possibilities: + +1) Switch our ``post_save`` signal handler to ``pre_save``, and diff + the model fields on updates to determine whether we've changed the + UID. +2) Make the management command more complex, and have it call the + program enrollment-linking code itself rather than relying on these + signals. diff --git a/lms/djangoapps/program_enrollments/signals.py b/lms/djangoapps/program_enrollments/signals.py index bb5299984f..a0f062c56c 100644 --- a/lms/djangoapps/program_enrollments/signals.py +++ b/lms/djangoapps/program_enrollments/signals.py @@ -33,9 +33,6 @@ def listen_for_social_auth_creation(sender, instance, created, **kwargs): # pyl """ Post-save signal that will attempt to link a social auth entry with waiting enrollments """ - if not created: - return - try: matriculate_learner(instance.user, instance.uid) except Exception as e: diff --git a/lms/djangoapps/program_enrollments/tests/test_signals.py b/lms/djangoapps/program_enrollments/tests/test_signals.py index f4bf0799af..19152ce7ea 100644 --- a/lms/djangoapps/program_enrollments/tests/test_signals.py +++ b/lms/djangoapps/program_enrollments/tests/test_signals.py @@ -171,6 +171,30 @@ class SocialAuthEnrollmentCompletionSignalTest(CacheIsolationTestCase): self.assertEqual(student_course_enrollment.course.id, program_course_enrollment.course_key) self.assertEqual(student_course_enrollment.mode, mode) + def test_update_social_auth(self): + """ + Makes sure we can update a social_auth row to trigger the same program enrollments + """ + program_enrollment = self._create_waiting_program_enrollment() + program_course_enrollments = self._create_waiting_course_enrollments(program_enrollment) + + user_social_auth = UserSocialAuth.objects.create( + user=self.user, + uid='{0}:{1}'.format(self.provider_slug, 'gobbledegook') + ) + + # Not yet a thing, didn't match + program_enrollment.refresh_from_db() + self.assertIsNone(program_enrollment.user) + + user_social_auth.uid = '{0}:{1}'.format(self.provider_slug, self.external_id) + user_social_auth.save() + + # now we see the enrollments realized + self._assert_program_enrollment_user(program_enrollment, self.user) + for program_course_enrollment in program_course_enrollments: + self._assert_program_course_enrollment(program_course_enrollment) + def test_waiting_course_enrollments_completed(self): program_enrollment = self._create_waiting_program_enrollment() program_course_enrollments = self._create_waiting_course_enrollments(program_enrollment)