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
This commit is contained in:
Binod Pant
72
docs/decisions/0010-extract-enrollment-python-api.rst
Normal file
72
docs/decisions/0010-extract-enrollment-python-api.rst
Normal file
@@ -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.
|
||||
@@ -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.
|
||||
|
||||
30
openedx/core/djangoapps/enrollments/tests/test_utils.py
Normal file
30
openedx/core/djangoapps/enrollments/tests/test_utils.py
Normal file
@@ -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
|
||||
20
openedx/core/djangoapps/enrollments/utils.py
Normal file
20
openedx/core/djangoapps/enrollments/utils.py
Normal file
@@ -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')
|
||||
@@ -0,0 +1,12 @@
|
||||
# lint-amnesty, pylint: disable=missing-module-docstring
|
||||
|
||||
class CourseIdMissingException(Exception):
|
||||
"""
|
||||
course_id missing
|
||||
"""
|
||||
|
||||
|
||||
class UserDoesNotExistException(Exception):
|
||||
"""
|
||||
course_id invalid
|
||||
"""
|
||||
@@ -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()
|
||||
108
openedx/features/enterprise_support/enrollments/utils.py
Normal file
108
openedx/features/enterprise_support/enrollments/utils.py
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user