From 0d0503a716f1e03b76d4e83c70c912f45ffd1bb2 Mon Sep 17 00:00:00 2001 From: Kyrylo Kireiev <90455454+KyryloKireiev@users.noreply.github.com> Date: Mon, 8 Apr 2024 13:03:33 +0300 Subject: [PATCH 001/260] feat: [AXM-200] Implement user's enrolments status API (#2530) * feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <81557788+NiedielnitsevIvan@users.noreply.github.com> Co-authored-by: Glib Glugovskiy --- lms/djangoapps/mobile_api/users/tests.py | 135 +++++++++++++++++++++++ lms/djangoapps/mobile_api/users/urls.py | 7 +- lms/djangoapps/mobile_api/users/views.py | 129 ++++++++++++++++++++++ 3 files changed, 269 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 65b1fba65ce3..25b2ab2b2aa8 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -9,6 +9,7 @@ import ddt import pytz +from completion.models import BlockCompletion from completion.test_utils import CompletionWaffleTestMixin, submit_completions_for_testing from django.conf import settings from django.db import transaction @@ -768,3 +769,137 @@ def test_discussion_tab_url(self, discussion_tab_enabled): assert isinstance(discussion_url, str) else: assert discussion_url is None + + +@ddt.ddt +class UserEnrollmentsStatus(MobileAPITestCase, MobileAuthUserTestMixin): + """ + Tests for /api/mobile/{api_version}/users//enrollments_status/ + """ + + REVERSE_INFO = {'name': 'user-enrollments-status', 'params': ['username', 'api_version']} + + def test_no_mobile_available_courses(self) -> None: + self.login() + courses = [CourseFactory.create(org="edx", mobile_available=False) for _ in range(3)] + for course in courses: + self.enroll(course.id) + + response = self.api_response(api_version=API_V1) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertListEqual(response.data, []) + + def test_no_enrollments(self) -> None: + self.login() + for _ in range(3): + CourseFactory.create(org="edx", mobile_available=True) + + response = self.api_response(api_version=API_V1) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertListEqual(response.data, []) + + def test_user_have_only_active_enrollments_and_no_completions(self) -> None: + self.login() + courses = [CourseFactory.create(org="edx", mobile_available=True) for _ in range(3)] + for course in courses: + self.enroll(course.id) + + response = self.api_response(api_version=API_V1) + + expected_response = [ + {'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'is_active': True}, + {'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'is_active': True}, + {'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'is_active': True}, + ] + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertListEqual(response.data, expected_response) + + def test_user_have_active_and_inactive_enrollments_and_no_completions(self) -> None: + self.login() + courses = [CourseFactory.create(org="edx", mobile_available=True) for _ in range(3)] + for course in courses: + self.enroll(course.id) + old_course = CourseFactory.create(org="edx", mobile_available=True) + self.enroll(old_course.id) + old_enrollment = CourseEnrollment.objects.filter(user=self.user, course=old_course.course_id).first() + old_enrollment.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=31) + old_enrollment.save() + + response = self.api_response(api_version=API_V1) + + expected_response = [ + {'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'is_active': True}, + {'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'is_active': True}, + {'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'is_active': True}, + {'course_id': str(old_course.course_id), 'course_name': old_course.display_name, 'is_active': False} + ] + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertListEqual(response.data, expected_response) + + @ddt.data( + (27, True), + (28, True), + (29, True), + (31, False), + (32, False), + ) + @ddt.unpack + def test_different_enrollment_dates(self, enrolled_days_ago: int, is_active_status: bool) -> None: + self.login() + course = CourseFactory.create(org="edx", mobile_available=True, run='1001') + self.enroll(course.id) + enrollment = CourseEnrollment.objects.filter(user=self.user, course=course.course_id).first() + enrollment.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=enrolled_days_ago) + enrollment.save() + + response = self.api_response(api_version=API_V1) + + expected_response = [ + {'course_id': str(course.course_id), 'course_name': course.display_name, 'is_active': is_active_status} + ] + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertListEqual(response.data, expected_response) + + @ddt.data( + (27, True), + (28, True), + (29, True), + (31, False), + (32, False), + ) + @ddt.unpack + def test_different_completion_dates(self, completed_days_ago: int, is_active_status: bool) -> None: + self.login() + course = CourseFactory.create(org="edx", mobile_available=True, run='1010') + section = BlockFactory.create( + parent=course, + category='chapter', + ) + self.enroll(course.id) + enrollment = CourseEnrollment.objects.filter(user=self.user, course=course.course_id).first() + # make enrollment older 30 days ago + enrollment.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=50) + enrollment.save() + completion = BlockCompletion.objects.create( + user=self.user, + context_key=course.context_key, + block_type='course', + block_key=section.location, + completion=0.5, + ) + completion.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=completed_days_ago) + completion.save() + + response = self.api_response(api_version=API_V1) + + expected_response = [ + {'course_id': str(course.course_id), 'course_name': course.display_name, 'is_active': is_active_status} + ] + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertListEqual(response.data, expected_response) diff --git a/lms/djangoapps/mobile_api/users/urls.py b/lms/djangoapps/mobile_api/users/urls.py index 266644246e88..874730d4d0f0 100644 --- a/lms/djangoapps/mobile_api/users/urls.py +++ b/lms/djangoapps/mobile_api/users/urls.py @@ -6,7 +6,7 @@ from django.conf import settings from django.urls import re_path -from .views import UserCourseEnrollmentsList, UserCourseStatus, UserDetail +from .views import UserCourseEnrollmentsList, UserCourseStatus, UserDetail, UserEnrollmentsStatus urlpatterns = [ re_path('^' + settings.USERNAME_PATTERN + '$', UserDetail.as_view(), name='user-detail'), @@ -17,5 +17,8 @@ ), re_path(f'^{settings.USERNAME_PATTERN}/course_status_info/{settings.COURSE_ID_PATTERN}', UserCourseStatus.as_view(), - name='user-course-status') + name='user-course-status'), + re_path(f'^{settings.USERNAME_PATTERN}/enrollments_status/', + UserEnrollmentsStatus.as_view(), + name='user-enrollments-status') ] diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 049678dcd7ba..2b6264263061 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -3,9 +3,13 @@ """ +import datetime import logging +from typing import Dict, List, Optional +import pytz from completion.exceptions import UnavailableCompletionData +from completion.models import BlockCompletion from completion.utilities import get_key_to_last_completed_block from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.contrib.auth.signals import user_logged_in @@ -410,3 +414,128 @@ def my_user_info(request, api_version): # updating it from the oauth2 related code is too complex user_logged_in.send(sender=User, user=request.user, request=request) return redirect("user-detail", api_version=api_version, username=request.user.username) + + +class UserCourseEnrollmentsV4Pagination(DefaultPagination): + """ + Pagination for `UserCourseEnrollments` API v4. + """ + page_size = 5 + max_page_size = 50 + + +@mobile_view(is_user=True) +class UserEnrollmentsStatus(views.APIView): + """ + **Use Case** + + Get information about user's enrolments status. + + Returns active enrolment status if user was enrolled for the course + less than 30 days ago or has progressed in the course in the last 30 days. + Otherwise, the registration is considered inactive. + + **Example Request** + + GET /api/mobile/{api_version}/users//enrollments_status/ + + **Response Values** + + If the request for information about the user's enrolments is successful, the + request returns an HTTP 200 "OK" response. + + The HTTP 200 response has the following values. + + * course_id (str): The course id associated with the user's enrollment. + * course_name (str): The course name associated with the user's enrollment. + * is_active (bool): User's course enrolment status. + + + The HTTP 200 response contains a list of dictionaries that contain info + about each user's enrolment status. + + **Example Response** + + ```json + [ + { + "course_id": "course-v1:a+a+a", + "course_name": "a", + "is_active": true + }, + { + "course_id": "course-v1:b+b+b", + "course_name": "b", + "is_active": true + }, + { + "course_id": "course-v1:c+c+c", + "course_name": "c", + "is_active": false + }, + ... + ] + ``` + """ + def get(self, request, *args, **kwargs) -> Response: + """ + Gets user's enrollments status. + """ + active_status_date = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30) + username = kwargs.get('username') + course_ids_where_user_has_completions = self._get_course_ids_where_user_has_completions( + username, + active_status_date, + ) + enrollments_status = self._build_enrollments_status_dict( + username, + active_status_date, + course_ids_where_user_has_completions + ) + return Response(enrollments_status) + + def _build_enrollments_status_dict( + self, + username: str, + active_status_date: datetime, + course_ids: List[str], + ) -> List[Dict[str, bool]]: + """ + Builds list with dictionaries with user's enrolments statuses. + """ + user_enrollments = CourseEnrollment.objects.filter( + user__username=username, + is_active=True, + ) + mobile_available = [ + enrollment for enrollment in user_enrollments + if is_mobile_available_for_user(self.request.user, enrollment.course_overview) + ] + enrollments_status = [] + for user_enrollment in mobile_available: + course_id = str(user_enrollment.course_overview.id) + enrollments_status.append( + { + 'course_id': course_id, + 'course_name': user_enrollment.course_overview.display_name, + 'is_active': bool( + course_id in course_ids + or user_enrollment.created > active_status_date + ) + } + ) + return enrollments_status + + @staticmethod + def _get_course_ids_where_user_has_completions( + username: str, + active_status_date: datetime, + ) -> List[str]: + """ + Gets course ids where user has completions. + """ + user_completions_last_month = BlockCompletion.objects.filter( + user__username=username, + created__gte=active_status_date + ) + return [str(completion.block_key.course_key) for completion in user_completions_last_month] From bd8b35d0d84c42e8227bc7f0c7d3e0f8e8fae8c2 Mon Sep 17 00:00:00 2001 From: KyryloKireiev Date: Mon, 27 May 2024 14:42:16 +0300 Subject: [PATCH 002/260] fix: [AXM-549] Added missing import --- lms/djangoapps/mobile_api/users/tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 25b2ab2b2aa8..4ebda3ecd47f 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -19,6 +19,7 @@ from django.utils.timezone import now from milestones.tests.utils import MilestonesTestCaseMixin from opaque_keys.edx.keys import CourseKey +from rest_framework import status from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment From e474abda4fd00bebcd9f2a92a81b636dbe93b7fe Mon Sep 17 00:00:00 2001 From: KyryloKireiev Date: Mon, 27 May 2024 18:14:12 +0300 Subject: [PATCH 003/260] style: [AXM-549] Remove unused import --- lms/djangoapps/mobile_api/users/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 2b6264263061..0be90f255cb2 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -5,7 +5,7 @@ import datetime import logging -from typing import Dict, List, Optional +from typing import Dict, List import pytz from completion.exceptions import UnavailableCompletionData From 1a7f55b01e273739802a06fcaa3b3bc05c84ca55 Mon Sep 17 00:00:00 2001 From: KyryloKireiev Date: Fri, 21 Jun 2024 18:30:47 +0300 Subject: [PATCH 004/260] refactor: [AXM-549] Refactor UserEnrollmentsStatus API --- lms/djangoapps/mobile_api/users/tests.py | 2 +- lms/djangoapps/mobile_api/users/views.py | 18 ++++-------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 4ebda3ecd47f..df7c4382971e 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -773,7 +773,7 @@ def test_discussion_tab_url(self, discussion_tab_enabled): @ddt.ddt -class UserEnrollmentsStatus(MobileAPITestCase, MobileAuthUserTestMixin): +class TestUserEnrollmentsStatus(MobileAPITestCase, MobileAuthUserTestMixin): """ Tests for /api/mobile/{api_version}/users//enrollments_status/ """ diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 0be90f255cb2..03149157811a 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -14,7 +14,7 @@ from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.contrib.auth.signals import user_logged_in from django.db import transaction -from django.shortcuts import redirect +from django.shortcuts import get_object_or_404, redirect from django.utils import dateparse from django.utils.decorators import method_decorator from opaque_keys import InvalidKeyError @@ -416,14 +416,6 @@ def my_user_info(request, api_version): return redirect("user-detail", api_version=api_version, username=request.user.username) -class UserCourseEnrollmentsV4Pagination(DefaultPagination): - """ - Pagination for `UserCourseEnrollments` API v4. - """ - page_size = 5 - max_page_size = 50 - - @mobile_view(is_user=True) class UserEnrollmentsStatus(views.APIView): """ @@ -503,13 +495,11 @@ def _build_enrollments_status_dict( """ Builds list with dictionaries with user's enrolments statuses. """ - user_enrollments = CourseEnrollment.objects.filter( - user__username=username, - is_active=True, - ) + user = get_object_or_404(User, username=username) + user_enrollments = CourseEnrollment.enrollments_for_user(user).select_related('course') mobile_available = [ enrollment for enrollment in user_enrollments - if is_mobile_available_for_user(self.request.user, enrollment.course_overview) + if is_mobile_available_for_user(user, enrollment.course_overview) ] enrollments_status = [] for user_enrollment in mobile_available: From 1eec8b80e43a0fd4c746aa53c5d04ba30be39088 Mon Sep 17 00:00:00 2001 From: KyryloKireiev Date: Tue, 8 Oct 2024 17:47:43 +0300 Subject: [PATCH 005/260] fix: [AXM-549] Use more efficient query --- lms/djangoapps/mobile_api/users/views.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 091eb054038a..d634b3072dab 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -641,11 +641,12 @@ def _get_course_ids_where_user_has_completions( """ Gets course ids where user has completions. """ - user_completions_last_month = BlockCompletion.objects.filter( + context_keys = BlockCompletion.objects.filter( user__username=username, created__gte=active_status_date - ) - return [str(completion.block_key.course_key) for completion in user_completions_last_month] + ).values_list('context_key', flat=True).distinct() + + return [str(context_key) for context_key in context_keys] class UserCourseEnrollmentsV4Pagination(DefaultPagination): From bf073fb68c69ea3b586ec5cd5a0f2c25068aa00e Mon Sep 17 00:00:00 2001 From: KyryloKireiev Date: Thu, 17 Oct 2024 18:06:03 +0300 Subject: [PATCH 006/260] refactor: [AXM-549] Change enrollments status API field name --- lms/djangoapps/mobile_api/users/tests.py | 30 +++++++++++++++--------- lms/djangoapps/mobile_api/users/views.py | 10 ++++---- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 6a0ef0a72fdf..2cfefaa058d9 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -1422,9 +1422,9 @@ def test_user_have_only_active_enrollments_and_no_completions(self) -> None: response = self.api_response(api_version=API_V1) expected_response = [ - {'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'is_active': True}, - {'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'is_active': True}, - {'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'is_active': True}, + {'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'recently_active': True}, + {'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'recently_active': True}, + {'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'recently_active': True}, ] self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -1444,10 +1444,10 @@ def test_user_have_active_and_inactive_enrollments_and_no_completions(self) -> N response = self.api_response(api_version=API_V1) expected_response = [ - {'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'is_active': True}, - {'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'is_active': True}, - {'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'is_active': True}, - {'course_id': str(old_course.course_id), 'course_name': old_course.display_name, 'is_active': False} + {'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'recently_active': True}, + {'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'recently_active': True}, + {'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'recently_active': True}, + {'course_id': str(old_course.course_id), 'course_name': old_course.display_name, 'recently_active': False} ] self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -1461,7 +1461,7 @@ def test_user_have_active_and_inactive_enrollments_and_no_completions(self) -> N (32, False), ) @ddt.unpack - def test_different_enrollment_dates(self, enrolled_days_ago: int, is_active_status: bool) -> None: + def test_different_enrollment_dates(self, enrolled_days_ago: int, recently_active_status: bool) -> None: self.login() course = CourseFactory.create(org="edx", mobile_available=True, run='1001') self.enroll(course.id) @@ -1472,7 +1472,11 @@ def test_different_enrollment_dates(self, enrolled_days_ago: int, is_active_stat response = self.api_response(api_version=API_V1) expected_response = [ - {'course_id': str(course.course_id), 'course_name': course.display_name, 'is_active': is_active_status} + { + 'course_id': str(course.course_id), + 'course_name': course.display_name, + 'recently_active': recently_active_status + } ] self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -1486,7 +1490,7 @@ def test_different_enrollment_dates(self, enrolled_days_ago: int, is_active_stat (32, False), ) @ddt.unpack - def test_different_completion_dates(self, completed_days_ago: int, is_active_status: bool) -> None: + def test_different_completion_dates(self, completed_days_ago: int, recently_active_status: bool) -> None: self.login() course = CourseFactory.create(org="edx", mobile_available=True, run='1010') section = BlockFactory.create( @@ -1511,7 +1515,11 @@ def test_different_completion_dates(self, completed_days_ago: int, is_active_sta response = self.api_response(api_version=API_V1) expected_response = [ - {'course_id': str(course.course_id), 'course_name': course.display_name, 'is_active': is_active_status} + { + 'course_id': str(course.course_id), + 'course_name': course.display_name, + 'recently_active': recently_active_status + } ] self.assertEqual(response.status_code, status.HTTP_200_OK) diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index d634b3072dab..b37a797403a0 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -557,7 +557,7 @@ class UserEnrollmentsStatus(views.APIView): * course_id (str): The course id associated with the user's enrollment. * course_name (str): The course name associated with the user's enrollment. - * is_active (bool): User's course enrolment status. + * recently_active (bool): User's course enrolment status. The HTTP 200 response contains a list of dictionaries that contain info @@ -570,17 +570,17 @@ class UserEnrollmentsStatus(views.APIView): { "course_id": "course-v1:a+a+a", "course_name": "a", - "is_active": true + "recently_active": true }, { "course_id": "course-v1:b+b+b", "course_name": "b", - "is_active": true + "recently_active": true }, { "course_id": "course-v1:c+c+c", "course_name": "c", - "is_active": false + "recently_active": false }, ... ] @@ -625,7 +625,7 @@ def _build_enrollments_status_dict( { 'course_id': course_id, 'course_name': user_enrollment.course_overview.display_name, - 'is_active': bool( + 'recently_active': bool( course_id in course_ids or user_enrollment.created > active_status_date ) From 795d03958116f34a535d1c65be7f24a3d9b6e0f9 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 17 Oct 2024 12:02:26 -0400 Subject: [PATCH 007/260] feat: Upstream Sync with Content Library Blocks (#34925) This introdues the idea of "upstream" and "downstream" content, where downstreams (like course components) can pull content updates from upstreams (like learning core-backed content library blocks). This supports the upcoming Content Libraries Relaunch Beta for Sumac. New features include: * A new XBlockMixin: UpstreamSyncMixin. * A new CMS Python API: cms.lib.xblock.upstream_sync * A new CMS JSON API: /api/contentstore/v2/downstreams * A temporary, very basic UI for syncing from Content Library blocks Implements: https://github.com/kdmccormick/edx-platform/blob/kdmccormick/upstream-proto/docs/decisions/0020-upstream-block.rst Co-authored-by: Braden MacDonald --- cms/djangoapps/contentstore/helpers.py | 47 +- .../rest_api/v1/serializers/vertical_block.py | 13 + .../v1/views/tests/test_vertical_block.py | 12 + .../rest_api/v1/views/vertical_block.py | 23 + .../contentstore/rest_api/v2/urls.py | 24 +- .../rest_api/v2/views/__init__.py | 3 - .../rest_api/v2/views/downstreams.py | 251 ++++++++++ .../v2/views/tests/test_downstreams.py | 266 +++++++++++ .../views/tests/test_clipboard_paste.py | 74 +++ .../xblock_storage_handlers/view_handlers.py | 3 +- cms/envs/common.py | 2 + cms/lib/xblock/test/test_upstream_sync.py | 337 +++++++++++++ cms/lib/xblock/upstream_sync.py | 451 ++++++++++++++++++ cms/templates/studio_xblock_wrapper.html | 32 +- docs/decisions/0020-upstream-downstream.rst | 5 + mypy.ini | 4 +- .../core/djangoapps/content_libraries/api.py | 9 +- .../core/djangoapps/content_staging/api.py | 4 +- .../core/djangoapps/content_staging/data.py | 1 + .../0005_stagedcontent_version_num.py | 18 + .../core/djangoapps/content_staging/models.py | 2 + .../core/djangoapps/content_staging/views.py | 4 +- 22 files changed, 1562 insertions(+), 23 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py create mode 100644 cms/lib/xblock/test/test_upstream_sync.py create mode 100644 cms/lib/xblock/upstream_sync.py create mode 100644 openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index a4ece6c85d59..e9f599772d3e 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -9,6 +9,7 @@ from attrs import frozen, Factory from django.conf import settings +from django.contrib.auth import get_user_model from django.utils.translation import gettext as _ from opaque_keys.edx.keys import AssetKey, CourseKey, UsageKey from opaque_keys.edx.locator import DefinitionLocator, LocalId @@ -22,6 +23,7 @@ from xmodule.xml_block import XmlMixin from cms.djangoapps.models.settings.course_grading import CourseGradingModel +from cms.lib.xblock.upstream_sync import UpstreamLink, BadUpstream, BadDownstream, fetch_customizable_fields from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers import openedx.core.djangoapps.content_staging.api as content_staging_api import openedx.core.djangoapps.content_tagging.api as content_tagging_api @@ -30,6 +32,10 @@ log = logging.getLogger(__name__) + +User = get_user_model() + + # Note: Grader types are used throughout the platform but most usages are simply in-line # strings. In addition, new grader types can be defined on the fly anytime one is needed # (because they're just strings). This dict is an attempt to constrain the sprawl in Studio. @@ -282,9 +288,10 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> node, parent_xblock, store, - user_id=request.user.id, + user=request.user, slug_hint=user_clipboard.source_usage_key.block_id, copied_from_block=str(user_clipboard.source_usage_key), + copied_from_version_num=user_clipboard.content.version_num, tags=user_clipboard.content.tags, ) # Now handle static files that need to go into Files & Uploads: @@ -293,7 +300,6 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> staged_content_id=user_clipboard.content.id, static_files=static_files, ) - return new_xblock, notices @@ -302,12 +308,15 @@ def _import_xml_node_to_parent( parent_xblock: XBlock, # The modulestore we're using store, - # The ID of the user who is performing this operation - user_id: int, + # The user who is performing this operation + user: User, # Hint to use as usage ID (block_id) for the new XBlock slug_hint: str | None = None, # UsageKey of the XBlock that this one is a copy of copied_from_block: str | None = None, + # Positive int version of source block, if applicable (e.g., library block). + # Zero if not applicable (e.g., course block). + copied_from_version_num: int = 0, # Content tags applied to the source XBlock(s) tags: dict[str, str] | None = None, ) -> XBlock: @@ -373,12 +382,32 @@ def _import_xml_node_to_parent( raise NotImplementedError("We don't yet support pasting XBlocks with children") temp_xblock.parent = parent_key if copied_from_block: - # Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin) - temp_xblock.copied_from_block = copied_from_block + # Try to link the pasted block (downstream) to the copied block (upstream). + temp_xblock.upstream = copied_from_block + try: + UpstreamLink.get_for_block(temp_xblock) + except (BadDownstream, BadUpstream): + # Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an + # upstream. That's fine! Instead, we store a reference to where this block was copied from, in the + # 'copied_from_block' field (from AuthoringMixin). + temp_xblock.upstream = None + temp_xblock.copied_from_block = copied_from_block + else: + # But if it doesn't fail, then populate the `upstream_version` field based on what was copied. Note that + # this could be the latest published version, or it could be an an even newer draft version. + temp_xblock.upstream_version = copied_from_version_num + # Also, fetch upstream values (`upstream_display_name`, etc.). + # Recall that the copied block could be a draft. So, rather than fetching from the published upstream (which + # could be older), fetch from the copied block itself. That way, if an author customizes a field, but then + # later wants to restore it, it will restore to the value that the field had when the block was pasted. Of + # course, if the author later syncs updates from a *future* published upstream version, then that will fetch + # new values from the published upstream content. + fetch_customizable_fields(upstream=temp_xblock, downstream=temp_xblock, user=user) + # Save the XBlock into modulestore. We need to save the block and its parent for this to work: - new_xblock = store.update_item(temp_xblock, user_id, allow_not_found=True) + new_xblock = store.update_item(temp_xblock, user.id, allow_not_found=True) parent_xblock.children.append(new_xblock.location) - store.update_item(parent_xblock, user_id) + store.update_item(parent_xblock, user.id) children_handled = False if hasattr(new_xblock, 'studio_post_paste'): @@ -394,7 +423,7 @@ def _import_xml_node_to_parent( child_node, new_xblock, store, - user_id=user_id, + user=user, copied_from_block=str(child_copied_from), tags=tags, ) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py index d72707ed7836..f2e8b6ef431b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py @@ -103,6 +103,18 @@ def get_assets_url(self, obj): return None +class UpstreamLinkSerializer(serializers.Serializer): + """ + Serializer holding info for syncing a block with its upstream (eg, a library block). + """ + upstream_ref = serializers.CharField() + version_synced = serializers.IntegerField() + version_available = serializers.IntegerField(allow_null=True) + version_declined = serializers.IntegerField(allow_null=True) + error_message = serializers.CharField(allow_null=True) + ready_to_sync = serializers.BooleanField() + + class ChildVerticalContainerSerializer(serializers.Serializer): """ Serializer for representing a xblock child of vertical container. @@ -113,6 +125,7 @@ class ChildVerticalContainerSerializer(serializers.Serializer): block_type = serializers.CharField() user_partition_info = serializers.DictField() user_partitions = serializers.ListField() + upstream_link = UpstreamLinkSerializer(allow_null=True) actions = serializers.SerializerMethodField() validation_messages = MessageValidation(many=True) render_error = serializers.CharField() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py index d3fc37198213..7cac074a433f 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py @@ -70,6 +70,8 @@ def setup_xblock(self): parent=self.vertical.location, category="html", display_name="Html Content 2", + upstream="lb:FakeOrg:FakeLib:html:FakeBlock", + upstream_version=5, ) def create_block(self, parent, category, display_name, **kwargs): @@ -193,6 +195,7 @@ def test_children_content(self): "name": self.html_unit_first.display_name_with_default, "block_id": str(self.html_unit_first.location), "block_type": self.html_unit_first.location.block_type, + "upstream_link": None, "user_partition_info": expected_user_partition_info, "user_partitions": expected_user_partitions, "actions": { @@ -218,12 +221,21 @@ def test_children_content(self): "can_delete": True, "can_manage_tags": True, }, + "upstream_link": { + "upstream_ref": "lb:FakeOrg:FakeLib:html:FakeBlock", + "version_synced": 5, + "version_available": None, + "version_declined": None, + "error_message": "Linked library item was not found in the system", + "ready_to_sync": False, + }, "user_partition_info": expected_user_partition_info, "user_partitions": expected_user_partitions, "validation_messages": [], "render_error": "", }, ] + self.maxDiff = None self.assertEqual(response.data["children"], expected_response) def test_not_valid_usage_key_string(self): diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py index 670b94afbbe0..0798c341cc1c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py @@ -20,6 +20,7 @@ ContainerHandlerSerializer, VerticalContainerSerializer, ) +from cms.lib.xblock.upstream_sync import UpstreamLink from openedx.core.lib.api.view_utils import view_auth_classes from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order @@ -198,6 +199,7 @@ def get(self, request: Request, usage_key_string: str): "block_type": "drag-and-drop-v2", "user_partition_info": {}, "user_partitions": {} + "upstream_link": null, "actions": { "can_copy": true, "can_duplicate": true, @@ -215,6 +217,13 @@ def get(self, request: Request, usage_key_string: str): "block_type": "video", "user_partition_info": {}, "user_partitions": {} + "upstream_link": { + "upstream_ref": "lb:org:mylib:video:404", + "version_synced": 16 + "version_available": null, + "error_message": "Linked library item not found: lb:org:mylib:video:404", + "ready_to_sync": false, + }, "actions": { "can_copy": true, "can_duplicate": true, @@ -232,6 +241,13 @@ def get(self, request: Request, usage_key_string: str): "block_type": "html", "user_partition_info": {}, "user_partitions": {}, + "upstream_link": { + "upstream_ref": "lb:org:mylib:html:abcd", + "version_synced": 43, + "version_available": 49, + "error_message": null, + "ready_to_sync": true, + }, "actions": { "can_copy": true, "can_duplicate": true, @@ -267,6 +283,7 @@ def get(self, request: Request, usage_key_string: str): child_info = modulestore().get_item(child) user_partition_info = get_visibility_partition_info(child_info, course=course) user_partitions = get_user_partition_info(child_info, course=course) + upstream_link = UpstreamLink.try_get_for_block(child_info) validation_messages = get_xblock_validation_messages(child_info) render_error = get_xblock_render_error(request, child_info) @@ -277,6 +294,12 @@ def get(self, request: Request, usage_key_string: str): "block_type": child_info.location.block_type, "user_partition_info": user_partition_info, "user_partitions": user_partitions, + "upstream_link": ( + # If the block isn't linked to an upstream (which is by far the most common case) then just + # make this field null, which communicates the same info, but with less noise. + upstream_link.to_json() if upstream_link.upstream_ref + else None + ), "validation_messages": validation_messages, "render_error": render_error, }) diff --git a/cms/djangoapps/contentstore/rest_api/v2/urls.py b/cms/djangoapps/contentstore/rest_api/v2/urls.py index ad61cc937015..3e653d07fbcf 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v2/urls.py @@ -1,15 +1,31 @@ """Contenstore API v2 URLs.""" -from django.urls import path - -from cms.djangoapps.contentstore.rest_api.v2.views import HomePageCoursesViewV2 +from django.conf import settings +from django.urls import path, re_path +from cms.djangoapps.contentstore.rest_api.v2.views import home, downstreams app_name = "v2" urlpatterns = [ path( "home/courses", - HomePageCoursesViewV2.as_view(), + home.HomePageCoursesViewV2.as_view(), name="courses", ), + # TODO: Potential future path. + # re_path( + # fr'^downstreams/$', + # downstreams.DownstreamsListView.as_view(), + # name="downstreams_list", + # ), + re_path( + fr'^downstreams/{settings.USAGE_KEY_PATTERN}$', + downstreams.DownstreamView.as_view(), + name="downstream" + ), + re_path( + fr'^downstreams/{settings.USAGE_KEY_PATTERN}/sync$', + downstreams.SyncFromUpstreamView.as_view(), + name="sync_from_upstream" + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py index 73ddde98440c..e69de29bb2d1 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py @@ -1,3 +0,0 @@ -"""Module for v2 views.""" - -from cms.djangoapps.contentstore.rest_api.v2.views.home import HomePageCoursesViewV2 diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py new file mode 100644 index 000000000000..5079698082be --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -0,0 +1,251 @@ +""" +API Views for managing & syncing links between upstream & downstream content + +API paths (We will move these into proper api_doc_tools annotations soon +https://github.com/openedx/edx-platform/issues/35653): + + /api/contentstore/v2/downstreams/{usage_key_string} + + GET: Inspect a single downstream block's link to upstream content. + 200: Upstream link details successfully fetched. Returns UpstreamLink (may contain an error_message). + 404: Downstream block not found or user lacks permission to edit it. + + DELETE: Sever a single downstream block's link to upstream content. + 204: Block successfully unlinked (or it wasn't linked in the first place). No response body. + 404: Downstream block not found or user lacks permission to edit it. + + PUT: Establish or modify a single downstream block's link to upstream content. An authoring client could use this + endpoint to add library content in a two-step process, specifically: (1) add a blank block to a course, then + (2) link it to a content library with ?sync=True. + REQUEST BODY: { + "upstream_ref": str, // reference to upstream block (eg, library block usage key) + "sync": bool, // whether to sync in upstream content (False by default) + } + 200: Downstream block's upstream link successfully edited (and synced, if requested). Returns UpstreamLink. + 400: upstream_ref is malformed, missing, or inaccessible. + 400: Content at upstream_ref does not support syncing. + 404: Downstream block not found or user lacks permission to edit it. + + /api/contentstore/v2/downstreams/{usage_key_string}/sync + + POST: Sync a downstream block with upstream content. + 200: Downstream block successfully synced with upstream content. + 400: Downstream block is not linked to upstream content. + 400: Upstream is malformed, missing, or inaccessible. + 400: Upstream block does not support syncing. + 404: Downstream block not found or user lacks permission to edit it. + + DELETE: Decline an available sync for a downstream block. + 204: Sync successfuly dismissed. No response body. + 400: Downstream block is not linked to upstream content. + 404: Downstream block not found or user lacks permission to edit it. + + # NOT YET IMPLEMENTED -- Will be needed for full Libraries Relaunch in ~Teak. + /api/contentstore/v2/downstreams + /api/contentstore/v2/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true + GET: List downstream blocks that can be synced, filterable by course or sync-readiness. + 200: A paginated list of applicable & accessible downstream blocks. Entries are UpstreamLinks. + +UpstreamLink response schema: + { + "upstream_ref": string? + "version_synced": string?, + "version_available": string?, + "version_declined": string?, + "error_message": string?, + "ready_to_sync": Boolean + } +""" +import logging + +from django.contrib.auth.models import User # pylint: disable=imported-auth-user +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey +from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.request import Request +from rest_framework.response import Response +from rest_framework.views import APIView +from xblock.core import XBlock + +from cms.lib.xblock.upstream_sync import ( + UpstreamLink, UpstreamLinkException, NoUpstream, BadUpstream, BadDownstream, + fetch_customizable_fields, sync_from_upstream, decline_sync, sever_upstream_link +) +from common.djangoapps.student.auth import has_studio_write_access, has_studio_read_access +from openedx.core.lib.api.view_utils import ( + DeveloperErrorViewMixin, + view_auth_classes, +) +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError + + +logger = logging.getLogger(__name__) + + +class _AuthenticatedRequest(Request): + """ + Alias for the `Request` class which tells mypy to assume that `.user` is not an AnonymousUser. + + Using this does NOT ensure the request is actually authenticated-- + you will some other way to ensure that, such as `@view_auth_classes(is_authenticated=True)`. + """ + user: User + + +# TODO: Potential future view. +# @view_auth_classes(is_authenticated=True) +# class DownstreamListView(DeveloperErrorViewMixin, APIView): +# """ +# List all blocks which are linked to upstream content, with optional filtering. +# """ +# def get(self, request: _AuthenticatedRequest) -> Response: +# """ +# Handle the request. +# """ +# course_key_string = request.GET['course_id'] +# syncable = request.GET['ready_to_sync'] +# ... + + +@view_auth_classes(is_authenticated=True) +class DownstreamView(DeveloperErrorViewMixin, APIView): + """ + Inspect or manage an XBlock's link to upstream content. + """ + def get(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Inspect an XBlock's link to upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=False) + return Response(UpstreamLink.try_get_for_block(downstream).to_json()) + + def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Edit an XBlock's link to upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + new_upstream_ref = request.data.get("upstream_ref") + + # Set `downstream.upstream` so that we can try to sync and/or fetch. + # Note that, if this fails and we raise a 4XX, then we will not call modulstore().update_item, + # thus preserving the former value of `downstream.upstream`. + downstream.upstream = new_upstream_ref + sync_param = request.data.get("sync", "false").lower() + if sync_param not in ["true", "false"]: + raise ValidationError({"sync": "must be 'true' or 'false'"}) + try: + if sync_param == "true": + sync_from_upstream(downstream=downstream, user=request.user) + else: + # Even if we're not syncing (i.e., updating the downstream's values with the upstream's), we still need + # to fetch the upstream's customizable values and store them as hidden fields on the downstream. This + # ensures that downstream authors can restore defaults based on the upstream. + fetch_customizable_fields(downstream=downstream, user=request.user) + except BadDownstream as exc: + logger.exception( + "'%s' is an invalid downstream; refusing to set its upstream to '%s'", + usage_key_string, + new_upstream_ref, + ) + raise ValidationError(str(exc)) from exc + except BadUpstream as exc: + logger.exception( + "'%s' is an invalid upstream reference; refusing to set it as upstream of '%s'", + new_upstream_ref, + usage_key_string, + ) + raise ValidationError({"upstream_ref": str(exc)}) from exc + except NoUpstream as exc: + raise ValidationError({"upstream_ref": "value missing"}) from exc + modulestore().update_item(downstream, request.user.id) + # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the + # upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen. + return Response(UpstreamLink.get_for_block(downstream).to_json()) + + def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Sever an XBlock's link to upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + try: + sever_upstream_link(downstream) + except NoUpstream as exc: + logger.exception( + "Tried to DELETE upstream link of '%s', but it wasn't linked to anything in the first place. " + "Will do nothing. ", + usage_key_string, + ) + else: + modulestore().update_item(downstream, request.user.id) + return Response(status=204) + + +@view_auth_classes(is_authenticated=True) +class SyncFromUpstreamView(DeveloperErrorViewMixin, APIView): + """ + Accept or decline an opportunity to sync a downstream block from its upstream content. + """ + + def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Pull latest updates to the block at {usage_key_string} from its linked upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + try: + sync_from_upstream(downstream, request.user) + except UpstreamLinkException as exc: + logger.exception( + "Could not sync from upstream '%s' to downstream '%s'", + downstream.upstream, + usage_key_string, + ) + raise ValidationError(detail=str(exc)) from exc + modulestore().update_item(downstream, request.user.id) + # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the + # upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen. + return Response(UpstreamLink.get_for_block(downstream).to_json()) + + def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Decline the latest updates to the block at {usage_key_string}. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + try: + decline_sync(downstream) + except (NoUpstream, BadUpstream, BadDownstream) as exc: + # This is somewhat unexpected. If the upstream link is missing or invalid, then the downstream author + # shouldn't have been prompted to accept/decline a sync in the first place. Of course, they could have just + # hit the HTTP API anyway, or they could be viewing a Studio page which hasn't been refreshed in a while. + # So, it's a 400, not a 500. + logger.exception( + "Tried to decline a sync to downstream '%s', but the upstream link '%s' is invalid.", + usage_key_string, + downstream.upstream, + ) + raise ValidationError(str(exc)) from exc + modulestore().update_item(downstream, request.user.id) + return Response(status=204) + + +def _load_accessible_block(user: User, usage_key_string: str, *, require_write_access: bool) -> XBlock: + """ + Given a logged in-user and a serialized usage key of an upstream-linked XBlock, load it from the ModuleStore, + raising a DRF-friendly exception if anything goes wrong. + + Raises NotFound if usage key is malformed, if the user lacks access, or if the block doesn't exist. + """ + not_found = NotFound(detail=f"Block not found or not accessible: {usage_key_string}") + try: + usage_key = UsageKey.from_string(usage_key_string) + except InvalidKeyError as exc: + raise ValidationError(detail=f"Malformed block usage key: {usage_key_string}") from exc + if require_write_access and not has_studio_write_access(user, usage_key.context_key): + raise not_found + if not has_studio_read_access(user, usage_key.context_key): + raise not_found + try: + block = modulestore().get_item(usage_key) + except ItemNotFoundError as exc: + raise not_found from exc + return block diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py new file mode 100644 index 000000000000..616035473e7e --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -0,0 +1,266 @@ +""" +Unit tests for /api/contentstore/v2/downstreams/* JSON APIs. +""" +from unittest.mock import patch + +from cms.lib.xblock.upstream_sync import UpstreamLink, BadUpstream +from common.djangoapps.student.tests.factories import UserFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory + +from .. import downstreams as downstreams_views + + +MOCK_UPSTREAM_REF = "mock-upstream-ref" +MOCK_UPSTREAM_ERROR = "your LibraryGPT subscription has expired" + + +def _get_upstream_link_good_and_syncable(downstream): + return UpstreamLink( + upstream_ref=downstream.upstream, + version_synced=downstream.upstream_version, + version_available=(downstream.upstream_version or 0) + 1, + version_declined=downstream.upstream_version_declined, + error_message=None, + ) + + +def _get_upstream_link_bad(_downstream): + raise BadUpstream(MOCK_UPSTREAM_ERROR) + + +class _DownstreamViewTestMixin: + """ + Shared data and error test cases. + """ + + def setUp(self): + """ + Create a simple course with one unit and two videos, one of which is linked to an "upstream". + """ + super().setUp() + self.course = CourseFactory.create() + chapter = BlockFactory.create(category='chapter', parent=self.course) + sequential = BlockFactory.create(category='sequential', parent=chapter) + unit = BlockFactory.create(category='vertical', parent=sequential) + self.regular_video_key = BlockFactory.create(category='video', parent=unit).usage_key + self.downstream_video_key = BlockFactory.create( + category='video', parent=unit, upstream=MOCK_UPSTREAM_REF, upstream_version=123, + ).usage_key + self.fake_video_key = self.course.id.make_usage_key("video", "NoSuchVideo") + self.superuser = UserFactory(username="superuser", password="password", is_staff=True, is_superuser=True) + self.learner = UserFactory(username="learner", password="password") + + def call_api(self, usage_key_string): + raise NotImplementedError + + def test_404_downstream_not_found(self): + """ + Do we raise 404 if the specified downstream block could not be loaded? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.fake_video_key) + assert response.status_code == 404 + assert "not found" in response.data["developer_message"] + + def test_404_downstream_not_accessible(self): + """ + Do we raise 404 if the user lacks read access on the specified downstream block? + """ + self.client.login(username="learner", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 404 + assert "not found" in response.data["developer_message"] + + +class GetDownstreamViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `GET /api/v2/contentstore/downstreams/...` inspects a downstream's link to an upstream. + """ + def call_api(self, usage_key_string): + return self.client.get(f"/api/contentstore/v2/downstreams/{usage_key_string}") + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_200_good_upstream(self): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 200 + assert response.data['upstream_ref'] == MOCK_UPSTREAM_REF + assert response.data['error_message'] is None + assert response.data['ready_to_sync'] is True + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_bad) + def test_200_bad_upstream(self): + """ + If the upstream link is broken, do we still return 200, but with an error message in body? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 200 + assert response.data['upstream_ref'] == MOCK_UPSTREAM_REF + assert response.data['error_message'] == MOCK_UPSTREAM_ERROR + assert response.data['ready_to_sync'] is False + + def test_200_no_upstream(self): + """ + If the upstream link is missing, do we still return 200, but with an error message in body? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key) + assert response.status_code == 200 + assert response.data['upstream_ref'] is None + assert "is not linked" in response.data['error_message'] + assert response.data['ready_to_sync'] is False + + +class PutDownstreamViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `PUT /api/v2/contentstore/downstreams/...` edits a downstream's link to an upstream. + """ + def call_api(self, usage_key_string, sync: str | None = None): + return self.client.put( + f"/api/contentstore/v2/downstreams/{usage_key_string}", + data={ + "upstream_ref": MOCK_UPSTREAM_REF, + **({"sync": sync} if sync else {}), + }, + content_type="application/json", + ) + + @patch.object(downstreams_views, "fetch_customizable_fields") + @patch.object(downstreams_views, "sync_from_upstream") + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_200_with_sync(self, mock_sync, mock_fetch): + """ + Does the happy path work (with sync=True)? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key, sync='true') + assert response.status_code == 200 + video_after = modulestore().get_item(self.regular_video_key) + assert mock_sync.call_count == 1 + assert mock_fetch.call_count == 0 + assert video_after.upstream == MOCK_UPSTREAM_REF + + @patch.object(downstreams_views, "fetch_customizable_fields") + @patch.object(downstreams_views, "sync_from_upstream") + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_200_no_sync(self, mock_sync, mock_fetch): + """ + Does the happy path work (with sync=False)? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key, sync='false') + assert response.status_code == 200 + video_after = modulestore().get_item(self.regular_video_key) + assert mock_sync.call_count == 0 + assert mock_fetch.call_count == 1 + assert video_after.upstream == MOCK_UPSTREAM_REF + + @patch.object(downstreams_views, "fetch_customizable_fields", side_effect=BadUpstream(MOCK_UPSTREAM_ERROR)) + def test_400(self, sync: str): + """ + Do we raise a 400 if the provided upstream reference is malformed or not accessible? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 400 + assert MOCK_UPSTREAM_ERROR in response.data['developer_message']['upstream_ref'] + video_after = modulestore().get_item(self.regular_video_key) + assert video_after.upstream is None + + +class DeleteDownstreamViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `DELETE /api/v2/contentstore/downstreams/...` severs a downstream's link to an upstream. + """ + def call_api(self, usage_key_string): + return self.client.delete(f"/api/contentstore/v2/downstreams/{usage_key_string}") + + @patch.object(downstreams_views, "sever_upstream_link") + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_204(self, mock_sever): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 204 + assert mock_sever.call_count == 1 + + @patch.object(downstreams_views, "sever_upstream_link") + def test_204_no_upstream(self, mock_sever): + """ + If there's no upsream, do we still happily return 204? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key) + assert response.status_code == 204 + assert mock_sever.call_count == 1 + + +class _DownstreamSyncViewTestMixin(_DownstreamViewTestMixin): + """ + Shared tests between the /api/contentstore/v2/downstreams/.../sync endpoints. + """ + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_bad) + def test_400_bad_upstream(self): + """ + If the upstream link is bad, do we get a 400? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 400 + assert MOCK_UPSTREAM_ERROR in response.data["developer_message"][0] + + def test_400_no_upstream(self): + """ + If the upstream link is missing, do we get a 400? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key) + assert response.status_code == 400 + assert "is not linked" in response.data["developer_message"][0] + + +class PostDownstreamSyncViewTest(_DownstreamSyncViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `POST /api/v2/contentstore/downstreams/.../sync` initiates a sync from the linked upstream. + """ + def call_api(self, usage_key_string): + return self.client.post(f"/api/contentstore/v2/downstreams/{usage_key_string}/sync") + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + @patch.object(downstreams_views, "sync_from_upstream") + def test_200(self, mock_sync_from_upstream): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 200 + assert mock_sync_from_upstream.call_count == 1 + + +class DeleteDownstreamSyncViewtest(_DownstreamSyncViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `DELETE /api/v2/contentstore/downstreams/.../sync` declines a sync from the linked upstream. + """ + def call_api(self, usage_key_string): + return self.client.delete(f"/api/contentstore/v2/downstreams/{usage_key_string}/sync") + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + @patch.object(downstreams_views, "decline_sync") + def test_204(self, mock_decline_sync): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 204 + assert mock_decline_sync.call_count == 1 diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 7979a422a331..5706b44e2cec 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -7,11 +7,13 @@ from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient from openedx_tagging.core.tagging.models import Tag +from organizations.models import Organization from xmodule.modulestore.django import contentstore, modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory, LibraryFactory from cms.djangoapps.contentstore.utils import reverse_usage_url +from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangoapps.content_tagging import api as tagging_api CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/" @@ -391,6 +393,78 @@ def test_paste_with_assets(self): assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged. +class ClipboardPasteFromV2LibraryTestCase(ModuleStoreTestCase): + """ + Test Clipboard Paste functionality with a "new" (as of Sumac) library + """ + + def setUp(self): + """ + Set up a v2 Content Library and a library content block + """ + super().setUp() + self.client = APIClient() + self.client.login(username=self.user.username, password=self.user_password) + self.store = modulestore() + + self.library = library_api.create_library( + library_type=library_api.COMPLEX, + org=Organization.objects.create(name="Test Org", short_name="CL-TEST"), + slug="lib", + title="Library", + ) + + self.lib_block_key = library_api.create_library_block(self.library.key, "problem", "p1").usage_key # v==1 + library_api.set_library_block_olx(self.lib_block_key, """ + + + + + Wrong + Right + + + + """) # v==2 + library_api.publish_changes(self.library.key) + library_api.set_library_block_olx(self.lib_block_key, """ + + + + + Wrong + Right + + + + """) # v==3 + lib_block_meta = library_api.get_library_block(self.lib_block_key) + assert lib_block_meta.published_version_num == 2 + assert lib_block_meta.draft_version_num == 3 + + self.course = CourseFactory.create(display_name='Course') + + def test_paste_from_library_creates_link(self): + """ + When we copy a v2 lib block into a course, the dest block should be linked up to the lib block. + """ + copy_response = self.client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(self.lib_block_key)}, format="json") + assert copy_response.status_code == 200 + + paste_response = self.client.post(XBLOCK_ENDPOINT, { + "parent_locator": str(self.course.usage_key), + "staged_content": "clipboard", + }, format="json") + assert paste_response.status_code == 200 + + new_block_key = UsageKey.from_string(paste_response.json()["locator"]) + new_block = modulestore().get_item(new_block_key) + assert new_block.upstream == str(self.lib_block_key) + assert new_block.upstream_version == 3 + assert new_block.upstream_display_name == "MCQ-draft" + assert new_block.upstream_max_attempts == 5 + + class ClipboardPasteFromV1LibraryTestCase(ModuleStoreTestCase): """ Test Clipboard Paste functionality with legacy (v1) library content diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 79137cfde11f..e4d37f942331 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -539,7 +539,8 @@ def _create_block(request): # Paste from the user's clipboard (content_staging app clipboard, not browser clipboard) into 'usage_key': try: created_xblock, notices = import_staged_content_from_user_clipboard( - parent_key=usage_key, request=request + parent_key=usage_key, + request=request, ) except Exception: # pylint: disable=broad-except log.exception( diff --git a/cms/envs/common.py b/cms/envs/common.py index 63221ee0b0a4..7297adae8354 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -127,6 +127,7 @@ from lms.djangoapps.lms_xblock.mixin import LmsBlockMixin from cms.lib.xblock.authoring_mixin import AuthoringMixin +from cms.lib.xblock.upstream_sync import UpstreamSyncMixin from xmodule.modulestore.edit_info import EditInfoMixin from openedx.core.djangoapps.theming.helpers_dirs import ( get_themes_unchecked, @@ -995,6 +996,7 @@ XModuleMixin, EditInfoMixin, AuthoringMixin, + UpstreamSyncMixin, ) # .. setting_name: XBLOCK_EXTRA_MIXINS diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py new file mode 100644 index 000000000000..5db020393eab --- /dev/null +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -0,0 +1,337 @@ +""" +Test CMS's upstream->downstream syncing system +""" +import ddt + +from organizations.api import ensure_organization +from organizations.models import Organization + +from cms.lib.xblock.upstream_sync import ( + UpstreamLink, + sync_from_upstream, decline_sync, fetch_customizable_fields, sever_upstream_link, + NoUpstream, BadUpstream, BadDownstream, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries import api as libs +from openedx.core.djangoapps.xblock import api as xblock +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory + + +@ddt.ddt +class UpstreamTestCase(ModuleStoreTestCase): + """ + Tests the upstream_sync mixin, data object, and Python APIs. + """ + + def setUp(self): + """ + Create a simple course with one unit, and simple V2 library with two blocks. + """ + super().setUp() + course = CourseFactory.create() + chapter = BlockFactory.create(category='chapter', parent=course) + sequential = BlockFactory.create(category='sequential', parent=chapter) + self.unit = BlockFactory.create(category='vertical', parent=sequential) + + ensure_organization("TestX") + self.library = libs.create_library( + org=Organization.objects.get(short_name="TestX"), + slug="TestLib", + title="Test Upstream Library", + ) + self.upstream_key = libs.create_library_block(self.library.key, "html", "test-upstream").usage_key + libs.create_library_block(self.library.key, "video", "video-upstream") + + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.display_name = "Upstream Title V2" + upstream.data = "Upstream content V2" + upstream.save() + + def test_sync_bad_downstream(self): + """ + Syncing into an unsupported downstream (such as a another Content Library block) raises BadDownstream, but + doesn't affect the block. + """ + downstream_lib_block_key = libs.create_library_block(self.library.key, "html", "bad-downstream").usage_key + downstream_lib_block = xblock.load_block(downstream_lib_block_key, self.user) + downstream_lib_block.display_name = "Another lib block" + downstream_lib_block.data = "another lib block" + downstream_lib_block.upstream = str(self.upstream_key) + downstream_lib_block.save() + + with self.assertRaises(BadDownstream): + sync_from_upstream(downstream_lib_block, self.user) + + assert downstream_lib_block.display_name == "Another lib block" + assert downstream_lib_block.data == "another lib block" + + def test_sync_no_upstream(self): + """ + Trivial case: Syncing a block with no upstream is a no-op + """ + block = BlockFactory.create(category='html', parent=self.unit) + block.display_name = "Block Title" + block.data = "Block content" + + with self.assertRaises(NoUpstream): + sync_from_upstream(block, self.user) + + assert block.display_name == "Block Title" + assert block.data == "Block content" + assert not block.upstream_display_name + + @ddt.data( + ("not-a-key-at-all", ".*is malformed.*"), + ("course-v1:Oops+ItsA+CourseKey", ".*is malformed.*"), + ("block-v1:The+Wrong+KindOfUsageKey+type@html+block@nope", ".*is malformed.*"), + ("lb:TestX:NoSuchLib:html:block-id", ".*not found in the system.*"), + ("lb:TestX:TestLib:video:should-be-html-but-is-a-video", ".*type mismatch.*"), + ("lb:TestX:TestLib:html:no-such-html", ".*not found in the system.*"), + ) + @ddt.unpack + def test_sync_bad_upstream(self, upstream, message_regex): + """ + Syncing with a bad upstream raises BadUpstream, but doesn't affect the block + """ + block = BlockFactory.create(category='html', parent=self.unit, upstream=upstream) + block.display_name = "Block Title" + block.data = "Block content" + + with self.assertRaisesRegex(BadUpstream, message_regex): + sync_from_upstream(block, self.user) + + assert block.display_name == "Block Title" + assert block.data == "Block content" + assert not block.upstream_display_name + + def test_sync_not_accessible(self): + """ + Syncing with an block that exists, but is inaccessible, raises BadUpstream + """ + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + user_who_cannot_read_upstream = UserFactory.create(username="rando", is_staff=False, is_superuser=False) + with self.assertRaisesRegex(BadUpstream, ".*could not be loaded.*") as exc: + sync_from_upstream(downstream, user_who_cannot_read_upstream) + + def test_sync_updates_happy_path(self): + """ + Can we sync updates from a content library block to a linked out-of-date course block? + """ + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + + # Initial sync + sync_from_upstream(downstream, self.user) + assert downstream.upstream_version == 2 # Library blocks start at version 2 (v1 is the empty new block) + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + + # Upstream updates + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.display_name = "Upstream Title V3" + upstream.data = "Upstream content V3" + upstream.save() + + # Follow-up sync. Assert that updates are pulled into downstream. + sync_from_upstream(downstream, self.user) + assert downstream.upstream_version == 3 + assert downstream.upstream_display_name == "Upstream Title V3" + assert downstream.display_name == "Upstream Title V3" + assert downstream.data == "Upstream content V3" + + def test_sync_updates_to_modified_content(self): + """ + If we sync to modified content, will it preserve customizable fields, but overwrite the rest? + """ + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + + # Initial sync + sync_from_upstream(downstream, self.user) + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + + # Upstream updates + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.display_name = "Upstream Title V3" + upstream.data = "Upstream content V3" + upstream.save() + + # Downstream modifications + downstream.display_name = "Downstream Title Override" # "safe" customization + downstream.data = "Downstream content override" # "unsafe" override + downstream.save() + + # Follow-up sync. Assert that updates are pulled into downstream, but customizations are saved. + sync_from_upstream(downstream, self.user) + assert downstream.upstream_display_name == "Upstream Title V3" + assert downstream.display_name == "Downstream Title Override" # "safe" customization survives + assert downstream.data == "Upstream content V3" # "unsafe" override is gone + + # For the Content Libraries Relaunch Beta, we do not yet need to support this edge case. + # See "PRESERVING DOWNSTREAM CUSTOMIZATIONS and RESTORING UPSTREAM DEFAULTS" in cms/lib/xblock/upstream_sync.py. + # + # def test_sync_to_downstream_with_subtle_customization(self): + # """ + # Edge case: If our downstream customizes a field, but then the upstream is changed to match the + # customization do we still remember that the downstream field is customized? That is, + # if the upstream later changes again, do we retain the downstream customization (rather than + # following the upstream update?) + # """ + # # Start with an uncustomized downstream block. + # downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + # sync_from_upstream(downstream, self.user) + # assert downstream.downstream_customized == [] + # assert downstream.display_name == downstream.upstream_display_name == "Upstream Title V2" + # + # # Then, customize our downstream title. + # downstream.display_name = "Title V3" + # downstream.save() + # assert downstream.downstream_customized == ["display_name"] + # + # # Syncing should retain the customization. + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 2 + # assert downstream.upstream_display_name == "Upstream Title V2" + # assert downstream.display_name == "Title V3" + # + # # Whoa, look at that, the upstream has updated itself to the exact same title... + # upstream = xblock.load_block(self.upstream_key, self.user) + # upstream.display_name = "Title V3" + # upstream.save() + # + # # ...which is reflected when we sync. + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 3 + # assert downstream.upstream_display_name == downstream.display_name == "Title V3" + # + # # But! Our downstream knows that its title is still customized. + # assert downstream.downstream_customized == ["display_name"] + # # So, if the upstream title changes again... + # upstream.display_name = "Title V4" + # upstream.save() + # + # # ...then the downstream title should remain put. + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 4 + # assert downstream.upstream_display_name == "Title V4" + # assert downstream.display_name == "Title V3" + # + # # Finally, if we "de-customize" the display_name field, then it should go back to syncing normally. + # downstream.downstream_customized = [] + # upstream.display_name = "Title V5" + # upstream.save() + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 5 + # assert downstream.upstream_display_name == downstream.display_name == "Title V5" + + @ddt.data(None, "Title From Some Other Upstream Version") + def test_fetch_customizable_fields(self, initial_upstream_display_name): + """ + Can we fetch a block's upstream field values without syncing it? + + Test both with and without a pre-"fetched" upstrema values on the downstream. + """ + downstream = BlockFactory.create(category='html', parent=self.unit) + downstream.upstream_display_name = initial_upstream_display_name + downstream.display_name = "Some Title" + downstream.data = "Some content" + + # Note that we're not linked to any upstream. fetch_customizable_fields shouldn't care. + assert not downstream.upstream + assert not downstream.upstream_version + + # fetch! + upstream = xblock.load_block(self.upstream_key, self.user) + fetch_customizable_fields(upstream=upstream, downstream=downstream, user=self.user) + + # Ensure: fetching doesn't affect the upstream link (or lack thereof). + assert not downstream.upstream + assert not downstream.upstream_version + + # Ensure: fetching doesn't affect actual content or settings. + assert downstream.display_name == "Some Title" + assert downstream.data == "Some content" + + # Ensure: fetching DOES set the upstream_* fields. + assert downstream.upstream_display_name == "Upstream Title V2" + + def test_prompt_and_decline_sync(self): + """ + Is the user prompted for sync when it's available? Does declining remove the prompt until a new sync is ready? + """ + # Initial conditions (pre-sync) + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced is None + assert link.version_declined is None + assert link.version_available == 2 # Library block with content starts at version 2 + assert link.ready_to_sync is True + + # Initial sync to V2 + sync_from_upstream(downstream, self.user) + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined is None + assert link.version_available == 2 + assert link.ready_to_sync is False + + # Upstream updated to V3 + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.data = "Upstream content V3" + upstream.save() + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined is None + assert link.version_available == 3 + assert link.ready_to_sync is True + + # Decline to sync to V3 -- ready_to_sync becomes False. + decline_sync(downstream) + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined == 3 + assert link.version_available == 3 + assert link.ready_to_sync is False + + # Upstream updated to V4 -- ready_to_sync becomes True again. + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.data = "Upstream content V4" + upstream.save() + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined == 3 + assert link.version_available == 4 + assert link.ready_to_sync is True + + def test_sever_upstream_link(self): + """ + Does sever_upstream_link correctly disconnect a block from its upstream? + """ + # Start with a course block that is linked+synced to a content library block. + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + sync_from_upstream(downstream, self.user) + + # (sanity checks) + assert downstream.upstream == str(self.upstream_key) + assert downstream.upstream_version == 2 + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + assert downstream.copied_from_block is None + + # Now, disconnect the course block. + sever_upstream_link(downstream) + + # All upstream metadata has been wiped out. + assert downstream.upstream is None + assert downstream.upstream_version is None + assert downstream.upstream_display_name is None + + # BUT, the content which was synced into the upstream remains. + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + + # AND, we have recorded the old upstream as our copied_from_block. + assert downstream.copied_from_block == str(self.upstream_key) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py new file mode 100644 index 000000000000..785cc7dc7e36 --- /dev/null +++ b/cms/lib/xblock/upstream_sync.py @@ -0,0 +1,451 @@ +""" +Synchronize content and settings from upstream blocks to their downstream usages. + +At the time of writing, we assume that for any upstream-downstream linkage: +* The upstream is a Component from a Learning Core-backed Content Library. +* The downstream is a block of matching type in a SplitModuleStore-backed Course. +* They are both on the same Open edX instance. + +HOWEVER, those assumptions may loosen in the future. So, we consider these to be INTERNAL ASSUMPIONS that should not be +exposed through this module's public Python interface. +""" +from __future__ import annotations + +import logging +import typing as t +from dataclasses import dataclass, asdict + +from django.core.exceptions import PermissionDenied +from django.utils.translation import gettext_lazy as _ +from rest_framework.exceptions import NotFound +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from xblock.exceptions import XBlockNotFoundError +from xblock.fields import Scope, String, Integer +from xblock.core import XBlockMixin, XBlock + +if t.TYPE_CHECKING: + from django.contrib.auth.models import User # pylint: disable=imported-auth-user + + +logger = logging.getLogger(__name__) + + +class UpstreamLinkException(Exception): + """ + Raised whenever we try to inspect, sync-from, fetch-from, or delete a block's link to upstream content. + + There are three flavors (defined below): BadDownstream, BadUpstream, NoUpstream. + + Should be constructed with a human-friendly, localized, PII-free message, suitable for API responses and UI display. + For now, at least, the message can assume that upstreams are Content Library blocks and downstreams are Course + blocks, although that may need to change (see module docstring). + """ + + +class BadDownstream(UpstreamLinkException): + """ + Downstream content does not support sync. + """ + + +class BadUpstream(UpstreamLinkException): + """ + Reference to upstream content is malformed, invalid, and/or inaccessible. + """ + + +class NoUpstream(UpstreamLinkException): + """ + The downstream content does not have an upstream link at all (...as is the case for most XBlocks usages). + + (This isn't so much an "error" like the other two-- it's just a case that needs to be handled exceptionally, + usually by logging a message and then doing nothing.) + """ + def __init__(self): + super().__init__(_("Content is not linked to a Content Library.")) + + +@dataclass(frozen=True) +class UpstreamLink: + """ + Metadata about some downstream content's relationship with its linked upstream content. + """ + upstream_ref: str | None # Reference to the upstream content, e.g., a serialized library block usage key. + version_synced: int | None # Version of the upstream to which the downstream was last synced. + version_available: int | None # Latest version of the upstream that's available, or None if it couldn't be loaded. + version_declined: int | None # Latest version which the user has declined to sync with, if any. + error_message: str | None # If link is valid, None. Otherwise, a localized, human-friendly error message. + + @property + def ready_to_sync(self) -> bool: + """ + Should we invite the downstream's authors to sync the latest upstream updates? + """ + return bool( + self.upstream_ref and + self.version_available and + self.version_available > (self.version_synced or 0) and + self.version_available > (self.version_declined or 0) + ) + + def to_json(self) -> dict[str, t.Any]: + """ + Get an JSON-API-friendly representation of this upstream link. + """ + return { + **asdict(self), + "ready_to_sync": self.ready_to_sync, + } + + @classmethod + def try_get_for_block(cls, downstream: XBlock) -> t.Self: + """ + Same as `get_for_block`, but upon failure, sets `.error_message` instead of raising an exception. + """ + try: + return cls.get_for_block(downstream) + except UpstreamLinkException as exc: + logger.exception( + "Tried to inspect an unsupported, broken, or missing downstream->upstream link: '%s'->'%s'", + downstream.usage_key, + downstream.upstream, + ) + return cls( + upstream_ref=downstream.upstream, + version_synced=downstream.upstream_version, + version_available=None, + version_declined=None, + error_message=str(exc), + ) + + @classmethod + def get_for_block(cls, downstream: XBlock) -> t.Self: + """ + Get info on a block's relationship with its linked upstream content (without actually loading the content). + + Currently, the only supported upstreams are LC-backed Library Components. This may change in the future (see + module docstring). + + If link exists, is supported, and is followable, returns UpstreamLink. + Otherwise, raises an UpstreamLinkException. + """ + if not downstream.upstream: + raise NoUpstream() + if not isinstance(downstream.usage_key.context_key, CourseKey): + raise BadDownstream(_("Cannot update content because it does not belong to a course.")) + if downstream.has_children: + raise BadDownstream(_("Updating content with children is not yet supported.")) + try: + upstream_key = LibraryUsageLocatorV2.from_string(downstream.upstream) + except InvalidKeyError as exc: + raise BadUpstream(_("Reference to linked library item is malformed")) from exc + downstream_type = downstream.usage_key.block_type + if upstream_key.block_type != downstream_type: + # Note: Currently, we strictly enforce that the downstream and upstream block_types must exactly match. + # It could be reasonable to relax this requirement in the future if there's product need for it. + # For example, there's no reason that a StaticTabBlock couldn't take updates from an HtmlBlock. + raise BadUpstream( + _("Content type mismatch: {downstream_type} cannot be linked to {upstream_type}.").format( + downstream_type=downstream_type, upstream_type=upstream_key.block_type + ) + ) from TypeError( + f"downstream block '{downstream.usage_key}' is linked to " + f"upstream block of different type '{upstream_key}'" + ) + # We import this here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.content_libraries.api import ( + get_library_block # pylint: disable=wrong-import-order + ) + try: + lib_meta = get_library_block(upstream_key) + except XBlockNotFoundError as exc: + raise BadUpstream(_("Linked library item was not found in the system")) from exc + return cls( + upstream_ref=downstream.upstream, + version_synced=downstream.upstream_version, + version_available=(lib_meta.draft_version_num if lib_meta else None), + # TODO: Previous line is wrong. It should use the published version instead, but the + # LearningCoreXBlockRuntime APIs do not yet support published content yet. + # Will be fixed in a follow-up task: https://github.com/openedx/edx-platform/issues/35582 + # version_available=(lib_meta.published_version_num if lib_meta else None), + version_declined=downstream.upstream_version_declined, + error_message=None, + ) + + +def sync_from_upstream(downstream: XBlock, user: User) -> None: + """ + Update `downstream` with content+settings from the latest available version of its linked upstream content. + + Preserves overrides to customizable fields; overwrites overrides to other fields. + Does not save `downstream` to the store. That is left up to the caller. + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + link, upstream = _load_upstream_link_and_block(downstream, user) + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) + _update_non_customizable_fields(upstream=upstream, downstream=downstream) + downstream.upstream_version = link.version_available + + +def fetch_customizable_fields(*, downstream: XBlock, user: User, upstream: XBlock | None = None) -> None: + """ + Fetch upstream-defined value of customizable fields and save them on the downstream. + + If `upstream` is provided, use that block as the upstream. + Otherwise, load the block specified by `downstream.upstream`, which may raise an UpstreamLinkException. + """ + if not upstream: + _link, upstream = _load_upstream_link_and_block(downstream, user) + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) + + +def _load_upstream_link_and_block(downstream: XBlock, user: User) -> tuple[UpstreamLink, XBlock]: + """ + Load the upstream metadata and content for a downstream block. + + Assumes that the upstream content is an XBlock in an LC-backed content libraries. This assumption may need to be + relaxed in the future (see module docstring). + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException + # We import load_block here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.xblock.api import load_block # pylint: disable=wrong-import-order + try: + lib_block: XBlock = load_block(LibraryUsageLocatorV2.from_string(downstream.upstream), user) + except (NotFound, PermissionDenied) as exc: + raise BadUpstream(_("Linked library item could not be loaded: {}").format(downstream.upstream)) from exc + return link, lib_block + + +def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fetch: bool) -> None: + """ + For each customizable field: + * Save the upstream value to a hidden field on the downstream ("FETCH"). + * If `not only_fetch`, and if the field *isn't* customized on the downstream, then: + * Update it the downstream field's value from the upstream field ("SYNC"). + + Concrete example: Imagine `lib_problem` is our upstream and `course_problem` is our downstream. + + * Say that the customizable fields are [display_name, max_attempts]. + + * Set `course_problem.upstream_display_name = lib_problem.display_name` ("fetch"). + * If `not only_fetch`, and `course_problem.display_name` wasn't customized, then: + * Set `course_problem.display_name = lib_problem.display_name` ("sync"). + + * Set `course_problem.upstream_max_attempts = lib_problem.max_attempts` ("fetch"). + * If `not only_fetch`, and `course_problem.max_attempts` wasn't customized, then: + * Set `course_problem.max_attempts = lib_problem.max_attempts` ("sync"). + """ + syncable_field_names = _get_synchronizable_fields(upstream, downstream) + + for field_name, fetch_field_name in downstream.get_customizable_fields().items(): + + if field_name not in syncable_field_names: + continue + + # FETCH the upstream's value and save it on the downstream (ie, `downstream.upstream_$FIELD`). + old_upstream_value = getattr(downstream, fetch_field_name) + new_upstream_value = getattr(upstream, field_name) + setattr(downstream, fetch_field_name, new_upstream_value) + + if only_fetch: + continue + + # Okay, now for the nuanced part... + # We need to update the downstream field *iff it has not been customized**. + # Determining whether a field has been customized will differ in Beta vs Future release. + # (See "PRESERVING DOWNSTREAM CUSTOMIZATIONS" comment below for details.) + + ## FUTURE BEHAVIOR: field is "customized" iff we have noticed that the user edited it. + # if field_name in downstream.downstream_customized: + # continue + + ## BETA BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. + downstream_value = getattr(downstream, field_name) + if old_upstream_value and downstream_value != old_upstream_value: + continue # Field has been customized. Don't touch it. Move on. + + # Field isn't customized -- SYNC it! + setattr(downstream, field_name, new_upstream_value) + + +def _update_non_customizable_fields(*, upstream: XBlock, downstream: XBlock) -> None: + """ + For each field `downstream.blah` that isn't customizable: set it to `upstream.blah`. + """ + syncable_fields = _get_synchronizable_fields(upstream, downstream) + customizable_fields = set(downstream.get_customizable_fields().keys()) + for field_name in syncable_fields - customizable_fields: + new_upstream_value = getattr(upstream, field_name) + setattr(downstream, field_name, new_upstream_value) + + +def _get_synchronizable_fields(upstream: XBlock, downstream: XBlock) -> set[str]: + """ + The syncable fields are the ones which are content- or settings-scoped AND are defined on both (up,down)stream. + """ + return set.intersection(*[ + set( + field_name + for (field_name, field) in block.__class__.fields.items() + if field.scope in [Scope.settings, Scope.content] + ) + for block in [upstream, downstream] + ]) + + +def decline_sync(downstream: XBlock) -> None: + """ + Given an XBlock that is linked to upstream content, mark the latest available update as 'declined' so that its + authors are not prompted (until another upstream version becomes available). + + Does not save `downstream` to the store. That is left up to the caller. + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + upstream_link = UpstreamLink.get_for_block(downstream) # Can raise UpstreamLinkException + downstream.upstream_version_declined = upstream_link.version_available + + +def sever_upstream_link(downstream: XBlock) -> None: + """ + Given an XBlock that is linked to upstream content, disconnect the link, such that authors are never again prompted + to sync upstream updates. Erase all `.upstream*` fields from the downtream block. + + However, before nulling out the `.upstream` field, we copy its value over to `.copied_from_block`. This makes sense, + because once a downstream block has been de-linked from source (e.g., a Content Library block), it is no different + than if the block had just been copy-pasted in the first place. + + Does not save `downstream` to the store. That is left up to the caller. + + If `downstream` lacks a link, then this raises NoUpstream (though it is reasonable for callers to handle such + exception and ignore it, as the end result is the same: `downstream.upstream is None`). + """ + if not downstream.upstream: + raise NoUpstream() + downstream.copied_from_block = downstream.upstream + downstream.upstream = None + downstream.upstream_version = None + for _, fetched_upstream_field in downstream.get_customizable_fields().items(): + setattr(downstream, fetched_upstream_field, None) # Null out upstream_display_name, et al. + + +class UpstreamSyncMixin(XBlockMixin): + """ + Allows an XBlock in the CMS to be associated & synced with an upstream. + + Mixed into CMS's XBLOCK_MIXINS, but not LMS's. + """ + + # Upstream synchronization metadata fields + upstream = String( + help=( + "The usage key of a block (generally within a content library) which serves as a source of upstream " + "updates for this block, or None if there is no such upstream. Please note: It is valid for this " + "field to hold a usage key for an upstream block that does not exist (or does not *yet* exist) on " + "this instance, particularly if this downstream block was imported from a different instance." + ), + default=None, scope=Scope.settings, hidden=True, enforce_type=True + ) + upstream_version = Integer( + help=( + "Record of the upstream block's version number at the time this block was created from it. If this " + "upstream_version is smaller than the upstream block's latest published version, then the author will be " + "invited to sync updates into this downstream block, presuming that they have not already declined to sync " + "said version." + ), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + upstream_version_declined = Integer( + help=( + "Record of the latest upstream version for which the author declined to sync updates, or None if they have " + "never declined an update." + ), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + + # Store the fetched upstream values for customizable fields. + upstream_display_name = String( + help=("The value of display_name on the linked upstream block."), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + upstream_max_attempts = Integer( + help=("The value of max_attempts on the linked upstream block."), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + + @classmethod + def get_customizable_fields(cls) -> dict[str, str]: + """ + Mapping from each customizable field to the field which can be used to restore its upstream value. + + XBlocks outside of edx-platform can override this in order to set up their own customizable fields. + """ + return { + "display_name": "upstream_display_name", + "max_attempts": "upstream_max_attempts", + } + + # PRESERVING DOWNSTREAM CUSTOMIZATIONS and RESTORING UPSTREAM VALUES + # + # For the full Content Libraries Relaunch, we would like to keep track of which customizable fields the user has + # actually customized. The idea is: once an author has customized a customizable field.... + # + # - future upstream syncs will NOT blow away the customization, + # - but future upstream syncs WILL fetch the upstream values and tuck them away in a hidden field, + # - and the author can can revert back to said fetched upstream value at any point. + # + # Now, whether field is "customized" (and thus "revertible") is dependent on whether they have ever edited it. + # To instrument this, we need to keep track of which customizable fields have been edited using a new XBlock field: + # `downstream_customized` + # + # Implementing `downstream_customized` has proven difficult, because there is no simple way to keep it up-to-date + # with the many different ways XBlock fields can change. The `.save()` and `.editor_saved()` methods are promising, + # but we need to do more due diligence to be sure that they cover all cases, including API edits, import/export, + # copy/paste, etc. We will figure this out in time for the full Content Libraries Relaunch (related ticket: + # https://github.com/openedx/frontend-app-authoring/issues/1317). But, for the Beta realease, we're going to + # implement something simpler: + # + # - We fetch upstream values for customizable fields and tuck them away in a hidden field (same as above). + # - If a customizable field DOES match the fetched upstream value, then future upstream syncs DO update it. + # - If a customizable field does NOT the fetched upstream value, then future upstream syncs DO NOT update it. + # - There is no UI option for explicitly reverting back to the fetched upstream value. + # + # For future reference, here is a partial implementation of what we are thinking for the full Content Libraries + # Relaunch:: + # + # downstream_customized = List( + # help=( + # "Names of the fields which have values set on the upstream block yet have been explicitly " + # "overridden on this downstream block. Unless explicitly cleared by the user, these customizations " + # "will persist even when updates are synced from the upstream." + # ), + # default=[], scope=Scope.settings, hidden=True, enforce_type=True, + # ) + # + # def save(self, *args, **kwargs): + # """ + # Update `downstream_customized` when a customizable field is modified. + # + # NOTE: This does not work, because save() isn't actually called in all the cases that we'd want it to be. + # """ + # super().save(*args, **kwargs) + # customizable_fields = self.get_customizable_fields() + # + # # Loop through all the fields that are potentially cutomizable. + # for field_name, restore_field_name in self.get_customizable_fields(): + # + # # If the field is already marked as customized, then move on so that we don't + # # unneccessarily query the block for its current value. + # if field_name in self.downstream_customized: + # continue + # + # # If this field's value doesn't match the synced upstream value, then mark the field + # # as customized so that we don't clobber it later when syncing. + # # NOTE: Need to consider the performance impact of all these field lookups. + # if getattr(self, field_name) != getattr(self, restore_field_name): + # self.downstream_customized.append(field_name) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 282629456633..0ee1099ca982 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -8,6 +8,7 @@ dump_js_escaped_json, js_escaped_string ) from cms.djangoapps.contentstore.toggles import use_new_text_editor, use_new_problem_editor, use_new_video_editor, use_video_gallery_flow +from cms.lib.xblock.upstream_sync import UpstreamLink from openedx.core.djangoapps.content_tagging.toggles import is_tagging_feature_disabled %> <% @@ -23,6 +24,8 @@ messages = xblock.validate().to_json() has_not_configured_message = messages.get('summary',{}).get('type', None) == 'not-configured' block_is_unit = is_unit(xblock) + +upstream_info = UpstreamLink.try_get_for_block(xblock) %> <%namespace name='static' file='static_content.html'/> @@ -103,7 +106,22 @@ ${label} % else: - ${label} + % if upstream_info.upstream_ref: + % if upstream_info.error_message: + + + ${_("Sourced from a library - but the upstream link is broken/invalid.")} + % else: + + + ${_("Sourced from a library.")} + % endif + % endif + ${label} % endif % if selected_groups_label:

${selected_groups_label}

@@ -114,6 +132,18 @@
    % if not is_root: % if can_edit: + % if upstream_info.ready_to_sync: +
  • + +
    • + % endif % if use_tagging:
  • % endif diff --git a/docs/decisions/0020-upstream-downstream.rst b/docs/decisions/0020-upstream-downstream.rst index 8ceb9e775274..3f6bbacfb839 100644 --- a/docs/decisions/0020-upstream-downstream.rst +++ b/docs/decisions/0020-upstream-downstream.rst @@ -257,6 +257,11 @@ To support the Libraries Relaunch in Sumac: For reference, here are some excerpts of a potential implementation. This may change through development and code review. +(UPDATE: When implementing, we ended up factoring this code differently. +Particularly, we opted to use regular functions rather than add new +XBlock Runtime methods, allowing us to avoid mucking with the complicated +inheritance hierarchy of CachingDescriptorSystem and SplitModuleStoreRuntime.) + .. code-block:: python ########################################################################### diff --git a/mypy.ini b/mypy.ini index 5027c3bb5595..4e69ef616391 100644 --- a/mypy.ini +++ b/mypy.ini @@ -6,7 +6,9 @@ plugins = mypy_django_plugin.main, mypy_drf_plugin.main files = - openedx/core/djangoapps/content/learning_sequences/, + cms/lib/xblock/upstream_sync.py, + cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py, + openedx/core/djangoapps/content/learning_sequences, openedx/core/djangoapps/content_staging, openedx/core/djangoapps/content_libraries, openedx/core/djangoapps/xblock, diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 90b73ef7f474..7d4da2a998c5 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -225,6 +225,8 @@ class LibraryXBlockMetadata: usage_key = attr.ib(type=LibraryUsageLocatorV2) created = attr.ib(type=datetime) modified = attr.ib(type=datetime) + draft_version_num = attr.ib(type=int) + published_version_num = attr.ib(default=None, type=int) display_name = attr.ib("") last_published = attr.ib(default=None, type=datetime) last_draft_created = attr.ib(default=None, type=datetime) @@ -246,6 +248,7 @@ def from_component(cls, library_key, component, associated_collections=None): published_by = last_publish_log.published_by.username draft = component.versioning.draft + published = component.versioning.published last_draft_created = draft.created if draft else None last_draft_created_by = draft.publishable_entity_version.created_by if draft else None @@ -254,9 +257,11 @@ def from_component(cls, library_key, component, associated_collections=None): library_key, component, ), - display_name=component.versioning.draft.title, + display_name=draft.title, created=component.created, - modified=component.versioning.draft.created, + modified=draft.created, + draft_version_num=draft.version_num, + published_version_num=published.version_num if published else None, last_published=None if last_publish_log is None else last_publish_log.published_at, published_by=published_by, last_draft_created=last_draft_created, diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 8632d0cb0740..5efaa28d670c 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -34,7 +34,7 @@ log = logging.getLogger(__name__) -def save_xblock_to_user_clipboard(block: XBlock, user_id: int) -> UserClipboardData: +def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int | None = None) -> UserClipboardData: """ Copy an XBlock's OLX to the user's clipboard. """ @@ -64,6 +64,7 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int) -> UserClipboardD display_name=block_metadata_utils.display_name_with_default(block), suggested_url_name=usage_key.block_id, tags=block_data.tags, + version_num=(version_num or 0), ) (clipboard, _created) = _UserClipboard.objects.update_or_create(user_id=user_id, defaults={ "content": staged_content, @@ -205,6 +206,7 @@ def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardDat block_type=content.block_type, display_name=content.display_name, tags=content.tags, + version_num=content.version_num, ), source_usage_key=clipboard.source_usage_key, source_context_title=clipboard.get_source_context_title(), diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index e952357f4cf9..d077d05a0aa4 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -43,6 +43,7 @@ class StagedContentData: block_type: str = field(validator=validators.instance_of(str)) display_name: str = field(validator=validators.instance_of(str)) tags: dict = field(validator=validators.optional(validators.instance_of(dict))) + version_num: int = field(validator=validators.instance_of(int)) @frozen diff --git a/openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py b/openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py new file mode 100644 index 000000000000..c3438ecb813b --- /dev/null +++ b/openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.16 on 2024-10-09 16:07 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_staging', '0004_stagedcontent_tags'), + ] + + operations = [ + migrations.AddField( + model_name='stagedcontent', + name='version_num', + field=models.PositiveIntegerField(default=0), + ), + ] diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 334fc1c6b5d3..2eab7954e826 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -63,6 +63,8 @@ class Meta: # A _suggested_ URL name to use for this content. Since this suggestion may already be in use, it's fine to generate # a new url_name instead. suggested_url_name = models.CharField(max_length=1024) + # If applicable, an int >=1 indicating the version of copied content. If not applicable, zero (default). + version_num = models.PositiveIntegerField(default=0) # Tags applied to the original source block(s) will be copied to the new block(s) on paste. tags = models.JSONField(null=True, help_text=_("Content tags applied to these blocks")) diff --git a/openedx/core/djangoapps/content_staging/views.py b/openedx/core/djangoapps/content_staging/views.py index 1b9790cfbeee..2a08ccfd3873 100644 --- a/openedx/core/djangoapps/content_staging/views.py +++ b/openedx/core/djangoapps/content_staging/views.py @@ -101,6 +101,7 @@ def post(self, request): "You must be a member of the course team in Studio to export OLX using this API." ) block = modulestore().get_item(usage_key) + version_num = None elif isinstance(course_key, LibraryLocatorV2): lib_api.require_permission_for_library_key( @@ -109,6 +110,7 @@ def post(self, request): lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) block = xblock_api.load_block(usage_key, user=None) + version_num = lib_api.get_library_block(usage_key).draft_version_num else: raise ValidationError("Invalid usage_key for the content.") @@ -116,7 +118,7 @@ def post(self, request): except ItemNotFoundError as exc: raise NotFound("The requested usage key does not exist.") from exc - clipboard = api.save_xblock_to_user_clipboard(block=block, user_id=request.user.id) + clipboard = api.save_xblock_to_user_clipboard(block=block, version_num=version_num, user_id=request.user.id) # Return the current clipboard exactly as if GET was called: serializer = UserClipboardSerializer(clipboard, context={"request": request}) From 451012460de15fef4798c218da04219266a1c183 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 15 Oct 2024 13:01:08 -0400 Subject: [PATCH 008/260] feat: versioned asset support for Learning Core XBlock runtime Add support for displaying static assets in the Learing Core XBlock runtime via "/static/asset-name" style substitutions in the OLX. This is currently used for new Content Library components. Static asset display is version-aware, so viewing older versions of the XBlock content via the embed view will show the appropriate assets for that version. --- .../tests/test_embed_block.py | 53 ++++++- .../xblock/runtime/learning_core_runtime.py | 143 +++++++++++++++++- .../lib/xblock_serializer/block_serializer.py | 39 +++-- 3 files changed, 213 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py index 712117e3d245..a554e6157e8e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py @@ -48,7 +48,7 @@ class LibrariesEmbedViewTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestM """ @XBlock.register_temp_plugin(FieldsTestBlock, FieldsTestBlock.BLOCK_TYPE) - def test_embed_vew_versions(self): + def test_embed_view_versions(self): """ Test that the embed_view renders a block and can render different versions of it. """ @@ -177,8 +177,55 @@ def check_fields(display_name, setting_field, content_field): html = self._embed_block(block_id) check_fields(display_name="DN-01", setting_field="SV-01", content_field="CV-01") - # TODO: test that any static assets referenced in the student_view html are loaded as the correct version, and not - # always loaded as "latest draft". + def test_embed_view_versions_static_assets(self): + """ + Test asset substitution and version-awareness. + """ + # Create a library: + lib = self._create_library( + slug="test-eb-asset-1", title="Asset Test Library", description="", + ) + lib_id = lib["id"] + + # Create an HTMLBlock. This will be the empty version 1: + create_response = self._add_block_to_library(lib_id, "html", "asset_block") + block_id = create_response["id"] + + # Create version 2 of the block by setting its OLX. This has a reference + # to an image, but not the image itself–so it won't get auto-replaced. + olx_response = self._set_library_block_olx(block_id, """ + This is the enemy of our garden:

    +

    + ]]> + """) + assert olx_response["version_num"] == 2 + + # Create version 3 with some bogus file data + self._set_library_block_asset(block_id, "static/deer.jpg", b"This is not a valid JPEG file") + + # Publish the library (making version 3 the published state): + self._commit_library_changes(lib_id) + + # Create version 4 by deleting the asset + self._delete_library_block_asset(block_id, "static/deer.jpg") + + # Grab version 2, which has the asset reference but not the asset. No + # substitution should happen. + html = self._embed_block(block_id, version=2) + assert 'src="/static/deer.jpg"' in html + + # Grab the published version 3. This has the asset, so the link should + # show up. + html = self._embed_block(block_id, version='published') + # This is the pattern we're looking for: + # + assert re.search(r'/library_assets/[0-9a-f-]*/static/deer.jpg', html) + + # Now grab the draft version (4), which is going to once again not have + # the asset (because we deleted it). + html = self._embed_block(block_id, version='draft') + assert 'src="/static/deer.jpg"' in html # TODO: if we are ever able to run these tests in the LMS, test that the LMS only allows accessing the published # version. diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index 8d22626e081d..0942a5a8b3c7 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -9,6 +9,7 @@ from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.transaction import atomic +from django.urls import reverse from openedx_learning.api import authoring as authoring_api @@ -19,7 +20,9 @@ from xblock.fields import Field, Scope, ScopeIds from xblock.field_data import FieldData +from openedx.core.djangoapps.xblock.api import get_xblock_app_config from openedx.core.lib.xblock_serializer.api import serialize_modulestore_block_for_learning_core +from openedx.core.lib.xblock_serializer.data import StaticFile from ..data import AuthoredDataMode, LatestVersion from ..learning_context.manager import get_learning_context_impl from .runtime import XBlockRuntime @@ -230,6 +233,33 @@ def get_block(self, usage_key, for_parent=None, *, version: int | LatestVersion return block + def get_block_assets(self, block): + """ + Return a list of StaticFile entries. + + TODO: When we want to copy a whole Section at a time, doing these + lookups one by one is going to get slow. At some point we're going to + want something to look up a bunch of blocks at once. + """ + component_version = self._get_component_version_from_block(block) + + # cvc = the ComponentVersionContent through-table + cvc_list = ( + component_version + .componentversioncontent_set + .filter(content__has_file=True) + .order_by('key') + ) + + return [ + StaticFile( + name=cvc.key, + url=self._absolute_url_for_asset(component_version, cvc.key), + data=None, + ) + for cvc in cvc_list + ] + def save_block(self, block): """ Save any pending field data values to Learning Core data models. @@ -300,19 +330,120 @@ def _get_component_from_usage_key(self, usage_key): return component - def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: # pylint: disable=unused-argument + def _get_component_version_from_block(self, block): + """ + Given an XBlock instance, return the Learning Core ComponentVersion. + + This relies on our runtime setting the _runtime_requested_version + attribute on blocks that it fetches. + """ + usage_key = block.usage_key + component = self._get_component_from_usage_key(usage_key) + + block_version = block._runtime_requested_version # pylint: disable=protected-access + if block_version == LatestVersion.DRAFT: + component_version = component.versioning.draft + elif block_version == LatestVersion.PUBLISHED: + component_version = component.versioning.published + else: + component_version = component.versioning.version_num(block_version) + + return component_version + + def _absolute_url_for_asset(self, component_version, asset_path): + """ + The full URL for a specific library asset in a ComponentVersion. + + This does not check for whether the path actually exists–it just returns + where it would be if it did exist. + """ + # This function should return absolute URLs, so we need the site root. + site_root_url = get_xblock_app_config().get_site_root_url() + + return site_root_url + reverse( + 'content_libraries:library-assets', + kwargs={ + 'component_version_uuid': component_version.uuid, + 'asset_path': f"static/{asset_path}", + } + ) + + def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: """ Return an absolute URL for the specified static asset file that may belong to this XBlock. - e.g. if the XBlock settings have a field value like "/static/foo.png" - then this method will be called with asset_path="foo.png" and should - return a URL like https://cdn.none/xblock/f843u89789/static/foo.png + e.g. if the XBlock settings have a field value like "/static/test.png" + then this method will be called with asset_path="test.png" and should + return a URL like: + + http://studio.local.openedx.io:8001/library_assets/cd31871e-a342-4c3f-ba2f-a661bf630996/static/test.png If the asset file is not recognized, return None This is called by the XBlockRuntime superclass in the .runtime module. - TODO: Implement as part of larger static asset effort. + Implementation Details + ---------------------- + The standard XBlock "static/{asset_path}" substitution strips out the + leading "static/" part because it assumes that all files will exist in a + shared, flat namespace (i.e. a course's Files and Uploads). + + Learning Core treats assets differently. Each Component has its own, + isolated namespace for asset storage. Furthermore, that namespace + contains content that are not meant to be downloadable, like the + block.xml (the OLX of the Component). There may one day be other files + that are not meant to be externally downloadable there as well, like + Markdown or LaTeX source files or grader code. + + By convention, the static assets that we store in Learning Core and are + meant for download sit inside a static/ directory that is local to each + Component (and actually separate for each Component Version). + + So the transformation looks like this: + + 1. The Learning Core ComponentVersion has an asset stored as + ``static/test.png`` in the database. + 2. The original OLX content we store references ``/static/test.png``, + per OLX convention. Note the leading "/". + 3. The ReplaceURLService XBlock runtime service invokes + ``static_replace`` and strips out ``/static/``. + 4. The method we're in now is invoked with a ``static_path`` of just + ``test.png``, because that's the transformation that works for + ModuleStore-based courses, where everything is stored in the root of + a shared Files and Uploads space. + 5. This method then builds a URL that re-adds the "static/" prefix, and + then points to the ComponentVersion-specific location for that asset. + + Note: This means that the URL for a static asset will change with every + new version of the Component that is created, i.e. with every edit + that's saved–even if the asset itself doesn't change. This was the + tradeoff we made in order to put each version's assets in an isolated + space, and to make sure that we don't break things like relative links. + On the backend, we only store the asset once. + + Performance Note + ---------------- + This can theoretically get very expensive if there are many, many static + asset references in a Component. It's also very cacheable–we could put + it in a RequestCache with a key of (usage_key, version_num), and have + the value be the component_version.uuid and the full list of assets. I'm + not doing this yet in order to keep the code simpler, but I'm leaving + this note here in case someone needs to optimize this later. """ - return None + component_version = self._get_component_version_from_block(block) + + try: + content = ( + component_version + .componentversioncontent_set + .filter(content__has_file=True) + .get(key=f"static/{asset_path}") + ) + except ObjectDoesNotExist: + # This means we see a path that _looks_ like it should be a static + # asset for this Component, but that static asset doesn't really + # exist. + return None + + return self._absolute_url_for_asset(component_version, asset_path) diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index f12bf5336af5..4f94b1acb11b 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -37,19 +37,32 @@ def __init__(self, block): # Search the OLX for references to files stored in the course's # "Files & Uploads" (contentstore): self.olx_str = utils.rewrite_absolute_static_urls(self.olx_str, course_key) - for asset in utils.collect_assets_from_text(self.olx_str, course_key): - path = asset['path'] - if path not in [sf.name for sf in self.static_files]: - self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) - - if block.scope_ids.usage_id.block_type in ['problem', 'vertical']: - py_lib_zip_file = utils.get_python_lib_zip_if_using(self.olx_str, course_key) - if py_lib_zip_file: - self.static_files.append(py_lib_zip_file) - - js_input_files = utils.get_js_input_files_if_using(self.olx_str, course_key) - for js_input_file in js_input_files: - self.static_files.append(js_input_file) + + runtime_supports_explicit_assets = hasattr(block.runtime, 'get_block_assets') + if runtime_supports_explicit_assets: + # If a block supports explicitly tracked assets, things are simple. + # Learning Core backed content supports this, which currently means + # v2 Content Libraries. + self.static_files.extend( + block.runtime.get_block_assets(block) + ) + else: + # Otherwise, we have to scan the content to extract associated asset + # by inference. This is what we have to do for Modulestore-backed + # courses, which store files a course-global "Files and Uploads". + for asset in utils.collect_assets_from_text(self.olx_str, course_key): + path = asset['path'] + if path not in [sf.name for sf in self.static_files]: + self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) + + if block.scope_ids.usage_id.block_type in ['problem', 'vertical']: + py_lib_zip_file = utils.get_python_lib_zip_if_using(self.olx_str, course_key) + if py_lib_zip_file: + self.static_files.append(py_lib_zip_file) + + js_input_files = utils.get_js_input_files_if_using(self.olx_str, course_key) + for js_input_file in js_input_files: + self.static_files.append(js_input_file) def _serialize_block(self, block) -> etree.Element: """ Serialize an XBlock to OLX/XML. """ From 23c4276ec62fab02795245000046cd8be7d3b133 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 17 Oct 2024 12:08:01 -0400 Subject: [PATCH 009/260] test: add data.py to acceptable isolated app imports Per OEP-49, both api.py and data.py are allowed to be imported into other apps: https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0049-django-app-patterns.html#api-py --- setup.cfg | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index f36e0f8b0203..987f4474531f 100644 --- a/setup.cfg +++ b/setup.cfg @@ -183,9 +183,10 @@ isolated_apps = openedx.core.djangoapps.xblock openedx.core.lib.xblock_serializer allowed_modules = - # Only imports from api.py are allowed elsewhere in the code + # Only imports from api.py and data.py are allowed elsewhere in the code # See https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0049-django-app-patterns.html#api-py api + data [importlinter:contract:3] name = Do not import apps from openedx-learning (only import from openedx_learning.api.* and openedx_learning.lib.*). From 2bb4728ab8f42bf9e48b9bc0a4ec200c4120b2e4 Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Fri, 18 Oct 2024 17:01:44 +0500 Subject: [PATCH 010/260] chore: removed temp logs in email digest (#35663) --- openedx/core/djangoapps/notifications/email/utils.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/openedx/core/djangoapps/notifications/email/utils.py b/openedx/core/djangoapps/notifications/email/utils.py index 81a245b2cf12..582e867d629d 100644 --- a/openedx/core/djangoapps/notifications/email/utils.py +++ b/openedx/core/djangoapps/notifications/email/utils.py @@ -3,7 +3,6 @@ """ import datetime import json -import logging from bs4 import BeautifulSoup from django.conf import settings @@ -32,7 +31,6 @@ User = get_user_model() -log = logging.getLogger(__name__) def is_email_notification_flag_enabled(user=None): @@ -413,6 +411,4 @@ def get_updated_preference(pref): if pref_value else EmailCadence.NEVER type_prefs['email_cadence'] = cadence_value preference.save() - if not user.id: - log.info(f" - user.id is null - {encrypted_username} ") notification_preference_unsubscribe_event(user) From 42febb62ce9fae115a07a03676731ac749ad670b Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Fri, 18 Oct 2024 17:02:03 +0500 Subject: [PATCH 011/260] fix: update email_content when grouping notifications (#35656) --- .../notifications/grouping_notifications.py | 1 + .../tests/test_notification_grouping.py | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/openedx/core/djangoapps/notifications/grouping_notifications.py b/openedx/core/djangoapps/notifications/grouping_notifications.py index 9b888ca31c98..b24baeb33136 100644 --- a/openedx/core/djangoapps/notifications/grouping_notifications.py +++ b/openedx/core/djangoapps/notifications/grouping_notifications.py @@ -80,6 +80,7 @@ def group(self, new_notification, old_notification): context['grouped'] = True context['replier_name_list'].append(new_notification.content_context['replier_name']) context['grouped_count'] += 1 + context['email_content'] = new_notification.content_context.get('email_content', '') return context diff --git a/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py b/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py index 283dbb986c12..e46cde7e8378 100644 --- a/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py +++ b/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py @@ -88,6 +88,19 @@ def test_group_appends_to_existing_grouping(self): self.assertEqual(len(updated_context['replier_name_list']), 3) self.assertEqual(updated_context['grouped_count'], 3) + def test_group_email_content(self): + """ + Tests email_content in content_context when grouping notification + """ + self.old_notification.content_context['email_content'] = 'old content' + self.new_notification.content_context = { + 'email_content': 'new content', + 'replier_name': 'user_2', + } + content_context = NewCommentGrouper().group(self.new_notification, self.old_notification) + self.assertIn('email_content', content_context) + self.assertEqual(content_context['email_content'], 'new content') + class TestGroupUserNotifications(unittest.TestCase): """ From e4a1e4136745ce02fc7424a3226f30d4c0965c8d Mon Sep 17 00:00:00 2001 From: Sagirov Evgeniy <34642612+UvgenGen@users.noreply.github.com> Date: Fri, 18 Oct 2024 17:03:07 +0300 Subject: [PATCH 012/260] feat: new Studio view for rendering a Unit in an iframe [FC-0070] The first attempt at creating a new MFE-driven page for Studio Unit rendering involved rendering each XBlock separately in its own iframe. This turned out to be prohibitively slow because of the many redundant assets and JavaScript processing (e.g. MathJax) that happens for each XBlock component. In order to mitigate some of these issues, we decided to try a hybrid approach where we render the entire Unit's worth of XBlocks at once on the server side in a Studio view + template, and then invoke that from frontend-app-authoring as an iframe. The frontend-app-authoring MFE would still be responsible for displaying most of the interactive UI, but the per-component actions like "edit" would be triggered by buttons on the server-rendered Unit display. When one of those buttons is pressed, the server-rendered UI code in the iframe would use postMessage to communicate to the frontend-app-authoring MFE, which would then display the appropriate actions. To make this work, we're making a new view and template that copies a lot of existing code used to display the Unit in pre-MFE Studio, and then modifying that to remove things like the header/footer so that it can be invoked from an iframe. This entire design is a compromise in order to do as much of the UI development in frontend-app-authoring as possible while keeping XBlock rendering performance tolerable. We hope that we can find better solutions for this later. Authored-by: Sagirov Eugeniy --- .../contentstore/views/component.py | 34 +- .../views/tests/test_container_page.py | 58 ++ cms/envs/common.py | 6 + cms/static/images/pencil-icon.svg | 3 + cms/static/js/views/pages/container.js | 168 +++-- .../sass/course-unit-mfe-iframe-bundle.scss | 651 ++++++++++++++++++ .../elements/_course-unit-mfe-iframe.scss | 65 ++ .../partials/cms/theme/_variables-v1.scss | 9 + cms/templates/container_chromeless.html | 278 ++++++++ cms/urls.py | 2 + 10 files changed, 1197 insertions(+), 77 deletions(-) create mode 100644 cms/static/images/pencil-icon.svg create mode 100644 cms/static/sass/course-unit-mfe-iframe-bundle.scss create mode 100644 cms/static/sass/elements/_course-unit-mfe-iframe.scss create mode 100644 cms/templates/container_chromeless.html diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index ba767df78dc9..8fbadad799fc 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -11,6 +11,7 @@ from django.http import Http404, HttpResponseBadRequest from django.shortcuts import redirect from django.utils.translation import gettext as _ +from django.views.decorators.clickjacking import xframe_options_exempt from django.views.decorators.http import require_GET from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey @@ -35,7 +36,8 @@ __all__ = [ 'container_handler', - 'component_handler' + 'component_handler', + 'container_embed_handler', ] log = logging.getLogger(__name__) @@ -141,6 +143,36 @@ def container_handler(request, usage_key_string): # pylint: disable=too-many-st return HttpResponseBadRequest("Only supports HTML requests") +@require_GET +@login_required +@xframe_options_exempt +def container_embed_handler(request, usage_key_string): # pylint: disable=too-many-statements + """ + Returns an HttpResponse with HTML content for the container XBlock. + The returned HTML is a chromeless rendering of the XBlock. + + GET + html: returns the HTML page for editing a container + json: not currently supported + """ + + # Avoiding a circular dependency + from ..utils import get_container_handler_context + + try: + usage_key = UsageKey.from_string(usage_key_string) + except InvalidKeyError: # Raise Http404 on invalid 'usage_key_string' + return HttpResponseBadRequest() + with modulestore().bulk_operations(usage_key.course_key): + try: + course, xblock, lms_link, preview_lms_link = _get_item_in_course(request, usage_key) + except ItemNotFoundError: + raise Http404 # lint-amnesty, pylint: disable=raise-missing-from + + container_handler_context = get_container_handler_context(request, usage_key, course, xblock) + return render_to_response('container_chromeless.html', container_handler_context) + + def get_component_templates(courselike, library=False): # lint-amnesty, pylint: disable=too-many-statements """ Returns the applicable component templates that can be used by the specified course or library. diff --git a/cms/djangoapps/contentstore/views/tests/test_container_page.py b/cms/djangoapps/contentstore/views/tests/test_container_page.py index 1d5b52905357..426477e23408 100644 --- a/cms/djangoapps/contentstore/views/tests/test_container_page.py +++ b/cms/djangoapps/contentstore/views/tests/test_container_page.py @@ -242,3 +242,61 @@ def test_container_page_with_valid_and_invalid_usage_key_string(self): usage_key_string=str(self.vertical.location) ) self.assertEqual(response.status_code, 200) + + +class ContainerEmbedPageTestCase(ContainerPageTestCase): # lint-amnesty, pylint: disable=test-inherits-tests + """ + Unit tests for the container embed page. + """ + + def test_container_html(self): + assets_url = reverse( + 'assets_handler', kwargs={'course_key_string': str(self.child_container.location.course_key)} + ) + self._test_html_content( + self.child_container, + expected_section_tag=( + '