From 3efb0808388af287f3f1b4a027574826be2e3501 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Mon, 25 Sep 2023 11:36:06 +0500 Subject: [PATCH] feat: force all enrollments (#2763) * feat: force all enrollments * linting * fix test --- courses/api.py | 15 +++------------ courses/api_test.py | 34 +++++++++------------------------- courseware/api.py | 13 +++++-------- courseware/api_test.py | 34 ++++++++++++++-------------------- 4 files changed, 31 insertions(+), 65 deletions(-) diff --git a/courses/api.py b/courses/api.py index 485e25ee0..05439280f 100644 --- a/courses/api.py +++ b/courses/api.py @@ -9,15 +9,8 @@ from requests.exceptions import ConnectionError as RequestsConnectionError, HTTPError from courses.constants import ENROLL_CHANGE_STATUS_DEFERRED -from courses.models import ( - CourseRun, - CourseRunEnrollment, - ProgramEnrollment, -) -from courseware.api import ( - enroll_in_edx_course_runs, - unenroll_edx_course_run, -) +from courses.models import CourseRun, CourseRunEnrollment, ProgramEnrollment +from courseware.api import enroll_in_edx_course_runs, unenroll_edx_course_run from courseware.exceptions import ( EdxApiEnrollErrorException, EdxEnrollmentCreateError, @@ -100,7 +93,6 @@ def create_run_enrollments( keep_failed_enrollments=False, order=None, company=None, - force_enrollment=False, ): # pylint: disable=too-many-arguments """ Creates local records of a user's enrollment in course runs, and attempts to enroll them @@ -122,7 +114,7 @@ def create_run_enrollments( """ successful_enrollments = [] try: - enroll_in_edx_course_runs(user, runs, force_enrollment=force_enrollment) + enroll_in_edx_course_runs(user, runs) except ( EdxApiEnrollErrorException, UnknownEdxApiEnrollException, @@ -348,7 +340,6 @@ def defer_enrollment( order=from_enrollment.order, company=from_enrollment.company, keep_failed_enrollments=keep_failed_enrollments, - force_enrollment=force, ) if to_enrollments: from_enrollment = deactivate_run_enrollment( diff --git a/courses/api_test.py b/courses/api_test.py index 2b37af07b..5ee886805 100644 --- a/courses/api_test.py +++ b/courses/api_test.py @@ -119,8 +119,7 @@ def key_func(enrollment): @pytest.mark.django_db -@pytest.mark.parametrize("force_enrollment", [True, False]) -def test_create_run_enrollments(mocker, user, force_enrollment): +def test_create_run_enrollments(mocker, user): """ create_run_enrollments should call the edX API to create enrollments, create or reactivate local enrollment records, and notify enrolled users via email @@ -143,11 +142,9 @@ def test_create_run_enrollments(mocker, user, force_enrollment): ) successful_enrollments, edx_request_success = create_run_enrollments( - user, runs, order=order, company=company, force_enrollment=force_enrollment - ) - patched_edx_enroll.assert_called_once_with( - user, runs, force_enrollment=force_enrollment + user, runs, order=order, company=company ) + patched_edx_enroll.assert_called_once_with(user, runs) assert patched_send_enrollment_email.call_count == num_runs assert edx_request_success is True assert len(successful_enrollments) == num_runs @@ -165,8 +162,7 @@ def test_create_run_enrollments(mocker, user, force_enrollment): "exception_cls", [NoEdxApiAuthError, HTTPError, RequestsConnectionError, OpenEdXOAuth2Error], ) -@pytest.mark.parametrize("force_enrollment", [True, False]) -def test_create_run_enrollments_api_fail(mocker, user, exception_cls, force_enrollment): +def test_create_run_enrollments_api_fail(mocker, user, exception_cls): """ create_run_enrollments should log a message and still create local enrollment records when certain exceptions are raised if a flag is set to true @@ -185,11 +181,8 @@ def test_create_run_enrollments_api_fail(mocker, user, exception_cls, force_enro order=None, company=None, keep_failed_enrollments=True, - force_enrollment=force_enrollment, - ) - patched_edx_enroll.assert_called_once_with( - user, [run], force_enrollment=force_enrollment ) + patched_edx_enroll.assert_called_once_with(user, [run]) patched_log_exception.assert_called_once() patched_send_enrollment_email.assert_not_called() assert len(successful_enrollments) == 1 @@ -198,7 +191,6 @@ def test_create_run_enrollments_api_fail(mocker, user, exception_cls, force_enro @pytest.mark.django_db @pytest.mark.parametrize("keep_failed_enrollments", [True, False]) -@pytest.mark.parametrize("force_enrollment", [True, False]) @pytest.mark.parametrize( "exception_cls,inner_exception", [ @@ -210,7 +202,6 @@ def test_create_run_enrollments_enroll_api_fail( mocker, user, keep_failed_enrollments, - force_enrollment, exception_cls, inner_exception, ): # pylint: disable=too-many-arguments @@ -240,11 +231,8 @@ def test_create_run_enrollments_enroll_api_fail( order=None, company=None, keep_failed_enrollments=keep_failed_enrollments, - force_enrollment=force_enrollment, ) - patched_edx_enroll.assert_called_once_with( - user, runs, force_enrollment=force_enrollment - ) + patched_edx_enroll.assert_called_once_with(user, runs) if keep_failed_enrollments: patched_log_exception.assert_called_once() else: @@ -256,8 +244,7 @@ def test_create_run_enrollments_enroll_api_fail( @pytest.mark.django_db -@pytest.mark.parametrize("force_enrollment", [True, False]) -def test_create_run_enrollments_creation_fail(mocker, user, force_enrollment): +def test_create_run_enrollments_creation_fail(mocker, user): """ create_run_enrollments should log a message and send an admin email if there's an error during the creation of local enrollment records @@ -273,11 +260,9 @@ def test_create_run_enrollments_creation_fail(mocker, user, force_enrollment): patched_mail_api = mocker.patch("courses.api.mail_api") successful_enrollments, edx_request_success = create_run_enrollments( - user, runs, order=None, company=None, force_enrollment=force_enrollment - ) - patched_edx_enroll.assert_called_once_with( - user, runs, force_enrollment=force_enrollment + user, runs, order=None, company=None ) + patched_edx_enroll.assert_called_once_with(user, runs) patched_log_exception.assert_called_once() patched_mail_api.send_course_run_enrollment_email.assert_not_called() patched_mail_api.send_enrollment_failure_message.assert_called_once() @@ -479,7 +464,6 @@ def test_defer_enrollment(mocker, keep_failed_enrollments, force_enrollment): order=order, company=company, keep_failed_enrollments=keep_failed_enrollments, - force_enrollment=force_enrollment, ) patched_deactivate_enrollments.assert_called_once_with( existing_enrollment, diff --git a/courseware/api.py b/courseware/api.py index 0fac805ee..5f9569def 100644 --- a/courseware/api.py +++ b/courseware/api.py @@ -8,7 +8,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.core.exceptions import ImproperlyConfigured -from django.db import transaction, IntegrityError +from django.db import IntegrityError, transaction from django.shortcuts import reverse from edx_api.client import EdxApi from oauth2_provider.models import AccessToken, Application @@ -616,13 +616,14 @@ def get_enrollment(user: User, course_run: CourseRun): ) -def enroll_in_edx_course_runs(user, course_runs, force_enrollment=False): +def enroll_in_edx_course_runs(user, course_runs, force_enrollment=True): """ Enrolls a user in edx course runs Args: user (users.models.User): The user to enroll course_runs (iterable of CourseRun): The course runs to enroll in + force_enrollment (bool): Force enrollments in edX Returns: list of edx_api.enrollments.models.Enrollment: @@ -632,12 +633,8 @@ def enroll_in_edx_course_runs(user, course_runs, force_enrollment=False): EdxApiEnrollErrorException: Raised if the underlying edX API HTTP request fails UnknownEdxApiEnrollException: Raised if an unknown error was encountered during the edX API request """ - username = None - if force_enrollment: - edx_client = get_edx_api_service_client() - username = user.username - else: - edx_client = get_edx_api_client(user) + edx_client = get_edx_api_service_client() + username = user.username results = [] for course_run in course_runs: try: diff --git a/courseware/api_test.py b/courseware/api_test.py index bd7d9d46b..6add37a85 100644 --- a/courseware/api_test.py +++ b/courseware/api_test.py @@ -19,9 +19,9 @@ from courses.factories import CourseRunEnrollmentFactory, CourseRunFactory from courseware.api import ( - OpenEdxUser, ACCESS_TOKEN_HEADER_NAME, OPENEDX_AUTH_DEFAULT_TTL_IN_SECONDS, + OpenEdxUser, create_edx_auth_token, create_edx_user, create_user, @@ -417,38 +417,31 @@ def test_get_edx_api_client(mocker, settings, user): ) -@pytest.mark.parametrize("force_enrollment", [True, False]) -def test_enroll_in_edx_course_runs(mocker, user, force_enrollment): +def test_enroll_in_edx_course_runs(mocker, user): """Tests that enroll_in_edx_course_runs uses the EdxApi client to enroll in course runs""" mock_client = mocker.MagicMock() enroll_return_values = ["result1", "result2"] mock_client.enrollments.create_student_enrollment = mocker.Mock( side_effect=enroll_return_values ) - if force_enrollment: - mocker.patch( - "courseware.api.get_edx_api_service_client", return_value=mock_client - ) - else: - mocker.patch("courseware.api.get_edx_api_client", return_value=mock_client) + mocker.patch("courseware.api.get_edx_api_service_client", return_value=mock_client) course_runs = CourseRunFactory.build_batch(2) enroll_results = enroll_in_edx_course_runs( user, course_runs, - force_enrollment=force_enrollment, ) - expected_username = user.username if force_enrollment else None + expected_username = user.username mock_client.enrollments.create_student_enrollment.assert_any_call( course_runs[0].courseware_id, mode=EDX_ENROLLMENT_PRO_MODE, username=expected_username, - force_enrollment=force_enrollment, + force_enrollment=True, ) mock_client.enrollments.create_student_enrollment.assert_any_call( course_runs[1].courseware_id, mode=EDX_ENROLLMENT_PRO_MODE, username=expected_username, - force_enrollment=force_enrollment, + force_enrollment=True, ) assert enroll_results == enroll_return_values @@ -463,7 +456,7 @@ def test_enroll_in_edx_course_runs_audit(mocker, user, error_text): side_effect=[HTTPError(response=pro_enrollment_response), audit_result] ) patched_log_error = mocker.patch("courseware.api.log.error") - mocker.patch("courseware.api.get_edx_api_client", return_value=mock_client) + mocker.patch("courseware.api.get_edx_api_service_client", return_value=mock_client) course_run = CourseRunFactory.build() results = enroll_in_edx_course_runs(user, [course_run]) @@ -471,14 +464,14 @@ def test_enroll_in_edx_course_runs_audit(mocker, user, error_text): mock_client.enrollments.create_student_enrollment.assert_any_call( course_run.courseware_id, mode=EDX_ENROLLMENT_PRO_MODE, - username=None, - force_enrollment=False, + username=user.username, + force_enrollment=True, ) mock_client.enrollments.create_student_enrollment.assert_any_call( course_run.courseware_id, mode=EDX_ENROLLMENT_AUDIT_MODE, - username=None, - force_enrollment=False, + username=user.username, + force_enrollment=True, ) assert results == [audit_result] patched_log_error.assert_called_once() @@ -494,14 +487,14 @@ def test_enroll_pro_api_fail(mocker, user): mock_client.enrollments.create_student_enrollment = mocker.Mock( side_effect=HTTPError(response=pro_enrollment_response) ) - mocker.patch("courseware.api.get_edx_api_client", return_value=mock_client) + mocker.patch("courseware.api.get_edx_api_service_client", return_value=mock_client) course_run = CourseRunFactory.build() with pytest.raises(EdxApiEnrollErrorException): enroll_in_edx_course_runs(user, [course_run]) -def test_enroll_pro_unknown_fail(mocker, user): +def test_enroll_pro_unknown_fail(mocker, user, settings): """ Tests that enroll_in_edx_course_runs raises an UnknownEdxApiEnrollException if an unexpected exception is encountered @@ -512,6 +505,7 @@ def test_enroll_pro_unknown_fail(mocker, user): ) mocker.patch("courseware.api.get_edx_api_client", return_value=mock_client) course_run = CourseRunFactory.build() + settings.OPENEDX_SERVICE_WORKER_API_TOKEN = "mock_api_token" with pytest.raises(UnknownEdxApiEnrollException): enroll_in_edx_course_runs(user, [course_run])