From 536493f261a8fbaaafecfec2226f374677c80a49 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Tue, 21 Nov 2023 00:39:35 -0300 Subject: [PATCH 01/13] refactor: inheritable studioeditableblock's callbacks for editing & duplication Solves https://github.com/openedx/edx-platform/issues/33715 Instead of applying duplicated logic after getattr, that logic is incorporated into inheritable methods of the StudioEditable block. Risks: - Assumes all cms blocks that are duplicated inherit from StudioEditableBlock. --- cms/djangoapps/contentstore/utils.py | 76 ++++--------------- cms/djangoapps/contentstore/views/block.py | 4 +- .../contentstore/views/component.py | 4 +- cms/djangoapps/contentstore/views/preview.py | 4 +- .../xblock_storage_handlers/view_handlers.py | 76 +++---------------- xmodule/library_content_block.py | 9 ++- xmodule/services.py | 48 ++++++++++++ xmodule/studio_editable.py | 70 ++++++++++------- xmodule/util/duplicate.py | 15 ++++ 9 files changed, 141 insertions(+), 165 deletions(-) create mode 100644 xmodule/util/duplicate.py diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 8e3f97eee98..d354d012dd3 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -15,7 +15,6 @@ from django.utils import translation from django.utils.translation import gettext as _ from help_tokens.core import HelpUrlExpert -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 @@ -27,9 +26,7 @@ from cms.djangoapps.contentstore.toggles import exam_setting_view_enabled from common.djangoapps.course_action_state.models import CourseRerunUIStateManager from common.djangoapps.course_modes.models import CourseMode -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, STUDIO_EDIT_ROLES from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import ( CourseInstructorRole, @@ -45,7 +42,6 @@ get_namespace_choices, generate_milestone_namespace ) -from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from openedx.core import toggles as core_toggles from openedx.core.djangoapps.credit.api import get_credit_requirements, is_credit_course from openedx.core.djangoapps.discussions.config.waffle import ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND @@ -79,12 +75,12 @@ use_tagging_taxonomy_list_page, ) from cms.djangoapps.models.settings.course_grading import CourseGradingModel -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 +from xmodule.partitions.partitions_service import get_all_partitions_for_course +from xmodule.services import load_services_for_studio +from xmodule.util.duplicate import handle_children_duplication # lint-amnesty, pylint: disable=wrong-import-order log = logging.getLogger(__name__) @@ -1055,23 +1051,17 @@ def duplicate_block( ) children_handled = False - - if hasattr(dest_block, 'studio_post_duplicate'): + 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) + load_services_for_studio(source_item.runtime, user) + children_handled = dest_block.studio_post_duplicate( + store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow + ) + + if not children_handled: + handle_children_duplication( + store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow + ) # pylint: disable=protected-access if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: @@ -1173,25 +1163,6 @@ def gather_block_attributes(source_item, display_name=None, is_child=False): 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 - - def update_course_details(request, course_key, payload, course_block): """ Utils is used to update course details. @@ -1666,24 +1637,3 @@ def get_course_videos_context(course_block, pagination_conf, course_key=None): # Cached state for transcript providers' credentials (org-specific) course_video_context['transcript_credentials'] = get_transcript_credentials_state_for_org(course.id.org) return course_video_context - - -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 91078ccd11c..1ce1630fb76 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -27,7 +27,8 @@ ) from xmodule.modulestore.django import ( modulestore, -) # lint-amnesty, pylint: disable=wrong-import-order +) +from xmodule.services import load_services_for_studio # lint-amnesty, pylint: disable=wrong-import-order from xmodule.x_module import ( @@ -46,7 +47,6 @@ from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import ( handle_xblock, create_xblock_info, - load_services_for_studio, get_block_info, get_xblock, delete_orphans, diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index bafcdad35c7..f06aa8f5017 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -34,14 +34,14 @@ from openedx.core.djangoapps.content_staging import api as content_staging_api from openedx.core.djangoapps.content_tagging.api import get_content_tags 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.modulestore.exceptions import ItemNotFoundError +from xmodule.services import load_services_for_studio # lint-amnesty, pylint: disable=wrong-import-order from ..toggles import use_new_unit_page from ..utils import get_lms_link_for_item, get_sibling_urls, reverse_course_url, get_unit_url from ..helpers import get_parent_xblock, is_unit, xblock_type_display_name from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import ( add_container_page_publishing_info, create_xblock_info, - load_services_for_studio, ) __all__ = [ diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index acab3547181..096de387157 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -21,7 +21,7 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import XBlockI18nService, modulestore from xmodule.partitions.partitions_service import PartitionService -from xmodule.services import SettingsService, TeamsConfigurationService +from xmodule.services import SettingsService, StudioPermissionsService, TeamsConfigurationService from xmodule.studio_editable import has_author_view from xmodule.util.sandboxing import SandboxService from xmodule.util.builtin_assets import add_webpack_js_to_fragment @@ -45,7 +45,7 @@ wrap_xblock_aside ) -from ..utils import get_visibility_partition_info, StudioPermissionsService +from ..utils import get_visibility_partition_info from .access import get_user_role from .session_kv_store import SessionKeyValueStore diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index f82a6f599d1..10b2c85adcf 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -31,7 +31,6 @@ ) from edx_proctoring.exceptions import ProctoredExamNotFoundException from help_tokens.core import HelpUrlExpert -from lti_consumer.models import CourseAllowPIISharingInLTIFlag from opaque_keys.edx.locator import LibraryUsageLocator from pytz import UTC from xblock.core import XBlock @@ -41,7 +40,6 @@ from cms.djangoapps.contentstore.toggles import ENABLE_COPY_PASTE_UNITS, use_tagging_taxonomy_list_page from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.ai_aside_summary_config import AiAsideSummaryConfig -from common.djangoapps.edxmako.services import MakoService from common.djangoapps.static_replace import replace_static_urls from common.djangoapps.student.auth import ( has_studio_read_access, @@ -49,7 +47,6 @@ ) 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 @@ -63,8 +60,9 @@ from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata -from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService +from xmodule.services import load_services_for_studio from xmodule.tabs import CourseTabList +from xmodule.util.duplicate import handle_children_duplication from ..utils import ( ancestor_has_staff_lock, @@ -296,46 +294,6 @@ def modify_xblock(usage_key, request): ) -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 - - def _update_with_callback(xblock, user, old_metadata=None, old_content=None): """ Updates the xblock in the modulestore. @@ -860,29 +818,17 @@ def _duplicate_block( ) 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. - # TODO: Make this a proper method in the base class so we don't need getattr. - # See https://github.com/openedx/edx-platform/issues/33715 - 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) + load_services_for_studio(source_item.runtime, user) + children_handled = dest_block.studio_post_duplicate( + source_item, store, user, duplication_function=_duplicate_block, shallow=False + ) + + if not children_handled: + handle_children_duplication( + dest_block, source_item, store, user, duplication_function=_duplicate_block, shallow=False + ) # pylint: disable=protected-access if "detached" not in source_item.runtime.load_block_type(category)._class_tags: diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index e4c4af267a1..e7542f19a87 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -8,6 +8,7 @@ import random from copy import copy from gettext import ngettext, gettext +from typing import Callable import bleach from django.conf import settings @@ -587,7 +588,11 @@ def children_are_syncing(self, request, suffix=''): # pylint: disable=unused-ar is_updating = False return Response(json.dumps(is_updating)) - def studio_post_duplicate(self, store, source_block): + def studio_post_duplicate( + self, + source_item, + *_, + ) -> None: # pylint: disable=unused-argument """ Used by the studio after basic duplication of a source block. We handle the children ourselves, because we have to properly reference the library upstream and set the overrides. @@ -595,7 +600,7 @@ def studio_post_duplicate(self, store, source_block): Otherwise we'll end up losing data on the next refresh. """ self._validate_sync_permissions() - self.get_tools(to_read_library_content=True).trigger_duplication(source_block=source_block, dest_block=self) + self.get_tools(to_read_library_content=True).trigger_duplication(source_block=source_item, dest_block=self) return True # Children have been handled. def _validate_library_version(self, validation, lib_tools, version, library_key): diff --git a/xmodule/services.py b/xmodule/services.py index 25c680a9653..4835de84d44 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -6,15 +6,23 @@ import inspect import logging from functools import partial +from common.djangoapps.edxmako.services import MakoService +from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from config_models.models import ConfigurationModel from django.conf import settings from eventtracking import tracker from edx_when.field_data import DateLookupFieldData +from lti_consumer.models import CourseAllowPIISharingInLTIFlag from xblock.reference.plugins import Service from xblock.runtime import KvsFieldData +import xmodule from common.djangoapps.track import contexts +from common.djangoapps.student.auth import ( + has_studio_read_access, + has_studio_write_access, +) from lms.djangoapps.courseware.masquerade import is_masquerading_as_specific_student from xmodule.modulestore.django import modulestore @@ -308,3 +316,43 @@ def _handle_deprecated_progress_event(self, block, event): # in order to avoid duplicate work and possibly conflicting semantics. if not getattr(block, 'has_custom_completion', False): self.completion_service.submit_completion(block.scope_ids.usage_id, 1.0) + + +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": xmodule.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/xmodule/studio_editable.py b/xmodule/studio_editable.py index 332025619eb..08f91d7d3da 100644 --- a/xmodule/studio_editable.py +++ b/xmodule/studio_editable.py @@ -1,7 +1,11 @@ """ Mixin to support editing in Studio. """ +from typing import Callable + from xblock.core import XBlock, XBlockMixin +from xmodule.util.duplicate import handle_children_duplication + from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW @@ -49,35 +53,43 @@ def get_preview_view_name(block): """ return AUTHOR_VIEW if has_author_view(block) else STUDENT_VIEW - # Some parts of the code use getattr to dynamically check for the following three methods on subclasses. - # We'd like to refactor so that we can actually declare them here as overridable methods. - # For now, we leave them here as documentation. - # See https://github.com/openedx/edx-platform/issues/33715. - # - # def editor_saved(self, old_metadata, old_content) -> None: # pylint: disable=unused-argument - # """ - # Called right *before* the block is written to the DB. Can be used, e.g., to modify fields before saving. - # - # By default, is a no-op. Can be overriden in subclasses. - # """ - # - # def post_editor_saved(self, old_metadata, old_content) -> None: # pylint: disable=unused-argument - # """ - # Called right *after* the block is written to the DB. Can be used, e.g., to spin up followup tasks. - # - # By default, is a no-op. Can be overriden in subclasses. - # """ - # - # def studio_post_duplicate(self, dest_block) -> bool: # pylint: disable=unused-argument - # """ - # Called when a the block is duplicated. Can be used, e.g., for special handling of child duplication. - # - # Returns 'True' if children have been handled and thus shouldn't be handled by the standard - # duplication logic. - # - # By default, is a no-op. Can be overriden in subclasses. - # """ - # return False + def editor_saved(self, old_metadata, old_content) -> None: # pylint: disable=unused-argument + """ + Called right *before* the block is written to the DB. Can be used, e.g., to modify fields before saving. + + By default, is a no-op. Can be overriden in subclasses. + """ + + def post_editor_saved(self, old_metadata, old_content) -> None: # pylint: disable=unused-argument + """ + Called right *after* the block is written to the DB. Can be used, e.g., to spin up followup tasks. + + By default, is a no-op. Can be overriden in subclasses. + """ + + def studio_post_duplicate( + self, + source_item, + store, + user, + duplication_function: Callable[..., None], + shallow: bool, + ) -> None: # pylint: disable=unused-argument + """ + Called when a the block is duplicated. Can be used, e.g., for special handling of child duplication. + + Returns 'True' if children have been handled and thus shouldn't be handled by the standard + duplication logic. + + By default, implements standard duplication logic. + """ + self.handle_children_duplication(source_item, store, user, duplication_function, shallow) + return True + + def handle_children_duplication( + self, source_item, store, user, duplication_function: Callable[..., None], shallow: bool + ): + handle_children_duplication(self, source_item, store, user, duplication_function, shallow) def has_author_view(block): diff --git a/xmodule/util/duplicate.py b/xmodule/util/duplicate.py new file mode 100644 index 00000000000..249a4404434 --- /dev/null +++ b/xmodule/util/duplicate.py @@ -0,0 +1,15 @@ +from typing import Callable + + +def handle_children_duplication( + xblock, source_item, store, user, duplication_function: Callable[..., None], shallow: bool +): + if not source_item.has_children or shallow: + return + + xblock.children = xblock.children or [] + for child in source_item.children: + dupe = duplication_function(xblock.location, child, user=user, is_child=True) + if dupe not in xblock.children: # duplicate_fun may add the child for us. + xblock.children.append(dupe) + store.update_item(xblock, user.id) From 5901e25eaf97349006166c550c23d3de7e1f7d0a Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Fri, 24 Nov 2023 21:44:55 -0300 Subject: [PATCH 02/13] refactor: remove hasattr and add editable studio to cms xblock modules --- cms/djangoapps/contentstore/utils.py | 11 +++++------ .../xblock_storage_handlers/view_handlers.py | 11 +++++------ cms/envs/common.py | 2 ++ 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index d354d012dd3..885e7848f04 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1051,12 +1051,11 @@ def duplicate_block( ) 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. - load_services_for_studio(source_item.runtime, user) - children_handled = dest_block.studio_post_duplicate( - store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow - ) + # Allow an XBlock to do anything fancy it may need to when duplicated from another block. + load_services_for_studio(source_item.runtime, user) + children_handled = dest_block.studio_post_duplicate( + store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow + ) if not children_handled: handle_children_duplication( diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 10b2c85adcf..2896318ec7a 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -818,12 +818,11 @@ def _duplicate_block( ) 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. - load_services_for_studio(source_item.runtime, user) - children_handled = dest_block.studio_post_duplicate( - source_item, store, user, duplication_function=_duplicate_block, shallow=False - ) + # Allow an XBlock to do anything fancy it may need to when duplicated from another block. + load_services_for_studio(source_item.runtime, user) + children_handled = dest_block.studio_post_duplicate( + source_item, store, user, duplication_function=_duplicate_block, shallow=False + ) if not children_handled: handle_children_duplication( diff --git a/cms/envs/common.py b/cms/envs/common.py index 11afd870a01..7525e5974a8 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -959,6 +959,7 @@ # Import after sys.path fixup from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin +from xmodule.studio_editable import StudioEditableBlock # These are the Mixins that should be added to every XBlock. # This should be moved into an XBlock Runtime/Application object @@ -969,6 +970,7 @@ XModuleMixin, EditInfoMixin, AuthoringMixin, + StudioEditableBlock, ) XBLOCK_EXTRA_MIXINS = () From 5c2dabbb94d54256edde3557940f9ea01fa00f79 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Sat, 25 Nov 2023 00:51:29 -0300 Subject: [PATCH 03/13] fix: tests --- cms/djangoapps/contentstore/utils.py | 6 +++--- xmodule/library_content_block.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 885e7848f04..4a3003d9615 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1054,12 +1054,12 @@ def duplicate_block( # Allow an XBlock to do anything fancy it may need to when duplicated from another block. load_services_for_studio(source_item.runtime, user) children_handled = dest_block.studio_post_duplicate( - store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow + source_item, store, user, duplication_function=duplicate_block, shallow=shallow ) if not children_handled: handle_children_duplication( - store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow + dest_block, store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow ) # pylint: disable=protected-access @@ -1347,7 +1347,7 @@ def get_course_team(auth_user, course_key, user_perms): 'context_course': course_block, 'show_transfer_ownership_hint': auth_user in instructors and len(instructors) == 1, 'users': formatted_users, - 'allow_actions': bool(user_perms & STUDIO_EDIT_ROLES), + 'allow_actions': bool(user_perms & auth.STUDIO_EDIT_ROLES), } return course_team_context diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index e7542f19a87..a40656f5869 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -592,6 +592,7 @@ def studio_post_duplicate( self, source_item, *_, + **__, ) -> None: # pylint: disable=unused-argument """ Used by the studio after basic duplication of a source block. We handle the children From 3e9bb60f84670a7a7236de56a88e80ea6e26d35b Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Sat, 25 Nov 2023 01:06:24 -0300 Subject: [PATCH 04/13] fix: library preview --- xmodule/conditional_block.py | 1 + xmodule/studio_editable.py | 5 ++--- xmodule/vertical_block.py | 2 ++ 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 78a5b8c1c1c..0a0690e1584 100644 --- a/xmodule/conditional_block.py +++ b/xmodule/conditional_block.py @@ -83,6 +83,7 @@ class ConditionalBlock( And my_property/my_method will be called for required blocks. """ + has_author_view = True display_name = String( display_name=_("Display Name"), diff --git a/xmodule/studio_editable.py b/xmodule/studio_editable.py index 08f91d7d3da..e290ba11ce3 100644 --- a/xmodule/studio_editable.py +++ b/xmodule/studio_editable.py @@ -16,7 +16,6 @@ class StudioEditableBlock(XBlockMixin): This class is only intended to be used with an XBlock! """ - has_author_view = True def render_children(self, context, fragment, can_reorder=False, can_add=False): """ @@ -53,14 +52,14 @@ def get_preview_view_name(block): """ return AUTHOR_VIEW if has_author_view(block) else STUDENT_VIEW - def editor_saved(self, old_metadata, old_content) -> None: # pylint: disable=unused-argument + def editor_saved(self, user, old_metadata, old_content) -> None: # pylint: disable=unused-argument """ Called right *before* the block is written to the DB. Can be used, e.g., to modify fields before saving. By default, is a no-op. Can be overriden in subclasses. """ - def post_editor_saved(self, old_metadata, old_content) -> None: # pylint: disable=unused-argument + def post_editor_saved(self, user, old_metadata, old_content) -> None: # pylint: disable=unused-argument """ Called right *after* the block is written to the DB. Can be used, e.g., to spin up followup tasks. diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index 10ff0a54087..a76f0cc16f2 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -76,6 +76,8 @@ class VerticalBlock( Layout XBlock for rendering subblocks vertically. """ + has_author_view = True + resources_dir = 'assets/vertical' mako_template = 'widgets/sequence-edit.html' From 42c2037f17a6771ef638cf9dad0550ad37bfeabd Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Sat, 25 Nov 2023 01:13:10 -0300 Subject: [PATCH 05/13] style: ran darker --- xmodule/conditional_block.py | 1 + 1 file changed, 1 insertion(+) diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 0a0690e1584..97ea2366c27 100644 --- a/xmodule/conditional_block.py +++ b/xmodule/conditional_block.py @@ -83,6 +83,7 @@ class ConditionalBlock( And my_property/my_method will be called for required blocks. """ + has_author_view = True display_name = String( From b4a4c96059b341d9835a996ecf1e9b388e8d9323 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Sat, 25 Nov 2023 01:20:52 -0300 Subject: [PATCH 06/13] style: pylint fixes --- cms/djangoapps/contentstore/utils.py | 7 ++++++- .../contentstore/xblock_storage_handlers/view_handlers.py | 1 - xmodule/library_content_block.py | 1 - xmodule/util/duplicate.py | 4 ++++ 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 4a3003d9615..f03e3daa7a3 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1059,7 +1059,12 @@ def duplicate_block( if not children_handled: handle_children_duplication( - dest_block, store, source_item, store, user, duplication_function=duplicate_block, shallow=shallow + dest_block, + source_item, + store, + user, + duplication_function=duplicate_block, + shallow=shallow, ) # pylint: disable=protected-access diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 2896318ec7a..b796c5e646c 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -54,7 +54,6 @@ from openedx.core.lib.cache_utils import request_cached from openedx.core.toggles import ENTRANCE_EXAMS from xmodule.course_block import DEFAULT_START_DATE -from xmodule.library_tools import LibraryToolsService from xmodule.modulestore import EdxJSONEncoder, ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index a40656f5869..eb9e6ba695c 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -8,7 +8,6 @@ import random from copy import copy from gettext import ngettext, gettext -from typing import Callable import bleach from django.conf import settings diff --git a/xmodule/util/duplicate.py b/xmodule/util/duplicate.py index 249a4404434..dbbbdf7f653 100644 --- a/xmodule/util/duplicate.py +++ b/xmodule/util/duplicate.py @@ -1,9 +1,13 @@ +""" +Utility to aid xblock duplication +""" from typing import Callable def handle_children_duplication( xblock, source_item, store, user, duplication_function: Callable[..., None], shallow: bool ): + """Implements default behaviour to handle children duplication.""" if not source_item.has_children or shallow: return From aaa26bfacdf7545862559dafde3968432b58c42f Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Wed, 29 Nov 2023 21:44:35 -0300 Subject: [PATCH 07/13] style: use *_args and **_kwargs --- xmodule/library_content_block.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index eb9e6ba695c..081dbb65787 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -590,9 +590,9 @@ def children_are_syncing(self, request, suffix=''): # pylint: disable=unused-ar def studio_post_duplicate( self, source_item, - *_, - **__, - ) -> None: # pylint: disable=unused-argument + *_args, + **__kwargs, + ) -> None: """ Used by the studio after basic duplication of a source block. We handle the children ourselves, because we have to properly reference the library upstream and set the overrides. @@ -601,7 +601,6 @@ def studio_post_duplicate( """ self._validate_sync_permissions() self.get_tools(to_read_library_content=True).trigger_duplication(source_block=source_item, dest_block=self) - return True # Children have been handled. def _validate_library_version(self, validation, lib_tools, version, library_key): """ From 116b013bb3b8f5a7313780a7430a2897639cf248 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Wed, 29 Nov 2023 21:59:00 -0300 Subject: [PATCH 08/13] refactor: move load_services_for_studio back into --- cms/djangoapps/contentstore/utils.py | 65 ++++++++++++++----- cms/djangoapps/contentstore/views/block.py | 2 +- .../contentstore/views/component.py | 3 +- .../xblock_storage_handlers/view_handlers.py | 2 +- xmodule/services.py | 40 ------------ xmodule/studio_editable.py | 9 +-- 6 files changed, 53 insertions(+), 68 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index f03e3daa7a3..83fa7c8c40d 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -15,6 +15,7 @@ from django.utils import translation from django.utils.translation import gettext as _ from help_tokens.core import HelpUrlExpert +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 @@ -26,7 +27,9 @@ from cms.djangoapps.contentstore.toggles import exam_setting_view_enabled from common.djangoapps.course_action_state.models import CourseRerunUIStateManager from common.djangoapps.course_modes.models import CourseMode +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, STUDIO_EDIT_ROLES from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import ( CourseInstructorRole, @@ -42,6 +45,7 @@ get_namespace_choices, generate_milestone_namespace ) +from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from openedx.core import toggles as core_toggles from openedx.core.djangoapps.credit.api import get_credit_requirements, is_credit_course from openedx.core.djangoapps.discussions.config.waffle import ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND @@ -75,12 +79,12 @@ use_tagging_taxonomy_list_page, ) from cms.djangoapps.models.settings.course_grading import CourseGradingModel +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 -from xmodule.services import load_services_for_studio -from xmodule.util.duplicate import handle_children_duplication # 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__) @@ -1050,23 +1054,12 @@ def duplicate_block( asides=asides_to_create ) - children_handled = False # Allow an XBlock to do anything fancy it may need to when duplicated from another block. load_services_for_studio(source_item.runtime, user) - children_handled = dest_block.studio_post_duplicate( + dest_block.studio_post_duplicate( source_item, store, user, duplication_function=duplicate_block, shallow=shallow ) - if not children_handled: - handle_children_duplication( - dest_block, - source_item, - store, - user, - duplication_function=duplicate_block, - shallow=shallow, - ) - # pylint: disable=protected-access if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: parent = store.get_item(parent_usage_key) @@ -1167,6 +1160,25 @@ def gather_block_attributes(source_item, display_name=None, is_child=False): 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 + + def update_course_details(request, course_key, payload, course_block): """ Utils is used to update course details. @@ -1352,7 +1364,7 @@ def get_course_team(auth_user, course_key, user_perms): 'context_course': course_block, 'show_transfer_ownership_hint': auth_user in instructors and len(instructors) == 1, 'users': formatted_users, - 'allow_actions': bool(user_perms & auth.STUDIO_EDIT_ROLES), + 'allow_actions': bool(user_perms & STUDIO_EDIT_ROLES), } return course_team_context @@ -1641,3 +1653,24 @@ def get_course_videos_context(course_block, pagination_conf, course_key=None): # Cached state for transcript providers' credentials (org-specific) course_video_context['transcript_credentials'] = get_transcript_credentials_state_for_org(course.id.org) return course_video_context + + +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 1ce1630fb76..b7b75d23b22 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -9,6 +9,7 @@ from django.http import Http404, HttpResponse from django.utils.translation import gettext as _ from django.views.decorators.http import require_http_methods +from cms.djangoapps.contentstore.utils import load_services_for_studio from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment @@ -28,7 +29,6 @@ from xmodule.modulestore.django import ( modulestore, ) -from xmodule.services import load_services_for_studio # lint-amnesty, pylint: disable=wrong-import-order from xmodule.x_module import ( diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index f06aa8f5017..b0e270c4bbc 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -35,9 +35,8 @@ from openedx.core.djangoapps.content_tagging.api import get_content_tags from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.services import load_services_for_studio # lint-amnesty, pylint: disable=wrong-import-order from ..toggles import use_new_unit_page -from ..utils import get_lms_link_for_item, get_sibling_urls, reverse_course_url, get_unit_url +from ..utils import get_lms_link_for_item, get_sibling_urls, load_services_for_studio, reverse_course_url, get_unit_url from ..helpers import get_parent_xblock, is_unit, xblock_type_display_name from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import ( add_container_page_publishing_info, diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index b796c5e646c..d6e68d71499 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -59,7 +59,6 @@ from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata -from xmodule.services import load_services_for_studio from xmodule.tabs import CourseTabList from xmodule.util.duplicate import handle_children_duplication @@ -74,6 +73,7 @@ is_currently_visible_to_students, is_self_paced, get_taxonomy_tags_widget_url, + load_services_for_studio, ) from .create_xblock import create_xblock diff --git a/xmodule/services.py b/xmodule/services.py index 4835de84d44..48a79c85f2b 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -316,43 +316,3 @@ def _handle_deprecated_progress_event(self, block, event): # in order to avoid duplicate work and possibly conflicting semantics. if not getattr(block, 'has_custom_completion', False): self.completion_service.submit_completion(block.scope_ids.usage_id, 1.0) - - -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": xmodule.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/xmodule/studio_editable.py b/xmodule/studio_editable.py index e290ba11ce3..df48fd8e9e2 100644 --- a/xmodule/studio_editable.py +++ b/xmodule/studio_editable.py @@ -77,17 +77,10 @@ def studio_post_duplicate( """ Called when a the block is duplicated. Can be used, e.g., for special handling of child duplication. - Returns 'True' if children have been handled and thus shouldn't be handled by the standard - duplication logic. + Children must always be handled. In case of inheritance it can be done by running this method with super(). By default, implements standard duplication logic. """ - self.handle_children_duplication(source_item, store, user, duplication_function, shallow) - return True - - def handle_children_duplication( - self, source_item, store, user, duplication_function: Callable[..., None], shallow: bool - ): handle_children_duplication(self, source_item, store, user, duplication_function, shallow) From c5cb3f88e1de7aa3a497990d37d08d54254c87b2 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Wed, 29 Nov 2023 23:23:50 -0300 Subject: [PATCH 09/13] refactor: move editor_saved, post_editor_saved, and studio_post_duplicate to authoringmixin --- cms/djangoapps/contentstore/views/preview.py | 4 +- .../xblock_storage_handlers/view_handlers.py | 11 +---- cms/envs/common.py | 2 - cms/lib/xblock/authoring_mixin.py | 40 +++++++++++++++++++ xmodule/conditional_block.py | 2 - xmodule/studio_editable.py | 33 +-------------- xmodule/util/duplicate.py | 19 --------- xmodule/vertical_block.py | 2 - 8 files changed, 45 insertions(+), 68 deletions(-) delete mode 100644 xmodule/util/duplicate.py diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 096de387157..5b963267434 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -21,7 +21,7 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import XBlockI18nService, modulestore from xmodule.partitions.partitions_service import PartitionService -from xmodule.services import SettingsService, StudioPermissionsService, TeamsConfigurationService +from xmodule.services import SettingsService, TeamsConfigurationService from xmodule.studio_editable import has_author_view from xmodule.util.sandboxing import SandboxService from xmodule.util.builtin_assets import add_webpack_js_to_fragment @@ -45,7 +45,7 @@ wrap_xblock_aside ) -from ..utils import get_visibility_partition_info +from ..utils import StudioPermissionsService, get_visibility_partition_info from .access import get_user_role from .session_kv_store import SessionKeyValueStore diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index d6e68d71499..c7e080fa7d1 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -60,7 +60,6 @@ from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata from xmodule.tabs import CourseTabList -from xmodule.util.duplicate import handle_children_duplication from ..utils import ( ancestor_has_staff_lock, @@ -816,17 +815,9 @@ def _duplicate_block( asides=asides_to_create, ) - children_handled = False # Allow an XBlock to do anything fancy it may need to when duplicated from another block. load_services_for_studio(source_item.runtime, user) - children_handled = dest_block.studio_post_duplicate( - source_item, store, user, duplication_function=_duplicate_block, shallow=False - ) - - if not children_handled: - handle_children_duplication( - dest_block, source_item, store, user, duplication_function=_duplicate_block, shallow=False - ) + dest_block.studio_post_duplicate(source_item, store, user, duplication_function=_duplicate_block, shallow=False) # pylint: disable=protected-access if "detached" not in source_item.runtime.load_block_type(category)._class_tags: diff --git a/cms/envs/common.py b/cms/envs/common.py index 7525e5974a8..11afd870a01 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -959,7 +959,6 @@ # Import after sys.path fixup from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin -from xmodule.studio_editable import StudioEditableBlock # These are the Mixins that should be added to every XBlock. # This should be moved into an XBlock Runtime/Application object @@ -970,7 +969,6 @@ XModuleMixin, EditInfoMixin, AuthoringMixin, - StudioEditableBlock, ) XBLOCK_EXTRA_MIXINS = () diff --git a/cms/lib/xblock/authoring_mixin.py b/cms/lib/xblock/authoring_mixin.py index a3d3b3298ce..c2e336c44a3 100644 --- a/cms/lib/xblock/authoring_mixin.py +++ b/cms/lib/xblock/authoring_mixin.py @@ -4,6 +4,7 @@ import logging +from typing import Callable from django.conf import settings from web_fragments.fragment import Fragment @@ -51,3 +52,42 @@ def visibility_view(self, _context=None): scope=Scope.settings, enforce_type=True, ) + + def editor_saved(self, user, old_metadata, old_content) -> None: # pylint: disable=unused-argument + """ + Called right *before* the block is written to the DB. Can be used, e.g., to modify fields before saving. + + By default, is a no-op. Can be overriden in subclasses. + """ + + def post_editor_saved(self, user, old_metadata, old_content) -> None: # pylint: disable=unused-argument + """ + Called right *after* the block is written to the DB. Can be used, e.g., to spin up followup tasks. + + By default, is a no-op. Can be overriden in subclasses. + """ + + def studio_post_duplicate( + self, + source_item, + store, + user, + duplication_function: Callable[..., None], + shallow: bool, + ) -> None: # pylint: disable=unused-argument + """ + Called when a the block is duplicated. Can be used, e.g., for special handling of child duplication. + + Children must always be handled. In case of inheritance it can be done by running this method with super(). + + By default, implements standard duplication logic. + """ + if not source_item.has_children or shallow: + return + + self.children = self.children or [] + for child in source_item.children: + dupe = duplication_function(self.location, child, user=user, is_child=True) + if dupe not in self.children: # duplicate_fun may add the child for us. + self.children.append(dupe) + store.update_item(self, user.id) diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 97ea2366c27..78a5b8c1c1c 100644 --- a/xmodule/conditional_block.py +++ b/xmodule/conditional_block.py @@ -84,8 +84,6 @@ class ConditionalBlock( """ - has_author_view = True - display_name = String( display_name=_("Display Name"), help=_("The display name for this component."), diff --git a/xmodule/studio_editable.py b/xmodule/studio_editable.py index df48fd8e9e2..43fe7c05aa7 100644 --- a/xmodule/studio_editable.py +++ b/xmodule/studio_editable.py @@ -4,7 +4,6 @@ from typing import Callable from xblock.core import XBlock, XBlockMixin -from xmodule.util.duplicate import handle_children_duplication from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW @@ -17,6 +16,8 @@ class StudioEditableBlock(XBlockMixin): This class is only intended to be used with an XBlock! """ + has_author_view = True + def render_children(self, context, fragment, can_reorder=False, can_add=False): """ Renders the children of the block with HTML appropriate for Studio. If can_reorder is True, @@ -52,36 +53,6 @@ def get_preview_view_name(block): """ return AUTHOR_VIEW if has_author_view(block) else STUDENT_VIEW - def editor_saved(self, user, old_metadata, old_content) -> None: # pylint: disable=unused-argument - """ - Called right *before* the block is written to the DB. Can be used, e.g., to modify fields before saving. - - By default, is a no-op. Can be overriden in subclasses. - """ - - def post_editor_saved(self, user, old_metadata, old_content) -> None: # pylint: disable=unused-argument - """ - Called right *after* the block is written to the DB. Can be used, e.g., to spin up followup tasks. - - By default, is a no-op. Can be overriden in subclasses. - """ - - def studio_post_duplicate( - self, - source_item, - store, - user, - duplication_function: Callable[..., None], - shallow: bool, - ) -> None: # pylint: disable=unused-argument - """ - Called when a the block is duplicated. Can be used, e.g., for special handling of child duplication. - - Children must always be handled. In case of inheritance it can be done by running this method with super(). - - By default, implements standard duplication logic. - """ - handle_children_duplication(self, source_item, store, user, duplication_function, shallow) def has_author_view(block): diff --git a/xmodule/util/duplicate.py b/xmodule/util/duplicate.py deleted file mode 100644 index dbbbdf7f653..00000000000 --- a/xmodule/util/duplicate.py +++ /dev/null @@ -1,19 +0,0 @@ -""" -Utility to aid xblock duplication -""" -from typing import Callable - - -def handle_children_duplication( - xblock, source_item, store, user, duplication_function: Callable[..., None], shallow: bool -): - """Implements default behaviour to handle children duplication.""" - if not source_item.has_children or shallow: - return - - xblock.children = xblock.children or [] - for child in source_item.children: - dupe = duplication_function(xblock.location, child, user=user, is_child=True) - if dupe not in xblock.children: # duplicate_fun may add the child for us. - xblock.children.append(dupe) - store.update_item(xblock, user.id) diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index a76f0cc16f2..10ff0a54087 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -76,8 +76,6 @@ class VerticalBlock( Layout XBlock for rendering subblocks vertically. """ - has_author_view = True - resources_dir = 'assets/vertical' mako_template = 'widgets/sequence-edit.html' From 619958821dc2b9c965aa6170123f52520ced63be Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Thu, 30 Nov 2023 00:53:04 -0300 Subject: [PATCH 10/13] refactor: move duplication logic into authoringmixin --- cms/djangoapps/contentstore/utils.py | 67 +++---------- .../xblock_storage_handlers/view_handlers.py | 97 ++----------------- cms/lib/xblock/authoring_mixin.py | 83 +++++++++++++++- 3 files changed, 96 insertions(+), 151 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 83fa7c8c40d..e5f73b8137e 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -6,8 +6,7 @@ import logging from collections import defaultdict from contextlib import contextmanager -from datetime import datetime, timezone -from uuid import uuid4 +from datetime import datetime from django.conf import settings from django.core.exceptions import ValidationError @@ -18,8 +17,6 @@ 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 milestones import api as milestones_api from pytz import UTC from xblock.fields import Scope @@ -1026,64 +1023,22 @@ def duplicate_block( 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. + If shallow is True, does not copy children. """ 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 - ) - - # Allow an XBlock to do anything fancy it may need to when duplicated from another block. - load_services_for_studio(source_item.runtime, user) - dest_block.studio_post_duplicate( - source_item, store, user, duplication_function=duplicate_block, shallow=shallow + return source_item.studio_duplicate( + parent_usage_key=parent_usage_key, + duplicate_source_usage_key=duplicate_source_usage_key, + user=user, + store=store, + dest_usage_key=dest_usage_key, + display_name=display_name, + shallow=shallow, + is_child=is_child, ) - # 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): """ diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index c7e080fa7d1..98decdb3d93 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -10,7 +10,6 @@ """ import logging from datetime import datetime -from uuid import uuid4 from attrs import asdict from django.conf import settings @@ -18,11 +17,8 @@ from django.contrib.auth.models import User # pylint: disable=imported-auth-user from django.core.exceptions import PermissionDenied from django.http import HttpResponse, HttpResponseBadRequest -from django.utils.timezone import timezone from django.utils.translation import gettext as _ 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 openedx_tagging.core.tagging import api as tagging_api from edx_proctoring.api import ( does_backend_support_onboarding, @@ -755,94 +751,15 @@ def _duplicate_block( 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, - ) - - # Allow an XBlock to do anything fancy it may need to when duplicated from another block. - load_services_for_studio(source_item.runtime, user) - dest_block.studio_post_duplicate(source_item, store, user, duplication_function=_duplicate_block, shallow=False) - - # 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 source_item.studio_duplicate( + parent_usage_key=parent_usage_key, + duplicate_source_usage_key=duplicate_source_usage_key, + user=user, + store=store, + display_name=display_name, + is_child=is_child, ) - return dest_block.location - @login_required def delete_item(request, usage_key): diff --git a/cms/lib/xblock/authoring_mixin.py b/cms/lib/xblock/authoring_mixin.py index c2e336c44a3..9310da49098 100644 --- a/cms/lib/xblock/authoring_mixin.py +++ b/cms/lib/xblock/authoring_mixin.py @@ -4,12 +4,16 @@ import logging -from typing import Callable +from datetime import datetime, timezone +from uuid import uuid4 from django.conf import settings from web_fragments.fragment import Fragment from xblock.core import XBlock, XBlockMixin from xblock.fields import String, Scope +from openedx_events.content_authoring.data import DuplicatedXBlockData +from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED + log = logging.getLogger(__name__) @@ -67,16 +71,84 @@ def post_editor_saved(self, user, old_metadata, old_content) -> None: # pylint: By default, is a no-op. Can be overriden in subclasses. """ + def studio_duplicate( + self, + parent_usage_key, + duplicate_source_usage_key, + user, + store, + 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. + """ + from cms.djangoapps.contentstore.utils import gather_block_attributes, load_services_for_studio + + if not dest_usage_key: + # Change the blockID to be unique. + dest_usage_key = self.location.replace(name=uuid4().hex) + + category = dest_usage_key.block_type + + duplicate_metadata, asides_to_create = gather_block_attributes( + self, + 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=self.get_explicitly_set_fields_by_scope(Scope.content), + metadata=duplicate_metadata, + runtime=self.runtime, + asides=asides_to_create, + ) + + # Allow an XBlock to do anything fancy it may need to when duplicated from another block. + load_services_for_studio(self.runtime, user) + dest_block.studio_post_duplicate(self, store, user, shallow=shallow) + # pylint: disable=protected-access + if "detached" not in self.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 self.location in parent.children: + source_index = parent.children.index(self.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 studio_post_duplicate( self, source_item, store, user, - duplication_function: Callable[..., None], shallow: bool, ) -> None: # pylint: disable=unused-argument """ - Called when a the block is duplicated. Can be used, e.g., for special handling of child duplication. + Called when after a block is duplicated. Can be used, e.g., for special handling of child duplication. Children must always be handled. In case of inheritance it can be done by running this method with super(). @@ -87,7 +159,8 @@ def studio_post_duplicate( self.children = self.children or [] for child in source_item.children: - dupe = duplication_function(self.location, child, user=user, is_child=True) - if dupe not in self.children: # duplicate_fun may add the child for us. + child_block = store.get_item(child) + dupe = child_block.studio_duplicate(self.location, child, user=user, store=store, is_child=True) + if dupe not in self.children: # studio_duplicate may add the child for us. self.children.append(dupe) store.update_item(self, user.id) From 7f6924fa95011d59af6a59f037465a542557603b Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Thu, 30 Nov 2023 01:29:37 -0300 Subject: [PATCH 11/13] style: pylint and pep8 fixes --- xmodule/services.py | 8 -------- xmodule/studio_editable.py | 3 --- 2 files changed, 11 deletions(-) diff --git a/xmodule/services.py b/xmodule/services.py index 48a79c85f2b..25c680a9653 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -6,23 +6,15 @@ import inspect import logging from functools import partial -from common.djangoapps.edxmako.services import MakoService -from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from config_models.models import ConfigurationModel from django.conf import settings from eventtracking import tracker from edx_when.field_data import DateLookupFieldData -from lti_consumer.models import CourseAllowPIISharingInLTIFlag from xblock.reference.plugins import Service from xblock.runtime import KvsFieldData -import xmodule from common.djangoapps.track import contexts -from common.djangoapps.student.auth import ( - has_studio_read_access, - has_studio_write_access, -) from lms.djangoapps.courseware.masquerade import is_masquerading_as_specific_student from xmodule.modulestore.django import modulestore diff --git a/xmodule/studio_editable.py b/xmodule/studio_editable.py index 43fe7c05aa7..d190c966cab 100644 --- a/xmodule/studio_editable.py +++ b/xmodule/studio_editable.py @@ -1,8 +1,6 @@ """ Mixin to support editing in Studio. """ -from typing import Callable - from xblock.core import XBlock, XBlockMixin from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW @@ -54,7 +52,6 @@ def get_preview_view_name(block): return AUTHOR_VIEW if has_author_view(block) else STUDENT_VIEW - def has_author_view(block): """ Returns True if the xmodule linked to the block supports "author_view". From adeec3ab392a1879609690678791e1d3c6b9cfa9 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Thu, 30 Nov 2023 01:32:30 -0300 Subject: [PATCH 12/13] refactor: rename source_item to source_block --- cms/lib/xblock/authoring_mixin.py | 6 +++--- xmodule/library_content_block.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cms/lib/xblock/authoring_mixin.py b/cms/lib/xblock/authoring_mixin.py index 9310da49098..d66f9f88cd0 100644 --- a/cms/lib/xblock/authoring_mixin.py +++ b/cms/lib/xblock/authoring_mixin.py @@ -142,7 +142,7 @@ def studio_duplicate( def studio_post_duplicate( self, - source_item, + source_block, store, user, shallow: bool, @@ -154,11 +154,11 @@ def studio_post_duplicate( By default, implements standard duplication logic. """ - if not source_item.has_children or shallow: + if not source_block.has_children or shallow: return self.children = self.children or [] - for child in source_item.children: + for child in source_block.children: child_block = store.get_item(child) dupe = child_block.studio_duplicate(self.location, child, user=user, store=store, is_child=True) if dupe not in self.children: # studio_duplicate may add the child for us. diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index 081dbb65787..6ba986034a4 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -589,7 +589,7 @@ def children_are_syncing(self, request, suffix=''): # pylint: disable=unused-ar def studio_post_duplicate( self, - source_item, + source_block, *_args, **__kwargs, ) -> None: @@ -600,7 +600,7 @@ def studio_post_duplicate( Otherwise we'll end up losing data on the next refresh. """ self._validate_sync_permissions() - self.get_tools(to_read_library_content=True).trigger_duplication(source_block=source_item, dest_block=self) + self.get_tools(to_read_library_content=True).trigger_duplication(source_block=source_block, dest_block=self) def _validate_library_version(self, validation, lib_tools, version, library_key): """ From cd78eb3b6b8fab921901237d3d1d36c7d0cf894f Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Fri, 1 Dec 2023 08:14:45 -0300 Subject: [PATCH 13/13] refactor: remove redundant hasattrs due to inheritance --- cms/djangoapps/contentstore/views/block.py | 2 +- .../xblock_storage_handlers/view_handlers.py | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index b7b75d23b22..810a824498f 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -9,10 +9,10 @@ from django.http import Http404, HttpResponse from django.utils.translation import gettext as _ from django.views.decorators.http import require_http_methods -from cms.djangoapps.contentstore.utils import load_services_for_studio from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment +from cms.djangoapps.contentstore.utils import load_services_for_studio from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW from common.djangoapps.edxmako.shortcuts import render_to_string from common.djangoapps.student.auth import ( diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 98decdb3d93..9e385e9d91a 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -301,13 +301,10 @@ def _update_with_callback(xblock, user, old_metadata=None, old_content=None): old_metadata = own_metadata(xblock) if old_content is None: old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) - if hasattr(xblock, "editor_saved"): - load_services_for_studio(xblock.runtime, user) - xblock.editor_saved(user, old_metadata, old_content) + load_services_for_studio(xblock.runtime, user) + xblock.editor_saved(user, old_metadata, old_content) xblock_updated = modulestore().update_item(xblock, user.id) - if hasattr(xblock_updated, "post_editor_saved"): - load_services_for_studio(xblock_updated.runtime, user) - xblock_updated.post_editor_saved(user, old_metadata, old_content) + xblock_updated.post_editor_saved(user, old_metadata, old_content) return xblock_updated