From 33cdf634b4ffe0133fcde7273bb63aa94c1d293d Mon Sep 17 00:00:00 2001 From: Binod Pant Date: Wed, 21 Jul 2021 16:59:45 -0400 Subject: [PATCH] refactor: Extract core functionality of enrollment api in a python api to avoid REST calls from edx-enterprise (#28202) * feat: Refactor out non REST portions of enrollment api from enrollment POST method For use with edx-enterprise to avoid making REST calls for bulk enrollment and other use cases ENT-4746 * feat: Remove unused test Testing is covered by test_views * refactor: isort isort fixes * docs: ADR for why this change ADR ENT-4746 * test: Fix test failure by restoring course_id to correct object * test: Test fix * refactor: pylint fixes * refactor: raise from to avoid pylint error * refactor: Start to work toward a util in enterprise_support instead of refactoring this endpoint * feat: Add util function in enterprise_support to eventually handle enrollment, only used by bulk enrollment for now * feat: One more revised idea, this time low risk in edx platform and also helps address enterprise specific flow. testing pending * feat: syntax and unused constant * feat: Restore view and add new util function to use in edx-enterprise instead * feat: breakpoint * unused import * feat: don't fail on existing enrollment * docs: ADR update * docs: docstring minor update * test: unit test add_user_to_course_cohort * refactor: imports * feat: remove unused error classes * refactor: lint * test: Test cases * test: Two more tests for negative cases * feat: missing init.py file * test: Fix tests to use correct user mock * unused import * refactor: Review feedback, test fixes, needs rebase now * feat: rebase changes * feat: keep audit_log with similar logic as in the view * refactor: Review feedback, test constant usage --- .../0010-extract-enrollment-python-api.rst | 72 +++++++ openedx/core/djangoapps/enrollments/errors.py | 1 + .../enrollments/tests/test_utils.py | 30 +++ openedx/core/djangoapps/enrollments/utils.py | 20 ++ .../enrollments/__init__.py | 0 .../enrollments/exceptions.py | 12 ++ .../enrollments/tests/__init__.py | 0 .../enrollments/tests/test_utils.py | 196 ++++++++++++++++++ .../enterprise_support/enrollments/utils.py | 108 ++++++++++ 9 files changed, 439 insertions(+) create mode 100644 docs/decisions/0010-extract-enrollment-python-api.rst create mode 100644 openedx/core/djangoapps/enrollments/tests/test_utils.py create mode 100644 openedx/core/djangoapps/enrollments/utils.py create mode 100644 openedx/features/enterprise_support/enrollments/__init__.py create mode 100644 openedx/features/enterprise_support/enrollments/exceptions.py create mode 100644 openedx/features/enterprise_support/enrollments/tests/__init__.py create mode 100644 openedx/features/enterprise_support/enrollments/tests/test_utils.py create mode 100644 openedx/features/enterprise_support/enrollments/utils.py diff --git a/docs/decisions/0010-extract-enrollment-python-api.rst b/docs/decisions/0010-extract-enrollment-python-api.rst new file mode 100644 index 0000000000..1ad69caf4f --- /dev/null +++ b/docs/decisions/0010-extract-enrollment-python-api.rst @@ -0,0 +1,72 @@ +Extract python api from POST /api/enrollment/v1/enrollment handler +================================================================== + +Status +------ +Accepted + + +Context +------- + +edx-enterprise bulk enrollment use case currently invokes the mentioned endpoint once per learner + course +which leads to a large number of REST api calls in a short period. It is expensive, and leads to +hitting rate limits when used with our own services/functions within Enterprise. + + + +Decisions +--------- + +We will copy some of the core enrollment functionality present in the POST handler from + `POST /api/enrollment/v1/enrollment` +into a non-REST Python method without modify the existing POST handler. + +This is so we can call the basic 'create enrollment in LMS' functionality from edx-enterprise without +incurring REST expense. We also do not need to apply the rigorous access and embargo checks present +in the api handler that check the `request` object, since the caller inthis case will be our co-located +code running with the LMS already. + +We are not changing the POST handler because it serves various use cases and parameters, as well as +performs authorization checks on request object, none of which are needed and would require careful +and rigorous testing of various enrollment flows, and also introduce risk of regressions if done in a single round of work. + +We will add a new function to the `enterprise_support` package in edx-platform to achieve this. + +A few other features of the endpoint are also not needed in order to obtain the functionality needed +to replace the existing POST call: + +- any request parsing (since it's not a REST based api) +- any code related to checking access or authorization based on request.user (such as has_api_key_permissions) (since this api will be called from an already secured endpoint within edx-enterprise via bulk enrollment (or other) pathways. +- embargo check (since the caller of this api won't be external or browser-based and there is not request object in scope anymore : the embargo check is based off of the request's IP address) +- `is_active` logic (since we already use is_active=True in the existing post call) +- any logic related to `explicit_linked_enterprise` since we are only using this call to perform a LMS enrollment (student_courseenrollment) and all EnterprisecourseEnrollment specific work will be done after calling this function back in edx-enterprise +- any logic realted to mode changes : this appears to be a use case that is also not relevant to bulk enrollment +- email opt in: modeling afer how we call he endpoint today we don't use this option so not including it + + +NOTE: No changes will be made to the REST endpoint mentioned, since it also has external customers who may be using +parameters such as `explicit_linked_enterprise` and other pieces of logic too none of which are relevant +to the usage within edx-enterprise and also increase the scope of the task to a great extent. + + +Consequences +------------ + +None expected on existing clients, since it's a separate code path from the POST handler. + +Benefits: installed apps like edx-enterprise can now avoid making REST API calls to LMS for +enrolling one user in one course run, which is all that is needed for the current use case in bulk enrollment. + +Alternatives considered +----------------------- + +Write a new endpoint in edx-platform that can handle bulk enrollment in a single REST invocation. +Since an existing bulk_enroll endpoint does exist in edx-platform writing a new one is not sensible. + +Use the existing bulk_enroll endpoint that exists in edx-platform somehow. This endpoint cannot be +used as is, and we will still need to handle enterprise-specific concerns. + +Add batching and pause between batches in the existing edx-enterprise codebase. This will +avoid the too many requests per unit time. But it involves adding pauses in a request cycle. This +is not ideal for user experience. diff --git a/openedx/core/djangoapps/enrollments/errors.py b/openedx/core/djangoapps/enrollments/errors.py index d75ec94352..e68228d367 100644 --- a/openedx/core/djangoapps/enrollments/errors.py +++ b/openedx/core/djangoapps/enrollments/errors.py @@ -7,6 +7,7 @@ class CourseEnrollmentError(Exception): Describes any error that may occur when reading or updating enrollment information for a user or a course. """ + def __init__(self, msg, data=None): super().__init__(msg) # Corresponding information to help resolve the error. diff --git a/openedx/core/djangoapps/enrollments/tests/test_utils.py b/openedx/core/djangoapps/enrollments/tests/test_utils.py new file mode 100644 index 0000000000..19534f8acd --- /dev/null +++ b/openedx/core/djangoapps/enrollments/tests/test_utils.py @@ -0,0 +1,30 @@ +""" +Tests for enrollment utils +""" +import unittest +from unittest.mock import patch + +from openedx.core.djangoapps.enrollments.utils import add_user_to_course_cohort + + +class EnrollmentUtilsTest(unittest.TestCase): + """ + Enrollment utils test cases + """ + @patch("openedx.core.djangoapps.enrollments.utils.add_user_to_cohort") + @patch("openedx.core.djangoapps.enrollments.utils.get_cohort_by_name") + def test_adds_user_to_cohort(self, mock_get_cohort_by_name, mock_add_user_to_cohort): + user = {} + mock_get_cohort_by_name.return_value = "a_cohort" + + add_user_to_course_cohort("a_cohort", "a_course_id", user) + assert mock_add_user_to_cohort.call_count == 1 + + @patch("openedx.core.djangoapps.enrollments.utils.add_user_to_cohort") + @patch("openedx.core.djangoapps.enrollments.utils.get_cohort_by_name") + def test_does_not_add_user_to_cohort(self, mock_get_cohort_by_name, mock_add_user_to_cohort): + user = {} + mock_get_cohort_by_name.return_value = "a_cohort" + + add_user_to_course_cohort(None, "a_course_id", user) + assert mock_add_user_to_cohort.call_count == 0 diff --git a/openedx/core/djangoapps/enrollments/utils.py b/openedx/core/djangoapps/enrollments/utils.py new file mode 100644 index 0000000000..44a6f247c3 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/utils.py @@ -0,0 +1,20 @@ +""" +Utils for use in enrollment code +""" +import logging +from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, get_cohort_by_name + +logger = logging.getLogger(__name__) + + +def add_user_to_course_cohort(cohort_name, course_id, user): + """ + If cohort_name is provided, adds user to the cohort + """ + if cohort_name is not None: + cohort = get_cohort_by_name(course_id, cohort_name) + try: + add_user_to_cohort(cohort, user) + except ValueError: + # user already in cohort, probably because they were un-enrolled and re-enrolled + logger.exception('Cohort re-addition') diff --git a/openedx/features/enterprise_support/enrollments/__init__.py b/openedx/features/enterprise_support/enrollments/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/features/enterprise_support/enrollments/exceptions.py b/openedx/features/enterprise_support/enrollments/exceptions.py new file mode 100644 index 0000000000..0364bd1f26 --- /dev/null +++ b/openedx/features/enterprise_support/enrollments/exceptions.py @@ -0,0 +1,12 @@ +# lint-amnesty, pylint: disable=missing-module-docstring + +class CourseIdMissingException(Exception): + """ + course_id missing + """ + + +class UserDoesNotExistException(Exception): + """ + course_id invalid + """ diff --git a/openedx/features/enterprise_support/enrollments/tests/__init__.py b/openedx/features/enterprise_support/enrollments/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/features/enterprise_support/enrollments/tests/test_utils.py b/openedx/features/enterprise_support/enrollments/tests/test_utils.py new file mode 100644 index 0000000000..638b16bb48 --- /dev/null +++ b/openedx/features/enterprise_support/enrollments/tests/test_utils.py @@ -0,0 +1,196 @@ +""" +Test the enterprise support utils. +""" + +from unittest import mock +from unittest.case import TestCase + +from django.core.exceptions import ObjectDoesNotExist +from opaque_keys.edx.keys import CourseKey + +from openedx.core.djangoapps.course_groups.cohorts import CourseUserGroup +from openedx.core.djangoapps.enrollments.errors import CourseEnrollmentError, CourseEnrollmentExistsError +from openedx.core.djangolib.testing.utils import skip_unless_lms +from openedx.features.enterprise_support.enrollments.exceptions import ( + CourseIdMissingException, + UserDoesNotExistException +) +from openedx.features.enterprise_support.enrollments.utils import lms_enroll_user_in_course + +COURSE_STRING = 'course-v1:OpenEdX+OutlineCourse+Run3' +ENTERPRISE_UUID = 'enterprise_uuid' +COURSE_ID = CourseKey.from_string(COURSE_STRING) +USERNAME = 'test' +USER_ID = 1223 +COURSE_MODE = 'verified' + + +@skip_unless_lms +class EnrollmentUtilsTest(TestCase): + """ + Test enterprise support utils. + """ + + def setUp(self): + super().setUp() + self.a_user = mock.MagicMock() + self.a_user.id = USER_ID + self.a_user.username = USERNAME + + def test_validation_of_inputs_course_id(self): + with self.assertRaises(CourseIdMissingException): + lms_enroll_user_in_course(USERNAME, None, COURSE_MODE, ENTERPRISE_UUID) + + def test_validation_of_inputs_user_not_provided(self): + with self.assertRaises(UserDoesNotExistException): + lms_enroll_user_in_course( + None, + COURSE_ID, + COURSE_MODE, + ENTERPRISE_UUID, + ) + + @mock.patch('openedx.features.enterprise_support.enrollments.utils.User.objects.get') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.transaction') + def test_validation_of_inputs_user_not_found(self, mock_tx, mock_user_model): + mock_tx.return_value.atomic.side_effect = None + mock_user_model.side_effect = ObjectDoesNotExist() + with self.assertRaises(UserDoesNotExistException): + lms_enroll_user_in_course( + USERNAME, + COURSE_ID, + COURSE_MODE, + ENTERPRISE_UUID, + ) + + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.add_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.get_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.add_user_to_course_cohort') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.User.objects.get') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.transaction') + def test_course_enrollment_error_raises( + self, + mock_tx, + mock_user_model, + mock_add_user_to_course_cohort, + mock_get_enrollment_api, + mock_add_enrollment_api, + ): + enrollment_response = {'mode': COURSE_MODE, 'is_active': True} + + mock_add_enrollment_api.side_effect = CourseEnrollmentError("test") + mock_tx.return_value.atomic.side_effect = None + + mock_get_enrollment_api.return_value = enrollment_response + + mock_user_model.return_value = self.a_user + + with self.assertRaises(CourseEnrollmentError): + lms_enroll_user_in_course(USERNAME, COURSE_ID, COURSE_MODE, ENTERPRISE_UUID) + mock_add_user_to_course_cohort.assert_not_called() + mock_get_enrollment_api.assert_called_once() + + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.add_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.get_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.add_user_to_course_cohort') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.User.objects.get') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.transaction') + def test_course_group_error_raises( + self, + mock_tx, + mock_user_model, + mock_add_user_to_course_cohort, + mock_get_enrollment_api, + mock_add_enrollment_api, + ): + enrollment_response = {'mode': COURSE_MODE, 'is_active': True} + + mock_add_enrollment_api.side_effect = CourseUserGroup.DoesNotExist() + mock_tx.return_value.atomic.side_effect = None + + mock_get_enrollment_api.return_value = enrollment_response + + mock_user_model.return_value = self.a_user + + with self.assertRaises(CourseUserGroup.DoesNotExist): + lms_enroll_user_in_course(USERNAME, COURSE_ID, COURSE_MODE, ENTERPRISE_UUID) + mock_add_user_to_course_cohort.assert_not_called() + mock_get_enrollment_api.assert_called_once() + + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.add_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.get_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.add_user_to_course_cohort') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.User.objects.get') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.transaction') + def test_calls_enrollment_and_cohort_apis( + self, + mock_tx, + mock_user_model, + mock_add_user_to_course_cohort, + mock_get_enrollment_api, + mock_add_enrollment_api, + ): + + expected_response = {'a': 'value'} + enrollment_response = {'mode': COURSE_MODE, 'is_active': True} + + mock_add_enrollment_api.return_value = expected_response + mock_tx.return_value.atomic.side_effect = None + + mock_get_enrollment_api.return_value = enrollment_response + + mock_user_model.return_value = self.a_user + + response = lms_enroll_user_in_course(USERNAME, COURSE_ID, COURSE_MODE, ENTERPRISE_UUID) + + assert response == expected_response + mock_add_enrollment_api.assert_called_once_with( + USERNAME, + str(COURSE_ID), + mode=COURSE_MODE, + is_active=True, + enrollment_attributes=None, + enterprise_uuid=ENTERPRISE_UUID, + ) + + mock_add_user_to_course_cohort.assert_called_once() + mock_get_enrollment_api.assert_called_once_with(USERNAME, str(COURSE_ID)) + + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.add_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.enrollment_api.get_enrollment') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.add_user_to_course_cohort') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.User.objects.get') + @mock.patch('openedx.features.enterprise_support.enrollments.utils.transaction') + def test_existing_enrollment_does_not_fail( + self, + mock_tx, + mock_user_model, + mock_add_user_to_course_cohort, + mock_get_enrollment_api, + mock_add_enrollment_api, + ): + + expected_response = None + enrollment_response = {'mode': COURSE_MODE, 'is_active': True} + + mock_add_enrollment_api.side_effect = CourseEnrollmentExistsError("test", {}) + mock_tx.return_value.atomic.side_effect = None + + mock_get_enrollment_api.return_value = enrollment_response + + mock_user_model.return_value = self.a_user + + response = lms_enroll_user_in_course(USERNAME, COURSE_ID, COURSE_MODE, ENTERPRISE_UUID) + + assert response == expected_response + mock_add_enrollment_api.assert_called_once_with( + USERNAME, + str(COURSE_ID), + mode=COURSE_MODE, + is_active=True, + enrollment_attributes=None, + enterprise_uuid=ENTERPRISE_UUID, + ) + + mock_add_user_to_course_cohort.assert_not_called() + mock_get_enrollment_api.assert_called_once() diff --git a/openedx/features/enterprise_support/enrollments/utils.py b/openedx/features/enterprise_support/enrollments/utils.py new file mode 100644 index 0000000000..b52a809043 --- /dev/null +++ b/openedx/features/enterprise_support/enrollments/utils.py @@ -0,0 +1,108 @@ +""" +Utils for use in enrollment codebase such as views. +""" +import logging + +from django.core.exceptions import ObjectDoesNotExist # lint-amnesty, pylint: disable=wrong-import-order +from django.db import transaction + +from common.djangoapps.student.models import User +from openedx.core.djangoapps.course_groups.cohorts import CourseUserGroup +from openedx.core.djangoapps.enrollments import api as enrollment_api +from openedx.core.djangoapps.enrollments.errors import CourseEnrollmentError, CourseEnrollmentExistsError +from openedx.core.djangoapps.enrollments.utils import add_user_to_course_cohort +from openedx.core.lib.log_utils import audit_log +from openedx.features.enterprise_support.enrollments.exceptions import ( + CourseIdMissingException, + UserDoesNotExistException +) + +log = logging.getLogger(__name__) + + +def lms_enroll_user_in_course( + username, + course_id, + mode, + enterprise_uuid, + cohort_name=None, + is_active=True, +): + """ + Enrollment function meant to be called by edx-enterprise to replace the + current uses of the EnrollmentApiClient + The REST enrollment endpoint may also eventually also want to reuse this function + since it's a subset of what the endpoint handles + + Unlike the REST endpoint, this function does not check for enterprise enabled, or user api key + permissions etc. Those concerns are still going to be used by REST endpoint but this function + is meant for use from within edx-enterprise hence already presume such privileges. + + Arguments: + - username (str): User name + - course_id (obj) : Course key obtained using CourseKey.from_string(course_id_input) + - mode (CourseMode): course mode + - enterprise_uuid (str): id to identify the enterprise to enroll under + - cohort_name (str): Optional. If provided, user will be added to cohort + - is_active (bool): Optional. A Boolean value that indicates whether the + enrollment is to be set to inactive (if False). Usually we want a True if enrolling anew. + + Returns: A serializable dictionary of the new course enrollment. If it hits + `CourseEnrollmentExistsError` then it logs the error and returns None. + """ + user = _validate_enrollment_inputs(username, course_id) + + with transaction.atomic(): + try: + response = enrollment_api.add_enrollment( + username, + str(course_id), + mode=mode, + is_active=is_active, + enrollment_attributes=None, + enterprise_uuid=enterprise_uuid, + ) + add_user_to_course_cohort(cohort_name, course_id, user) + log.info('The user [%s] has been enrolled in course run [%s].', username, course_id) + return response + except CourseEnrollmentExistsError as error: + log.warning('An enrollment already exists for user [%s] in course run [%s].', username, course_id) + return None + except CourseEnrollmentError as error: + log.exception("An error occurred while creating the new course enrollment for user " + "[%s] in course run [%s]", username, course_id) + raise error + except CourseUserGroup.DoesNotExist as error: + log.exception('Missing cohort [%s] in course run [%s]', cohort_name, course_id) + raise error + finally: + # Assumes that the ecommerce service uses an API key to authenticate. + current_enrollment = enrollment_api.get_enrollment(username, str(course_id)) + audit_log( + 'enrollment_change_requested', + course_id=str(course_id), + requested_mode=mode, + actual_mode=current_enrollment['mode'] if current_enrollment else None, + requested_activation=is_active, + actual_activation=current_enrollment['is_active'] if current_enrollment else None, + user_id=user.id + ) + + +def _validate_enrollment_inputs(username, course_id): + """ + Validates username and course_id. + Raises: + - UserDoesNotExistException if user not found. + - CourseIdMissingException if course_id not provided. + """ + if not course_id: + raise CourseIdMissingException("Course ID must be specified to create a new enrollment.") + if not username: + raise UserDoesNotExistException('username is a required argument for enrollment') + try: + # Lookup the user, instead of using request.user, since request.user may not match the username POSTed. + user = User.objects.get(username=username) + except ObjectDoesNotExist as error: + raise UserDoesNotExistException(f'The user {username} does not exist.') from error + return user