Skip to content

feat: serve course advanced settings from authoring v1 - #39303

Draft
taimoor-ahmed-1 wants to merge 10 commits into
openedx:masterfrom
edly-io:feat/standardize-advanced-settings-api
Draft

taimoor-ahmed-1 wants to merge 10 commits into
openedx:masterfrom
edly-io:feat/standardize-advanced-settings-api

Conversation

@taimoor-ahmed-1

@taimoor-ahmed-1 taimoor-ahmed-1 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Part of #39059

Standardizes the advanced settings resource of the Course Management area (#39059, part of #38137). This is the first of eight resource-group PRs for the area. The others (course optimizer, custom pages and tabs, file assets, video transcripts and images, course settings, group configurations, team members) follow separately.

GET /api/authoring/v1/courses/{course_key}/advanced_settings/ and PATCH on the same address (URL name authoring_v1:course_advanced_settings) replace GET/PATCH /api/contentstore/v0/advanced_settings/{course_id}. v0 stays mounted and unchanged. It is marked deprecated: true in the CMS schema and in its docstrings.

It applies ADRs 0025–0038 and OEP-69.

Representations. Without view (or with view=minimal), the GET returns the settings course authors edit: v0's answer with fetch_all=0, which is what the Advanced Settings page requests. view=full returns every settings field, which is v0's default. fields (comma-separated setting names, applied with the library's project()) narrows either view and replaces v0's filter_fields. PATCH takes {<setting>: {"value": ...}} and returns the editable set, as v0 does.

Why a new version and not a fix in place. Each of these changes the contract for v0's callers, so they ride v1 (ADR 0037):

  • errors use the standard envelope, and the Advanced Settings MFE page parses v0's error body;
  • Bearer tokens are refused;
  • the address gains a trailing slash and a converter-checked course key;
  • the default read is the editable set rather than every field;
  • a refused value is reported with the course rule's own message (team size and team-set rules, proctoring rules, missing escalation contact) where v0 wrote one, and otherwise with one fixed message in place of exception text.

Changes

Commit Endpoint(s) Legacy behaviour kept New behaviour
feat: add course existence and advanced settings permission classes — check_course_advanced_settings_access and verify_course_exists unchanged IsExistingCourse (404 for an unknown, version-only or library key), CanViewAdvancedSettings and CanManageAdvancedSettings, which make the same check_course_advanced_settings_access(read|write) call v0 makes
feat: serve course advanced settings from authoring v1 GET/PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/ (new) v0 route, view and URL name unchanged The endpoint above, its service and serializers, and its tests
docs: mark the v0 advanced settings deprecated in favour of authoring v1 GET/PATCH /api/contentstore/v0/advanced_settings/{course_id} Route, view and responses unchanged deprecated: true in the schema (one exact SUPERSEDED_PATHS entry); the docstring notes it added are reverted below
test: import the authoring URLconf inside the test that needs it — — The URLconf was imported at module level, before the path converters were registered, so test collection failed; also wraps a line pylint reported
test: align the advanced settings tests with the platform's behaviour — — From the first CI run: superusers are admitted (as on v0); the proctoring tests enable proctored exams; the wiring test ignores the class the JWT middleware appends
test: share the authoring v1 test helpers — — The recorder, comparison, schema and client helpers move unchanged into v1/views/tests/helpers.py for the other authoring v1 tests to reuse
docs: describe what an inactive session and a superuser are allowed — — The authentication comment says what an inactive session gets; the superuser test gives the real reason a superuser is allowed (saving one sets is_staff)
docs: remove the deprecation notes from the v0 advanced settings docstrings the two v0 operations v0 file back to its base content A view docstring is the published description of the v0 operation, so the notes changed the legacy schema text and the OPTIONS body; SUPERSEDED_PATHS and the #39128 entry carry the deprecation
fix: return the course rules' own messages for refused advanced settings PATCH — A message matching one of CourseMetadata's rules (team size limits, duplicate ids, team-set problems, the three proctoring rules, missing escalation contact) is returned as written; anything else (exception text from converting a value) is logged and replaced by the fixed message. Declares and tests OPTIONS and PUT on an unknown course; removes an empty test section header
docs: describe what an inactive session and a superuser are allowed — — The authentication comment says what an inactive session gets instead of where the classes came from; the superuser test is renamed and gives the real reason a superuser is allowed (saving one sets is_staff)

ADRs applied

  • 0025 serializers — separate output, request and query serializers with help_text on every field and .v1 component names; the response published as an object keyed by setting name
  • 0026 permissions — IsAuthenticated, IsExistingCourse, then a per-action class making the same read/write check as v0; unmapped actions are refused by construction; deny matrix with policy permissions off and on
  • 0027 schema / docs — @extend_schema on both actions with summary, description, view/fields parameters, examples and every 4xx as ErrorResponse; both tagged openedx-platform-sdk
  • 0028 viewsets — applied except router registration: a ViewSet backed by a service module, mounted with path(...) and the shared course_key converter, as every route in authoring_urls.py is
  • 0029 errors — partial: the envelope on every response this view produces, with fixed messages; malformed JSON, 405, 406 and 415 are typed internal until edx-drf-extensions catalogues them, and a malformed key gets the CMS HTML 404 at routing (both pinned by tests)
  • 0030 GET idempotence — met for this code (no write, signal or event on GET or HEAD), with one inherited write outside it: openedx-authz inserts its PolicyCacheControl row on the first permission check against a database that lacks it, as on v0
  • 0031 merged endpoints — n/a: GET and PATCH on one resource are standard verbs, not an action-URL family
  • 0032 pagination — n/a: a single object whose size is fixed by the course's settings fields, not a list
  • 0033 filtering — n/a: no list; view and fields select a representation and are validated by a serializer
  • 0034 authentication — (JwtAuthentication, SessionAuthenticationAllowInactiveUser) declared with its reason, as ADR 0034 requires for an endpoint that used SessionAuthenticationAllowInactiveUser; Bearer dropped
  • 0035 MFE config — n/a: per-course authoring data, not site or front-end configuration
  • 0036 nested JSON — the full field set is opt-in with view=full; fields projects with the library helper
  • 0037 versioning — partial: new v1 module, v0 not edited, deprecated: true in the schema through SUPERSEDED_PATHS; the DEPR entry on #39128 is posted; the removal follows [DEPR]: Contentstore and enrollment API versions superseded by FC-0118 #39128's window rather than a named release
  • 0038 URL structure — /api/authoring/v1/courses/{course_key}/advanced_settings/: domain API name, one-per-course child of courses, shared converter, trailing slash, version-free URL name; the v0 name is kept
  • OEP-66 queryset scoping — n/a as a mechanism: one object read from the modulestore with no get_queryset(); fields and view only narrow what the caller may already read
  • OEP-69 conventions — the decisions above as recorded, with the 0028, 0029, 0030 and 0037 departures stated; no governance identifiers in code or schema text

Compliance matrix

Decision Status How it is met, or the concrete reason it is excluded Evidence (test / gate)
0025 met CourseAdvancedSettingsSerializer (an object keyed by setting name, each an AdvancedSetting.v1 with value, display_name, help, deprecated, hide_on_enabled_publisher), CourseAdvancedSettingsUpdateSerializer (request) and AdvancedSettingsQuerySerializer. The map shape is published through a small OpenApiSerializerExtension: drf-spectacular 0.30 cannot publish a named map-of-serializer component otherwise, and edx-drf-extensions has nothing equivalent. value is untyped because a setting's type depends on the field. v0's per-field typed schema also omitted the inherited settings that the endpoint returns AdvancedSettingsSchemaTest (test_every_member_is_described, test_the_settings_are_an_object_keyed_by_setting_name, test_the_update_body_is_json_settings_each_with_a_value), AdvancedSettingsReadTest.test_the_body_matches_the_published_schema; hygiene H9
0026 met Every v0 check has a v1 home (Authorization below). The per-action classes make the same check_course_advanced_settings_access(user, course_key, "read"|"write") call, so both the policy-permission branch and the legacy Studio-access branch are inherited unchanged. No DISABLE_ADVANCED_SETTINGS gate is added: v0's REST endpoint has none, and Pages & Resources depends on this endpoint. An action missing from the per-action map raises ImproperlyConfigured rather than running with login only AdvancedSettingsAccessLegacyPermissionsTest, AdvancedSettingsAccessAuthzTest, AdvancedSettingsSiteRestriction{LegacyPermissions,Authz}Test, AdvancedSettingsPermissionWiringTest, AdvancedSettingsProctoringProviderTest; each access case asserts v0's answer too
0027 met Summary, description, view enum (minimal, full), fields, examples, 200/400/401/403/404 with ErrorResponse. Operation IDs v1_courses_advanced_settings_retrieve / _partial_update. Both tagged openedx-platform-sdk; the deprecated v0 operations lose the tag through untag_deprecated_operations AdvancedSettingsSchemaTest (incl. test_the_v0_operations_are_deprecated_and_left_out_of_the_sdk, test_only_superseded_or_migrated_addresses_are_deprecated)
0028 applied except router registration CourseAdvancedSettingsViewSet(StandardizedErrorMixin, viewsets.ViewSet) with retrieve and partial_update over v1/advanced_settings_service.py. It is a plain ViewSet because the data path is CourseMetadata over the modulestore. Deviation: mounted with path("courses/<course_key:course_key>/advanced_settings/", ...as_view({...})), because a router's regex routes cannot use the shared path converters AdvancedSettingsRoutingTest.test_the_address; AdvancedSettingsQueryCount{LegacyPermissions,Authz}Test
0029 partial StandardizedErrorMixin first. 401 authn, 403 authz, 404 not-found (unknown, version-only or library course key; a course the modulestore does not return), 400 validation with errors keyed by setting name. A refused value is reported with the course rule's own message where one exists (the team size limits, duplicate team ids, the team-set problems, the three proctoring-provider rules, the missing escalation contact: v1 recognises the rules' wording and returns it as written, so authors keep the reason the authoring app shows); any other explanation, such as exception text from converting the value, is logged with the course key and setting name and replaced by INVALID_SETTING_MESSAGE. If a rule's wording changes, v1 falls back to the fixed message and AdvancedSettingsRuleMessagesTest goes red. Tab-save and unexpected failures return a fixed message. Gaps pinned by tests that flip when fixed: malformed JSON (400), 405, 406 and 415 are typed internal (edx-drf-extensions 10.9.0 does not catalogue them), and a malformed course key gets the CMS HTML 404 at routing AdvancedSettingsUpdateTest (test_a_refused_value_is_reported_with_a_fixed_message_and_nothing_is_stored, test_each_message_is_listed_once_per_setting, the tab-save and unexpected-failure tests); AdvancedSettingsRuleMessagesTest (each rule's text reaches v0 and v1 identically; exception text does not); AdvancedSettingsRuleMessageCheckTest (no database: the real validators' output is recognised, exception-like text is not); AdvancedSettingsRoutingTest (test_a_malformed_json_body_is_a_400_typed_internal, test_an_unsupported_media_type_is_a_415_typed_internal, test_other_methods_are_not_allowed, test_unsupported_accept_is_refused_with_406, test_an_unparsable_or_slash_form_key_is_an_html_404)
0030 met for this code; one inherited write GET and HEAD read the course and its settings fields only. On every read (default, view=full, fields), with policy permissions off and on, nothing is written to SQL or Mongo and no Django signal or Open edX event is sent, including callbacks run on commit. The exception is the first read on a database without openedx-authz's PolicyCacheControl row, which inserts it (one INSERT, its pre_save/post_save). That write belongs to the permission library, happens on v0 too, and is allow-listed by name AdvancedSettingsReadSideEffects{LegacyPermissions,Authz}Test (test_reads_have_no_side_effects, test_the_v0_read_makes_the_same_first_lookup_write, test_the_recorder_sees_writes_signals_and_modulestore_updates, incl. an on-commit case)
0031 excluded GET and PATCH on one resource are standard verbs —
0032 excluded A single object; its size is set by the course block's settings fields, not by course data —
0033 excluded No list. view is enum-validated. fields is validated by a serializer. v0's fetch_all and filter_fields are not carried over and are ignored on v1 AdvancedSettingsReadTest.test_the_v0_query_parameters_are_ignored
0034 met with declared deviation (JwtAuthentication, SessionAuthenticationAllowInactiveUser), commented at the declaration: an inactive account's browser session stays authenticated, so the course permission decides: staff status admits it, and its roles count only on courses where policy-based permissions are on. Bearer is dropped. This matches #39100, #39157 and #39227; #39229/#39274 use the platform default on /api/authoring/v2/, so the authoring mounts are not yet uniform AdvancedSettingsAuthenticationTest (test_a_bearer_token_is_refused, the JWT and inactive-session tests); hygiene H4
0035 excluded Not configuration; per-course, per-user —
0036 met Thin by default (the editable set); every field only with view=full; fields projects with edx_rest_framework_extensions.shaping.project AdvancedSettingsReadTest (view and fields tests), _ParityMixin.test_equivalent_reads_return_identical_bodies
0037 partial New module in v1/. v0 not edited: the file is identical to the base. deprecated: true through one exact SUPERSEDED_PATHS entry; no runtime header (frozen route). DEPR: entry on #39128; the removal follows #39128's window versions gate; CmsMarkSupersededPathsTest; AdvancedSettingsSchemaTest.test_the_v0_operations_are_deprecated_and_left_out_of_the_sdk
0038 met authoring API name, courses/{course_key}/advanced_settings/ (a one-per-course child), course_key converter, trailing slash, course_advanced_settings AdvancedSettingsRoutingTest (test_the_address, test_the_address_without_its_trailing_slash_redirects_to_it, test_v0_still_takes_a_slash_separated_key_that_v1_does_not)
OEP-66 mechanism excluded, layering met (1) endpoint access: the permission classes; (2) record visibility: one course's settings, as on v0; (3) user selection: view and fields narrow within what the caller may read and cannot widen it access tests above; AdvancedSettingsReadTest fields tests
OEP-69 met The decisions above, with the departures stated in their rows. No ADR/OEP/PR identifiers or narration in code or schema text hygiene gate PASS (0 FAIL, 0 WARN)

Backward compatibility

  • v0 is unchanged. v0/views/advanced_settings.py is not in the diff. Its route, view and URL name are untouched, and v0's own tests are not edited.

  • Parity. v1 is compared with v0 through the Django test client, with policy permissions off and on. Equivalent reads return identical bodies: no view matches v0 fetch_all=0, view=full matches v0's default, and fields matches filter_fields. A valid PATCH stores the same values and returns the same body. The only declared body differences, verbatim from the test module:

    INTENTIONAL_DIFFERENCES = [
        ("type", "errors use the standardized envelope"),
        ("title", "errors use the standardized envelope"),
        ("status", "errors use the standardized envelope"),
        ("detail", "errors use the standardized envelope"),
        ("errors", "a refused update maps the name of each refused setting to its messages: a course rule's own "
                   "message as v0 wrote it, or INVALID_SETTING_MESSAGE in place of exception text; v0 lists "
                   "the validator's text under developer_message"),
        ("developer_message", "the v0 error body carries developer_message; the envelope does not"),
        ("error_code", "the v0 not-found body carries error_code; the envelope does not"),
    ]

    The comparison fails both on an undeclared difference and on a declared difference that never occurs.

  • Status and request differences from v0. Each is pinned against v0's own answer:

    Request v0 v1 Test
    course the modulestore returns nothing for 500 404 AdvancedSettingsResponseDifferencesTest.test_a_course_the_modulestore_returns_nothing_for
    PATCH body that is not an object 500 400 …test_an_update_whose_body_is_not_an_object
    PATCH entry without value 500 400 …test_an_update_entry_without_a_value
    unknown view value ignored, 200 400 …test_an_unknown_view
    Bearer token 200 401 …test_a_bearer_token
    slash-separated course key 404 JSON HTML 404 (no route) …test_a_slash_separated_course_key
    address without the trailing slash served 301 to the slashed address AdvancedSettingsRoutingTest.test_the_address_without_its_trailing_slash_redirects_to_it
    refused value 400, validator text (a rule's message or exception text) 400, the rule's message as written, or the fixed message in place of exception text AdvancedSettingsRuleMessagesTest; AdvancedSettingsUpdateTest.test_a_refused_value_is_reported_with_a_fixed_message_and_nothing_is_stored
    OPTIONS on an unknown course 200 404 test_options_on_an_unknown_course
    PUT on an unknown course 405 404 test_put_on_an_unknown_course
    default read every field editable set (view=full for every field) _ParityMixin.test_the_same_parameters_return_different_settings

    Malformed entries for settings authors do not edit are ignored on both versions (test_a_malformed_entry_of_a_setting_authors_do_not_edit_is_ignored_by_both). The 401 → 404 → 403 order is the same as v0's.

  • Query counts. For a GET, v1 runs no more queries than v0, measured in the same test. No literal baseline was measured, because no platform environment was available.

  • Schema. Two new operations, and deprecated: true on the two v0 operations. docs/cms-openapi.yaml is not edited; the generate_openapi_schemas workflow regenerates it.

In-place edits

File Category Evidence
cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py strictly opt-in addition: one import, one route no line removed; AdvancedSettingsRoutingTest
cms/djangoapps/contentstore/views/permissions.py (shared) three classes and a message constant added; existing classes unchanged AdvancedSettingsPermissionWiringTest and the access tests
cms/lib/spectacular.py, cms/lib/tests/test_spectacular.py one exact SUPERSEDED_PATHS entry; the comment above the tuple reworded to describe exact-path matching (identical to #39229's wording); the exact-set test and the "unrelated path" example (now /api/contentstore/v0/heartbeat, never deprecated) updated CmsMarkSupersededPathsTest

New files inside the pre-existing v1/: advanced_settings_service.py, serializers/advanced_settings.py, views/advanced_settings.py, views/tests/test_advanced_settings.py. They import nothing from rest_api/v0/.

Error-format decision

Versioned. v0 keeps its developer_message/error_code bodies, its 500s and the validator's text unchanged. v1 uses the envelope. The Advanced Settings page reads v0's error body today (frontend-app-authoring src/advanced-settings/AdvancedSettings.tsx), so the format change rides the new version. Its move to v1 is a separate frontend change, which will show the fixed per-setting message in place of the validator text.

Deprecations

GET/PATCH /api/contentstore/v0/advanced_settings/{course_id} are superseded by GET/PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/. They are deprecated: true in the CMS schema; the v0 view is not edited and sends no runtime header. Tracked on the FC-0118 umbrella DEPR ticket, #39128; this resource's entry is posted there. Window per #39128: removed one named release after the release this ships in.

Migration notes:

  • Call /api/authoring/v1/courses/{course_key}/advanced_settings/, with the trailing slash.
  • v0 fetch_all=0: no view. v0 default (every field): view=full. filter_fields=a,b: fields=a,b (it selects within the chosen view). fetch_all and filter_fields are ignored on v1.
  • A refused value is reported under its setting name: the course rule's message where v0 wrote one, otherwise a fixed message (never exception text).
  • Errors use the standard envelope.
  • Bearer tokens are refused; use JWT or a session.

Authorization

Legacy check Where it lives in v1 Deny-path tests
@view_auth_classes(is_authenticated=True) IsAuthenticated test_anonymous_caller_is_refused_with_401 (both permission modes), incl. on an unknown course
@verify_course_exists() IsExistingCourse (also 404 for version-only and library keys) unknown course, version-only key and library key → 404, before any role check
check_course_advanced_settings_access(user, key, "read") in get CanViewAdvancedSettings, same call no role; staff/instructor of another course; limited staff; policy roles data researcher, beta tester, limited staff; an admin role on another course; legacy staff without a policy role when policy permissions are on
check_course_advanced_settings_access(user, key, "write") in patch CanManageAdvancedSettings, same call as above, plus view-only policy roles (auditor, editor) → 403 on PATCH
CourseMetadata.validate_proctoring_settings: only global staff may change proctoring_provider after the course starts unchanged inside the reused update helper, with the caller's user AdvancedSettingsProctoringProviderTest: course staff → 400 and the value unchanged; global staff → 200; before start → 200 (v0 and v1)

Order: 401, then 404 for a missing course, then 403. Allowed: course staff and instructor, org staff and instructor, global staff, and with policy permissions on, the roles that grant viewing (GET) or managing (PATCH) advanced settings. A superuser is admitted, as on v0: saving a superuser also sets is_staff, so every superuser is global staff (test_a_superuser_is_allowed).

Gate report

Run on the host against upstream-pr/master (87cd3811a4). No platform environment was available, so four gates were skipped. The test module has not been run locally, and CI is its first run.

Gate Result
versions PASS: 1 ALLOWED (v1/authoring_urls.py), 5 ADDED (including v1/views/tests/helpers.py), nothing protected touched (23 shared legacy modules protected)
hygiene PASS (0 FAIL, 0 WARN), 9 files
ruff clean
schema SKIPPED locally (no platform environment); the reviewer's container run of the schema diff showed no removals
urls SKIPPED (no platform environment)
old-tests SKIPPED (no platform environment)
tests SKIPPED locally (no platform environment); CI runs them and was green on 25c5768 (42 checks). The rule-message tests are new in 4f180c5 and run first in CI

Follow-ups

  • SDK: both operations carry openedx-platform-sdk; the next openedx-platform-sdk regeneration gains them.
  • frontend-app-authoring: src/advanced-settings/data/api.ts and src/pages-and-resources/data/api.js can move to the v1 address. Pages & Resources reads v0's default full set, so it needs view=full (or fields=).
  • Unhandled errors lose got_request_exception. On v1 an unhandled error (PATCH teams_configuration with {"value": [1]} crashes v0 too) becomes a 500 envelope without the signal, so Sentry and logging lose the traceback. This comes from StandardizedErrorMixin: an edx-drf-extensions follow-up.
  • edx-drf-extensions: catalogue ParseError, MethodNotAllowed, NotAcceptable and UnsupportedMediaType, and render a field-keyed detail without the Python repr of the error dict. Issue to be filed.
  • openedx-authz PolicyCacheControl: the first permission check on a fresh database inserts the cache row on a read; seeding it by migration would make reads write-free. Issue to be filed.
  • Shared existence check. feat: serve the course outline from authoring v2 #39229 (open) adds the same COURSE_NOT_FOUND_MESSAGE and an inline course-existence check in CanViewCourse. Whichever of the two lands second should build on IsExistingCourse.
  • Authentication across /api/authoring/: v1 and the feat: apply FC-0118 ADRs to course content APIs #39157 v2 views keep SessionAuthenticationAllowInactiveUser; feat: serve the course outline from authoring v2 #39229 and feat: serve course containers from authoring v2 #39274 use the default. One policy for new versions of such endpoints should be agreed.

Taimoor Ahmed added 3 commits October 25, 2026 15:09
The v0 advanced settings view checks course existence with the
@verify_course_exists decorator and access with an inline
check_course_advanced_settings_access call in each handler. The
conforming views need these as permission classes.

IsExistingCourse answers 404 for a course key that names no course run:
an unknown course, a key that names only a version, or a library key.
CanViewAdvancedSettings and CanManageAdvancedSettings make the same
check_course_advanced_settings_access(read|write) call the v0 view
makes, so the policy-permission and legacy Studio-access branches are
unchanged. Existing classes in the module are untouched.
GET/PATCH /api/contentstore/v0/advanced_settings/{course_id} answers
errors with developer_message bodies and some failures with a 500,
accepts Bearer tokens, returns every settings field by default, and
reports a refused value with the field parser's own text, which can be
Python exception text.

GET/PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/ is
its successor. Errors use the standard envelope; a refused value is
reported under its setting name with one fixed message and the original
text is logged. The default read is the set of settings course authors
edit (v0 with fetch_all=0); view=full returns every field and fields
narrows either view. Authentication keeps the session class that admits
inactive users, as the legacy endpoint did; Bearer is dropped. Access is
IsAuthenticated, IsExistingCourse and a per-action class making v0's
read or write check, and an action without a declared permission is
never served. Parity tests compare v1 with v0 through the test client;
the declared differences are the error envelope and the fixed message.
GET/PATCH /api/contentstore/v0/advanced_settings/{course_id} now has a
conforming successor at /api/authoring/v1/courses/{course_key}/advanced_settings/.
Both v0 operations are marked deprecated in the CMS schema through an
exact SUPERSEDED_PATHS entry, and the view's docstrings name the
successor, how each v0 read maps to a v1 read, and the deprecation
ticket. The view itself is unchanged. The schema hook test that used
this path as its example of an unrelated contentstore path now uses the
heartbeat, which is not deprecated.
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 25, 2026
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @taimoor-ahmed-1!

This repository is currently maintained by @openedx/wg-maintenance-openedx-platform-oncall.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.


Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

Taimoor Ahmed added 3 commits October 25, 2026 17:55
Importing cms.djangoapps.contentstore.rest_api.v1.authoring_urls at
module level builds its routes before cms/urls.py registers the
course_key path converter, so collecting the module failed with an
ImproperlyConfigured error. The URL-name test now imports it when it
runs, as the youtube transcript tests do. Also wraps a line pylint
reported as too long.
The first CI run of the module showed three expectations that did not
match the platform:

- The legacy role checks consult the policy engine, which admits
  superusers, so a superuser who is not global staff is served by v0 and
  v1 alike. The test now expects that.
- The proctoring provider field keeps its default unless proctored exams
  are enabled, so the proctoring tests enable them.
- The JWT middleware appends NotJwtRestrictedApplication to the
  permissions of views that accept JWTs, so the wiring test compares only
  the classes the view declares.
The side-effect recorder, the body comparison helpers, the schema
helpers and the client helpers in the advanced settings tests are not
specific to advanced settings. They move unchanged, under public names,
into v1/views/tests/helpers.py so the other authoring v1 endpoint tests
reuse one copy instead of each carrying its own.
The authentication comment narrated where the classes came from; it now
says what an inactive session gets: staff status admits it, and its roles
count only on courses where policy-based permissions are on.

The superuser test claimed to cover a superuser who is not global staff.
Saving a superuser sets is_staff, so the factory user is global staff;
the test is renamed and its docstring gives that reason.
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Oct 27, 2026

@Faraz32123 Faraz32123 left a comment

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.

Reviewed locally in the CMS container: 195 tests pass (v1, v0 and spectacular), CI green, schema diff shows no removals. Permissions, 401/404/403 order, proctoring re-check and success bodies all match v0 caller by caller.

Main points:

  • Deprecation notes in the v0 docstrings (inline).
  • v1 replaces v0's per-setting messages with one fixed message (inline). Declared, but worth a decision before v0 is deprecated toward v1.
  • Minor: for an unknown course, OPTIONS gives 404 (v0 200) and PUT gives 404 (v0 405); not in the differences table.
  • Minor: on v1 an unhandled error (e.g. PATCH teams_configuration with {"value": [1]}, which crashes on v0 too) becomes a 500 envelope without got_request_exception, so Sentry/logging lose the traceback. This comes from StandardizedErrorMixin, so probably one for edx-drf-extensions.
  • PR body gate report is stale: helpers.py is missing from the new files, and tests/schema are listed as SKIPPED though they now pass.

All findings were compared with the previous version's behaviour; anything inherited from it is marked as such.

Comment on lines +27 to +28
Deprecated: use ``GET`` and ``PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/``
instead. See https://github.com/openedx/openedx-platform/issues/39128.

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.

Can we drop this deprecation note? We agreed on #39100 not to add DEPR notes in docstrings. This docstring is the published description of the v0 operation, so it changes the legacy schema text and puts an issue URL into it (which also makes the OEP-69 tick untrue). SUPERSEDED_PATHS plus the #39128 entry already carry the deprecation. Here it also changes the live OPTIONS response body of v0.

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.

Dropped: v0/views/advanced_settings.py is back to its base content (de829c6), so the legacy operation descriptions and OPTIONS bodies are unchanged. SUPERSEDED_PATHS and the #39128 entry carry the deprecation.


# Reported under the name of each setting whose value the course refused. The
# validator's own explanation is logged, not returned.
INVALID_SETTING_MESSAGE = "The value is not valid for this setting."

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.

This also replaces v0's hand-written messages, not just exception text, e.g. "The proctoring provider cannot be modified after a course has started. Contact …", "max_team_size cannot be greater than 500", duplicate team ids, missing escalation email. The authoring MFE shows message per setting (ModalErrorListItem.jsx), so once the page moves to v1 authors lose the reason. Could we keep the curated messages in errors[name] and only hide raw exception text? Same contract shape.

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.

Agreed, and done in 4f180c5. CourseMetadata is frozen, so v1 can't be told at the source which messages are hand-written; instead it recognises the wording of the rules' own messages (the max_team_size limits, duplicate ids, the comma-joined team-set problems, the three proctoring rules including the unknown-provider sentence, and the missing escalation contact, under both settings it is reported on) and returns those as written. Anything else, such as the TypeError from converting a value, is still logged and replaced by the fixed message. Same shape: errors[name] is a list of strings, each listed once, in v0's order. Tests send the same PATCH to v0 and v1 for each rule and check the text is identical, plus a no-database test that the classifier accepts the real validators' output and refuses exception text. If a rule's wording ever changes, v1 falls back to the fixed message and those tests go red, so nothing can leak.

Comment on lines +123 to +125
# ---------------------------------------------------------------------------
# The published schema
# ---------------------------------------------------------------------------

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.

nit: empty section header left over from the move to helpers.py.

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.

Removed (4f180c5). The same commit also declares and tests the two differences from your summary: OPTIONS (v0 200, v1 404) and PUT (v0 405, v1 404) on an unknown course.

Taimoor Ahmed added 3 commits October 31, 2026 14:36
…trings

A view docstring is the published description of the v0 operation, so
the notes changed the legacy schema text and the OPTIONS body. The
deprecation is carried by SUPERSEDED_PATHS and the openedx#39128 entry; the v0
file is back to its base content.
Every refused value got one fixed message, which also replaced the
messages the course's own rules write: the team size limits, duplicate
team ids, the team-set problems, the proctoring provider rules and the
missing escalation contact. The authoring app shows these per setting,
so authors lost the reason. A message matching one of those rules'
wording is now returned as written; anything else, such as the text of
an exception raised while converting the value, is still logged and
replaced by the fixed message, so no exception text reaches a caller.

Also declares and tests OPTIONS and PUT on an unknown course (404 on v1
where v0 gave 200 and 405), and removes an empty section header left in
the test module.
The text it used was a 'type {} is invalid,extra keys: {}' message, since
each {} stands for any text, so the check rightly accepted it. A repeated
team-set problem cannot be a rule message however the text is split.
taimoor-ahmed-1 pushed a commit to edly-io/edx-platform that referenced this pull request Oct 31, 2026
… into feat/standardize-course-settings-api for IsExistingCourse

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Waiting on Author

Development

Successfully merging this pull request may close these issues.

3 participants