Repository navigation
feat: apply FC-0118 ADRs to legacy libraries API #39238
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ADR 0032: with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
| 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): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ADR 0032 consistency: the description calls this the second local copy after Could the schema fix live in one shared paginator (e.g. a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
| ) | ||
There was a problem hiding this comment.
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 CMShandler404returning the envelope for/api/paths, as the Follow-ups suggest) and referencing it from both PRs.There was a problem hiding this comment.
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.