From b041a1f3e6fd04023906161c245499af021df809 Mon Sep 17 00:00:00 2001 From: Zach Hancock Date: Tue, 7 May 2019 17:47:57 -0400 Subject: [PATCH] complete waiting enrollments --- .../api/v1/tests/test_views.py | 44 ++++- .../program_enrollments/api/v1/views.py | 3 +- lms/djangoapps/program_enrollments/apps.py | 6 + lms/djangoapps/program_enrollments/models.py | 47 +++-- lms/djangoapps/program_enrollments/signals.py | 50 +++++ .../{api/v1 => }/tests/factories.py | 0 .../program_enrollments/tests/test_models.py | 68 ++++++- .../program_enrollments/tests/test_signals.py | 176 ++++++++++++++++++ 8 files changed, 367 insertions(+), 27 deletions(-) create mode 100644 lms/djangoapps/program_enrollments/signals.py rename lms/djangoapps/program_enrollments/{api/v1 => }/tests/factories.py (100%) create mode 100644 lms/djangoapps/program_enrollments/tests/test_signals.py diff --git a/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py b/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py index ae869642a9..20d079f9cb 100644 --- a/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py +++ b/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py @@ -16,10 +16,11 @@ from rest_framework import status from rest_framework.test import APITestCase from six import text_type +from course_modes.models import CourseMode from lms.djangoapps.courseware.tests.factories import GlobalStaffFactory from lms.djangoapps.program_enrollments.api.v1.constants import CourseEnrollmentResponseStatuses as CourseStatuses -from lms.djangoapps.program_enrollments.models import ProgramEnrollment, ProgramCourseEnrollment -from student.tests.factories import UserFactory, CourseEnrollmentFactory +from lms.djangoapps.program_enrollments.models import ProgramCourseEnrollment, ProgramEnrollment +from lms.djangoapps.program_enrollments.tests.factories import ProgramCourseEnrollmentFactory, ProgramEnrollmentFactory from openedx.core.djangoapps.catalog.cache import PROGRAM_CACHE_KEY_TPL from openedx.core.djangoapps.catalog.tests.factories import ( CourseFactory, @@ -28,7 +29,7 @@ from openedx.core.djangoapps.catalog.tests.factories import ( ) from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangolib.testing.utils import CacheIsolationMixin -from .factories import ProgramCourseEnrollmentFactory, ProgramEnrollmentFactory +from student.tests.factories import UserFactory, CourseEnrollmentFactory class ListViewTestMixin(object): @@ -310,6 +311,7 @@ class BaseCourseEnrollmentTestsMixin(ProgramCacheTestCaseMixin): course_enrollment = CourseEnrollmentFactory.create( course_id=self.course_key, user=program_enrollment.user, + mode=CourseMode.MASTERS ) course_enrollment.is_active = course_status == "active" course_enrollment.save() @@ -324,7 +326,7 @@ class BaseCourseEnrollmentTestsMixin(ProgramCacheTestCaseMixin): program_enrollment = self.create_program_enrollment(external_user_key, user) return self.create_program_course_enrollment(program_enrollment, course_status=course_status) - def assert_program_course_enrollment(self, external_user_key, expected_status, has_user): + def assert_program_course_enrollment(self, external_user_key, expected_status, has_user, mode=CourseMode.MASTERS): """ Convenience method to assert that a ProgramCourseEnrollment exists, and potentially that a CourseEnrollment also exists @@ -340,6 +342,7 @@ class BaseCourseEnrollmentTestsMixin(ProgramCacheTestCaseMixin): self.assertIsNotNone(course_enrollment) self.assertEqual(expected_status == "active", course_enrollment.is_active) self.assertEqual(self.course_key, course_enrollment.course_id) + self.assertEqual(mode, course_enrollment.mode) else: self.assertIsNone(course_enrollment) @@ -452,13 +455,44 @@ class CourseEnrollmentPostTests(BaseCourseEnrollmentTestsMixin, APITestCase): self.assert_program_course_enrollment("learner-3", "active", False) self.assert_program_course_enrollment("learner-4", "inactive", False) - def test_user_already_enrolled_in_course(self): + def test_program_course_enrollment_exists(self): + """ + The program enrollments application already has a program_course_enrollment + record for this user and course + """ self.create_program_and_course_enrollments('learner-1') post_data = [self.learner_enrollment("learner-1")] response = self.request(self.default_url, post_data) self.assertEqual(422, response.status_code) self.assertDictEqual({'learner-1': CourseStatuses.CONFLICT}, response.data) + def test_user_currently_enrolled_in_course(self): + """ + If a user is already enrolled in a course through a different method + that enrollment should be linked but not overwritten as masters. + """ + CourseEnrollmentFactory.create( + course_id=self.course_key, + user=self.student, + mode=CourseMode.VERIFIED + ) + + self.create_program_enrollment('learner-1', user=self.student) + + post_data = [ + self.learner_enrollment("learner-1", "active") + ] + response = self.request(self.default_url, post_data) + + self.assertEqual(200, response.status_code) + self.assertDictEqual( + { + "learner-1": "active" + }, + response.data + ) + self.assert_program_course_enrollment("learner-1", "active", True, mode=CourseMode.VERIFIED) + def test_207_multistatus(self): self.create_program_enrollment('learner-1') post_data = [self.learner_enrollment("learner-1"), self.learner_enrollment("learner-2")] diff --git a/lms/djangoapps/program_enrollments/api/v1/views.py b/lms/djangoapps/program_enrollments/api/v1/views.py index 89f5350994..970205450e 100644 --- a/lms/djangoapps/program_enrollments/api/v1/views.py +++ b/lms/djangoapps/program_enrollments/api/v1/views.py @@ -560,7 +560,8 @@ class ProgramCourseEnrollmentsView(DeveloperErrorViewMixin, ProgramCourseRunSpec """ if program_course_enrollment: return CourseEnrollmentResponseStatuses.CONFLICT - return ProgramCourseEnrollment.enroll( + + return ProgramCourseEnrollment.create_program_course_enrollment( program_enrollment, self.course_key, enrollment_request['status'] diff --git a/lms/djangoapps/program_enrollments/apps.py b/lms/djangoapps/program_enrollments/apps.py index 7cdabf684a..2ec8923c19 100644 --- a/lms/djangoapps/program_enrollments/apps.py +++ b/lms/djangoapps/program_enrollments/apps.py @@ -24,3 +24,9 @@ class ProgramEnrollmentsConfig(AppConfig): } }, } + + def ready(self): + """ + Connect handlers to signals. + """ + from . import signals # pylint: disable=unused-variable diff --git a/lms/djangoapps/program_enrollments/models.py b/lms/djangoapps/program_enrollments/models.py index 896a13f5bc..05698f915c 100644 --- a/lms/djangoapps/program_enrollments/models.py +++ b/lms/djangoapps/program_enrollments/models.py @@ -14,7 +14,7 @@ from lms.djangoapps.program_enrollments.api.v1.constants import CourseEnrollment from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import CourseKeyField from simple_history.models import HistoricalRecords -from student.models import CourseEnrollment as StudentCourseEnrollment +from student.models import AlreadyEnrolledError, CourseEnrollment as StudentCourseEnrollment logger = logging.getLogger(__name__) # pylint: disable=invalid-name @@ -143,27 +143,19 @@ class ProgramCourseEnrollment(TimeStampedModel): # pylint: disable=model-missin return '[ProgramCourseEnrollment id={}]'.format(self.id) @classmethod - def enroll(cls, program_enrollment, course_key, status): + def create_program_course_enrollment(cls, program_enrollment, course_key, status): """ Create ProgramCourseEnrollment for the given course and program enrollment """ - course_enrollment = None - if program_enrollment.user: - course_enrollment = StudentCourseEnrollment.enroll( - program_enrollment.user, - course_key, - mode=CourseMode.MASTERS, - check_access=True, - ) - if status == CourseEnrollmentResponseStatuses.INACTIVE: - course_enrollment.deactivate() - program_course_enrollment = ProgramCourseEnrollment.objects.create( program_enrollment=program_enrollment, - course_enrollment=course_enrollment, course_key=course_key, status=status, ) + + if program_enrollment.user: + program_course_enrollment.enroll(program_enrollment.user) + return program_course_enrollment.status def change_status(self, status): @@ -196,3 +188,30 @@ class ProgramCourseEnrollment(TimeStampedModel): # pylint: disable=model-missin )) self.save() return self.status + + def enroll(self, user): + """ + Create a StudentCourseEnrollment to enroll user in course + """ + try: + self.course_enrollment = StudentCourseEnrollment.enroll( + user, + self.course_key, + mode=CourseMode.MASTERS, + check_access=True, + ) + except AlreadyEnrolledError: + course_enrollment = StudentCourseEnrollment.objects.get( + user=user, + course_id=self.course_key, + ) + if course_enrollment.mode == CourseMode.AUDIT or course_enrollment.mode == CourseMode.HONOR: + course_enrollment.mode = CourseMode.MASTERS + course_enrollment.save() + self.course_enrollment = course_enrollment + message = ("Attempted to create course enrollment for user={user} and course={course}" + " but an enrollment already exists. Existing enrollment will be used instead") + logger.info(message.format(user=user.id, course=self.course_key)) + if self.status == CourseEnrollmentResponseStatuses.INACTIVE: + self.course_enrollment.deactivate() + self.save() diff --git a/lms/djangoapps/program_enrollments/signals.py b/lms/djangoapps/program_enrollments/signals.py new file mode 100644 index 0000000000..f24bf84269 --- /dev/null +++ b/lms/djangoapps/program_enrollments/signals.py @@ -0,0 +1,50 @@ +""" +Signal handlers for program enrollments +""" +import logging +from django.db.models.signals import post_save +from django.dispatch import receiver +from social_django.models import UserSocialAuth +from student.models import CourseEnrollmentException +from third_party_auth.models import SAMLProviderConfig + +from lms.djangoapps.program_enrollments.models import ProgramEnrollment + +logger = logging.getLogger(__name__) + + +@receiver(post_save, sender=UserSocialAuth) +def martriculate_learner(sender, instance, **kwargs): # pylint: disable=unused-argument + """ + Post-save signal to update any waiting program enrollments with a user, + and enroll the user in any waiting course enrollments. + + In most cases this will just short-circuit. Enrollments will only be updated + for a SAML provider with a linked organization. + """ + try: + user = instance.user + provider_slug, external_user_key = instance.uid.split(':') + if not SAMLProviderConfig.objects.get(slug=provider_slug).organization: + return + except (AttributeError, ValueError, SAMLProviderConfig.DoesNotExist): + return + + incomplete_enrollments = ProgramEnrollment.objects.filter( + external_user_key=external_user_key + ).prefetch_related('program_course_enrollments') + + incomplete_enrollments.update(user=user) + + for enrollment in incomplete_enrollments: + for program_course_enrollment in enrollment.program_course_enrollments.all(): + try: + program_course_enrollment.enroll(user) + except CourseEnrollmentException as e: + logger.warning( + u'Failed to enroll user=%s with waiting program_course_enrollment=%s: %s', + user.id, + program_course_enrollment.id, + e, + ) + raise e diff --git a/lms/djangoapps/program_enrollments/api/v1/tests/factories.py b/lms/djangoapps/program_enrollments/tests/factories.py similarity index 100% rename from lms/djangoapps/program_enrollments/api/v1/tests/factories.py rename to lms/djangoapps/program_enrollments/tests/factories.py diff --git a/lms/djangoapps/program_enrollments/tests/test_models.py b/lms/djangoapps/program_enrollments/tests/test_models.py index 19572d6c46..8a51dd41ae 100644 --- a/lms/djangoapps/program_enrollments/tests/test_models.py +++ b/lms/djangoapps/program_enrollments/tests/test_models.py @@ -6,12 +6,17 @@ from __future__ import unicode_literals from uuid import uuid4 from testfixtures import LogCapture +import ddt from django.test import TestCase from opaque_keys.edx.keys import CourseKey +from course_modes.models import CourseMode +from edx_django_utils.cache import RequestCache from lms.djangoapps.program_enrollments.models import ProgramEnrollment, ProgramCourseEnrollment +from student.models import CourseEnrollment from student.tests.factories import UserFactory, CourseEnrollmentFactory from openedx.core.djangoapps.catalog.tests.factories import generate_course_run_key +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory class ProgramEnrollmentModelTests(TestCase): @@ -102,6 +107,7 @@ class ProgramEnrollmentModelTests(TestCase): self.assertEquals(record.external_user_key, None) +@ddt.ddt class ProgramCourseEnrollmentModelTests(TestCase): """ Tests for the ProgramCourseEnrollment model. @@ -111,6 +117,7 @@ class ProgramCourseEnrollmentModelTests(TestCase): Set up test data """ super(ProgramCourseEnrollmentModelTests, self).setUp() + RequestCache.clear_all_namespaces() self.user = UserFactory.create() self.program_uuid = uuid4() self.program_enrollment = ProgramEnrollment.objects.create( @@ -121,21 +128,37 @@ class ProgramCourseEnrollmentModelTests(TestCase): status='enrolled' ) self.course_key = CourseKey.from_string(generate_course_run_key()) - self.course_enrollment = CourseEnrollmentFactory.create( + CourseOverviewFactory(id=self.course_key) + + def _create_completed_program_course_enrollment(self): + """ helper function create program course enrollment """ + course_enrollment = CourseEnrollmentFactory.create( course_id=self.course_key, user=self.user, + mode=CourseMode.MASTERS ) - self.program_course_enrollment = ProgramCourseEnrollment.objects.create( + program_course_enrollment = ProgramCourseEnrollment.objects.create( program_enrollment=self.program_enrollment, course_key=self.course_key, - course_enrollment=self.course_enrollment, + course_enrollment=course_enrollment, + status="active" + ) + return program_course_enrollment + + def _create_waiting_program_course_enrollment(self): + """ helper function create program course enrollment with no lms user """ + return ProgramCourseEnrollment.objects.create( + program_enrollment=self.program_enrollment, + course_key=self.course_key, + course_enrollment=None, status="active" ) def test_change_status_no_enrollment(self): + program_course_enrollment = self._create_completed_program_course_enrollment() with LogCapture() as capture: - self.program_course_enrollment.course_enrollment = None - self.program_course_enrollment.change_status("inactive") + program_course_enrollment.course_enrollment = None + program_course_enrollment.change_status("inactive") expected_message = "User {} {} {} has no course_enrollment".format( self.user, self.program_enrollment, @@ -146,12 +169,43 @@ class ProgramCourseEnrollmentModelTests(TestCase): ) def test_change_status_not_active_or_inactive(self): + program_course_enrollment = self._create_completed_program_course_enrollment() with LogCapture() as capture: status = "potential-future-status-0123" - self.program_course_enrollment.change_status(status) + program_course_enrollment.change_status(status) message = ("Changed {} status to {}, not changing course_enrollment" " status because status is not 'active' or 'inactive'") - expected_message = message.format(self.program_course_enrollment, status) + expected_message = message.format(program_course_enrollment, status) capture.check( ('lms.djangoapps.program_enrollments.models', 'WARNING', expected_message) ) + + def test_enroll_new_course_enrollment(self): + program_course_enrollment = self._create_waiting_program_course_enrollment() + program_course_enrollment.enroll(self.user) + + course_enrollment = CourseEnrollment.objects.get(user=self.user, course_id=self.course_key) + self.assertEqual(course_enrollment.user, self.user) + self.assertEqual(course_enrollment.course.id, self.course_key) + self.assertEqual(course_enrollment.mode, CourseMode.MASTERS) + + @ddt.data( + (CourseMode.VERIFIED, CourseMode.VERIFIED), + (CourseMode.AUDIT, CourseMode.MASTERS), + (CourseMode.HONOR, CourseMode.MASTERS) + ) + @ddt.unpack + def test_enroll_existing_course_enrollment(self, original_mode, result_mode): + course_enrollment = CourseEnrollmentFactory.create( + course_id=self.course_key, + user=self.user, + mode=original_mode + ) + program_course_enrollment = self._create_waiting_program_course_enrollment() + + program_course_enrollment.enroll(self.user) + + course_enrollment = CourseEnrollment.objects.get(user=self.user, course_id=self.course_key) + self.assertEqual(course_enrollment.user, self.user) + self.assertEqual(course_enrollment.course.id, self.course_key) + self.assertEqual(course_enrollment.mode, result_mode) diff --git a/lms/djangoapps/program_enrollments/tests/test_signals.py b/lms/djangoapps/program_enrollments/tests/test_signals.py new file mode 100644 index 0000000000..63b05efdb5 --- /dev/null +++ b/lms/djangoapps/program_enrollments/tests/test_signals.py @@ -0,0 +1,176 @@ +""" +Unit tests for completing program course enrollments +once a social auth entry for the user is created. +""" +from django.test import TestCase +import mock +from opaque_keys.edx.keys import CourseKey +import pytest +from social_django.models import UserSocialAuth +from testfixtures import LogCapture + +from course_modes.models import CourseMode +from edx_django_utils.cache import RequestCache +from lms.djangoapps.program_enrollments.signals import logger +from lms.djangoapps.program_enrollments.tests.factories import ProgramCourseEnrollmentFactory, ProgramEnrollmentFactory +from organizations.tests.factories import OrganizationFactory +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory +from student.models import CourseEnrollmentException +from student.tests.factories import CourseEnrollmentFactory, UserFactory +from third_party_auth.tests.factories import SAMLProviderConfigFactory + + +class SocialAuthEnrollmentCompletionSignalTest(TestCase): + """ + Test post-save handler on UserSocialAuth + """ + + def setUp(self): + super(SocialAuthEnrollmentCompletionSignalTest, self).setUp() + RequestCache.clear_all_namespaces() + + @classmethod + def setUpClass(cls): + super(SocialAuthEnrollmentCompletionSignalTest, cls).setUpClass() + + cls.external_id = '0000' + cls.provider_slug = 'uox' + cls.course_keys = [ + CourseKey.from_string('course-v1:edX+DemoX+Test_Course'), + CourseKey.from_string('course-v1:edX+DemoX+Another_Test_Course'), + ] + cls.organization = OrganizationFactory.create() + cls.user = UserFactory.create() + + for course_key in cls.course_keys: + CourseOverviewFactory(id=course_key) + SAMLProviderConfigFactory.create(organization=cls.organization, slug=cls.provider_slug) + + def _create_waiting_program_enrollment(self): + """ helper method to create a waiting program enrollment """ + return ProgramEnrollmentFactory.create( + user=None, + external_user_key=self.external_id + ) + + def _create_waiting_course_enrollments(self, program_enrollment): + """ helper method to create waiting course enrollments """ + return [ + ProgramCourseEnrollmentFactory( + program_enrollment=program_enrollment, + course_enrollment=None, + course_key=course_key + ) + for course_key in self.course_keys + ] + + def _assert_program_enrollment_user(self, program_enrollment): + """ validate program enrollment has a user """ + program_enrollment.refresh_from_db() + self.assertEqual(program_enrollment.user, self.user) + + def _assert_program_course_enrollment(self, program_course_enrollment, mode=CourseMode.MASTERS): + """ validate program course enrollment has a valid course enrollment """ + program_course_enrollment.refresh_from_db() + student_course_enrollment = program_course_enrollment.course_enrollment + self.assertEqual(student_course_enrollment.user, self.user) + self.assertEqual(student_course_enrollment.course.id, program_course_enrollment.course_key) + self.assertEqual(student_course_enrollment.mode, mode) + + def test_waiting_course_enrollments_completed(self): + program_enrollment = self._create_waiting_program_enrollment() + program_course_enrollments = self._create_waiting_course_enrollments(program_enrollment) + + UserSocialAuth.objects.create( + user=self.user, + uid='{0}:{1}'.format(self.provider_slug, self.external_id) + ) + + self._assert_program_enrollment_user(program_enrollment) + for program_course_enrollment in program_course_enrollments: + self._assert_program_course_enrollment(program_course_enrollment) + + def test_learner_already_enrolled_in_course(self): + course_key = self.course_keys[0] + course = CourseOverview.objects.get(id=course_key) + CourseEnrollmentFactory(user=self.user, course=course, mode=CourseMode.VERIFIED) + + program_enrollment = self._create_waiting_program_enrollment() + program_course_enrollments = self._create_waiting_course_enrollments(program_enrollment) + + UserSocialAuth.objects.create( + user=self.user, + uid='{0}:{1}'.format(self.provider_slug, self.external_id) + ) + + self._assert_program_enrollment_user(program_enrollment) + + duplicate_program_course_enrollment = program_course_enrollments[0] + self._assert_program_course_enrollment(duplicate_program_course_enrollment, CourseMode.VERIFIED) + + program_course_enrollment = program_course_enrollments[1] + self._assert_program_course_enrollment(program_course_enrollment) + + def test_enrolled_with_no_course_enrollments(self): + program_enrollment = self._create_waiting_program_enrollment() + + UserSocialAuth.objects.create( + user=self.user, + uid='{0}:{1}'.format(self.provider_slug, self.external_id) + ) + + self._assert_program_enrollment_user(program_enrollment) + + def test_create_social_auth_with_no_waiting_enrollments(self): + """ + No exceptions should be raised if there are no enrollments to update + """ + UserSocialAuth.objects.create( + user=self.user, + uid='{0}:{1}'.format(self.provider_slug, self.external_id) + ) + + def test_create_social_auth_provider_has_no_organization(self): + """ + No exceptions should be raised if provider is not linked to an organization + """ + provider = SAMLProviderConfigFactory.create() + UserSocialAuth.objects.create( + user=self.user, + uid='{0}:{1}'.format(provider.slug, self.external_id) + ) + + def test_create_social_auth_non_saml_provider(self): + """ + No exceptions should be raised for a non-SAML uid or if a SAML provider cannot be found + """ + UserSocialAuth.objects.create( + user=self.user, + uid='google-oauth-user@gmail.com' + ) + UserSocialAuth.objects.create( + user=self.user, + uid='123:123:123' + ) + + def test_log_on_enrollment_failure(self): + program_enrollment = self._create_waiting_program_enrollment() + program_course_enrollments = self._create_waiting_course_enrollments(program_enrollment) + + with mock.patch('student.models.CourseEnrollment.enroll') as enrollMock: + enrollMock.side_effect = CourseEnrollmentException('something has gone wrong') + with LogCapture(logger.name) as log: + with pytest.raises(CourseEnrollmentException): + UserSocialAuth.objects.create( + user=self.user, + uid='{0}:{1}'.format(self.provider_slug, self.external_id) + ) + error_tmpl = u'Failed to enroll user={} with waiting program_course_enrollment={}: {}' + log.check_present( + ( + logger.name, + 'WARNING', + error_tmpl.format(self.user.id, program_course_enrollments[0].id, 'something has gone wrong') + ) + )