From 597cd305ed2c113236b5de9b8c8282622b62b18b Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 1 Aug 2025 18:22:10 -0500 Subject: [PATCH 01/19] feat: add instructor task for async batch enrollment --- .../bulk_enroll/tests/test_views.py | 40 ++- lms/djangoapps/bulk_enroll/views.py | 2 +- lms/djangoapps/instructor/enrollment.py | 38 ++- lms/djangoapps/instructor/message_types.py | 12 + lms/djangoapps/instructor/tests/test_api.py | 38 ++- lms/djangoapps/instructor/utils.py | 261 ++++++++++++++++++ lms/djangoapps/instructor/views/api.py | 180 ++++++------ lms/djangoapps/instructor/views/serializer.py | 1 + lms/djangoapps/instructor_task/api.py | 56 ++++ lms/djangoapps/instructor_task/data.py | 1 + .../instructor_task/notifications.py | 181 ++++++++++++ lms/djangoapps/instructor_task/tasks.py | 12 + .../tasks_helper/enrollments.py | 102 ++++++- .../js/instructor_dashboard/membership.js | 44 +++ .../edx_ace/batchenrollment/email/body.html | 56 ++++ .../edx_ace/batchenrollment/email/body.txt | 13 + .../batchenrollment/email/from_name.txt | 1 + .../edx_ace/batchenrollment/email/head.html | 1 + .../edx_ace/batchenrollment/email/subject.txt | 2 + .../edx_ace/batchenrollment/push/body.txt | 4 + .../edx_ace/batchenrollment/push/title.txt | 2 + .../instructor_dashboard_2/membership.html | 15 + .../djangoapps/ace_common/templatetags/ace.py | 4 +- 23 files changed, 948 insertions(+), 118 deletions(-) create mode 100644 lms/djangoapps/instructor/utils.py create mode 100644 lms/djangoapps/instructor_task/notifications.py create mode 100644 lms/templates/instructor/edx_ace/batchenrollment/email/body.html create mode 100644 lms/templates/instructor/edx_ace/batchenrollment/email/body.txt create mode 100644 lms/templates/instructor/edx_ace/batchenrollment/email/from_name.txt create mode 100644 lms/templates/instructor/edx_ace/batchenrollment/email/head.html create mode 100644 lms/templates/instructor/edx_ace/batchenrollment/email/subject.txt create mode 100644 lms/templates/instructor/edx_ace/batchenrollment/push/body.txt create mode 100644 lms/templates/instructor/edx_ace/batchenrollment/push/title.txt diff --git a/lms/djangoapps/bulk_enroll/tests/test_views.py b/lms/djangoapps/bulk_enroll/tests/test_views.py index 780b89fff33d..7a9321f872f6 100644 --- a/lms/djangoapps/bulk_enroll/tests/test_views.py +++ b/lms/djangoapps/bulk_enroll/tests/test_views.py @@ -14,7 +14,7 @@ from opaque_keys.edx.keys import CourseKey from rest_framework.test import APIRequestFactory, APITestCase, force_authenticate -from common.djangoapps.student.models import ( # pylint: disable=line-too-long +from common.djangoapps.student.models import ( ENROLLED_TO_UNENROLLED, UNENROLLED_TO_ENROLLED, CourseEnrollment, @@ -150,6 +150,9 @@ def test_invalid_email(self): { "identifier": 'percivaloctavius@', "invalidIdentifier": True, + "success": False, + "error_type": "invalid_identifier", + "error_message": "Invalid email address", } ] } @@ -182,6 +185,9 @@ def test_invalid_username(self): { "identifier": 'percivaloctavius', "invalidIdentifier": True, + "success": False, + "error_type": "invalid_identifier", + "error_message": "Invalid email address", } ] } @@ -224,7 +230,9 @@ def test_enroll_with_username(self): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -274,7 +282,9 @@ def test_enroll_with_email(self, use_json): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -328,7 +338,9 @@ def test_unenroll(self, use_json): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": ENROLLED_TO_UNENROLLED, } ] } @@ -432,7 +444,9 @@ def test_add_to_valid_cohort(self): "user": True, "allowed": False, "cohort": 'cohort1', - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -483,7 +497,9 @@ def test_readd_to_different_cohort(self): "user": True, "allowed": False, "cohort": 'cohort1', - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -531,7 +547,9 @@ def test_readd_to_different_cohort(self): "user": True, "allowed": False, "cohort": 'cohort2', - } + }, + "success": True, + "state_transition": ENROLLED_TO_ENROLLED, } ] } @@ -578,7 +596,9 @@ def test_readd_to_same_cohort(self): "user": True, "allowed": False, "cohort": 'cohort1', - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -627,7 +647,9 @@ def test_readd_to_same_cohort(self): "user": True, "allowed": False, "cohort": 'cohort1', - } + }, + "success": True, + "state_transition": ENROLLED_TO_ENROLLED, } ] } diff --git a/lms/djangoapps/bulk_enroll/views.py b/lms/djangoapps/bulk_enroll/views.py index 22f4650c3d08..7193e44ab9bf 100644 --- a/lms/djangoapps/bulk_enroll/views.py +++ b/lms/djangoapps/bulk_enroll/views.py @@ -90,7 +90,7 @@ def post(self, request): # pylint: disable=missing-function-docstring # Internal request to DRF view view = StudentsUpdateEnrollmentView() response_content = view._process_student_enrollment( # pylint: disable=protected-access - user=request.user, + request=request, course_id=course_id, data=request.data, secure=request.is_secure() diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 79fa2a7b1db8..a8b470b86dcf 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -10,8 +10,10 @@ from datetime import datetime import pytz +from crum import get_current_request from django.conf import settings -from django.contrib.auth.models import User # pylint: disable=imported-auth-user +from django.contrib.auth.models import User +from django.contrib.sites.models import Site from django.template.loader import render_to_string from django.urls import reverse from django.utils.translation import override as override_language @@ -50,7 +52,9 @@ ) from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.djangoapps.theming.helpers import get_current_site from openedx.core.djangoapps.user_api.models import UserPreference +from openedx.core.lib.celery.task_utils import emulate_http_request from xmodule.modulestore.django import modulestore # pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # pylint: disable=wrong-import-order @@ -591,7 +595,37 @@ def send_mail_to_student(student, param_dict, language=None): language=language, user_context=param_dict, ) - ace.send(message) + + current_request = get_current_request() + + if current_request is None: + # We're in a Celery task context, need to emulate HTTP request + site = get_current_site() + if not site: + try: + site = Site.objects.get(id=settings.SITE_ID) + except Site.DoesNotExist: + try: + site = Site.objects.first() + except Exception: # pylint: disable=broad-except + site = None + + # Get the recipient user for tracking purposes + user = None + if lms_user_id and lms_user_id > 0: + try: + user = User.objects.get(id=lms_user_id) + except User.DoesNotExist: + pass + + # Use emulate_http_request to provide the necessary context for template tags + # that require a request object, such as google_analytics_tracking_pixel + with emulate_http_request(site=site, user=user): + ace.send(message) + else: + # We're in a web context, just send the message directly + # The current request already provides the necessary context + ace.send(message) def render_message_to_string(subject_template, message_template, param_dict, language=None): diff --git a/lms/djangoapps/instructor/message_types.py b/lms/djangoapps/instructor/message_types.py index 6354cfb54f63..470bae398c0c 100644 --- a/lms/djangoapps/instructor/message_types.py +++ b/lms/djangoapps/instructor/message_types.py @@ -83,3 +83,15 @@ class RemoveBetaTester(BaseMessageType): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.options['transactional'] = True + + +class BatchEnrollment(BaseMessageType): + """ + A message for instructors when finish the batch enrollment async process. + """ + + APP_LABEL = "instructor" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.options["transactional"] = True diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 5210d35dd8cf..f314506a5994 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1104,6 +1104,9 @@ def test_invalid_email(self): { "identifier": 'percivaloctavius@', "invalidIdentifier": True, + "success": False, + "error_type": "invalid_identifier", + "error_message": "Invalid email address", } ] } @@ -1125,6 +1128,9 @@ def test_invalid_username(self): { "identifier": 'percivaloctavius', "invalidIdentifier": True, + "success": False, + "error_type": "invalid_identifier", + "error_message": "Invalid email address", } ] } @@ -1156,7 +1162,9 @@ def test_enroll_with_username(self): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -1195,7 +1203,9 @@ def test_enroll_without_email(self): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -1241,7 +1251,9 @@ def test_enroll_with_email(self, protocol): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": UNENROLLED_TO_ENROLLED, } ] } @@ -1428,7 +1440,9 @@ def test_unenroll_without_email(self): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": ENROLLED_TO_UNENROLLED, } ] } @@ -1471,7 +1485,9 @@ def test_unenroll_with_email(self): "auto_enroll": False, "user": True, "allowed": False, - } + }, + "success": True, + "state_transition": ENROLLED_TO_UNENROLLED, } ] } @@ -1525,7 +1541,9 @@ def test_unenroll_with_email_allowed_student(self): "auto_enroll": False, "user": False, "allowed": False, - } + }, + "success": True, + "state_transition": ALLOWEDTOENROLL_TO_ENROLLED, } ] } @@ -1725,7 +1743,9 @@ def test_unenrolled_allowed_to_enroll_user(self): "auto_enroll": False, "user": True, "allowed": True, - } + }, + "success": True, + "state_transition": ALLOWEDTOENROLL_TO_ENROLLED, } ] } @@ -1767,7 +1787,9 @@ def test_unenrolled_already_not_enrolled_user(self): "auto_enroll": False, "user": False, "allowed": False, - } + }, + "success": True, + "state_transition": UNENROLLED_TO_UNENROLLED, } ] } diff --git a/lms/djangoapps/instructor/utils.py b/lms/djangoapps/instructor/utils.py new file mode 100644 index 000000000000..84df2fc5f823 --- /dev/null +++ b/lms/djangoapps/instructor/utils.py @@ -0,0 +1,261 @@ +""" +Utility functions for student enrollment operations. + +This module contains reusable functions for processing student enrollments +that can be used in both synchronous and asynchronous contexts. +""" + +import logging +from typing import Callable, Optional + +from django.contrib.auth import get_user_model +from django.core.exceptions import ValidationError +from django.core.validators import validate_email +from opaque_keys.edx.keys import CourseKey + +from common.djangoapps.student.models import ( + ALLOWEDTOENROLL_TO_ENROLLED, + ALLOWEDTOENROLL_TO_UNENROLLED, + DEFAULT_TRANSITION_STATE, + ENROLLED_TO_ENROLLED, + ENROLLED_TO_UNENROLLED, + UNENROLLED_TO_ALLOWEDTOENROLL, + UNENROLLED_TO_ENROLLED, + UNENROLLED_TO_UNENROLLED, + CourseEnrollment, + EnrollStatusChange, + ManualEnrollmentAudit, +) +from lms.djangoapps.instructor.enrollment import ( + enroll_email, + get_email_params, + get_user_email_language, + unenroll_email, +) +from lms.djangoapps.instructor.views.tools import get_student_from_identifier +from openedx.core.lib.courses import get_course_by_id + +log = logging.getLogger(__name__) + + +User = get_user_model() + + +def _determine_enroll_state_transition(before_state: dict, after_state: dict) -> str: + """ + Determine the state transition for an enrollment operation. + + Args: + before_state (dict): State before enrollment with keys 'enrollment', 'user', 'allowed' + after_state (dict): State after enrollment with keys 'enrollment', 'allowed' + + Returns: + str: The state transition constant + """ + # User was not registered before + if not before_state["user"]: + if after_state["allowed"]: + return UNENROLLED_TO_ALLOWEDTOENROLL + return DEFAULT_TRANSITION_STATE + + # User was registered and enrolled successfully + if after_state["enrollment"]: + if before_state["enrollment"]: + return ENROLLED_TO_ENROLLED + if before_state["allowed"]: + return ALLOWEDTOENROLL_TO_ENROLLED + return UNENROLLED_TO_ENROLLED + + return DEFAULT_TRANSITION_STATE + + +def _determine_unenroll_state_transition(before_state: dict) -> str: + """ + Determine the state transition for an unenrollment operation. + + Args: + before_state (dict): State before unenrollment with keys 'enrollment', 'allowed' + + Returns: + str: The state transition constant + """ + if before_state["enrollment"]: + return ENROLLED_TO_UNENROLLED + if before_state["allowed"]: + return ALLOWEDTOENROLL_TO_UNENROLLED + return UNENROLLED_TO_UNENROLLED + + +def process_single_student_enrollment( + request_user, + course_key: CourseKey, + action: str, + identifier: str, + auto_enroll: bool, + email_students: bool, + reason: str | None, + email_params: dict | None, +): + """ + Process enrollment/unenrollment for a single student. + + Args: + request_user (User): User who initiated the enrollment operation + course_key (CourseKey): CourseKey object for the course + action (str): 'enroll' or 'unenroll' + identifier (str): Student identifier (email or username) + auto_enroll (bool): Whether to auto-enroll in verified track if applicable + email_students (bool): Whether to send enrollment emails + reason (str | None): Optional reason for enrollment change + email_params (dict | None): Pre-computed email parameters (optional) + + Returns: + dict: Result of the enrollment operation with keys: + - identifier: The student identifier + - success: Boolean indicating if operation was successful + - before: State before operation (if successful) + - after: State after operation (if successful) + - error_type: Type of error ('invalid_identifier', 'validation_error', 'general_error') + - error_message: Error message (if failed) + """ + enrollment_obj = None + state_transition = DEFAULT_TRANSITION_STATE + identified_user = None + email = None + language = None + + try: + identified_user = get_student_from_identifier(identifier) + except User.DoesNotExist: + email = identifier + else: + email = identified_user.email + language = get_user_email_language(identified_user) + + try: + validate_email(email) # Raises ValidationError if invalid + + if action == EnrollStatusChange.enroll: + before, after, enrollment_obj = enroll_email( + course_key, email, auto_enroll, email_students, {**email_params}, language=language + ) + before_state = before.to_dict() + after_state = after.to_dict() + state_transition = _determine_enroll_state_transition(before_state, after_state) + + elif action == EnrollStatusChange.unenroll: + before, after = unenroll_email( + course_key, email, email_students, {**email_params}, language=language + ) + before_state = before.to_dict() + after_state = after.to_dict() + state_transition = _determine_unenroll_state_transition(before_state) + enrollment_obj = CourseEnrollment.get_enrollment(identified_user, course_key) if identified_user else None + + # Create audit record + ManualEnrollmentAudit.create_manual_enrollment_audit( + request_user, email, state_transition, reason, enrollment_obj + ) + + return { + "identifier": identifier, + "before": before_state, + "after": after_state, + "success": True, + "state_transition": state_transition, + } + except ValidationError: + return { + "identifier": identifier, + "invalidIdentifier": True, + "success": False, + "error_type": "invalid_identifier", + "error_message": "Invalid email address", + } + except Exception as exc: # pylint: disable=broad-exception-caught + log.exception("Error while processing student") + log.exception(exc) + return { + "identifier": identifier, + "error": True, + "success": False, + "error_type": "general_error", + "error_message": str(exc), + } + + +def process_student_enrollment_batch( + request_user, + course_key: CourseKey, + action: str, + identifiers: list[str], + auto_enroll: bool, + email_students: bool, + reason: str | None, + secure: bool, + progress_callback: Optional[Callable] = None, +): + """ + Process a batch of student enrollment/unenrollment operations. + + Args: + request_user (User): User who initiated the batch operation + course_key (CourseKey): CourseKey object for the course + action (str): 'enroll' or 'unenroll' + identifiers (list[str]): List of student identifiers (emails or usernames) + auto_enroll (bool): Whether to auto-enroll in verified track if applicable + email_students (bool): Whether to send enrollment emails + reason (str | None): Optional reason for enrollment change + secure (bool): Whether the request is secure (HTTPS) + progress_callback (Optional[Callable]): Optional callback function to report progress + Should accept (current, total, results) parameters + + Returns: + dict: Batch processing results with keys: + - action: The action performed + - auto_enroll: Auto-enrollment setting + - results: List of individual enrollment results + - successful_operations: Count of successful operations + - failed_operations: Count of failed operations + - total_students: Total number of students processed + """ + email_params = {} + if email_students: + course = get_course_by_id(course_key) + email_params = get_email_params(course, auto_enroll, secure=secure) + + results = [] + successful_operations = 0 + failed_operations = 0 + total_students = len(identifiers) + + for idx, identifier in enumerate(identifiers): + result = process_single_student_enrollment( + request_user=request_user, + course_key=course_key, + action=action, + identifier=identifier, + auto_enroll=auto_enroll, + email_students=email_students, + reason=reason, + email_params=email_params, + ) + + results.append(result) + + if result["success"]: + successful_operations += 1 + else: + failed_operations += 1 + + if progress_callback: + progress_callback(idx + 1, total_students, results) + + return { + "action": action, + "auto_enroll": auto_enroll, + "results": results, + "successful_operations": successful_operations, + "failed_operations": failed_operations, + "total_students": total_students, + } diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 033d1ef893c9..04b90a7d9b1e 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -54,14 +54,7 @@ from common.djangoapps.student import auth from common.djangoapps.student.api import is_user_enrolled_in_course from common.djangoapps.student.models import ( - ALLOWEDTOENROLL_TO_ENROLLED, - ALLOWEDTOENROLL_TO_UNENROLLED, - DEFAULT_TRANSITION_STATE, - ENROLLED_TO_ENROLLED, - ENROLLED_TO_UNENROLLED, - UNENROLLED_TO_ALLOWEDTOENROLL, UNENROLLED_TO_ENROLLED, - UNENROLLED_TO_UNENROLLED, CourseEnrollment, CourseEnrollmentAllowed, EntranceExamConfiguration, @@ -91,11 +84,10 @@ from lms.djangoapps.instructor.enrollment import ( enroll_email, get_email_params, - get_user_email_language, send_beta_role_email, send_mail_to_student, - unenroll_email, ) +from lms.djangoapps.instructor.utils import process_student_enrollment_batch from lms.djangoapps.instructor.views.instructor_task_helpers import extract_email_features, extract_task_features from lms.djangoapps.instructor.views.serializer import ( AccessSerializer, @@ -726,7 +718,8 @@ def create_and_enroll_user( @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') -class StudentsUpdateEnrollmentView(APIView): +@method_decorator(transaction.non_atomic_requests, name='dispatch') +class StudentsUpdateEnrollmentView(DeveloperErrorViewMixin, APIView): """ API view to enroll or unenroll students in a course. """ @@ -735,6 +728,7 @@ class StudentsUpdateEnrollmentView(APIView): permission_name = permissions.CAN_ENROLL @method_decorator(ensure_csrf_cookie) + @transaction.non_atomic_requests def post(self, request, course_id): """ Handle POST request to enroll or unenroll students. @@ -744,24 +738,25 @@ def post(self, request, course_id): - identifiers (str): comma/newline separated emails or usernames - auto_enroll (bool): auto-enroll in verified track if applicable - email_students (bool): whether to send enrollment emails + - async_processing (bool): whether to process asynchronously - reason (str, optional): reason for enrollment change Returns: - JSON response with action, auto_enroll flag, and enrollment results. """ response_payload = self._process_student_enrollment( - user=request.user, + request=request, course_id=course_id, data=request.data, secure=request.is_secure() ) return JsonResponse(response_payload) - def _process_student_enrollment(self, user, course_id, data, secure): # pylint: disable=too-many-statements + def _process_student_enrollment(self, request, course_id, data, secure): # pylint: disable=too-many-statements """ Core logic for enrolling or unenrolling students. - :param user: User making the request + :param request: The HTTP request object :param course_id: Course identifier :param data: Request data containing action, identifiers, etc. :param secure: Whether the request is secure (HTTPS) @@ -776,6 +771,7 @@ def _process_student_enrollment(self, user, course_id, data, secure): # pylint: identifiers_raw = serializer.validated_data['identifiers'] auto_enroll = serializer.validated_data['auto_enroll'] email_students = serializer.validated_data['email_students'] + async_processing = serializer.validated_data["async_processing"] reason = serializer.validated_data.get('reason') # Parse identifiers @@ -783,97 +779,93 @@ def _process_student_enrollment(self, user, course_id, data, secure): # pylint: course_key = CourseKey.from_string(course_id) - enrollment_obj = None - state_transition = DEFAULT_TRANSITION_STATE + if async_processing: - email_params = {} - if email_students: - course = get_course_by_id(course_key) - email_params = get_email_params(course, auto_enroll, secure=secure) + try: + instructor_task = task_api.submit_student_enrollment_batch( + request=request, + course_key=course_key, + action=action, + identifiers=identifiers, + auto_enroll=auto_enroll, + email_students=email_students, + reason=reason, + secure=secure, + ) - results = [] + return { + "action": action, + "auto_enroll": auto_enroll, + "async_processing": True, + "task_id": instructor_task.task_id, + "task_state": instructor_task.task_state, + "message": f"Async {action} task submitted for {len(identifiers)} students", + "total_students": len(identifiers), + } - for identifier in identifiers: # pylint: disable=too-many-nested-blocks - identified_user = None - email = None - language = None + except AlreadyRunningError: + return { + "action": action, + "auto_enroll": auto_enroll, + "async_processing": True, + "error": "A similar enrollment task is already running. Please wait for it to complete.", + "total_students": len(identifiers), + } - try: - identified_user = get_student_from_identifier(identifier) - except User.DoesNotExist: - email = identifier - else: - email = identified_user.email - language = get_user_email_language(identified_user) + return self._process_enrollment_sync( + request.user, course_key, action, identifiers, auto_enroll, email_students, reason, secure + ) - try: - validate_email(email) # Raises ValidationError if invalid + def _process_enrollment_sync( + self, + request_user: User, + course_key: CourseKey, + action: str, + identifiers: list[str], + auto_enroll: bool, + email_students: bool, + reason: str | None, + secure: bool, + ): + """ + Process student enrollment/unenrollment operations synchronously. - if action == 'enroll': - before, after, enrollment_obj = enroll_email( - course_key, email, auto_enroll, email_students, {**email_params}, language=language - ) - before_enrollment = before.to_dict()['enrollment'] - before_user_registered = before.to_dict()['user'] - before_allowed = before.to_dict()['allowed'] - after_enrollment = after.to_dict()['enrollment'] - after_allowed = after.to_dict()['allowed'] - - if before_user_registered: - if after_enrollment: - if before_enrollment: - state_transition = ENROLLED_TO_ENROLLED - elif before_allowed: - state_transition = ALLOWEDTOENROLL_TO_ENROLLED - else: - state_transition = UNENROLLED_TO_ENROLLED - elif after_allowed: - state_transition = UNENROLLED_TO_ALLOWEDTOENROLL - - elif action == 'unenroll': - before, after = unenroll_email( - course_key, email, email_students, {**email_params}, language=language - ) - before_enrollment = before.to_dict()['enrollment'] - before_allowed = before.to_dict()['allowed'] - enrollment_obj = ( - CourseEnrollment.get_enrollment(identified_user, course_key) - if identified_user else None - ) + This method handles batch enrollment operations by calling the + `process_student_enrollment_batch` utility function and returns a + simplified response containing the action, auto_enroll setting, + and enrollment results. - if before_enrollment: - state_transition = ENROLLED_TO_UNENROLLED - elif before_allowed: - state_transition = ALLOWEDTOENROLL_TO_UNENROLLED - else: - state_transition = UNENROLLED_TO_UNENROLLED + Args: + request_user (User): User who initiated the enrollment operation + course_key (CourseKey): CourseKey object for the target course + action (str): The enrollment action to perform ('enroll' or 'unenroll') + identifiers (list[str]): List of student identifiers (emails or usernames) + auto_enroll (bool): Whether to auto-enroll students in verified track if applicable + email_students (bool): Whether to send enrollment notification emails + reason (str | None): Optional reason for the enrollment change + secure (bool): Whether the request was made over HTTPS - except ValidationError: - results.append({ - 'identifier': identifier, - 'invalidIdentifier': True, - }) - except Exception as exc: # pylint: disable=broad-except - log.exception("Error while processing student") - log.exception(exc) - results.append({ - 'identifier': identifier, - 'error': True, - }) - else: - ManualEnrollmentAudit.create_manual_enrollment_audit( - identified_user, email, state_transition, reason, enrollment_obj - ) - results.append({ - 'identifier': identifier, - 'before': before.to_dict(), - 'after': after.to_dict(), - }) + Returns: + dict: Enrollment operation results containing: + - action: The action that was performed + - auto_enroll: The auto-enrollment setting used + - results: List of individual enrollment results for each student + """ + batch_result = process_student_enrollment_batch( + request_user=request_user, + course_key=course_key, + action=action, + identifiers=identifiers, + auto_enroll=auto_enroll, + email_students=email_students, + reason=reason, + secure=secure, + ) return { - 'action': action, - 'auto_enroll': auto_enroll, - 'results': results, + "action": batch_result["action"], + "auto_enroll": batch_result["auto_enroll"], + "results": batch_result["results"], } diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 081081d52ecd..b2817d5f2032 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -565,6 +565,7 @@ class StudentsUpdateEnrollmentSerializer(serializers.Serializer): identifiers = serializers.CharField() auto_enroll = serializers.BooleanField(default=False) email_students = serializers.BooleanField(default=False) + async_processing = serializers.BooleanField(default=False) reason = serializers.CharField(required=False, allow_blank=True) diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 558be9df8377..22aa61c2adee 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -13,6 +13,8 @@ import pytz from celery.states import READY_STATES +from django.http import HttpRequest +from opaque_keys.edx.keys import CourseKey from common.djangoapps.util import milestones_helpers from lms.djangoapps.bulk_email.api import get_course_email @@ -50,6 +52,7 @@ rescore_problem, reset_problem_attempts, send_bulk_course_email, + student_enrollment_batch, ) from xmodule.modulestore.django import modulestore # pylint: disable=wrong-import-order @@ -607,3 +610,56 @@ def process_scheduled_instructor_tasks(): submit_scheduled_task(schedule) except QueueConnectionError as exc: log.error(f"Error processing scheduled task with task id '{schedule.task.id}': {exc}") + + +def submit_student_enrollment_batch( + request: HttpRequest, + course_key: CourseKey, + action: str, + identifiers: list[str], + auto_enroll: bool, + email_students: bool, + reason: str | None, + secure: bool, +): + """ + Request to have student enrollment operations processed as a background task. + + The task will process a batch of enrollment/unenrollment operations for the specified + students in the given course. + + Args: + request (HttpRequest): The HTTP request object + course_key (CourseKey): Course identifier + action (str): 'enroll' or 'unenroll' + identifiers (list[str]): List of student identifiers (emails or usernames) + auto_enroll (bool): Whether to auto-enroll in verified track if applicable + email_students (bool): Whether to send enrollment emails + reason (str | None): Optional reason for enrollment change + secure (bool): Whether the request is secure (HTTPS) + + Returns: + InstructorTask object representing the submitted background task + + Raises: + AlreadyRunningError: If the same task is already running + """ + task_type = InstructorTaskTypes.STUDENT_ENROLLMENT_BATCH + task_class = student_enrollment_batch + + task_input = { + "action": action, + "identifiers": identifiers, + "auto_enroll": auto_enroll, + "email_students": email_students, + "reason": reason, + "secure": secure, + } + + # Create task key based on course, action and first few identifiers for uniqueness + # Limit identifiers in key to avoid exceeding max length + key_identifiers = identifiers[:5] if len(identifiers) > 5 else identifiers + task_key_stub = f'{course_key}_{action}_{"_".join(key_identifiers)}' + task_key = hashlib.md5(task_key_stub.encode("utf-8")).hexdigest() + + return submit_task(request, task_type, task_class, course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/data.py b/lms/djangoapps/instructor_task/data.py index ad0fe5764135..4b77468159ca 100644 --- a/lms/djangoapps/instructor_task/data.py +++ b/lms/djangoapps/instructor_task/data.py @@ -33,3 +33,4 @@ class InstructorTaskTypes(str, Enum): # noqa: UP042 RESCORE_PROBLEM = "rescore_problem" RESCORE_PROBLEM_IF_HIGHER = "rescore_problem_if_higher" RESET_PROBLEM_ATTEMPTS = "reset_problem_attempts" + STUDENT_ENROLLMENT_BATCH = "student_enrollment_batch" diff --git a/lms/djangoapps/instructor_task/notifications.py b/lms/djangoapps/instructor_task/notifications.py new file mode 100644 index 000000000000..4f16187a71b4 --- /dev/null +++ b/lms/djangoapps/instructor_task/notifications.py @@ -0,0 +1,181 @@ +""" +Notification utilities for instructor tasks. + +This module contains functions for sending email notifications about completed +instructor tasks (enrollments, grades, certificates, etc.). +""" + +import json +import logging + +from django.conf import settings +from django.contrib.sites.models import Site +from edx_ace import ace +from edx_ace.recipient import Recipient +from opaque_keys.edx.keys import CourseKey + +from common.djangoapps.student.models import EnrollStatusChange +from lms.djangoapps.instructor.message_types import BatchEnrollment +from lms.djangoapps.instructor_task.models import InstructorTask +from openedx.core.djangoapps.ace_common.template_context import get_base_template_context +from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY +from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.djangoapps.user_api.preferences.api import get_user_preference +from openedx.core.lib.celery.task_utils import emulate_http_request +from openedx.core.lib.courses import get_course_by_id + +TASK_LOG = logging.getLogger("edx.celery.task") + + +def _get_current_site() -> Site | None: + """ + Get the current Django Site instance with fallback logic. + + Returns: + Site | None: The current Site object or None if unavailable + """ + # Try to get current site if method exists + if hasattr(Site.objects, "get_current"): + site = Site.objects.get_current() + if site: + return site + + # Fallback to SITE_ID from settings + try: + return Site.objects.get(id=settings.SITE_ID) + except Site.DoesNotExist: + pass + + # Last resort: get first site + try: + return Site.objects.first() + except Exception: # pylint: disable=broad-except + return None + + +def _parse_task_input(instructor_task: InstructorTask) -> dict: + """ + Parse and return the task input JSON from InstructorTask. + + Args: + instructor_task (InstructorTask): The InstructorTask instance + + Returns: + dict: Parsed task input or empty dict if parsing fails + """ + try: + return json.loads(instructor_task.task_input) + except (json.JSONDecodeError, ValueError): + return {} + + +def _get_action_display_name(action: str) -> str: + """ + Get the localized display name for an enrollment action. + + Args: + action (str): The enrollment action ('enroll' or 'unenroll') + + Returns: + str: Localized action name + """ + from django.utils.translation import gettext_lazy as _ + + return _("enrollment") if action == EnrollStatusChange.enroll else _("unenrollment") + + +def _build_enrollment_email_context( + course_key: CourseKey, + requester, + action: str, + task_result: dict, + site: Site | None, + task_input: dict, +) -> dict: + """ + Build the email context dictionary for enrollment completion email. + + Args: + course_key (CourseKey): The course key + requester (User): The user who initiated the task + action (str): The enrollment action + task_result (dict): Dictionary with task results + site (Site | None): The current site object + task_input (dict): Parsed task input dictionary + + Returns: + dict: Complete context for email template + """ + course = get_course_by_id(course_key) + site_name = configuration_helpers.get_value("SITE_NAME", settings.SITE_NAME) + secure = task_input.get("secure", True) + protocol = "https" if secure else "http" + + context = { + "action_name": _get_action_display_name(action), + "course_name": course.display_name_with_default, + "total_processed": task_result.get("total_processed", 0), + "successful": task_result.get("successful", 0), + "failed": task_result.get("failed", 0), + "user_name": requester.username, + "platform_name": settings.PLATFORM_NAME, + "course_url": f"{protocol}://{site_name}/courses/{course_key}/", + } + + # Add base template context + context.update(get_base_template_context(site)) + + return context + + +def send_enrollment_task_completion_email( + course_key: CourseKey, instructor_task: InstructorTask, action: str, task_result: dict +) -> None: + """ + Send a completion email to the user who initiated the enrollment batch task. + + Args: + course_key (CourseKey): The course key + instructor_task (InstructorTask): The InstructorTask object + action (str): The action (e.g., 'enroll', 'unenroll') + task_result (dict): Dictionary containing task completion results with keys: + - total_processed: Total number of students processed + - successful: Number of successful operations + - failed: Number of failed operations + """ + requester = instructor_task.requester + site = _get_current_site() + task_input = _parse_task_input(instructor_task) + user_language = get_user_preference(requester, LANGUAGE_KEY) + + # Build email context + user_context = _build_enrollment_email_context( + course_key=course_key, + requester=requester, + action=action, + task_result=task_result, + site=site, + task_input=task_input, + ) + + # Create and send message + message = BatchEnrollment().personalize( + recipient=Recipient(lms_user_id=requester.id, email_address=requester.email), + language=user_language, + user_context=user_context, + ) + + with emulate_http_request(site=site, user=requester): + ace.send(message) + + TASK_LOG.info( + "Enrollment task completion email sent via ACE to user %s (%s) for course %s. " + "Action: %s, Results: %d successful, %d failed out of %d total", + requester.username, + requester.email, + course_key, + action, + user_context["successful"], + user_context["failed"], + user_context["total_processed"], + ) diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index a17551115a59..5baee74adbbc 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -31,6 +31,7 @@ from lms.djangoapps.instructor_task.tasks_base import BaseInstructorTask from lms.djangoapps.instructor_task.tasks_helper.certs import generate_students_certificates from lms.djangoapps.instructor_task.tasks_helper.enrollments import ( + process_student_enrollment_batch, upload_inactive_enrolled_students_info_csv, upload_may_enroll_csv, upload_students_csv, @@ -359,3 +360,14 @@ def export_ora2_summary(entry_id, xblock_instance_args): action_name = gettext_noop('generated') task_fn = partial(upload_ora2_summary, xblock_instance_args) return run_main_task(entry_id, task_fn, action_name) + + +@shared_task(base=BaseInstructorTask) +@set_code_owner_attribute +def student_enrollment_batch(entry_id, xblock_instance_args): + """ + Process student enrollment/unenrollment operations in batch asynchronously. + """ + action_name = gettext_noop('processed') + task_fn = partial(process_student_enrollment_batch, xblock_instance_args) + return run_main_task(entry_id, task_fn, action_name) diff --git a/lms/djangoapps/instructor_task/tasks_helper/enrollments.py b/lms/djangoapps/instructor_task/tasks_helper/enrollments.py index 889f52a3061b..789bb62d0978 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/enrollments.py +++ b/lms/djangoapps/instructor_task/tasks_helper/enrollments.py @@ -2,23 +2,27 @@ Instructor tasks related to enrollments. """ - import logging from datetime import datetime from time import time +from typing import Any +from opaque_keys.edx.keys import CourseKey from pytz import UTC -from common.djangoapps.student.models import CourseEnrollment # pylint: disable=unused-import +from common.djangoapps.student.models import CourseEnrollment +from lms.djangoapps.instructor.utils import process_student_enrollment_batch as process_batch from lms.djangoapps.instructor_analytics.basic import ( enrolled_students_features, list_inactive_enrolled_students, list_may_enroll, ) from lms.djangoapps.instructor_analytics.csvs import format_dictlist +from lms.djangoapps.instructor_task.models import InstructorTask +from lms.djangoapps.instructor_task.notifications import send_enrollment_task_completion_email from .runner import TaskProgress -from .utils import upload_csv_to_report_store # pylint: disable=unused-import +from .utils import upload_csv_to_report_store TASK_LOG = logging.getLogger('edx.celery.task') FILTERED_OUT_ROLES = ['staff', 'instructor', 'finance_admin', 'sales_admin'] @@ -121,3 +125,95 @@ def upload_students_csv(_xblock_instance_args, _entry_id, course_id, task_input, upload_csv_to_report_store(rows, upload_filename, course_id, start_date, parent_dir=upload_parent_dir) return task_progress.update_task_state(extra_meta=current_step) + + +def process_student_enrollment_batch( + _xblock_instance_args: Any, + _entry_id: int, + course_id: str | CourseKey, + task_input: dict, + action_name: str, +) -> dict: + """ + Process a batch of student enrollment/unenrollment operations asynchronously. + + Args: + _xblock_instance_args: XBlock instance arguments (unused) + _entry_id: The primary key for the InstructorTask entry + course_id: The course identifier (string or CourseKey) + task_input: Dictionary containing: + - action: 'enroll' or 'unenroll' + - identifiers: list of student identifiers (emails or usernames) + - auto_enroll: boolean for auto-enrollment + - email_students: boolean to send enrollment emails + - reason: optional reason for enrollment change + - secure: boolean indicating if request was secure (HTTPS) + action_name: Name of the action being performed + + Returns: + dict: Task progress dictionary with results of enrollment operations + """ + instructor_task = InstructorTask.objects.get(pk=_entry_id) + start_time = time() + start_date = datetime.now(UTC) + + action = task_input.get("action") + identifiers = task_input.get("identifiers", []) + course_key = CourseKey.from_string(course_id) if isinstance(course_id, str) else course_id + total_students = len(identifiers) + task_progress = TaskProgress(action_name, total_students, start_time) + + current_step = {"step": f"Processing {action} operations for {total_students} students"} + task_progress.update_task_state(extra_meta=current_step) + + def progress_callback(current: int, total: int, results: list[dict]) -> None: + """Update task progress for enrollment batch operations.""" + task_progress.attempted = current + task_progress.succeeded = sum(1 for r in results if r.get("success", False)) + task_progress.failed = current - task_progress.succeeded + + # Update progress every 10 operations or at the end + if current % 10 == 0 or current == total: + current_step = { + "step": f"Processed {current}/{total} {action} operations", + "succeeded": task_progress.succeeded, + "failed": task_progress.failed, + } + task_progress.update_task_state(extra_meta=current_step) + + batch_result = process_batch( + request_user=instructor_task.requester, + course_key=course_key, + action=action, + identifiers=identifiers, + auto_enroll=task_input.get("auto_enroll", False), + email_students=task_input.get("email_students", False), + reason=task_input.get("reason"), + secure=task_input.get("secure", False), + progress_callback=progress_callback, + ) + + task_progress.attempted = batch_result["total_students"] + task_progress.succeeded = batch_result["successful_operations"] + task_progress.failed = batch_result["failed_operations"] + task_progress.skipped = 0 + + final_step = { + "step": f"Completed {action} batch processing", + "total_processed": batch_result["total_students"], + "successful": batch_result["successful_operations"], + "failed": batch_result["failed_operations"], + } + + CSV_FIELDS = ["identifier", "success", "state_transition", "error_type", "error_message"] + CSV_DEFAULTS = {"identifier": "", "success": False, "state_transition": "", "error_type": "", "error_message": ""} + + def extract_csv_row(result: dict) -> list[str]: + """Extract CSV row data from result dictionary.""" + return [result.get(field, CSV_DEFAULTS[field]) for field in CSV_FIELDS] + + rows = [CSV_FIELDS] + [extract_csv_row(result) for result in batch_result["results"]] + upload_csv_to_report_store(rows, "enrollment_batch_results", course_id, start_date) + send_enrollment_task_completion_email(course_key, instructor_task, action, final_step) + + return task_progress.update_task_state(extra_meta=final_step) diff --git a/lms/static/js/instructor_dashboard/membership.js b/lms/static/js/instructor_dashboard/membership.js index ac5d63fbdc4c..c90606ad4f12 100644 --- a/lms/static/js/instructor_dashboard/membership.js +++ b/lms/static/js/instructor_dashboard/membership.js @@ -608,7 +608,9 @@ such that the value can be defined later than this assignment (file load order). this.$reason_field = this.$container.find("textarea[name='reason-field']"); this.$checkbox_autoenroll = this.$container.find("input[name='auto-enroll']"); this.$checkbox_emailstudents = this.$container.find("input[name='email-students']"); + this.$checkbox_asyncprocessing = this.$container.find("input[name='async-processing']"); this.checkbox_emailstudents_initialstate = this.$checkbox_emailstudents.is(':checked'); + this.checkbox_asyncprocessing_initialstate = this.$checkbox_asyncprocessing.is(':checked'); this.$task_response = this.$container.find('.request-response'); this.$request_response_error = this.$container.find('.request-response-error'); this.$enrollment_button.click(function(event) { @@ -624,6 +626,7 @@ such that the value can be defined later than this assignment (file load order). identifiers: batchEnroll.$identifier_input.val(), auto_enroll: batchEnroll.$checkbox_autoenroll.is(':checked'), email_students: emailStudents, + async_processing: batchEnroll.$checkbox_asyncprocessing.is(':checked'), reason: batchEnroll.$reason_field.val() }; return $.ajax({ @@ -645,6 +648,7 @@ such that the value can be defined later than this assignment (file load order). this.$identifier_input.val(''); this.$reason_field.val(''); this.$checkbox_emailstudents.attr('checked', this.checkbox_emailstudents_initialstate); + this.$checkbox_asyncprocessing.attr('checked', this.checkbox_asyncprocessing_initialstate); return this.$checkbox_autoenroll.attr('checked', true); }; @@ -655,6 +659,43 @@ such that the value can be defined later than this assignment (file load order). return this.$request_response_error.text(msg); }; + batchEnrollment.prototype.show_async_processing_message = function (dataFromServer) { + const { action: rawAction = 'process' } = dataFromServer; + + const actionsMap = { + enroll: { + text: gettext('enrollment'), + title: gettext('Enrollment Request Submitted') + }, + unenroll: { + text: gettext('unenrollment'), + title: gettext('Unenrollment Request Submitted') + }, + process: { + text: gettext('processing'), + title: gettext('Request Submitted') + } + }; + + const { text: actionText, title } = actionsMap[rawAction] || actionsMap.process; + + const message = interpolate( + gettext( + 'Your %(action)s request is being processed in the background. ' + + 'This may take several minutes to complete. You will receive an email ' + + 'notification when the process is finished.' + ), + { action: actionText }, + true + ); + + const $taskResSection = $('
', { class: 'request-res-section' }) + .append($('

', { text: title })) + .append($('
    ').append($('
  • ', { text: message }))); + + return this.$task_response.append($taskResSection); + }; + batchEnrollment.prototype.display_response = function(dataFromServer) { var allowed, autoenrolled, enrolled, errors, errorsLabel, invalidIdentifier, notenrolled, notunenrolled, renderList, sr, studentResults, @@ -671,6 +712,9 @@ such that the value can be defined later than this assignment (file load order). notenrolled = []; notunenrolled = []; ref = dataFromServer.results; + if (!ref) { + return this.show_async_processing_message(dataFromServer); + } for (i = 0, len = ref.length; i < len; i++) { studentResults = ref[i]; if (studentResults.invalidIdentifier) { diff --git a/lms/templates/instructor/edx_ace/batchenrollment/email/body.html b/lms/templates/instructor/edx_ace/batchenrollment/email/body.html new file mode 100644 index 000000000000..3ec977a9dd2d --- /dev/null +++ b/lms/templates/instructor/edx_ace/batchenrollment/email/body.html @@ -0,0 +1,56 @@ +{% extends 'ace_common/edx_ace/common/base_body.html' %} + +{% load i18n %} +{% load static %} +{% block content %} + + + + +
    +

    + {% autoescape off %} + {# xss-lint: disable=django-blocktrans-missing-escape-filter #} + {% blocktrans %}Batch {{ action_name }} completed for {{ course_name }}{% endblocktrans %} + {% endautoescape %} +

    + +

    + {% autoescape off %} + {# xss-lint: disable=django-blocktrans-missing-escape-filter #} + {% blocktrans %}The batch {{ action_name }} process for {{ course_name }} is complete.{% endblocktrans %} + {% endautoescape %} +
    +

    + +

    + {% autoescape off %} + {# xss-lint: disable=django-blocktrans-missing-escape-filter #} + {% blocktrans %}Out of {{ total_processed }} users:{% endblocktrans %} + {% endautoescape %} +
    +

    + +
    +
      +
    • + ✓ + {% filter force_escape %} + {% blocktrans %}{{ successful }} successfully processed{% endblocktrans %} + {% endfilter %} +
      • +
    • + ✗ + {% filter force_escape %} + {% blocktrans %}{{ failed }} failed to process{% endblocktrans %} + {% endfilter %} +
      • +
    +
    + + {% filter force_escape %} + {% blocktrans asvar course_cta_text %}Download the CSV report{% endblocktrans %} + {% endfilter %} + {% include "ace_common/edx_ace/common/return_to_course_cta.html" with course_cta_text=course_cta_text course_cta_url=course_url|add:"instructor#view-data_download" %} +
    +{% endblock %} diff --git a/lms/templates/instructor/edx_ace/batchenrollment/email/body.txt b/lms/templates/instructor/edx_ace/batchenrollment/email/body.txt new file mode 100644 index 000000000000..6183ead45b61 --- /dev/null +++ b/lms/templates/instructor/edx_ace/batchenrollment/email/body.txt @@ -0,0 +1,13 @@ +{% load i18n %} +{% autoescape off %} +{% blocktrans %}Batch {{ action_name }} completed for {{ course_name }}{% endblocktrans %} + +{% blocktrans %}The batch {{ action_name }} process for {{ course_name }} is complete.{% endblocktrans %} + +{% blocktrans %}Out of {{ total_processed }} users:{% endblocktrans %} + +✓ {% blocktrans %}{{ successful }} successfully processed{% endblocktrans %} +✗ {% blocktrans %}{{ failed }} failed to process{% endblocktrans %} + +{% blocktrans %}Download the CSV report{% endblocktrans %} +{% endautoescape %} diff --git a/lms/templates/instructor/edx_ace/batchenrollment/email/from_name.txt b/lms/templates/instructor/edx_ace/batchenrollment/email/from_name.txt new file mode 100644 index 000000000000..dcbc23c00480 --- /dev/null +++ b/lms/templates/instructor/edx_ace/batchenrollment/email/from_name.txt @@ -0,0 +1 @@ +{{ platform_name }} diff --git a/lms/templates/instructor/edx_ace/batchenrollment/email/head.html b/lms/templates/instructor/edx_ace/batchenrollment/email/head.html new file mode 100644 index 000000000000..366ada7ad92e --- /dev/null +++ b/lms/templates/instructor/edx_ace/batchenrollment/email/head.html @@ -0,0 +1 @@ +{% extends 'ace_common/edx_ace/common/base_head.html' %} diff --git a/lms/templates/instructor/edx_ace/batchenrollment/email/subject.txt b/lms/templates/instructor/edx_ace/batchenrollment/email/subject.txt new file mode 100644 index 000000000000..4ecd16bea64a --- /dev/null +++ b/lms/templates/instructor/edx_ace/batchenrollment/email/subject.txt @@ -0,0 +1,2 @@ +{% load i18n %} +{% blocktrans %}Batch {{ action_name }} completed for {{ course_name }}{% endblocktrans %} diff --git a/lms/templates/instructor/edx_ace/batchenrollment/push/body.txt b/lms/templates/instructor/edx_ace/batchenrollment/push/body.txt new file mode 100644 index 000000000000..39af1c274c82 --- /dev/null +++ b/lms/templates/instructor/edx_ace/batchenrollment/push/body.txt @@ -0,0 +1,4 @@ +{% load i18n %} +{% autoescape off %} +{% blocktrans %}Batch {{ action_name }} completed: {{ successful }}/{{ total_processed }} users processed for {{ course_name }}. CSV report available.{% endblocktrans %} +{% endautoescape %} diff --git a/lms/templates/instructor/edx_ace/batchenrollment/push/title.txt b/lms/templates/instructor/edx_ace/batchenrollment/push/title.txt new file mode 100644 index 000000000000..4ecd16bea64a --- /dev/null +++ b/lms/templates/instructor/edx_ace/batchenrollment/push/title.txt @@ -0,0 +1,2 @@ +{% load i18n %} +{% blocktrans %}Batch {{ action_name }} completed for {{ course_name }}{% endblocktrans %} diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 9b1f64dfb9d1..d2721470a8db 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -49,6 +49,21 @@

+
+ +
diff --git a/openedx/core/djangoapps/ace_common/templatetags/ace.py b/openedx/core/djangoapps/ace_common/templatetags/ace.py index 085f29def784..70177b8d8d25 100644 --- a/openedx/core/djangoapps/ace_common/templatetags/ace.py +++ b/openedx/core/djangoapps/ace_common/templatetags/ace.py @@ -96,9 +96,11 @@ def google_analytics_tracking_pixel(context): def _get_google_analytics_tracking_url(context): site, user, message = _get_variables_from_context(context, 'google_analytics_tracking_pixel') + user_id = user.id if user else None + pixel = GoogleAnalyticsTrackingPixel( site=site, - user_id=user.id, + user_id=user_id, campaign_source=message.app_label, campaign_name=message.name, campaign_content=message.uuid, From 408e6c6c74d759e013b990ee22bfa29965b8a492 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 25 Nov 2025 19:03:20 -0500 Subject: [PATCH 02/19] feat: add tests for submit_student_enrollment_batch api function --- .../instructor_task/tests/test_api.py | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 321414041a89..6f5349527cbc 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -3,7 +3,9 @@ """ import datetime +import hashlib import json +from unittest import mock from unittest.mock import MagicMock, Mock, patch from uuid import uuid4 @@ -11,6 +13,8 @@ import pytest import pytz from celery.states import FAILURE, SUCCESS +from django.http import HttpRequest +from opaque_keys.edx.keys import CourseKey from testfixtures import LogCapture from common.djangoapps.student.tests.factories import UserFactory @@ -43,6 +47,7 @@ submit_rescore_problem_for_student, submit_reset_problem_attempts_for_all_students, submit_reset_problem_attempts_in_entrance_exam, + submit_student_enrollment_batch, ) from lms.djangoapps.instructor_task.api_helper import AlreadyRunningError, QueueConnectionError from lms.djangoapps.instructor_task.data import InstructorTaskTypes @@ -51,6 +56,7 @@ export_ora2_data, export_ora2_submission_files, generate_anonymous_ids_for_course, + student_enrollment_batch, ) from lms.djangoapps.instructor_task.tests.factories import InstructorTaskFactory, InstructorTaskScheduleFactory from lms.djangoapps.instructor_task.tests.test_base import ( @@ -530,3 +536,98 @@ def test_process_scheduled_tasks_expect_error(self, mock_scheduled_task): process_scheduled_instructor_tasks() log.check_present((LOG_PATH, "ERROR", expected_messages[0]),) + + +class SubmitStudentEnrollmentBatchTests(InstructorTaskCourseTestCase): + """ + Tests for the submit_student_enrollment_batch API function. + """ + + def setUp(self): + self.request = HttpRequest() + self.course_key = CourseKey.from_string("course-v1:edX+DemoX+2025") + + @mock.patch("lms.djangoapps.instructor_task.api.submit_task") + def test_basic_submission(self, mock_submit_task): + """ + Basic test with <= 5 identifiers. + Verifies: task_input, task_type, task_class, task_key. + """ + identifiers = ["u1", "u2", "username3@example.com"] + action = "enroll" + mock_submit_task.return_value = "task-result" + expected_input = { + "action": action, + "identifiers": identifiers, + "auto_enroll": True, + "email_students": False, + "reason": "test", + "secure": True, + } + + result = submit_student_enrollment_batch( + request=self.request, + course_key=self.course_key, + action=action, + identifiers=identifiers, + auto_enroll=True, + email_students=False, + reason="test", + secure=True, + ) + + self.assertEqual(result, "task-result") + key_stub = f"{self.course_key}_{action}_{'_'.join(identifiers)}" + expected_key = hashlib.md5(key_stub.encode("utf-8")).hexdigest() + mock_submit_task.assert_called_once_with( + self.request, + InstructorTaskTypes.STUDENT_ENROLLMENT_BATCH, + student_enrollment_batch, + self.course_key, + expected_input, + expected_key, + ) + + @mock.patch("lms.djangoapps.instructor_task.api.submit_task") + def test_identifiers_are_truncated_to_5_for_key(self, mock_submit_task): + """ + When identifiers > 5, only first 5 go into the task_key. + """ + identifiers = ["u1", "u2", "u3", "u4", "u5", "u6", "u7"] + + submit_student_enrollment_batch( + request=self.request, + course_key=self.course_key, + action="unenroll", + identifiers=identifiers, + auto_enroll=False, + email_students=True, + reason=None, + secure=False, + ) + + truncated = identifiers[:5] + key_stub = f"{self.course_key}_unenroll_{'_'.join(truncated)}" + expected_key = hashlib.md5(key_stub.encode("utf-8")).hexdigest() + call_args = mock_submit_task.call_args[0] + received_key = call_args[5] + self.assertEqual(received_key, expected_key) + + @mock.patch("lms.djangoapps.instructor_task.api.submit_task") + def test_already_running_error_is_propagated(self, mock_submit_task): + """ + submit_task may raise AlreadyRunningError; our function should not swallow it. + """ + mock_submit_task.side_effect = AlreadyRunningError("Task already running") + + with self.assertRaises(AlreadyRunningError): + submit_student_enrollment_batch( + request=self.request, + course_key=self.course_key, + action="enroll", + identifiers=["john"], + auto_enroll=False, + email_students=False, + reason=None, + secure=False, + ) From 847c02a73248e14d45df3ca0176c26d1ae1b3fd9 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 25 Nov 2025 19:34:38 -0500 Subject: [PATCH 03/19] feat: add tests for async enrollment and unenrollment processes --- lms/djangoapps/instructor/tests/test_api.py | 200 ++++++++++++++++++++ 1 file changed, 200 insertions(+) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index f314506a5994..2b9b83ed1106 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1904,6 +1904,206 @@ def test_get_enrollment_status(self): res_json = json.loads(response.content.decode('utf-8')) assert res_json['enrollment_status'] == 'Enrollment status for nonotever@example.com: never enrolled' + @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") + def test_enroll_async_processing_success(self, mock_submit_task): + """Test async enrollment with async_processing=True""" + mock_task = Mock() + mock_task.task_id = "test-task-id-123" + mock_task.task_state = "QUEUED" + mock_submit_task.return_value = mock_task + url = reverse("students_update_enrollment", kwargs={"course_id": str(self.course.id)}) + + response = self.client.post( + url, + { + "identifiers": self.notenrolled_student.email, + "action": "enroll", + "email_students": False, + "async_processing": True, + }, + ) + + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content.decode("utf-8")) + + # Verify async response structure + self.assertEqual(res_json, { + "action": "enroll", + "auto_enroll": False, + "async_processing": True, + "task_id": "test-task-id-123", + "task_state": "QUEUED", + "message": "Async enroll task submitted for 1 students", + "total_students": 1 + }) + + # Verify the task was called with correct parameters + self.assertTrue(mock_submit_task.called) + call_args = mock_submit_task.call_args + self.assertEqual(call_args[1]["course_key"], self.course.id) + self.assertEqual(call_args[1]["action"], "enroll") + self.assertEqual(call_args[1]["identifiers"], [self.notenrolled_student.email]) + + @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") + def test_unenroll_async_processing_success(self, mock_submit_task): + """Test async unenrollment with async_processing=True""" + mock_task = Mock() + mock_task.task_id = "test-unenroll-task-456" + mock_task.task_state = "QUEUED" + mock_submit_task.return_value = mock_task + url = reverse("students_update_enrollment", kwargs={"course_id": str(self.course.id)}) + + response = self.client.post( + url, + { + "identifiers": self.enrolled_student.email, + "action": "unenroll", + "email_students": True, + "async_processing": True, + }, + ) + + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content.decode("utf-8")) + + self.assertEqual(res_json, { + "action": "unenroll", + "auto_enroll": False, + "async_processing": True, + "task_id": "test-unenroll-task-456", + "task_state": "QUEUED", + "message": "Async unenroll task submitted for 1 students", + "total_students": 1 + }) + + @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") + def test_async_enrollment_already_running_error(self, mock_submit_task): + """Test handling of AlreadyRunningError in async mode""" + mock_submit_task.side_effect = AlreadyRunningError("Task already running") + url = reverse("students_update_enrollment", kwargs={"course_id": str(self.course.id)}) + + response = self.client.post( + url, {"identifiers": self.notenrolled_student.email, "action": "enroll", "async_processing": True} + ) + + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content.decode("utf-8")) + self.assertTrue(res_json["async_processing"]) + self.assertIn("error", res_json) + self.assertIn("already running", res_json["error"].lower()) + self.assertEqual(res_json["action"], "enroll") + + @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") + def test_async_enrollment_multiple_identifiers(self, mock_submit_task): + """Test async enrollment with multiple student identifiers""" + student2 = UserFactory(username="Student2", email="student2@example.com") + student3 = UserFactory(username="Student3", email="student3@example.com") + mock_task = Mock() + mock_task.task_id = "test-bulk-task-789" + mock_task.task_state = "QUEUED" + mock_submit_task.return_value = mock_task + identifiers = f"{self.notenrolled_student.email},{student2.email},{student3.email}" + url = reverse("students_update_enrollment", kwargs={"course_id": str(self.course.id)}) + + response = self.client.post( + url, + { + "identifiers": identifiers, + "action": "enroll", + "email_students": False, + "async_processing": True, + "auto_enroll": True, + }, + ) + + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content.decode("utf-8")) + self.assertEqual(res_json["total_students"], 3) + self.assertTrue(res_json["auto_enroll"]) + + # Verify task was called with all identifiers + call_args = mock_submit_task.call_args + identifiers_list = call_args[1]["identifiers"] + self.assertEqual(len(identifiers_list), 3) + self.assertIn(self.notenrolled_student.email, identifiers_list) + self.assertIn(student2.email, identifiers_list) + self.assertIn(student3.email, identifiers_list) + + def test_async_processing_default_false(self): + """Test that async_processing defaults to False for backward compatibility""" + url = reverse("students_update_enrollment", kwargs={"course_id": str(self.course.id)}) + + response = self.client.post( + url, + { + "identifiers": self.notenrolled_student.email, + "action": "enroll", + "email_students": False, + # async_processing not provided + }, + ) + + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content.decode("utf-8")) + + # Should have sync response structure (with 'results') + self.assertIn("results", res_json) + self.assertNotIn("async_processing", res_json) + self.assertEqual(res_json["action"], "enroll") + + @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") + def test_async_enrollment_with_reason(self, mock_submit_task): + """Test async enrollment with reason field""" + mock_task = Mock() + mock_task.task_id = "test-task-with-reason" + mock_task.task_state = "QUEUED" + mock_submit_task.return_value = mock_task + url = reverse("students_update_enrollment", kwargs={"course_id": str(self.course.id)}) + + response = self.client.post( + url, + { + "identifiers": self.notenrolled_student.email, + "action": "enroll", + "email_students": False, + "async_processing": True, + "reason": "Testing async enrollment", + }, + ) + + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content.decode("utf-8")) + self.assertTrue(res_json["async_processing"]) + + # Verify reason was passed to task + call_args = mock_submit_task.call_args + self.assertEqual(call_args[1]["reason"], "Testing async enrollment") + + def test_sync_enrollment_still_works(self): + """Test that synchronous enrollment still works (async_processing=False)""" + url = reverse("students_update_enrollment", kwargs={"course_id": str(self.course.id)}) + + response = self.client.post( + url, + { + "identifiers": self.notenrolled_student.email, + "action": "enroll", + "email_students": False, + "async_processing": False, + }, + ) + + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content.decode("utf-8")) + + # Should have sync response structure + self.assertIn("results", res_json) + self.assertEqual(len(res_json["results"]), 1) + self.assertTrue(res_json["results"][0]["success"]) + + # Verify actual enrollment happened + self.assertTrue(CourseEnrollment.is_enrolled(self.notenrolled_student, self.course.id)) + @ddt.ddt class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTestCase): From c62669da24974b1f9f98e9739af9ef1a3e01c15f Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 26 Nov 2025 10:40:48 -0500 Subject: [PATCH 04/19] feat: add unit tests for student enrollment utility functions --- lms/djangoapps/instructor/tests/test_utils.py | 430 ++++++++++++++++++ 1 file changed, 430 insertions(+) create mode 100644 lms/djangoapps/instructor/tests/test_utils.py diff --git a/lms/djangoapps/instructor/tests/test_utils.py b/lms/djangoapps/instructor/tests/test_utils.py new file mode 100644 index 000000000000..31680ee94384 --- /dev/null +++ b/lms/djangoapps/instructor/tests/test_utils.py @@ -0,0 +1,430 @@ +""" +Unit tests for instructor.utils module. + +Tests for student enrollment utility functions that can be used +in both synchronous and asynchronous contexts. +""" + +from unittest.mock import Mock, patch + +import ddt +from django.core.exceptions import ValidationError +from django.test import TestCase +from opaque_keys.edx.keys import CourseKey + +from common.djangoapps.student.models import ( + ALLOWEDTOENROLL_TO_ENROLLED, + ALLOWEDTOENROLL_TO_UNENROLLED, + DEFAULT_TRANSITION_STATE, + ENROLLED_TO_ENROLLED, + ENROLLED_TO_UNENROLLED, + UNENROLLED_TO_ALLOWEDTOENROLL, + UNENROLLED_TO_ENROLLED, + UNENROLLED_TO_UNENROLLED, + EnrollStatusChange, +) +from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.instructor.enrollment import EmailEnrollmentState +from lms.djangoapps.instructor.utils import ( + _determine_enroll_state_transition, + _determine_unenroll_state_transition, + process_single_student_enrollment, + process_student_enrollment_batch, +) + + +# @ddt.ddt +# class TestDetermineEnrollStateTransition(TestCase): +# """ +# Test the _determine_enroll_state_transition function. +# """ + +# @ddt.data( +# # User not registered, allowed to enroll +# ( +# {"user": False, "enrollment": False, "allowed": False}, +# {"enrollment": False, "allowed": True}, +# UNENROLLED_TO_ALLOWEDTOENROLL, +# ), +# # User not registered, not allowed +# ( +# {"user": False, "enrollment": False, "allowed": False}, +# {"enrollment": False, "allowed": False}, +# DEFAULT_TRANSITION_STATE, +# ), +# # User registered, was enrolled, still enrolled +# ( +# {"user": True, "enrollment": True, "allowed": False}, +# {"enrollment": True, "allowed": False}, +# ENROLLED_TO_ENROLLED, +# ), +# # User registered, was not enrolled, now enrolled +# ( +# {"user": True, "enrollment": False, "allowed": False}, +# {"enrollment": True, "allowed": False}, +# UNENROLLED_TO_ENROLLED, +# ), +# # User registered, was allowed, now enrolled +# ( +# {"user": True, "enrollment": False, "allowed": True}, +# {"enrollment": True, "allowed": False}, +# ALLOWEDTOENROLL_TO_ENROLLED, +# ), +# # User registered, not enrolled +# ( +# {"user": True, "enrollment": False, "allowed": False}, +# {"enrollment": False, "allowed": False}, +# DEFAULT_TRANSITION_STATE, +# ), +# ) +# @ddt.unpack +# def test_determine_enroll_state_transition(self, before_state: dict, after_state: dict, expected_transition: str): +# """Test state transition determination for enrollment.""" +# result = _determine_enroll_state_transition(before_state, after_state) + +# self.assertEqual(result, expected_transition) + + +# @ddt.ddt +# class TestDetermineUnenrollStateTransition(TestCase): +# """ +# Test the _determine_unenroll_state_transition function. +# """ + +# @ddt.data( +# # User was enrolled +# ( +# {"enrollment": True, "allowed": False}, +# ENROLLED_TO_UNENROLLED, +# ), +# # User was allowed to enroll +# ( +# {"enrollment": False, "allowed": True}, +# ALLOWEDTOENROLL_TO_UNENROLLED, +# ), +# # User was neither enrolled nor allowed +# ( +# {"enrollment": False, "allowed": False}, +# UNENROLLED_TO_UNENROLLED, +# ), +# ) +# @ddt.unpack +# def test_determine_unenroll_state_transition(self, before_state: dict, expected_transition: str): +# """Test state transition determination for unenrollment.""" +# result = _determine_unenroll_state_transition(before_state) + +# self.assertEqual(result, expected_transition) + + +class TestProcessSingleStudentEnrollment(TestCase): + """ + Test the process_single_student_enrollment function. + """ + + def setUp(self): + super().setUp() + self.course_key = CourseKey.from_string("course-v1:edX+DemoX+Demo_Course") + self.user = UserFactory.create(username="testuser", email="test@example.com") + self.request_user = UserFactory.create(username="instructor", email="instructor@example.com") + self.email_params = {"course_name": "Test Course", "course_url": "http://example.com/course"} + + @patch("lms.djangoapps.instructor.utils.enroll_email") + def test_process_single_student_enrollment_success(self, mock_enroll_email: Mock): + """Test successful enrollment of a single student.""" + before_state = EmailEnrollmentState(self.course_key, self.user.email) + before_state.enrollment = False + before_state.user = True + before_state.allowed = False + after_state = EmailEnrollmentState(self.course_key, self.user.email) + after_state.enrollment = True + after_state.user = True + after_state.allowed = False + mock_enroll_email.return_value = (before_state, after_state, None) + + result = process_single_student_enrollment( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifier=self.user.email, + auto_enroll=False, + email_students=True, + reason="Test enrollment", + email_params=self.email_params, + ) + + self.assertTrue(result["success"]) + self.assertEqual(result["identifier"], self.user.email) + self.assertEqual(result["state_transition"], UNENROLLED_TO_ENROLLED) + self.assertIn("before", result) + self.assertIn("after", result) + + @patch("lms.djangoapps.instructor.utils.unenroll_email") + def test_process_single_student_unenrollment_success(self, mock_unenroll_email: Mock): + """Test successful unenrollment of a single student.""" + before_state = EmailEnrollmentState(self.course_key, self.user.email) + before_state.enrollment = True + before_state.user = True + before_state.allowed = False + after_state = EmailEnrollmentState(self.course_key, self.user.email) + after_state.enrollment = False + after_state.user = True + after_state.allowed = False + mock_unenroll_email.return_value = (before_state, after_state) + + result = process_single_student_enrollment( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.unenroll, + identifier=self.user.email, + auto_enroll=False, + email_students=True, + reason="Test unenrollment", + email_params=self.email_params, + ) + + self.assertTrue(result["success"]) + self.assertEqual(result["identifier"], self.user.email) + self.assertEqual(result["state_transition"], ENROLLED_TO_UNENROLLED) + + @patch("lms.djangoapps.instructor.utils.validate_email") + def test_process_single_student_enrollment_invalid_email(self, mock_validate_email: Mock): + """Test enrollment with invalid email address.""" + mock_validate_email.side_effect = ValidationError("Invalid email") + + result = process_single_student_enrollment( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifier="invalid-email", + auto_enroll=False, + email_students=True, + reason="Test enrollment", + email_params=self.email_params, + ) + + self.assertFalse(result["success"]) + self.assertEqual(result["identifier"], "invalid-email") + self.assertTrue(result["invalidIdentifier"]) + self.assertEqual(result["error_type"], "invalid_identifier") + self.assertEqual(result["error_message"], "Invalid email address") + + @patch("lms.djangoapps.instructor.utils.enroll_email") + def test_process_single_student_enrollment_general_error( + self, + mock_enroll_email: Mock, + ): + """Test enrollment with general exception.""" + mock_enroll_email.side_effect = Exception("Database error") + + result = process_single_student_enrollment( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifier=self.user.email, + auto_enroll=False, + email_students=True, + reason="Test enrollment", + email_params=self.email_params, + ) + + self.assertFalse(result["success"]) + self.assertEqual(result["identifier"], self.user.email) + self.assertTrue(result["error"]) + self.assertEqual(result["error_type"], "general_error") + self.assertEqual(result["error_message"], "Database error") + + +class TestProcessStudentEnrollmentBatch(TestCase): + """ + Test the process_student_enrollment_batch function. + """ + + def setUp(self): + super().setUp() + self.course_key = CourseKey.from_string("course-v1:edX+DemoX+Demo_Course") + self.user1 = UserFactory.create(username="testuser1", email="test1@example.com") + self.user2 = UserFactory.create(username="testuser2", email="test2@example.com") + self.request_user = UserFactory.create(username="instructor", email="instructor@example.com") + + @patch("lms.djangoapps.instructor.utils.get_course_by_id") + @patch("lms.djangoapps.instructor.utils.get_email_params") + @patch("lms.djangoapps.instructor.utils.process_single_student_enrollment") + def test_process_student_enrollment_batch_success( + self, mock_process_single: Mock, mock_get_course: Mock, mock_get_email_params: Mock + ): + """Test batch processing with all successful enrollments.""" + mock_get_course.return_value = Mock(display_name_with_default="Test Course") + mock_get_email_params.return_value = {"course_name": "Test Course"} + mock_process_single.side_effect = [ + { + "identifier": self.user1.email, + "success": True, + "before": {}, + "after": {}, + "state_transition": UNENROLLED_TO_ENROLLED, + }, + { + "identifier": self.user2.email, + "success": True, + "before": {}, + "after": {}, + "state_transition": UNENROLLED_TO_ENROLLED, + }, + ] + identifiers = [self.user1.email, self.user2.email] + + result = process_student_enrollment_batch( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifiers=identifiers, + auto_enroll=False, + email_students=True, + reason="Batch enrollment test", + secure=True, + ) + + self.assertEqual(result["action"], EnrollStatusChange.enroll) + self.assertFalse(result["auto_enroll"]) + self.assertEqual(result["total_students"], 2) + self.assertEqual(result["successful_operations"], 2) + self.assertEqual(result["failed_operations"], 0) + self.assertEqual(len(result["results"]), 2) + self.assertEqual(mock_process_single.call_count, 2) + + @patch("lms.djangoapps.instructor.utils.get_course_by_id") + @patch("lms.djangoapps.instructor.utils.get_email_params") + @patch("lms.djangoapps.instructor.utils.process_single_student_enrollment") + def test_process_student_enrollment_batch_mixed_results( + self, + mock_process_single: Mock, + mock_get_email_params: Mock, + mock_get_course: Mock, + ): + """Test batch processing with mixed success and failure.""" + mock_get_course.return_value = Mock(display_name_with_default="Test Course") + mock_get_email_params.return_value = {"course_name": "Test Course"} + mock_process_single.side_effect = [ + { + "identifier": self.user1.email, + "success": True, + "before": {}, + "after": {}, + "state_transition": UNENROLLED_TO_ENROLLED, + }, + { + "identifier": "invalid@", + "success": False, + "invalidIdentifier": True, + "error_type": "invalid_identifier", + "error_message": "Invalid email address", + }, + ] + identifiers = [self.user1.email, "invalid@"] + + result = process_student_enrollment_batch( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifiers=identifiers, + auto_enroll=False, + email_students=True, + reason="Batch enrollment test", + secure=True, + ) + + self.assertEqual(result["total_students"], 2) + self.assertEqual(result["successful_operations"], 1) + self.assertEqual(result["failed_operations"], 1) + self.assertEqual(len(result["results"]), 2) + + @patch("lms.djangoapps.instructor.utils.get_course_by_id") + @patch("lms.djangoapps.instructor.utils.get_email_params") + @patch("lms.djangoapps.instructor.utils.process_single_student_enrollment") + def test_process_student_enrollment_batch_with_progress_callback( + self, + mock_process_single: Mock, + mock_get_email_params: Mock, + mock_get_course: Mock, + ): + """Test batch processing with progress callback.""" + mock_get_course.return_value = Mock(display_name_with_default="Test Course") + mock_get_email_params.return_value = {"course_name": "Test Course"} + mock_process_single.return_value = { + "identifier": self.user1.email, + "success": True, + "before": {}, + "after": {}, + "state_transition": UNENROLLED_TO_ENROLLED, + } + identifiers = [self.user1.email, self.user2.email] + progress_calls = [] + + def progress_callback(current, total, results): + progress_calls.append((current, total, len(results))) + + result = process_student_enrollment_batch( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifiers=identifiers, + auto_enroll=False, + email_students=False, + reason=None, + secure=True, + progress_callback=progress_callback, + ) + + self.assertEqual(len(progress_calls), 2) + self.assertEqual(progress_calls[0], (1, 2, 1)) + self.assertEqual(progress_calls[1], (2, 2, 2)) + self.assertEqual(result["successful_operations"], 2) + + @patch("lms.djangoapps.instructor.utils.process_single_student_enrollment") + def test_process_student_enrollment_batch_no_email(self, mock_process_single: Mock): + """Test batch processing without sending emails.""" + mock_process_single.return_value = { + "identifier": self.user1.email, + "success": True, + "before": {}, + "after": {}, + "state_transition": UNENROLLED_TO_ENROLLED, + } + identifiers = [self.user1.email] + + result = process_student_enrollment_batch( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifiers=identifiers, + auto_enroll=False, + email_students=False, + reason=None, + secure=True, + ) + + self.assertEqual(result["successful_operations"], 1) + call_kwargs = mock_process_single.call_args[1] + self.assertEqual(call_kwargs["email_params"], {}) + + @patch("lms.djangoapps.instructor.utils.get_course_by_id") + @patch("lms.djangoapps.instructor.utils.get_email_params") + @patch("lms.djangoapps.instructor.utils.process_single_student_enrollment") + def test_process_student_enrollment_batch_empty_list(self, mock_process_single: Mock, _: Mock, __: Mock): + """Test batch processing with empty identifier list.""" + result = process_student_enrollment_batch( + request_user=self.request_user, + course_key=self.course_key, + action=EnrollStatusChange.enroll, + identifiers=[], + auto_enroll=False, + email_students=False, + reason=None, + secure=True, + ) + + self.assertEqual(result["total_students"], 0) + self.assertEqual(result["successful_operations"], 0) + self.assertEqual(result["failed_operations"], 0) + self.assertEqual(len(result["results"]), 0) + mock_process_single.assert_not_called() From 783e8fad40a1a5553b0ffaaee795fc6152631880 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 26 Nov 2025 10:41:35 -0500 Subject: [PATCH 05/19] refactor: update user identification method --- lms/djangoapps/instructor/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/utils.py b/lms/djangoapps/instructor/utils.py index 84df2fc5f823..2c8b371accd9 100644 --- a/lms/djangoapps/instructor/utils.py +++ b/lms/djangoapps/instructor/utils.py @@ -32,7 +32,7 @@ get_user_email_language, unenroll_email, ) -from lms.djangoapps.instructor.views.tools import get_student_from_identifier +from common.djangoapps.student.models import get_user_by_username_or_email from openedx.core.lib.courses import get_course_by_id log = logging.getLogger(__name__) @@ -125,7 +125,7 @@ def process_single_student_enrollment( language = None try: - identified_user = get_student_from_identifier(identifier) + identified_user = get_user_by_username_or_email(identifier) except User.DoesNotExist: email = identifier else: From e1f5f2df83d0425a7a3eed36a85d385f9ee409d3 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 26 Nov 2025 11:18:15 -0500 Subject: [PATCH 06/19] test: uncomment tests --- lms/djangoapps/instructor/tests/test_utils.py | 162 +++++++++--------- 1 file changed, 81 insertions(+), 81 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_utils.py b/lms/djangoapps/instructor/tests/test_utils.py index 31680ee94384..fbff46c9a5c4 100644 --- a/lms/djangoapps/instructor/tests/test_utils.py +++ b/lms/djangoapps/instructor/tests/test_utils.py @@ -33,87 +33,87 @@ ) -# @ddt.ddt -# class TestDetermineEnrollStateTransition(TestCase): -# """ -# Test the _determine_enroll_state_transition function. -# """ - -# @ddt.data( -# # User not registered, allowed to enroll -# ( -# {"user": False, "enrollment": False, "allowed": False}, -# {"enrollment": False, "allowed": True}, -# UNENROLLED_TO_ALLOWEDTOENROLL, -# ), -# # User not registered, not allowed -# ( -# {"user": False, "enrollment": False, "allowed": False}, -# {"enrollment": False, "allowed": False}, -# DEFAULT_TRANSITION_STATE, -# ), -# # User registered, was enrolled, still enrolled -# ( -# {"user": True, "enrollment": True, "allowed": False}, -# {"enrollment": True, "allowed": False}, -# ENROLLED_TO_ENROLLED, -# ), -# # User registered, was not enrolled, now enrolled -# ( -# {"user": True, "enrollment": False, "allowed": False}, -# {"enrollment": True, "allowed": False}, -# UNENROLLED_TO_ENROLLED, -# ), -# # User registered, was allowed, now enrolled -# ( -# {"user": True, "enrollment": False, "allowed": True}, -# {"enrollment": True, "allowed": False}, -# ALLOWEDTOENROLL_TO_ENROLLED, -# ), -# # User registered, not enrolled -# ( -# {"user": True, "enrollment": False, "allowed": False}, -# {"enrollment": False, "allowed": False}, -# DEFAULT_TRANSITION_STATE, -# ), -# ) -# @ddt.unpack -# def test_determine_enroll_state_transition(self, before_state: dict, after_state: dict, expected_transition: str): -# """Test state transition determination for enrollment.""" -# result = _determine_enroll_state_transition(before_state, after_state) - -# self.assertEqual(result, expected_transition) - - -# @ddt.ddt -# class TestDetermineUnenrollStateTransition(TestCase): -# """ -# Test the _determine_unenroll_state_transition function. -# """ - -# @ddt.data( -# # User was enrolled -# ( -# {"enrollment": True, "allowed": False}, -# ENROLLED_TO_UNENROLLED, -# ), -# # User was allowed to enroll -# ( -# {"enrollment": False, "allowed": True}, -# ALLOWEDTOENROLL_TO_UNENROLLED, -# ), -# # User was neither enrolled nor allowed -# ( -# {"enrollment": False, "allowed": False}, -# UNENROLLED_TO_UNENROLLED, -# ), -# ) -# @ddt.unpack -# def test_determine_unenroll_state_transition(self, before_state: dict, expected_transition: str): -# """Test state transition determination for unenrollment.""" -# result = _determine_unenroll_state_transition(before_state) - -# self.assertEqual(result, expected_transition) +@ddt.ddt +class TestDetermineEnrollStateTransition(TestCase): + """ + Test the _determine_enroll_state_transition function. + """ + + @ddt.data( + # User not registered, allowed to enroll + ( + {"user": False, "enrollment": False, "allowed": False}, + {"enrollment": False, "allowed": True}, + UNENROLLED_TO_ALLOWEDTOENROLL, + ), + # User not registered, not allowed + ( + {"user": False, "enrollment": False, "allowed": False}, + {"enrollment": False, "allowed": False}, + DEFAULT_TRANSITION_STATE, + ), + # User registered, was enrolled, still enrolled + ( + {"user": True, "enrollment": True, "allowed": False}, + {"enrollment": True, "allowed": False}, + ENROLLED_TO_ENROLLED, + ), + # User registered, was not enrolled, now enrolled + ( + {"user": True, "enrollment": False, "allowed": False}, + {"enrollment": True, "allowed": False}, + UNENROLLED_TO_ENROLLED, + ), + # User registered, was allowed, now enrolled + ( + {"user": True, "enrollment": False, "allowed": True}, + {"enrollment": True, "allowed": False}, + ALLOWEDTOENROLL_TO_ENROLLED, + ), + # User registered, not enrolled + ( + {"user": True, "enrollment": False, "allowed": False}, + {"enrollment": False, "allowed": False}, + DEFAULT_TRANSITION_STATE, + ), + ) + @ddt.unpack + def test_determine_enroll_state_transition(self, before_state: dict, after_state: dict, expected_transition: str): + """Test state transition determination for enrollment.""" + result = _determine_enroll_state_transition(before_state, after_state) + + self.assertEqual(result, expected_transition) + + +@ddt.ddt +class TestDetermineUnenrollStateTransition(TestCase): + """ + Test the _determine_unenroll_state_transition function. + """ + + @ddt.data( + # User was enrolled + ( + {"enrollment": True, "allowed": False}, + ENROLLED_TO_UNENROLLED, + ), + # User was allowed to enroll + ( + {"enrollment": False, "allowed": True}, + ALLOWEDTOENROLL_TO_UNENROLLED, + ), + # User was neither enrolled nor allowed + ( + {"enrollment": False, "allowed": False}, + UNENROLLED_TO_UNENROLLED, + ), + ) + @ddt.unpack + def test_determine_unenroll_state_transition(self, before_state: dict, expected_transition: str): + """Test state transition determination for unenrollment.""" + result = _determine_unenroll_state_transition(before_state) + + self.assertEqual(result, expected_transition) class TestProcessSingleStudentEnrollment(TestCase): From fa4d1c2b62733925010502cc4ed5a78be9ce2ebf Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 22 Apr 2026 08:51:16 -0500 Subject: [PATCH 07/19] fix: satisfy ruff and pylint rules --- lms/djangoapps/instructor/tests/test_api.py | 87 ++++++++++--------- lms/djangoapps/instructor/tests/test_utils.py | 86 +++++++++--------- lms/djangoapps/instructor/utils.py | 6 +- .../instructor_task/tests/test_api.py | 6 +- 4 files changed, 93 insertions(+), 92 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 2b9b83ed1106..e0b68549f6ca 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1923,11 +1923,11 @@ def test_enroll_async_processing_success(self, mock_submit_task): }, ) - self.assertEqual(response.status_code, 200) + assert response.status_code == 200 res_json = json.loads(response.content.decode("utf-8")) # Verify async response structure - self.assertEqual(res_json, { + assert res_json == { "action": "enroll", "auto_enroll": False, "async_processing": True, @@ -1935,14 +1935,14 @@ def test_enroll_async_processing_success(self, mock_submit_task): "task_state": "QUEUED", "message": "Async enroll task submitted for 1 students", "total_students": 1 - }) + } # Verify the task was called with correct parameters - self.assertTrue(mock_submit_task.called) + assert mock_submit_task.called call_args = mock_submit_task.call_args - self.assertEqual(call_args[1]["course_key"], self.course.id) - self.assertEqual(call_args[1]["action"], "enroll") - self.assertEqual(call_args[1]["identifiers"], [self.notenrolled_student.email]) + assert call_args[1]["course_key"] == self.course.id + assert call_args[1]["action"] == "enroll" + assert call_args[1]["identifiers"] == [self.notenrolled_student.email] @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") def test_unenroll_async_processing_success(self, mock_submit_task): @@ -1963,18 +1963,21 @@ def test_unenroll_async_processing_success(self, mock_submit_task): }, ) - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 200) # noqa: PT009 res_json = json.loads(response.content.decode("utf-8")) - self.assertEqual(res_json, { - "action": "unenroll", - "auto_enroll": False, - "async_processing": True, - "task_id": "test-unenroll-task-456", - "task_state": "QUEUED", - "message": "Async unenroll task submitted for 1 students", - "total_students": 1 - }) + self.assertEqual( # noqa: PT009 + res_json, + { + "action": "unenroll", + "auto_enroll": False, + "async_processing": True, + "task_id": "test-unenroll-task-456", + "task_state": "QUEUED", + "message": "Async unenroll task submitted for 1 students", + "total_students": 1, + }, + ) @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") def test_async_enrollment_already_running_error(self, mock_submit_task): @@ -1986,12 +1989,12 @@ def test_async_enrollment_already_running_error(self, mock_submit_task): url, {"identifiers": self.notenrolled_student.email, "action": "enroll", "async_processing": True} ) - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 200) # noqa: PT009 res_json = json.loads(response.content.decode("utf-8")) - self.assertTrue(res_json["async_processing"]) - self.assertIn("error", res_json) - self.assertIn("already running", res_json["error"].lower()) - self.assertEqual(res_json["action"], "enroll") + self.assertTrue(res_json["async_processing"]) # noqa: PT009 + self.assertIn("error", res_json) # noqa: PT009 + self.assertIn("already running", res_json["error"].lower()) # noqa: PT009 + self.assertEqual(res_json["action"], "enroll") # noqa: PT009 @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") def test_async_enrollment_multiple_identifiers(self, mock_submit_task): @@ -2016,18 +2019,18 @@ def test_async_enrollment_multiple_identifiers(self, mock_submit_task): }, ) - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 200) # noqa: PT009 res_json = json.loads(response.content.decode("utf-8")) - self.assertEqual(res_json["total_students"], 3) - self.assertTrue(res_json["auto_enroll"]) + self.assertEqual(res_json["total_students"], 3) # noqa: PT009 + self.assertTrue(res_json["auto_enroll"]) # noqa: PT009 # Verify task was called with all identifiers call_args = mock_submit_task.call_args identifiers_list = call_args[1]["identifiers"] - self.assertEqual(len(identifiers_list), 3) - self.assertIn(self.notenrolled_student.email, identifiers_list) - self.assertIn(student2.email, identifiers_list) - self.assertIn(student3.email, identifiers_list) + self.assertEqual(len(identifiers_list), 3) # noqa: PT009 + self.assertIn(self.notenrolled_student.email, identifiers_list) # noqa: PT009 + self.assertIn(student2.email, identifiers_list) # noqa: PT009 + self.assertIn(student3.email, identifiers_list) # noqa: PT009 def test_async_processing_default_false(self): """Test that async_processing defaults to False for backward compatibility""" @@ -2043,13 +2046,13 @@ def test_async_processing_default_false(self): }, ) - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 200) # noqa: PT009 res_json = json.loads(response.content.decode("utf-8")) # Should have sync response structure (with 'results') - self.assertIn("results", res_json) - self.assertNotIn("async_processing", res_json) - self.assertEqual(res_json["action"], "enroll") + self.assertIn("results", res_json) # noqa: PT009 + self.assertNotIn("async_processing", res_json) # noqa: PT009 + self.assertEqual(res_json["action"], "enroll") # noqa: PT009 @patch("lms.djangoapps.instructor_task.api.submit_student_enrollment_batch") def test_async_enrollment_with_reason(self, mock_submit_task): @@ -2071,13 +2074,13 @@ def test_async_enrollment_with_reason(self, mock_submit_task): }, ) - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 200) # noqa: PT009 res_json = json.loads(response.content.decode("utf-8")) - self.assertTrue(res_json["async_processing"]) + self.assertTrue(res_json["async_processing"]) # noqa: PT009 # Verify reason was passed to task call_args = mock_submit_task.call_args - self.assertEqual(call_args[1]["reason"], "Testing async enrollment") + self.assertEqual(call_args[1]["reason"], "Testing async enrollment") # noqa: PT009 def test_sync_enrollment_still_works(self): """Test that synchronous enrollment still works (async_processing=False)""" @@ -2093,16 +2096,16 @@ def test_sync_enrollment_still_works(self): }, ) - self.assertEqual(response.status_code, 200) - res_json = json.loads(response.content.decode("utf-8")) + self.assertEqual(response.status_code, 200) # noqa: PT009 + res_json = json.loads(response.content.decode("utf-8")) # noqa: PT009 # Should have sync response structure - self.assertIn("results", res_json) - self.assertEqual(len(res_json["results"]), 1) - self.assertTrue(res_json["results"][0]["success"]) + self.assertIn("results", res_json) # noqa: PT009 + self.assertEqual(len(res_json["results"]), 1) # noqa: PT009 + self.assertTrue(res_json["results"][0]["success"]) # noqa: PT009 # Verify actual enrollment happened - self.assertTrue(CourseEnrollment.is_enrolled(self.notenrolled_student, self.course.id)) + self.assertTrue(CourseEnrollment.is_enrolled(self.notenrolled_student, self.course.id)) # noqa: PT009 @ddt.ddt diff --git a/lms/djangoapps/instructor/tests/test_utils.py b/lms/djangoapps/instructor/tests/test_utils.py index fbff46c9a5c4..008a70a3cb33 100644 --- a/lms/djangoapps/instructor/tests/test_utils.py +++ b/lms/djangoapps/instructor/tests/test_utils.py @@ -82,7 +82,7 @@ def test_determine_enroll_state_transition(self, before_state: dict, after_state """Test state transition determination for enrollment.""" result = _determine_enroll_state_transition(before_state, after_state) - self.assertEqual(result, expected_transition) + self.assertEqual(result, expected_transition) # noqa: PT009 @ddt.ddt @@ -113,7 +113,7 @@ def test_determine_unenroll_state_transition(self, before_state: dict, expected_ """Test state transition determination for unenrollment.""" result = _determine_unenroll_state_transition(before_state) - self.assertEqual(result, expected_transition) + self.assertEqual(result, expected_transition) # noqa: PT009 class TestProcessSingleStudentEnrollment(TestCase): @@ -152,11 +152,11 @@ def test_process_single_student_enrollment_success(self, mock_enroll_email: Mock email_params=self.email_params, ) - self.assertTrue(result["success"]) - self.assertEqual(result["identifier"], self.user.email) - self.assertEqual(result["state_transition"], UNENROLLED_TO_ENROLLED) - self.assertIn("before", result) - self.assertIn("after", result) + self.assertTrue(result["success"]) # noqa: PT009 + self.assertEqual(result["identifier"], self.user.email) # noqa: PT009 + self.assertEqual(result["state_transition"], UNENROLLED_TO_ENROLLED) # noqa: PT009 + self.assertIn("before", result) # noqa: PT009 + self.assertIn("after", result) # noqa: PT009 @patch("lms.djangoapps.instructor.utils.unenroll_email") def test_process_single_student_unenrollment_success(self, mock_unenroll_email: Mock): @@ -182,9 +182,9 @@ def test_process_single_student_unenrollment_success(self, mock_unenroll_email: email_params=self.email_params, ) - self.assertTrue(result["success"]) - self.assertEqual(result["identifier"], self.user.email) - self.assertEqual(result["state_transition"], ENROLLED_TO_UNENROLLED) + self.assertTrue(result["success"]) # noqa: PT009 + self.assertEqual(result["identifier"], self.user.email) # noqa: PT009 + self.assertEqual(result["state_transition"], ENROLLED_TO_UNENROLLED) # noqa: PT009 @patch("lms.djangoapps.instructor.utils.validate_email") def test_process_single_student_enrollment_invalid_email(self, mock_validate_email: Mock): @@ -202,11 +202,11 @@ def test_process_single_student_enrollment_invalid_email(self, mock_validate_ema email_params=self.email_params, ) - self.assertFalse(result["success"]) - self.assertEqual(result["identifier"], "invalid-email") - self.assertTrue(result["invalidIdentifier"]) - self.assertEqual(result["error_type"], "invalid_identifier") - self.assertEqual(result["error_message"], "Invalid email address") + self.assertFalse(result["success"]) # noqa: PT009 + self.assertEqual(result["identifier"], "invalid-email") # noqa: PT009 + self.assertTrue(result["invalidIdentifier"]) # noqa: PT009 + self.assertEqual(result["error_type"], "invalid_identifier") # noqa: PT009 + self.assertEqual(result["error_message"], "Invalid email address") # noqa: PT009 @patch("lms.djangoapps.instructor.utils.enroll_email") def test_process_single_student_enrollment_general_error( @@ -227,11 +227,11 @@ def test_process_single_student_enrollment_general_error( email_params=self.email_params, ) - self.assertFalse(result["success"]) - self.assertEqual(result["identifier"], self.user.email) - self.assertTrue(result["error"]) - self.assertEqual(result["error_type"], "general_error") - self.assertEqual(result["error_message"], "Database error") + self.assertFalse(result["success"]) # noqa: PT009 + self.assertEqual(result["identifier"], self.user.email) # noqa: PT009 + self.assertTrue(result["error"]) # noqa: PT009 + self.assertEqual(result["error_type"], "general_error") # noqa: PT009 + self.assertEqual(result["error_message"], "Database error") # noqa: PT009 class TestProcessStudentEnrollmentBatch(TestCase): @@ -284,13 +284,13 @@ def test_process_student_enrollment_batch_success( secure=True, ) - self.assertEqual(result["action"], EnrollStatusChange.enroll) - self.assertFalse(result["auto_enroll"]) - self.assertEqual(result["total_students"], 2) - self.assertEqual(result["successful_operations"], 2) - self.assertEqual(result["failed_operations"], 0) - self.assertEqual(len(result["results"]), 2) - self.assertEqual(mock_process_single.call_count, 2) + self.assertEqual(result["action"], EnrollStatusChange.enroll) # noqa: PT009 + self.assertFalse(result["auto_enroll"]) # noqa: PT009 + self.assertEqual(result["total_students"], 2) # noqa: PT009 + self.assertEqual(result["successful_operations"], 2) # noqa: PT009 + self.assertEqual(result["failed_operations"], 0) # noqa: PT009 + self.assertEqual(len(result["results"]), 2) # noqa: PT009 + self.assertEqual(mock_process_single.call_count, 2) # noqa: PT009 @patch("lms.djangoapps.instructor.utils.get_course_by_id") @patch("lms.djangoapps.instructor.utils.get_email_params") @@ -333,10 +333,10 @@ def test_process_student_enrollment_batch_mixed_results( secure=True, ) - self.assertEqual(result["total_students"], 2) - self.assertEqual(result["successful_operations"], 1) - self.assertEqual(result["failed_operations"], 1) - self.assertEqual(len(result["results"]), 2) + self.assertEqual(result["total_students"], 2) # noqa: PT009 + self.assertEqual(result["successful_operations"], 1) # noqa: PT009 + self.assertEqual(result["failed_operations"], 1) # noqa: PT009 + self.assertEqual(len(result["results"]), 2) # noqa: PT009 @patch("lms.djangoapps.instructor.utils.get_course_by_id") @patch("lms.djangoapps.instructor.utils.get_email_params") @@ -375,10 +375,10 @@ def progress_callback(current, total, results): progress_callback=progress_callback, ) - self.assertEqual(len(progress_calls), 2) - self.assertEqual(progress_calls[0], (1, 2, 1)) - self.assertEqual(progress_calls[1], (2, 2, 2)) - self.assertEqual(result["successful_operations"], 2) + self.assertEqual(len(progress_calls), 2) # noqa: PT009 + self.assertEqual(progress_calls[0], (1, 2, 1)) # noqa: PT009 + self.assertEqual(progress_calls[1], (2, 2, 2)) # noqa: PT009 + self.assertEqual(result["successful_operations"], 2) # noqa: PT009 @patch("lms.djangoapps.instructor.utils.process_single_student_enrollment") def test_process_student_enrollment_batch_no_email(self, mock_process_single: Mock): @@ -403,14 +403,12 @@ def test_process_student_enrollment_batch_no_email(self, mock_process_single: Mo secure=True, ) - self.assertEqual(result["successful_operations"], 1) + self.assertEqual(result["successful_operations"], 1) # noqa: PT009 call_kwargs = mock_process_single.call_args[1] - self.assertEqual(call_kwargs["email_params"], {}) + self.assertEqual(call_kwargs["email_params"], {}) # noqa: PT009 - @patch("lms.djangoapps.instructor.utils.get_course_by_id") - @patch("lms.djangoapps.instructor.utils.get_email_params") @patch("lms.djangoapps.instructor.utils.process_single_student_enrollment") - def test_process_student_enrollment_batch_empty_list(self, mock_process_single: Mock, _: Mock, __: Mock): + def test_process_student_enrollment_batch_empty_list(self, mock_process_single: Mock): """Test batch processing with empty identifier list.""" result = process_student_enrollment_batch( request_user=self.request_user, @@ -423,8 +421,8 @@ def test_process_student_enrollment_batch_empty_list(self, mock_process_single: secure=True, ) - self.assertEqual(result["total_students"], 0) - self.assertEqual(result["successful_operations"], 0) - self.assertEqual(result["failed_operations"], 0) - self.assertEqual(len(result["results"]), 0) + self.assertEqual(result["total_students"], 0) # noqa: PT009 + self.assertEqual(result["successful_operations"], 0) # noqa: PT009 + self.assertEqual(result["failed_operations"], 0) # noqa: PT009 + self.assertEqual(len(result["results"]), 0) # noqa: PT009 mock_process_single.assert_not_called() diff --git a/lms/djangoapps/instructor/utils.py b/lms/djangoapps/instructor/utils.py index 2c8b371accd9..d137c4674003 100644 --- a/lms/djangoapps/instructor/utils.py +++ b/lms/djangoapps/instructor/utils.py @@ -6,7 +6,7 @@ """ import logging -from typing import Callable, Optional +from collections.abc import Callable from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError @@ -25,6 +25,7 @@ CourseEnrollment, EnrollStatusChange, ManualEnrollmentAudit, + get_user_by_username_or_email, ) from lms.djangoapps.instructor.enrollment import ( enroll_email, @@ -32,7 +33,6 @@ get_user_email_language, unenroll_email, ) -from common.djangoapps.student.models import get_user_by_username_or_email from openedx.core.lib.courses import get_course_by_id log = logging.getLogger(__name__) @@ -193,7 +193,7 @@ def process_student_enrollment_batch( email_students: bool, reason: str | None, secure: bool, - progress_callback: Optional[Callable] = None, + progress_callback: Callable[..., None] | None = None, ): """ Process a batch of student enrollment/unenrollment operations. diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 6f5349527cbc..58f731e0631b 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -576,7 +576,7 @@ def test_basic_submission(self, mock_submit_task): secure=True, ) - self.assertEqual(result, "task-result") + self.assertEqual(result, "task-result") # noqa: PT009 key_stub = f"{self.course_key}_{action}_{'_'.join(identifiers)}" expected_key = hashlib.md5(key_stub.encode("utf-8")).hexdigest() mock_submit_task.assert_called_once_with( @@ -611,7 +611,7 @@ def test_identifiers_are_truncated_to_5_for_key(self, mock_submit_task): expected_key = hashlib.md5(key_stub.encode("utf-8")).hexdigest() call_args = mock_submit_task.call_args[0] received_key = call_args[5] - self.assertEqual(received_key, expected_key) + self.assertEqual(received_key, expected_key) # noqa: PT009 @mock.patch("lms.djangoapps.instructor_task.api.submit_task") def test_already_running_error_is_propagated(self, mock_submit_task): @@ -620,7 +620,7 @@ def test_already_running_error_is_propagated(self, mock_submit_task): """ mock_submit_task.side_effect = AlreadyRunningError("Task already running") - with self.assertRaises(AlreadyRunningError): + with pytest.raises(AlreadyRunningError): submit_student_enrollment_batch( request=self.request, course_key=self.course_key, From a11a0bc4cd0a7f008be809cf3568653e1a52738e Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 22 Apr 2026 11:34:21 -0500 Subject: [PATCH 08/19] fix: correct state transition --- lms/djangoapps/instructor/tests/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index e0b68549f6ca..d68633c819c6 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1543,7 +1543,7 @@ def test_unenroll_with_email_allowed_student(self): "allowed": False, }, "success": True, - "state_transition": ALLOWEDTOENROLL_TO_ENROLLED, + "state_transition": ALLOWEDTOENROLL_TO_UNENROLLED, } ] } From 6ba612ff57958db70bd3a6a78b2aaaae8d1f3a95 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 22 Apr 2026 12:40:37 -0500 Subject: [PATCH 09/19] fix: add type hints for request_user parameter and improve error handling --- lms/djangoapps/instructor/utils.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/instructor/utils.py b/lms/djangoapps/instructor/utils.py index d137c4674003..5e7402130b07 100644 --- a/lms/djangoapps/instructor/utils.py +++ b/lms/djangoapps/instructor/utils.py @@ -87,7 +87,7 @@ def _determine_unenroll_state_transition(before_state: dict) -> str: def process_single_student_enrollment( - request_user, + request_user: User, course_key: CourseKey, action: str, identifier: str, @@ -173,8 +173,7 @@ def process_single_student_enrollment( "error_message": "Invalid email address", } except Exception as exc: # pylint: disable=broad-exception-caught - log.exception("Error while processing student") - log.exception(exc) + log.exception("Error while processing student: %s", exc) return { "identifier": identifier, "error": True, @@ -185,7 +184,7 @@ def process_single_student_enrollment( def process_student_enrollment_batch( - request_user, + request_user: User, course_key: CourseKey, action: str, identifiers: list[str], From e44abe569577f405a14d07e90ec2a2e2c5b78ad5 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 22 Apr 2026 13:46:31 -0500 Subject: [PATCH 10/19] fix: improve error messages and task key generation in enrollment processes --- lms/djangoapps/instructor/tests/test_utils.py | 7 +++- lms/djangoapps/instructor/utils.py | 5 ++- lms/djangoapps/instructor_task/api.py | 6 +-- lms/djangoapps/instructor_task/api_helper.py | 3 +- .../instructor_task/tests/test_api.py | 37 +++++++++++++------ .../js/instructor_dashboard/membership.js | 3 ++ 6 files changed, 42 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_utils.py b/lms/djangoapps/instructor/tests/test_utils.py index 008a70a3cb33..ae23f47d723c 100644 --- a/lms/djangoapps/instructor/tests/test_utils.py +++ b/lms/djangoapps/instructor/tests/test_utils.py @@ -214,7 +214,7 @@ def test_process_single_student_enrollment_general_error( mock_enroll_email: Mock, ): """Test enrollment with general exception.""" - mock_enroll_email.side_effect = Exception("Database error") + mock_enroll_email.side_effect = Exception() result = process_single_student_enrollment( request_user=self.request_user, @@ -231,7 +231,10 @@ def test_process_single_student_enrollment_general_error( self.assertEqual(result["identifier"], self.user.email) # noqa: PT009 self.assertTrue(result["error"]) # noqa: PT009 self.assertEqual(result["error_type"], "general_error") # noqa: PT009 - self.assertEqual(result["error_message"], "Database error") # noqa: PT009 + self.assertEqual( # noqa: PT009 + result["error_message"], + "Something went wrong while processing this learner. Please try again or contact support.", + ) class TestProcessStudentEnrollmentBatch(TestCase): diff --git a/lms/djangoapps/instructor/utils.py b/lms/djangoapps/instructor/utils.py index 5e7402130b07..15ec6b516829 100644 --- a/lms/djangoapps/instructor/utils.py +++ b/lms/djangoapps/instructor/utils.py @@ -11,6 +11,7 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.core.validators import validate_email +from django.utils.translation import gettext as _ from opaque_keys.edx.keys import CourseKey from common.djangoapps.student.models import ( @@ -179,7 +180,9 @@ def process_single_student_enrollment( "error": True, "success": False, "error_type": "general_error", - "error_message": str(exc), + "error_message": _( + "Something went wrong while processing this learner. Please try again or contact support." + ), } diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 22aa61c2adee..73d30bc3eb13 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -8,6 +8,7 @@ """ import datetime import hashlib +import json import logging from collections import Counter @@ -656,10 +657,7 @@ def submit_student_enrollment_batch( "secure": secure, } - # Create task key based on course, action and first few identifiers for uniqueness - # Limit identifiers in key to avoid exceeding max length - key_identifiers = identifiers[:5] if len(identifiers) > 5 else identifiers - task_key_stub = f'{course_key}_{action}_{"_".join(key_identifiers)}' + task_key_stub = f"{course_key}_{action}_{json.dumps(identifiers)}" task_key = hashlib.md5(task_key_stub.encode("utf-8")).hexdigest() return submit_task(request, task_type, task_class, course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/api_helper.py b/lms/djangoapps/instructor_task/api_helper.py index eb0f6a11fbe7..3a2cba1be1a9 100644 --- a/lms/djangoapps/instructor_task/api_helper.py +++ b/lms/djangoapps/instructor_task/api_helper.py @@ -113,7 +113,8 @@ def generate_already_running_error_message(task_type): 'proctored_exam_results_report': _('proctored exam results'), 'export_ora2_data': _('ORA data'), 'grade_course': _('grade'), - 'inactive_enrolled_students_info_csv': _('inactive enrollment') + 'inactive_enrolled_students_info_csv': _('inactive enrollment'), + 'student_enrollment_batch': _('student enrollment batch'), } if report_types.get(task_type): diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 58f731e0631b..8d9cd83affce 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -577,7 +577,7 @@ def test_basic_submission(self, mock_submit_task): ) self.assertEqual(result, "task-result") # noqa: PT009 - key_stub = f"{self.course_key}_{action}_{'_'.join(identifiers)}" + key_stub = f'{self.course_key}_{action}_{json.dumps(identifiers)}' expected_key = hashlib.md5(key_stub.encode("utf-8")).hexdigest() mock_submit_task.assert_called_once_with( self.request, @@ -589,29 +589,44 @@ def test_basic_submission(self, mock_submit_task): ) @mock.patch("lms.djangoapps.instructor_task.api.submit_task") - def test_identifiers_are_truncated_to_5_for_key(self, mock_submit_task): + def test_task_key_differs_when_tail_identifiers_differ(self, mock_submit_task): """ - When identifiers > 5, only first 5 go into the task_key. + Task key must incorporate the full list so batches that share the same first IDs + are not incorrectly treated as duplicates of different work. """ - identifiers = ["u1", "u2", "u3", "u4", "u5", "u6", "u7"] + prefix = ["u1", "u2", "u3", "u4", "u5"] + batch_a = prefix + ["tail-a"] + batch_b = prefix + ["tail-b"] submit_student_enrollment_batch( request=self.request, course_key=self.course_key, action="unenroll", - identifiers=identifiers, + identifiers=batch_a, auto_enroll=False, email_students=True, reason=None, secure=False, ) + key_a = mock_submit_task.call_args[0][5] - truncated = identifiers[:5] - key_stub = f"{self.course_key}_unenroll_{'_'.join(truncated)}" - expected_key = hashlib.md5(key_stub.encode("utf-8")).hexdigest() - call_args = mock_submit_task.call_args[0] - received_key = call_args[5] - self.assertEqual(received_key, expected_key) # noqa: PT009 + submit_student_enrollment_batch( + request=self.request, + course_key=self.course_key, + action="unenroll", + identifiers=batch_b, + auto_enroll=False, + email_students=True, + reason=None, + secure=False, + ) + key_b = mock_submit_task.call_args[0][5] + + expected_a = hashlib.md5(f"{self.course_key}_unenroll_{json.dumps(batch_a)}".encode()).hexdigest() + expected_b = hashlib.md5(f"{self.course_key}_unenroll_{json.dumps(batch_b)}".encode()).hexdigest() + self.assertEqual(key_a, expected_a) # noqa: PT009 + self.assertEqual(key_b, expected_b) # noqa: PT009 + self.assertNotEqual(key_a, key_b) # noqa: PT009 @mock.patch("lms.djangoapps.instructor_task.api.submit_task") def test_already_running_error_is_propagated(self, mock_submit_task): diff --git a/lms/static/js/instructor_dashboard/membership.js b/lms/static/js/instructor_dashboard/membership.js index c90606ad4f12..0d6147c26bce 100644 --- a/lms/static/js/instructor_dashboard/membership.js +++ b/lms/static/js/instructor_dashboard/membership.js @@ -713,6 +713,9 @@ such that the value can be defined later than this assignment (file load order). notunenrolled = []; ref = dataFromServer.results; if (!ref) { + if (dataFromServer.error) { + return this.fail_with_error(dataFromServer.error); + } return this.show_async_processing_message(dataFromServer); } for (i = 0, len = ref.length; i < len; i++) { From 59ab047c8e02dbd248eb64bda95ce6731300da5d Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 22 Apr 2026 17:33:46 -0500 Subject: [PATCH 11/19] chore: implement suggestions from code review --- lms/djangoapps/instructor/message_types.py | 2 +- lms/djangoapps/instructor/utils.py | 4 ++-- lms/djangoapps/instructor_task/api.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/instructor/message_types.py b/lms/djangoapps/instructor/message_types.py index 470bae398c0c..cd56f478af94 100644 --- a/lms/djangoapps/instructor/message_types.py +++ b/lms/djangoapps/instructor/message_types.py @@ -87,7 +87,7 @@ def __init__(self, *args, **kwargs): class BatchEnrollment(BaseMessageType): """ - A message for instructors when finish the batch enrollment async process. + A message for instructors when they finish the batch enrollment async process. """ APP_LABEL = "instructor" diff --git a/lms/djangoapps/instructor/utils.py b/lms/djangoapps/instructor/utils.py index 15ec6b516829..e2062856bba2 100644 --- a/lms/djangoapps/instructor/utils.py +++ b/lms/djangoapps/instructor/utils.py @@ -171,10 +171,10 @@ def process_single_student_enrollment( "invalidIdentifier": True, "success": False, "error_type": "invalid_identifier", - "error_message": "Invalid email address", + "error_message": _("Invalid email address"), } except Exception as exc: # pylint: disable=broad-exception-caught - log.exception("Error while processing student: %s", exc) + log.exception("Error while processing student %s: %s", identifier, exc) return { "identifier": identifier, "error": True, diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 73d30bc3eb13..84dca71896ab 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -657,7 +657,7 @@ def submit_student_enrollment_batch( "secure": secure, } - task_key_stub = f"{course_key}_{action}_{json.dumps(identifiers)}" + task_key_stub = f"{course_key}_{action}_{json.dumps(sorted(identifiers))}" task_key = hashlib.md5(task_key_stub.encode("utf-8")).hexdigest() return submit_task(request, task_type, task_class, course_key, task_input, task_key) From 1d8343c1d9586412a76304dbbfe3d927495f27b0 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 22 Apr 2026 18:00:25 -0500 Subject: [PATCH 12/19] fix: update task key generation in tests --- lms/djangoapps/instructor_task/tests/test_api.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 8d9cd83affce..f22dc97293d3 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -622,8 +622,10 @@ def test_task_key_differs_when_tail_identifiers_differ(self, mock_submit_task): ) key_b = mock_submit_task.call_args[0][5] - expected_a = hashlib.md5(f"{self.course_key}_unenroll_{json.dumps(batch_a)}".encode()).hexdigest() - expected_b = hashlib.md5(f"{self.course_key}_unenroll_{json.dumps(batch_b)}".encode()).hexdigest() + key_stub_a = f"{self.course_key}_unenroll_{json.dumps(sorted(batch_a))}" + key_stub_b = f"{self.course_key}_unenroll_{json.dumps(sorted(batch_b))}" + expected_a = hashlib.md5(key_stub_a.encode("utf-8")).hexdigest() + expected_b = hashlib.md5(key_stub_b.encode("utf-8")).hexdigest() self.assertEqual(key_a, expected_a) # noqa: PT009 self.assertEqual(key_b, expected_b) # noqa: PT009 self.assertNotEqual(key_a, key_b) # noqa: PT009 From ab9f19e2ef1c485cf0b2b4d1eec914448405c721 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 7 May 2026 16:40:06 -0500 Subject: [PATCH 13/19] fix: update state transition in bulk enrollment tests --- lms/djangoapps/bulk_enroll/tests/test_views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/bulk_enroll/tests/test_views.py b/lms/djangoapps/bulk_enroll/tests/test_views.py index 7a9321f872f6..f378dde0154a 100644 --- a/lms/djangoapps/bulk_enroll/tests/test_views.py +++ b/lms/djangoapps/bulk_enroll/tests/test_views.py @@ -549,7 +549,7 @@ def test_readd_to_different_cohort(self): "cohort": 'cohort2', }, "success": True, - "state_transition": ENROLLED_TO_ENROLLED, + "state_transition": ENROLLED_TO_UNENROLLED, } ] } @@ -649,7 +649,7 @@ def test_readd_to_same_cohort(self): "cohort": 'cohort1', }, "success": True, - "state_transition": ENROLLED_TO_ENROLLED, + "state_transition": ENROLLED_TO_UNENROLLED, } ] } From d3f21e9141226bd5daa271abc658a44b845354d5 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 7 May 2026 17:37:06 -0500 Subject: [PATCH 14/19] fix: disable pylint warning for imported User model in enrollment.py --- lms/djangoapps/instructor/enrollment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index a8b470b86dcf..565eef116913 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -12,7 +12,7 @@ import pytz from crum import get_current_request from django.conf import settings -from django.contrib.auth.models import User +from django.contrib.auth.models import User # pylint: disable=imported-auth-user from django.contrib.sites.models import Site from django.template.loader import render_to_string from django.urls import reverse From 3fff477d9113f969eff72945255e6deff7f47abe Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 8 May 2026 10:56:05 -0500 Subject: [PATCH 15/19] fix: correct test method names and update state transitions in bulk enrollment tests --- lms/djangoapps/bulk_enroll/tests/test_views.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/bulk_enroll/tests/test_views.py b/lms/djangoapps/bulk_enroll/tests/test_views.py index f378dde0154a..7261e05a1a7d 100644 --- a/lms/djangoapps/bulk_enroll/tests/test_views.py +++ b/lms/djangoapps/bulk_enroll/tests/test_views.py @@ -15,6 +15,7 @@ from rest_framework.test import APIRequestFactory, APITestCase, force_authenticate from common.djangoapps.student.models import ( + ENROLLED_TO_ENROLLED, ENROLLED_TO_UNENROLLED, UNENROLLED_TO_ENROLLED, CourseEnrollment, @@ -460,7 +461,7 @@ def test_add_to_valid_cohort(self): assert res_json == expected - def test_readd_to_different_cohort(self): + def test_read_to_different_cohort(self): config_course_cohorts(self.course, is_cohorted=True, manual_cohorts=["cohort1", "cohort2"]) response = self.request_bulk_enroll({ 'identifiers': self.notenrolled_student.username, @@ -549,7 +550,7 @@ def test_readd_to_different_cohort(self): "cohort": 'cohort2', }, "success": True, - "state_transition": ENROLLED_TO_UNENROLLED, + "state_transition": ENROLLED_TO_ENROLLED, } ] } @@ -559,7 +560,7 @@ def test_readd_to_different_cohort(self): assert get_cohort_id(self.notenrolled_student, CourseKey.from_string(self.course_key)) is not None assert res2_json == expected2 - def test_readd_to_same_cohort(self): + def test_read_to_same_cohort(self): config_course_cohorts(self.course, is_cohorted=True, manual_cohorts=["cohort1", "cohort2"]) response = self.request_bulk_enroll({ 'identifiers': self.notenrolled_student.username, @@ -649,7 +650,7 @@ def test_readd_to_same_cohort(self): "cohort": 'cohort1', }, "success": True, - "state_transition": ENROLLED_TO_UNENROLLED, + "state_transition": ENROLLED_TO_ENROLLED, } ] } From fcb07007709b15cf7389928ca99e55bf10a8a85d Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 8 May 2026 12:13:05 -0500 Subject: [PATCH 16/19] fix: wrap enrollment sync process in a transaction to ensure atomicity --- lms/djangoapps/instructor/views/api.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 04b90a7d9b1e..a22b83bb0c04 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -812,9 +812,10 @@ def _process_student_enrollment(self, request, course_id, data, secure): # pyli "total_students": len(identifiers), } - return self._process_enrollment_sync( - request.user, course_key, action, identifiers, auto_enroll, email_students, reason, secure - ) + with transaction.atomic(): + return self._process_enrollment_sync( + request.user, course_key, action, identifiers, auto_enroll, email_students, reason, secure + ) def _process_enrollment_sync( self, From 8851b1ee4e45513fec03d701b41f2237f1dc5fbf Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 8 May 2026 13:20:27 -0500 Subject: [PATCH 17/19] feat: add site_id parameter to enrollment batch processing --- lms/djangoapps/instructor/views/api.py | 5 +++++ lms/djangoapps/instructor_task/api.py | 3 +++ .../instructor_task/notifications.py | 19 ++++++++++++++----- .../tasks_helper/enrollments.py | 1 + 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index a22b83bb0c04..61f44c25d752 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -123,6 +123,7 @@ from openedx.core.djangoapps.course_groups.models import CourseUserGroup from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings, Role from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.djangoapps.theming.helpers import get_current_site from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser @@ -779,6 +780,9 @@ def _process_student_enrollment(self, request, course_id, data, secure): # pyli course_key = CourseKey.from_string(course_id) + site = get_current_site() + site_id = site.id if site else None + if async_processing: try: @@ -791,6 +795,7 @@ def _process_student_enrollment(self, request, course_id, data, secure): # pyli email_students=email_students, reason=reason, secure=secure, + site_id=site_id, ) return { diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 84dca71896ab..4416a8ce99bf 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -622,6 +622,7 @@ def submit_student_enrollment_batch( email_students: bool, reason: str | None, secure: bool, + site_id: int | None = None, ): """ Request to have student enrollment operations processed as a background task. @@ -638,6 +639,7 @@ def submit_student_enrollment_batch( email_students (bool): Whether to send enrollment emails reason (str | None): Optional reason for enrollment change secure (bool): Whether the request is secure (HTTPS) + site_id (int | None): Optional site ID for notification emails Returns: InstructorTask object representing the submitted background task @@ -655,6 +657,7 @@ def submit_student_enrollment_batch( "email_students": email_students, "reason": reason, "secure": secure, + "site_id": site_id, } task_key_stub = f"{course_key}_{action}_{json.dumps(sorted(identifiers))}" diff --git a/lms/djangoapps/instructor_task/notifications.py b/lms/djangoapps/instructor_task/notifications.py index 4f16187a71b4..5ade4f51edc3 100644 --- a/lms/djangoapps/instructor_task/notifications.py +++ b/lms/djangoapps/instructor_task/notifications.py @@ -27,13 +27,23 @@ TASK_LOG = logging.getLogger("edx.celery.task") -def _get_current_site() -> Site | None: +def _get_current_site(site_id: int | None = None) -> Site | None: """ Get the current Django Site instance with fallback logic. + Args: + site_id (int | None): Optional site ID to retrieve. If provided, attempts + to get this specific site first before falling back. + Returns: Site | None: The current Site object or None if unavailable """ + if site_id: + try: + return Site.objects.get(id=site_id) + except Site.DoesNotExist: + pass + # Try to get current site if method exists if hasattr(Site.objects, "get_current"): site = Site.objects.get_current() @@ -46,7 +56,6 @@ def _get_current_site() -> Site | None: except Site.DoesNotExist: pass - # Last resort: get first site try: return Site.objects.first() except Exception: # pylint: disable=broad-except @@ -144,11 +153,12 @@ def send_enrollment_task_completion_email( - failed: Number of failed operations """ requester = instructor_task.requester - site = _get_current_site() task_input = _parse_task_input(instructor_task) + + site_id = task_input.get("site_id") + site = _get_current_site(site_id) user_language = get_user_preference(requester, LANGUAGE_KEY) - # Build email context user_context = _build_enrollment_email_context( course_key=course_key, requester=requester, @@ -158,7 +168,6 @@ def send_enrollment_task_completion_email( task_input=task_input, ) - # Create and send message message = BatchEnrollment().personalize( recipient=Recipient(lms_user_id=requester.id, email_address=requester.email), language=user_language, diff --git a/lms/djangoapps/instructor_task/tasks_helper/enrollments.py b/lms/djangoapps/instructor_task/tasks_helper/enrollments.py index 789bb62d0978..b043994a84e9 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/enrollments.py +++ b/lms/djangoapps/instructor_task/tasks_helper/enrollments.py @@ -148,6 +148,7 @@ def process_student_enrollment_batch( - email_students: boolean to send enrollment emails - reason: optional reason for enrollment change - secure: boolean indicating if request was secure (HTTPS) + - site_id: optional site ID for notification emails action_name: Name of the action being performed Returns: From ea5a625776604c9dec131fe274ee79a539f86db0 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 8 May 2026 17:05:56 -0500 Subject: [PATCH 18/19] fix: add site_id to the enrollment batch submission test case --- lms/djangoapps/instructor_task/tests/test_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index f22dc97293d3..cbe3085077dd 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -563,6 +563,7 @@ def test_basic_submission(self, mock_submit_task): "email_students": False, "reason": "test", "secure": True, + "site_id": None, } result = submit_student_enrollment_batch( From 479ff5cc7e875e916794e3c6a1ba701d9f14ad78 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 12 May 2026 15:42:51 -0500 Subject: [PATCH 19/19] fix: wrap enrollment and audit operations in a transaction for atomicity --- lms/djangoapps/instructor/utils.py | 46 +++++++++++++++----------- lms/djangoapps/instructor/views/api.py | 7 ++-- 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/lms/djangoapps/instructor/utils.py b/lms/djangoapps/instructor/utils.py index e2062856bba2..c2ea750c4984 100644 --- a/lms/djangoapps/instructor/utils.py +++ b/lms/djangoapps/instructor/utils.py @@ -11,6 +11,7 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.core.validators import validate_email +from django.db import transaction from django.utils.translation import gettext as _ from opaque_keys.edx.keys import CourseKey @@ -136,27 +137,32 @@ def process_single_student_enrollment( try: validate_email(email) # Raises ValidationError if invalid - if action == EnrollStatusChange.enroll: - before, after, enrollment_obj = enroll_email( - course_key, email, auto_enroll, email_students, {**email_params}, language=language + # Wrap enrollment and audit operations in an atomic transaction + # to ensure both succeed or both are rolled back + with transaction.atomic(): + if action == EnrollStatusChange.enroll: + before, after, enrollment_obj = enroll_email( + course_key, email, auto_enroll, email_students, {**email_params}, language=language + ) + before_state = before.to_dict() + after_state = after.to_dict() + state_transition = _determine_enroll_state_transition(before_state, after_state) + + elif action == EnrollStatusChange.unenroll: + before, after = unenroll_email( + course_key, email, email_students, {**email_params}, language=language + ) + before_state = before.to_dict() + after_state = after.to_dict() + state_transition = _determine_unenroll_state_transition(before_state) + enrollment_obj = ( + CourseEnrollment.get_enrollment(identified_user, course_key) if identified_user else None + ) + + # Create audit record + ManualEnrollmentAudit.create_manual_enrollment_audit( + request_user, email, state_transition, reason, enrollment_obj ) - before_state = before.to_dict() - after_state = after.to_dict() - state_transition = _determine_enroll_state_transition(before_state, after_state) - - elif action == EnrollStatusChange.unenroll: - before, after = unenroll_email( - course_key, email, email_students, {**email_params}, language=language - ) - before_state = before.to_dict() - after_state = after.to_dict() - state_transition = _determine_unenroll_state_transition(before_state) - enrollment_obj = CourseEnrollment.get_enrollment(identified_user, course_key) if identified_user else None - - # Create audit record - ManualEnrollmentAudit.create_manual_enrollment_audit( - request_user, email, state_transition, reason, enrollment_obj - ) return { "identifier": identifier, diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 61f44c25d752..9b629fd807c7 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -817,10 +817,9 @@ def _process_student_enrollment(self, request, course_id, data, secure): # pyli "total_students": len(identifiers), } - with transaction.atomic(): - return self._process_enrollment_sync( - request.user, course_key, action, identifiers, auto_enroll, email_students, reason, secure - ) + return self._process_enrollment_sync( + request.user, course_key, action, identifiers, auto_enroll, email_students, reason, secure + ) def _process_enrollment_sync( self,