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

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<string | null> {
-		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,

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 () => {

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;
 		}
 

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;
 
 	/**

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

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;

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;
 			}

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' ) );

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;
-	}
-
 }