diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c77c86867..c28ba1bb71 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,6 @@ on: push: branches: [master] pull_request: - branches: [master] concurrency: group: ci-${{ github.event.pull_request.number || github.ref }} diff --git a/.github/workflows/mysql8-migrations.yml b/.github/workflows/mysql8-migrations.yml index b42ee35537..8e2ed8af9c 100644 --- a/.github/workflows/mysql8-migrations.yml +++ b/.github/workflows/mysql8-migrations.yml @@ -56,7 +56,7 @@ jobs: pip uninstall -y mysqlclient pip install --no-binary mysqlclient mysqlclient pip uninstall -y xmlsec - pip install --no-binary xmlsec xmlsec + pip install --no-binary xmlsec xmlsec==1.3.13 pip install backports.zoneinfo - name: Initiate Services run: | diff --git a/enterprise/admin/__init__.py b/enterprise/admin/__init__.py index 665745c1d0..2b2a3958ec 100644 --- a/enterprise/admin/__init__.py +++ b/enterprise/admin/__init__.py @@ -202,7 +202,7 @@ class EnterpriseCustomerAdmin(DjangoObjectActions, SimpleHistoryAdmin): ('Integration and learning platform settings', { 'fields': ('enable_portal_lms_configurations_screen', 'enable_portal_saml_configuration_screen', 'enable_slug_login', 'replace_sensitive_sso_username', 'hide_course_original_price', - 'enable_generation_of_api_credentials') + 'enable_generation_of_api_credentials', 'allow_enrollment_in_invite_only_courses') }), ('Recommended default settings for all enterprise customers', { 'fields': ('site', 'customer_type', 'enable_learner_portal', diff --git a/enterprise/api/v1/views/enterprise_customer.py b/enterprise/api/v1/views/enterprise_customer.py index 1685304d17..3ce2a692ca 100644 --- a/enterprise/api/v1/views/enterprise_customer.py +++ b/enterprise/api/v1/views/enterprise_customer.py @@ -38,6 +38,7 @@ from enterprise.utils import ( enroll_subsidy_users_in_courses, get_best_mode_from_course_key, + get_course_details_from_course_keys, track_enrollment, validate_email_to_link, ) @@ -241,6 +242,8 @@ def enroll_learners_in_courses(self, request, pk): for course_run in course_runs_modes: course_runs_modes[course_run] = get_best_mode_from_course_key(course_run) + course_details = get_course_details_from_course_keys(course_runs_modes.keys()) + emails = set() for info in enrollments_info: @@ -254,6 +257,7 @@ def enroll_learners_in_courses(self, request, pk): else: emails.add(info['email']) info['course_mode'] = course_runs_modes[info['course_run_key']] + info['invitation_only'] = course_details[info['course_run_key']].invitation_only for email in emails: try: diff --git a/enterprise/api_client/lms.py b/enterprise/api_client/lms.py index 47e08edb49..169b9ad359 100644 --- a/enterprise/api_client/lms.py +++ b/enterprise/api_client/lms.py @@ -6,6 +6,7 @@ import time from urllib.parse import urljoin +import requests from opaque_keys.edx.keys import CourseKey from requests.exceptions import ( # pylint: disable=redefined-builtin ConnectionError, @@ -274,6 +275,34 @@ def get_enrolled_courses(self, username): response.raise_for_status() return response.json() + def allow_enrollment(self, email, course_id, auto_enroll=False): + """ + Call the enrollment API to allow enrollment for the given email and course_id. + + Args: + email (str): The email address of the user to be allowed to enroll in the course. + course_id (str): The string value of the course's unique identifier. + auto_enroll (bool): Whether to auto-enroll the user in the course upon registration / activation. + + Returns: + dict: A dictionary containing details of the created CourseEnrollmentAllowed object. + + """ + api_url = self.get_api_url("enrollment_allowed") + response = self.client.post( + f"{api_url}/", + json={ + 'email': email, + 'course_id': course_id, + 'auto_enroll': auto_enroll, + } + ) + if response.status_code == requests.codes.conflict: + LOGGER.info(response.json()["message"]) + else: + response.raise_for_status() + return response.json() + class CourseApiClient(NoAuthAPIClient): """ diff --git a/enterprise/utils.py b/enterprise/utils.py index 32770d8b52..8b25aa1849 100644 --- a/enterprise/utils.py +++ b/enterprise/utils.py @@ -58,6 +58,11 @@ CourseUserGroup = None CourseEnrollmentError = None +try: + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +except ImportError: + CourseOverview = None + try: from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollmentAllowed @@ -2028,6 +2033,7 @@ def enroll_subsidy_users_in_courses(enterprise_customer, subsidy_users_info, dis user_id = subsidy_user_info.get('user_id') user_email = subsidy_user_info['email'].strip().lower() if 'email' in subsidy_user_info else None course_mode = subsidy_user_info.get('course_mode') + invitation_only = subsidy_user_info.get('invitation_only') course_run_key = subsidy_user_info.get('course_run_key') license_uuid = subsidy_user_info.get('license_uuid') transaction_id = subsidy_user_info.get('transaction_id') @@ -2054,6 +2060,12 @@ def enroll_subsidy_users_in_courses(enterprise_customer, subsidy_users_info, dis enrollment_source = enterprise_enrollment_source_model().get_source( enterprise_enrollment_source_model().CUSTOMER_ADMIN ) + if invitation_only and enterprise_customer.allow_enrollment_in_invite_only_courses: + CourseEnrollmentAllowed.objects.update_or_create( + course_id=course_run_key, + email=user.email, + ) + succeeded, created, source_uuid = customer_admin_enroll_user_with_status( enterprise_customer, user, @@ -2291,6 +2303,14 @@ def get_best_mode_from_course_key(course_key): return CourseModes.AUDIT +def get_course_details_from_course_keys(course_keys): + """ + Helper to get a mapping of course keys to course details. + """ + course_overviews = CourseOverview.objects.filter(id__in=course_keys) + return {str(course_overview.id): course_overview for course_overview in course_overviews} + + def parse_lms_api_datetime(datetime_string, datetime_format=LMS_API_DATETIME_FORMAT): """ Parse a received datetime into a timezone-aware, Python datetime object. @@ -2387,19 +2407,14 @@ def truncate_string(string, max_length=MAX_ALLOWED_TEXT_LENGTH): def ensure_course_enrollment_is_allowed(course_id, email, enrollment_api_client): """ - Create a CourseEnrollmentAllowed object for invitation-only courses. + Calls the enrollment API to create a CourseEnrollmentAllowed object for + invitation-only courses. Arguments: course_id (str): ID of the course to allow enrollment email (str): email of the user whose enrollment should be allowed enrollment_api_client (:class:`enterprise.api_client.lms.EnrollmentApiClient`): Enrollment API Client """ - if not CourseEnrollmentAllowed: - raise NotConnectedToOpenEdX() - course_details = enrollment_api_client.get_course_details(course_id) if course_details["invite_only"]: - CourseEnrollmentAllowed.objects.update_or_create( - course_id=course_id, - email=email, - ) + enrollment_api_client.allow_enrollment(email, course_id) diff --git a/enterprise/views.py b/enterprise/views.py index 1086fdb4d8..8029e62501 100644 --- a/enterprise/views.py +++ b/enterprise/views.py @@ -683,6 +683,15 @@ def _enroll_learner_in_course( existing_enrollment.get('mode') == constants.CourseModes.AUDIT or existing_enrollment.get('is_active') is False ): + if enterprise_customer.allow_enrollment_in_invite_only_courses: + ensure_course_enrollment_is_allowed(course_id, request.user.email, enrollment_api_client) + LOGGER.info( + 'User {user} is allowed to enroll in Course {course_id}.'.format( + user=request.user.username, + course_id=course_id + ) + ) + course_mode = get_best_mode_from_course_key(course_id) LOGGER.info( 'Retrieved Course Mode: {course_modes} for Course {course_id}'.format( diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 0b0c68a216..631465641d 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -4437,6 +4437,7 @@ def tearDown(self): }, ) @ddt.unpack + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys') @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') @mock.patch('enterprise.api.v1.views.enterprise_customer.track_enrollment') @mock.patch("enterprise.models.EnterpriseCustomer.notify_enrolled_learners") @@ -4445,6 +4446,7 @@ def test_bulk_enrollment_in_bulk_courses_pending_licenses( mock_notify_task, mock_track_enroll, mock_get_course_mode, + mock_get_course_details, body, expected_code, expected_response, @@ -4464,6 +4466,7 @@ def test_bulk_enrollment_in_bulk_courses_pending_licenses( permission = Permission.objects.get(name='Can add Enterprise Customer') self.user.user_permissions.add(permission) mock_get_course_mode.return_value = VERIFIED_SUBSCRIPTION_COURSE_MODE + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = False self.assertEqual(len(PendingEnrollment.objects.all()), 0) response = self.client.post( @@ -4488,6 +4491,7 @@ def test_bulk_enrollment_in_bulk_courses_pending_licenses( # no notifications to be sent unless 'notify' specifically asked for in payload mock_notify_task.assert_not_called() + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys') @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') @mock.patch('enterprise.api.v1.views.enterprise_customer.track_enrollment') @mock.patch('enterprise.models.EnterpriseCustomer.notify_enrolled_learners') @@ -4498,6 +4502,7 @@ def test_bulk_enrollment_in_bulk_courses_existing_users( mock_notify_task, mock_track_enroll, mock_get_course_mode, + mock_get_course_details, ): """ Tests the bulk enrollment endpoint at enroll_learners_in_courses. @@ -4505,6 +4510,7 @@ def test_bulk_enrollment_in_bulk_courses_existing_users( This tests the case where existing users are supplied, so the enrollments are fulfilled rather than pending. """ mock_update_or_create_enrollment.return_value = True + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = False user_one = factories.UserFactory(is_active=True) user_two = factories.UserFactory(is_active=True) @@ -4575,6 +4581,7 @@ def test_bulk_enrollment_in_bulk_courses_existing_users( assert mock_update_or_create_enrollment.call_count == 2 + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys') @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') @mock.patch('enterprise.api.v1.views.enterprise_customer.track_enrollment') @mock.patch('enterprise.models.EnterpriseCustomer.notify_enrolled_learners') @@ -4583,12 +4590,15 @@ def test_bulk_enrollment_in_bulk_courses_nonexisting_user_id( mock_notify_task, mock_track_enroll, mock_get_course_mode, + mock_get_course_details, ): """ Tests the bulk enrollment endpoint at enroll_learners_in_courses. This tests the case where a non-existent user_id is supplied, so an error should occur. """ + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = False + user = factories.UserFactory(is_active=True) factories.EnterpriseCustomerFactory( @@ -4647,6 +4657,7 @@ def test_bulk_enrollment_in_bulk_courses_nonexisting_user_id( }, ) @ddt.unpack + @mock.patch("enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys") @mock.patch("enterprise.api.v1.views.enterprise_subsidy_fulfillment.enrollment_api") @mock.patch( 'enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key' @@ -4657,6 +4668,7 @@ def test_bulk_enrollment_enroll_after_cancel( mock_platform_enrollment, mock_get_course_mode, mock_update_or_create_enrollment, + mock_get_course_details, old_transaction_id, new_transaction_id, ): @@ -4666,6 +4678,7 @@ def test_bulk_enrollment_enroll_after_cancel( """ mock_platform_enrollment.return_value = True mock_get_course_mode.return_value = VERIFIED_SUBSCRIPTION_COURSE_MODE + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = False # Needed for the cancel endpoint: mock_update_or_create_enrollment.update_enrollment.return_value = mock.Mock() @@ -4768,12 +4781,14 @@ def test_bulk_enrollment_enroll_after_cancel( }, ) @ddt.unpack + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys') @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') @mock.patch('enterprise.utils.lms_update_or_create_enrollment') def test_bulk_enrollment_includes_fulfillment_source_uuid( self, mock_get_course_mode, mock_update_or_create_enrollment, + mock_get_course_details, body, fulfillment_source, ): @@ -4782,6 +4797,7 @@ def test_bulk_enrollment_includes_fulfillment_source_uuid( generated subsidized enrollment uuid value as part of the response payload. """ mock_update_or_create_enrollment.return_value = True + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = False user, _, enterprise_customer = self._create_user_and_enterprise_customer( body.get('enrollments_info')[0].get('email'), 'test_password' @@ -4878,6 +4894,7 @@ def test_bulk_enrollment_includes_fulfillment_source_uuid( }, ) @ddt.unpack + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys') @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') @mock.patch('enterprise.api.v1.views.enterprise_customer.track_enrollment') @mock.patch("enterprise.models.EnterpriseCustomer.notify_enrolled_learners") @@ -4886,6 +4903,7 @@ def test_bulk_enrollment_with_notification( mock_notify_task, mock_track_enroll, mock_get_course_mode, + mock_get_course_details, body, expected_code, expected_response, @@ -4905,6 +4923,7 @@ def test_bulk_enrollment_with_notification( permission = Permission.objects.get(name='Can add Enterprise Customer') self.user.user_permissions.add(permission) mock_get_course_mode.return_value = VERIFIED_SUBSCRIPTION_COURSE_MODE + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = False self.assertEqual(len(PendingEnrollment.objects.all()), 0) @@ -4951,12 +4970,72 @@ def _make_call(course_run, enrolled_learners): mock_notify_task.assert_has_calls(mock_calls, any_order=True) + @mock.patch('enterprise.utils.CourseEnrollmentAllowed') + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys') + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') + @mock.patch('enterprise.utils.lms_update_or_create_enrollment') + def test_bulk_enrollment_invitation_only( + self, + mock_update_or_create_enrollment, + mock_get_course_mode, + mock_get_course_details, + mock_cea, + ): + """ + Tests that bulk enrollment endpoint creates CourseEnrollmentAllowed object when enterprise customer allows + enrollment in invitation only courses and the course is invitation only. + """ + mock_update_or_create_enrollment.return_value = True + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = True + mock_get_course_mode.return_value = VERIFIED_SUBSCRIPTION_COURSE_MODE + + user, _, enterprise_customer = self._create_user_and_enterprise_customer("abc@test.com", "test_password") + course_id = 'course-v1:edX+DemoX+Demo_Course' + body = { + 'enrollments_info': [ + { + 'user_id': user.id, + 'course_run_key': course_id, + 'license_uuid': '5a88bdcade7c4ecb838f8111b68e18ac' + }, + ] + } + + def enroll(): + self.client.post( + settings.TEST_SERVER + reverse( + 'enterprise-customer-enroll-learners-in-courses', (enterprise_customer.uuid,) + ), + data=json.dumps(body), + content_type='application/json', + ) + + enroll() + mock_cea.objects.update_or_create.assert_not_called() + + enterprise_customer.allow_enrollment_in_invite_only_courses = True + enterprise_customer.save() + + enroll() + mock_cea.objects.update_or_create.assert_called_with( + course_id=course_id, + email=user.email + ) + + @mock.patch('enterprise.api.v1.views.enterprise_customer.get_course_details_from_course_keys') @mock.patch('enterprise.api.v1.views.enterprise_customer.enroll_subsidy_users_in_courses') @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') - def test_enroll_learners_in_courses_partial_failure(self, mock_get_course_mode, mock_enroll_user): + def test_enroll_learners_in_courses_partial_failure( + self, + mock_get_course_mode, + mock_enroll_user, + mock_get_course_details, + ): """ Tests that bulk users bulk enrollment endpoint properly handles partial failures. """ + mock_get_course_details.return_value.__getitem__.return_value.invitation_only = True + ent_customer = factories.EnterpriseCustomerFactory( uuid=FAKE_UUIDS[0], name="test_enterprise" diff --git a/tests/test_enterprise/test_utils.py b/tests/test_enterprise/test_utils.py index be10f1ede4..b304e60230 100644 --- a/tests/test_enterprise/test_utils.py +++ b/tests/test_enterprise/test_utils.py @@ -539,10 +539,9 @@ def test_truncate_string(self): self.assertEqual(len(truncated_string), MAX_ALLOWED_TEXT_LENGTH) @ddt.data(True, False) - @mock.patch("enterprise.utils.CourseEnrollmentAllowed") - def test_ensure_course_enrollment_is_allowed(self, invite_only, mock_cea): + def test_ensure_course_enrollment_is_allowed(self, invite_only): """ - Test that the CourseEnrollmentAllowed is created only for the "invite_only" courses. + Test that the enrollment allow endpoint is called for the "invite_only" courses. """ self.create_user() mock_enrollment_api = mock.Mock() @@ -551,9 +550,9 @@ def test_ensure_course_enrollment_is_allowed(self, invite_only, mock_cea): ensure_course_enrollment_is_allowed("test-course-id", self.user.email, mock_enrollment_api) if invite_only: - mock_cea.objects.update_or_create.assert_called_with( - course_id="test-course-id", - email=self.user.email + mock_enrollment_api.allow_enrollment.assert_called_with( + self.user.email, + "test-course-id", ) else: - mock_cea.objects.update_or_create.assert_not_called() + mock_enrollment_api.allow_enrollment.assert_not_called() diff --git a/tests/test_enterprise/views/test_course_enrollment_view.py b/tests/test_enterprise/views/test_course_enrollment_view.py index 8ed1819d5a..5ff637f2df 100644 --- a/tests/test_enterprise/views/test_course_enrollment_view.py +++ b/tests/test_enterprise/views/test_course_enrollment_view.py @@ -1623,10 +1623,8 @@ def test_post_course_specific_enrollment_view_premium_mode( @mock.patch('enterprise.views.EnrollmentApiClient') @mock.patch('enterprise.views.get_data_sharing_consent') @mock.patch('enterprise.utils.Registry') - @mock.patch('enterprise.utils.CourseEnrollmentAllowed') def test_post_course_specific_enrollment_view_invite_only_courses( self, - mock_cea, registry_mock, get_data_sharing_consent_mock, enrollment_api_client_mock, @@ -1664,9 +1662,9 @@ def test_post_course_specific_enrollment_view_invite_only_courses( } ) - mock_cea.objects.update_or_create.assert_called_with( - course_id=course_id, - email=self.user.email + enrollment_api_client_mock.return_value.allow_enrollment.assert_called_with( + self.user.email, + course_id, ) assert response.status_code == 302 diff --git a/tests/test_enterprise/views/test_grant_data_sharing_permissions.py b/tests/test_enterprise/views/test_grant_data_sharing_permissions.py index 642a9bcd8e..bbb339b01f 100644 --- a/tests/test_enterprise/views/test_grant_data_sharing_permissions.py +++ b/tests/test_enterprise/views/test_grant_data_sharing_permissions.py @@ -90,6 +90,30 @@ def _assert_enterprise_linking_messages(self, response, user_is_active=True): 'You will not be able to log back into your account until you have activated it.' ) + def _assert_allow_enrollment_is_called_correctly( + self, + mock_enrollment_api_client, + license_is_present, + course_invite_only, + enrollment_in_invite_only_courses_allowed, + ): + """ + Verify that the allow_enrollment endpoint is called only when: + - License is present + - Course is invite only + - Enrollment in invite only courses is allowed + """ + if license_is_present: + if course_invite_only: + if enrollment_in_invite_only_courses_allowed: + mock_enrollment_api_client.return_value.allow_enrollment.assert_called_once() + else: + mock_enrollment_api_client.return_value.allow_enrollment.assert_not_called() + else: + mock_enrollment_api_client.return_value.allow_enrollment.assert_not_called() + else: + mock_enrollment_api_client.return_value.allow_enrollment.assert_not_called() + @mock.patch('enterprise.views.render', side_effect=fake_render) @mock.patch('enterprise.models.EnterpriseCatalogApiClient') @mock.patch('enterprise.api_client.discovery.CourseCatalogApiServiceClient') @@ -398,12 +422,21 @@ def test_get_course_specific_consent_not_needed( @mock.patch('enterprise.views.get_best_mode_from_course_key') @mock.patch('enterprise.api_client.discovery.CourseCatalogApiServiceClient') @ddt.data( - str(uuid.uuid4()), - '', + (str(uuid.uuid4()), True, True), + (str(uuid.uuid4()), True, False), + (str(uuid.uuid4()), False, True), + (str(uuid.uuid4()), False, False), + ('', True, True), + ('', True, False), + ('', False, True), + ('', False, False), ) + @ddt.unpack def test_get_course_specific_data_sharing_consent_not_enabled( self, license_uuid, + course_invite_only, + allow_enrollment_in_invite_only_courses, course_catalog_api_client_mock, mock_get_course_mode, mock_enrollment_api_client, @@ -414,6 +447,7 @@ def test_get_course_specific_data_sharing_consent_not_enabled( enterprise_customer = EnterpriseCustomerFactory( name='Starfleet Academy', enable_data_sharing_consent=False, + allow_enrollment_in_invite_only_courses=allow_enrollment_in_invite_only_courses, ) content_filter = { 'key': [ @@ -432,6 +466,8 @@ def test_get_course_specific_data_sharing_consent_not_enabled( course_catalog_api_client_mock.return_value.program_exists.return_value = True course_catalog_api_client_mock.return_value.get_course_id.return_value = course_id + mock_enrollment_api_client.return_value.get_course_details.return_value = {"invite_only": course_invite_only} + course_mode = 'verified' mock_get_course_mode.return_value = course_mode mock_enrollment_api_client.return_value.get_course_enrollment.return_value = { @@ -467,6 +503,13 @@ def test_get_course_specific_data_sharing_consent_not_enabled( else: assert not mock_enrollment_api_client.return_value.enroll_user_in_course.called + self._assert_allow_enrollment_is_called_correctly( + mock_enrollment_api_client, + bool(license_uuid), + course_invite_only, + allow_enrollment_in_invite_only_courses + ) + @mock.patch('enterprise.views.render', side_effect=fake_render) @mock.patch('enterprise.views.get_best_mode_from_course_key') @mock.patch('enterprise.models.EnterpriseCatalogApiClient')