feat: apply FC-0118 ADRs to course content APIs - #39157
Faraz32123 wants to merge 1 commit into
Conversation
2416efb to
ac5b339
Compare
Add a conforming v2 surface for the textbooks and video-settings
endpoints at /api/authoring/v2/courses/{course_key}/{textbooks,video_settings}/.
The video endpoint's read path no longer reconciles stale upload status,
fixing a runtime-verified write-on-GET bug; that reconciliation stays
on the unchanged legacy route. The v1 routes are untouched and keep
resolving exactly as they do today.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ac5b339 to
5bd8e9d
Compare
| 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', | ||
| # Used for tag extraction only. Paths are emitted in full so they resolve | ||
| # against the service-root SERVERS below. | ||
| 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', |
There was a problem hiding this comment.
cms/envs/common.py now has its own SPECTACULAR_SETTINGS (#39025), and that's what cms.envs.development uses to generate the committed docs/cms-openapi.yaml. It still has SCHEMA_PATH_PREFIX_TRIM, so after this merges that file mixes trimmed paths (/v1/textbooks/{course_id}) with full ones (/api/authoring/v2/courses/{course_key}/textbooks/) and has no servers. I merged this branch onto master and generated it to check. Please drop SCHEMA_PATH_PREFIX_TRIM and widen SCHEMA_PATH_PREFIX in common.py the same way when you rebase.
| Without them, a wrong verb or an unsatisfiable ``Accept`` header would be | ||
| reported to the client as an internal server error at a 4xx status. | ||
| """ | ||
| register_error_type(MethodNotAllowed, "method-not-allowed", "Method Not Allowed") |
There was a problem hiding this comment.
The registry in edx_rest_framework_extensions.errors is process-wide, so this changes the error type of every standardized Studio endpoint, not only these two. On this branch POST /api/contentstore/v3/home/ answers 405 with .../errors/method-not-allowed and a GET with Accept: application/xml answers 406 with .../errors/not-acceptable. Master returns .../errors/internal for both. #39102 left these unregistered for that reason and lists the fix as an edx-drf-extensions follow-up. Do you want to do the same here?
| class TextbookChapterSerializer(serializers.Serializer): | ||
| """One chapter of a course PDF textbook.""" | ||
|
|
||
| title = serializers.CharField(help_text="Chapter title shown in the textbook's table of contents.") |
There was a problem hiding this comment.
A chapter missing title or url still fails the whole page. I stored chapters: [{"title": "c"}] and got a 500 with the internal-error envelope, same as legacy, and a stored chapters: null comes back as null even though the schema says array. Is a malformed chapter out of scope here, or do you want the chapter fields to be as forgiving as id and tab_title?
| return video_settings | ||
|
|
||
|
|
||
| def get_previous_uploads(course): |
There was a problem hiding this comment.
#39102 adds a paginated videos/ collection that lists the same uploads with the same write-free status computation, so the status logic exists twice in flight. What needs previous_uploads on this endpoint once that lands? If there is a reason to keep this capability, I think it should share one implementation with that PR, and ?view=full is unpaginated.
| from cms.djangoapps.contentstore.views.permissions import CanViewPagesAndResources | ||
|
|
||
|
|
||
| class CourseTextbookPagination(DefaultPagination): |
There was a problem hiding this comment.
nit: Add a note here that this subclass can go once edx-drf-extensions documents all seven keys in DefaultPagination.get_paginated_response_schema, with the issue link if there is one.
| child=serializers.CharField(), | ||
| help_text="Accepted video file extensions.", | ||
| ) | ||
| video_upload_max_file_size = serializers.IntegerField(help_text="Largest accepted video file, in gigabytes.") |
There was a problem hiding this comment.
nit: The description says the response differs from v1 only by pagination_context and the renamed transcript field. video_upload_max_file_size is also a number here where v1 sends a string, and status is always English. Worth listing for people moving off v1.
Related Issue: #39070
Standardizes the two v1 course-content endpoints (
CourseTextbooksView,CourseVideosViewincms/djangoapps/contentstore/rest_api/v1/views/{textbooks,videos}.py) onto a new, additive v2 surface. v1 is completely untouched — both legacy files have a literal zero-line diff against master, and every existing address still resolves to the same view with the same body.Only the two endpoints listed in #39070 are migrated.
New addresses
GET /api/contentstore/v1/textbooks/{course_id}GET /api/authoring/v2/courses/{course_key}/textbooks/GET /api/contentstore/v1/videos/{course_id}GET /api/authoring/v2/courses/{course_key}/video_settings/Note on the video address.
CourseVideosViewis not in issue #39061 (Video Management) or issue #39060 (Course Videos, PR #39102) — it belongs here, in #39070, per the issue's own table. PR #39102's description currently attributes this payload to #39061; that attribution is incorrect.#39061's approved plan explicitly excludesCourseVideosViewfrom its scope for this reason. The new address,video_settings/, does not collide with #39102'svideos/collection or#39061's plannedvideo_usages//video_archives/collections.No deprecation markers are added to the legacy views or the schema in this pass — the two v1 files are untouched, and no
deprecated: trueis set on their operations.Two things worth calling out
The video endpoint fixes a runtime-verified write-on-GET bug. The legacy
GET .../videos/{course_id}(andHEAD) can flip a stuck video upload's status fromuploadtoupload_failed, viaVideo.save(), as a side effect of computing the display status — a genuine ADR 0030 violation, confirmed by a live probe (a 30-hour-olduploadrow flipped after a singleHEADrequest). The new endpoint's read path never invokes that reconciliation logic; a dedicated test proves zero writes (SQL-capture, signal-spy, and event-spy, not just a mock assertion), and the legacy route's behavior is left completely unchanged for existing callers.The video endpoint is light by default.
GET .../video_settings/returns only the course's upload/transcript settings by default;?view=fulladdsprevious_uploads, matching ADR 0036's "the heavy payload is never the default" guidance.previous_uploadsitems are field-for-field identical to the legacy shape (including the non-ISOcreatedstring format), because a known consumer (enterprise-catalog) persists every field of every item as-is — the only corrections are dropping the always-emptypagination_contextand fixing a typo'd field name that has no reader.ADR compliance
help_texton every field, explicitMeta.ref_name.video_settingsdocuments both its light and full (?view=full) response shapes.has_studio_read_accesscheck — no upload-pipeline gate is added, since this is a read of already-uploaded state.@extend_schemaon both actions,tags=["openedx-platform-sdk"]on both, every 4xx documented against the standardized envelope.ViewSets, registered via explicitpath(), matching the pattern in-flight sibling PRs use for the same converter-based routing.StandardizedErrorMixinfirst in the MRO. A stored textbook missingid/chapters(an unhandled 500 today, reachable from ordinary imported courses) becomes a 200 withid: null/chapters: []— a fix to bad stored data, not a shape change, since a 4xx would incorrectly blame the client.previous_uploadsis opt-in via?view=fullrather than the default. Textbooks' list is flat and thin.contentstore/utils.py(category 1, byte-identical output verified across 16 cases)./api/authoring/v2/courses/{course_key}/..., course key via path converter, trailing slash, version-free URL names.get_queryset()for the mixin to wrap.Compatibility evidence
textbooks,course_videos).rest_api/v1/views/tests/{test_textbooks,test_videos}.pyandviews/tests/{test_textbooks,test_textbooks_permissions,test_videos}.py— all five files untouched in this diff.v1/views/textbooks.pyandv1/views/videos.pyare not modified at all (literal zero-line diff).Gate results
On the schema-compare failure. Removing
SCHEMA_PATH_PREFIX_TRIMre-keys every existing path from its trimmed form to its full address, which the differ reports as breaking. Reconciled mechanically: base 57 paths → head 59; 0 removed paths fail to reappear at their full address; 2 genuinely new paths (this pass's two endpoints); zero operations differ in any key anywhere in the schema — because this pass adds no deprecation markers, every existing operation is completely untouched.Known gaps
cms/djangoapps/contentstore/rest_api/v2/authoring_urls.pymay be extended by a sibling in-flight area (Video Management, [API] Video Management #39071) once both land; the two additive route sets merge without conflict.🤖 Generated with Claude Code