Skip to content

Commit

Permalink
fix: introduce NoUpstream exception; more logging; more consistent HT…
Browse files Browse the repository at this point in the history 
…TP API errors
  • Loading branch information
@kdmccormick
kdmccormick committed Oct 16, 2024
1 parent a295cbd commit b403256
Show file tree
Hide file tree
Showing 7 changed files with 307 additions and 153 deletions.
6 changes: 3 additions & 3 deletions cms/djangoapps/contentstore/helpers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,16 +382,16 @@ def _import_xml_node_to_parent(
raise NotImplementedError("We don't yet support pasting XBlocks with children")
temp_xblock.parent = parent_key
if copied_from_block:
# Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin)
temp_xblock.copied_from_block = copied_from_block
# Try to link the pasted block (downstream) to the copied block (upstream).
temp_xblock.upstream = copied_from_block
try:
UpstreamLink.get_for_block(temp_xblock)
except (BadDownstream, BadUpstream):
# Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an
# upstream. That's fine; just don't link.
# upstream. That's fine! Instead, we store a reference to where this block was copied from, in the
# 'copied_from_block' field (from AuthoringMixin).
temp_xblock.upstream = None
temp_xblock.copied_from_block = copied_from_block
else:
# But if it doesn't fail, then populate the `upstream_version` field based on what was copied. Note that
# this could be the latest published version, or it could be an an even newer draft version.
Expand Down
7 changes: 6 additions & 1 deletion cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,12 @@ def get(self, request: Request, usage_key_string: str):
"block_type": child_info.location.block_type,
"user_partition_info": user_partition_info,
"user_partitions": user_partitions,
"upstream_link": upstream_link.to_json() if upstream_link else None,
"upstream_link": (
# If the block isn't linked to an upstream (which is by far the most common case) then just
# make this field null, which communicates the same info, but with less noise.
upstream_link.to_json() if upstream_link.upstream_ref
else None
),
"validation_messages": validation_messages,
"render_error": render_error,
})
Expand Down
183 changes: 102 additions & 81 deletions cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py
View file
Original file line number Diff line number Diff line change
@@ -1,66 +1,63 @@
"""
API Views for managing & syncing links between upstream & downstream content
Only [BETA] endpoints are implemented currently.
The [FULL] endpoints should be implemented for the Full Libraries Relaunch, or removed from this doc.
[FULL] List downstream blocks that can be synced, filterable by course or sync-readiness.
GET /api/v2/contentstore/downstreams
GET /api/v2/contentstore/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true
200: A paginated list of applicable & accessible downstream blocks.
[BETA] Inspect a single downstream block's link to upstream content.
GET /api/v2/contentstore/downstreams/{usage_key_string}
200: Upstream link details successfully fetched.
404: Block not found, OR user lacks permission to read block.
400: Blocks is not linked to upstream content.
[FULL] Sever a single downstream block's link to upstream content.
DELETE /api/v2/contentstore/downstreams/{usage_key_string}
204: Block successfully unlinked. No response body.
404: Block not found, OR user lacks permission to edit block
400: Blocks is not linked to upstream content.
[BETA] Establish or modify a single downstream block's link to upstream content.
PUT /api/v2/contentstore/downstreams/{usage_key_string}
API paths (We will move these into proper api_doc_tools annotations soon
https://github.com/openedx/edx-platform/issues/35653):
/api/v2/contentstore/downstreams/{usage_key_string}
GET: Inspect a single downstream block's link to upstream content.
200: Upstream link details successfully fetched. Returns UpstreamLink (may contain an error_message).
404: Downstream block not found or user lacks permission to edit it.
DELETE: Sever a single downstream block's link to upstream content.
204: Block successfully unlinked (or it wasn't linked in the first place). No response body.
404: Downstream block not found or user lacks permission to edit it.
PUT: Establish or modify a single downstream block's link to upstream content. An authoring client could use this
endpoint to add library content in a two-step process, specifically: (1) add a blank block to a course, then
(2) link it to a content library with ?sync=True.
REQUEST BODY: {
"upstream_ref": str, // reference to upstream block (eg, library block usage key)
"sync": bool, // whether to sync in upstream content (False by default)
}
200: Block's upstream link successfully edited (and synced, if requested).
404: Block not found, OR user lacks permission to edit block
200: Downstream block's upstream link successfully edited (and synced, if requested). Returns UpstreamLink.
400: upstream_ref is malformed, missing, or inaccessible.
400: Upstream block does not support syncing.
400: Content at upstream_ref does not support syncing.
404: Downstream block not found or user lacks permission to edit it.
/api/v2/contentstore/downstreams/{usage_key_string}/sync
[BETA] Sync a downstream block with upstream content.
POST /api/v2/contentstore/downstreams/{usage_key_string}/sync
200: Block successfully synced with upstream content.
404: Block is not linked to upstream, OR block not found, OR user lacks permission to edit block.
400: Blocks is not linked to upstream content.
POST: Sync a downstream block with upstream content.
200: Downstream block successfully synced with upstream content.
400: Downstream block is not linked to upstream content.
400: Upstream is malformed, missing, or inaccessible.
400: Upstream block does not support syncing.
404: Downstream block not found or user lacks permission to edit it.
[BETA] Decline an available sync for a downstream block.
DELETE /api/v2/contentstore/downstreams/{usage_key_string}/sync
DELETE: Decline an available sync for a downstream block.
204: Sync successfuly dismissed. No response body.
404: Block not found, OR user lacks permission to edit block.
400: Blocks is not linked to upstream content.
400: Downstream block is not linked to upstream content.
404: Downstream block not found or user lacks permission to edit it.
Schema for 200 responses, except where noted:
{
"upstream_ref": string?
"version_synced": string?,
"version_available": string?,
"version_declined": string?,
"error_message": string?,
"ready_to_sync": Boolean
}
# NOT YET IMPLEMENTED -- Will be needed for full Libraries Relaunch in ~Teak.
/api/v2/contentstore/downstreams
/api/v2/contentstore/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true
GET: List downstream blocks that can be synced, filterable by course or sync-readiness.
200: A paginated list of applicable & accessible downstream blocks. Entries are UpstreamLinks.
Schema for 4XX responses:
UpstreamLink response schema:
{
"developer_message": string?
"upstream_ref": string?
"version_synced": string?,
"version_available": string?,
"version_declined": string?,
"error_message": string?,
"ready_to_sync": Boolean
}
"""
import logging

