From 586f663703f07dd2af8d38bd769cd349f159f502 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Sun, 13 Oct 2024 11:41:41 -0400 Subject: [PATCH] feat!: Remove outdated Libraries Relaunch cruft The V2 libraries project had a few past iterations which were never launched. This commit cleans up pieces from those which we don't need for the real Libraries Relaunch MVP in Sumac: * Remove ENABLE_LIBRARY_AUTHORING_MICROFRONTEND, LIBRARY_AUTHORING_FRONTEND_URL, and REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND, all of which are obsolete now that library authoring has been merged into https://github.com/openedx/frontend-app-authoring. More details on the new Content Libraries configuration settings are here: https://github.com/openedx/frontend-app-authoring/issues/1334 * Remove dangling support for syncing V2 (learning core-backed) library content using the LibraryContentBlock. This code was all based on an older understanding of V2 Content Libraries, where the libraries were smaller and versioned as a whole rather then versioned by-item. Reference to V2 libraries will be done on a per-block basis using the upstream/downstream system, described here: https://github.com/openedx/edx-platform/blob/master/docs/decisions/0020-upstream-downstream.rst It's important that we remove this support now so that OLX course authors don't stuble upon it and use it, which would be buggy and complicate future migrations. * Remove the "mode" parameter from LibraryContentBlock. The only supported mode was and is "random". We will not be adding any further modes. Going forward for V2, we will have an ItemBank block for randomizing items (regardless of source), which can be synthesized with upstream referenced as described above. Existing LibraryContentBlocks will be migrated. * Finally, some renamings: * LibraryContentBlock -> LegacyLibraryContentBlock * LibraryToolsService -> LegacyLibraryToolsService * LibrarySummary -> LegacyLibrarySummary Module names and the old OLX tag (library_content) are unchanged. Closes: https://github.com/openedx/frontend-app-authoring/issues/1115 --- cms/djangoapps/contentstore/config/waffle.py | 16 +- .../rest_api/v1/serializers/home.py | 2 - .../contentstore/rest_api/v1/views/home.py | 2 - .../rest_api/v1/views/tests/test_home.py | 2 - cms/djangoapps/contentstore/utils.py | 13 +- cms/djangoapps/contentstore/views/library.py | 16 -- .../contentstore/views/tests/test_block.py | 2 +- .../views/tests/test_clipboard_paste.py | 43 ++-- cms/envs/common.py | 15 +- cms/envs/devstack.py | 3 - cms/templates/index.html | 4 - docs/docs_settings.py | 1 - .../transformers/library_content.py | 8 +- lms/djangoapps/courseware/block_render.py | 4 +- lms/djangoapps/grades/rest_api/v1/views.py | 4 +- .../core/djangoapps/content_libraries/api.py | 79 +------ .../djangoapps/content_libraries/tasks.py | 195 +++--------------- openedx/core/lib/xblock_utils/__init__.py | 2 +- .../completion_integration/test_services.py | 4 +- setup.py | 2 +- webpack.builtinblocks.config.js | 9 +- .../public/js/library_content_edit_helpers.js | 54 ----- xmodule/library_content_block.py | 77 ++----- xmodule/library_tools.py | 75 +++---- xmodule/modulestore/mixed.py | 2 +- .../split_mongo/caching_descriptor_system.py | 4 +- xmodule/modulestore/split_mongo/split.py | 6 +- xmodule/tests/test_library_content.py | 63 +++--- xmodule/tests/test_library_tools.py | 73 ++----- xmodule/tests/test_randomize_block.py | 2 +- xmodule/vertical_block.py | 2 +- 31 files changed, 180 insertions(+), 604 deletions(-) delete mode 100644 xmodule/assets/library_content/public/js/library_content_edit_helpers.js 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 b9f3779af539..5876fe79dc25 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, @@ -97,10 +96,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 @@ -408,8 +404,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 @@ -1265,77 +1261,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: