diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index deaf47510c..7f708dfda4 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -545,6 +545,19 @@ class CourseMode(models.Model): """ return cls.PROFESSIONAL in modes_dict or cls.NO_ID_PROFESSIONAL_MODE in modes_dict + @classmethod + def contains_audit_mode(cls, modes_dict): + """ + Check whether the modes_dict contains an audit mode. + + Args: + modes_dict (dict): a dict of course modes + + Returns: + bool: whether modes_dict contains an audit mode + """ + return cls.AUDIT in modes_dict + @classmethod def is_professional_mode(cls, course_mode_tuple): """ diff --git a/lms/djangoapps/program_enrollments/api/linking.py b/lms/djangoapps/program_enrollments/api/linking.py index eb7a869475..cc17988c63 100644 --- a/lms/djangoapps/program_enrollments/api/linking.py +++ b/lms/djangoapps/program_enrollments/api/linking.py @@ -1,5 +1,5 @@ """ -Python API function to link program enrollments and external_student_keys to an +Python API function to link program enrollments and external_user_keys to an LMS user. Outside of this subpackage, import these functions @@ -12,6 +12,7 @@ import logging from django.contrib.auth import get_user_model from django.db import IntegrityError, transaction +from course_modes.models import CourseMode from student.api import get_access_role_by_role_name from student.models import CourseEnrollmentException @@ -24,16 +25,15 @@ User = get_user_model() NO_PROGRAM_ENROLLMENT_TEMPLATE = ( 'No program enrollment found for program uuid={program_uuid} and external student ' - 'key={external_student_key}' + 'key={external_user_key}' ) NO_LMS_USER_TEMPLATE = 'No user found with username {}' EXISTING_USER_TEMPLATE = ( - 'Program enrollment with external_student_key={external_student_key} is already linked to ' + 'Program enrollment with external_student_key={external_user_key} is already linked to ' '{account_relation} account username={username}' ) -@transaction.atomic def link_program_enrollments(program_uuid, external_keys_to_usernames): """ Utility function to link ProgramEnrollments to LMS Users @@ -55,11 +55,20 @@ def link_program_enrollments(program_uuid, external_keys_to_usernames): For each external_user_key:lms_username, if: - The user is not found - No enrollment is found for the given program and external_user_key - - The enrollment already has a user + - The enrollment already has a user and that user is the same as the given user An error message will be logged, and added to a dictionary of error messages keyed by external_key. The input will be skipped. All other inputs will be processed and enrollments updated, and then the function will return the dictionary of error messages. + For each external_user_key:lms_username, if the enrollment already has a user, but that user + is different than the requested user, we do the following. We unlink the existing user from + the program enrollment and link the requested user to the program enrollment. This is accomplished by + removing the existing user's link to the program enrollment. If the program enrollment + has course enrollments, then we unenroll the user. If there is an audit track in the course, + we also move the enrollment into the audit track. We also remove the association between those + course enrollments and the program course enrollments. The + requested user is then linked to the program following the above logic. + If there is an error while enrolling a user in a waiting program course enrollment, the error will be logged, and added to the returned error dictionary, and we will roll back all transactions for that user so that their db state will be the same as it was before this @@ -72,37 +81,56 @@ def link_program_enrollments(program_uuid, external_keys_to_usernames): program_uuid, external_keys_to_usernames.keys() ) users_by_username = _get_lms_users(external_keys_to_usernames.values()) - for external_student_key, username in external_keys_to_usernames.items(): - program_enrollment = program_enrollments.get(external_student_key) + for external_user_key, username in external_keys_to_usernames.items(): + program_enrollment = program_enrollments.get(external_user_key) user = users_by_username.get(username) if not user: error_message = NO_LMS_USER_TEMPLATE.format(username) elif not program_enrollment: error_message = NO_PROGRAM_ENROLLMENT_TEMPLATE.format( program_uuid=program_uuid, - external_student_key=external_student_key + external_user_key=external_user_key ) - elif program_enrollment.user: + # if we're trying to establish a link that already exists + elif program_enrollment.user and program_enrollment.user == user: error_message = _user_already_linked_message(program_enrollment, user) else: error_message = None if error_message: logger.warning(error_message) - errors[external_student_key] = error_message + errors[external_user_key] = error_message continue try: with transaction.atomic(): + # If the ProgramEnrollment already has a linked edX user that is different than + # the requested user, then we should sever the link to the existing edX user before + # linking the ProgramEnrollment to the new user. + if program_enrollment.user and program_enrollment.user != user: + message = ('Unlinking user with username={old_username} from program enrollment with ' + 'program uuid={program_uuid} with external_student_key={external_user_key} ' + 'and linking user with username={new_username} ' + 'to program enrollment.').format( + old_username=program_enrollment.user.username, + program_uuid=program_uuid, + external_user_key=external_user_key, + new_username=user, + ) + logger.info(_user_already_linked_message(program_enrollment, user)) + logger.info(message) + + unlink_program_enrollment(program_enrollment) + link_program_enrollment_to_lms_user(program_enrollment, user) except (CourseEnrollmentException, IntegrityError) as e: logger.exception("Rolling back all operations for {}:{}".format( - external_student_key, + external_user_key, username, )) error_message = type(e).__name__ if str(e): error_message += ': ' error_message += str(e) - errors[external_student_key] = error_message + errors[external_user_key] = error_message return errors @@ -111,22 +139,22 @@ def _user_already_linked_message(program_enrollment, user): Creates an error message that the specified program enrollment is already linked to an lms user """ existing_username = program_enrollment.user.username - external_student_key = program_enrollment.external_user_key + external_user_key = program_enrollment.external_user_key return EXISTING_USER_TEMPLATE.format( - external_student_key=external_student_key, + external_user_key=external_user_key, account_relation='target' if program_enrollment.user.id == user.id else 'a different', username=existing_username, ) -def _get_program_enrollments_by_ext_key(program_uuid, external_student_keys): +def _get_program_enrollments_by_ext_key(program_uuid, external_user_keys): """ Does a bulk read of ProgramEnrollments for a given program and list of external student keys and returns a dict keyed by external student key """ program_enrollments = fetch_program_enrollments( program_uuid=program_uuid, - external_user_keys=external_student_keys, + external_user_keys=external_user_keys, ).prefetch_related( 'program_course_enrollments' ).select_related('user') @@ -146,6 +174,45 @@ def _get_lms_users(lms_usernames): } +def unlink_program_enrollment(program_enrollment): + """ + Unlinks CourseEnrollments from the ProgramEnrollment by doing the following for + each ProgramCourseEnrollment associated with the Program Enrollment. + 1. unenrolling the corresponding user from the course + 2. moving the user into the audit track, if the track exists + 3. removing the link between the ProgramCourseEnrollment and the CourseEnrollment + + Arguments: + program_enrollment: the ProgramEnrollment object + """ + program_course_enrollments = program_enrollment.program_course_enrollments.all() + + for pce in program_course_enrollments: + course_key = pce.course_enrollment.course.id + modes = CourseMode.modes_for_course_dict(course_key) + + update_enrollment_kwargs = { + 'is_active': False, + 'skip_refund': True, + } + + if CourseMode.contains_audit_mode(modes): + # if the course contains an audit mode, move the + # learner's enrollment into the audit mode + update_enrollment_kwargs['mode'] = 'audit' + + # deactive the learner's course enrollment and move them into the + # audit track, if it exists + pce.course_enrollment.update_enrollment(**update_enrollment_kwargs) + + # sever ties to the user from the ProgramCourseEnrollment + pce.course_enrollment = None + pce.save() + + program_enrollment.user = None + program_enrollment.save() + + def link_program_enrollment_to_lms_user(program_enrollment, user): """ Attempts to link the given program enrollment to the given user diff --git a/lms/djangoapps/program_enrollments/api/tests/test_linking.py b/lms/djangoapps/program_enrollments/api/tests/test_linking.py index dedef68504..c71d692e02 100644 --- a/lms/djangoapps/program_enrollments/api/tests/test_linking.py +++ b/lms/djangoapps/program_enrollments/api/tests/test_linking.py @@ -5,6 +5,7 @@ Tests for account linking Python API. from uuid import uuid4 +from unittest.mock import patch from django.test import TestCase from edx_django_utils.cache import RequestCache from opaque_keys.edx.keys import CourseKey @@ -103,7 +104,7 @@ class TestLinkProgramEnrollmentsMixin(object): def _assert_user_enrolled_in_program_courses(self, user, program_uuid, *course_keys): """ - Assert that the given user is has active enrollments in the given courses + Assert that the given user has active enrollments in the given courses through the given program. """ user.refresh_from_db() @@ -256,6 +257,86 @@ class TestLinkProgramEnrollments(TestLinkProgramEnrollmentsMixin, TestCase): # assert that all CourseAccessRoleAssignment objects are deleted assert not active_enrollment_1.courseaccessroleassignment_set.all().exists() + @staticmethod + def _assert_course_enrollments_in_mode(course_enrollments, course_keys_to_mode): + """ + Assert that all program course enrollments are in the correct modes as + described by course_keys_to_mode. + + Arguments: + user: the user whose course enrollments we are checking + program_uuid: the UUID of the program in which the user is enrolled + course_keys_to_mode: a mapping from course keys to the the mode + slug the user's enrollment should be in + """ + assert len(course_enrollments) == len(course_keys_to_mode) + + for course_enrollment in course_enrollments: + assert course_enrollment.mode == course_keys_to_mode[course_enrollment.course.id] + + @patch('lms.djangoapps.program_enrollments.api.linking.CourseMode.modes_for_course_dict') + def test_update_linking_enrollment_to_another_user(self, mock_modes_for_course_dict): + """ + Test that when link_program_enrollments is called with a program and an external_user_key, + user pair and that program is already linked to a different user with the same external_user_key + that the original user's link is removed and replaced by a link with the new user. + """ + program_enrollment = self._create_waiting_enrollment(self.program, '0001') + + self._create_waiting_course_enrollment(program_enrollment, self.fruit_course) + self._create_waiting_course_enrollment(program_enrollment, self.animal_course) + + # in order to test what happens to a learner's enrollment in a course without an audit mode + # (e.g. Master's only course), we need to mock out the course modes that exist for our courses; + # doing it this way helps to avoid needing to use the modulestore when using the CourseModeFactory + def mocked_modes_for_course_dict(course_key): + if course_key == self.animal_course: + return {'masters': 'masters'} + else: + return {'audit': 'audit'} + + mock_modes_for_course_dict.side_effect = mocked_modes_for_course_dict + + # do the initial link of user_1 to the program enrollment + link_program_enrollments(self.program, {'0001': self.user_1.username}) + + self._assert_program_enrollment(self.user_1, self.program, '0001', refresh=False) + self._assert_no_program_enrollment(self.user_2, self.program, refresh=False) + + # grab the user's original course enrollment before the link between the program + # and the course enrollments is severed + course_enrollments_for_user_1 = [pce.course_enrollment + for pce + in program_enrollment.program_course_enrollments.all()] + + errors = link_program_enrollments( + self.program, + { + '0001': self.user_2.username, + } + ) + + assert errors == {} + self._assert_program_enrollment(self.user_2, self.program, '0001') + self._assert_no_program_enrollment(self.user_1, self.program) + # assert that all of user_1's course enrollments as part of the program + # are inactive + for course_enrollment in course_enrollments_for_user_1: + course_enrollment.refresh_from_db() + assert not course_enrollment.is_active + + # assert that user_1's course enrollments are in the expected mode + # after unlinking + course_keys_to_mode = { + self.fruit_course: 'audit', + self.animal_course: 'masters', + } + self._assert_course_enrollments_in_mode(course_enrollments_for_user_1, course_keys_to_mode) + + # assert that user_2 has been successfully linked to the program + self._assert_program_enrollment(self.user_2, self.program, '0001') + self._assert_user_enrolled_in_program_courses(self.user_2, self.program, self.fruit_course, self.animal_course) + class TestLinkProgramEnrollmentsErrors(TestLinkProgramEnrollmentsMixin, TestCase): """ Tests for linking error behavior """ @@ -285,7 +366,7 @@ class TestLinkProgramEnrollmentsErrors(TestLinkProgramEnrollmentsMixin, TestCase ) expected_error_msg = NO_PROGRAM_ENROLLMENT_TEMPLATE.format( program_uuid=self.program, - external_student_key='0002' + external_user_key='0002' ) logger.check_present((LOG_PATH, 'WARNING', expected_error_msg)) @@ -337,34 +418,6 @@ class TestLinkProgramEnrollmentsErrors(TestLinkProgramEnrollmentsMixin, TestCase self._assert_program_enrollment(self.user_1, self.program, '0001') self._assert_program_enrollment(self.user_2, self.program, '0002') - def test_enrollment_already_linked_to_different_user(self): - self._create_waiting_enrollment(self.program, '0001') - enrollment = ProgramEnrollmentFactory.create( - program_uuid=self.program, - external_user_key='0003', - ) - user_3 = enrollment.user - - self._assert_no_program_enrollment(self.user_1, self.program, refresh=False) - self._assert_no_program_enrollment(self.user_2, self.program, refresh=False) - self._assert_program_enrollment(user_3, self.program, '0003', refresh=False) - - with LogCapture() as logger: - errors = link_program_enrollments( - self.program, - { - '0001': self.user_1.username, - '0003': self.user_2.username, - } - ) - expected_error_msg = _user_already_linked_message(enrollment, self.user_2) - logger.check_present((LOG_PATH, 'WARNING', expected_error_msg)) - - self.assertDictEqual(errors, {'0003': expected_error_msg}) - self._assert_program_enrollment(self.user_1, self.program, '0001') - self._assert_no_program_enrollment(self.user_2, self.program) - self._assert_program_enrollment(user_3, self.program, '0003') - def test_error_enrolling_in_course(self): nonexistant_course = CourseKey.from_string('course-v1:edX+Zilch+Bupkis') diff --git a/lms/djangoapps/program_enrollments/api/writing.py b/lms/djangoapps/program_enrollments/api/writing.py index 17963803be..e138a69e24 100644 --- a/lms/djangoapps/program_enrollments/api/writing.py +++ b/lms/djangoapps/program_enrollments/api/writing.py @@ -382,7 +382,7 @@ def enroll_in_masters_track(user, course_key, status): Arguments: user (User) course_key (CourseKey|str) - status (str): from ProgramCourseEnrollmenStatuses + status (str): from ProgramCourseEnrollmentStatuses Returns: CourseEnrollment @@ -390,7 +390,7 @@ def enroll_in_masters_track(user, course_key, status): """ _ensure_course_exists(course_key, user.id) if status not in ProgramCourseEnrollmentStatuses.__ALL__: - raise ValueError("invalid ProgramCourseEnrollmenStatus: {}".format(status)) + raise ValueError("invalid ProgramCourseEnrollmentStatus: {}".format(status)) if CourseEnrollment.is_enrolled(user, course_key): course_enrollment = CourseEnrollment.objects.get( user=user, diff --git a/lms/djangoapps/program_enrollments/models.py b/lms/djangoapps/program_enrollments/models.py index 7aca257591..134a3df6a8 100644 --- a/lms/djangoapps/program_enrollments/models.py +++ b/lms/djangoapps/program_enrollments/models.py @@ -105,11 +105,6 @@ class ProgramCourseEnrollment(TimeStampedModel): # For each program enrollment, there may be only one # waiting program-course enrollment per course key. - # This same constraint is implicitly enforced for - # completed program-course enrollments by the - # OneToOneField on `course_enrollment`, which mandates that - # there may be at most one program-course enrollment per - # (user, course) pair. unique_together = ( ('program_enrollment', 'course_key'), ) diff --git a/lms/djangoapps/support/tests/test_views.py b/lms/djangoapps/support/tests/test_views.py index 04d2dcd91b..c1aacadbec 100644 --- a/lms/djangoapps/support/tests/test_views.py +++ b/lms/djangoapps/support/tests/test_views.py @@ -550,6 +550,74 @@ class SupportViewLinkProgramEnrollmentsTests(SupportViewTestCase): render_call_dict = mocked_render.call_args[0][1] assert render_call_dict['errors'] == [msg] + def _setup_user_from_username(self, username): + """ + Setup a user from the passed in username. + If username passed in is falsy, return None + """ + created_user = None + if username: + created_user = UserFactory(username=username, password=self.PASSWORD) + return created_user + + def _setup_enrollments(self, external_user_key, linked_user=None): + """ + Create enrollments for testing linking. + The enrollments can be create with already linked edX user. + """ + program_enrollment = ProgramEnrollmentFactory.create( + external_user_key=external_user_key, + program_uuid=self.program_uuid, + user=linked_user + ) + course_enrollment = None + if linked_user: + course_enrollment = CourseEnrollmentFactory.create( + course_id=self.course.id, + user=linked_user, + mode=CourseMode.MASTERS, + is_active=True + ) + program_course_enrollment = ProgramCourseEnrollmentFactory.create( + program_enrollment=program_enrollment, + course_key=self.course.id, + course_enrollment=course_enrollment, + status='active' + ) + + return program_enrollment, program_course_enrollment + + @ddt.data( + ('', None), + ('linked_user', None), + ('linked_user', 'original_user') + ) + @ddt.unpack + @patch_render + def test_linking_program_enrollment(self, username, original_username, mocked_render): + external_user_key = '0001' + linked_user = self._setup_user_from_username(username) + original_user = self._setup_user_from_username(original_username) + program_enrollment, program_course_enrollment = self._setup_enrollments( + external_user_key, + original_user + ) + self.client.post(self.url, data={ + 'program_uuid': self.program_uuid, + 'text': external_user_key + ',' + username + }) + render_call_dict = mocked_render.call_args[0][1] + if username: + expected_success = "('{}', '{}')".format(external_user_key, username) + assert render_call_dict['successes'] == [expected_success] + program_enrollment.refresh_from_db() + assert program_enrollment.user == linked_user + program_course_enrollment.refresh_from_db() + assert program_course_enrollment.course_enrollment.user == linked_user + else: + error = u"All linking lines must be in the format 'external_user_key,lms_username'" + assert render_call_dict['errors'] == [error] + @ddt.ddt class ProgramEnrollmentsInspectorViewTests(SupportViewTestCase):