fix: address PR comments other than logging & nits

Author: kdmccormick

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.

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,
@@ -995,7 +996,7 @@
     XModuleMixin,
     EditInfoMixin,
     AuthoringMixin,
-    'cms.lib.xblock.upstream_sync.UpstreamSyncMixin',
+    UpstreamSyncMixin,
 )
 
 # .. setting_name: XBLOCK_EXTRA_MIXINS

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

openedx/core/djangoapps/content_libraries/api.py

@@ -257,10 +257,10 @@ def from_component(cls, library_key, component, associated_collections=None):
                 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,