diff --git a/cms/djangoapps/contentstore/config/waffle.py b/cms/djangoapps/contentstore/config/waffle.py index f84290ba83ae..ae0e6ea467b5 100644 --- a/cms/djangoapps/contentstore/config/waffle.py +++ b/cms/djangoapps/contentstore/config/waffle.py @@ -4,7 +4,7 @@ """ -from edx_toggles.toggles import WaffleFlag, WaffleSwitch +from edx_toggles.toggles import WaffleSwitch from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag @@ -26,20 +26,6 @@ f'{WAFFLE_NAMESPACE}.show_review_rules', __name__, LOG_PREFIX ) -# Waffle flag to redirect to the library authoring MFE. -# .. toggle_name: contentstore.library_authoring_mfe -# .. toggle_implementation: WaffleFlag -# .. toggle_default: False -# .. toggle_description: Toggles the new micro-frontend-based implementation of the library authoring experience. -# .. toggle_use_cases: temporary, open_edx -# .. toggle_creation_date: 2020-08-03 -# .. toggle_target_removal_date: 2020-12-31 -# .. toggle_warning: Also set settings.LIBRARY_AUTHORING_MICROFRONTEND_URL and ENABLE_LIBRARY_AUTHORING_MICROFRONTEND. -# .. toggle_tickets: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4106944527/Libraries+Relaunch+Proposal+For+Product+Review -REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND = WaffleFlag( - f'{WAFFLE_NAMESPACE}.library_authoring_mfe', __name__, LOG_PREFIX -) - # .. toggle_name: studio.custom_relative_dates # .. toggle_implementation: CourseWaffleFlag diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py index 0aa06d8b8dcc..4c3e2a4321d3 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py @@ -59,10 +59,8 @@ class CourseHomeSerializer(serializers.Serializer): libraries = LibraryViewSerializer(many=True, required=False, allow_null=True) libraries_enabled = serializers.BooleanField() taxonomies_enabled = serializers.BooleanField() - library_authoring_mfe_url = serializers.CharField() taxonomy_list_mfe_url = serializers.CharField() optimization_enabled = serializers.BooleanField() - redirect_to_library_authoring_mfe = serializers.BooleanField() request_course_creator_url = serializers.CharField() rerun_creator_status = serializers.BooleanField() show_new_library_button = serializers.BooleanField() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/home.py b/cms/djangoapps/contentstore/rest_api/v1/views/home.py index d72042cff611..ff476090ee7a 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/home.py @@ -59,9 +59,7 @@ def get(self, request: Request): "in_process_course_actions": [], "libraries": [], "libraries_enabled": true, - "library_authoring_mfe_url": "//localhost:3001/course/course-v1:edX+P315+2T2023", "optimization_enabled": true, - "redirect_to_library_authoring_mfe": false, "request_course_creator_url": "/request_course_creator", "rerun_creator_status": true, "show_new_library_button": true, diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py index a8b4cf5e3933..c3c9652e5d9e 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py @@ -52,10 +52,8 @@ def test_home_page_courses_response(self): "libraries": [], "libraries_enabled": True, "taxonomies_enabled": True, - "library_authoring_mfe_url": settings.LIBRARY_AUTHORING_MICROFRONTEND_URL, "taxonomy_list_mfe_url": 'http://course-authoring-mfe/taxonomies', "optimization_enabled": False, - "redirect_to_library_authoring_mfe": False, "request_course_creator_url": "/request_course_creator", "rerun_creator_status": True, "show_new_library_button": True, diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 214193918eb4..035e44d5ce31 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -98,7 +98,7 @@ ) from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.djangoapps.models.settings.course_metadata import CourseMetadata -from xmodule.library_tools import LibraryToolsService +from xmodule.library_tools import LegacyLibraryToolsService from xmodule.course_block import DEFAULT_START_DATE # lint-amnesty, pylint: disable=wrong-import-order from xmodule.data import CertificatesDisplayBehaviors from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order @@ -1265,7 +1265,7 @@ def load_services_for_studio(runtime, user): "settings": SettingsService(), "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), - "library_tools": LibraryToolsService(modulestore(), user.id) + "library_tools": LegacyLibraryToolsService(modulestore(), user.id) } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access @@ -1671,9 +1671,7 @@ def get_home_context(request, no_course=False): ENABLE_GLOBAL_STAFF_OPTIMIZATION, ) from cms.djangoapps.contentstore.views.library import ( - LIBRARY_AUTHORING_MICROFRONTEND_URL, LIBRARIES_ENABLED, - should_redirect_to_library_authoring_mfe, user_can_view_create_library_button, ) @@ -1699,12 +1697,9 @@ def get_home_context(request, no_course=False): 'in_process_course_actions': in_process_course_actions, 'libraries_enabled': LIBRARIES_ENABLED, 'taxonomies_enabled': not is_tagging_feature_disabled(), - 'redirect_to_library_authoring_mfe': should_redirect_to_library_authoring_mfe(), - 'library_authoring_mfe_url': LIBRARY_AUTHORING_MICROFRONTEND_URL, 'taxonomy_list_mfe_url': get_taxonomy_list_url(), 'libraries': libraries, - 'show_new_library_button': user_can_view_create_library_button(user) - and not should_redirect_to_library_authoring_mfe(), + 'show_new_library_button': user_can_view_create_library_button(user), 'user': user, 'request_course_creator_url': reverse('request_course_creator'), 'course_creator_status': _get_course_creator_status(user), @@ -2202,7 +2197,7 @@ class StudioPermissionsService: Deprecated. To be replaced by a more general authorization service. - Only used by LibraryContentBlock (and library_tools.py). + Only used by LegacyLibraryContentBlock (and library_tools.py). """ def __init__(self, user): diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 8c314caa6697..340cadb4e244 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -41,7 +41,6 @@ ) from common.djangoapps.util.json_request import JsonResponse, JsonResponseBadRequest, expect_json -from ..config.waffle import REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND from ..utils import add_instructor, reverse_library_url from .component import CONTAINER_TEMPLATES, get_component_templates from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import create_xblock_info @@ -52,21 +51,6 @@ log = logging.getLogger(__name__) LIBRARIES_ENABLED = settings.FEATURES.get('ENABLE_CONTENT_LIBRARIES', False) -ENABLE_LIBRARY_AUTHORING_MICROFRONTEND = settings.FEATURES.get('ENABLE_LIBRARY_AUTHORING_MICROFRONTEND', False) -LIBRARY_AUTHORING_MICROFRONTEND_URL = settings.LIBRARY_AUTHORING_MICROFRONTEND_URL - - -def should_redirect_to_library_authoring_mfe(): - """ - Boolean helper method, returns whether or not to redirect to the Library - Authoring MFE based on settings and flags. - """ - - return ( - ENABLE_LIBRARY_AUTHORING_MICROFRONTEND and - LIBRARY_AUTHORING_MICROFRONTEND_URL and - REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND.is_enabled() - ) def _user_can_create_library_for_org(user, org=None): diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 80a253559887..f3e20b45b2ea 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -982,7 +982,7 @@ def test_shallow_duplicate(self): def test_duplicate_library_content_block(self): # pylint: disable=too-many-statements """ - Test the LibraryContentBlock's special duplication process. + Test the LegacyLibraryContentBlock's special duplication process. """ store = modulestore() diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 76dba57f1d67..7979a422a331 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -7,13 +7,11 @@ 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 +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/" @@ -165,12 +163,12 @@ def _setup_tagged_content(self, course_key) -> dict: publish_item=True, ).location - library = ClipboardLibraryContentPasteTestCase.setup_library() + library = ClipboardPasteFromV1LibraryTestCase.setup_library() with self.store.bulk_operations(course_key): library_content_block_key = BlockFactory.create( parent=self.store.get_item(unit_key), category="library_content", - source_library_id=str(library.key), + source_library_id=str(library.context_key), display_name="LC Block", publish_item=True, ).location @@ -393,27 +391,27 @@ def test_paste_with_assets(self): assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged. -class ClipboardLibraryContentPasteTestCase(ModuleStoreTestCase): +class ClipboardPasteFromV1LibraryTestCase(ModuleStoreTestCase): """ - Test Clipboard Paste functionality with library content + Test Clipboard Paste functionality with legacy (v1) library content """ def setUp(self): """ - Set up a v2 Content Library and a library content block + Set up a v1 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() - library = self.setup_library() + self.library = self.setup_library() # Create a library content block (lc), point it out our library, and sync it. self.course = CourseFactory.create(display_name='Course') self.orig_lc_block = BlockFactory.create( parent=self.course, category="library_content", - source_library_id=str(library.key), + source_library_id=str(self.library.context_key), display_name="LC Block", publish_item=False, ) @@ -426,18 +424,15 @@ def setUp(self): @classmethod def setup_library(cls): """ - Creates and returns a content library. + Creates and returns a legacy content library with 1 problem """ - 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", - ) - # Populate it with a problem: - problem_key = library_api.create_library_block(library.key, "problem", "p1").usage_key - library_api.set_library_block_olx(problem_key, """ - + library = LibraryFactory.create(display_name='Library') + lib_block = BlockFactory.create( + parent_location=library.usage_key, + category="problem", + display_name="MCQ", + max_attempts=1, + data=""" @@ -445,9 +440,9 @@ def setup_library(cls): Right - - """) - library_api.publish_changes(library.key) + """, + publish_item=False, + ) return library def test_paste_library_content_block(self): diff --git a/cms/envs/common.py b/cms/envs/common.py index 7521cc21fa11..63221ee0b0a4 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -434,18 +434,6 @@ # .. toggle_tickets: https://openedx.atlassian.net/browse/DEPR-58 'DEPRECATE_OLD_COURSE_KEYS_IN_STUDIO': True, - # .. toggle_name: FEATURES['ENABLE_LIBRARY_AUTHORING_MICROFRONTEND'] - # .. toggle_implementation: DjangoSetting - # .. toggle_default: False - # .. toggle_description: Set to True to enable the Library Authoring MFE - # .. toggle_use_cases: temporary - # .. toggle_creation_date: 2020-06-20 - # .. toggle_target_removal_date: 2020-12-31 - # .. toggle_tickets: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4106944527/Libraries+Relaunch+Proposal+For+Product+Review - # .. toggle_warning: Also set settings.LIBRARY_AUTHORING_MICROFRONTEND_URL and see - # REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND for rollout. - 'ENABLE_LIBRARY_AUTHORING_MICROFRONTEND': False, - # .. toggle_name: FEATURES['DISABLE_COURSE_CREATION'] # .. toggle_implementation: DjangoSetting # .. toggle_default: False @@ -601,7 +589,6 @@ COURSE_AUTHORING_MICROFRONTEND_URL = None DISCUSSIONS_MICROFRONTEND_URL = None DISCUSSIONS_MFE_FEEDBACK_URL = None -LIBRARY_AUTHORING_MICROFRONTEND_URL = None # .. toggle_name: ENABLE_AUTHN_RESET_PASSWORD_HIBP_POLICY # .. toggle_implementation: DjangoSetting # .. toggle_default: False @@ -2779,6 +2766,7 @@ CUSTOM_PAGES_HELP_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/course_assets/pages.html#adding-custom-pages" COURSE_LIVE_HELP_URL = "https://edx.readthedocs.io/projects/edx-partner-course-staff/en/latest/course_assets/course_live.html" ORA_SETTINGS_HELP_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/course_assets/pages.html#configuring-course-level-open-response-assessment-settings" +# pylint: enable=line-too-long # keys for big blue button live provider COURSE_LIVE_GLOBAL_CREDENTIALS = {} @@ -2810,6 +2798,7 @@ # Learn More link in upgraded discussion notification alert # pylint: disable=line-too-long DISCUSSIONS_INCONTEXT_LEARNMORE_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/manage_discussions/discussions.html" +# pylint: enable=line-too-long #### django-simple-history## # disable indexing on date field its coming django-simple-history. diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 1d3a510cdc4c..1200a61b0617 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -174,9 +174,6 @@ def should_show_debug_toolbar(request): # lint-amnesty, pylint: disable=missing ################### FRONTEND APPLICATION PUBLISHER URL ################### FEATURES['FRONTEND_APP_PUBLISHER_URL'] = 'http://localhost:18400' -################### FRONTEND APPLICATION LIBRARY AUTHORING ################### -LIBRARY_AUTHORING_MICROFRONTEND_URL = 'http://localhost:3001' - ################### FRONTEND APPLICATION COURSE AUTHORING ################### COURSE_AUTHORING_MICROFRONTEND_URL = 'http://localhost:2001' diff --git a/cms/templates/index.html b/cms/templates/index.html index 766d68da780c..e55859730729 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -348,9 +348,6 @@

