From 629d458979399406c8d6b55cd6d2cb4ad52d3ac2 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 14 Oct 2024 22:41:40 -0400 Subject: [PATCH] fix: address PR comments other than logging & nits --- .../rest_api/v2/views/downstreams.py | 39 ++++++++++--------- cms/envs/common.py | 3 +- cms/lib/xblock/upstream_sync.py | 15 +++++-- .../core/djangoapps/content_libraries/api.py | 6 +-- 4 files changed, 37 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 2fdac9ad7c6b..5dc3990c4496 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -61,7 +61,7 @@ "developer_message": string? } """ -from django.contrib.auth.models import AbstractUser +from django.contrib.auth.models import User # pylint: disable=imported-auth-user from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey from rest_framework.exceptions import NotFound, ValidationError @@ -82,13 +82,23 @@ from xmodule.modulestore.exceptions import ItemNotFoundError +class _AuthenticatedRequest(Request): + """ + Alias for the `Request` class which tells mypy to assume that `.user` is not an AnonymousUser. + + Using this does NOT ensure the request is actually authenticated-- + you will some other way to ensure that, such as `@view_auth_classes(is_authenticated=True)`. + """ + user: User + + # TODO: Potential future view. # @view_auth_classes(is_authenticated=True) # class DownstreamListView(DeveloperErrorViewMixin, APIView): # """ # List all blocks which are linked to upstream content, with optional filtering. # """ -# def get(self, request: Request) -> Response: +# def get(self, request: _AuthenticatedRequest) -> Response: # """ # Handle the request. # """ @@ -102,22 +112,20 @@ class DownstreamView(DeveloperErrorViewMixin, APIView): """ Inspect or manage an XBlock's link to upstream content. """ - def get(self, request: Request, usage_key_string: str) -> Response: + def get(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ Inspect an XBlock's link to upstream content. """ - assert isinstance(request.user, AbstractUser) downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=False) _ensure_upstream_ref(downstream) if link := UpstreamLink.try_get_for_block(downstream): return Response(link.to_json()) raise ValidationError(detail=f"Block '{usage_key_string}' is not linked to an upstream") - def put(self, request: Request, usage_key_string: str) -> Response: + def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ Edit an XBlock's link to upstream content. """ - assert isinstance(request.user, AbstractUser) downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) new_upstream_ref = request.data.get("upstream_ref") if not isinstance(new_upstream_ref, str): @@ -140,15 +148,13 @@ def put(self, request: Request, usage_key_string: str) -> Response: except BadUpstream as exc: raise ValidationError({"upstream_ref": str(exc)}) from exc modulestore().update_item(downstream, request.user.id) - link = UpstreamLink.get_for_block(downstream) - assert link + link: UpstreamLink = UpstreamLink.get_for_block(downstream) # type: ignore[assignment] return Response(link.to_json()) - # def delete(self, request: Request, usage_key_string: str) -> Response: + # def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: # """ # Sever an XBlock's link to upstream content. # """ - # assert isinstance(request.user, AbstractUser) # downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) # _ensure_upstream_ref(downstream) # .... @@ -160,11 +166,10 @@ class SyncFromUpstreamView(DeveloperErrorViewMixin, APIView): Accept or decline an opportunity to sync a downstream block from its upstream content. """ - def post(self, request: Request, usage_key_string: str) -> Response: + def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ Pull latest updates to the block at {usage_key_string} from its linked upstream content. """ - assert isinstance(request.user, AbstractUser) downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) _ensure_upstream_ref(downstream) if not downstream.upstream: @@ -175,15 +180,13 @@ def post(self, request: Request, usage_key_string: str) -> Response: except (BadUpstream, BadDownstream) as exc: raise ValidationError(detail=str(exc)) from exc modulestore().update_item(downstream, request.user.id) - upstream_link = UpstreamLink.get_for_block(downstream) - assert upstream_link - return Response(upstream_link.to_json(), status=200) + link: UpstreamLink = UpstreamLink.get_for_block(downstream) # type: ignore[assignment] + return Response(link.to_json(), status=200) - def delete(self, request: Request, usage_key_string: str) -> Response: + def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ Decline the latest updates to the block at {usage_key_string}. """ - assert isinstance(request.user, AbstractUser) downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) _ensure_upstream_ref(downstream) try: @@ -194,7 +197,7 @@ def delete(self, request: Request, usage_key_string: str) -> Response: return Response(status=204) -def _load_accessible_block(user: AbstractUser, usage_key_string: str, *, require_write_access: bool) -> XBlock: +def _load_accessible_block(user: User, usage_key_string: str, *, require_write_access: bool) -> XBlock: """ Given a logged in-user and a serialized usage key of an upstream-linked XBlock, load it from the ModuleStore, raising a DRF-friendly exception if anything goes wrong. diff --git a/cms/envs/common.py b/cms/envs/common.py index fbd296020fb6..623fdaa929ed 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -127,6 +127,7 @@ from lms.djangoapps.lms_xblock.mixin import LmsBlockMixin from cms.lib.xblock.authoring_mixin import AuthoringMixin +from cms.lib.xblock.upstream_sync import UpstreamSyncMixin from xmodule.modulestore.edit_info import EditInfoMixin from openedx.core.djangoapps.theming.helpers_dirs import ( get_themes_unchecked, @@ -1008,7 +1009,7 @@ XModuleMixin, EditInfoMixin, AuthoringMixin, - 'cms.lib.xblock.upstream_sync.UpstreamSyncMixin', + UpstreamSyncMixin, ) # .. setting_name: XBLOCK_EXTRA_MIXINS diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 3cdc5edcf5d3..d529879a6fb3 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -9,10 +9,11 @@ HOWEVER, those assumptions may loosen in the future. So, we consider these to be INTERNAL ASSUMPIONS that should not be exposed through this module's public Python interface. """ +from __future__ import annotations + import typing as t from dataclasses import dataclass, asdict -from django.contrib.auth.models import AbstractUser from django.core.exceptions import PermissionDenied from django.utils.translation import gettext_lazy as _ from rest_framework.exceptions import NotFound @@ -23,8 +24,8 @@ from xblock.fields import Scope, String, Integer from xblock.core import XBlockMixin, XBlock -import openedx.core.djangoapps.xblock.api as xblock_api -from openedx.core.djangoapps.content_libraries.api import get_library_block +if t.TYPE_CHECKING: + from django.contrib.auth.models import AbstractUser class BadUpstream(Exception): @@ -127,6 +128,10 @@ def get_for_block(cls, downstream: XBlock) -> t.Self | None: f"downstream block '{downstream.usage_key}' is linked to " f"upstream block of different type '{upstream_key}'" ) + # We import this here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.content_libraries.api import ( + get_library_block # pylint: disable=wrong-import-order + ) try: lib_meta = get_library_block(upstream_key) except XBlockNotFoundError as exc: @@ -188,8 +193,10 @@ def _load_upstream_link_and_block(downstream: XBlock, user: AbstractUser) -> tup """ if not (link := UpstreamLink.get_for_block(downstream)): # can raise BadUpstream, BadDownstream return None + # We import load_block here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.xblock.api import load_block # pylint: disable=wrong-import-order try: - lib_block: XBlock = xblock_api.load_block(UsageKey.from_string(downstream.upstream), user) + lib_block: XBlock = load_block(UsageKey.from_string(downstream.upstream), user) except (NotFound, PermissionDenied) as exc: raise BadUpstream(_("Linked library item could not be loaded: {}").format(downstream.upstream)) from exc return link, lib_block diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 7dc8f2e798ff..0c1d89d6600e 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -243,10 +243,10 @@ def from_component(cls, library_key, component): library_key, component, ), - display_name=component.versioning.draft.title, + display_name=draft.title, created=component.created, - modified=component.versioning.draft.created, - draft_version_num=component.versioning.draft.version_num, + modified=draft.created, + draft_version_num=draft.version_num, published_version_num=published.version_num if published else None, last_published=None if last_publish_log is None else last_publish_log.published_at, published_by=published_by,