From f0c51e21f3797daf2cefa3669805f1e5c0f75494 Mon Sep 17 00:00:00 2001 From: Lucas Werkmeister Date: Tue, 6 Feb 2024 18:21:01 +0100 Subject: [PATCH] Use EditEntity for MergeLexemesInteractor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This lets EditEntity take care of various checks and other concerns, and will be especially useful for IP Masking (EditEntity already knows how to create temp users); compare Wikibase change I919db202f9. The code for the bot flag, including the TODO about EDIT_IGNORE_CONSTRAINTS, is taken from MediaWikiLexemeRepository; once we change the getLexeme() calls in the interactor to use an entity (revision?) lookup, LexemeRepository will be completely unused and we’ll be able to remove it, but let’s do that in separate changes. MergeLexemesInteractorTest is turned into an integration test (I’m not sure it was 100% unit before anyway), using the real entity store, lookup and EditEntity factory. Bug: T356149 Bug: T356764 Change-Id: Iae0c7c3b979118559c9ce2276618c6cdec11e63d --- WikibaseLexeme.mediawiki-services.php | 4 +- .../MergeLexemes/MergeLexemesInteractor.php | 67 ++++++++++---- .../Merge/MergeLexemesInteractorTest.php | 88 +++++++++++-------- 3 files changed, 104 insertions(+), 55 deletions(-) diff --git a/WikibaseLexeme.mediawiki-services.php b/WikibaseLexeme.mediawiki-services.php index 3d635a1045..152ff221bf 100644 --- a/WikibaseLexeme.mediawiki-services.php +++ b/WikibaseLexeme.mediawiki-services.php @@ -269,9 +269,11 @@ static function ( MediaWikiServices $mediawikiServices ): EntityLookupLemmaLooku $summaryFormatter, $lexemeRedirectorFactory, $entityPermissionChecker, + $mediaWikiServices->getPermissionManager(), $entityTitleStoreLookup, $mediaWikiServices->getWatchedItemStore(), - $lexemeRepositoryFactory + $lexemeRepositoryFactory, + WikibaseRepo::getEditEntityFactory( $mediaWikiServices ) ); }, ]; diff --git a/src/Interactors/MergeLexemes/MergeLexemesInteractor.php b/src/Interactors/MergeLexemes/MergeLexemesInteractor.php index 581501a0ec..b1eadaefac 100644 --- a/src/Interactors/MergeLexemes/MergeLexemesInteractor.php +++ b/src/Interactors/MergeLexemes/MergeLexemesInteractor.php @@ -3,6 +3,7 @@ namespace Wikibase\Lexeme\Interactors\MergeLexemes; use IContextSource; +use MediaWiki\Permissions\PermissionManager; use WatchedItemStoreInterface; use Wikibase\DataModel\Entity\EntityDocument; use Wikibase\Lexeme\DataAccess\Store\MediaWikiLexemeRedirectorFactory; @@ -21,9 +22,12 @@ use Wikibase\Lexeme\Domain\Storage\UpdateLexemeException; use Wikibase\Lib\FormatableSummary; use Wikibase\Lib\Summary; +use Wikibase\Repo\Content\EntityContent; +use Wikibase\Repo\EditEntity\MediaWikiEditEntityFactory; use Wikibase\Repo\Store\EntityPermissionChecker; use Wikibase\Repo\Store\EntityTitleStoreLookup; use Wikibase\Repo\SummaryFormatter; +use Wikibase\Repo\WikibaseRepo; /** * @license GPL-2.0-or-later @@ -47,6 +51,8 @@ class MergeLexemesInteractor { private EntityPermissionChecker $permissionChecker; + private PermissionManager $permissionManager; + /** * @var EntityTitleStoreLookup */ @@ -62,22 +68,28 @@ class MergeLexemesInteractor { */ private $watchedItemStore; + private MediaWikiEditEntityFactory $editEntityFactory; + public function __construct( LexemeMerger $lexemeMerger, SummaryFormatter $summaryFormatter, MediaWikiLexemeRedirectorFactory $lexemeRedirectorFactory, EntityPermissionChecker $permissionChecker, + PermissionManager $permissionManager, EntityTitleStoreLookup $entityTitleLookup, WatchedItemStoreInterface $watchedItemStore, - MediaWikiLexemeRepositoryFactory $repoFactory + MediaWikiLexemeRepositoryFactory $repoFactory, + MediaWikiEditEntityFactory $editEntityFactory ) { $this->lexemeMerger = $lexemeMerger; $this->summaryFormatter = $summaryFormatter; $this->lexemeRedirectorFactory = $lexemeRedirectorFactory; $this->permissionChecker = $permissionChecker; + $this->permissionManager = $permissionManager; $this->entityTitleLookup = $entityTitleLookup; $this->watchedItemStore = $watchedItemStore; $this->repoFactory = $repoFactory; + $this->editEntityFactory = $editEntityFactory; } /** @@ -101,6 +113,7 @@ public function mergeLexemes( $repo = $this->repoFactory->newFromContext( $context, $botEditRequested, $tags ); + // TODO replace repo with an EntityLookup $source = $this->getLexeme( $repo, $sourceId ); $target = $this->getLexeme( $repo, $targetId ); @@ -108,7 +121,7 @@ public function mergeLexemes( $this->lexemeMerger->merge( $source, $target ); - $this->attemptSaveMerge( $repo, $source, $target, $summary ); + $this->attemptSaveMerge( $source, $target, $context, $summary, $botEditRequested, $tags ); $this->updateWatchlistEntries( $sourceId, $targetId ); $this->lexemeRedirectorFactory @@ -173,37 +186,57 @@ private function getSummary( $direction, LexemeId $id, $customSummary = null ) { } private function attemptSaveMerge( - LexemeRepository $repo, Lexeme $source, Lexeme $target, - ?string $summary + IContextSource $context, + ?string $summary, + bool $botEditRequested, + array $tags ) { $this->saveLexeme( - $repo, $source, - $this->getSummary( 'to', $target->getId(), $summary ) + $context, + $this->getSummary( 'to', $target->getId(), $summary ), + $botEditRequested, + $tags ); $this->saveLexeme( - $repo, $target, - $this->getSummary( 'from', $source->getId(), $summary ) + $context, + $this->getSummary( 'from', $source->getId(), $summary ), + $botEditRequested, + $tags ); } private function saveLexeme( - LexemeRepository $repo, Lexeme $lexeme, - FormatableSummary $summary + IContextSource $context, + FormatableSummary $summary, + bool $botEditRequested, + array $tags ) { + // TODO: the EntityContent::EDIT_IGNORE_CONSTRAINTS flag does not seem to be used by Lexeme + // (LexemeHandler has no onSaveValidators) + $flags = EDIT_UPDATE | EntityContent::EDIT_IGNORE_CONSTRAINTS; + if ( $botEditRequested && $this->permissionManager->userHasRight( $context->getUser(), 'bot' ) ) { + $flags |= EDIT_FORCE_BOT; + } - try { - $repo->updateLexeme( - $lexeme, - $this->summaryFormatter->formatSummary( $summary ) - ); - } catch ( UpdateLexemeException $ex ) { - throw new LexemeSaveFailedException( $ex->getMessage(), $ex->getCode(), $ex ); + $formattedSummary = $this->summaryFormatter->formatSummary( $summary ); + + $editEntity = $this->editEntityFactory->newEditEntity( $context, $lexeme->getId() ); + $status = $editEntity->attemptSave( + $lexeme, + $formattedSummary, + $flags, + false, + null, + $tags + ); + if ( !$status->isOK() ) { + throw new LexemeSaveFailedException( $status->getWikiText() ); } } diff --git a/tests/phpunit/composer/Merge/MergeLexemesInteractorTest.php b/tests/phpunit/composer/Merge/MergeLexemesInteractorTest.php index 3ad828500f..f7d0495538 100644 --- a/tests/phpunit/composer/Merge/MergeLexemesInteractorTest.php +++ b/tests/phpunit/composer/Merge/MergeLexemesInteractorTest.php @@ -5,9 +5,8 @@ use IContextSource; use MediaWiki\Status\Status; use MediaWiki\Title\Title; -use MediaWiki\User\User; +use MediaWikiIntegrationTestCase; use PHPUnit\Framework\MockObject\MockObject; -use PHPUnit\Framework\TestCase; use WatchedItemStoreInterface; use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Services\Statement\GuidGenerator; @@ -26,9 +25,12 @@ use Wikibase\Lexeme\Domain\Merge\NoCrossReferencingLexemeStatements; use Wikibase\Lexeme\Domain\Model\Lexeme; use Wikibase\Lexeme\Interactors\MergeLexemes\MergeLexemesInteractor; -use Wikibase\Lexeme\Tests\TestDoubles\FakeLexemeRepository; use Wikibase\Lexeme\Tests\Unit\DataModel\NewLexeme; -use Wikibase\Lib\Store\EntityStore; +use Wikibase\Lib\Store\EntityRevisionLookup; +use Wikibase\Lib\Store\LatestRevisionIdResult; +use Wikibase\Lib\Store\StorageException; +use Wikibase\Repo\EditEntity\EditEntity; +use Wikibase\Repo\EditEntity\MediaWikiEditEntityFactory; use Wikibase\Repo\Store\EntityPermissionChecker; use Wikibase\Repo\Store\EntityTitleStoreLookup; use Wikibase\Repo\SummaryFormatter; @@ -37,20 +39,16 @@ /** * @covers \Wikibase\Lexeme\Interactors\MergeLexemes\MergeLexemesInteractor * + * @group Database * @license GPL-2.0-or-later */ -class MergeLexemesInteractorTest extends TestCase { +class MergeLexemesInteractorTest extends MediaWikiIntegrationTestCase { /** * @var LexemeMerger|MockObject */ private $lexemeMerger; - /** - * @var EntityStore|MockObject - */ - private $entityStore; - /** * @var EntityPermissionChecker */ @@ -85,20 +83,12 @@ class MergeLexemesInteractorTest extends TestCase { private $watchedItemStore; /** - * @var FakeLexemeRepository - */ - private $lexemeRepository; - - /** @var MediaWikiLexemeRepositoryFactory|MockObject */ - private $lexemeRepositoryFactory; - - /** - * @var Lexeme + * @var ?Lexeme */ private $sourceLexeme; /** - * @var Lexeme + * @var ?Lexeme */ private $targetLexeme; @@ -108,12 +98,6 @@ protected function setUp(): void { $this->sourceLexeme = NewLexeme::havingId( 'L123' )->build(); $this->targetLexeme = NewLexeme::havingId( 'L321' )->build(); - $this->lexemeRepository = new FakeLexemeRepository( $this->sourceLexeme, $this->targetLexeme ); - $this->lexemeRepositoryFactory = $this->createMock( MediaWikiLexemeRepositoryFactory::class ); - $this->lexemeRepositoryFactory->method( 'newFromContext' ) - ->willReturnCallback( function () { - return $this->lexemeRepository; - } ); $this->lexemeMerger = $this->createMock( LexemeMerger::class ); $this->permissionChecker = $this->createConfiguredMock( EntityPermissionChecker::class, [ 'getPermissionStatusForEntityId' => Status::newGood(), @@ -121,7 +105,7 @@ protected function setUp(): void { $this->summaryFormatter = $this->newMockSummaryFormatter(); $this->context = $this->createMock( IContextSource::class ); $this->context->method( 'getUser' ) - ->willReturn( $this->createMock( User::class ) ); + ->willReturnCallback( fn() => $this->getTestUser()->getUser() ); [ $this->redirector, $this->redirectorFactory ] = $this->newMockRedirectorAndFactory(); $this->entityTitleLookup = $this->newMockTitleLookup(); $this->watchedItemStore = $this->createMock( WatchedItemStoreInterface::class ); @@ -139,8 +123,6 @@ public function testGivenMergeSucceeds_targetIsChangedCorrectly() { ->withLemma( 'en-gb', 'sand box' ) ->build(); - $this->lexemeRepository = new FakeLexemeRepository( $this->sourceLexeme, $this->targetLexeme ); - $this->redirector->expects( $this->once() ) ->method( 'redirect' ) ->with( $this->sourceLexeme->getId(), $this->targetLexeme->getId() ); @@ -170,7 +152,9 @@ public function testGivenMergeSucceeds_targetIsChangedCorrectly() { $this->assertCount( 2, - $this->lexemeRepository->getLexemeById( $this->targetLexeme->getId() )->getLemmas() + WikibaseRepo::getEntityLookup( $this->getServiceContainer() ) + ->getEntity( $this->targetLexeme->getId() ) + ->getLemmas() ); } @@ -215,23 +199,30 @@ public function testGivenUserDoesNotHavePermission_throwsException() { } public function testGivenSourceNotFound_throwsException() { - $this->lexemeRepository = new FakeLexemeRepository( $this->targetLexeme ); + $sourceLexemeId = $this->sourceLexeme->getId(); + $this->sourceLexeme = null; $this->expectException( LexemeNotFoundException::class ); $this->newMergeInteractor() - ->mergeLexemes( $this->sourceLexeme->getId(), $this->targetLexeme->getId(), $this->context ); + ->mergeLexemes( $sourceLexemeId, $this->targetLexeme->getId(), $this->context ); } public function testGivenTargetNotFound_throwsException() { - $this->lexemeRepository = new FakeLexemeRepository( $this->sourceLexeme ); + $targetLexemeId = $this->targetLexeme->getId(); + $this->targetLexeme = null; $this->expectException( LexemeNotFoundException::class ); $this->newMergeInteractor() - ->mergeLexemes( $this->sourceLexeme->getId(), $this->targetLexeme->getId(), $this->context ); + ->mergeLexemes( $this->sourceLexeme->getId(), $targetLexemeId, $this->context ); } public function testGivenExceptionInLoadEntity_throwsAppropriateException() { - $this->lexemeRepository->throwOnRead(); + $throwingEntityRevisionLookup = $this->createMock( EntityRevisionLookup::class ); + $throwingEntityRevisionLookup->method( 'getLatestRevisionId' ) + ->willReturn( LatestRevisionIdResult::concreteRevision( 123, '123' ) ); + $throwingEntityRevisionLookup->method( 'getEntityRevision' ) + ->willThrowException( new StorageException() ); + $this->setService( 'WikibaseRepo.EntityRevisionLookup', $throwingEntityRevisionLookup ); $this->expectException( LexemeLoadingException::class ); $this->newMergeInteractor() @@ -239,7 +230,13 @@ public function testGivenExceptionInLoadEntity_throwsAppropriateException() { } public function testGivenEntitySaveFails_throwsException() { - $this->lexemeRepository->throwOnWrite(); + $failingEditEntity = $this->createMock( EditEntity::class ); + $failingEditEntity->method( 'attemptSave' ) + ->willReturn( Status::newFatal( 'failed-save' ) ); + $this->setService( 'WikibaseRepo.EditEntityFactory', + $this->createConfiguredMock( MediaWikiEditEntityFactory::class, [ + 'newEditEntity' => $failingEditEntity, + ] ) ); $this->expectException( LexemeSaveFailedException::class ); $this->newMergeInteractor() @@ -247,14 +244,31 @@ public function testGivenEntitySaveFails_throwsException() { } private function newMergeInteractor() { + $services = $this->getServiceContainer(); + $entityStore = WikibaseRepo::getEntityStore( $services ); + $user = $this->getTestUser()->getUser(); + if ( $this->sourceLexeme !== null ) { + $entityStore->saveEntity( $this->sourceLexeme, 'test setup', $user ); + } + if ( $this->targetLexeme !== null ) { + $entityStore->saveEntity( $this->targetLexeme, 'test setup', $user ); + } + $permissionManager = $services->getPermissionManager(); + $lexemeRepositoryFactory = new MediaWikiLexemeRepositoryFactory( + $entityStore, + WikibaseRepo::getEntityRevisionLookup( $services ), $permissionManager + ); + return new MergeLexemesInteractor( $this->lexemeMerger, $this->summaryFormatter, $this->redirectorFactory, $this->permissionChecker, + $permissionManager, $this->entityTitleLookup, $this->watchedItemStore, - $this->lexemeRepositoryFactory + $lexemeRepositoryFactory, + WikibaseRepo::getEditEntityFactory( $services ) ); } -- 2.43.0