from django.contrib.auth.models import User # pylint: disable=imported-auth-user
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import UsageKey
Expand All @@ -71,7 +68,8 @@
from xblock.core import XBlock

from cms.lib.xblock.upstream_sync import (
UpstreamLink, sync_from_upstream, decline_sync, BadUpstream, BadDownstream, fetch_customizable_fields
UpstreamLink, UpstreamLinkException, NoUpstream, BadUpstream, BadDownstream,
fetch_customizable_fields, sync_from_upstream, decline_sync, sever_upstream_link
)
from common.djangoapps.student.auth import has_studio_write_access, has_studio_read_access
from openedx.core.lib.api.view_utils import (
Expand All @@ -82,6 +80,9 @@
from xmodule.modulestore.exceptions import ItemNotFoundError


logger = logging.getLogger(__name__)


class _AuthenticatedRequest(Request):
"""
Alias for the `Request` class which tells mypy to assume that `.user` is not an AnonymousUser.
Expand Down Expand Up @@ -117,19 +118,14 @@ def get(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response
Inspect an XBlock's link to upstream content.
"""
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")
return Response(UpstreamLink.try_get_for_block(downstream).to_json())

def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
"""
Edit an XBlock's link to upstream content.
"""
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):
raise ValidationError({"upstream_ref": "value missing"})

