Skip to content

Refactor the Subject move: read the source page once, drop dead paths - #1361

Merged
alistair3149 merged 4 commits into
masterfrom
feature/1338-move-subject-simplify
Sep 8, 2026
Merged

Refactor the Subject move: read the source page once, drop dead paths#1361
alistair3149 merged 4 commits into
masterfrom
feature/1338-move-subject-simplify

Conversation

@JeroenDeDauw

@JeroenDeDauw JeroenDeDauw commented Sep 6, 2026

Copy link
Copy Markdown
Member

Follows-up to #1356, rebased onto master after it merged. One commit per item, so any of them can be dropped on its own. User-visible behaviour is unchanged; the one API-visible difference is under the second item.

  • One read of the source page. PageSubjects::without() answers a copy without the Subject. The move writes the copy, and the rollback writes back the object it read, which nothing has mutated. The second read of the source page goes, and with it the reliance on repository reads handing out distinct objects, which SubjectRepository never promised.
  • One not-found outcome for the source. A source page that vanishes between the read check and its write is presented as the Subject not found: it is written first, so nothing has been written yet. presentSourcePageNotFound goes. The response is still 404, with the message Subject not found instead of Page not found.
  • One error path in the REST client. moveSubject no longer reads a server message off a non-ok Response. The production client rejects on every non-2xx but 422, so that branch never carried one. The rejection path still delivers the server's message to the dialog, unchanged.
  • Three @group Database tests dropped: source-page 403, source untouched on an unreadable target, edit summary on both pages. MoveSubjectActionTest covers each; the target-page 403 test keeps proving the Authority wiring.

Considered, omitted:

  • Moving page creation out of MoveSubjectDialog into a PageCreator service behind the registry: it takes the mw.Api call and error-code parsing out of the Vue component, but costs an interface, a class and three registration points for one caller. Worth doing only together with MappingCreatorDialog, which has the same inline mw.Api().create, as a second caller.
  • Replacing dropFromRegistryOnceUnlisted with a plain listing reload: the moved Subject's registry copy carries its old page, so dropping it does real work.
  • Sharing the Codex lookup handling between PagePicker and SubjectPicker: the largest duplication in the original PR, but it means reworking SubjectPicker; a PR of its own.

AI-authored — Claude Code, Fable 5.1 (max); @JeroenDeDauw asked for a follow-up PR that reduces complexity after an in-session assessment of the original, scope chosen by the assistant, then cut by one item at his direction; diff not yet human-reviewed; make cs, the move-related PHPUnit classes and the full tsci green locally on the final branch, the full PHPUnit suite green on the branch before the cut, each new test mutation-checked.

Production notes

Design, diff review and this text by Fable 5.1 (max); implementation, tests and the rebase onto master by Opus 5 (max) subagents from written specs.

@JeroenDeDauw

Copy link
Copy Markdown
Member Author

Ugh. Try to get claude to remove lines. 🤔

@JeroenDeDauw
JeroenDeDauw force-pushed the feature/1338-move-subject-to-page branch from 5c41fec to 583af10 Compare September 6, 2026 22:50
Base automatically changed from feature/1338-move-subject-to-page to master September 7, 2026 14:54
JeroenDeDauw and others added 3 commits September 7, 2026 18:59
PageSubjects::without() answers a copy without the given Subject. The move
writes that copy and the rollback writes back the object it read, which nothing
has mutated. The second read of the source page goes, and with it the reliance
on repository reads handing out distinct objects, which SubjectRepository never
promised.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A source page that vanishes between the read check and its write is written
first, so nothing has been written yet; from the caller's side the Subject is
gone. presentSourcePageNotFound() goes. The response is still 404, with the
message "Subject not found" instead of "Page not found".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The production HTTP client rejects on every non-2xx status but 422, so a non-ok
Response never carried a server message. moveSubject now fails generically on
one, like every sibling method; the rejection path still delivers the server's
own message to the dialog, unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@JeroenDeDauw
JeroenDeDauw force-pushed the feature/1338-move-subject-simplify branch from 35c41b5 to 62ddb9f Compare September 7, 2026 17:38
@JeroenDeDauw
JeroenDeDauw marked this pull request as ready for review September 7, 2026 17:57
@JeroenDeDauw JeroenDeDauw changed the title Simplify the Subject move Simplify moving a Subject to another page Sep 7, 2026
@JeroenDeDauw JeroenDeDauw changed the title Simplify moving a Subject to another page Refactor the Subject move: extract page creation, read the source page once, drop dead paths Sep 7, 2026
Source-page 403, source untouched on an unreadable target, and the edit summary
reaching both pages are each asserted in MoveSubjectActionTest; the target-page
403 test keeps proving the Authority wiring through the real stack.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@JeroenDeDauw JeroenDeDauw changed the title Refactor the Subject move: extract page creation, read the source page once, drop dead paths Refactor the Subject move: read the source page once, drop dead paths Sep 7, 2026
@JeroenDeDauw
JeroenDeDauw force-pushed the feature/1338-move-subject-simplify branch from 62ddb9f to 0cc21b4 Compare September 7, 2026 18:05
@alistair3149

Copy link
Copy Markdown
Member

Moving page creation out of MoveSubjectDialog into a PageCreator service behind the registry

Might be a good direction to take because of #1374. @malberts what do you think?
This is not a blocker and can be done as a follow-up.

@alistair3149
alistair3149 merged commit 5806c50 into master Sep 8, 2026
17 checks passed
@alistair3149
alistair3149 deleted the feature/1338-move-subject-simplify branch September 8, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants