diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 019f67a594d9..eb9b109cdd35 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -15,8 +15,8 @@ from xmodule.x_module import STUDIO_VIEW from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, parse_json -from cms.djangoapps.contentstore.utils import reverse_library_url, reverse_url, reverse_usage_url -from cms.djangoapps.contentstore.views.block import _duplicate_block +from cms.djangoapps.contentstore.utils import reverse_library_url, reverse_url, \ + reverse_usage_url, duplicate_block from cms.djangoapps.contentstore.views.preview import _load_preview_block from cms.djangoapps.contentstore.views.tests.test_library import LIBRARY_REST_URL from cms.djangoapps.course_creators.views import add_user_with_status_granted @@ -947,7 +947,7 @@ def test_persistent_overrides(self, duplicate): if duplicate: # Check that this also works when the RCB is duplicated. self.lc_block = modulestore().get_item( - _duplicate_block(self.course.location, self.lc_block.location, self.user) + duplicate_block(self.course.location, self.lc_block.location, self.user) ) self.problem_in_course = modulestore().get_item(self.lc_block.children[0]) else: @@ -1006,7 +1006,7 @@ def test_duplicated_version(self): # Duplicate self.lc_block: duplicate = store.get_item( - _duplicate_block(self.course.location, self.lc_block.location, self.user) + duplicate_block(self.course.location, self.lc_block.location, self.user) ) # The duplicate should have identical children to the original: self.assertEqual(len(duplicate.children), 1) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 14f86917b4c3..ed1d1e5df8f7 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -5,20 +5,28 @@ from collections import defaultdict import logging from contextlib import contextmanager -from datetime import datetime +from datetime import datetime, timezone +from uuid import uuid4 from django.conf import settings from django.urls import reverse from django.utils import translation from django.utils.translation import gettext as _ +from lti_consumer.models import CourseAllowPIISharingInLTIFlag from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryLocator +from openedx_events.content_authoring.data import DuplicatedXBlockData +from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED from pytz import UTC +from xblock.fields import Scope from cms.djangoapps.contentstore.toggles import exam_setting_view_enabled +from common.djangoapps.edxmako.services import MakoService from common.djangoapps.student import auth +from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole +from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from openedx.core.djangoapps.course_apps.toggles import proctoring_settings_modal_view_enabled from openedx.core.djangoapps.discussions.config.waffle import ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration @@ -29,8 +37,6 @@ from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.content_type_gating.partitions import CONTENT_TYPE_GATING_SCHEME from cms.djangoapps.contentstore.toggles import ( - use_new_text_editor, - use_new_video_editor, use_new_advanced_settings_page, use_new_course_outline_page, use_new_export_page, @@ -44,10 +50,13 @@ use_new_updates_page, use_new_video_uploads_page, ) +from cms.djangoapps.contentstore.toggles import use_new_text_editor, use_new_video_editor +from xmodule.library_tools import LibraryToolsService from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order from xmodule.partitions.partitions_service import get_all_partitions_for_course # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.services import SettingsService, ConfigurationService, TeamsConfigurationService log = logging.getLogger(__name__) @@ -935,3 +944,202 @@ def update_course_discussions_settings(course_key): course = store.get_course(course_key) course.discussions_settings['provider_type'] = provider store.update_item(course, course.published_by) + + +def duplicate_block( + parent_usage_key, + duplicate_source_usage_key, + user, + dest_usage_key=None, + display_name=None, + shallow=False, + is_child=False +): + """ + Duplicate an existing xblock as a child of the supplied parent_usage_key. You can + optionally specify what usage key the new duplicate block will use via dest_usage_key. + + If shallow is True, does not copy children. Otherwise, this function calls itself + recursively, and will set the is_child flag to True when dealing with recursed child + blocks. + """ + store = modulestore() + with store.bulk_operations(duplicate_source_usage_key.course_key): + source_item = store.get_item(duplicate_source_usage_key) + if not dest_usage_key: + # Change the blockID to be unique. + dest_usage_key = source_item.location.replace(name=uuid4().hex) + + category = dest_usage_key.block_type + + duplicate_metadata, asides_to_create = gather_block_attributes( + source_item, display_name=display_name, is_child=is_child, + ) + + dest_block = store.create_item( + user.id, + dest_usage_key.course_key, + dest_usage_key.block_type, + block_id=dest_usage_key.block_id, + definition_data=source_item.get_explicitly_set_fields_by_scope(Scope.content), + metadata=duplicate_metadata, + runtime=source_item.runtime, + asides=asides_to_create + ) + + children_handled = False + + if hasattr(dest_block, 'studio_post_duplicate'): + # Allow an XBlock to do anything fancy it may need to when duplicated from another block. + # These blocks may handle their own children or parenting if needed. Let them return booleans to + # let us know if we need to handle these or not. + load_services_for_studio(dest_block.runtime, user) + children_handled = dest_block.studio_post_duplicate(store, source_item) + + # Children are not automatically copied over (and not all xblocks have a 'children' attribute). + # Because DAGs are not fully supported, we need to actually duplicate each child as well. + if source_item.has_children and not shallow and not children_handled: + dest_block.children = dest_block.children or [] + for child in source_item.children: + dupe = duplicate_block(dest_block.location, child, user=user, is_child=True) + if dupe not in dest_block.children: # _duplicate_block may add the child for us. + dest_block.children.append(dupe) + store.update_item(dest_block, user.id) + + # pylint: disable=protected-access + if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: + parent = store.get_item(parent_usage_key) + # If source was already a child of the parent, add duplicate immediately afterward. + # Otherwise, add child to end. + if source_item.location in parent.children: + source_index = parent.children.index(source_item.location) + parent.children.insert(source_index + 1, dest_block.location) + else: + parent.children.append(dest_block.location) + store.update_item(parent, user.id) + + # .. event_implemented_name: XBLOCK_DUPLICATED + XBLOCK_DUPLICATED.send_event( + time=datetime.now(timezone.utc), + xblock_info=DuplicatedXBlockData( + usage_key=dest_block.location, + block_type=dest_block.location.block_type, + source_usage_key=duplicate_source_usage_key, + ) + ) + + return dest_block.location + + +def update_from_source(*, source_block, destination_block, user_id): + """ + Update a block to have all the settings and attributes of another source. + + Copies over all attributes and settings of a source block to a destination + block. Blocks must be the same type. This function does not modify or duplicate + children. + + This function is useful when a block, originally copied from a source block, drifts + and needs to be updated to match the original. + + The modulestore function copy_from_template will copy a block's children recursively, + replacing the target block's children. It does not, however, update any of the target + block's settings. copy_from_template, then, is useful for cases like the Library + Content Block, where the children are the same across all instances, but the settings + may differ. + + By contrast, for cases where we're copying a block that has drifted from its source, + we need to update the target block's settings, but we don't want to replace its children, + or, at least, not only replace its children. update_from_source is useful for these cases. + + This function is meant to be imported by pluggable django apps looking to manage duplicated + sections of a course. It is placed here for lack of a more appropriate location, since this + code has not yet been brought up to the standards in OEP-45. + """ + duplicate_metadata, asides = gather_block_attributes(source_block, display_name=source_block.display_name) + for key, value in duplicate_metadata.items(): + setattr(destination_block, key, value) + for key, value in source_block.get_explicitly_set_fields_by_scope(Scope.content).items(): + setattr(destination_block, key, value) + modulestore().update_item( + destination_block, + user_id, + metadata=duplicate_metadata, + asides=asides, + ) + + +def gather_block_attributes(source_item, display_name=None, is_child=False): + """ + Gather all the attributes of the source block that need to be copied over to a new or updated block. + """ + # Update the display name to indicate this is a duplicate (unless display name provided). + # Can't use own_metadata(), b/c it converts data for JSON serialization - + # not suitable for setting metadata of the new block + duplicate_metadata = {} + for field in source_item.fields.values(): + if field.scope == Scope.settings and field.is_set_on(source_item): + duplicate_metadata[field.name] = field.read_from(source_item) + + if is_child: + display_name = display_name or source_item.display_name or source_item.category + + if display_name is not None: + duplicate_metadata['display_name'] = display_name + else: + if source_item.display_name is None: + duplicate_metadata['display_name'] = _("Duplicate of {0}").format(source_item.category) + else: + duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name) + + asides_to_create = [] + for aside in source_item.runtime.get_asides(source_item): + for field in aside.fields.values(): + if field.scope in (Scope.settings, Scope.content,) and field.is_set_on(aside): + asides_to_create.append(aside) + break + + for aside in asides_to_create: + for field in aside.fields.values(): + if field.scope not in (Scope.settings, Scope.content,): + field.delete_from(aside) + return duplicate_metadata, asides_to_create + + +def load_services_for_studio(runtime, user): + """ + Function to set some required services used for XBlock edits and studio_view. + (i.e. whenever we're not loading _prepare_runtime_for_preview.) This is required to make information + about the current user (especially permissions) available via services as needed. + """ + services = { + "user": DjangoXBlockUserService(user), + "studio_user_permissions": StudioPermissionsService(user), + "mako": MakoService(), + "settings": SettingsService(), + "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), + "teams_configuration": TeamsConfigurationService(), + "library_tools": LibraryToolsService(modulestore(), user.id) + } + + runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access + + +class StudioPermissionsService: + """ + Service that can provide information about a user's permissions. + + Deprecated. To be replaced by a more general authorization service. + + Only used by LibraryContentBlock (and library_tools.py). + """ + def __init__(self, user): + self._user = user + + def can_read(self, course_key): + """ Does the user have read access to the given course/library? """ + return has_studio_read_access(self._user, course_key) + + def can_write(self, course_key): + """ Does the user have read access to the given course/library? """ + return has_studio_write_access(self._user, course_key) diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index 2cadea08ea33..a35230f3b8a5 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -4,19 +4,15 @@ from collections import OrderedDict from datetime import datetime from functools import partial -from uuid import uuid4 from django.conf import settings from django.contrib.auth.decorators import login_required from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import PermissionDenied from django.http import Http404, HttpResponse, HttpResponseBadRequest -from django.utils.timezone import timezone from django.utils.translation import gettext as _ from django.views.decorators.http import require_http_methods from edx_django_utils.plugins import pluggable_override -from openedx_events.content_authoring.data import DuplicatedXBlockData -from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED from edx_proctoring.api import ( does_backend_support_onboarding, get_exam_by_content_id, @@ -24,7 +20,6 @@ ) from edx_proctoring.exceptions import ProctoredExamNotFoundException from help_tokens.core import HelpUrlExpert -from lti_consumer.models import CourseAllowPIISharingInLTIFlag from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryUsageLocator from pytz import UTC @@ -35,13 +30,11 @@ from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW -from common.djangoapps.edxmako.services import MakoService from common.djangoapps.edxmako.shortcuts import render_to_string from common.djangoapps.static_replace import replace_static_urls from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access from common.djangoapps.util.date_utils import get_default_time_display from common.djangoapps.util.json_request import JsonResponse, expect_json -from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from openedx.core.djangoapps.bookmarks import api as bookmarks_api from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration from openedx.core.djangoapps.video_config.toggles import PUBLIC_VIDEO_SHARE @@ -49,13 +42,11 @@ from openedx.core.lib.xblock_utils import hash_resource, request_token, wrap_xblock, wrap_xblock_aside from openedx.core.toggles import ENTRANCE_EXAMS from xmodule.course_block import DEFAULT_START_DATE # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.library_tools import LibraryToolsService # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore import EdxJSONEncoder, ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.inheritance import own_metadata # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService # lint-amnesty, pylint: disable=wrong-import-order from xmodule.tabs import CourseTabList # lint-amnesty, pylint: disable=wrong-import-order from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW, STUDIO_VIEW # lint-amnesty, pylint: disable=wrong-import-order @@ -68,7 +59,7 @@ get_visibility_partition_info, has_children_visible_to_specific_partition_groups, is_currently_visible_to_students, - is_self_paced + is_self_paced, duplicate_block, load_services_for_studio ) from .helpers import ( create_xblock, @@ -245,11 +236,11 @@ def xblock_handler(request, usage_key_string=None): status=400 ) - dest_usage_key = _duplicate_block( + dest_usage_key = duplicate_block( parent_usage_key, duplicate_source_usage_key, request.user, - request.json.get('display_name'), + display_name=request.json.get('display_name'), ) return JsonResponse({ 'locator': str(dest_usage_key), @@ -277,45 +268,6 @@ def xblock_handler(request, usage_key_string=None): ) -class StudioPermissionsService: - """ - Service that can provide information about a user's permissions. - - Deprecated. To be replaced by a more general authorization service. - - Only used by LibraryContentBlock (and library_tools.py). - """ - def __init__(self, user): - self._user = user - - def can_read(self, course_key): - """ Does the user have read access to the given course/library? """ - return has_studio_read_access(self._user, course_key) - - def can_write(self, course_key): - """ Does the user have read access to the given course/library? """ - return has_studio_write_access(self._user, course_key) - - -def load_services_for_studio(runtime, user): - """ - Function to set some required services used for XBlock edits and studio_view. - (i.e. whenever we're not loading _prepare_runtime_for_preview.) This is required to make information - about the current user (especially permissions) available via services as needed. - """ - services = { - "user": DjangoXBlockUserService(user), - "studio_user_permissions": StudioPermissionsService(user), - "mako": MakoService(), - "settings": SettingsService(), - "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), - "teams_configuration": TeamsConfigurationService(), - "library_tools": LibraryToolsService(modulestore(), user.id) - } - - runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access - - @require_http_methods("GET") @login_required @expect_json @@ -880,103 +832,6 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non return JsonResponse(context) -def _duplicate_block(parent_usage_key, duplicate_source_usage_key, user, display_name=None, is_child=False): - """ - Duplicate an existing xblock as a child of the supplied parent_usage_key. - """ - store = modulestore() - with store.bulk_operations(duplicate_source_usage_key.course_key): - source_item = store.get_item(duplicate_source_usage_key) - # Change the blockID to be unique. - dest_usage_key = source_item.location.replace(name=uuid4().hex) - category = dest_usage_key.block_type - - # Update the display name to indicate this is a duplicate (unless display name provided). - # Can't use own_metadata(), b/c it converts data for JSON serialization - - # not suitable for setting metadata of the new block - duplicate_metadata = {} - for field in source_item.fields.values(): - if field.scope == Scope.settings and field.is_set_on(source_item): - duplicate_metadata[field.name] = field.read_from(source_item) - - if is_child: - display_name = display_name or source_item.display_name or source_item.category - - if display_name is not None: - duplicate_metadata['display_name'] = display_name - else: - if source_item.display_name is None: - duplicate_metadata['display_name'] = _("Duplicate of {0}").format(source_item.category) - else: - duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name) - - asides_to_create = [] - for aside in source_item.runtime.get_asides(source_item): - for field in aside.fields.values(): - if field.scope in (Scope.settings, Scope.content,) and field.is_set_on(aside): - asides_to_create.append(aside) - break - - for aside in asides_to_create: - for field in aside.fields.values(): - if field.scope not in (Scope.settings, Scope.content,): - field.delete_from(aside) - - dest_block = store.create_item( - user.id, - dest_usage_key.course_key, - dest_usage_key.block_type, - block_id=dest_usage_key.block_id, - definition_data=source_item.get_explicitly_set_fields_by_scope(Scope.content), - metadata=duplicate_metadata, - runtime=source_item.runtime, - asides=asides_to_create - ) - - children_handled = False - - if hasattr(dest_block, 'studio_post_duplicate'): - # Allow an XBlock to do anything fancy it may need to when duplicated from another block. - # These blocks may handle their own children or parenting if needed. Let them return booleans to - # let us know if we need to handle these or not. - load_services_for_studio(dest_block.runtime, user) - children_handled = dest_block.studio_post_duplicate(store, source_item) - - # Children are not automatically copied over (and not all xblocks have a 'children' attribute). - # Because DAGs are not fully supported, we need to actually duplicate each child as well. - if source_item.has_children and not children_handled: - dest_block.children = dest_block.children or [] - for child in source_item.children: - dupe = _duplicate_block(dest_block.location, child, user=user, is_child=True) - if dupe not in dest_block.children: # _duplicate_block may add the child for us. - dest_block.children.append(dupe) - store.update_item(dest_block, user.id) - - # pylint: disable=protected-access - if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: - parent = store.get_item(parent_usage_key) - # If source was already a child of the parent, add duplicate immediately afterward. - # Otherwise, add child to end. - if source_item.location in parent.children: - source_index = parent.children.index(source_item.location) - parent.children.insert(source_index + 1, dest_block.location) - else: - parent.children.append(dest_block.location) - store.update_item(parent, user.id) - - # .. event_implemented_name: XBLOCK_DUPLICATED - XBLOCK_DUPLICATED.send_event( - time=datetime.now(timezone.utc), - xblock_info=DuplicatedXBlockData( - usage_key=dest_block.location, - block_type=dest_block.location.block_type, - source_usage_key=duplicate_source_usage_key, - ) - ) - - return dest_block.location - - @login_required @expect_json def delete_item(request, usage_key): diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 60d6f68c9adc..8aadd418698a 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -35,9 +35,10 @@ from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order -from ..utils import get_lms_link_for_item, get_sibling_urls, reverse_course_url +from ..utils import get_lms_link_for_item, get_sibling_urls, reverse_course_url, \ + load_services_for_studio from .helpers import get_parent_xblock, is_unit, xblock_type_display_name -from .block import add_container_page_publishing_info, create_xblock_info, load_services_for_studio +from .block import add_container_page_publishing_info, create_xblock_info __all__ = [ 'container_handler', diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 841e93c656fe..bb827d361c63 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -48,7 +48,7 @@ from xmodule.x_module import STUDENT_VIEW, STUDIO_VIEW from cms.djangoapps.contentstore.tests.utils import CourseTestCase -from cms.djangoapps.contentstore.utils import reverse_course_url, reverse_usage_url +from cms.djangoapps.contentstore.utils import reverse_course_url, reverse_usage_url, duplicate_block, update_from_source from cms.djangoapps.contentstore.views import block as item_module from common.djangoapps.student.tests.factories import StaffFactory, UserFactory from common.djangoapps.xblock_django.models import ( @@ -787,6 +787,30 @@ def verify_name(source_usage_key, parent_usage_key, expected_name, display_name= # Now send a custom display name for the duplicate. verify_name(self.seq_usage_key, self.chapter_usage_key, "customized name", display_name="customized name") + def test_shallow_duplicate(self): + """ + Test that duplicate_block(..., shallow=True) can duplicate a block but ignores its children. + """ + source_course = CourseFactory() + user = UserFactory.create() + source_chapter = BlockFactory(parent=source_course, category='chapter', display_name='Source Chapter') + BlockFactory(parent=source_chapter, category='html', display_name='Child') + # Refresh. + source_chapter = self.store.get_item(source_chapter.location) + self.assertEqual(len(source_chapter.get_children()), 1) + destination_course = CourseFactory() + destination_location = duplicate_block( + parent_usage_key=destination_course.location, + duplicate_source_usage_key=source_chapter.location, + user=user, + display_name=source_chapter.display_name, + shallow=True, + ) + # Refresh here, too, just to be sure. + destination_chapter = self.store.get_item(destination_location) + self.assertEqual(len(destination_chapter.get_children()), 0) + self.assertEqual(destination_chapter.display_name, 'Source Chapter') + @ddt.ddt class TestMoveItem(ItemTest): @@ -3495,3 +3519,111 @@ def test_self_paced_item_visibility_state(self, store_type): # Check that in self paced course content has live state now xblock_info = self._get_xblock_info(chapter.location) self._verify_visibility_state(xblock_info, VisibilityState.live) + + +@patch('xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types', + lambda self, block: ['test_aside']) +class TestUpdateFromSource(ModuleStoreTestCase): + """ + Test update_from_source. + """ + + def setUp(self): + """ + Set up the runtime for tests. + """ + super().setUp() + key_store = DictKeyValueStore() + field_data = KvsFieldData(key_store) + self.runtime = TestRuntime(services={'field-data': field_data}) + + def create_source_block(self, course): + """ + Create a chapter with all the fixings. + """ + source_block = BlockFactory( + parent=course, + category='course_info', + display_name='Source Block', + metadata={'due': datetime(2010, 11, 22, 4, 0, tzinfo=UTC)}, + ) + + def_id = self.runtime.id_generator.create_definition('html') + usage_id = self.runtime.id_generator.create_usage(def_id) + + aside = AsideTest(scope_ids=ScopeIds('user', 'html', def_id, usage_id), runtime=self.runtime) + aside.field11 = 'html_new_value1' + + # The data attribute is handled in a special manner and should be updated. + source_block.data = '
test
' + # This field is set on the content scope (definition_data), which should be updated. + source_block.items = ['test', 'beep'] + + self.store.update_item(source_block, self.user.id, asides=[aside]) + + # quick sanity checks + source_block = self.store.get_item(source_block.location) + self.assertEqual(source_block.due, datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) + self.assertEqual(source_block.display_name, 'Source Block') + self.assertEqual(source_block.runtime.get_asides(source_block)[0].field11, 'html_new_value1') + self.assertEqual(source_block.data, '
test
') + self.assertEqual(source_block.items, ['test', 'beep']) + + return source_block + + def check_updated(self, source_block, destination_key): + """ + Check that the destination block has been updated to match our source block. + """ + revised = self.store.get_item(destination_key) + self.assertEqual(source_block.display_name, revised.display_name) + self.assertEqual(source_block.due, revised.due) + self.assertEqual(revised.data, source_block.data) + self.assertEqual(revised.items, source_block.items) + + self.assertEqual( + revised.runtime.get_asides(revised)[0].field11, + source_block.runtime.get_asides(source_block)[0].field11, + ) + + @XBlockAside.register_temp_plugin(AsideTest, 'test_aside') + def test_update_from_source(self): + """ + Test that update_from_source updates the destination block. + """ + course = CourseFactory() + user = UserFactory.create() + + source_block = self.create_source_block(course) + + destination_block = BlockFactory(parent=course, category='course_info', display_name='Destination Problem') + update_from_source(source_block=source_block, destination_block=destination_block, user_id=user.id) + self.check_updated(source_block, destination_block.location) + + @XBlockAside.register_temp_plugin(AsideTest, 'test_aside') + def test_update_clobbers(self): + """ + Verify that our update replaces all settings on the block. + """ + course = CourseFactory() + user = UserFactory.create() + + source_block = self.create_source_block(course) + + destination_block = BlockFactory( + parent=course, + category='course_info', + display_name='Destination Chapter', + metadata={'due': datetime(2025, 10, 21, 6, 5, tzinfo=UTC)}, + ) + + def_id = self.runtime.id_generator.create_definition('html') + usage_id = self.runtime.id_generator.create_usage(def_id) + aside = AsideTest(scope_ids=ScopeIds('user', 'html', def_id, usage_id), runtime=self.runtime) + aside.field11 = 'Other stuff' + destination_block.data = '
other stuff
' + destination_block.items = ['other stuff', 'boop'] + self.store.update_item(destination_block, user.id, asides=[aside]) + + update_from_source(source_block=source_block, destination_block=destination_block, user_id=user.id) + self.check_updated(source_block, destination_block.location) diff --git a/common/static/data/geoip/GeoLite2-Country.mmdb b/common/static/data/geoip/GeoLite2-Country.mmdb index 3bd749e071c9..963f72622fb2 100644 Binary files a/common/static/data/geoip/GeoLite2-Country.mmdb and b/common/static/data/geoip/GeoLite2-Country.mmdb differ diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 4d86edcc103e..afad888fe0c5 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -10,21 +10,10 @@ import time from collections import Counter from datetime import datetime -from smtplib import SMTPConnectError, SMTPDataError, SMTPException, SMTPServerDisconnected, SMTPSenderRefused +from smtplib import SMTPConnectError, SMTPDataError, SMTPException, SMTPSenderRefused, SMTPServerDisconnected from time import sleep -from boto.exception import AWSConnectionError -from boto.ses.exceptions import ( - SESAddressBlacklistedError, - SESAddressNotVerifiedError, - SESDailyQuotaExceededError, - SESDomainEndsWithDotError, - SESDomainNotConfirmedError, - SESIdentityNotVerifiedError, - SESIllegalAddressError, - SESLocalAddressCharacterError, - SESMaxSendingRateExceededError -) +from botocore.exceptions import ClientError, EndpointConnectionError from celery import current_task, shared_task from celery.exceptions import RetryTaskError from celery.states import FAILURE, RETRY, SUCCESS @@ -34,8 +23,8 @@ from django.core.mail.message import forbid_multi_line_headers from django.urls import reverse from django.utils import timezone -from django.utils.translation import override as override_language from django.utils.translation import gettext as _ +from django.utils.translation import override as override_language from edx_django_utils.monitoring import set_code_owner_attribute from markupsafe import escape @@ -43,15 +32,12 @@ from common.djangoapps.util.string_utils import _has_non_ascii_characters from lms.djangoapps.branding.api import get_logo_url_for_email from lms.djangoapps.bulk_email.api import get_unsubscribed_link -from lms.djangoapps.bulk_email.messages import ( - DjangoEmail, - ACEEmail, -) +from lms.djangoapps.bulk_email.messages import ACEEmail, DjangoEmail +from lms.djangoapps.bulk_email.models import CourseEmail, Optout from lms.djangoapps.bulk_email.toggles import ( is_bulk_email_edx_ace_enabled, - is_email_use_course_id_from_for_bulk_enabled, + is_email_use_course_id_from_for_bulk_enabled ) -from lms.djangoapps.bulk_email.models import CourseEmail, Optout from lms.djangoapps.courseware.courses import get_course from lms.djangoapps.instructor_task.models import InstructorTask from lms.djangoapps.instructor_task.subtasks import ( @@ -66,13 +52,11 @@ log = logging.getLogger('edx.celery.task') + # Errors that an individual email is failing to be sent, and should just # be treated as a fail. SINGLE_EMAIL_FAILURE_ERRORS = ( - SESAddressBlacklistedError, # Recipient's email address has been temporarily blacklisted. - SESDomainEndsWithDotError, # Recipient's email address' domain ends with a period/dot. - SESIllegalAddressError, # Raised when an illegal address is encountered. - SESLocalAddressCharacterError, # An address contained a control or whitespace character. + ClientError ) # Exceptions that, if caught, should cause the task to be re-tried. @@ -80,7 +64,7 @@ LIMITED_RETRY_ERRORS = ( SMTPConnectError, SMTPServerDisconnected, - AWSConnectionError, + EndpointConnectionError, ) # Errors that indicate that a mailing task should be retried without limit. @@ -91,20 +75,17 @@ # Those not in this range (i.e. in the 5xx range) are treated as hard failures # and thus like SINGLE_EMAIL_FAILURE_ERRORS. INFINITE_RETRY_ERRORS = ( - SESMaxSendingRateExceededError, # Your account's requests/second limit has been exceeded. SMTPDataError, SMTPSenderRefused, + ClientError ) # Errors that are known to indicate an inability to send any more emails, # and should therefore not be retried. For example, exceeding a quota for emails. # Also, any SMTP errors that are not explicitly enumerated above. BULK_EMAIL_FAILURE_ERRORS = ( - SESAddressNotVerifiedError, # Raised when a "Reply-To" address has not been validated in SES yet. - SESIdentityNotVerifiedError, # Raised when an identity has not been verified in SES yet. - SESDomainNotConfirmedError, # Raised when domain ownership is not confirmed for DKIM. - SESDailyQuotaExceededError, # 24-hour allotment of outbound email has been exceeded. - SMTPException, + ClientError, + SMTPException ) @@ -588,13 +569,16 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas except SINGLE_EMAIL_FAILURE_ERRORS as exc: # This will fall through and not retry the message. - total_recipients_failed += 1 - log.exception( - f"BulkEmail ==> Status: Failed(SINGLE_EMAIL_FAILURE_ERRORS), Task: {parent_task_id}, SubTask: " - f"{task_id}, EmailId: {email_id}, Recipient num: {recipient_num}/{total_recipients}, Recipient " - f"UserId: {current_recipient['pk']}" - ) - subtask_status.increment(failed=1) + if exc.response['Error']['Code'] in ['MessageRejected', 'MailFromDomainNotVerified', 'MailFromDomainNotVerifiedException', 'FromEmailAddressNotVerifiedException']: # lint-amnesty, pylint: disable=line-too-long + total_recipients_failed += 1 + log.exception( + f"BulkEmail ==> Status: Failed(SINGLE_EMAIL_FAILURE_ERRORS), Task: {parent_task_id}, SubTask: " + f"{task_id}, EmailId: {email_id}, Recipient num: {recipient_num}/{total_recipients}, Recipient " + f"UserId: {current_recipient['pk']}" + ) + subtask_status.increment(failed=1) + else: + raise exc else: total_recipients_successful += 1 @@ -631,10 +615,13 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas except INFINITE_RETRY_ERRORS as exc: # Increment the "retried_nomax" counter, update other counters with progress to date, # and set the state to RETRY: - subtask_status.increment(retried_nomax=1, state=RETRY) - return _submit_for_retry( - entry_id, email_id, to_list, global_email_context, exc, subtask_status, skip_retry_max=True - ) + if isinstance(exc, (SMTPDataError, SMTPSenderRefused)) or exc.response['Error']['Code'] in ['LimitExceededException']: # lint-amnesty, pylint: disable=line-too-long + subtask_status.increment(retried_nomax=1, state=RETRY) + return _submit_for_retry( + entry_id, email_id, to_list, global_email_context, exc, subtask_status, skip_retry_max=True + ) + else: + raise exc except LIMITED_RETRY_ERRORS as exc: # Errors caught here cause the email to be retried. The entire task is actually retried @@ -642,21 +629,28 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas # Errors caught are those that indicate a temporary condition that might succeed on retry. # Increment the "retried_withmax" counter, update other counters with progress to date, # and set the state to RETRY: + subtask_status.increment(retried_withmax=1, state=RETRY) return _submit_for_retry( entry_id, email_id, to_list, global_email_context, exc, subtask_status, skip_retry_max=False ) except BULK_EMAIL_FAILURE_ERRORS as exc: - num_pending = len(to_list) - log.exception( - f"Task {task_id}: email with id {email_id} caused send_course_email task to fail with 'fatal' exception. " - f"{num_pending} emails unsent." - ) - # Update counters with progress to date, counting unsent emails as failures, - # and set the state to FAILURE: - subtask_status.increment(failed=num_pending, state=FAILURE) - return subtask_status, exc + if isinstance(exc, SMTPException) or exc.response['Error']['Code'] in [ + 'AccountSendingPausedException', 'MailFromDomainNotVerifiedException', 'LimitExceededException' + ]: + num_pending = len(to_list) + log.exception( + f"Task {task_id}: email with id {email_id} caused send_course_email " + f"task to fail with 'fatal' exception. " + f"{num_pending} emails unsent." + ) + # Update counters with progress to date, counting unsent emails as failures, + # and set the state to FAILURE: + subtask_status.increment(failed=num_pending, state=FAILURE) + return subtask_status, exc + else: + raise exc except Exception as exc: # pylint: disable=broad-except # Errors caught here cause the email to be retried. The entire task is actually retried diff --git a/lms/djangoapps/bulk_email/tests/test_tasks.py b/lms/djangoapps/bulk_email/tests/test_tasks.py index e73db046aab8..96c48e0d9e21 100644 --- a/lms/djangoapps/bulk_email/tests/test_tasks.py +++ b/lms/djangoapps/bulk_email/tests/test_tasks.py @@ -7,27 +7,23 @@ """ -from datetime import datetime -from dateutil.relativedelta import relativedelta import json # lint-amnesty, pylint: disable=wrong-import-order +from datetime import datetime from itertools import chain, cycle, repeat # lint-amnesty, pylint: disable=wrong-import-order -from smtplib import SMTPAuthenticationError, SMTPConnectError, SMTPDataError, SMTPServerDisconnected, SMTPSenderRefused # lint-amnesty, pylint: disable=wrong-import-order +from smtplib import ( # lint-amnesty, pylint: disable=wrong-import-order + SMTPAuthenticationError, + SMTPConnectError, + SMTPDataError, + SMTPSenderRefused, + SMTPServerDisconnected +) from unittest.mock import Mock, patch # lint-amnesty, pylint: disable=wrong-import-order from uuid import uuid4 # lint-amnesty, pylint: disable=wrong-import-order + import pytest -from boto.exception import AWSConnectionError -from boto.ses.exceptions import ( - SESAddressBlacklistedError, - SESAddressNotVerifiedError, - SESDailyQuotaExceededError, - SESDomainEndsWithDotError, - SESDomainNotConfirmedError, - SESIdentityNotVerifiedError, - SESIllegalAddressError, - SESLocalAddressCharacterError, - SESMaxSendingRateExceededError -) +from botocore.exceptions import ClientError, EndpointConnectionError from celery.states import FAILURE, SUCCESS +from dateutil.relativedelta import relativedelta from django.conf import settings from django.core.management import call_command from django.test.utils import override_settings @@ -268,19 +264,22 @@ def test_smtp_blacklisted_user(self): def test_ses_blacklisted_user(self): # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESAddressBlacklistedError(554, "Email address is blacklisted")) - def test_ses_illegal_address(self): - # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESIllegalAddressError(554, "Email address is illegal")) + operation_name = '' + parsed_response = {'Error': {'Code': 'MessageRejected', 'Message': 'Error Uploading'}} + self._test_email_address_failures(ClientError(parsed_response, operation_name)) - def test_ses_local_address_character_error(self): + def test_ses_illegal_address(self): # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESLocalAddressCharacterError(554, "Email address contains a bad character")) + operation_name = '' + parsed_response = {'Error': {'Code': 'MailFromDomainNotVerifiedException', 'Message': 'Error Uploading'}} + self._test_email_address_failures(ClientError(parsed_response, operation_name)) def test_ses_domain_ends_with_dot(self): # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESDomainEndsWithDotError(554, "Email address ends with a dot")) + operation_name = '' + parsed_response = {'Error': {'Code': 'MailFromDomainNotVerifiedException', 'Message': 'invalid domain'}} + self._test_email_address_failures(ClientError(parsed_response, operation_name)) def test_bulk_email_skip_with_non_ascii_emails(self): """ @@ -367,12 +366,12 @@ def test_max_retry_after_smtp_connect_error(self): def test_retry_after_aws_connect_error(self): self._test_retry_after_limited_retry_error( - AWSConnectionError("Unable to provide secure connection through proxy") + EndpointConnectionError(endpoint_url="Could not connect to the endpoint URL:") ) def test_max_retry_after_aws_connect_error(self): self._test_max_retry_limit_causes_failure( - AWSConnectionError("Unable to provide secure connection through proxy") + EndpointConnectionError(endpoint_url="Could not connect to the endpoint URL:") ) def test_retry_after_general_error(self): @@ -416,11 +415,6 @@ def test_retry_after_smtp_sender_refused_error(self): SMTPSenderRefused(421, "Throttling: Sending rate exceeded", self.instructor.email) ) - def test_retry_after_ses_throttling_error(self): - self._test_retry_after_unlimited_retry_error( - SESMaxSendingRateExceededError(455, "Throttling: Sending rate exceeded") - ) - def _test_immediate_failure(self, exception): """Test that celery can hit a maximum number of retries.""" # Doesn't really matter how many recipients, since we expect @@ -444,18 +438,6 @@ def _test_immediate_failure(self, exception): def test_failure_on_unhandled_smtp(self): self._test_immediate_failure(SMTPAuthenticationError(403, "That password doesn't work!")) - def test_failure_on_ses_quota_exceeded(self): - self._test_immediate_failure(SESDailyQuotaExceededError(403, "You're done for the day!")) - - def test_failure_on_ses_address_not_verified(self): - self._test_immediate_failure(SESAddressNotVerifiedError(403, "Who *are* you?")) - - def test_failure_on_ses_identity_not_verified(self): - self._test_immediate_failure(SESIdentityNotVerifiedError(403, "May I please see an ID!")) - - def test_failure_on_ses_domain_not_confirmed(self): - self._test_immediate_failure(SESDomainNotConfirmedError(403, "You're out of bounds!")) - def test_bulk_emails_with_unicode_course_image_name(self): # Test bulk email with unicode characters in course image name course_image = '在淡水測試.jpg' diff --git a/lms/djangoapps/course_home_api/dates/views.py b/lms/djangoapps/course_home_api/dates/views.py index 3ca59b6bdce9..6c95f82349d4 100644 --- a/lms/djangoapps/course_home_api/dates/views.py +++ b/lms/djangoapps/course_home_api/dates/views.py @@ -13,9 +13,10 @@ from common.djangoapps.student.models import CourseEnrollment from lms.djangoapps.course_goals.models import UserActivity from lms.djangoapps.course_home_api.dates.serializers import DatesTabSerializer +from lms.djangoapps.course_home_api.utils import get_course_or_403 from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.context_processor import user_timezone_locale_prefs -from lms.djangoapps.courseware.courses import get_course_date_blocks, get_course_with_access +from lms.djangoapps.courseware.courses import get_course_date_blocks from lms.djangoapps.courseware.date_summary import TodaysDate from lms.djangoapps.courseware.masquerade import setup_masquerade from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser @@ -59,6 +60,7 @@ class DatesTabView(RetrieveAPIView): * 200 on success with above fields. * 401 if the user is not authenticated. + * 403 if the user does not have access to the course. * 404 if the course is not available or cannot be seen. """ @@ -79,7 +81,7 @@ def get(self, request, *args, **kwargs): monitoring_utils.set_custom_attribute('user_id', request.user.id) monitoring_utils.set_custom_attribute('is_staff', request.user.is_staff) - course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=False) + course = get_course_or_403(request.user, 'load', course_key, check_if_enrolled=False) is_staff = bool(has_access(request.user, 'staff', course_key)) _, request.user = setup_masquerade( diff --git a/lms/djangoapps/course_home_api/outline/serializers.py b/lms/djangoapps/course_home_api/outline/serializers.py index 20b205e3040d..29b46d30e967 100644 --- a/lms/djangoapps/course_home_api/outline/serializers.py +++ b/lms/djangoapps/course_home_api/outline/serializers.py @@ -109,6 +109,16 @@ class ResumeCourseSerializer(serializers.Serializer): url = serializers.URLField() +class OutlineTabCourseAccessRedirectSerializer(serializers.Serializer): + """ + Serializer for a Course Access Redirect response from the outline tab + """ + url = serializers.URLField() + error_code = serializers.CharField(source='access_error.error_code') + developer_message = serializers.CharField(source='access_error.developer_message') + user_message = serializers.CharField(source='access_error.user_message') + + class OutlineTabSerializer(DatesBannerSerializer, VerifiedModeSerializer): """ Serializer for the Outline Tab diff --git a/lms/djangoapps/course_home_api/outline/views.py b/lms/djangoapps/course_home_api/outline/views.py index 253779005e91..7650c2ae5320 100644 --- a/lms/djangoapps/course_home_api/outline/views.py +++ b/lms/djangoapps/course_home_api/outline/views.py @@ -27,10 +27,13 @@ get_course_goal, ) from lms.djangoapps.course_goals.models import CourseGoal -from lms.djangoapps.course_home_api.outline.serializers import OutlineTabSerializer +from lms.djangoapps.course_home_api.outline.serializers import ( + OutlineTabSerializer, +) +from lms.djangoapps.course_home_api.utils import get_course_or_403 from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.context_processor import user_timezone_locale_prefs -from lms.djangoapps.courseware.courses import get_course_date_blocks, get_course_info_section, get_course_with_access +from lms.djangoapps.courseware.courses import get_course_date_blocks, get_course_info_section from lms.djangoapps.courseware.date_summary import TodaysDate from lms.djangoapps.courseware.masquerade import is_masquerading, setup_masquerade from lms.djangoapps.courseware.views.views import get_cert_data @@ -76,7 +79,9 @@ class OutlineTabView(RetrieveAPIView): **Response Values** - Body consists of the following fields: + Body consists of two possible shapes. + + For a good 200 response, the response will include: access_expiration: An object detailing when access to this course will expire expiration_date: (str) When the access expires, in ISO 8601 notation @@ -143,9 +148,19 @@ class OutlineTabView(RetrieveAPIView): welcome_message_html: (str) Raw HTML for the course updates banner user_has_passing_grade: (bool) Whether the user currently is passing the course + If the learner does not have access to the course for a specific reason and should be redirected this + view will return a 403 and the following data: + + url: (str) The URL to which the user should be redirected + error_code: (str) A system identifier for the reason the user is being redirected + developer_message: (str) A message explaining why the user is being redirected, + intended for developer debugging. + user_message: (str) A message explaining why the user is being redirected, intended to be shown to the user. + **Returns** - * 200 on success with above fields. + * 200 on success. + * 403 if the user does not currently have access to the course and should be redirected. * 404 if the course is not available or cannot be seen. """ @@ -167,7 +182,7 @@ def get(self, request, *args, **kwargs): # pylint: disable=too-many-statements monitoring_utils.set_custom_attribute('user_id', request.user.id) monitoring_utils.set_custom_attribute('is_staff', request.user.is_staff) - course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=False) + course = get_course_or_403(request.user, 'load', course_key, check_if_enrolled=False) masquerade_object, request.user = setup_masquerade( request, @@ -365,7 +380,7 @@ def dismiss_welcome_message(request): # pylint: disable=missing-function-docstr try: course_key = CourseKey.from_string(course_id) - course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) + course = get_course_or_403(request.user, 'load', course_key, check_if_enrolled=True) dismiss_current_update_for_user(request, course) return Response({'message': _('Welcome message successfully dismissed.')}) except Exception: diff --git a/lms/djangoapps/course_home_api/progress/views.py b/lms/djangoapps/course_home_api/progress/views.py index fd96ff6c08a5..8c21335dcacf 100644 --- a/lms/djangoapps/course_home_api/progress/views.py +++ b/lms/djangoapps/course_home_api/progress/views.py @@ -21,8 +21,9 @@ from lms.djangoapps.course_blocks.transformers import start_date from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException +from lms.djangoapps.course_home_api.utils import get_course_or_403 from lms.djangoapps.courseware.courses import ( - get_course_blocks_completion_summary, get_course_with_access, get_studio_url, + get_course_blocks_completion_summary, get_studio_url, ) from lms.djangoapps.courseware.masquerade import setup_masquerade from lms.djangoapps.courseware.views.views import credit_course_requirements, get_cert_data @@ -128,6 +129,7 @@ class ProgressTabView(RetrieveAPIView): * 200 on success with above fields. * 401 if the user is not authenticated or not enrolled. + * 403 if the user does not have access to the course. * 404 if the course is not available or cannot be seen. """ @@ -190,7 +192,7 @@ def get(self, request, *args, **kwargs): student = self._get_student_user(request, course_key, student_id, is_staff) username = get_enterprise_learner_generic_name(request) or student.username - course = get_course_with_access(student, 'load', course_key, check_if_enrolled=False) + course = get_course_or_403(student, 'load', course_key, check_if_enrolled=False) course_overview = CourseOverview.get_from_id(course_key) enrollment = CourseEnrollment.get_enrollment(student, course_key) diff --git a/lms/djangoapps/course_home_api/tests/test_utils.py b/lms/djangoapps/course_home_api/tests/test_utils.py new file mode 100644 index 000000000000..1d665a6b376a --- /dev/null +++ b/lms/djangoapps/course_home_api/tests/test_utils.py @@ -0,0 +1,53 @@ +""" Tests for course home api utils """ + +from contextlib import contextmanager +from rest_framework.exceptions import PermissionDenied +from unittest import mock + +from lms.djangoapps.course_home_api.utils import get_course_or_403 +from lms.djangoapps.courseware.access_response import AccessError +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + + +class GetCourseOr403Test(ModuleStoreTestCase): + """ Tests for get_course_or_403 """ + + @contextmanager + def mock_get_course(self, *args, **kwargs): + """ Mock base_get_course_with_access helper """ + with mock.patch( + 'lms.djangoapps.course_home_api.utils.base_get_course_with_access', + *args, + **kwargs + ) as mock_get: + yield mock_get + + def test_no_exception(self): + """ If no exception is raised we should return the return of get_course_with_access """ + expected_return = mock.Mock() + with self.mock_get_course(return_value=expected_return): + assert get_course_or_403() == expected_return + + def test_redirect(self): + """ Test for behavior when get_course_with_access raises a redirect error """ + expected_url = "www.testError.access/redirect.php?work=yes" + mock_access_error = AccessError('code', 'dev_msg', 'usr_msg') + mock_course_access_redirect = CourseAccessRedirect(expected_url, mock_access_error) + + with self.mock_get_course(side_effect=mock_course_access_redirect): + try: + get_course_or_403() + self.fail('Call to get_course_or_403 should raise exception') + except PermissionDenied as e: + assert str(e.detail) == mock_access_error.user_message + assert e.detail.code == mock_access_error.error_code + + def test_other_exception(self): + """ Any other exception should not be caught """ + class MyException(Exception): + pass + + with self.mock_get_course(side_effect=MyException()): + with self.assertRaises(MyException): + get_course_or_403() diff --git a/lms/djangoapps/course_home_api/utils.py b/lms/djangoapps/course_home_api/utils.py new file mode 100644 index 000000000000..c215061db0e4 --- /dev/null +++ b/lms/djangoapps/course_home_api/utils.py @@ -0,0 +1,24 @@ +""" Utilities for views in the course home api""" +from rest_framework.exceptions import PermissionDenied + +from lms.djangoapps.courseware.courses import get_course_with_access as base_get_course_with_access +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect + + +def get_course_or_403(*args, **kwargs): + """ + When we make requests to the various Learner Home API endpoints, we do not want to return the actual redirects, + Instead we should return an error code. The redirect info is returned from the course metadata endpoint and the + URL can be followed by whatever client is calling. + + Raises: + - 404 if course is not found + - 403 if the requesting user does not have access to the course + """ + try: + return base_get_course_with_access(*args, **kwargs) + except CourseAccessRedirect as e: + raise PermissionDenied( + detail=e.access_error.user_message, + code=e.access_error.error_code + ) from e diff --git a/lms/envs/common.py b/lms/envs/common.py index 9cb75c7c4f5c..4d1b2a876ff9 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -54,6 +54,7 @@ ENTERPRISE_CATALOG_ADMIN_ROLE, ENTERPRISE_DASHBOARD_ADMIN_ROLE, ENTERPRISE_ENROLLMENT_API_ADMIN_ROLE, + ENTERPRISE_FULFILLMENT_OPERATOR_ROLE, ENTERPRISE_REPORTING_CONFIG_ADMIN_ROLE, ENTERPRISE_OPERATOR_ROLE ) @@ -4633,6 +4634,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring ENTERPRISE_CATALOG_ADMIN_ROLE, ENTERPRISE_ENROLLMENT_API_ADMIN_ROLE, ENTERPRISE_REPORTING_CONFIG_ADMIN_ROLE, + ENTERPRISE_FULFILLMENT_OPERATOR_ROLE, ], } diff --git a/openedx/core/djangoapps/user_authn/api/tests/test_data.py b/openedx/core/djangoapps/user_authn/api/tests/test_data.py new file mode 100644 index 000000000000..05d0f7cf6b8e --- /dev/null +++ b/openedx/core/djangoapps/user_authn/api/tests/test_data.py @@ -0,0 +1,141 @@ +""" Mocked data for testing """ + +mfe_context_data_keys = { + 'contextData', + 'registrationFields', + 'optionalFields' +} + +mock_mfe_context_data = { + 'context_data': { + 'currentProvider': 'edX', + 'platformName': 'edX', + 'providers': [ + { + 'id': 'oa2-facebook', + 'name': 'Facebook', + 'iconClass': 'fa-facebook', + 'iconImage': None, + 'skipHintedLogin': False, + 'skipRegistrationForm': False, + 'loginUrl': 'https://facebook.com/login', + 'registerUrl': 'https://facebook.com/register' + }, + { + 'id': 'oa2-google-oauth2', + 'name': 'Google', + 'iconClass': 'fa-google-plus', + 'iconImage': None, + 'skipHintedLogin': False, + 'skipRegistrationForm': False, + 'loginUrl': 'https://google.com/login', + 'registerUrl': 'https://google.com/register' + } + ], + 'secondaryProviders': [], + 'finishAuthUrl': 'https://edx.com/auth/finish', + 'errorMessage': None, + 'registerFormSubmitButtonText': 'Create Account', + 'autoSubmitRegForm': False, + 'syncLearnerProfileData': False, + 'countryCode': '', + 'pipeline_user_details': { + 'username': 'test123', + 'email': 'test123@edx.com', + 'fullname': 'Test Test', + 'first_name': 'Test', + 'last_name': 'Test' + } + }, + 'registration_fields': {}, + 'optional_fields': { + 'extended_profile': [] + } +} + +mock_default_mfe_context_data = { + 'context_data': { + 'currentProvider': None, + 'platformName': 'édX', + 'providers': [], + 'secondaryProviders': [], + 'finishAuthUrl': None, + 'errorMessage': None, + 'registerFormSubmitButtonText': 'Create Account', + 'autoSubmitRegForm': False, + 'syncLearnerProfileData': False, + 'countryCode': '', + 'pipeline_user_details': {} + }, + 'registration_fields': {}, + 'optional_fields': { + 'extended_profile': [] + } +} + +expected_mfe_context_data = { + 'contextData': { + 'currentProvider': 'edX', + 'platformName': 'edX', + 'providers': [ + { + 'id': 'oa2-facebook', + 'name': 'Facebook', + 'iconClass': 'fa-facebook', + 'iconImage': None, + 'skipHintedLogin': False, + 'skipRegistrationForm': False, + 'loginUrl': 'https://facebook.com/login', + 'registerUrl': 'https://facebook.com/register' + }, + { + 'id': 'oa2-google-oauth2', + 'name': 'Google', + 'iconClass': 'fa-google-plus', + 'iconImage': None, + 'skipHintedLogin': False, + 'skipRegistrationForm': False, + 'loginUrl': 'https://google.com/login', + 'registerUrl': 'https://google.com/register' + } + ], + 'secondaryProviders': [], + 'finishAuthUrl': 'https://edx.com/auth/finish', + 'errorMessage': None, + 'registerFormSubmitButtonText': 'Create Account', + 'autoSubmitRegForm': False, + 'syncLearnerProfileData': False, + 'countryCode': '', + 'pipelineUserDetails': { + 'username': 'test123', + 'email': 'test123@edx.com', + 'name': 'Test Test', + 'firstName': 'Test', + 'lastName': 'Test' + } + }, + 'registrationFields': {}, + 'optionalFields': { + 'extended_profile': [] + } +} + +default_expected_mfe_context_data = { + 'contextData': { + 'currentProvider': None, + 'platformName': 'édX', + 'providers': [], + 'secondaryProviders': [], + 'finishAuthUrl': None, + 'errorMessage': None, + 'registerFormSubmitButtonText': 'Create Account', + 'autoSubmitRegForm': False, + 'syncLearnerProfileData': False, + 'countryCode': '', + 'pipelineUserDetails': {} + }, + 'registrationFields': {}, + 'optionalFields': { + 'extended_profile': [] + } +} diff --git a/openedx/core/djangoapps/user_authn/api/tests/test_serializers.py b/openedx/core/djangoapps/user_authn/api/tests/test_serializers.py index 54e9a9c96c27..348292b172f0 100644 --- a/openedx/core/djangoapps/user_authn/api/tests/test_serializers.py +++ b/openedx/core/djangoapps/user_authn/api/tests/test_serializers.py @@ -2,6 +2,12 @@ from django.test import TestCase +from openedx.core.djangoapps.user_authn.api.tests.test_data import ( + mock_mfe_context_data, + expected_mfe_context_data, + mock_default_mfe_context_data, + default_expected_mfe_context_data, +) from openedx.core.djangoapps.user_authn.serializers import MFEContextSerializer @@ -10,128 +16,29 @@ class TestMFEContextSerializer(TestCase): High-level unit tests for MFEContextSerializer """ - @staticmethod - def get_mock_mfe_context_data(): - """ - Helper function to generate mock data for the MFE Context API view. - """ - - mock_context_data = { - 'context_data': { - 'currentProvider': 'edX', - 'platformName': 'edX', - 'providers': [ - { - 'id': 'oa2-facebook', - 'name': 'Facebook', - 'iconClass': 'fa-facebook', - 'iconImage': None, - 'skipHintedLogin': False, - 'skipRegistrationForm': False, - 'loginUrl': 'https://facebook.com/login', - 'registerUrl': 'https://facebook.com/register' - }, - { - 'id': 'oa2-google-oauth2', - 'name': 'Google', - 'iconClass': 'fa-google-plus', - 'iconImage': None, - 'skipHintedLogin': False, - 'skipRegistrationForm': False, - 'loginUrl': 'https://google.com/login', - 'registerUrl': 'https://google.com/register' - } - ], - 'secondaryProviders': [], - 'finishAuthUrl': 'https://edx.com/auth/finish', - 'errorMessage': None, - 'registerFormSubmitButtonText': 'Create Account', - 'autoSubmitRegForm': False, - 'syncLearnerProfileData': False, - 'countryCode': '', - 'pipeline_user_details': { - 'username': 'test123', - 'email': 'test123@edx.com', - 'fullname': 'Test Test', - 'first_name': 'Test', - 'last_name': 'Test' - } - }, - 'registration_fields': {}, - 'optional_fields': { - 'extended_profile': [] - } - } - - return mock_context_data - - @staticmethod - def get_expected_data(): - """ - Helper function to generate expected data for the MFE Context API view serializer. - """ - - expected_data = { - 'contextData': { - 'currentProvider': 'edX', - 'platformName': 'edX', - 'providers': [ - { - 'id': 'oa2-facebook', - 'name': 'Facebook', - 'iconClass': 'fa-facebook', - 'iconImage': None, - 'skipHintedLogin': False, - 'skipRegistrationForm': False, - 'loginUrl': 'https://facebook.com/login', - 'registerUrl': 'https://facebook.com/register' - }, - { - 'id': 'oa2-google-oauth2', - 'name': 'Google', - 'iconClass': 'fa-google-plus', - 'iconImage': None, - 'skipHintedLogin': False, - 'skipRegistrationForm': False, - 'loginUrl': 'https://google.com/login', - 'registerUrl': 'https://google.com/register' - } - ], - 'secondaryProviders': [], - 'finishAuthUrl': 'https://edx.com/auth/finish', - 'errorMessage': None, - 'registerFormSubmitButtonText': 'Create Account', - 'autoSubmitRegForm': False, - 'syncLearnerProfileData': False, - 'countryCode': '', - 'pipelineUserDetails': { - 'username': 'test123', - 'email': 'test123@edx.com', - 'name': 'Test Test', - 'firstName': 'Test', - 'lastName': 'Test' - } - }, - 'registrationFields': {}, - 'optionalFields': { - 'extended_profile': [] - } - } - - return expected_data - def test_mfe_context_serializer(self): """ Test MFEContextSerializer with mock data that serializes data correctly """ - mfe_context_data = self.get_mock_mfe_context_data() - expected_data = self.get_expected_data() output_data = MFEContextSerializer( - mfe_context_data + mock_mfe_context_data ).data self.assertDictEqual( output_data, - expected_data + expected_mfe_context_data + ) + + def test_mfe_context_serializer_default_response(self): + """ + Test MFEContextSerializer with default data + """ + serialized_data = MFEContextSerializer( + mock_default_mfe_context_data + ).data + + self.assertDictEqual( + serialized_data, + default_expected_mfe_context_data ) diff --git a/openedx/core/djangoapps/user_authn/api/tests/test_views.py b/openedx/core/djangoapps/user_authn/api/tests/test_views.py index 75111a792f1b..d8ab2c37c1a1 100644 --- a/openedx/core/djangoapps/user_authn/api/tests/test_views.py +++ b/openedx/core/djangoapps/user_authn/api/tests/test_views.py @@ -16,9 +16,10 @@ from common.djangoapps.student.tests.factories import UserFactory from common.djangoapps.third_party_auth import pipeline from common.djangoapps.third_party_auth.tests.testutil import ThirdPartyAuthTestMixin, simulate_running_pipeline -from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration from openedx.core.djangoapps.geoinfo.api import country_code_from_ip +from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration from openedx.core.djangoapps.user_api.tests.test_views import UserAPITestCase +from openedx.core.djangoapps.user_authn.api.tests.test_data import mfe_context_data_keys from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -351,6 +352,32 @@ def test_response_structure(self): response = self.client.get(self.url, self.query_params) assert response.data == self.get_context() + def test_mfe_context_api_serialized_response(self): + """ + Test MFE Context API serialized response + """ + response = self.client.get(self.url, self.query_params) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + params = { + 'next': self.query_params['next'] + } + + self.assertEqual( + response.data, + self.get_context(params) + ) + + def test_mfe_context_api_response_keys(self): + """ + Test MFE Context API response keys + """ + response = self.client.get(self.url, self.query_params) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + response_keys = set(response.data.keys()) + self.assertSetEqual(response_keys, mfe_context_data_keys) + @skip_unless_lms class SendAccountActivationEmail(UserAPITestCase): diff --git a/requirements/constraints.txt b/requirements/constraints.txt index ed4b01da010a..ae580a05e947 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -25,7 +25,7 @@ django-storages<1.9 # The team that owns this package will manually bump this package rather than having it pulled in automatically. # This is to allow them to better control its deployment and to do it in a process that works better # for them. -edx-enterprise==3.63.0 +edx-enterprise==3.65.1 # oauthlib>3.0.1 causes test failures ( also remove the django-oauth-toolkit constraint when this is fixed ) oauthlib==3.0.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index e9ad77fd973a..c5e6ca2605d8 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -483,7 +483,7 @@ edx-drf-extensions==8.8.0 # edx-when # edxval # learner-pathway-progress -edx-enterprise==3.63.0 +edx-enterprise==3.65.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.in @@ -779,7 +779,7 @@ openedx-events==8.0.0 # -r requirements/edx/base.in # edx-event-bus-kafka # edx-event-bus-redis -openedx-filters==1.2.0 +openedx-filters==1.3.0 # via # -r requirements/edx/base.in # lti-consumer-xblock diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index f42d50d7870c..5283bd176573 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -607,7 +607,7 @@ edx-drf-extensions==8.8.0 # edx-when # edxval # learner-pathway-progress -edx-enterprise==3.63.0 +edx-enterprise==3.65.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/testing.txt @@ -1037,7 +1037,7 @@ openedx-events==8.0.0 # -r requirements/edx/testing.txt # edx-event-bus-kafka # edx-event-bus-redis -openedx-filters==1.2.0 +openedx-filters==1.3.0 # via # -r requirements/edx/testing.txt # lti-consumer-xblock diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 2f0a79d3dfa1..464463ac943d 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -585,7 +585,7 @@ edx-drf-extensions==8.8.0 # edx-when # edxval # learner-pathway-progress -edx-enterprise==3.63.0 +edx-enterprise==3.65.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt @@ -984,7 +984,7 @@ openedx-events==8.0.0 # -r requirements/edx/base.txt # edx-event-bus-kafka # edx-event-bus-redis -openedx-filters==1.2.0 +openedx-filters==1.3.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock diff --git a/xmodule/annotatable_block.py b/xmodule/annotatable_block.py index 774ad900979b..9ec6bf21dace 100644 --- a/xmodule/annotatable_block.py +++ b/xmodule/annotatable_block.py @@ -4,7 +4,7 @@ import textwrap from lxml import etree -from pkg_resources import resource_string +from pkg_resources import resource_filename from web_fragments.fragment import Fragment from xblock.core import XBlock from xblock.fields import Scope, String @@ -75,28 +75,28 @@ class AnnotatableBlock( preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/html/display.js'), - resource_string(__name__, 'js/src/annotatable/display.js'), - resource_string(__name__, 'js/src/javascript_loader.js'), - resource_string(__name__, 'js/src/collapsible.js'), + resource_filename(__name__, 'js/src/html/display.js'), + resource_filename(__name__, 'js/src/annotatable/display.js'), + resource_filename(__name__, 'js/src/javascript_loader.js'), + resource_filename(__name__, 'js/src/collapsible.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [ - resource_string(__name__, 'css/annotatable/display.scss'), + resource_filename(__name__, 'css/annotatable/display.scss'), ], } studio_view_js = { 'js': [ - resource_string(__name__, 'js/src/raw/edit/xml.js'), + resource_filename(__name__, 'js/src/raw/edit/xml.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [ - resource_string(__name__, 'css/codemirror/codemirror.scss'), + resource_filename(__name__, 'css/codemirror/codemirror.scss'), ], } studio_js_module_name = "XMLEditingDescriptor" diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 3c02053c0eb0..2b2ec8082534 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -19,7 +19,7 @@ from django.utils.encoding import smart_str from django.utils.functional import cached_property from lxml import etree -from pkg_resources import resource_string +from pkg_resources import resource_filename from pytz import utc from web_fragments.fragment import Fragment from xblock.core import XBlock @@ -168,32 +168,32 @@ class ProblemBlock( preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/javascript_loader.js'), - resource_string(__name__, 'js/src/capa/display.js'), - resource_string(__name__, 'js/src/collapsible.js'), - resource_string(__name__, 'js/src/capa/imageinput.js'), - resource_string(__name__, 'js/src/capa/schematic.js'), + resource_filename(__name__, 'js/src/javascript_loader.js'), + resource_filename(__name__, 'js/src/capa/display.js'), + resource_filename(__name__, 'js/src/collapsible.js'), + resource_filename(__name__, 'js/src/capa/imageinput.js'), + resource_filename(__name__, 'js/src/capa/schematic.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js') + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js') } preview_view_css = { 'scss': [ - resource_string(__name__, 'css/capa/display.scss'), + resource_filename(__name__, 'css/capa/display.scss'), ], } studio_view_js = { 'js': [ - resource_string(__name__, 'js/src/problem/edit.js'), + resource_filename(__name__, 'js/src/problem/edit.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [ - resource_string(__name__, 'css/editor/edit.scss'), - resource_string(__name__, 'css/problem/edit.scss'), + resource_filename(__name__, 'css/editor/edit.scss'), + resource_filename(__name__, 'css/problem/edit.scss'), ] } diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 91e7af2c156e..7fe545d1a307 100644 --- a/xmodule/conditional_block.py +++ b/xmodule/conditional_block.py @@ -9,7 +9,7 @@ from lazy import lazy from lxml import etree from opaque_keys.edx.locator import BlockUsageLocator -from pkg_resources import resource_string +from pkg_resources import resource_filename from web_fragments.fragment import Fragment from xblock.core import XBlock from xblock.fields import ReferenceList, Scope, String @@ -148,11 +148,11 @@ class ConditionalBlock( preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/conditional/display.js'), - resource_string(__name__, 'js/src/javascript_loader.js'), - resource_string(__name__, 'js/src/collapsible.js'), + resource_filename(__name__, 'js/src/conditional/display.js'), + resource_filename(__name__, 'js/src/javascript_loader.js'), + resource_filename(__name__, 'js/src/collapsible.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [], @@ -161,8 +161,8 @@ class ConditionalBlock( mako_template = 'widgets/metadata-edit.html' studio_js_module_name = 'SequenceDescriptor' studio_view_js = { - 'js': [resource_string(__name__, 'js/src/sequence/edit.js')], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'js': [resource_filename(__name__, 'js/src/sequence/edit.js')], + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [], diff --git a/xmodule/html_block.py b/xmodule/html_block.py index ed5d84e395c0..48786c2c5611 100644 --- a/xmodule/html_block.py +++ b/xmodule/html_block.py @@ -8,7 +8,7 @@ import textwrap from datetime import datetime -from pkg_resources import resource_string +from pkg_resources import resource_filename from django.conf import settings from fs.errors import ResourceNotFound @@ -144,15 +144,15 @@ def studio_view(self, _context): preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/html/display.js'), - resource_string(__name__, 'js/src/javascript_loader.js'), - resource_string(__name__, 'js/src/collapsible.js'), - resource_string(__name__, 'js/src/html/imageModal.js'), - resource_string(__name__, 'js/common_static/js/vendor/draggabilly.js'), + resource_filename(__name__, 'js/src/html/display.js'), + resource_filename(__name__, 'js/src/javascript_loader.js'), + resource_filename(__name__, 'js/src/collapsible.js'), + resource_filename(__name__, 'js/src/html/imageModal.js'), + resource_filename(__name__, 'js/common_static/js/vendor/draggabilly.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } - preview_view_css = {'scss': [resource_string(__name__, 'css/html/display.scss')]} + preview_view_css = {'scss': [resource_filename(__name__, 'css/html/display.scss')]} uses_xmodule_styles_setup = True @@ -164,14 +164,14 @@ def studio_view(self, _context): studio_view_js = { 'js': [ - resource_string(__name__, 'js/src/html/edit.js') + resource_filename(__name__, 'js/src/html/edit.js') ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [ - resource_string(__name__, 'css/editor/edit.scss'), - resource_string(__name__, 'css/html/edit.scss') + resource_filename(__name__, 'css/editor/edit.scss'), + resource_filename(__name__, 'css/html/edit.scss') ] } diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index 04abe5de6eb3..7193247f58d5 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -17,7 +17,7 @@ from lxml import etree from lxml.etree import XMLSyntaxError from opaque_keys.edx.locator import LibraryLocator -from pkg_resources import resource_string +from pkg_resources import resource_filename from web_fragments.fragment import Fragment from webob import Response from xblock.completable import XBlockCompletionMode @@ -97,7 +97,7 @@ class LibraryContentBlock( preview_view_js = { 'js': [], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [], @@ -107,9 +107,9 @@ class LibraryContentBlock( studio_js_module_name = "VerticalDescriptor" studio_view_js = { 'js': [ - resource_string(__name__, 'js/src/vertical/edit.js'), + resource_filename(__name__, 'js/src/vertical/edit.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [], diff --git a/xmodule/lti_block.py b/xmodule/lti_block.py index 8bf60f1c4486..76c03d73a671 100644 --- a/xmodule/lti_block.py +++ b/xmodule/lti_block.py @@ -68,7 +68,7 @@ from django.conf import settings from lxml import etree from oauthlib.oauth1.rfc5849 import signature -from pkg_resources import resource_string +from pkg_resources import resource_filename from pytz import UTC from webob import Response from web_fragments.fragment import Fragment @@ -374,13 +374,13 @@ class LTIBlock( preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/lti/lti.js') + resource_filename(__name__, 'js/src/lti/lti.js') ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [ - resource_string(__name__, 'css/lti/lti.scss') + resource_filename(__name__, 'css/lti/lti.scss') ], } @@ -389,9 +389,9 @@ class LTIBlock( studio_js_module_name = 'MetadataOnlyEditingDescriptor' studio_view_js = { 'js': [ - resource_string(__name__, 'js/src/raw/edit/metadata-only.js') + resource_filename(__name__, 'js/src/raw/edit/metadata-only.js') ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [], diff --git a/xmodule/modulestore/split_mongo/split.py b/xmodule/modulestore/split_mongo/split.py index e76f6c81204d..f72f950a5405 100644 --- a/xmodule/modulestore/split_mongo/split.py +++ b/xmodule/modulestore/split_mongo/split.py @@ -57,7 +57,6 @@ import copy import datetime -import hashlib import logging from collections import defaultdict from importlib import import_module @@ -102,7 +101,7 @@ ) from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, DjangoFlexPersistenceBackend -from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES +from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES, derived_key from xmodule.partitions.partitions_service import PartitionService from xmodule.util.misc import get_library_or_course_attribute @@ -2369,6 +2368,9 @@ def copy_from_template(self, source_keys, dest_usage, user_id, head_validation=T time this method is called for the same source block and dest_usage, the same resulting block id will be generated. + Note also that this function does not override any of the attributes on the destination + block-- it only replaces the destination block's children. + :param source_keys: a list of BlockUsageLocators. Order is preserved. :param dest_usage: The BlockUsageLocator that will become the parent of an inherited copy @@ -2442,7 +2444,6 @@ def _copy_from_template( for usage_key in source_keys: src_course_key = usage_key.course_key - hashable_source_id = src_course_key.for_version(None) block_key = BlockKey(usage_key.block_type, usage_key.block_id) source_structure = source_structures[src_course_key] @@ -2450,15 +2451,7 @@ def _copy_from_template( raise ItemNotFoundError(usage_key) source_block_info = source_structure['blocks'][block_key] - # Compute a new block ID. This new block ID must be consistent when this - # method is called with the same (source_key, dest_structure) pair - unique_data = "{}:{}:{}".format( - str(hashable_source_id).encode("utf-8"), - block_key.id, - new_parent_block_key.id, - ) - new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20] - new_block_key = BlockKey(block_key.type, new_block_id) + new_block_key = derived_key(src_course_key, block_key, new_parent_block_key) # Now clone block_key to new_block_key: new_block_info = copy.deepcopy(source_block_info) diff --git a/xmodule/modulestore/store_utilities.py b/xmodule/modulestore/store_utilities.py index 45082ff43e75..ced5c9728ec0 100644 --- a/xmodule/modulestore/store_utilities.py +++ b/xmodule/modulestore/store_utilities.py @@ -1,5 +1,5 @@ # lint-amnesty, pylint: disable=missing-module-docstring - +import hashlib import logging import re import uuid @@ -7,6 +7,8 @@ from xblock.core import XBlock +from xmodule.modulestore.split_mongo import BlockKey + DETACHED_XBLOCK_TYPES = {name for name, __ in XBlock.load_tagged_classes("detached")} @@ -104,3 +106,31 @@ def get_draft_subtree_roots(draft_nodes): for draft_node in draft_nodes: if draft_node.parent_url not in urls: yield draft_node + + +def derived_key(courselike_source_key, block_key, dest_parent: BlockKey): + """ + Return a new reproducible block ID for a given root, source block, and destination parent. + + When recursively copying a block structure, we need to generate new block IDs for the + blocks. We don't want to use the exact same IDs as we might copy blocks multiple times. + However, we do want to be able to reproduce the same IDs when copying the same block + so that if we ever need to re-copy the block from its source (that is, to update it with + upstream changes) we don't affect any data tied to the ID, such as grades. + + This is used by the copy_from_template function of the modulestore, and can be used by + pluggable django apps that need to copy blocks from one course to another in an + idempotent way. In the future, this should be created into a proper API function + in the spirit of OEP-49. + """ + hashable_source_id = courselike_source_key.for_version(None) + + # Compute a new block ID. This new block ID must be consistent when this + # method is called with the same (source_key, dest_structure) pair + unique_data = "{}:{}:{}".format( + str(hashable_source_id).encode("utf-8"), + block_key.id, + dest_parent.id, + ) + new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20] + return BlockKey(block_key.type, new_block_id) diff --git a/xmodule/modulestore/tests/test_store_utilities.py b/xmodule/modulestore/tests/test_store_utilities.py index 1c1c86f4fcd3..a0b5cfcbdbb6 100644 --- a/xmodule/modulestore/tests/test_store_utilities.py +++ b/xmodule/modulestore/tests/test_store_utilities.py @@ -4,11 +4,15 @@ import unittest +from unittest import TestCase from unittest.mock import Mock import ddt -from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots +from opaque_keys.edx.keys import CourseKey + +from xmodule.modulestore.split_mongo import BlockKey +from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots, derived_key @ddt.ddt @@ -82,3 +86,43 @@ def test_get_draft_subtree_roots(self, node_arguments_list, expected_roots_urls) subtree_roots_urls = [root.url for root in get_draft_subtree_roots(block_nodes)] # check that we return the expected urls assert set(subtree_roots_urls) == set(expected_roots_urls) + + +mock_block = Mock() +mock_block.id = CourseKey.from_string('course-v1:Beeper+B33P+BOOP') + + +derived_key_scenarios = [ + { + 'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), + 'block_key': BlockKey('chapter', 'interactive_demonstrations'), + 'parent': mock_block, + 'expected': BlockKey( + 'chapter', '5793ec64e25ed870a7dd', + ), + }, + { + 'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), + 'block_key': BlockKey('chapter', 'interactive_demonstrations'), + 'parent': BlockKey( + 'chapter', 'thingy', + ), + 'expected': BlockKey( + 'chapter', '599792a5622d85aa41e6', + ), + } +] + + +@ddt.ddt +class TestDerivedKey(TestCase): + """ + Test reproducible block ID generation. + """ + @ddt.data(*derived_key_scenarios) + @ddt.unpack + def test_derived_key(self, courselike_source_key, block_key, parent, expected): + """ + Test that derived_key returns the expected value. + """ + self.assertEqual(derived_key(courselike_source_key, block_key, parent), expected) diff --git a/xmodule/poll_block.py b/xmodule/poll_block.py index b29ed710e922..0ce6d38e1e51 100644 --- a/xmodule/poll_block.py +++ b/xmodule/poll_block.py @@ -13,7 +13,7 @@ from collections import OrderedDict from copy import deepcopy -from pkg_resources import resource_string +from pkg_resources import resource_filename from web_fragments.fragment import Fragment from lxml import etree @@ -86,15 +86,15 @@ class PollBlock( preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/javascript_loader.js'), - resource_string(__name__, 'js/src/poll/poll.js'), - resource_string(__name__, 'js/src/poll/poll_main.js') + resource_filename(__name__, 'js/src/javascript_loader.js'), + resource_filename(__name__, 'js/src/poll/poll.js'), + resource_filename(__name__, 'js/src/poll/poll_main.js') ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [ - resource_string(__name__, 'css/poll/display.scss') + resource_filename(__name__, 'css/poll/display.scss') ], } @@ -102,7 +102,7 @@ class PollBlock( # the static_content command happy. studio_view_js = { 'js': [], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js') + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js') } studio_view_css = { diff --git a/xmodule/seq_block.py b/xmodule/seq_block.py index 0d8be411fd58..250a69d83b6a 100644 --- a/xmodule/seq_block.py +++ b/xmodule/seq_block.py @@ -14,7 +14,7 @@ from lxml import etree from opaque_keys.edx.keys import UsageKey -from pkg_resources import resource_string +from pkg_resources import resource_filename from pytz import UTC from web_fragments.fragment import Fragment from xblock.completable import XBlockCompletionMode @@ -273,14 +273,14 @@ class SequenceBlock( preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/sequence/display.js'), + resource_filename(__name__, 'js/src/sequence/display.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js') + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js') } preview_view_css = { 'scss': [ - resource_string(__name__, 'css/sequence/display.scss'), + resource_filename(__name__, 'css/sequence/display.scss'), ], } @@ -288,7 +288,7 @@ class SequenceBlock( # the static_content command happy. studio_view_js = { 'js': [], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js') + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js') } studio_view_css = { diff --git a/xmodule/split_test_block.py b/xmodule/split_test_block.py index c45848800fb3..b638eb7a50ba 100644 --- a/xmodule/split_test_block.py +++ b/xmodule/split_test_block.py @@ -12,7 +12,7 @@ from django.utils.functional import cached_property from lxml import etree -from pkg_resources import resource_string +from pkg_resources import resource_filename from web_fragments.fragment import Fragment from webob import Response from xblock.core import XBlock @@ -160,7 +160,7 @@ class SplitTestBlock( # lint-amnesty, pylint: disable=abstract-method preview_view_js = { 'js': [], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [], @@ -169,8 +169,8 @@ class SplitTestBlock( # lint-amnesty, pylint: disable=abstract-method mako_template = "widgets/metadata-only-edit.html" studio_js_module_name = 'SequenceDescriptor' studio_view_js = { - 'js': [resource_string(__name__, 'js/src/sequence/edit.js')], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'js': [resource_filename(__name__, 'js/src/sequence/edit.js')], + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [], @@ -409,7 +409,7 @@ def log_child_render(self, request, suffix=''): # lint-amnesty, pylint: disable ) raise else: - self.runtime.publish('xblock.split_test.child_render', {'child_id': child_id}) + self.runtime.publish(self, 'xblock.split_test.child_render', {'child_id': child_id}) return Response() def get_icon_class(self): diff --git a/xmodule/static_content.py b/xmodule/static_content.py index 2447347da90f..58ffcb2de12b 100755 --- a/xmodule/static_content.py +++ b/xmodule/static_content.py @@ -13,7 +13,7 @@ import sys import textwrap from collections import defaultdict -from pkg_resources import resource_string +from pkg_resources import resource_filename import django from docopt import docopt @@ -43,27 +43,27 @@ class VideoBlock(HTMLSnippet): # lint-amnesty, pylint: disable=abstract-method preview_view_js = { 'js': [ - resource_string(__name__, 'js/src/video/10_main.js'), + resource_filename(__name__, 'js/src/video/10_main.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js') + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js') } preview_view_css = { 'scss': [ - resource_string(__name__, 'css/video/display.scss'), - resource_string(__name__, 'css/video/accessible_menu.scss'), + resource_filename(__name__, 'css/video/display.scss'), + resource_filename(__name__, 'css/video/accessible_menu.scss'), ], } studio_view_js = { 'js': [ - resource_string(__name__, 'js/src/tabs/tabs-aggregator.js'), + resource_filename(__name__, 'js/src/tabs/tabs-aggregator.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [ - resource_string(__name__, 'css/tabs/tabs.scss'), + resource_filename(__name__, 'css/tabs/tabs.scss'), ] } @@ -132,7 +132,9 @@ def _write_styles(selector, output_root, classes, css_attribute, suffix): for class_ in classes: class_css = getattr(class_, css_attribute)() for filetype in ('sass', 'scss', 'css'): - for idx, fragment in enumerate(class_css.get(filetype, [])): + for idx, fragment_path in enumerate(class_css.get(filetype, [])): + with open(fragment_path, 'rb') as fragment_file: + fragment = fragment_file.read() css_fragments[idx, filetype, fragment].add(class_.__name__) css_imports = defaultdict(set) for (idx, filetype, fragment), classes in sorted(css_fragments.items()): # lint-amnesty, pylint: disable=redefined-argument-from-local @@ -177,10 +179,14 @@ def _write_js(output_root, classes, js_attribute): fragment_owners = defaultdict(list) for class_ in classes: module_js = getattr(class_, js_attribute)() + with open(module_js.get('xmodule_js'), 'rb') as xmodule_js_file: + xmodule_js_fragment = xmodule_js_file.read() # It will enforce 000 prefix for xmodule.js. - fragment_owners[(0, 'js', module_js.get('xmodule_js'))].append(getattr(class_, js_attribute + '_bundle_name')()) + fragment_owners[(0, 'js', xmodule_js_fragment)].append(getattr(class_, js_attribute + '_bundle_name')()) for filetype in ('coffee', 'js'): - for idx, fragment in enumerate(module_js.get(filetype, [])): + for idx, fragment_path in enumerate(module_js.get(filetype, [])): + with open(fragment_path, 'rb') as fragment_file: + fragment = fragment_file.read() fragment_owners[(idx + 1, filetype, fragment)].append(getattr(class_, js_attribute + '_bundle_name')()) for (idx, filetype, fragment), owners in sorted(fragment_owners.items()): diff --git a/xmodule/template_block.py b/xmodule/template_block.py index 70bafca7753e..71b2c21f1441 100644 --- a/xmodule/template_block.py +++ b/xmodule/template_block.py @@ -6,7 +6,7 @@ from xblock.core import XBlock from lxml import etree -from pkg_resources import resource_string +from pkg_resources import resource_filename from web_fragments.fragment import Fragment from xmodule.editing_block import EditingMixin from xmodule.raw_block import RawMixin @@ -67,17 +67,17 @@ class CustomTagBlock(CustomTagTemplateBlock): # pylint: disable=abstract-method preview_view_js = { 'js': [], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [], } studio_view_js = { - 'js': [resource_string(__name__, 'js/src/raw/edit/xml.js')], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'js': [resource_filename(__name__, 'js/src/raw/edit/xml.js')], + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { - 'scss': [resource_string(__name__, 'css/codemirror/codemirror.scss')], + 'scss': [resource_filename(__name__, 'css/codemirror/codemirror.scss')], } def studio_view(self, _context): diff --git a/xmodule/word_cloud_block.py b/xmodule/word_cloud_block.py index 2e7fadd0a2be..d7d35dedc5f3 100644 --- a/xmodule/word_cloud_block.py +++ b/xmodule/word_cloud_block.py @@ -10,7 +10,7 @@ import json import logging -from pkg_resources import resource_string +from pkg_resources import resource_filename from web_fragments.fragment import Fragment from xblock.core import XBlock @@ -114,21 +114,21 @@ class WordCloudBlock( # pylint: disable=abstract-method preview_view_js = { 'js': [ - resource_string(__name__, 'assets/word_cloud/src/js/word_cloud.js'), + resource_filename(__name__, 'assets/word_cloud/src/js/word_cloud.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } preview_view_css = { 'scss': [ - resource_string(__name__, 'css/word_cloud/display.scss'), + resource_filename(__name__, 'css/word_cloud/display.scss'), ], } studio_view_js = { 'js': [ - resource_string(__name__, 'js/src/raw/edit/metadata-only.js'), + resource_filename(__name__, 'js/src/raw/edit/metadata-only.js'), ], - 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + 'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'), } studio_view_css = { 'scss': [],