Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from django.urls import path

from cms.djangoapps.contentstore.rest_api.v4.views import home
from cms.djangoapps.contentstore.rest_api.v4.views import home, legacy_libraries

app_name = "authoring_v4"

Expand All @@ -12,4 +12,9 @@
home.HomeCoursesViewSet.as_view({"get": "list"}),
name="course_list",
),
path(
"legacy_libraries/",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ADR 0029 cross-PR consistency: Error-format decision says an address below this collection gets the CMS HTML 404 and that "no merged area has one" (a catch-all). #39186, open at the same time, takes the opposite approach for its two collections and adds four named catch-all routes to UnknownRouteView. Having one approach per area would leave the Authoring API with JSON 404s in some places and HTML 404s in others. It may be worth agreeing on one fix for all /api/authoring/ addresses (the CMS handler404 returning the envelope for /api/ paths, as the Follow-ups suggest) and referencing it from both PRs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the inconsistency with #39186. Added a Follow-up calling out that the two PRs should land on one consistent approach for all /api/authoring/ addresses before either merges.

legacy_libraries.LegacyLibraryViewSet.as_view({"get": "list"}),
name="legacy_library_list",
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
"""Service layer for the v4 legacy library list endpoint."""

from cms.djangoapps.contentstore.toggles import libraries_v1_enabled
from cms.djangoapps.contentstore.views.course import _accessible_libraries_iter, format_library_for_view
from cms.djangoapps.modulestore_migrator import api as migrator_api


def _list_order(library):
"""Sort key: display name ignoring case, then library key, so that pages are stable."""
return str(library.display_name).casefold(), str(library.location.library_key)


def list_legacy_libraries(user, org=None, is_migrated=None):
"""
Return the legacy libraries ``user`` can read that match the filters, in list order.

Returns ``(libraries, migrations)``. ``libraries`` holds library summaries,
ordered by display name ignoring case and then by library key.
``migrations`` maps a library key to its forwarding migration, or None,
for each library whose migration had to be read to apply ``is_migrated``.
It is empty when ``is_migrated`` is not given, so that a caller returning
one page reads migrations for that page only.

The filters only narrow what ``user`` can read: ``org`` matches the
organization in the library key exactly, and ``is_migrated`` whether the
library forwards to a content library. Nothing is listed while legacy
libraries are disabled.
"""
if not libraries_v1_enabled():
return [], {}
libraries = [
library for library in _accessible_libraries_iter(user)
if org is None or library.location.library_key.org == org
]
migrations = {}
if is_migrated is not None:
migrations = {library.id: migrator_api.get_forwarding(library.id) for library in libraries}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ADR 0032: with is_migrated, every visible library's migration is read before pagination, one get_forwarding query each, so a page's cost grows with the total number of libraries rather than page_size. That's disclosed, pinned by test_filtering_on_migration_reads_one_migration_per_visible_library, and still better than v3, which always does it. Even so, a single ModulestoreSource.objects.filter(key__in=...) read with the same select_related, through a small bulk helper beside get_forwarding in modulestore_migrator/api/read_api.py, would make it constant. If that helper is out of scope, could it be added to Follow-ups?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point — added to Follow-ups. A bulk helper at modulestore_migrator/api/read_api.py would make the is_migrated filtering cost constant. Noted it needs an issue filed too.

libraries = [library for library in libraries if bool(migrations[library.id]) == is_migrated]
libraries.sort(key=_list_order)
return libraries, migrations


def format_legacy_libraries(libraries, request, migrations):
"""
Return one row per library in ``libraries``, as the Studio library listing shows it.

The forwarding migration of a library missing from ``migrations`` is read here.
"""
return [
format_library_for_view(
library,
request,
migration=(
migrations[library.id] if library.id in migrations else migrator_api.get_forwarding(library.id)
),
)
for library in libraries
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
"""Serializers for the legacy library list."""

from rest_framework import serializers

MIGRATION_FIELDS = (
"migrated_to_key",
"migrated_to_title",
"migrated_to_collection_key",
"migrated_to_collection_title",
)


class LegacyLibraryListQuerySerializer(serializers.Serializer):
"""
Query parameters of the legacy library list.

Validate a plain ``dict`` of the query string, not the ``QueryDict``: DRF
reads an absent boolean in form-style input as ``false``, which would
silently filter the list.
"""

org = serializers.CharField(
required=False,
help_text=(
"Return only libraries whose key has exactly this organization, for example `OpenedX`. "
"The match is case-sensitive: `openedx` does not match `OpenedX`."
),
)
is_migrated = serializers.BooleanField(
required=False,
help_text=(
"`true` returns only libraries that have been migrated to a content library they now forward to. "
"`false` returns all others, including libraries copied to a content library without forwarding. "
"Omit it to return both."
),
)


class LegacyLibrarySerializer(serializers.Serializer):
"""
A legacy library the requester can read.

The four `migrated_to_*` fields are present only when `is_migrated` is
true, and the two collection fields are then null when the migration did
not target a collection.
"""

display_name = serializers.CharField(help_text="Display name of the library.")
library_key = serializers.CharField(help_text="Key of the library, for example `library-v1:OpenedX+Lib1`.")
url = serializers.CharField(help_text="Path of the library's page in Studio.")
org = serializers.CharField(help_text="Organization part of the library key.")
number = serializers.CharField(help_text="Library code part of the library key.")
can_edit = serializers.BooleanField(help_text="Whether the requester can edit the library.")
is_migrated = serializers.BooleanField(
help_text="Whether the library has been migrated to a content library that it now forwards to.",
)
migrated_to_key = serializers.CharField(
required=False,
help_text="Key of the content library the library was migrated to.",
)
migrated_to_title = serializers.CharField(
required=False,
help_text="Title of the content library the library was migrated to.",
)
migrated_to_collection_key = serializers.CharField(
required=False,
allow_null=True,
help_text="Key of the collection the library was migrated into, or null if none.",
)
migrated_to_collection_title = serializers.CharField(
required=False,
allow_null=True,
help_text="Title of the collection the library was migrated into, or null if none.",
)

def to_representation(self, instance):
data = super().to_representation(instance)
# A nullable field reads a key missing from the row as null, so the
# collection fields are removed again for a library that is not migrated.
if not data["is_migrated"]:
for field_name in MIGRATION_FIELDS:
data.pop(field_name, None)
return data
135 changes: 135 additions & 0 deletions cms/djangoapps/contentstore/rest_api/v4/views/legacy_libraries.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
"""API views for the legacy library list."""

from drf_spectacular.utils import OpenApiResponse, extend_schema
from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication
from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser
from edx_rest_framework_extensions.errors import ErrorResponseSerializer
from edx_rest_framework_extensions.mixins import StandardizedErrorMixin
from edx_rest_framework_extensions.paginators import DefaultPagination, IterablePaginationMixin
from rest_framework import viewsets
from rest_framework.permissions import IsAuthenticated

from cms.djangoapps.contentstore.rest_api.v4.legacy_libraries_service import (
format_legacy_libraries,
list_legacy_libraries,
)
from cms.djangoapps.contentstore.rest_api.v4.serializers.legacy_libraries import (
LegacyLibraryListQuerySerializer,
LegacyLibrarySerializer,
)


class LegacyLibraryPagination(DefaultPagination):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ADR 0032 consistency: the description calls this the second local copy after CourseTextbookPagination, but the same v4/ URLconf already has a third, HomePageCoursesPaginator in v4/views/home.py. That one doesn't fix the schema. So the two list operations under /api/authoring/v4/ return the same seven-key envelope but publish different page schemas: courses/ with four keys, legacy_libraries/ with seven. This class and CourseTextbookPagination also differ already, since only this one documents the page_size default and cap.

Could the schema fix live in one shared paginator (e.g. a rest_api/pagination.py that both this view and textbooks use), so there's one class to delete when the library fix lands? Aligning HomePageCoursesPaginator may be out of scope, but one shared class would at least stop the copies multiplying.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point — added a Follow-up for a shared rest_api/pagination.py that both this view and textbooks (and home courses) can use, so they all delete their subclass at once when the library fix lands. Aligning HomePageCoursesPaginator's schema is noted there too.

"""
The standard page envelope, with a schema that describes all seven of its keys.

``DefaultPagination`` returns seven keys but inherits a response schema
that documents only four, so generated clients would drop ``num_pages``,
``current_page`` and ``start``. The inherited ``page_size`` description
also leaves out the default and the cap.

Delete this subclass and use ``DefaultPagination`` directly once
edx-drf-extensions documents all seven keys in
``DefaultPagination.get_paginated_response_schema`` and the page size
limits in its ``page_size`` description.
"""

page_size_query_description = (
"Number of libraries per page. Defaults to 10; a value above 100 returns 100 libraries per page."
)

def get_paginated_response_schema(self, schema):
return {
"type": "object",
"required": ["count", "num_pages", "current_page", "start", "next", "previous", "results"],
"properties": {
"count": {"type": "integer", "description": "Total number of items."},
"num_pages": {"type": "integer", "description": "Total number of pages."},
"current_page": {"type": "integer", "description": "Number of this page, starting at 1."},
"start": {"type": "integer", "description": "Zero-based index of the first item on this page."},
"next": {
"type": "string",
"format": "uri",
"nullable": True,
"description": "Address of the next page, or null on the last page.",
},
"previous": {
"type": "string",
"format": "uri",
"nullable": True,
"description": "Address of the previous page, or null on the first page.",
},
"results": schema,
},
}


@extend_schema(tags=["openedx-platform-sdk"])
class LegacyLibraryViewSet(StandardizedErrorMixin, IterablePaginationMixin, viewsets.ViewSet):
"""
The legacy libraries the requester can read, one page at a time.

Any authenticated user may call it; each library is listed only if the
requester has Studio read access to it, and ``can_edit`` says whether they
also have write access. Global staff see every legacy library. The list is
empty while legacy libraries are disabled. Listing changes nothing.

A plain ``ViewSet``: legacy libraries are read from the modulestore, not
from database rows.
"""

# The Studio library lists this one replaces accept sessions of inactive
# users, so this one does too and their callers keep the same access.
# Inactive users fail every library role check: of them, only global staff
# are shown any library.
authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser)
permission_classes = (IsAuthenticated,)
serializer_class = LegacyLibrarySerializer
pagination_class = LegacyLibraryPagination

def get_serializer(self, *args, **kwargs):
"""Instantiate the legacy library serializer declared on this viewset."""
return self.serializer_class(*args, **kwargs)

@extend_schema(
summary="List legacy libraries",
description=(
"Returns one page of the legacy libraries the requester can read, ordered by display name "
"ignoring case and then by library key. Content libraries, which replace legacy libraries, "
"are listed by `/api/libraries/v2/`."
),
parameters=[LegacyLibraryListQuerySerializer],
responses={
200: OpenApiResponse(
response=LegacyLibrarySerializer,
description="A page of legacy libraries.",
),
400: OpenApiResponse(
response=ErrorResponseSerializer,
description="A filter value is empty or not valid.",
),
401: OpenApiResponse(
response=ErrorResponseSerializer,
description="The requester is not authenticated.",
),
404: OpenApiResponse(
response=ErrorResponseSerializer,
description=(
"The requested page does not exist, for example a page past the last one "
"or a `page` that is not a number."
),
),
},
)
def list(self, request):
"""Return one page of the legacy libraries the requester can read."""
query = LegacyLibraryListQuerySerializer(data=request.query_params.dict())
query.is_valid(raise_exception=True)
libraries, migrations = list_legacy_libraries(request.user, **query.validated_data)
return self.paginate_iterable(
request,
libraries,
serialize=lambda page: self.get_serializer(
format_legacy_libraries(page, request, migrations), many=True,
).data,
)
Loading
Loading