diff --git a/resources/ext.neowiki/src/persistence/RestSubjectRepository.ts b/resources/ext.neowiki/src/persistence/RestSubjectRepository.ts index 2822ca06b..67c20dddb 100644 --- a/resources/ext.neowiki/src/persistence/RestSubjectRepository.ts +++ b/resources/ext.neowiki/src/persistence/RestSubjectRepository.ts @@ -368,7 +368,7 @@ export class RestSubjectRepository implements SubjectRepository { // A move can fail for reasons the user can act on - the target page is protected, the Subject // is already there - so the server's own message is carried through rather than collapsed into // a status code. The production client rejects on every non-2xx but 422, so that message - // arrives on the rejection rather than on a response. + // arrives on the rejection. try { response = await this.httpClient.post( `${ this.mediaWikiRestApiUrl }/neowiki/v0/subject/${ id.text }/move`, @@ -388,7 +388,7 @@ export class RestSubjectRepository implements SubjectRepository { } if ( !response.ok ) { - throw new Error( await this.errorMessageOf( response ) ?? 'Error moving subject' ); + throw new Error( 'Error moving subject' ); } } @@ -398,15 +398,6 @@ export class RestSubjectRepository implements SubjectRepository { return typeof message === 'string' ? message : null; } - private async errorMessageOf( response: Response ): Promise { - try { - const body = await response.json(); - return typeof body?.message === 'string' ? body.message : null; - } catch { - return null; - } - } - public async validateSubject( label: string | null, schemaName: SchemaName, diff --git a/resources/ext.neowiki/tests/persistence/RestSubjectRepositoryMove.unit.spec.ts b/resources/ext.neowiki/tests/persistence/RestSubjectRepositoryMove.unit.spec.ts index 93f0e185b..9c4e06907 100644 --- a/resources/ext.neowiki/tests/persistence/RestSubjectRepositoryMove.unit.spec.ts +++ b/resources/ext.neowiki/tests/persistence/RestSubjectRepositoryMove.unit.spec.ts @@ -72,14 +72,11 @@ describe( 'RestSubjectRepository.moveSubject', () => { .rejects.toThrow( 'Error moving subject' ); } ); - it( 'reads the message off a non-ok response too, for clients that do not reject', async () => { - const post = vi.fn().mockResolvedValue( { - ok: false, - json: async () => ( { status: 'error', message: 'Target page not found' } ), - } as unknown as Response ); + it( 'reports a non-ok response as a failed move', async () => { + const post = vi.fn().mockResolvedValue( { ok: false } as Response ); await expect( newRepository( { post } ).moveSubject( SUBJECT_ID, 12, false ) ) - .rejects.toThrow( 'Target page not found' ); + .rejects.toThrow( 'Error moving subject' ); } ); it( 'resolves when the move lands', async () => { diff --git a/src/Application/Actions/MoveSubject/MoveSubjectAction.php b/src/Application/Actions/MoveSubject/MoveSubjectAction.php index 0a2d94e8c..e37857fe5 100644 --- a/src/Application/Actions/MoveSubject/MoveSubjectAction.php +++ b/src/Application/Actions/MoveSubject/MoveSubjectAction.php @@ -96,14 +96,19 @@ public function moveSubject( MoveSubjectRequest $request ): void { throw new RuntimeException( 'You do not have the necessary permissions to move this subject' ); } - // The source page as it stands, kept aside untouched: reading it again is how the rollback - // below restores the page exactly, ordering included, rather than reconstructing it. - $sourceSubjectsBeforeMove = $this->subjectRepository->getSubjectsByPageId( $sourcePageId ); - - $sourceSubjects->removeSubject( $subjectId ); + // without() answers a copy, so $sourceSubjects stays the page as it was read - which is what + // the rollback below writes back, ordering included, rather than reconstructing it. + $sourceSubjectsAfterMove = $sourceSubjects->without( $subjectId ); $this->addToTarget( $targetSubjects, $subject, $request->makeMainSubject ); - $this->write( $request, $sourceSubjects, $sourceSubjectsBeforeMove, $sourcePageId, $targetSubjects, $targetPageId ); + $this->write( + request: $request, + sourcePageId: $sourcePageId, + sourceSubjectsAfterMove: $sourceSubjectsAfterMove, + sourceSubjectsBeforeMove: $sourceSubjects, + targetPageId: $targetPageId, + targetSubjects: $targetSubjects + ); } private function addToTarget( PageSubjects $targetSubjects, Subject $subject, bool $makeMainSubject ): void { @@ -124,18 +129,20 @@ private function addToTarget( PageSubjects $targetSubjects, Subject $subject, bo private function write( MoveSubjectRequest $request, - PageSubjects $sourceSubjects, - PageSubjects $sourceSubjectsBeforeMove, PageId $sourcePageId, - PageSubjects $targetSubjects, - PageId $targetPageId + PageSubjects $sourceSubjectsAfterMove, + PageSubjects $sourceSubjectsBeforeMove, + PageId $targetPageId, + PageSubjects $targetSubjects ): void { // Source first, for the projection reason in the class docblock: the target write has to be // the last word on the moved Subject's node. - $sourceStatus = $this->subjectRepository->savePageSubjects( $sourceSubjects, $sourcePageId, $request->comment ); + $sourceStatus = $this->subjectRepository->savePageSubjects( $sourceSubjectsAfterMove, $sourcePageId, $request->comment ); + // The source page went away under the write, which from the caller's side is the Subject + // going away. Nothing has been written, as the source page is written first. if ( $sourceStatus->status === PageContentSavingStatus::ERROR ) { - $this->presenter->presentSourcePageNotFound(); + $this->presenter->presentSubjectNotFound(); return; } diff --git a/src/Application/Actions/MoveSubject/MoveSubjectPresenter.php b/src/Application/Actions/MoveSubject/MoveSubjectPresenter.php index ec33ebc95..b38994b88 100644 --- a/src/Application/Actions/MoveSubject/MoveSubjectPresenter.php +++ b/src/Application/Actions/MoveSubject/MoveSubjectPresenter.php @@ -14,9 +14,11 @@ public function presentMoved(): void; public function presentNoChange(): void; /** - * Called when no page hosts the Subject, when the page that does no longer holds it, and when - * the caller may not read that page. All three take this one shape so a Subject on a hidden page - * cannot be told apart from one that does not exist. + * Called when no page hosts the Subject, when the page that does no longer holds it, when the + * caller may not read that page, and when that page went away between the read check and its + * write - by which point nothing has been written, since the source page is written first. All + * four take this one shape so a Subject on a hidden page cannot be told apart from one that does + * not exist. */ public function presentSubjectNotFound(): void; @@ -28,12 +30,6 @@ public function presentSubjectNotFound(): void; */ public function presentTargetPageNotFound(): void; - /** - * Called when the source page went away between the read check and its write. Nothing changed: - * it is the first page written, so the target page has not been touched. - */ - public function presentSourcePageNotFound(): void; - public function presentSubjectAlreadyOnTargetPage(): void; /** diff --git a/src/Domain/Page/PageSubjects.php b/src/Domain/Page/PageSubjects.php index 28397794e..57e8c59f7 100644 --- a/src/Domain/Page/PageSubjects.php +++ b/src/Domain/Page/PageSubjects.php @@ -64,6 +64,16 @@ public function removeSubject( SubjectId $id ): void { } } + /** + * A copy without the given Subject, leaving this instance untouched. + */ + public function without( SubjectId $id ): self { + return new self( + $this->isMainSubject( $id ) ? null : $this->mainSubject, + $this->childSubjects->without( $id ) + ); + } + /** * Updates the subject with the ID of the provided subject. * @throws OutOfBoundsException if the subject is not found diff --git a/src/Presentation/RestMoveSubjectPresenter.php b/src/Presentation/RestMoveSubjectPresenter.php index f1f688a9e..747ce082f 100644 --- a/src/Presentation/RestMoveSubjectPresenter.php +++ b/src/Presentation/RestMoveSubjectPresenter.php @@ -39,11 +39,6 @@ public function presentTargetPageNotFound(): void { $this->statusCode = 404; } - public function presentSourcePageNotFound(): void { - $this->apiResponse = [ 'status' => 'error', 'message' => 'Page not found' ]; - $this->statusCode = 404; - } - public function presentSubjectAlreadyOnTargetPage(): void { $this->apiResponse = [ 'status' => 'error', 'message' => 'Subject is already on the target page' ]; $this->statusCode = 409; diff --git a/tests/phpunit/Application/Actions/MoveSubjectActionTest.php b/tests/phpunit/Application/Actions/MoveSubjectActionTest.php index b9b3abe16..1ad5ddbf1 100644 --- a/tests/phpunit/Application/Actions/MoveSubjectActionTest.php +++ b/tests/phpunit/Application/Actions/MoveSubjectActionTest.php @@ -280,7 +280,7 @@ public function testFailingSourceSaveLeavesBothPagesUntouched(): void { $presenter = $this->newSpyPresenter(); $this->newAction( $presenter, $repository )->moveSubject( $this->newRequest() ); - $this->assertTrue( $presenter->sourcePageNotFound ); + $this->assertTrue( $presenter->subjectNotFound ); $this->assertFalse( $presenter->moved ); $source = $repository->getSubjectsByPageId( new PageId( self::SOURCE_PAGE_ID ) ); @@ -476,7 +476,6 @@ private function newSpyPresenter(): object { public bool $noChange = false; public bool $subjectNotFound = false; public bool $targetPageNotFound = false; - public bool $sourcePageNotFound = false; public bool $alreadyOnTargetPage = false; public bool $moveIncomplete = false; @@ -496,10 +495,6 @@ public function presentTargetPageNotFound(): void { $this->targetPageNotFound = true; } - public function presentSourcePageNotFound(): void { - $this->sourcePageNotFound = true; - } - public function presentSubjectAlreadyOnTargetPage(): void { $this->alreadyOnTargetPage = true; } diff --git a/tests/phpunit/Domain/Page/PageSubjectsTest.php b/tests/phpunit/Domain/Page/PageSubjectsTest.php index b32ee1521..df5e849a1 100644 --- a/tests/phpunit/Domain/Page/PageSubjectsTest.php +++ b/tests/phpunit/Domain/Page/PageSubjectsTest.php @@ -63,6 +63,61 @@ public function testRemoveChildSubject(): void { ); } + public function testWithoutChildSubjectAnswersACopyLackingIt(): void { + $mainSubject = TestSubject::build( TestSubject::uniqueId() ); + $firstChild = TestSubject::build( TestSubject::uniqueId() ); + $secondChild = TestSubject::build( TestSubject::uniqueId() ); + $thirdChild = TestSubject::build( TestSubject::uniqueId() ); + + $data = new PageSubjects( $mainSubject, new SubjectMap( $firstChild, $secondChild, $thirdChild ) ); + + $remaining = $data->without( $secondChild->id ); + + $this->assertSame( $mainSubject, $remaining->getMainSubject() ); + $this->assertEquals( + new SubjectMap( $firstChild, $thirdChild ), + $remaining->getChildSubjects() + ); + } + + public function testWithoutMainSubjectAnswersACopyWithoutOne(): void { + $firstChild = TestSubject::build( TestSubject::uniqueId() ); + $secondChild = TestSubject::build( TestSubject::uniqueId() ); + $mainSubject = TestSubject::build( TestSubject::uniqueId() ); + + $data = new PageSubjects( $mainSubject, new SubjectMap( $firstChild, $secondChild ) ); + + $remaining = $data->without( $mainSubject->id ); + + $this->assertNull( $remaining->getMainSubject() ); + $this->assertEquals( + new SubjectMap( $firstChild, $secondChild ), + $remaining->getChildSubjects() + ); + } + + public function testWithoutLeavesTheSubjectsItWasCalledOnAlone(): void { + // Moving a Subject keeps the page as it was read, so a failed move can write it back. + $mainSubject = TestSubject::build( TestSubject::uniqueId() ); + $child = TestSubject::build( TestSubject::uniqueId() ); + + $data = new PageSubjects( $mainSubject, new SubjectMap( $child ) ); + + $data->without( $mainSubject->id ); + + $this->assertSame( $mainSubject, $data->getMainSubject() ); + $this->assertEquals( new SubjectMap( $child ), $data->getChildSubjects() ); + } + + public function testWithoutAnIdThatIsNotOnThePageAnswersAnEqualCopy(): void { + $data = new PageSubjects( + TestSubject::build( TestSubject::uniqueId() ), + new SubjectMap( TestSubject::build( TestSubject::uniqueId() ) ) + ); + + $this->assertEquals( $data, $data->without( TestSubject::uniqueId() ) ); + } + public function testUpdateSubjectUpdatesTheMainSubject(): void { $mainSubject = TestSubject::build( TestSubject::uniqueId(), new SubjectLabel( 'original' ) ); $updatedSubject = TestSubject::build( $mainSubject->id->text, new SubjectLabel( 'updated' ) ); diff --git a/tests/phpunit/EntryPoints/REST/MoveSubjectApiTest.php b/tests/phpunit/EntryPoints/REST/MoveSubjectApiTest.php index aba4f5944..eb83d37a0 100644 --- a/tests/phpunit/EntryPoints/REST/MoveSubjectApiTest.php +++ b/tests/phpunit/EntryPoints/REST/MoveSubjectApiTest.php @@ -125,16 +125,6 @@ public function testPromotionMakesTheSubjectMainAndDemotesTheTargetsPreviousMain $this->assertTrue( $target->getChildSubjects()->hasSubject( new SubjectId( self::TARGET_MAIN_ID ) ) ); } - public function testBothPagesCarryTheSuppliedEditSummary(): void { - $this->executeHandler( - $this->newApi(), - $this->newRequest( body: [ 'targetPageId' => $this->targetPageId, 'comment' => 'Filed properly' ] ) - ); - - $this->assertSame( 'Filed properly', $this->latestCommentOf( $this->sourcePageId ) ); - $this->assertSame( 'Filed properly', $this->latestCommentOf( $this->targetPageId ) ); - } - public function testMovingToThePageTheSubjectIsAlreadyOnIsUnchanged(): void { $response = $this->executeHandler( $this->newApi(), @@ -184,16 +174,6 @@ public function testUnreadableTargetPageIsIndistinguishableFromNonexistentTarget ); } - public function testUnreadableTargetPageLeavesTheSourcePageUntouched(): void { - $this->executeHandler( - $this->newApi(), - $this->newRequest(), - authority: $this->authorityThatCannotReadPageId( $this->targetPageId ) - ); - - $this->assertTrue( $this->subjectsOf( $this->sourcePageId )->getAllSubjects()->hasSubject( $this->movedId() ) ); - } - public function testReadableButNotEditableTargetPageReturns403(): void { $response = $this->executeHandler( $this->newApi(), @@ -206,17 +186,6 @@ public function testReadableButNotEditableTargetPageReturns403(): void { $this->assertFalse( $this->subjectsOf( $this->targetPageId )->getAllSubjects()->hasSubject( $this->movedId() ) ); } - public function testReadableButNotEditableSourcePageReturns403(): void { - $response = $this->executeHandler( - $this->newApi(), - $this->newRequest(), - authority: $this->authorityThatCannotEditPageId( $this->sourcePageId ) - ); - - $this->assertSame( 403, $response->getStatusCode() ); - $this->assertFalse( $this->subjectsOf( $this->targetPageId )->getAllSubjects()->hasSubject( $this->movedId() ) ); - } - /** * @return string[] */ @@ -279,9 +248,4 @@ private function subjectsOf( int $pageId ): PageSubjects { ->getSubjectsByPageId( new PageId( $pageId ) ); } - private function latestCommentOf( int $pageId ): ?string { - return $this->getServiceContainer()->getRevisionLookup() - ->getRevisionByPageId( $pageId )?->getComment()?->text; - } - }