Repository navigation
feat: standardize the account retirement pipeline as /api/user/v2/ - #39330
Draft
Abdul-Muqadim-Arbisoft wants to merge 107 commits into
Draft
Abdul-Muqadim-Arbisoft wants to merge 107 commits into
Abdul-Muqadim-Arbisoft wants to merge 107 commits into
Conversation
The user directory, the stored-preference listings and the time-zone
reference list were the least conforming endpoints of the User API: the
only gate was the deprecated X-Edx-Api-Key header, which ignores
request.user entirely, so any holder of the key could read every user's
email address and full preference mapping. The listings were addressed by
database id, two of them put the parent key in the path instead of the
query string, the time-zone list returned an unbounded array, the stored
preference row embedded its owner's entire record (one extra query per
row), and the forum-role listing created the role it was asked about on
every GET.
Each of those is a contract change, so they land on a new version and
/api/user/v1/ is untouched and keeps serving them.
What callers get instead:
GET /api/user/v2/users/ (also ?preference_key=,
?forum_role=&course_key=)
GET /api/user/v2/users/{username}/
GET /api/user/v2/user_preferences/
GET /api/user/v2/user_preferences/{username},{key}/
GET /api/user/v2/time_zones/
Authenticated staff callers rather than an API key, the standard error
envelope, the standard pagination envelope on all three lists, filter
sets and a deterministic ordering, and the ?view= and ?fields= response
presets. The forum-role filter no longer creates a role, and an
unparsable course key is a validation error rather than a 500.
Being refused is part of the contract, so the refusals are explicit. A
deactivated staff account is turned away on both paths rather than only
one: the session authenticator already rejected it, but IsAdminUser reads
only is_staff and the JWT authenticator does not read is_active, so such
an account could read the whole directory until its token expired. These
addresses require an active user.
An unrecognised query parameter is a 400 rather than silence. A client
porting from a superseded address by changing the version segment alone
would otherwise pass a filter this version does not define, have it
ignored, and receive every row believing the page was filtered. The
allowlist is derived from the paginator, the ordering backend, the filter
set and the query serializer, so it cannot drift away from the filters it
describes.
Shared building blocks are used rather than rewritten:
StandardizedErrorMixin, DefaultPagination, IterablePaginationMixin,
ScopedQuerysetMixin with FullScopePolicy, MinimalViewMixin and project.
The two local classes each supply one predicate the library does not,
and each docstring says which.
The tests pin behaviour rather than structure: every allowed and every
refused caller with the error type asserted, value-level parity with each
superseded address, query counts that stay constant across 1, 5 and 25
rows where the superseded listing pays per row, SQL captures proving no
GET writes, and the URL literals each name reverses to.
The LMS drf-spectacular pre-processing hook is an allowlist and admitted only /api/enrollment/vN/, so everything under /api/user/ generated, routed and tested while being invisible in the published document. The new v2 addresses would have shipped undocumented and the schema gate would have reported the prefix missing with no clue as to why. The filter now admits exactly what this change owns: the v2 addresses, matched by their view package rather than by path prefix, plus the seven legacy paths they supersede, so each superseded address appears beside its replacement. It is deliberately not a /api/user/vN/ prefix. Admitting the whole prefix pulled thirty-eight further v1 operations into the document -- undocumented request bodies and internal docstrings published as client-facing text -- none of which this change owns or documents. SCHEMA_PATH_PREFIX becomes a two-branch pattern, stripped only to derive tags and operationIds. Every existing enrollment operationId was compared before and after and is unchanged, and so is every operationId, path and component of the service-wide /api-docs document: a generated client derives a module per operationId and a model per component, so a rename there deletes a module as surely as a removal does. The title follows the contents, and the three comments that named the old title follow it. Deprecation is marked where it belongs, outside the frozen modules: the existing post-processing hook gains a tuple of the seven exact legacy paths this change supersedes. Exact paths, not a prefix, so an address whose replacement nobody has written yet can never be marked -- the hook test includes neighbouring user addresses, the unversioned twins and another area's paths and asserts they are left alone. POSTPROCESSING_HOOKS is unchanged: setting it replaces drf-spectacular's defaults, so the enum hook stays restated and stays first.
Contributor
Author
Phase D1 — evidence tablesMoved out of the pull request description, which GitHub caps at 65,536 characters. Compliance matrix
AuthorizationThe permission model is carried forward as it stands; tightening it inside a
Deny-path testsNine caller fixtures, named by what each is entitled to: anonymous; another Every address runs all three sides of the matrix — each admitted caller served, Beyond the matrix: DRF's conjunction of the classes and its ordering of 401 Gate report
|
14 of 17 tasks
Abdul-Muqadim-Arbisoft
force-pushed
the
feat/api-adr-user-v2-phase-d1
branch
2 times, most recently
from
November 1, 2026 18:56
db47b87 to
f01a160
Compare
This was referenced Nov 1, 2026
Open
…image APIs as /api/user/v2/
The account surface was the largest non-conforming part of the User API and the
one most callers depend on: frontend-platform's account fetch, the account and
profile MFEs, and both mobile apps all read it. Ten of its operations answered
in shapes nothing documented, refused malformed input with server faults, and
addressed one account's records through a permission that read a username from
somewhere other than the address.
Every change here is a contract change, so they land on a new version:
/api/user/v1/ is untouched and keeps serving every one of its callers.
What this version answers:
GET /api/user/v2/accounts/ (?username=, ?email=,
?lms_user_id=, ?view=,
?fields=, ?ordering=)
GET /api/user/v2/accounts/me/
GET /api/user/v2/accounts/{username}/
PATCH /api/user/v2/accounts/{username}/
POST /api/user/v2/accounts/{username}/deactivation/
POST /api/user/v2/email_searches/
POST /api/user/v2/name_changes/
POST /api/user/v2/name_changes/{username}/confirmations/
GET /api/user/v2/accounts/{username}/verification_status/
GET /api/user/v2/accounts/{username}/verifications/
POST /api/user/v2/accounts/{username}/profile_image/
DELETE /api/user/v2/accounts/{username}/profile_image/
A refusal is part of the contract, so the refusals are explicit. The request
body is measured against a serializer before the account service reads it, so a
wrongly typed property is a validation envelope rather than a server fault; a
failure inside the service is a server fault rather than a validation envelope.
A permission reads the username from the address and from nowhere else, and
compares it exactly, because the handler behind it selects the record by that
same value. An account that is no longer active is refused on both halves of the
default authentication pair rather than only one.
Lists answer in the standard envelope, bounded and ordered by username; a filter
that matches nothing answers an empty page rather than reporting the address as
missing, and a status that has never been established answers "none" rather than
reporting the account as missing. An email search is bounded at a hundred
addresses, which bounds its answer.
The schema describes what the code does: the superseded addresses publish the
media types they actually read, every operation publishes the codes it really
answers, and the response components carry the properties the bodies really
hold, with presets modelled rather than implied.
The tests pin behaviour rather than structure. Every allowed and every refused
caller is named with the error type asserted; value-level parity is checked
against each superseded address; query counts are measured on the earlier
endpoint first and asserted not to grow with the collection; SQL captures prove
no read writes, including writes deferred to commit; and each guard was checked
by breaking what it covers and watching the right tests fail.
The profile-image endpoints are addressed under the User API but served by a
view that belongs to the profile_images app, so the conforming version lives in
that app's own new version package and is mounted into the user version two
URLconf exactly as the earlier view is imported today. The app's existing
modules are untouched, and the image validation they own is called rather than
copied.
An upload is read before anything is stored, so a truncated file, metadata that
cannot be parsed, or a header declaring more pixels than the site will ever draw
are answered as the caller's error rather than as a server fault, while a
failure to store a well-formed image stays a server fault. The media types the
address reads are published, and the body is published as required.
A caller may set and clear the image on the account the address names and on no
other, staff included, since clearing an image is a change to an account rather
than a moderation tool. The permission reads the username from the address and
compares it exactly, because that is the value the stored names are built from,
so the account a caller is admitted for is always the account written to.
The tests drive real uploads: each refusal with its error type, parity against
the earlier address for the bodies both answer, and a check that a caller
holding another spelling of the owner's name is refused and stores nothing.
This package is shared with the CMS, and the CI shard that runs it under cms.envs.test could not import this module at all: it read settings.ACCOUNT_VISIBILITY_CONFIGURATION, an LMS setting, in the module body. Collection failed, which failed the shard, which cancelled the rest of the matrix. Every test here that reads these sets is already @skip_unless_lms, but a module body runs before any skip can apply, so the setting is now read through the same guard user_api/accounts/tests/test_views.py uses for it, and the field sets fall back to empty when it is absent. Under cms.envs.test this module now collects, and the failures left in these two packages are the same 44 as on master, in the same tests -- an artefact of running two of the shard's sixty-nine paths rather than anything in this branch. Under lms.envs.test nothing changes.
…uth class Four views declare their own authentication_classes so that a caller who has not yet confirmed their email address can still reach their own account, its picture, its preferences and its email choice: the session class the platform configures refuses such a caller, so that half is replaced. For the token half they named DefaultJwtAuthentication. That class exists only to be named in DEFAULT_AUTHENTICATION_CLASSES -- its own module says it is only meant for that -- and the using_default_auth_classes attribute it records exists so an operator can find the endpoints that are on the defaults. A view that declares its own classes is not one of those, so recording the attribute filed a false signal and polluted the query the attribute serves. Nothing on master declares the class: two files name it, both definitional, against the library class used throughout the rest of the platform. The four views now name JwtAuthentication, the class the platform's own pair is built on. Two of these addresses, the account and its picture, were added earlier on this branch and are corrected in place here. The guards assert the attribute is absent, each alongside a control request to a sibling address that declares no classes and must still record it, so a pin cannot pass merely because a patch target was mistyped.
An audit across the three phases on this branch found four defects, each introduced by an earlier phase and each visible to a caller or to a generated client. They are corrected together here, as the JWT class was, rather than left to differ between sibling addresses of one version. The profile image published a username with no pattern, the only one of the eleven username path parameters in this version without it, so a generated client validated the name before the request on nine addresses of the resource and not on the tenth. The parameter helpers and the username pattern move from views.py into view_utils.py, which that module already imports from, and the parameter is built with the helper. The helper being private to one module was the cause, and a further module would have hit it again. The stored-preference resource pinned one operation id inline, which applies to every document, so the service-wide document published that operation beside a collection read carrying a different prefix, and a client generated from it derived the two from different modules. The pin moves into the per-document table beside the entries that already do this, so the resource's operations share their leading tokens in each document. One id changes, in the service-wide document only, to a name that document has never published. The username filter accepted a single exact name on two collections and a comma-separated list on a third, so a caller who learned the list form on the account collection was answered a 200 with an empty page rather than told the filter does not take a list. All three now accept the list, through one shared selector, matching the member address of that resource, which already reads a comma as a separator. The email search was the one collection in this version reading accounts directly without declaring a scoping seam, while the two collections over the same table declare one. It now declares the same policy through a queryset the shared mixin can wrap. No caller gains or loses access and no row changes: the policy is a pass-through today, and the seam is what was missing.
…er them Both documents already published these four addresses with a 200 and no body before their bodies were described. No handler answers that: each answers 204. The corrected code is now pinned in both documents and each handler is driven on its success path in the same test, so the published code and the answered one can only move together. Replaces the pin that covered two of the four in one document.
…here too Studio mounts the user API at the same addresses the LMS does and publishes its own unauthenticated document at /api-docs/, built from its own settings, so none of the marking applied in the LMS reached it: all twenty-five of those operations were published there as current while the LMS published them as superseded. A client generated from Studio's document therefore offered a caller an address the LMS document steers them off. The list of superseded addresses and the hook that reads it are shared rather than restated, so the two services cannot drift apart. Measured on the two generated documents: Studio marks 25 of 25, as the LMS does, and nothing else in Studio's document changes -- no path, operation, operationId or component is added or lost. The narrow Authoring API document Studio also serves carries none of these addresses, so the hook cannot reach it.
…ettings The documentation settings replaced the appended components wholesale rather than adding the security schemes to them, so any component name the service settings append was dropped from the settings the published reference is generated from. Nothing showed it: the name simply stops being in the reference, and in every client built from it, while every count stays where it was. They now add to what they inherit. The test loads those settings in a process of its own -- loading them here would turn every feature flag on and add Studio's applications to the settings this process already runs under -- and checks that a component the service settings append arrives alongside the security schemes.
Asking for a page of nothing, of fewer than nothing, or of something that is not a whole number of rows was answered 200 with the default page of ten, on every collection of this version that pages its rows. The framework drops a size it cannot honour and falls back without telling the caller, so the page reads as the one that was asked for; the links on it carry the value back, so a client that follows them keeps sending a number nothing reads. The shared paginator now reads the value itself and refuses one no page can have, the way this version already refuses a query parameter it does not read, a sort field it does not offer and a boolean naming neither side. A number above what a page carries is still answered with a full page, which is what it has always meant, and a value carrying nothing still leaves the size alone.
The standard error body carries one line for a person to read beside the list of what each parameter or property was refused for. Where the refusal named parameters, that line was produced by printing the list itself, so a caller read back the brackets, the quoting and the class names that write a data structure out instead of a sentence. Everything that puts the line in front of a person showed it: the message of the exception a generated client raises, a line in an integration log, the notification a page puts up. The line is now written from the same values as a sentence, naming each parameter and what it was refused for; a refusal that names nothing in particular is spelled on its own. The list itself is untouched, and a refusal that never had a list -- an address that matches nothing, a caller that may not read, a method the address does not serve -- keeps the line it had.
…ved it to A member address of the account collection was answered by matching the name it carried against the name each record that came back is stored under, by a reduced form of that name failing that, and anything matching neither was dropped. The database settles which stored records a name resolves to, and the name it resolves to need not be written the way the address wrote it: where it resolved to a record neither comparison matched, the list came back empty, the member handler indexed the first element of it, and an authenticated caller was answered 500. The superseded address answers 200 with that record, so the replacement was worse than the address it stands in for, and a client or MFE that pads or transliterates a name turned a working read into a server fault. A member address is now answered from the records the lookup resolved it to: the one stored under exactly the name given when there is one, and otherwise the single record that came back. An address the lookup resolves to several records, none of them stored under the name given, names no one of them, so it is refused with 400 rather than answered with whichever came back first. A lookup that comes back with nothing is reported as an absent address rather than as a fault. The page of a list keeps the order its names were asked in, which is what the matching is for; a record it resolved to under a name none of them is written as is now carried at the end of the page rather than left out of it in silence. Tests: a member address the lookup resolves to a record stored differently, one it resolves to nothing, one it resolves to several records with and without an exact match among them, and a page carrying a record resolved under another name. Each fails against the previous code, the first three with the 500 this corrects.
The account collection selects by the email address of an owner with a lookup that takes one record and refuses to answer at all where there is more than one. Nothing in the user model keeps one address to one account, so a staff caller looking up an address two accounts hold was answered 500, with nothing in the answer to say why. The superseded address fails the same way, so this was carried forward rather than introduced. The selector now names every account holding the address, and the page carries them all. A collection is what this address answers, and a page of the accounts holding an address is already what the search over addresses beside it answers with, so the page is the answer here whether the address is held once or several times. An address no account holds still leaves the page empty. Tests: the selector asked for an address two accounts hold, and the collection read by a staff caller for the same address, asserting both accounts in the page. Both fail against the previous code, the second with the 500 this corrects.
…source The accounts are one collection however many addresses reach into them, and six operations that read or change a row of it were served by views standing apart from it: the identity verification summary and attempts, the preference document and one preference of it, and setting and clearing the image, each declaring a flat list of classes of its own, none of them reading what the account resource admits a caller by. A rule declared for that resource therefore did not reach them. Measured with a rule admitting nobody added to the resource: the account read answered 403 while the summary, the attempts and the preferences answered 200, one preference and an upload reached their handlers, and clearing the image answered 204 and cleared the stored image -- a change to an account carried out while every read of the same account was refused. Deactivating an account read nothing of the resource either. The classes the resource admits a caller by are now declared once and carried by every address that works on one of its accounts, which applies them first and the classes of its own address after. The same measurement now refuses all eight, and the stored image is left alone. The declaration is read from the shared tuple rather than from the collection's own attribute because that attribute is added to in place by the first request to reach a view that reads a token, which would otherwise make admission depend on which addresses a process had served already. The account an address names is resolved through the resource as well, by one rule: the record stored under exactly the name given, or the single record the name came back with, and otherwise a refusal. Three operations resolved the record they write to with a lookup that takes one record and fails outright on more, each catching only the not-found case, while the class in front of them had settled the question on the name as text. Making a password unusable, confirming a profile name and recording an image can therefore no longer land on a record nobody addressed: each is refused with 400 and writes nothing. Recording an image is looked up again by the writing helper, which this version does not own, so the refusal is raised where that lookup comes back with more than one record. The preference addresses keep their own two-tier rule -- a read of somebody else's is a privilege, a change of it belongs to the owner -- and declare it through the hook the shared gate calls, so both tiers still apply and the resource's rules apply before them. One test outside this version's own modules moves with it: the preference guard harness handed a bare request to classes that are given the wrapped one by the view, and the classes carried in from the resource read the request through it, so the harness now authenticates the request the way the test client does. Tests: every read and every change of one account measured twice, once as they stand and once with a rule admitting nobody added to the resource, with the stored image asserted untouched after the refusal; a deactivation refused with the privilege for it held, so that the refusal measured is the resource's; and each of the three write targets, refused when the address names no one account and landing on the account stored under the name when it does. Removing the shared gate from the four views, or emptying the shared declaration, turns the refusal tests red.
The answer to an OPTIONS request is built from the parsers and the serializer declared on the view, which belong to the view rather than to the address being asked about, and two addresses of this version were described wrongly because of it. The preference document of one account is read and changed as a merge patch and a body in any other media type is refused, which is what the published document says of it. The description of that address named the three media types the platform reads bodies in everywhere else and left out the one it takes, because the parsers follow the action being served and the action recorded for this question is not the change. A caller negotiating from the description -- the one mechanism offered for it -- therefore sent a body in a media type the address refuses and was answered 415, with the two published descriptions of one address contradicting each other. The parsers now follow the methods the address serves, and the description reports the ones it would read a body with. Confirming a requested profile name carries out the change already asked for and reads no body, which is what the published operation says. The description of that address offered a body with a required name in it, built from the view's serializer, so a client or a form driven by it asked the operator for a name before confirming the change and sent a body the address ignores. An action that reads no body is now declared as such and no body is described for it. The address that asks for a name still describes the one it takes. Tests: the media types named by each preference address, a change sent in every media type the document address names being carried out, no body described for a confirmation, and the body still described for asking. Each fails against the previous code, the negotiated change with the 415 this corrects, and each half of the description turns its own tests red when taken back off.
…ently An address can have been held and left more than once, so more than one retirement of it can sit inside the cooling-off period. This version reports the most recent, being the only one a caller could still undo; the superseded version reads the same window with a lookup that takes a single record and so fails outright on the second. The window boundaries are the same in both. The selection is on a path declared to answer as the superseded one answers, and this is the one input they answer differently for, so the selector now says so where the next reader of it will be.
…ot to a maintainer
The two disposals deleted rows of the retirement and partner-report collections from un-routed views standing outside them, each with its own flat list of permission classes and reading neither the records nor the rules its collection reads. A restriction added to the collection therefore did not reach the operation that deletes those same rows, which is measurable: the same rule, in the same place, leaves the collection refusing a caller the disposal still serves. Both disposals are now registered on the same router, at addresses unchanged character for character, and take their rules and their records from the mixins their own resource uses. Neither declares either for itself. One ordering is load-bearing and is pinned by a test: the mixin carrying the queryset must sit after the one that declares a policy, because that one declares an empty policy of its own and shadows anything behind it. The visibility policy now reaches every action rather than the list and the queue drain alone, so a member address resolves the record from the selection the policy left, as the class already said it did. Nothing a caller is answered changes: the policy this deployment runs returns the selection unchanged, and both published documents are byte-identical. The refusal of an ambiguous name and the absence reported for a record outside the selection are each held by a mutation rather than observed. One answer does change, and it is the one the collection already gives: a caller without the capability who sends a method the disposal does not serve is now told which methods it serves, instead of being refused. The address serves one method, and that is already published.
The record published two properties the handler has no way to answer: it is built from the queue entry and from the enrolments of the account it names, and neither carries an identifier the deployment knows the user by or a per-organization reporting configuration. Both are removed, the class docstring no longer promises them, and a test holds the published property set equal to the set a claim really answers, so the two cannot drift apart again. Selecting properties reaches the top level only: a name written as a path, such as `user.email`, is read as `user` and the whole embedded account comes back. That is the shaping helper's own behaviour, shared with addresses already published, so it is stated in the parameter's description and pinned by a test rather than changed under a caller who is relying on it. One test literal named a step this deployment does not run, so the refusal it asserted could only ever have come from the media type it sends. It now names a step that is configured, which is what makes the assertion say what it claims to.
… own settings Two defects, both of which a test could not have caught as the tests stood. The gate that decides whether a retirement admits pipeline work was asked about the name a caller sent rather than about the record that name had already resolved to, by a model-layer call that reads the whole table. Where the records a view may act on are narrower than the table -- which the class invites, and says is applied by every operation it serves -- the row judged and the row worked on are different rows, and an erasure destroys an account the gate exists to refuse. The step check is now a method on the record, with the predicate, the message and the exception it always had, and the classmethod calls it after resolving a name, so the superseded version behaves exactly as before. The queue write that takes a record from another collection resolves it through the same narrowing rather than from the whole table. Two smaller losses of the superseded behaviour go with them: the delete that disposes of finished retirements states the step again, because a record moved off it between the redaction and the delete is one the caller was never answered about; and the name a second erasure falls back to is the name the account was registered under rather than the one it holds by then, which is a hash of it. The hold this schema takes on one generation setting no longer runs through the library's shared patch table, which restores and forgets every patch in it rather than its own. A document generated under settings of its own -- its title, its version and the address to write to about it -- lost all of them the moment a body of this kind was built, and the service answered with the fallback title and version while the file checked in beside it kept the real ones. A setting nothing had asked for is left unasked for afterwards, so a later change to a deployment's settings still reaches it.
… the pin table The documentation settings carried a second copy of the retirement-status component the service settings already append, which two places had to keep identical, and the settings they are built from were asked for it under a key that replaced the service's own. They now name the one definition the service holds and merge the component names under it, so a name appended on either side reaches the published reference, and the check of what those settings append covers both names rather than one. The table of route stand-ins the derived operation names are read from gained the two reads of the retirement queue, so the check that every pinned name replaces a derived one can see the names those two addresses pin.
…ondition once Two definitions of the name for a request that names more than one account stood in the same module, one shadowing the other, and the name was registered with the shared catalog twice. There is one of each now, describing both the address several accounts answer to and the name several records were requested under. A refusal that arrives carrying the records at fault wrote its own line about the request as a whole -- that a set of records was left alone and nothing deleted -- and that line was being replaced by one written from the list of records, which cannot be read to say it. A caller of a batch disposal read which record was at fault and no longer read that the rest were left alone. That line is now kept, and the records stay in the list beside it; a refusal whose line was printed from its list is written out as a sentence as before.
…d hold the records to it The parameter that selects which properties a response carries refuses a name written as a path, because reading it as the property it starts with hands a caller the whole of a property it asked one piece of. Its published description still said such a name is read as its first part and the whole of that property returned, so the document promised the behaviour the handler had stopped having. The retirement records are held to the refusal as the rest of this version is: asking for one property of the embedded account by path is refused with the names that can be selected, rather than answering with the account whole, which is personal data of the account being retired.
… answers Which representations an answer may be given in is settled as a request enters, before the record it names is read or written, so every address of the retirement pipeline already refused a request asking for one it cannot answer in and none of them published that refusal. Each operation publishes it now, and a sweep sends every method of every one of those addresses such a request: each answers the refusal, the covered methods are read from the module that routes them so an address mounted later is covered too, and the records the batch disposals delete, the data the erasure destroys and the request the cancellation undoes are all still there afterwards. The retirement queue joins the collections the page-size refusal is measured on, which is tied to the views that share the paginator. The check of what each list publishes about a page past its end carried two sets of expectations at once, the second quietly replacing the first; there is one set now, and it covers the queue. The write of one stored preference, which answers a request carrying no body, is where a body that needs nothing is read from; the change of a whole preference document is published as needing one because its shape names a member it cannot be sent without, and both are held here.
…o a maintainer A request asking an address what it serves is answered with the description its view class carries, so that description is published text. The four addresses of the retirement pipeline described themselves to somebody reading the module: why the resource is modelled as it is, which layer resolves what, and in one case a word for the machinery that narrows a page. Each now says what a caller can do there, in a paragraph or two, and the rest is written beside the class where nothing serves it. Asking the queue and the batch disposal what they serve is answered with exactly the text each class carries, so the descriptions above are measured as published rather than assumed to be, and the sweep over every mounted address counts the four this pipeline adds.
…ddresses too Studio's name pin covered the addresses this version replaced before the retirement pipeline joined them; it now covers those as well, each under the name both documents already carry -- checked against the other service's document, where none of them is named differently.
The repository's pylint reports four attributes of one retirement test set outside the constructor. They are declared empty where the rest of the case is set up, so the names exist before the helper that pairs two records assigns them, and the helper is unchanged.
The partial update of a version 1 retirement was the only reference to the retirement-status model, and that operation now publishes the object its handler really reads, so nothing generates the model any more. The service serving the newer version of this API appends the model to every document it builds; the service serving only the older one did not, so a client generated from its document of everything it serves lost that class. The definition now sits beside the rest of this API's schema behaviour and is read from there by both services and by the published reference. It is kept in a module that imports nothing, because a settings module reads it while the settings are still being assembled, which is too early for any module that reads a setting of its own.
…ument Studio's SPECTACULAR_SETTINGS build the Authoring API document (/authoring-api/schema/ and the committed docs/cms-openapi.yaml), whose pre-processing hook keeps only the contentstore and authoring addresses. Appending PatchedUserRetirementStatus there left two references, to RetirementUser and RetirementState, that the document never defines, and a client generator refuses such a document. Studio's service-wide /api-docs/ needs no append either: Studio's schema class describes the version 1 retirement update as master does, so that operation still generates the model, identical to the definition the LMS appends. The settings key and its import are removed, and docs_settings no longer re-adds the components the LMS settings already carry. Each service's module tests now request both documents the service serves and assert that no reference is left unresolved, through a small walker in the shared testing utilities. Studio's component test now pins the operation that generates the model there rather than an append. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both version 2 batch disposals refused a body naming more than 100 accounts. Version 1 sets no such bound, and the in-tree drivers rely on that: scripts/user_retirement/retirement_partner_report.py takes every waiting partner report entry and then disposes of all of them in one call, as retirement_archive_and_cleanup.py does when --batch_size is not given. Past 100 entries the disposal answered 400 after the reports had gone out, and the entries stayed taken, so the next claim returned none of them. The cap and its constant are removed, and the published description of `usernames` now says that one request may name any number of accounts. Each disposal is one statement over the matched identifiers, as on version 1. A parity test drives both versions with 101 names: 101 finished retirements disposed of through each version's cleanup, and 101 partner report entries taken and then disposed of through each version's claim and cleanup, leaving nothing behind on either. The views test that pinned the refusal now pins an answer for every one of 101 names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The names in a batch disposal's body went through DRF's default trim_whitespace, so " name " deleted the record of "name" and reported it as "name". Version 1 compares the string it is sent, refuses that request and deletes nothing, and the published description already says the names are compared exactly as spelled and reported as given. The names are now read untrimmed. A padded name matches no record, so it is reported as absent under the name as sent and nothing is deleted, which leaves the same records behind as version 1 does. The answer stays the version 2 one: an absent name is reported rather than refused. Parity tests send a name with spaces around it to each version of both disposals and check that every record is kept on both, and that version 2 reports the name as it was sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…un order The test asserted that the capability the disposal asks for came last in the classes it resolves. The restricted-application guard is appended after it unless EnsureJWTAuthSettingsMiddleware has already copied the guard onto the view's class attribute, which it does on the view's first request in the process. The test therefore passed only when another test in the same worker had requested the disposal first: run alone it failed. It now reads the classes from the view's own declaration, as the test beside it does, and pins the whole resolved list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A batch disposal's `usernames` list is refused item by item. The shared
readable-error layer now lists each refused item under a name of its own
(`usernames[1]`), so a disposal sent {"usernames": ["ok", null]} no longer
answers "usernames: {1: [ErrorDetail(string='This field may not be null.', code='null')]}".
The batch disposal tests, for both disposals, pin the item-named entries
and the sentence, and check that no refusal of a bad body carries
"ErrorDetail(" or "string=".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`cool_off_days` on the retirement list had a lower bound only. A count of days reaching back past the first year of the calendar overflowed datetime arithmetic and answered an undeclared 500. The version 1 queue read fails the same way (OverflowError, an HTML 500). The claim's `limit` had no upper bound either, and a value too large for the database's LIMIT answered 500 as well. `cool_off_days` now takes 0 to 36500 days, a century, which reaches back further than any retirement a deployment holds; a larger value is refused with 400 on the parameter. `limit` takes 1 to 100, the rows a page of any collection of this version holds; omitting it still takes every entry waiting, which is what the pipeline does. Both parameters publish their bounds, read from the field that enforces them. This is a declared difference from version 1 for `cool_off_days`, added to the parity harness and measured on both versions: past a century version 1 answered (with no rows, since no retirement is that old) and past the first year of the calendar it failed, where version 2 refuses both. A century answers the same rows on both. `limit` has no version 1 counterpart. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d helper The version 2 retirement service read the organizations to report a retired account to by calling a private static method of the version 1 partner-report view, and read two of that view's class attributes for the keys of the record it returns. That tied the newer version to a private name of the module it supersedes. The loop now lives in accounts/utils.py as get_orgs_for_user(), moved unchanged, and both versions call it. The version 1 view keeps _get_orgs_for_user() as a static method that returns the helper's result, so anything calling or patching it, including its own tests, sees the same set as before. Version 2 writes the two record keys out like the others in the same record. Two version 2 tests now cover the placeholder organization the helper leaves out, once for the queue and once for the report a partner reads; both fail if the helper stops leaving it out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Version 2 gates each pipeline operation on the record it has already resolved rather than on the name, so that the record judged is the record acted on. To do that it had extracted the state check of UserRetirementStatus.get_retirement_for_retirement_action() into a new model method and pointed the classmethod at it, which edited the model module version 1 and several other apps read. The model module is back to what it was before this version existed. The rule -- no work at a step the pipeline requires, nor at a step recording a finished part of it -- is written in the version 2 service, applied to the resolved record, and answered with the same conflict as before. The log line now names the step the retirement stands at instead of carrying the model's exception text. A parity test creates a retirement at every configured step and asserts that both gates refuse exactly the same thirteen steps and admit the other nine; it fails if either half of the rule is dropped from version 2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ting The move of a retirement is sent as PATCH but is read whole: a body missing the step to move to or the note about the step being left is refused. To publish it that way, the schema mixin both services use overrode a private method of drf-spectacular for every operation and, for this body, set COMPONENT_SPLIT_PATCH on the process-wide spectacular settings object and put it back afterwards. A release that renames the private method would break /api-docs/ in both services, and a document generated concurrently could read the held value. The serializer now says what it is: its partial attribute is always false, so neither DRF nor the schema generator, which copies DRF's PATCH handling by setting that attribute, can read the body as a set of changes. The override, the settings helper and the marker attribute are removed; no generation setting is written any more. The published body is unchanged: the two schema tests that pin it (component RetirementUpdate, both values required, no Patched variant) pass as before, and a new test builds the serializer with partial=True and asserts the missing value is still refused. All three fail if the attribute is not overridden. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The LMS settings pinned the two sets of choices version 2 publishes as ScopeEnum and OutcomeEnum. The pin keeps them stable, but those plain names are then held by the user API for the whole service: the next scope or outcome choice field anywhere in the LMS would be published under a prefixed name instead, and a test of it would have to know why. They are now RetirementErasureScopeEnum and RetirementBatchResultOutcomeEnum, the names the generator itself gives them once another field shares their field name. Neither the old nor the new names are in any document generated from master (both LMS documents and both Studio documents checked), so no published component name is lost; version 2 is unreleased. The tests that pin the names follow them; the collision test fails if the settings go back to the plain names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The comments this work added to the retirement settings, schema, URL, serializer, service and view modules explained in whole paragraphs what a line or two can say. Each is cut to the point it makes. Comments only: no code, docstring or help text changes, so every module's syntax tree is unchanged and the served documents are byte-identical. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… run order Four tests asserted that the capability an operation asks for came last in the classes the view resolves. The restricted-application guard is appended after it unless EnsureJWTAuthSettingsMiddleware has already copied the guard onto the view's class attribute, which it does on the view's first request in the process. Each test therefore passed only when another test in the same worker had requested the view first: run alone, by class, or in any other order, it failed. They now read the classes from the view's own declaration, as the test of the disposal's actions does, and pin the whole resolved list, with what each class answers where the test asks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Abdul-Muqadim-Arbisoft
force-pushed
the
feat/api-adr-user-v2-phase-d1
branch
from
November 5, 2026 19:40
ed83bea to
c59981f
Compare
This was referenced Nov 5, 2026
Contributor
Author
|
Rebased onto the rewritten #39314, plus this round's corrections (28 commits, up from 16). This branch was rewritten and force-pushed. The description is replaced and an evidence comment follows. What changed in the history. Each correction now sits in the PR whose code it changes. Before, some corrections to #39297's code (for example, the JWT class on the account and image views) existed only in #39314. Merging #39297 alone would have left them out. Now each PR is complete once the PRs before it merge (order: #39297 → #39314 → #39330 → #39334 / #39331). The corrections from the review round of 2026-11-04 are added as commits at the end of each PR. Commits that changed a lower PR's code moved down, with their authors and messages kept. Some fixes were folded into earlier commits of the same PR so that every commit passes the suites. How to review the rewrite. The new commits of the 2026-11-04 round are listed in the description's commit table. Each one names its subject, and each behaviour change comes with a test that fails without it. |
Contributor
Author
Evidence for #39330Each test is named as Declared differences → tests
The trimming of Compliance matrix
Gate report
There are 3 schema WARNs of the form "new write operation documents no requestBody", all on bodiless actions: Repo CI lint: ruff, xsslint, pii_check, check_keywords, lint-imports ("lms and cms are independent" KEPT), mypy, verify-dunder-init and pylint add 0 findings against #39314. commitlint passes on every commit. Test runs at this head (
Before the last commit, Tests removed or changed by the corrections
|
1 task
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
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
Part of
[API] Userunder the umbrella #38137, followingdocs/decisions/and OEP-69.This PR moves the eleven operations that the retirement pipeline driver calls onto
/api/user/v2/. They are the queue reads, the record read, the move, the cancellation, the two erasure steps, the two batch disposals, and the partner-report claim and queueing. The work extends the unreleasedv2package from #39297. Every v1 address keeps resolving unchanged.Why a new version. Some of the eleven v1 operations answer errors as bare JSON strings, and the driver reads those strings. An in-tree v1 test also asserts that an exception string is returned verbatim. Changing these in place would break a pinned contract on the pipeline that erases learner data.
Stack and merge order: #39297 → #39314 → #39330 (this PR, on #39314) → #39334 and #39331 (siblings, both on this PR).
28 commits, 27 files, +7860 / −42 (this PR's own range):
Operations
GET /api/user/v1/accounts/{username}/retirement_status/GET /api/user/v2/retirements/{username}/GET …/accounts/retirement_queue/,GET …/accounts/retirements_by_status_and_date/GET /api/user/v2/retirements/(filtersstate,created_after,created_before,cool_off_days)PATCH …/accounts/update_retirement_status/PATCH /api/user/v2/retirements/{username}/POST …/accounts/cancel_retirement/DELETE /api/user/v2/retirements/{username}/POST …/accounts/retire/,POST …/accounts/retire_misc/POST /api/user/v2/retirements/{username}/erasures/withscopelms_core/lms_miscPOST …/accounts/retirement_cleanup/POST /api/user/v2/retirement_cleanups/POST …/accounts/retirement_partner_report/POST /api/user/v2/retirement_partner_reports/PUT …/accounts/retirement_partner_report/PUT /api/user/v2/retirement_partner_reports/{username}/POST …/accounts/retirement_partner_report_cleanup/POST /api/user/v2/retirement_partner_report_cleanups/replace_usernamesanddeactivate_logoutare not in this PR. They follow in #39334.Declared differences a caller can see
The record differences are
RECORD_DIFFERENCES, and the filter differences areQUEUE_FILTER_DIFFERENCES, both intest_retirement_parity.py. Each test is named in the evidence comment.Every operation
400. A token issued to a restricted application is refused403, by each view itself.405withAllow. The v1 disposals answer the same request403.400user/ambiguous-account-namerather than acting on the first record.The collection and its records
statetakes step names, either repeated or as one comma-separated list.created_beforeis exclusive, where theend_dateit replaces included the whole day named.cool_off_daysabove 36,500 (a century) is refused400. Up to about 739,000 days, v1 answers such a value with no rows, and beyond that it fails with a500. Within a century, both versions answer the same rows.current_state,last_state), not as an object.?fields=reaches the top level only. A name written as a path, such asuser.email, is refused400.409user/retirement-state-conflict, and a missing record is404.new_stateandresponsein a move are trimmed, while v1 storesresponseverbatim. No in-tree caller pads either.get_retirement_for_retirement_actionapplies. A parity test holds both versions to the same 13 refused steps and 9 admitted steps.user_api/models.pyis unchanged.Batch disposals and the partner report
deleted/already_absent). Like v1, they put no limit on how many names one request carries, and they compare each name exactly as sent. A name with spaces around it matches no record and is reportedalready_absent, under the name as sent.usernames[1], anddetailis written in words.409, and nothing is deleted.limittakes 1 to 100, and a larger value is refused400. Omitting it takes every waiting entry, as before. v1 has nolimit. The placeholder organizationoutdated_entryis never named to partners, as on v1.Published documents
deprecated: true, and 10 of them publish the body their handler reads./api-docs/marks the 11 operations at those 10 addresses. Studio's Authoring API document carries no user-API component, as onmaster. Studio's/api-docs/still publishesPatchedUserRetirementStatus, generated from its own v1update_retirement_statusoperation and identical to the definition the LMS appends.RetirementErasureScopeEnumandRetirementBatchResultOutcomeEnum. These names exist only on this unreleased branch.ADRs applied
The evidence for each box is in the compliance matrix in the evidence comment.
serializer_classon every view, with request and response declared separately where they differuser/retirement-state-conflict), registered for this area's own exceptionretireplusretire_miscbecome one erasure withscopeFilterSet, a strict ordering backend, a deterministic default orderauthentication_classes?view=minimaland?fields=refuse unknown values and dotted namesv2;user_api/models.pyunchangedScopedQuerysetMixinwithFullScopePolicy()on the records and both disposalsfields/viewIn-place edits
openedx/core/djangoapps/user_api/accounts/utils.pyget_orgs_for_user(), moved unchanged fromAccountRetirementPartnerReportView._get_orgs_for_user, so that v2 does not call a private method of the v1 viewopenedx/core/djangoapps/user_api/accounts/views.pylms/envs/common.pyAPPEND_COMPONENTScarriesPatchedUserRetirementStatus;ENUM_NAME_OVERRIDESpins the two choice-set namescms/lib/spectacular.pySUPERSEDED_PATHScms/lib/tests/test_spectacular.py,lms/lib/tests/test_spectacular.pycms/tests.py,lms/tests.py$refopenedx/core/djangolib/testing/utils.py,…/tests/test_utils.pyunresolved_references()and its testsuser_api/schema.py,user_api/schema_components.py(new; from earlier in the stack)Error-format decision
Versioned. The v1 bodies, bare strings included, are unchanged.
Deprecations
The 11 operations are marked
deprecated: true, with no header and no note in code. They are tracked on #39128. This area's entry is still to be posted, and its text is drafted. Every replacement is LMS-only, so a driver that calls Studio must be pointed at the LMS before removal.Deliberate deviations
?limit=to drain a large queue in steps.AccountRetirementView,LMSAccountRetirementView). Listeners select on them as the signal senders, and they also hold the public erasure helpers. The DEPR removal must move both first.RetirementViewSet,RetirementPartnerReportViewSet,RetirementCleanupViewSetandRetirementPartnerReportCleanupViewSet. Their permissions come from access mixins, which the heuristic does not see.master, the gate passes.For an operator
POST /api/user/v2/retirement_partner_reports/withoutlimittakes the whole queue in one request and one transaction. The in-tree driver sends nolimit, so on a large backlog, configure it to send one.SPECTACULAR_SETTINGS['APPEND_COMPONENTS']and['ENUM_NAME_OVERRIDES']are load-bearing. A deployment settings patch that assigns either key, instead of merging into it, drops the pinned names.Known pre-existing behaviour, left as it is
UserRetirementStatus._validate_state_updatecompares a state object against a list of state names, so the dead-end-step guard in the model layer never fires. Both versions go through it, and it is drafted as a separate issue./api-docs/schema/caching, as described in feat: standardize the user accounts, profile image, verification and name-change API #39297.Evidence
The full tables are in the evidence comment.
d178206575for the comparison against master):/api-docs/and/lms-api/, Studio/api-docs/and/authoring-api/) has 0 unresolved$ref, checked bytest_every_reference_in_each_served_document_resolves.c59981f3d1), MongoDB attached, serial: LMS (user_api/,profile_images/,user_authn/,lms/lib/tests/,lms/tests.py,openedx/core/tests/test_apidocs.py) 2779 passed, 1 skipped, 0 failed; Studio (cms/lib/tests/,cms/tests.py,user_api/) 209 passed, 1866 skipped, 0 failed.test_retirement_views.pypasses in file order, in reversed order and under four random seeds (256 passed each time), and each of its 28 classes passes alone. The wholeuser_api/v2/suite: 1458 passed, also under a random order.origin/masterd2dbc6a593, conflict-free (wider targets, as in feat: standardize the user accounts, profile image, verification and name-change API #39297; the tree differs from this head by the C1803 line and the last commit's four tests): LMS 3526 passed, 8 skipped, 0 failed; Studio 381 passed, 2250 skipped, 0 failed. Every master test id is still collected.Follow-ups
RetirementListTests::test_a_cooling_off_period_of_a_century_is_takenasserts an empty answer with no positive control inside the test. Hardening it is a test-only follow-up.