feat: add a v2 cohorts API aligned with the REST API ADRs - #39140
Draft
taimoor-ahmed-1 wants to merge 13 commits into
Draft
taimoor-ahmed-1 wants to merge 13 commits into
taimoor-ahmed-1 wants to merge 13 commits into
Conversation
added 11 commits
September 15, 2026 11:55
Applies ADR 0037 to the cohorts API. The v1 cohort, cohort-users and cohort-settings views now emit Deprecation and Link response headers naming their v2 successor, so remaining callers can be identified from access logs before the v1 routes are retired. v1 behaviour is otherwise unchanged; the v2 surface is added in the commits that follow.
Applies ADR 0028 to the cohorts API. The three v1 surfaces (cohort list/detail, cohort membership and course cohort settings) become CohortViewSet, CohortMemberViewSet and CohortSettingsView, with the shared course-key resolution and cohort lookup moved into a mixin instead of being repeated per handler. Explicit serializers replace the hand-built dictionaries, so request payloads are validated rather than read straight off request.data. Not routed yet; authentication and permissions are added before the URLs are registered.
Applies ADR 0034 to the v2 cohorts API. The v1 views authenticate with JwtAuthentication, BearerAuthenticationAllowInactiveUser and SessionAuthenticationAllowInactiveUser; the deprecated Bearer class is left out of v2 entirely. SessionAuthenticationAllowInactiveUser is kept rather than dropped to the platform default, because v1 accepts inactive users over session and switching to the default session class would block callers that work today.
Applies ADR 0026 to the v2 cohorts API. Authorization is declared once as permission_classes on the shared access mixin instead of being repeated as a get_course_with_access call inside every handler, which is what the v1 views do. CanManageCohorts keeps the v1 access rules: course staff and instructors read and write, discussion moderators read only, global staff and superusers do either. It denies an unparseable course key rather than raising, and a malformed key is rejected before the permission check so that a 404 cannot be used to probe for courses.
Applies ADR 0038 to the v2 cohorts API. Each route resolves exactly one address: the trailing slash is required rather than optional, list and detail are separate patterns instead of one pattern with an optional identifier, and the username segment is bounded so it cannot swallow path separators. This avoids three defects carried by the v1 patterns, where 'users?' matches both /user and /users, '(?P<cohort_id>[0-9]+)?' serves both the collection and a member from one route, and '(?P<username>.+)?' captures slashes so /users/bob/extra yields the username 'bob/extra'. The settings route is named course_cohort_settings because the v1 route already holds cohort_settings in this namespace.
Applies ADR 0030 to the v2 cohorts API. Listing cohorts goes through get_course_cohorts(course_id=...) rather than passing the course object, because the course branch of that helper calls migrate_cohort_settings(), which creates a CourseCohortsSettings row and backfills CourseCohort rows. The v1 list endpoint takes that branch, so a GET there writes to the database. Reading cohort settings no longer calls is_course_cohorted(), which lazily migrates and persists settings for the same reason. The stored row is used when present; otherwise the authored value is reported and persisting it is left to a write request. Course existence is still checked, so listing cohorts for a course that does not exist is a 404 rather than an empty collection.
Applies ADR 0032 to the v2 cohorts API. Both list endpoints now return the standard envelope (count, num_pages, current_page, start, next, previous, results) via DefaultPagination, following the same IterablePaginationMixin pattern the enrollments v2 API uses. This avoids the v1 cohort list defect, where the handler paginates the queryset and then discards the paginator, returning a bare JSON array of the first page only. A caller has no count and no next link, so a course with more cohorts than one page silently appears to have just that page. The v1 cohort-users endpoint already returns the envelope, so v2 makes the two list endpoints consistent with each other.
Applies ADR 0033 to the v2 cohorts API. The list endpoint takes ?ordering=name and ?ordering=-name, the field-with-optional-minus convention used across the platform's list endpoints, instead of v1's ?ordering=asc|desc which names a direction rather than a field and so cannot be extended to a second sort key. Orderable fields are declared rather than open, and an unrecognised field is rejected with a message naming the valid ones.
…ints Applies ADR 0029 to the v2 cohorts API. The views use StandardizedErrorMixin and raise DRF exceptions instead of building responses through DeveloperErrorViewMixin.api_error, so every error carries the type, title, status, detail and instance fields rather than the legacy developer_message and error_code pair. Validation failures are raised against the field they concern, so a client is told which input was rejected instead of receiving a bare sentence. This matches the envelope the enrollments v2 API already returns.
Applies ADR 0027 to the v2 cohorts API. Every operation declares a summary, its path and query parameters, its request body and the responses it actually returns, including the 400, 403 and 404 cases, using extend_schema rather than the edx-api-doc-tools decorators that wrap drf-yasg. drf-spectacular is already the platform's DEFAULT_SCHEMA_CLASS, so these endpoints now appear in the generated OpenAPI 3 document with real schemas instead of being inferred.
Regression tests for the v2 cohort endpoints, covering routing, access control, read paths, pagination, ordering, error envelopes and the v1 deprecation headers. Several assert the absence of defects the v1 surface carries: that the members path does not also answer on /user, that a username cannot span path separators, that listing cohorts creates no rows, and that the list endpoint returns a pagination envelope rather than a bare array. Consolidated into one commit rather than split across the ten preceding ones because they exercise a single new surface.
|
Thanks for the pull request, @taimoor-ahmed-1! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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 PRYour 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Faraz32123
reviewed
Sep 24, 2026
Comment on lines
+38
to
+39
| if not user or not user.is_authenticated: | ||
| return False |
Contributor
There was a problem hiding this comment.
shouldn't we use and instead of or?
Comment on lines
+188
to
+189
| class CohortViewSet( | ||
| CohortAPIAccessMixin, |
Contributor
There was a problem hiding this comment.
can we add tags on our new APIs like @extend_schema(tags=["openedx-platform-sdk"]) so that we can filter out the new versioned APIs in our SDK.
added 2 commits
September 25, 2026 11:05
Rewrites the authentication guard as `not (user and user.is_authenticated)`. This is the same condition expressed the way the review asked for: swapping the operator literally would let an AnonymousUser through, because AnonymousUser is truthy, so `not user` is False and the guard would stop rejecting anonymous callers. Tags the three v2 view classes for the SDK so the new versioned endpoints can be filtered out of the generated schema, matching the enrollments v2 surface.
The tests used unittest-style assertions, which ruff rejects under this repository's PT ruleset; every one becomes a plain assert, matching the convention the existing cohorts tests already follow. Import order in the views module is corrected. The username route pattern was written out by hand in two places; it moves to a single shared constant beside the app's other constants. The deviation from the default authentication pair now carries its reason next to the declaration, and the test class whose name collided with the shared-paginator check is renamed for what it asserts.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a v2 cohorts API built to the FC-0118 REST API ADRs, and marks v1 superseded.
Closes #39056.
v1 is left functionally untouched — it only gains
Deprecation/Linkresponse headersnaming its successor — so nothing currently calling it changes behaviour. The ADRs are
applied to the new v2 surface only, one ADR per commit.
New endpoints, all under
/api/cohorts/v2/courses/{course_key}/:cohorts/cohorts/{cohort_id}/cohorts/{cohort_id}/users/cohorts/{cohort_id}/users/{username}/cohort_settings/ADRs applied
3a13f17Deprecation/Link9a37bd06c68f20BearerAuthenticationAllowInactiveUserleft out of v2579960cpermission_classes, not repeated per handlerbdc6389b8a2a04f5debd30620e7d?ordering=name/-namereplaces v1's?ordering=asc|desc3873f79StandardizedErrorMixinenvelope instead ofdeveloper_messagefb00d6fdrf-spectacularADRs not applied
Defects in v1 that v2 does not carry
These are why several of the above are more than style changes. v1 is left as-is here;
each is worth its own fix or DEPR decision.
CohortHandler.getpaginates the queryset, thendiscards the paginator and returns
Response(bare_list). A caller gets the first page as araw array with no
countand nonext, so a course with more cohorts than one page lookslike it has exactly one page.
CohortHandler.get→get_course_cohorts(course)→migrate_cohort_settings()creates aCourseCohortsSettingsrow and backfillsCourseCohortrows. The settings read reaches the same migration viais_course_cohorted().users?answers on/useras well as/users;(?P<cohort_id>[0-9]+)?serves collection and member from one route; and(?P<username>.+)?captures separators, so/users/bob/extrayields the usernamebob/extra.Deviations
/api/cohort/), but/api/cohorts/v2/already hosts the content-groups endpoints; renaming would orphan them.courses/{key}/cohorts/{id}/users/), one past the ADR 0038cap, to match the sibling
v2/courses/{key}/group_configurationsshape.cohort_idis a database primary key, which ADR 0038 rule 9 excludes.CourseUserGrouphas no stable opaque identifier; adding one needs a model change and migration.
course_cohort_settingsbecause v1 already holdscohort_settingsin this namespace.Testing
openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py— 22 tests coveringrouting, access control, read-path side effects, pagination envelopes, ordering, error envelopes
and the v1 deprecation headers.