From 3591e8752024322d25c70354d27e9d46a1d8c365 Mon Sep 17 00:00:00 2001 From: coder1918 Date: Tue, 23 Sep 2025 14:09:12 -0600 Subject: [PATCH 1/5] feat: add unified certificate task API with toggle, generate, and regenerate support --- .../instructor/tests/test_certificates.py | 128 +++++++++- lms/djangoapps/instructor/views/api.py | 241 ++++++++++++++++-- lms/djangoapps/instructor/views/api_urls.py | 3 + .../instructor/views/instructor_dashboard.py | 12 +- 4 files changed, 350 insertions(+), 34 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 3b6a9a2235..d58a70940d 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -35,6 +35,128 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order +@ddt.ddt +class CertificateTaskViewTests(SharedModuleStoreTestCase): + """Tests for the certificate panel of the instructor dash. """ + + @classmethod + def setUpClass(cls): + """ + Set up the test class with a test course and instructor dashboard URL. + """ + super().setUpClass() + cls.course = CourseFactory.create() + cls.url = reverse( + 'instructor_dashboard', + kwargs={'course_id': str(cls.course.id)} + ) + + def setUp(self): + """ + Set up test users and enable certificate generation configuration. + """ + super().setUp() + self.user = UserFactory.create() + self.global_staff = GlobalStaffFactory() + self.instructor = InstructorFactory(course_key=self.course.id) + + # Need to clear the cache for model-based configuration + cache.clear() + + # Enable the certificate generation feature + CertificateGenerationConfiguration.objects.create(enabled=True) + + def _login_as(self, role): + """ + Log in the test client as the specified user role. + """ + user_map = { + "user": self.user.username, + "instructor": self.instructor.username, + "global_staff": self.global_staff.username + } + self.client.login(username=user_map.get(role, "user"), password=self.TEST_PASSWORD) + + def _get_url(self, action): + """ + Build the unified certificate task URL for the given action. + """ + return reverse("certificate_task", kwargs={"course_id": self.course.id, "action": action}) + + def _assert_redirects_to_instructor_dash(self, response): + """Check that the response redirects to the certificates section. """ + expected_redirect = reverse( + 'instructor_dashboard', + kwargs={'course_id': str(self.course.id)} + ) + expected_redirect += '#view-certificates' + self.assertRedirects(response, expected_redirect) + + @ddt.data(True, False) + def test_certificate_generation_enable(self, is_enabled): + """ + Test enabling or disabling self-generated certificates as global staff. + """ + self._login_as("global_staff") + + params = {"certificates-enabled": "true" if is_enabled else "false"} + response = self.client.post( + self._get_url("toggle"), + data=params + ) + + # Expect a redirect back to the instructor dashboard + self._assert_redirects_to_instructor_dash(response) + + # Expect that certificate generation is now enabled for the course + actual_enabled = certs_api.has_self_generated_certificates_enabled(str(self.course.id)) + assert is_enabled == actual_enabled + + @ddt.data("user", "instructor", "global_staff") + def test_certificate_generation(self, role): + """ + Test permission-based access to certificate generation by role. + """ + self._login_as(role) + response = self.client.post(self._get_url("generate")) + actual_status_code = { + "user": 403, + "instructor": 200, + "global_staff": 200 + } + assert response.status_code == actual_status_code[role] + + @ddt.data( + ("downloadable", 200, True, 'Certificate regeneration task has been started. You can view ' + 'the status of the generation task in the "Pending Tasks" section.'), + ("generating", 400, False, 'Please select certificate statuses that lie with ' + 'in "certificate_statuses" entry in POST data.') + ) + @ddt.unpack + def test_certificate_regeneration_status_handling(self, cert_status, expected_status, success, expected_message): + """ + Test certificate regeneration with valid and invalid certificate statuses. + """ + # Create a certificate with the given status + GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course.id, + status=cert_status, + mode='honor' + ) + + self._login_as("global_staff") + response = self.client.post( + self._get_url("regenerate"), + data={'certificate_statuses': [cert_status]}, + ) + + assert response.status_code == expected_status + res_json = response.json() + assert res_json.get('success', False) is success + assert res_json.get('message') == expected_message + + @ddt.ddt class CertificatesInstructorDashTest(SharedModuleStoreTestCase): """Tests for the certificate panel of the instructor dash. """ @@ -138,7 +260,11 @@ class CertificatesInstructorDashTest(SharedModuleStoreTestCase): self.assertContains(response, 'enable-certificates-submit') self.assertNotContains(response, 'Generate Example Certificates') - @mock.patch.dict(settings.FEATURES, {'CERTIFICATES_HTML_VIEW': True}) + @mock.patch.dict(settings.FEATURES, { + 'CERTIFICATES_HTML_VIEW': True, + 'CERTIFICATES_INSTRUCTOR_GENERATION': False + } + ) def test_buttons_for_html_certs_in_self_paced_course(self): """ Tests `Enable Student-Generated Certificates` button is enabled diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index f8b6dc06f4..eae657ed19 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -39,7 +39,7 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_name from rest_framework.exceptions import MethodNotAllowed from rest_framework import serializers, status # lint-amnesty, pylint: disable=wrong-import-order -from rest_framework.permissions import IsAdminUser, IsAuthenticated # lint-amnesty, pylint: disable=wrong-import-order +from rest_framework.permissions import IsAdminUser, IsAuthenticated, BasePermission # lint-amnesty, pylint: disable=wrong-import-order from rest_framework.response import Response # lint-amnesty, pylint: disable=wrong-import-order from rest_framework.views import APIView # lint-amnesty, pylint: disable=wrong-import-order from submissions import api as sub_api # installed from the edx-submissions repository # lint-amnesty, pylint: disable=wrong-import-order @@ -3402,17 +3402,57 @@ def _instructor_dash_url(course_key, section=None): return url -@require_course_permission(permissions.ENABLE_CERTIFICATE_GENERATION) -@require_POST -def enable_certificate_generation(request, course_id=None): - """Enable/disable self-generated certificates for a course. +class HasCertificateActionPermission(BasePermission): + """ + DRF permission class to validate course-level certificate task permissions + based on the `action` URL parameter. + """ - Once self-generated certificates have been enabled, students - who have passed the course will be able to generate certificates. + permission_map = { + 'toggle': permissions.ENABLE_CERTIFICATE_GENERATION, + 'generate': permissions.START_CERTIFICATE_GENERATION, + 'regenerate': permissions.START_CERTIFICATE_REGENERATION, + } - Redirects back to the instructor dashboard once the - setting has been updated. + def has_permission(self, request, view): + """ + Check whether the user has permission to perform the requested certificate action + on the specified course. + """ + course_id = view.kwargs.get('course_id') + action = view.kwargs.get('action') + if not course_id or not action: + return False + + required_perm = self.permission_map.get(action) + if required_perm is None: + return False + + try: + course_key = CourseKey.from_string(course_id) + except (ValueError, TypeError): + return False + + return request.user.has_perm(required_perm, course_key) + + +def toggle_certificate_generation(request, course_id): + """ + Enable or disable student-generated certificates for a course. + + Based on the value of the POST field `certificates-enabled`, this function + updates the course setting to allow or prevent students from generating their + own certificates. This function assumes that permission checks + have already been performed. + + Args: + request (HttpRequest): The incoming POST request. + course_id (str): The course identifier in string format. + + Returns: + HttpResponseRedirect: Redirects back to the instructor dashboard + (certificates section) after updating the course setting. """ course_key = CourseKey.from_string(course_id) is_enabled = (request.POST.get('certificates-enabled', 'false') == 'true') @@ -3420,6 +3460,167 @@ def enable_certificate_generation(request, course_id=None): return redirect(_instructor_dash_url(course_key, section='certificates')) +def start_certificate_generation(request, course_id): + """ + Initiates the generation of certificates for all enrolled students in the course. + + This function triggers an asynchronous background task that generates certificates + for every student enrolled in the specified course. It returns a response payload + containing a confirmation message and the task ID for tracking the task's progress. + + Args: + request (HttpRequest): The HTTP request object. + course_key (CourseKey): The course identifier for which to generate certificates. + + Returns: + dict: A dictionary with a success message and the task ID. + """ + course_key = CourseKey.from_string(course_id) + task = task_api.generate_certificates_for_students(request, course_key) + + return { + "message": _( + "Certificate generation task for all students of this course has been started. " + "You can view the status of the generation task in the \"Pending Tasks\" section." + ), + "task_id": task.task_id + } + + +def start_certificate_regeneration(request, course_id, certificates_statuses): + """ + Initiates regeneration of certificates for students based on given certificate statuses. + + This function triggers a background task that regenerates certificates for students + whose certificates match the provided list of statuses. + + Args: + request (HttpRequest): The HTTP request object. + course_key (CourseKey): The identifier of the course for which certificates are being regenerated. + certificates_statuses (list[str]): A list of certificate statuses to filter the affected certificates. + + Returns: + dict: A dictionary with a success message and success status. + """ + course_key = CourseKey.from_string(course_id) + task_api.regenerate_certificates(request, course_key, certificates_statuses) + + return { + 'message': _( + 'Certificate regeneration task has been started. ' + 'You can view the status of the generation task in the "Pending Tasks" section.' + ), + 'success': True + } + + +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +@method_decorator(transaction.non_atomic_requests, name='dispatch') +class CertificateTask(DeveloperErrorViewMixin, APIView): + """ + API endpoint for handling certificate-related administrative tasks for a given course. + + Supported actions: + - "toggle": Enable or disable self-generated certificates. + - "generate": Initiate certificate generation for all enrolled students. + - "regenerate": Regenerate certificates based on selected certificate statuses. + + URL pattern: + POST /courses/{course_id}/instructor/api/certificates/{action}/ + + The `action` path parameter determines the task to perform. + The request must be authenticated and the user must have the appropriate permission for the action. + """ + permission_classes = [IsAuthenticated, HasCertificateActionPermission] + + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id, action=None): + """ + Handles POST requests for certificate actions. + + Depending on the `action` parameter, different tasks are performed: + + Args: + request (HttpRequest): The HTTP request object. + course_id (str): The ID of the course on which to perform the action. + action (str, optional): The certificate task to perform. Must be one of: + - "toggle": Enable or disable certificates for the course. No additional + parameters are required. + - "generate": Generate certificates for eligible learners. No additional + parameters are required. + - "regenerate": Regenerate certificates for learners. Requires an additional + parameter in the request body: + - `statuses` (list of str): List of certificate statuses to regenerate + (e.g., ["downloaded", "issued"]). + + Returns: + Response: A DRF Response object containing a success message or error details. + If the `action` is invalid, returns HTTP 400 with an error message. + + Example request body for `regenerate` action: + { + "statuses": ["downloaded", "issued"] + } + """ + if action == "toggle": + return self._handle_toggle(request, course_id) + elif action == "generate": + return self._handle_generate(request, course_id) + elif action == "regenerate": + return self._handle_regenerate(request, course_id) + else: + return Response( + {"error": f"Invalid action: {action}"}, + status=status.HTTP_400_BAD_REQUEST + ) + + def _handle_toggle(self, request, course_id): + """Handle certificate generation toggle.""" + # TODO: Update this to return a proper API response (e.g., {"enabled": true}) + return toggle_certificate_generation(request, course_id) + + def _handle_generate(self, request, course_id): + """Handle certificate generation for all students.""" + payload = start_certificate_generation(request, course_id) + return Response(payload, status=status.HTTP_200_OK) + + def _handle_regenerate(self, request, course_id): + """Handle certificate regeneration based on status.""" + # Validate and extract certificate statuses from the request + serializer = CertificateStatusesSerializer(data=request.data) + if not serializer.is_valid(): + return Response( + {'message': _( + 'Please select certificate statuses that ' + 'lie with in "certificate_statuses" entry in POST data.' + )}, + status=status.HTTP_400_BAD_REQUEST + ) + statuses = serializer.validated_data['certificate_statuses'] + payload = start_certificate_regeneration(request, course_id, statuses) + return Response(payload, status=status.HTTP_200_OK) + + +@require_course_permission(permissions.ENABLE_CERTIFICATE_GENERATION) +@require_POST +def enable_certificate_generation(request, course_id=None): + """ + View to toggle self-generated certificate availability for a course. + + This endpoint is protected by course-level permission checks and allows + enabling or disabling student-generated certificates. The logic is handled + by `toggle_certificate_generation`. + + Args: + request (HttpRequest): The incoming POST request. + course_id (str): The course identifier in string format. + + Returns: + HttpResponseRedirect: Redirects to the instructor dashboard after update. + """ + return toggle_certificate_generation(request, course_id) + + @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') class MarkStudentCanSkipEntranceExam(APIView): """ @@ -3471,16 +3672,8 @@ class StartCertificateGeneration(DeveloperErrorViewMixin, APIView): """ Generating certificates for all students enrolled in given course. """ - course_key = CourseKey.from_string(course_id) - task = task_api.generate_certificates_for_students(request, course_key) - message = _('Certificate generation task for all students of this course has been started. ' - 'You can view the status of the generation task in the "Pending Tasks" section.') - response_payload = { - 'message': message, - 'task_id': task.task_id - } - - return JsonResponse(response_payload) + payload = start_certificate_generation(request, course_id=course_id) + return JsonResponse(payload) @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') @@ -3501,7 +3694,6 @@ class StartCertificateRegeneration(DeveloperErrorViewMixin, APIView): """ certificate_statuses 'certificate_statuses' in POST data. """ - course_key = CourseKey.from_string(course_id) serializer = self.serializer_class(data=request.data) if not serializer.is_valid(): @@ -3511,13 +3703,8 @@ class StartCertificateRegeneration(DeveloperErrorViewMixin, APIView): ) certificates_statuses = serializer.validated_data['certificate_statuses'] - task_api.regenerate_certificates(request, course_key, certificates_statuses) - response_payload = { - 'message': _('Certificate regeneration task has been started. ' - 'You can view the status of the generation task in the "Pending Tasks" section.'), - 'success': True - } - return JsonResponse(response_payload) + payload = start_certificate_regeneration(request, course_id, certificates_statuses) + return JsonResponse(payload) @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 90a087443a..7d4db3e3c0 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -82,6 +82,9 @@ urlpatterns = [ # Cohort management path('add_users_to_cohorts', api.AddUsersToCohorts.as_view(), name='add_users_to_cohorts'), + # Unified endpoint for Certificate tasks + path('certificate_task/', api.CertificateTask.as_view(), name='certificate_task'), + # Certificates path('enable_certificate_generation', api.enable_certificate_generation, name='enable_certificate_generation'), path('start_certificate_generation', api.StartCertificateGeneration.as_view(), name='start_certificate_generation'), diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index be2ae51dbd..3c47a67b63 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -385,16 +385,16 @@ def _section_certificates(course): CertificateGenerationHistory.objects.filter(course_id=course.id).order_by("-created"), 'urls': { 'enable_certificate_generation': reverse( - 'enable_certificate_generation', - kwargs={'course_id': course.id} + 'certificate_task', + kwargs={'course_id': course.id, "action": "toggle"} ), 'start_certificate_generation': reverse( - 'start_certificate_generation', - kwargs={'course_id': course.id} + 'certificate_task', + kwargs={'course_id': course.id, "action": "generate"} ), 'start_certificate_regeneration': reverse( - 'start_certificate_regeneration', - kwargs={'course_id': course.id} + 'certificate_task', + kwargs={'course_id': course.id, "action": "regenerate"} ), 'list_instructor_tasks_url': reverse( 'list_instructor_tasks', From c9a26892d9c9da0404b6c7b0958f95a6909b7bff Mon Sep 17 00:00:00 2001 From: Awais Ansari Date: Thu, 21 Mar 2024 20:21:39 +0500 Subject: [PATCH 2/5] fix: survey questions options alignment in mobile view --- lms/static/sass/lms-course.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/static/sass/lms-course.scss b/lms/static/sass/lms-course.scss index a089484ac0..738d892257 100644 --- a/lms/static/sass/lms-course.scss +++ b/lms/static/sass/lms-course.scss @@ -27,7 +27,7 @@ @media only screen and (max-width: 767px) { .survey-table .survey-option .visible-mobile-only { - width: calc(100% - 21px) !important; + width: calc(100% - 54px) !important; } .survey-percentage .percentage { From d0140a25b9473aa81c2b9102851c30ca32f103da Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Wed, 1 Oct 2025 12:08:50 -0400 Subject: [PATCH 3/5] fix: Fix the docs build. The docs settings file needed an update for a new djangoapp that was added. Long term we need to probably think more deeply about how we want the LMS/CMS docs builds to work. Do we want to separate them, or make it easier to have a django settings file that will work with both. For now, we have a single settings file that is referenced for the docs build so it needs to have all the installed apps from both. --- docs/docs_settings.py | 1 + docs/lms-openapi.yaml | 138 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 136 insertions(+), 3 deletions(-) diff --git a/docs/docs_settings.py b/docs/docs_settings.py index a7e9963d2b..a260b5dd31 100644 --- a/docs/docs_settings.py +++ b/docs/docs_settings.py @@ -34,6 +34,7 @@ FEATURES["ENABLE_MKTG_SITE"] = False INSTALLED_APPS.extend( [ "cms.djangoapps.contentstore.apps.ContentstoreConfig", + 'cms.djangoapps.modulestore_migrator', "cms.djangoapps.course_creators", "cms.djangoapps.xblock_config.apps.XBlockConfig", "lms.djangoapps.lti_provider", diff --git a/docs/lms-openapi.yaml b/docs/lms-openapi.yaml index cd3082f46a..8057b8b2a5 100644 --- a/docs/lms-openapi.yaml +++ b/docs/lms-openapi.yaml @@ -3523,11 +3523,77 @@ paths: in: path required: true type: string + /dashboard/v0/programs/: + get: + operationId: dashboard_v0_programs_list + description: |- + For a learner, get list of enrolled programs with progress. + If an enterprise UUID ias provided, filter out all non-enterprise enrollments for the learner. + + **Example Request** + + GET /api/dashboard/v1/programs/{enterprise_uuid}/ + + **Parameters** + + * `enterprise_uuid`: UUID of an enterprise customer. + + **Example Response** + + [ + { + "uuid": "ff41a5eb-2a73-4933-8e80-a1c66068ed2c", + "title": "Demonstration Program", + "type": "MicroMasters", + "banner_image": { + "large": { + "url": "http://example.com/images/foo.large.jpg", + "width": 1440, + "height": 480 + }, + "medium": { + "url": "http://example.com/images/foo.medium.jpg", + "width": 726, + "height": 242 + }, + "small": { + "url": "http://example.com/images/foo.small.jpg", + "width": 435, + "height": 145 + }, + "x-small": { + "url": "http://example.com/images/foo.x-small.jpg", + "width": 348, + "height": 116 + } + }, + "authoring_organizations": [ + { + "key": "example" + } + ], + "progress": { + "uuid": "ff41a5eb-2a73-4933-8e80-a1c66068ed2c", + "completed": 0, + "in_progress": 0, + "not_started": 2 + } + } + ] + parameters: [] + responses: + '200': + description: '' + tags: + - dashboard + parameters: [] /dashboard/v0/programs/{enterprise_uuid}/: get: operationId: dashboard_v0_programs_read - summary: For an enterprise learner, get list of enrolled programs with progress. description: |- + For a learner, get list of enrolled programs with progress. + If an enterprise UUID ias provided, filter out all non-enterprise enrollments for the learner. + **Example Request** GET /api/dashboard/v1/programs/{enterprise_uuid}/ @@ -6446,11 +6512,77 @@ paths: tags: - learner_home parameters: [] + /learner_home/v1/programs/: + get: + operationId: learner_home_v1_programs_list + description: |- + For a learner, get list of enrolled programs with progress. + If an enterprise UUID ias provided, filter out all non-enterprise enrollments for the learner. + + **Example Request** + + GET /api/dashboard/v1/programs/{enterprise_uuid}/ + + **Parameters** + + * `enterprise_uuid`: UUID of an enterprise customer. + + **Example Response** + + [ + { + "uuid": "ff41a5eb-2a73-4933-8e80-a1c66068ed2c", + "title": "Demonstration Program", + "type": "MicroMasters", + "banner_image": { + "large": { + "url": "http://example.com/images/foo.large.jpg", + "width": 1440, + "height": 480 + }, + "medium": { + "url": "http://example.com/images/foo.medium.jpg", + "width": 726, + "height": 242 + }, + "small": { + "url": "http://example.com/images/foo.small.jpg", + "width": 435, + "height": 145 + }, + "x-small": { + "url": "http://example.com/images/foo.x-small.jpg", + "width": 348, + "height": 116 + } + }, + "authoring_organizations": [ + { + "key": "example" + } + ], + "progress": { + "uuid": "ff41a5eb-2a73-4933-8e80-a1c66068ed2c", + "completed": 0, + "in_progress": 0, + "not_started": 2 + } + } + ] + parameters: [] + responses: + '200': + description: '' + tags: + - learner_home + parameters: [] /learner_home/v1/programs/{enterprise_uuid}/: get: operationId: learner_home_v1_programs_read - summary: For an enterprise learner, get list of enrolled programs with progress. description: |- + For a learner, get list of enrolled programs with progress. + If an enterprise UUID ias provided, filter out all non-enterprise enrollments for the learner. + **Example Request** GET /api/dashboard/v1/programs/{enterprise_uuid}/ @@ -6912,7 +7044,7 @@ paths: django settings. This is a temporary change as a part of the migration of some legacy pages to MFEs. This is a temporary compatibility layer which will eventually be deprecated. - See [Link to DEPR ticket] for more details. todo: add link + See [DEPR ticket](https://github.com/openedx/edx-platform/issues/37210) for more details. The compatability means that settings from the legacy locations will continue to work but the settings listed below in the `_get_legacy_config` function should be added to the MFE From d382721cf854fe02e030c2825ba29144ea95bc45 Mon Sep 17 00:00:00 2001 From: "M. Tayyab Tahir Qureshi" <109274085+ttqureshi@users.noreply.github.com> Date: Fri, 3 Oct 2025 20:54:30 +0500 Subject: [PATCH 4/5] fix: support parsing the pointer-tag OLX for external XBlocks (#37133) Previously, the built-in XBlocks (problem, video, etc) could be parsed from pointer tag syntax, but externally-defined XBlocks (drag-and-drop, ORA, etc.) could only be parsed from inline syntax. This is because the built-in blocks have special parsing logic, defined in XmlMixin, which is not available to external blocks. This PR shifts the pointer tag parsing "up a level" such that the parent blocks parse the pointer tag, regardless of whether the child is built-in or external: * vertical (aka unit) * split_test (aka content experiment) * itembank (aka problem bank) * library_content (aka randomized legacy library) The following parent blocks still lack support for external pointer-tag children; we will fix this in a follow-up PR: * randomize * all externally defined container blocks Part of: https://github.com/openedx/XBlock/issues/823 --- xmodule/conditional_block.py | 16 ++++++++++++++-- xmodule/item_bank_block.py | 18 ++++++++++++++++-- xmodule/modulestore/xml.py | 20 +++++++++++++++++--- xmodule/tests/xml/__init__.py | 7 ++++--- xmodule/vertical_block.py | 18 ++++++++++++++++-- xmodule/x_module.py | 15 +++++++++++---- 6 files changed, 78 insertions(+), 16 deletions(-) diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 78a5b8c1c1..308d38bc18 100644 --- a/xmodule/conditional_block.py +++ b/xmodule/conditional_block.py @@ -20,7 +20,7 @@ from xmodule.seq_block import SequenceMixin from xmodule.studio_editable import StudioEditableBlock from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.xml_block import XmlMixin +from xmodule.xml_block import XmlMixin, is_pointer_tag from xmodule.x_module import ( ResourceTemplates, shim_xmodule_js, @@ -335,7 +335,19 @@ class ConditionalBlock( show_tag_list.append(location) else: try: - block = system.process_xml(etree.tostring(child, encoding='unicode')) + def_id = None + def_loaded = False + + if is_pointer_tag(child): + def_id = system.id_generator.create_definition(child.tag, child.get('url_name')) + child, _ = cls.load_definition_xml(child, system, def_id) + def_loaded = True + + block = system.process_xml( + etree.tostring(child, encoding='unicode'), + def_id, + def_loaded=def_loaded, + ) children.append(block.scope_ids.usage_id) except: # lint-amnesty, pylint: disable=bare-except msg = "Unable to load child when parsing Conditional." diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index b53617e2c8..3866aac985 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -24,7 +24,7 @@ from xmodule.mako_block import MakoTemplateBlockBase from xmodule.studio_editable import StudioEditableBlock from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.xml_block import XmlMixin +from xmodule.xml_block import XmlMixin, is_pointer_tag from xmodule.x_module import ( ResourceTemplates, XModuleMixin, @@ -406,7 +406,21 @@ class ItemBankMixin( for child in xml_object.getchildren(): try: - children.append(system.process_xml(etree.tostring(child)).scope_ids.usage_id) + def_id = None + def_loaded = False + + if is_pointer_tag(child): + def_id = system.id_generator.create_definition(child.tag, child.get('url_name')) + child, _ = cls.load_definition_xml(child, system, def_id) + def_loaded = True + + child_block = system.process_xml( + etree.tostring(child, encoding='unicode'), + def_id, + def_loaded=def_loaded, + ) + + children.append(child_block.scope_ids.usage_id) except (XMLSyntaxError, AttributeError): msg = ( "Unable to load child when parsing {self.usage_key.block_type} Block. " diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py index 738035b194..af28f21ff4 100644 --- a/xmodule/modulestore/xml.py +++ b/xmodule/modulestore/xml.py @@ -65,9 +65,21 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # lint-amnesty, pyl self.load_error_blocks = load_error_blocks self.modulestore = xmlstore - def process_xml(xml): # lint-amnesty, pylint: disable=too-many-statements - """Takes an xml string, and returns a XBlock created from + def process_xml(xml, def_id=None, def_loaded=False): # lint-amnesty, pylint: disable=too-many-statements + """ + Takes an xml string, and returns an XBlock created from that xml. + + Args: + xml (string): A string containing xml. + def_id (BlockUsageLocator): The :class:`BlockUsageLocator` to use as the + definition_id for the block. + def_loaded (bool): Indicates if the pointer tag definition is loaded from + from the pointed-to file. + + Returns: + XBlock: The fully instantiated :class:`~xblock.core.XBlock`. + """ def make_name_unique(xml_data): @@ -159,11 +171,13 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # lint-amnesty, pyl try: xml_data = etree.fromstring(xml) - make_name_unique(xml_data) + if not def_loaded: + make_name_unique(xml_data) block = self.xblock_from_node( xml_data, None, # parent_id id_manager, + def_id, ) except Exception as err: # pylint: disable=broad-except if not self.load_error_blocks: diff --git a/xmodule/tests/xml/__init__.py b/xmodule/tests/xml/__init__.py index 90545f1b88..835bc605dd 100644 --- a/xmodule/tests/xml/__init__.py +++ b/xmodule/tests/xml/__init__.py @@ -42,13 +42,14 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable ) self.id_generator = Mock() - def process_xml(self, xml): # pylint: disable=method-hidden + def process_xml(self, xml, def_id=None, def_loaded=False): # pylint: disable=method-hidden """Parse `xml` as an XBlock, and add it to `self._blocks`""" self.get_asides = Mock(return_value=[]) block = self.xblock_from_node( etree.fromstring(xml), None, CourseLocationManager(self.course_id), + def_id, ) self._blocks[str(block.location)] = block return block @@ -61,7 +62,7 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable class XModuleXmlImportTest(TestCase): """Base class for tests that use basic XML parsing""" @classmethod - def process_xml(cls, xml_import_data): + def process_xml(cls, xml_import_data, def_id=None, def_loaded=False): """Use the `xml_import_data` to import an :class:`XBlock` from XML.""" system = InMemorySystem(xml_import_data) - return system.process_xml(xml_import_data.xml_string) + return system.process_xml(xml_import_data.xml_string, def_id) diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index 150e631085..1f078ad8ee 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -23,7 +23,7 @@ from xmodule.studio_editable import StudioEditableBlock from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.util.misc import is_xblock_an_assignment from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW, XModuleFields -from xmodule.xml_block import XmlMixin +from xmodule.xml_block import XmlMixin, is_pointer_tag log = logging.getLogger(__name__) @@ -250,7 +250,21 @@ class VerticalBlock( children = [] for child in xml_object: try: - child_block = system.process_xml(etree.tostring(child, encoding='unicode')) + def_id = None + def_loaded = False + + if is_pointer_tag(child): + id_generator = system.id_generator + def_id = id_generator.create_definition(child.tag, child.get('url_name')) + + child, _ = cls.load_definition_xml(child, system, def_id) + def_loaded = True + + child_block = system.process_xml( + etree.tostring(child, encoding='unicode'), + def_id, + def_loaded=def_loaded, + ) children.append(child_block.scope_ids.usage_id) except Exception as exc: # pylint: disable=broad-except log.exception("Unable to load child when parsing Vertical. Continuing...") diff --git a/xmodule/x_module.py b/xmodule/x_module.py index d1d23342b7..22fdddd8ae 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1543,16 +1543,20 @@ class XMLParsingSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstr """ return self.xblock_from_node(node, parent_id, self.id_generator).scope_ids.usage_id - def xblock_from_node(self, node, parent_id, id_generator=None): + def xblock_from_node(self, node, parent_id, id_generator=None, def_id=None): """ Create an XBlock instance from XML data. Args: - xml_data (string): A string containing valid xml. + node (RestrictedElement): The :class:`openedx.core.lib.safe_lxml.xmlparser.RestrictedElement` + containing the XML data to parse. system (XMLParsingSystem): The :class:`.XMLParsingSystem` used to connect the block to the outside world. id_generator (IdGenerator): An :class:`~xblock.runtime.IdGenerator` that - will be used to construct the usage_id and definition_id for the block. + will be used to construct the usage_id and definition_id for the block + if `def_id` is None. + def_id (BlockUsageLocator): The :class:`BlockUsageLocator` to use as the + definition_id for the block. If None, a new definition_id will be generated. Returns: XBlock: The fully instantiated :class:`~xblock.core.XBlock`. @@ -1567,7 +1571,10 @@ class XMLParsingSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstr node.attrib.pop('xblock-family', None) url_name = node.get('url_name') # difference from XBlock.runtime - def_id = id_generator.create_definition(block_type, url_name) + + if not def_id: + def_id = id_generator.create_definition(block_type, url_name) + usage_id = id_generator.create_usage(def_id) keys = ScopeIds(None, block_type, def_id, usage_id) From 0c77083e3ae7bff1517a94acc8acaba51f123c03 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Fri, 3 Oct 2025 15:57:26 -0400 Subject: [PATCH 5/5] revert: fix: support parsing the pointer-tag OLX for external XBlocks (#37426) This reverts commit d382721cf854fe02e030c2825ba29144ea95bc45, which seems to break edx-platform's ability to load the content of externally- defined xblocks (e.g. lti_consumer) from course exports. --- xmodule/conditional_block.py | 16 ++-------------- xmodule/item_bank_block.py | 18 ++---------------- xmodule/modulestore/xml.py | 20 +++----------------- xmodule/tests/xml/__init__.py | 7 +++---- xmodule/vertical_block.py | 18 ++---------------- xmodule/x_module.py | 15 ++++----------- 6 files changed, 16 insertions(+), 78 deletions(-) diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 308d38bc18..78a5b8c1c1 100644 --- a/xmodule/conditional_block.py +++ b/xmodule/conditional_block.py @@ -20,7 +20,7 @@ from xmodule.seq_block import SequenceMixin from xmodule.studio_editable import StudioEditableBlock from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.xml_block import XmlMixin, is_pointer_tag +from xmodule.xml_block import XmlMixin from xmodule.x_module import ( ResourceTemplates, shim_xmodule_js, @@ -335,19 +335,7 @@ class ConditionalBlock( show_tag_list.append(location) else: try: - def_id = None - def_loaded = False - - if is_pointer_tag(child): - def_id = system.id_generator.create_definition(child.tag, child.get('url_name')) - child, _ = cls.load_definition_xml(child, system, def_id) - def_loaded = True - - block = system.process_xml( - etree.tostring(child, encoding='unicode'), - def_id, - def_loaded=def_loaded, - ) + block = system.process_xml(etree.tostring(child, encoding='unicode')) children.append(block.scope_ids.usage_id) except: # lint-amnesty, pylint: disable=bare-except msg = "Unable to load child when parsing Conditional." diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index 3866aac985..b53617e2c8 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -24,7 +24,7 @@ from xmodule.mako_block import MakoTemplateBlockBase from xmodule.studio_editable import StudioEditableBlock from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.xml_block import XmlMixin, is_pointer_tag +from xmodule.xml_block import XmlMixin from xmodule.x_module import ( ResourceTemplates, XModuleMixin, @@ -406,21 +406,7 @@ class ItemBankMixin( for child in xml_object.getchildren(): try: - def_id = None - def_loaded = False - - if is_pointer_tag(child): - def_id = system.id_generator.create_definition(child.tag, child.get('url_name')) - child, _ = cls.load_definition_xml(child, system, def_id) - def_loaded = True - - child_block = system.process_xml( - etree.tostring(child, encoding='unicode'), - def_id, - def_loaded=def_loaded, - ) - - children.append(child_block.scope_ids.usage_id) + children.append(system.process_xml(etree.tostring(child)).scope_ids.usage_id) except (XMLSyntaxError, AttributeError): msg = ( "Unable to load child when parsing {self.usage_key.block_type} Block. " diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py index af28f21ff4..738035b194 100644 --- a/xmodule/modulestore/xml.py +++ b/xmodule/modulestore/xml.py @@ -65,21 +65,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # lint-amnesty, pyl self.load_error_blocks = load_error_blocks self.modulestore = xmlstore - def process_xml(xml, def_id=None, def_loaded=False): # lint-amnesty, pylint: disable=too-many-statements - """ - Takes an xml string, and returns an XBlock created from + def process_xml(xml): # lint-amnesty, pylint: disable=too-many-statements + """Takes an xml string, and returns a XBlock created from that xml. - - Args: - xml (string): A string containing xml. - def_id (BlockUsageLocator): The :class:`BlockUsageLocator` to use as the - definition_id for the block. - def_loaded (bool): Indicates if the pointer tag definition is loaded from - from the pointed-to file. - - Returns: - XBlock: The fully instantiated :class:`~xblock.core.XBlock`. - """ def make_name_unique(xml_data): @@ -171,13 +159,11 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # lint-amnesty, pyl try: xml_data = etree.fromstring(xml) - if not def_loaded: - make_name_unique(xml_data) + make_name_unique(xml_data) block = self.xblock_from_node( xml_data, None, # parent_id id_manager, - def_id, ) except Exception as err: # pylint: disable=broad-except if not self.load_error_blocks: diff --git a/xmodule/tests/xml/__init__.py b/xmodule/tests/xml/__init__.py index 835bc605dd..90545f1b88 100644 --- a/xmodule/tests/xml/__init__.py +++ b/xmodule/tests/xml/__init__.py @@ -42,14 +42,13 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable ) self.id_generator = Mock() - def process_xml(self, xml, def_id=None, def_loaded=False): # pylint: disable=method-hidden + def process_xml(self, xml): # pylint: disable=method-hidden """Parse `xml` as an XBlock, and add it to `self._blocks`""" self.get_asides = Mock(return_value=[]) block = self.xblock_from_node( etree.fromstring(xml), None, CourseLocationManager(self.course_id), - def_id, ) self._blocks[str(block.location)] = block return block @@ -62,7 +61,7 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable class XModuleXmlImportTest(TestCase): """Base class for tests that use basic XML parsing""" @classmethod - def process_xml(cls, xml_import_data, def_id=None, def_loaded=False): + def process_xml(cls, xml_import_data): """Use the `xml_import_data` to import an :class:`XBlock` from XML.""" system = InMemorySystem(xml_import_data) - return system.process_xml(xml_import_data.xml_string, def_id) + return system.process_xml(xml_import_data.xml_string) diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index 1f078ad8ee..150e631085 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -23,7 +23,7 @@ from xmodule.studio_editable import StudioEditableBlock from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.util.misc import is_xblock_an_assignment from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW, XModuleFields -from xmodule.xml_block import XmlMixin, is_pointer_tag +from xmodule.xml_block import XmlMixin log = logging.getLogger(__name__) @@ -250,21 +250,7 @@ class VerticalBlock( children = [] for child in xml_object: try: - def_id = None - def_loaded = False - - if is_pointer_tag(child): - id_generator = system.id_generator - def_id = id_generator.create_definition(child.tag, child.get('url_name')) - - child, _ = cls.load_definition_xml(child, system, def_id) - def_loaded = True - - child_block = system.process_xml( - etree.tostring(child, encoding='unicode'), - def_id, - def_loaded=def_loaded, - ) + child_block = system.process_xml(etree.tostring(child, encoding='unicode')) children.append(child_block.scope_ids.usage_id) except Exception as exc: # pylint: disable=broad-except log.exception("Unable to load child when parsing Vertical. Continuing...") diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 22fdddd8ae..d1d23342b7 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1543,20 +1543,16 @@ class XMLParsingSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstr """ return self.xblock_from_node(node, parent_id, self.id_generator).scope_ids.usage_id - def xblock_from_node(self, node, parent_id, id_generator=None, def_id=None): + def xblock_from_node(self, node, parent_id, id_generator=None): """ Create an XBlock instance from XML data. Args: - node (RestrictedElement): The :class:`openedx.core.lib.safe_lxml.xmlparser.RestrictedElement` - containing the XML data to parse. + xml_data (string): A string containing valid xml. system (XMLParsingSystem): The :class:`.XMLParsingSystem` used to connect the block to the outside world. id_generator (IdGenerator): An :class:`~xblock.runtime.IdGenerator` that - will be used to construct the usage_id and definition_id for the block - if `def_id` is None. - def_id (BlockUsageLocator): The :class:`BlockUsageLocator` to use as the - definition_id for the block. If None, a new definition_id will be generated. + will be used to construct the usage_id and definition_id for the block. Returns: XBlock: The fully instantiated :class:`~xblock.core.XBlock`. @@ -1571,10 +1567,7 @@ class XMLParsingSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstr node.attrib.pop('xblock-family', None) url_name = node.get('url_name') # difference from XBlock.runtime - - if not def_id: - def_id = id_generator.create_definition(block_type, url_name) - + def_id = id_generator.create_definition(block_type, url_name) usage_id = id_generator.create_usage(def_id) keys = ScopeIds(None, block_type, def_id, usage_id)