# Set `downstream.upstream` so that we can try to sync and/or fetch.
# Note that, if this fails and we raise a 4XX, then we will not call modulstore().update_item,
Expand All @@ -142,22 +138,47 @@ def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response
if sync_param == "true":
sync_from_upstream(downstream=downstream, user=request.user)
else:
# Even if we're not syncing (i.e., updating the downstream's values with the upstream's), we still need
# to fetch the upstream's customizable values and store them as hidden fields on the downstream. This
# ensures that downstream authors can restore defaults based on the upstream.
fetch_customizable_fields(downstream=downstream, user=request.user)
except BadDownstream as exc:
logger.exception(
"'%s' is an invalid downstream; refusing to set its upstream to '%s'",
usage_key_string,
new_upstream_ref,
)
raise ValidationError(str(exc)) from exc
except BadUpstream as exc:
logger.exception(
"'%s' is an invalid upstream reference; refusing to set it as upstream of '%s'",
new_upstream_ref,
usage_key_string,
)
raise ValidationError({"upstream_ref": str(exc)}) from exc
except NoUpstream as exc:
raise ValidationError({"upstream_ref": "value missing"}) from exc
modulestore().update_item(downstream, request.user.id)
link: UpstreamLink = UpstreamLink.get_for_block(downstream) # type: ignore[assignment]
return Response(link.to_json())
# Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the
# upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen.
return Response(UpstreamLink.get_for_block(downstream).to_json())

# def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
# """
# Sever an XBlock's link to upstream content.
# """
# downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
# _ensure_upstream_ref(downstream)
# ....
def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
"""
Sever an XBlock's link to upstream content.
"""
downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
try:
sever_upstream_link(downstream)
except NoUpstream as exc:
logger.exception(
"Tried to DELETE upstream link of '%s', but it wasn't linked to anything in the first place. "
"Will do nothing. ",
usage_key_string,
)
else:
modulestore().update_item(downstream, request.user.id)
return Response(status=204)


@view_auth_classes(is_authenticated=True)
Expand All @@ -171,27 +192,37 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons
Pull latest updates to the block at {usage_key_string} from its linked upstream content.
"""
downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
_ensure_upstream_ref(downstream)
if not downstream.upstream:
raise NotFound(detail=f"Block '{usage_key_string}' is not linked to a library block")
old_version = downstream.upstream_version
try:
sync_from_upstream(downstream, request.user)
except (BadUpstream, BadDownstream) as exc:
except UpstreamLinkException as exc:
logger.exception(
"Could not sync from upstream '%s' to downstream '%s'",
downstream.upstream,
usage_key_string,
)
raise ValidationError(detail=str(exc)) from exc
modulestore().update_item(downstream, request.user.id)
link: UpstreamLink = UpstreamLink.get_for_block(downstream) # type: ignore[assignment]
return Response(link.to_json(), status=200)
# Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the
# upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen.
return Response(UpstreamLink.get_for_block(downstream).to_json())

def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
"""
Decline the latest updates to the block at {usage_key_string}.
"""
downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
_ensure_upstream_ref(downstream)
try:
decline_sync(downstream)
except (BadUpstream, BadDownstream) as exc:
except (NoUpstream, BadUpstream, BadDownstream) as exc:
# This is somewhat unexpected. If the upstream link is missing or invalid, then the downstream author
# shouldn't have been prompted to accept/decline a sync in the first place. Of course, they could have just
# hit the HTTP API anyway, or they could be viewing a Studio page which hasn't been refreshed in a while.
# So, it's a 400, not a 500.
logger.exception(
"Tried to decline a sync to downstream '%s', but the upstream link '%s' is invalid.",
usage_key_string,
downstream.upstream,
)
raise ValidationError(str(exc)) from exc
modulestore().update_item(downstream, request.user.id)
return Response(status=204)
Expand All @@ -202,9 +233,7 @@ def _load_accessible_block(user: User, usage_key_string: str, *, require_write_a
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.
Raises NotFound if usage key is malformed.
Raises NotFound if user lacks access.
Raises NotFound if block does not exist.
Raises NotFound if usage key is malformed, if the user lacks access, or if the block doesn't exist.
"""
not_found = NotFound(detail=f"Block not found or not accessible: {usage_key_string}")
try:
Expand All @@ -220,11 +249,3 @@ def _load_accessible_block(user: User, usage_key_string: str, *, require_write_a
except ItemNotFoundError as exc:
raise not_found from exc
return block


def _ensure_upstream_ref(block: XBlock) -> None:
"""
Raises ValidationError if block is not a downstream block.
"""
if not block.upstream:
raise ValidationError(detail=f"Block '{block.usage_key}' is not linked to a library block")
Loading

0 comments on commit b403256

Please sign in to comment.