[FC-0118] docs: add ADR for merging similar endpoints#38262
[FC-0118] docs: add ADR for merging similar endpoints#38262Abdul-Muqadim-Arbisoft wants to merge 1 commit intoopenedx:docs/ADRs-axim_api_improvementsfrom
Conversation
- Propose consolidation of narrow action-scoped URLs into unified parameterised views - Document certificate generation endpoints as primary consolidation candidate - Define action/mode parameter pattern and shared service layer approach
Abdul-Muqadim-Arbisoft
changed the title
deborahgu
left a comment
deborahgu
left a comment
There was a problem hiding this comment.
this makes some of the PR's I saw go through last year make so much more sense. 😀
| * **Use HTTP verbs exclusively (pure REST)**: Partially applicable — ``POST`` for create, | ||
| ``DELETE`` for unenroll — but breaks down for operations that do not map cleanly to HTTP verbs | ||
| (e.g., ``enable_certificate_generation``). A hybrid approach (HTTP verbs where natural, |
There was a problem hiding this comment.
This actually is so-called purely RESTful, honestly. The noun is certificate_task, the POST indicates that we are creating a certificate task, and the payload indicates what the task is going to be. You could get very busy and add more of the task details into the URL (e.g. POST /api/instructor/v1/certificate_task/:taskname/:course_id) but it's not required to make purists required.
So I'd say something more like
* **Use HTTP verbs exclusively (pure REST)**: Not applicable. This is already RESTful. The noun is `certificate_task`, the `POST` indicates that we are creating a certificate task, and the payload indicates what the task is going to be.
or suchlike.
There was a problem hiding this comment.
Good point, you're right that this is already RESTful when framed around certificate_task as the noun. Applying your suggestion. This actually strengthens the section since it shows the consolidated form is more REST-aligned, not a compromise against REST.
| .. code-block:: python | ||
|
|
||
| # lms/djangoapps/instructor/views/api.py | ||
| class CertificateTaskView(APIView): |
There was a problem hiding this comment.
I assume we would keep this 3 distinct permissions, and the downstream methods would do the permission checks based on the payload. Am I correct about that?
There was a problem hiding this comment.
I think the wording in the Implementation Requirements section ("move permission checking into a common service layer") is partly what makes this ambiguous, let me clarify the intent. Yes, the three distinct permissions are preserved. The shared service layer centralizes the infrastructure for permission enforcement (so we're not re-implementing the boilerplate three times), but each mode handler still enforces its own specific authorization. enable_certificate_generation's permission for toggle, the bulk-generate permission for generate, and the regenerate permission for regenerate.
| * Existing clients calling the legacy URLs require a migration period; deprecated aliases must be | ||
| maintained until adoption drops sufficiently. |
There was a problem hiding this comment.
at least in openedx code, I'm pretty sure that these are only called internally by the instructor djangoapp, right? Although come to think of it that's probably just because this work got done last year. 😆
There was a problem hiding this comment.
You're right, within openedx the only caller is the instructor dashboard frontend, so the in-tree migration is atomic (backend + frontend together). The migration-period language was aimed at out-of-tree callers, Tutor plugins, custom MFEs, operator integrations etc
|
approved but I added a suggested change. |
| Merge Similar Endpoints | ||
| ======================= | ||
|
|
||
| :Status: Proposed |
There was a problem hiding this comment.
We can update Status to Accepted
5 participants
Currently, Open edX APIs expose multiple narrow action-scoped endpoints for the same resource domain, duplicating permission checks, validation logic, and business logic across sibling views. This ADR proposes consolidating these fragmented endpoints into single parameterised DRF views backed by a shared service layer, using an action or mode parameter to distinguish operations
Issue: #38166