${course_info['display_name']}

% endif % if libraries_enabled: - % if redirect_to_library_authoring_mfe: -
  • ${_("Libraries")}
  • - %else:
  • % if split_studio_home: ${_("Libraries")} @@ -358,7 +355,6 @@

    ${course_info['display_name']}

    ${_("Libraries")} % endif
  • - % endif % endif % if taxonomies_enabled:
  • ${_("Taxonomies")}
  • diff --git a/docs/docs_settings.py b/docs/docs_settings.py index 5bc9b1594697..f12848876e8a 100644 --- a/docs/docs_settings.py +++ b/docs/docs_settings.py @@ -14,7 +14,6 @@ ADVANCED_PROBLEM_TYPES, COURSE_IMPORT_EXPORT_STORAGE, GIT_EXPORT_DEFAULT_IDENT, - LIBRARY_AUTHORING_MICROFRONTEND_URL, SCRAPE_YOUTUBE_THUMBNAILS_JOB_QUEUE, VIDEO_TRANSCRIPT_MIGRATIONS_JOB_QUEUE, UPDATE_SEARCH_INDEX_JOB_QUEUE, diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 616cf68f4b62..10ef8c2138b6 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -14,7 +14,7 @@ BlockStructureTransformer, FilteringTransformerMixin ) -from xmodule.library_content_block import LibraryContentBlock # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.library_content_block import LegacyLibraryContentBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from ..utils import get_student_module_as_dict @@ -47,7 +47,6 @@ def collect(cls, block_structure): Collects any information that's necessary to execute this transformer's transform method. """ - block_structure.request_xblock_fields('mode') block_structure.request_xblock_fields('max_count') block_structure.request_xblock_fields('category') store = modulestore() @@ -83,7 +82,6 @@ def transform_block_filters(self, usage_info, block_structure): if library_children: all_library_children.update(library_children) selected = [] - mode = block_structure.get_xblock_field(block_key, 'mode') max_count = block_structure.get_xblock_field(block_key, 'max_count') if max_count < 0: max_count = len(library_children) @@ -100,7 +98,7 @@ def transform_block_filters(self, usage_info, block_structure): # Update selected previous_count = len(selected) - block_keys = LibraryContentBlock.make_selection(selected, library_children, max_count, mode) + block_keys = LegacyLibraryContentBlock.make_selection(selected, library_children, max_count) selected = block_keys['selected'] # Save back any changes @@ -176,7 +174,7 @@ def publish_event(event_name, result, **kwargs): with tracker.get_tracker().context(full_event_name, context): tracker.emit(full_event_name, event_data) - LibraryContentBlock.publish_selected_children_events( + LegacyLibraryContentBlock.publish_selected_children_events( block_keys, format_block_keys, publish_event, diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 1bae90322487..de92692ce4fc 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -45,7 +45,7 @@ from openedx.core.lib.xblock_services.call_to_action import CallToActionService from xmodule.contentstore.django import contentstore from xmodule.exceptions import NotFoundError, ProcessingError -from xmodule.library_tools import LibraryToolsService +from xmodule.library_tools import LegacyLibraryToolsService from xmodule.modulestore.django import XBlockI18nService, modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.partitions.partitions_service import PartitionService @@ -626,7 +626,7 @@ def inner_get_block(block: XBlock) -> XBlock | None: ), 'completion': CompletionService(user=user, context_key=course_id) if user and user.is_authenticated else None, 'i18n': XBlockI18nService, - 'library_tools': LibraryToolsService(store, user_id=user.id if user else None), + 'library_tools': LegacyLibraryToolsService(store, user_id=user.id if user else None), 'partitions': PartitionService(course_id=course_id, cache=DEFAULT_REQUEST_CACHE.data), 'settings': SettingsService(), 'user_tags': UserTagsService(user=user, course_id=course_id), diff --git a/lms/djangoapps/grades/rest_api/v1/views.py b/lms/djangoapps/grades/rest_api/v1/views.py index 5f49f2299ae3..b6835fd61ec4 100644 --- a/lms/djangoapps/grades/rest_api/v1/views.py +++ b/lms/djangoapps/grades/rest_api/v1/views.py @@ -378,7 +378,7 @@ class SubmissionHistoryView(GradeViewMixin, PaginatedAPIView): def get(self, request, course_id=None): """ Get submission history details. This submission history is related to only - ProblemBlock and it doesn't support LibraryContentBlock or ContentLibraries + ProblemBlock and it doesn't support LegacyLibraryContentBlock or ContentLibraries as of now. **Usecases**: @@ -463,7 +463,7 @@ def _generate_course_structure(enrollments): @staticmethod def get_problem_blocks(course): """ Get a list of problem xblock for the course. - This doesn't support LibraryContentBlock or ContentLibraries + This doesn't support LegacyLibraryContentBlock or ContentLibraries as of now """ blocks = [] diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 6f45a10daf49..f30d5f047747 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -76,7 +76,6 @@ LibraryLocator as LibraryLocatorV1, LibraryCollectionLocator, ) -from opaque_keys import InvalidKeyError from openedx_events.content_authoring.data import ( ContentLibraryData, LibraryBlockData, @@ -99,10 +98,7 @@ from openedx.core.djangoapps.xblock.api import get_component_from_usage_key, xblock_type_display_name from openedx.core.lib.xblock_serializer.api import serialize_modulestore_block_for_learning_core -from xmodule.library_root_xblock import LibraryRoot as LibraryRootV1 -from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import ItemNotFoundError from . import permissions, tasks from .constants import ALL_RIGHTS_RESERVED, COMPLEX @@ -421,8 +417,8 @@ def get_library(library_key): # updated version of content that a course could pull in. But more recently, # we've decided to do those version references at the level of the # individual blocks being used, since a Learning Core backed library is - # intended to be used for many LibraryContentBlocks and not 1:1 like v1 - # libraries. The top level version stays for now because LibraryContentBlock + # intended to be referenced in multiple course locations and not 1:1 like v1 + # libraries. The top level version stays for now because LegacyLibraryContentBlock # uses it, but that should hopefully change before the Redwood release. version = 0 if last_publish_log is None else last_publish_log.pk published_by = None @@ -1340,77 +1336,6 @@ def get_library_collection_from_usage_key( raise ContentLibraryCollectionNotFound from exc -# V1/V2 Compatibility Helpers -# (Should be removed as part of -# https://github.com/openedx/edx-platform/issues/32457) -# ====================================================== - -def get_v1_or_v2_library( - library_id: str | LibraryLocatorV1 | LibraryLocatorV2, - version: str | int | None, -) -> LibraryRootV1 | ContentLibraryMetadata | None: - """ - Fetch either a V1 or V2 content library from a V1/V2 key (or key string) and version. - - V1 library versions are Mongo ObjectID strings. - V2 library versions can be positive ints, or strings of positive ints. - Passing version=None will return the latest version the library. - - Returns None if not found. - If key is invalid, raises InvalidKeyError. - For V1, if key has a version, it is ignored in favor of `version`. - For V2, if version is provided but it isn't an int or parseable to one, we raise a ValueError. - - Examples: - * get_v1_or_v2_library("library-v1:ProblemX+PR0B", None) -> - * get_v1_or_v2_library("library-v1:ProblemX+PR0B", "65ff...") -> - * get_v1_or_v2_library("lib:RG:rg-1", None) -> - * get_v1_or_v2_library("lib:RG:rg-1", "36") -> - * get_v1_or_v2_library("lib:RG:rg-1", "xyz") -> - * get_v1_or_v2_library("notakey", "xyz") -> - - If you just want to get a V2 library, use `get_library` instead. - """ - library_key: LibraryLocatorV1 | LibraryLocatorV2 - if isinstance(library_id, str): - try: - library_key = LibraryLocatorV1.from_string(library_id) - except InvalidKeyError: - library_key = LibraryLocatorV2.from_string(library_id) - else: - library_key = library_id - if isinstance(library_key, LibraryLocatorV2): - v2_version: int | None - if version: - v2_version = int(version) - else: - v2_version = None - try: - library = get_library(library_key) - if v2_version is not None and library.version != v2_version: - raise NotImplementedError( - f"Tried to load version {v2_version} of learning_core-based library {library_key}. " - f"Currently, only the latest version ({library.version}) may be loaded. " - "This is a known issue. " - "It will be fixed before the production release of learning_core-based (V2) content libraries. " - ) - return library - except ContentLibrary.DoesNotExist: - return None - elif isinstance(library_key, LibraryLocatorV1): - v1_version: str | None - if version: - v1_version = str(version) - else: - v1_version = None - store = modulestore() - library_key = library_key.for_branch(ModuleStoreEnum.BranchName.library).for_version(v1_version) - try: - return store.get_library(library_key, remove_version=False, remove_branch=False, head_validation=False) - except ItemNotFoundError: - return None - - # Import from Courseware # ====================== diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 9f4f7aaaf7dc..f56b4adfe313 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -21,33 +21,20 @@ from celery import shared_task from celery_utils.logged_task import LoggedTask from celery.utils.log import get_task_logger -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user -from django.core.exceptions import PermissionDenied from edx_django_utils.monitoring import set_code_owner_attribute, set_code_owner_attribute_from_module -from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import ( - BlockUsageLocator, - LibraryLocatorV2, - LibraryUsageLocatorV2, - LibraryLocator as LibraryLocatorV1 -) from user_tasks.tasks import UserTask, UserTaskStatus from xblock.fields import Scope -from common.djangoapps.student.auth import has_studio_write_access from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.content_libraries import api as library_api -from openedx.core.djangoapps.xblock.api import load_block +from opaque_keys.edx.locator import BlockUsageLocator from openedx.core.lib import ensure_cms from xmodule.capa_block import ProblemBlock -from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LibraryContentBlock -from xmodule.library_root_xblock import LibraryRoot as LibraryRootV1 +from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LegacyLibraryContentBlock +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.mixed import MixedModuleStore -from xmodule.modulestore.split_mongo import BlockKey -from xmodule.util.keys import derive_key from . import api from .models import ContentLibraryBlockImportTask @@ -84,77 +71,6 @@ def on_progress(block_key, block_num, block_count, exception=None): ) -def _import_block(store, user_id, source_block, dest_parent_key): - """ - Recursively import a learning core block and its children.` - """ - def generate_block_key(source_key, dest_parent_key): - """ - Deterministically generate an ID for the new block and return the key. - Keys are generated such that they appear identical to a v1 library with - the same input block_id, library name, library organization, and parent block using derive_key - """ - if not isinstance(source_key.lib_key, LibraryLocatorV2): - raise TypeError(f"Expected source library key of type LibraryLocatorV2, got {source_key.lib_key} instead.") - source_key_as_v1_course_key = LibraryLocatorV1( - org=source_key.lib_key.org, - library=source_key.lib_key.slug, - branch='library' - ) - derived_block_key = derive_key( - source=source_key_as_v1_course_key.make_usage_key(source_key.block_type, source_key.block_id), - dest_parent=BlockKey(dest_parent_key.block_type, dest_parent_key.block_id), - ) - return dest_parent_key.context_key.make_usage_key(*derived_block_key) - - source_key = source_block.scope_ids.usage_id - new_block_key = generate_block_key(source_key, dest_parent_key) - try: - new_block = store.get_item(new_block_key) - if new_block.parent.block_id != dest_parent_key.block_id: - raise ValueError( - "Expected existing block {} to be a child of {} but instead it's a child of {}".format( - new_block_key, dest_parent_key, new_block.parent, - ) - ) - except ItemNotFoundError: - new_block = store.create_child( - user_id, - dest_parent_key, - source_key.block_type, - block_id=new_block_key.block_id, - ) - - # Prepare a list of this block's static assets; any assets that are referenced as /static/{path} (the - # recommended way for referencing them) will stop working, and so we rewrite the url when importing. - # Copying assets not advised because modulestore doesn't namespace assets to each block like learning core, which - # might cause conflicts when the same filename is used across imported blocks. - if isinstance(source_key, LibraryUsageLocatorV2): - all_assets = library_api.get_library_block_static_asset_files(source_key) - else: - all_assets = [] - - for field_name, field in source_block.fields.items(): - if field.scope not in (Scope.settings, Scope.content): - continue # Only copy authored field data - if field.is_set_on(source_block) or field.is_set_on(new_block): - field_value = getattr(source_block, field_name) - setattr(new_block, field_name, field_value) - new_block.save() - store.update_item(new_block, user_id) - - if new_block.has_children: - # Delete existing children in the new block, which can be reimported again if they still exist in the - # source library - for existing_child_key in new_block.children: - store.delete_item(existing_child_key, user_id) - # Now import the children - for child in source_block.get_children(): - _import_block(store, user_id, child, new_block_key) - - return new_block_key - - def _filter_child(store, usage_key, capa_type): """ Return whether this block is both a problem and has a `capa_type` which is included in the filter. @@ -172,49 +88,6 @@ def _problem_type_filter(store, library, capa_type): return [key for key in library.children if _filter_child(store, key, capa_type)] -def _import_from_learning_core(user_id, store, dest_block, source_block_ids): - """ - Imports a block from a learning-core-based learning context (usually a - content library) into modulestore, as a new child of dest_block. - Any existing children of dest_block are replaced. - """ - dest_key = dest_block.scope_ids.usage_id - if not isinstance(dest_key, BlockUsageLocator): - raise TypeError(f"Destination {dest_key} should be a modulestore course.") - if user_id is None: - raise ValueError("Cannot check user permissions - LibraryTools user_id is None") - - if len(set(source_block_ids)) != len(source_block_ids): - # We don't support importing the exact same block twice because it would break the way we generate new IDs - # for each block and then overwrite existing copies of blocks when re-importing the same blocks. - raise ValueError("One or more library component IDs is a duplicate.") - - dest_course_key = dest_key.context_key - user = User.objects.get(id=user_id) - if not has_studio_write_access(user, dest_course_key): - raise PermissionDenied() - - # Read the source block; this will also confirm that user has permission to read it. - # (This could be slow and use lots of memory, except for the fact that LibraryContentBlock which calls this - # should be limiting the number of blocks to a reasonable limit. We load them all now instead of one at a - # time in order to raise any errors before we start actually copying blocks over.) - orig_blocks = [load_block(UsageKey.from_string(key), user) for key in source_block_ids] - - with store.bulk_operations(dest_course_key): - child_ids_updated = set() - - for block in orig_blocks: - new_block_id = _import_block(store, user_id, block, dest_key) - child_ids_updated.add(new_block_id) - - # Remove any existing children that are no longer used - for old_child_id in set(dest_block.children) - child_ids_updated: - store.delete_item(old_child_id, user_id) - # If this was called from a handler, it will save dest_block at the end, so we must update - # dest_block.children to avoid it saving the old value of children and deleting the new ones. - dest_block.children = store.get_item(dest_key).children - - class LibrarySyncChildrenTask(UserTask): # pylint: disable=abstract-method """ Base class for tasks which operate upon library_content children. @@ -244,7 +117,7 @@ def sync_from_library( self: LibrarySyncChildrenTask, user_id: int, dest_block_id: str, - library_version: str | int | None, + library_version: str | None, ) -> None: """ Celery task to update the children of the library_content block at `dest_block_id`. @@ -300,8 +173,8 @@ def _sync_children( task: LibrarySyncChildrenTask, store: MixedModuleStore, user_id: int, - dest_block: LibraryContentBlock, - library_version: int | str | None, + dest_block: LegacyLibraryContentBlock, + library_version: str | None, ) -> None: """ Implementation helper for `sync_from_library` and `duplicate_children` Celery tasks. @@ -309,41 +182,29 @@ def _sync_children( Can update children with a specific library `library_version`, or latest (`library_version=None`). """ source_blocks = [] - library_key = dest_block.source_library_key - filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE) - library = library_api.get_v1_or_v2_library(library_key, version=library_version) - if not library: + library_key = dest_block.source_library_key.for_branch( + ModuleStoreEnum.BranchName.library + ).for_version(library_version) + try: + library = store.get_library(library_key, remove_version=False, remove_branch=False, head_validation=False) + except ItemNotFoundError: task.status.fail(f"Requested library {library_key} not found.") - elif isinstance(library, LibraryRootV1): - if filter_children: - # Apply simple filtering based on CAPA problem types: - source_blocks.extend(_problem_type_filter(store, library, dest_block.capa_type)) - else: - source_blocks.extend(library.children) - with store.bulk_operations(dest_block.scope_ids.usage_id.context_key): - try: - dest_block.source_library_version = str(library.location.library_key.version_guid) - store.update_item(dest_block, user_id) - dest_block.children = store.copy_from_template( - source_blocks, dest_block.location, user_id, head_validation=True - ) - # ^-- copy_from_template updates the children in the DB - # but we must also set .children here to avoid overwriting the DB again - except Exception as exception: # pylint: disable=broad-except - TASK_LOGGER.exception('Error importing children for %s', dest_block.scope_ids.usage_id, exc_info=True) - if task.status.state != UserTaskStatus.FAILED: - task.status.fail({'raw_error_msg': str(exception)}) - raise - elif isinstance(library, library_api.ContentLibraryMetadata): - # TODO: add filtering by capa_type when V2 library will support different problem types + return + filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE) + if filter_children: + # Apply simple filtering based on CAPA problem types: + source_blocks.extend(_problem_type_filter(store, library, dest_block.capa_type)) + else: + source_blocks.extend(library.children) + with store.bulk_operations(dest_block.scope_ids.usage_id.context_key): try: - source_block_ids = [ - str(library_api.LibraryXBlockMetadata.from_component(library_key, component).usage_key) - for component in library_api.get_library_components(library_key) - ] - _import_from_learning_core(user_id, store, dest_block, source_block_ids) - dest_block.source_library_version = str(library.version) + dest_block.source_library_version = str(library.location.library_key.version_guid) store.update_item(dest_block, user_id) + dest_block.children = store.copy_from_template( + source_blocks, dest_block.location, user_id, head_validation=True + ) + # ^-- copy_from_template updates the children in the DB + # but we must also set .children here to avoid overwriting the DB again except Exception as exception: # pylint: disable=broad-except TASK_LOGGER.exception('Error importing children for %s', dest_block.scope_ids.usage_id, exc_info=True) if task.status.state != UserTaskStatus.FAILED: @@ -354,8 +215,8 @@ def _sync_children( def _copy_overrides( store: MixedModuleStore, user_id: int, - source_block: LibraryContentBlock, - dest_block: LibraryContentBlock + source_block: LegacyLibraryContentBlock, + dest_block: LegacyLibraryContentBlock ) -> None: """ Copy any overrides the user has made on children of `source` over to the children of `dest_block`, recursively. diff --git a/openedx/core/lib/xblock_utils/__init__.py b/openedx/core/lib/xblock_utils/__init__.py index 26127dbfb3b8..a8b76541b6e5 100644 --- a/openedx/core/lib/xblock_utils/__init__.py +++ b/openedx/core/lib/xblock_utils/__init__.py @@ -452,7 +452,7 @@ def xblock_resource_pkg(block): ProblemBlock, and most other built-in blocks currently. Handling for these assets does not interact with this function. 2. The (preferred) standard XBlock runtime resource loading system, used by - LibraryContentBlock. Handling for these assets *does* interact with this + LegacyLibraryContentBlock. Handling for these assets *does* interact with this function. We hope to migrate to (2) eventually, tracked by: diff --git a/openedx/tests/completion_integration/test_services.py b/openedx/tests/completion_integration/test_services.py index f4088678d99b..7a6fad8ece12 100644 --- a/openedx/tests/completion_integration/test_services.py +++ b/openedx/tests/completion_integration/test_services.py @@ -10,7 +10,7 @@ from django.conf import settings from django.test import override_settings from opaque_keys.edx.keys import CourseKey -from xmodule.library_tools import LibraryToolsService +from xmodule.library_tools import LegacyLibraryToolsService from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory, LibraryFactory from xmodule.tests import prepare_block_runtime @@ -122,7 +122,7 @@ def _bind_course_block(self, block): Bind a block (part of self.course) so we can access student-specific data. """ prepare_block_runtime(block.runtime, course_id=block.location.course_key) - block.runtime._services.update({'library_tools': LibraryToolsService(self.store, self.user.id)}) # lint-amnesty, pylint: disable=protected-access + block.runtime._services.update({'library_tools': LegacyLibraryToolsService(self.store, self.user.id)}) # lint-amnesty, pylint: disable=protected-access def get_block(descriptor): """Mocks module_system get_block_for_descriptor function""" diff --git a/setup.py b/setup.py index 28a25cc91476..21c8e537c97e 100644 --- a/setup.py +++ b/setup.py @@ -21,7 +21,7 @@ "html = xmodule.html_block:HtmlBlock", "image = xmodule.template_block:TranslateCustomTagBlock", "library = xmodule.library_root_xblock:LibraryRoot", - "library_content = xmodule.library_content_block:LibraryContentBlock", + "library_content = xmodule.library_content_block:LegacyLibraryContentBlock", "lti = xmodule.lti_block:LTIBlock", "poll_question = xmodule.poll_block:PollBlock", "problem = xmodule.capa_block:ProblemBlock", diff --git a/webpack.builtinblocks.config.js b/webpack.builtinblocks.config.js index d86f891dc6ce..1c5a9b1e0e9d 100644 --- a/webpack.builtinblocks.config.js +++ b/webpack.builtinblocks.config.js @@ -38,6 +38,10 @@ module.exports = { './xmodule/js/src/xmodule.js', './xmodule/js/src/html/edit.js' ], + LibraryContentBlockEditor: [ + './xmodule/js/src/xmodule.js', + './xmodule/js/src/vertical/edit.js' + ], LTIBlockDisplay: [ './xmodule/js/src/xmodule.js', './xmodule/js/src/lti/lti.js' @@ -46,11 +50,6 @@ module.exports = { './xmodule/js/src/xmodule.js', './xmodule/js/src/raw/edit/metadata-only.js' ], - LibraryContentBlockDisplay: './xmodule/js/src/xmodule.js', - LibraryContentBlockEditor: [ - './xmodule/js/src/xmodule.js', - './xmodule/js/src/vertical/edit.js' - ], PollBlockDisplay: [ './xmodule/js/src/xmodule.js', './xmodule/js/src/javascript_loader.js', diff --git a/xmodule/assets/library_content/public/js/library_content_edit_helpers.js b/xmodule/assets/library_content/public/js/library_content_edit_helpers.js deleted file mode 100644 index 564cc5fb0fbe..000000000000 --- a/xmodule/assets/library_content/public/js/library_content_edit_helpers.js +++ /dev/null @@ -1,54 +0,0 @@ -/* JavaScript for special editing operations that can be done on LibraryContentXBlock */ -// This is a temporary UI improvements that will be removed when V2 content libraries became -// fully functional - -/** - * Toggle the "Problem Type" settings section depending on selected library type. - * As for now, the V2 libraries don't support different problem types, so they can't be - * filtered by it. We're hiding the Problem Type field for them. - */ -function checkProblemTypeShouldBeVisible(editor) { - var libraries = editor.find('.wrapper-comp-settings.metadata_edit.is-active') - .data().metadata.source_library_id.options; - var selectedIndex = $("select[name='Library']", editor)[0].selectedIndex; - var libraryKey = libraries[selectedIndex].value; - var url = URI('/xblock') - .segment(editor.find('.xblock.xblock-studio_view.xblock-studio_view-library_content.xblock-initialized') - .data('usage-id')) - .segment('handler') - .segment('is_v2_library'); - - $.ajax({ - type: 'POST', - url: url, - data: JSON.stringify({'library_key': libraryKey}), - success: function(data) { - var problemTypeSelect = editor.find("select[name='Problem Type']") - .parents("li.field.comp-setting-entry.metadata_entry"); - data.is_v2 ? problemTypeSelect.hide() : problemTypeSelect.show(); - } - }); -} - -/** - * Waits untill editor html loaded, than calls checks for Program Type field toggling. - */ -function waitForEditorLoading() { - var checkContent = setInterval(function() { - var $modal = $('.xblock-editor'); - var content = $modal.html(); - if (content) { - clearInterval(checkContent); - checkProblemTypeShouldBeVisible($modal); - } - }, 10); -} -// Initial call -waitForEditorLoading(); - -var $librarySelect = $("select[name='Library']"); -$(document).on('change', $librarySelect, waitForEditorLoading) - -var $libraryContentEditors = $('.xblock-header.xblock-header-library_content'); -var $editBtns = $libraryContentEditors.find('.action-item.action-edit'); -$(document).on('click', $editBtns, waitForEditorLoading) diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index 09a5d1dee13e..adb07101d04c 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -1,5 +1,12 @@ """ -LibraryContent: The XBlock used to include blocks from a library in a course. +LegacyLibraryContent: The XBlock used to randomly select a subset of blocks from a "v1" (modulestore-backed) library. + +In Studio, it's called the "Randomized Content Module". + +In the long-term, this block is deprecated in favor of "v2" (learning core-backed) library references: +https://github.com/openedx/edx-platform/issues/32457 + +We need to retain backwards-compatibility, but please do not build any new features into this. """ from __future__ import annotations @@ -15,8 +22,7 @@ from django.utils.functional import classproperty from lxml import etree from lxml.etree import XMLSyntaxError -from opaque_keys import InvalidKeyError -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryLocator from rest_framework import status from web_fragments.fragment import Fragment from webob import Response @@ -78,7 +84,7 @@ def __init__(self): @XBlock.wants('studio_user_permissions') # Only available in CMS. @XBlock.wants('user') @XBlock.needs('mako') -class LibraryContentBlock( +class LegacyLibraryContentBlock( MakoTemplateBlockBase, XmlMixin, XModuleToXBlockMixin, @@ -87,7 +93,7 @@ class LibraryContentBlock( StudioEditableBlock, ): """ - An XBlock whose children are chosen dynamically from a content library. + An XBlock whose children are chosen dynamically from a legacy (v1) content library. Can be used to create randomized assessments among other things. Note: technically, all matching blocks from the content library are added @@ -135,17 +141,6 @@ def completion_mode(cls): # pylint: disable=no-self-argument display_name=_("Library Version"), scope=Scope.settings, ) - mode = String( - display_name=_("Mode"), - help=_("Determines how content is drawn from the library"), - default="random", - values=[ - {"display_name": _("Choose n at random"), "value": "random"} - # Future addition: Choose a new random set of n every time the student refreshes the block, for self tests - # Future addition: manually selected blocks - ], - scope=Scope.settings, - ) max_count = Integer( display_name=_("Count"), help=_("Enter the number of components to display to each student. Set it to -1 to display all components."), @@ -179,15 +174,12 @@ def source_library_key(self): """ Convenience method to get the library ID as a LibraryLocator and not just a string. - Supports either library v1 or library v2 locators. + Supports only v1 libraries. """ - try: - return LibraryLocator.from_string(self.source_library_id) - except InvalidKeyError: - return LibraryLocatorV2.from_string(self.source_library_id) + return LibraryLocator.from_string(self.source_library_id) @classmethod - def make_selection(cls, selected, children, max_count, mode): + def make_selection(cls, selected, children, max_count): """ Dynamically selects block_ids indicating which of the possible children are displayed to the current user. @@ -195,7 +187,6 @@ def make_selection(cls, selected, children, max_count, mode): selected - list of (block_type, block_id) tuples assigned to this student children - children of this block max_count - number of components to display to each student - mode - how content is drawn from the library Returns: A dict containing the following keys: @@ -231,12 +222,9 @@ def make_selection(cls, selected, children, max_count, mode): if num_to_add > 0: # We need to select [more] blocks to display to this user: pool = valid_block_keys - selected_keys - if mode == "random": - num_to_add = min(len(pool), num_to_add) - added_block_keys = set(rand.sample(list(pool), num_to_add)) - # We now have the correct n random children to show for this user. - else: - raise NotImplementedError("Unsupported mode.") + num_to_add = min(len(pool), num_to_add) + added_block_keys = set(rand.sample(list(pool), num_to_add)) + # We now have the correct n random children to show for this user. selected_keys |= added_block_keys if any((invalid_block_keys, overlimit_block_keys, added_block_keys)): @@ -334,7 +322,7 @@ def selected_children(self): if max_count < 0: max_count = len(self.children) - block_keys = self.make_selection(self.selected, self.children, max_count, "random") # pylint: disable=no-member + block_keys = self.make_selection(self.selected, self.children, max_count) # pylint: disable=no-member # Publish events for analytics purposes: lib_tools = self.get_tools() @@ -467,7 +455,6 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_cms_template(self.mako_template, self.get_context()) ) - fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/library_content_edit_helpers.js')) add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment @@ -481,16 +468,12 @@ def get_child_blocks(self): @property def non_editable_metadata_fields(self): non_editable_fields = super().non_editable_metadata_fields - # The only supported mode is currently 'random'. - # Add the mode field to non_editable_metadata_fields so that it doesn't - # render in the edit form. non_editable_fields.extend([ - LibraryContentBlock.mode, - LibraryContentBlock.source_library_version, + LegacyLibraryContentBlock.source_library_version, ]) return non_editable_fields - def get_tools(self, to_read_library_content: bool = False) -> 'LibraryToolsService': + def get_tools(self, to_read_library_content: bool = False) -> 'LegacyLibraryToolsService': """ Grab the library tools service and confirm that it'll work for us. Else, raise LibraryToolsUnavailable. """ @@ -564,22 +547,6 @@ def sync_from_library(self, upgrade_to_latest: bool = False) -> None: library_version=(None if upgrade_to_latest else self.source_library_version), ) - @XBlock.json_handler - def is_v2_library(self, data, suffix=''): # pylint: disable=unused-argument - """ - Check the library version by library_id. - - This is a temporary handler needed for hiding the Problem Type xblock editor field for V2 libraries. - """ - lib_key = data.get('library_key') - try: - LibraryLocatorV2.from_string(lib_key) - except InvalidKeyError: - is_v2 = False - else: - is_v2 = True - return {'is_v2': is_v2} - @XBlock.handler def children_are_syncing(self, request, suffix=''): # pylint: disable=unused-argument """ @@ -809,14 +776,14 @@ def definition_to_xml(self, resource_fs): return xml_object -class LibrarySummary: +class LegacyLibrarySummary: """ A library summary object which contains the fields required for library listing on studio. """ def __init__(self, library_locator, display_name): """ - Initialize LibrarySummary + Initialize LegacyLibrarySummary Arguments: library_locator (LibraryLocator): LibraryLocator object of the library. diff --git a/xmodule/library_tools.py b/xmodule/library_tools.py index 2c077a888482..7f9e83a9373d 100644 --- a/xmodule/library_tools.py +++ b/xmodule/library_tools.py @@ -1,18 +1,17 @@ """ -XBlock runtime services for LibraryContentBlock +XBlock runtime services for LegacyLibraryContentBlock """ from __future__ import annotations from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user -from django.conf import settings from django.core.exceptions import ObjectDoesNotExist +from opaque_keys.edx.locator import LibraryLocator from user_tasks.models import UserTaskStatus from openedx.core.lib import ensure_cms -from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangoapps.content_libraries import tasks as library_tasks -from xmodule.library_content_block import LibraryContentBlock -from xmodule.library_root_xblock import LibraryRoot as LibraryRootV1 +from xmodule.library_content_block import LegacyLibraryContentBlock +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError @@ -21,9 +20,9 @@ def normalize_key_for_search(library_key): return library_key.replace(version_guid=None, branch=None) -class LibraryToolsService: +class LegacyLibraryToolsService: """ - Service for LibraryContentBlock. + Service for LegacyLibraryContentBlock. Allows to interact with libraries in the modulestore and learning core. @@ -33,24 +32,31 @@ def __init__(self, modulestore, user_id): self.store = modulestore self.user_id = user_id - def get_latest_library_version(self, lib_key) -> str | None: + def get_latest_library_version(self, library_id: str | LibraryLocator) -> str | None: """ Get the version of the given library as string. The return value (library version) could be: str() - for V1 library; - str() - for V2 library. None - if the library does not exist. """ - library = library_api.get_v1_or_v2_library(lib_key, version=None) + library_key: LibraryLocator + if isinstance(library_id, str): + library_key = LibraryLocator.from_string(library_id) + else: + library_key = library_id + library_key = library_key.for_branch(ModuleStoreEnum.BranchName.library).for_version(None) + try: + library = self.store.get_library( + library_key, remove_version=False, remove_branch=False, head_validation=False + ) + except ItemNotFoundError: + return None if not library: return None - elif isinstance(library, LibraryRootV1): - # We need to know the library's version so ensure it's set in library.location.library_key.version_guid - assert library.location.library_key.version_guid is not None - return str(library.location.library_key.version_guid) - elif isinstance(library, library_api.ContentLibraryMetadata): - return str(library.version) + # We need to know the library's version so ensure it's set in library.location.library_key.version_guid + assert library.location.library_key.version_guid is not None + return str(library.location.library_key.version_guid) def create_block_analytics_summary(self, course_key, block_keys): """ @@ -96,7 +102,7 @@ def can_use_library_content(self, block): """ return self.store.check_supports(block.location.course_key, 'copy_from_template') - def trigger_library_sync(self, dest_block: LibraryContentBlock, library_version: str | int | None) -> None: + def trigger_library_sync(self, dest_block: LegacyLibraryContentBlock, library_version: str | None) -> None: """ Queue task to synchronize the children of `dest_block` with it source library (at `library_version` or latest). @@ -118,16 +124,20 @@ def trigger_library_sync(self, dest_block: LibraryContentBlock, library_version: `dest_block.children`. """ ensure_cms("library_content block children may only be synced in a CMS context") - if not isinstance(dest_block, LibraryContentBlock): + if not isinstance(dest_block, LegacyLibraryContentBlock): raise ValueError(f"Can only sync children for library_content blocks, not {dest_block.tag} blocks.") if not dest_block.source_library_id: dest_block.source_library_version = "" return - library_key = dest_block.source_library_key - if not library_api.get_v1_or_v2_library(library_key, version=library_version): + library_key = dest_block.source_library_key.for_branch( + ModuleStoreEnum.BranchName.library + ).for_version(library_version) + try: + self.store.get_library(library_key, remove_version=False, remove_branch=False, head_validation=False) + except ItemNotFoundError as exc: if library_version: - raise ObjectDoesNotExist(f"Version {library_version} of library {library_key} not found.") - raise ObjectDoesNotExist(f"Library {library_key} not found.") + raise ObjectDoesNotExist(f"Version {library_version} of library {library_key} not found.") from exc + raise ObjectDoesNotExist(f"Library {library_key} not found.") from exc # TODO: This task is synchronous until we can figure out race conditions with import. # These race conditions lead to failed imports of library content from course import. @@ -140,12 +150,14 @@ def trigger_library_sync(self, dest_block: LibraryContentBlock, library_version: ), ) - def trigger_duplication(self, source_block: LibraryContentBlock, dest_block: LibraryContentBlock) -> None: + def trigger_duplication( + self, source_block: LegacyLibraryContentBlock, dest_block: LegacyLibraryContentBlock + ) -> None: """ Queue a task to duplicate the children of `source_block` to `dest_block`. """ ensure_cms("library_content block children may only be duplicated in a CMS context") - if not isinstance(dest_block, LibraryContentBlock): + if not isinstance(dest_block, LegacyLibraryContentBlock): raise ValueError(f"Can only duplicate children for library_content blocks, not {dest_block.tag} blocks.") if source_block.scope_ids.usage_id.context_key != source_block.scope_ids.usage_id.context_key: raise ValueError( @@ -163,7 +175,7 @@ def trigger_duplication(self, source_block: LibraryContentBlock, dest_block: Lib dest_block_id=str(dest_block.scope_ids.usage_id), ) - def are_children_syncing(self, library_content_block: LibraryContentBlock) -> bool: + def are_children_syncing(self, library_content_block: LegacyLibraryContentBlock) -> bool: """ Is a task currently running to sync the children of `library_content_block`? @@ -179,21 +191,12 @@ def are_children_syncing(self, library_content_block: LibraryContentBlock) -> bo def list_available_libraries(self): """ - List all known libraries. + List all known legacy libraries. - Collects Only V2 Libaries if the FEATURES[ENABLE_LIBRARY_AUTHORING_MICROFRONTEND] setting is True. - Otherwise, return all v1 and v2 libraries. Returns tuples of (library key, display_name). """ user = User.objects.get(id=self.user_id) - v1_libs = [ + return [ (lib.location.library_key.replace(version_guid=None, branch=None), lib.display_name) for lib in self.store.get_library_summaries() ] - v2_query = library_api.get_libraries_for_user(user) - v2_libs_with_meta = library_api.get_metadata(v2_query) - v2_libs = [(lib.key, lib.title) for lib in v2_libs_with_meta] - - if settings.FEATURES.get('ENABLE_LIBRARY_AUTHORING_MICROFRONTEND'): - return v2_libs - return v1_libs + v2_libs diff --git a/xmodule/modulestore/mixed.py b/xmodule/modulestore/mixed.py index e1ea6640acc5..fb5be170412f 100644 --- a/xmodule/modulestore/mixed.py +++ b/xmodule/modulestore/mixed.py @@ -343,7 +343,7 @@ def get_library_keys(self): @strip_key def get_library_summaries(self, **kwargs): """ - Returns a list of LibrarySummary objects. + Returns a list of LegacyLibrarySummary objects. Information contains `location`, `display_name`, `locator` of the libraries in this modulestore. """ library_summaries = {} diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py index b0965d63fecb..a83fec32bac0 100644 --- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -13,7 +13,7 @@ from xmodule.error_block import ErrorBlock from xmodule.errortracker import exc_info_to_str -from xmodule.library_tools import LibraryToolsService +from xmodule.library_tools import LegacyLibraryToolsService from xmodule.mako_block import MakoDescriptorSystem from xmodule.modulestore import BlockData from xmodule.modulestore.edit_info import EditInfoRuntimeMixin @@ -78,7 +78,7 @@ def __init__(self, modulestore, course_entry, default_class, module_data, lazy, user = get_current_user() user_id = user.id if user else None - self._services['library_tools'] = LibraryToolsService(modulestore, user_id=user_id) + self._services['library_tools'] = LegacyLibraryToolsService(modulestore, user_id=user_id) # Cache of block field datas, keyed by the XBlock instance (since the ScopeId changes!) self.block_field_datas = weakref.WeakKeyDictionary() diff --git a/xmodule/modulestore/split_mongo/split.py b/xmodule/modulestore/split_mongo/split.py index 64e19420a152..e69a2ca0e53c 100644 --- a/xmodule/modulestore/split_mongo/split.py +++ b/xmodule/modulestore/split_mongo/split.py @@ -81,7 +81,7 @@ from xmodule.course_block import CourseSummary from xmodule.error_block import ErrorBlock from xmodule.errortracker import null_error_tracker -from xmodule.library_content_block import LibrarySummary +from xmodule.library_content_block import LegacyLibrarySummary from xmodule.modulestore import ( BlockData, BulkOperationsMixin, @@ -1029,7 +1029,7 @@ def get_library_keys(self): @autoretry_read() def get_library_summaries(self, **kwargs): """ - Returns a list of `LibrarySummary` objects. + Returns a list of `LegacyLibrarySummary` objects. kwargs can be valid db fields to match against active_versions collection e.g org='example_org'. """ @@ -1057,7 +1057,7 @@ def get_library_summaries(self, **kwargs): display_name = library_block_fields['display_name'] libraries_summaries.append( - LibrarySummary(library_locator, display_name) + LegacyLibrarySummary(library_locator, display_name) ) return libraries_summaries diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index e30e19922be5..9914ef2d9098 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -1,7 +1,5 @@ """ -Basic unit tests for LibraryContentBlock - -Higher-level tests are in `cms/djangoapps/contentstore/tests/test_libraries.py`. +Basic unit tests for LegacyLibraryContentBlock """ from unittest.mock import MagicMock, Mock, patch @@ -9,15 +7,15 @@ from bson.objectid import ObjectId from fs.memoryfs import MemoryFS from lxml import etree -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryLocator from rest_framework import status from search.search_engine_base import SearchEngine from web_fragments.fragment import Fragment from xblock.runtime import Runtime as VanillaRuntime from openedx.core.djangolib.testing.utils import skip_unless_cms -from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LibraryContentBlock -from xmodule.library_tools import LibraryToolsService +from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LegacyLibraryContentBlock +from xmodule.library_tools import LegacyLibraryToolsService from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory from xmodule.modulestore.tests.utils import MixedSplitTestCase @@ -33,15 +31,15 @@ @skip_unless_cms -class LibraryContentTest(MixedSplitTestCase): +class LegacyLibraryContentTest(MixedSplitTestCase): """ - Base class for tests of LibraryContentBlock (library_content_block.py) + Base class for tests of LegacyLibraryContentBlock (library_content_block.py) """ def setUp(self): super().setUp() self.user_id = UserFactory().id - self.tools = LibraryToolsService(self.store, self.user_id) + self.tools = LegacyLibraryToolsService(self.store, self.user_id) self.library = LibraryFactory.create(modulestore=self.store) self.lib_blocks = [ self.make_block("html", self.library, data=f"Hello world from block {i}") @@ -88,29 +86,22 @@ def get_block(descriptor): @ddt.ddt -class LibraryContentGeneralTest(LibraryContentTest): +class LegacyLibraryContentGeneralTest(LegacyLibraryContentTest): """ - Test the base functionality of the LibraryContentBlock. + Test the base functionality of the LegacyLibraryContentBlock. """ - @ddt.data( - ('library-v1:ProblemX+PR0B', LibraryLocator), - ('lib:ORG:test-1', LibraryLocatorV2) - ) - @ddt.unpack - def test_source_library_key(self, library_key, expected_locator_type): + def test_source_library_key(self): """ Test the source_library_key property of the xblock. - - The method should correctly work either with V1 or V2 libraries. """ library = self.make_block( "library_content", self.vertical, max_count=1, - source_library_id=library_key + source_library_id='library-v1:ProblemX+PR0B', ) - assert isinstance(library.source_library_key, expected_locator_type) + assert isinstance(library.source_library_key, LibraryLocator) def test_initial_sync_from_library(self): """ @@ -133,9 +124,9 @@ def test_initial_sync_from_library(self): assert len(self.lc_block.children) == len(self.lib_blocks) -class TestLibraryContentExportImport(LibraryContentTest): +class TestLibraryContentExportImport(LegacyLibraryContentTest): """ - Export and import tests for LibraryContentBlock + Export and import tests for LegacyLibraryContentBlock """ def setUp(self): super().setUp() @@ -173,7 +164,6 @@ def _verify_xblock_properties(self, imported_lc_block): assert imported_lc_block.display_name == self.lc_block.display_name assert imported_lc_block.source_library_id == self.lc_block.source_library_id assert imported_lc_block.source_library_version == self.lc_block.source_library_version - assert imported_lc_block.mode == self.lc_block.mode assert imported_lc_block.max_count == self.lc_block.max_count assert imported_lc_block.capa_type == self.lc_block.capa_type assert len(imported_lc_block.children) == len(self.lc_block.children) @@ -195,13 +185,13 @@ def test_xml_export_import_cycle(self): # Now import it. olx_element = etree.fromstring(exported_olx) - imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None) + imported_lc_block = LegacyLibraryContentBlock.parse_xml(olx_element, self.runtime, None) self._verify_xblock_properties(imported_lc_block) def test_xml_import_with_comments(self): """ - Test that XML comments within LibraryContentBlock are ignored during the import. + Test that XML comments within LegacyLibraryContentBlock are ignored during the import. """ olx_with_comments = ( '\n' @@ -219,15 +209,15 @@ def test_xml_import_with_comments(self): # Import the olx. olx_element = etree.fromstring(olx_with_comments) - imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None) + imported_lc_block = LegacyLibraryContentBlock.parse_xml(olx_element, self.runtime, None) self._verify_xblock_properties(imported_lc_block) @ddt.ddt -class LibraryContentBlockTestMixin: +class LegacyLibraryContentBlockTestMixin: """ - Basic unit tests for LibraryContentBlock + Basic unit tests for LegacyLibraryContentBlock """ problem_types = [ ["multiplechoiceresponse"], ["optionresponse"], ["optionresponse", "coderesponse"], @@ -424,8 +414,7 @@ def test_non_editable_settings(self): Test the settings that are marked as "non-editable". """ non_editable_metadata_fields = self.lc_block.non_editable_metadata_fields - assert LibraryContentBlock.mode in non_editable_metadata_fields - assert LibraryContentBlock.display_name not in non_editable_metadata_fields + assert LegacyLibraryContentBlock.display_name not in non_editable_metadata_fields def test_overlimit_blocks_chosen_randomly(self): """ @@ -503,7 +492,7 @@ def test_reset_selected_children_capa_blocks(self, allow_resetting_children, max @patch.object(SearchEngine, 'get_search_engine', Mock(return_value=None, autospec=True)) -class TestLibraryContentBlockWithSearchIndex(LibraryContentBlockTestMixin, LibraryContentTest): +class TestLegacyLibraryContentBlockWithSearchIndex(LegacyLibraryContentBlockTestMixin, LegacyLibraryContentTest): """ Tests for library container with mocked search engine response. """ @@ -532,9 +521,9 @@ def setUp(self): ) @patch('xmodule.html_block.HtmlBlock.author_view', dummy_render, create=True) @patch('xmodule.x_module.DescriptorSystem.applicable_aside_types', lambda self, block: []) -class TestLibraryContentRender(LibraryContentTest): +class TestLibraryContentRender(LegacyLibraryContentTest): """ - Rendering unit tests for LibraryContentBlock + Rendering unit tests for LegacyLibraryContentBlock """ def setUp(self): @@ -559,9 +548,9 @@ def test_author_view(self): # but some js initialization should happen -class TestLibraryContentAnalytics(LibraryContentTest): +class TestLibraryContentAnalytics(LegacyLibraryContentTest): """ - Test analytics features of LibraryContentBlock + Test analytics features of LegacyLibraryContentBlock """ def setUp(self): @@ -573,7 +562,7 @@ def setUp(self): def _assert_event_was_published(self, event_type): """ - Check that a LibraryContentBlock analytics event was published by self.lc_block. + Check that a LegacyLibraryContentBlock analytics event was published by self.lc_block. """ assert self.publisher.called assert len(self.publisher.call_args[0]) == 3 # pylint:disable=unsubscriptable-object diff --git a/xmodule/tests/test_library_tools.py b/xmodule/tests/test_library_tools.py index f93066cd5c63..30b007c3d963 100644 --- a/xmodule/tests/test_library_tools.py +++ b/xmodule/tests/test_library_tools.py @@ -1,23 +1,20 @@ """ -Tests for library tools service (only used by CMS) +Tests for legacy library tools service (only used by CMS) -Currently, the only known user of the LibraryToolsService is the -LibraryContentBlock, so these tests are all written with only that +The only known user of the LegacyLibraryToolsService is the +LegacyLibraryContentBlock, so these tests are all written with only that block type in mind. """ from unittest import mock import ddt -from django.conf import settings -from django.test import override_settings -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryLocator -from common.djangoapps.student.roles import CourseInstructorRole from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest -from xmodule.library_tools import LibraryToolsService +from xmodule.library_tools import LegacyLibraryToolsService from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory from xmodule.modulestore.tests.utils import MixedSplitTestCase @@ -26,34 +23,23 @@ @ddt.ddt class ContentLibraryToolsTest(MixedSplitTestCase, ContentLibrariesRestApiTest): """ - Tests for LibraryToolsService. - - Tests interaction with learning-core-based (V2) and mongo-based (V1) content libraries. + Tests for LegacyLibraryToolsService. """ def setUp(self): super().setUp() UserFactory(is_staff=True, id=self.user_id) - self.tools = LibraryToolsService(self.store, self.user_id) + self.tools = LegacyLibraryToolsService(self.store, self.user_id) def test_list_available_libraries(self): """ - Test listing of libraries. - - Collects Only V2 Libaries if the FEATURES[ENABLE_LIBRARY_AUTHORING_MICROFRONTEND] setting is True. - Otherwise, return all v1 and v2 libraries. + Test listing of v1 libraries. """ # create V1 library _ = LibraryFactory.create(modulestore=self.store) - # create V2 library + # create V2 library (should not be included in this list) self._create_library(slug="testlib1_preview", title="Test Library 1", description="Testing XBlocks") all_libraries = self.tools.list_available_libraries() - assert all_libraries - assert len(all_libraries) == 2 - - with override_settings(FEATURES={**settings.FEATURES, "ENABLE_LIBRARY_AUTHORING_MICROFRONTEND": True}): - all_libraries = self.tools.list_available_libraries() - assert all_libraries - assert len(all_libraries) == 1 + assert len(all_libraries) == 1 @mock.patch('xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_library_summaries') def test_list_available_libraries_fetch(self, mock_get_library_summaries): @@ -63,7 +49,7 @@ def test_list_available_libraries_fetch(self, mock_get_library_summaries): _ = self.tools.list_available_libraries() assert mock_get_library_summaries.called - def test_get_latest_v1_library_version(self): + def test_get_latest_library_version(self): """ Test get_v1_library_version for V1 libraries. @@ -84,49 +70,16 @@ def test_get_latest_v1_library_version(self): assert result == str(lib.location.library_key.version_guid) @ddt.data( - 'library-v1:Fake+Key', # V1 library key - 'lib:Fake:V-2', # V2 library key + 'library-v1:Fake+Key', LibraryLocator.from_string('library-v1:Fake+Key'), - LibraryLocatorV2.from_string('lib:Fake:V-2'), ) def test_get_latest_library_version_no_library(self, lib_key): """ Test get_latest_library_version result when the library does not exist. - - Provided lib_key's are valid V1 or V2 keys. """ assert self.tools.get_latest_library_version(lib_key) is None - def test_update_children_for_v2_lib(self): - """ - Test update_children with V2 library as a source. - """ - library = self._create_library( - slug="cool-v2-lib", title="The best Library", description="Spectacular description" - ) - self._add_block_to_library(library["id"], "unit", "unit1_id") - - course = CourseFactory.create(modulestore=self.store, user_id=self.user.id) - CourseInstructorRole(course.id).add_users(self.user) - - content_block = self.make_block( - "library_content", - course, - max_count=1, - source_library_id=library['id'] - ) - assert len(content_block.children) == 0 - - # Populate children from library - self.tools.trigger_library_sync(content_block, library_version=None) - - # The updates happen in a Celery task, so this particular content_block instance is no updated. - # We must re-instantiate it from modulstore in order to see the updated children list. - content_block = self.store.get_item(content_block.location) - - assert len(content_block.children) == 1 - - def test_update_children_for_v1_lib(self): + def test_update_children(self): """ Test update_children with V1 library as a source. diff --git a/xmodule/tests/test_randomize_block.py b/xmodule/tests/test_randomize_block.py index deebdfe4f1d6..52413faad3b7 100644 --- a/xmodule/tests/test_randomize_block.py +++ b/xmodule/tests/test_randomize_block.py @@ -16,7 +16,7 @@ class RandomizeBlockTest(MixedSplitTestCase): """ - Base class for tests of LibraryContentBlock (library_content_block.py) + Base class for tests of RandomizeBlock (randomize_block.py) """ maxDiff = None diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index 2a10ae44497f..81621a7a7b88 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -188,7 +188,7 @@ def block_has_access_error(self, block): if has_access_error: return True - # Check child nodes if they exist (e.g. randomized library question aka LibraryContentBlock) + # Check child nodes if they exist (e.g. randomized library question aka LegacyLibraryContentBlock) for child in block.get_children(): has_access_error = getattr(child, 'has_access_error', False) if has_access_error: