From d77191a3b5903ab2efa606eba028df093ef021d0 Mon Sep 17 00:00:00 2001 From: Demid Date: Mon, 12 Aug 2024 04:58:06 +0300 Subject: [PATCH] fix: create CEA object when enrolling using a license flow (#18) * fix: create CEA object when enrolling using a license flow * test: verify that allow_enrollment is called_correctly * fix: xmlsec issue https://github.com/xmlsec/python-xmlsec/issues/314 * build: enable CI for pull requests * style: fix some pycodestyle issues --- .github/workflows/ci.yml | 1 - .github/workflows/mysql8-migrations.yml | 2 +- enterprise/api_client/lms.py | 29 ++++++++++++ enterprise/utils.py | 11 ++--- enterprise/views.py | 9 ++++ tests/test_enterprise/api/test_views.py | 2 +- tests/test_enterprise/test_utils.py | 13 +++-- .../views/test_course_enrollment_view.py | 8 ++-- .../test_grant_data_sharing_permissions.py | 47 ++++++++++++++++++- 9 files changed, 97 insertions(+), 25 deletions(-) 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/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 99debbc724..8b25aa1849 100644 --- a/enterprise/utils.py +++ b/enterprise/utils.py @@ -2407,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 fa380974d3..631465641d 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -4512,7 +4512,6 @@ def test_bulk_enrollment_in_bulk_courses_existing_users( 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) @@ -5001,6 +5000,7 @@ def test_bulk_enrollment_invitation_only( }, ] } + def enroll(): self.client.post( settings.TEST_SERVER + reverse( 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')