Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: inheritable authoring mixin callbacks for editing & duplication #33756

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
536493f
refactor: inheritable studioeditableblock's callbacks for editing & d…
DanielVZ96 Nov 21, 2023
5901e25
refactor: remove hasattr and add editable studio to cms xblock modules
DanielVZ96 Nov 25, 2023
5c2dabb
fix: tests
DanielVZ96 Nov 25, 2023
3e9bb60
fix: library preview
DanielVZ96 Nov 25, 2023
42c2037
style: ran darker
DanielVZ96 Nov 25, 2023
b4a4c96
style: pylint fixes
DanielVZ96 Nov 25, 2023
aaa26bf
style: use *_args and **_kwargs
DanielVZ96 Nov 30, 2023
116b013
refactor: move load_services_for_studio back into
DanielVZ96 Nov 30, 2023
c5cb3f8
refactor: move editor_saved, post_editor_saved, and studio_post_dupli…
DanielVZ96 Nov 30, 2023
6199588
refactor: move duplication logic into authoringmixin
DanielVZ96 Nov 30, 2023
7f6924f
style: pylint and pep8 fixes
DanielVZ96 Nov 30, 2023
adeec3a
refactor: rename source_item to source_block
DanielVZ96 Nov 30, 2023
cd78eb3
refactor: remove redundant hasattrs due to inheritance
DanielVZ96 Dec 1, 2023
893ff58
Merge branch 'master' into dvz/studio-editable-block-callback-refactor
DanielVZ96 Dec 5, 2023
8ad064c
Merge branch 'master' into dvz/studio-editable-block-callback-refactor
DanielVZ96 Dec 5, 2023
e94fa52
Merge branch 'master' into dvz/studio-editable-block-callback-refactor
DanielVZ96 Dec 8, 2023
3cb2769
revert: remove studio duplicate hooks
DanielVZ96 Jul 7, 2024
5312b5e
Merge branch 'master' into dvz/studio-editable-block-callback-refactor
DanielVZ96 Jul 8, 2024
9c85399
style: fix pylint unused imports
DanielVZ96 Jul 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 13 additions & 64 deletions cms/djangoapps/contentstore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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__)
Expand Down Expand Up @@ -1055,23 +1051,16 @@ def duplicate_block(
)

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(
Agrendalath marked this conversation as resolved.
Show resolved Hide resolved
source_item, store, user, duplication_function=duplicate_block, shallow=shallow
)

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)
if not children_handled:
handle_children_duplication(
dest_block, 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:
Expand Down Expand Up @@ -1173,25 +1162,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):
DanielVZ96 marked this conversation as resolved.
Show resolved Hide resolved
"""
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.
Expand Down Expand Up @@ -1377,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
Expand Down Expand Up @@ -1666,24 +1636,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:
DanielVZ96 marked this conversation as resolved.
Show resolved Hide resolved
"""
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)
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/views/block.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -46,7 +47,6 @@
from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import (
handle_xblock,
create_xblock_info,
load_services_for_studio,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an indirect import, should be going directly to contentstore.utils

get_block_info,
get_xblock,
delete_orphans,
Expand Down
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/views/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__ = [
Expand Down
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -41,15 +40,13 @@
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,
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
Expand All @@ -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,
Expand Down Expand Up @@ -296,46 +294,6 @@ def modify_xblock(usage_key, request):
)


class StudioPermissionsService:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved into services for reusability and reduce dependencies too

"""
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.
Expand Down Expand Up @@ -860,29 +818,16 @@ def _duplicate_block(
)

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(
DanielVZ96 marked this conversation as resolved.
Show resolved Hide resolved
source_item, store, user, duplication_function=_duplicate_block, shallow=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)
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:
Expand Down
2 changes: 2 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -969,6 +970,7 @@
XModuleMixin,
EditInfoMixin,
AuthoringMixin,
StudioEditableBlock,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allowed? this was the only way i could make sure all blocks would have the required callbacks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielVZ96 Great question.

I would rather not add StudioEditableBlock to this list, as that would make render_children and get_preview_view_name available on every XBlock. I'd like to avoid adding methods to the base XBlock API except in the cases where we really need to (like editor_saved, post_editor_saved, studio_post_duplicate).

That said, you are right that we need some way of defining editor_saved, post_editor_saved, and studio_post_duplicate on every block. I was mistaken that we could just add them to StudioEditableBlock. Could I suggest adding them to AuthoringMixin instead, which is already mixed into all CMS blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! thanks for the suggestion

)
XBLOCK_EXTRA_MIXINS = ()

Expand Down
1 change: 1 addition & 0 deletions xmodule/conditional_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
10 changes: 8 additions & 2 deletions xmodule/library_content_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -587,15 +588,20 @@ 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,
Agrendalath marked this conversation as resolved.
Show resolved Hide resolved
*_,
**__,
) -> None: # pylint: disable=unused-argument
Agrendalath marked this conversation as resolved.
Show resolved Hide resolved
Agrendalath marked this conversation as resolved.
Show resolved Hide resolved
"""
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.

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):
Expand Down
Loading
Loading