From d20cead930bd94976f30eb73199b356051c343c0 Mon Sep 17 00:00:00 2001 From: thiemowmde Date: Tue, 7 Nov 2023 14:02:26 +0100 Subject: [PATCH] Split large hook handler function into named methods Change-Id: Ided2c233829166bd691f01cb8b619f03c1ca4ff2 --- src/RevisionSliderHooks.php | 84 ++++++++++++----------- tests/phpunit/RevisionSliderHooksTest.php | 30 ++++---- 2 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/RevisionSliderHooks.php b/src/RevisionSliderHooks.php index 48d4fc8f..0ebba991 100644 --- a/src/RevisionSliderHooks.php +++ b/src/RevisionSliderHooks.php @@ -2,13 +2,15 @@ namespace MediaWiki\Extension\RevisionSlider; -use Config; -use ConfigFactory; -use Html; use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; +use MediaWiki\Config\Config; +use MediaWiki\Config\ConfigFactory; use MediaWiki\Diff\Hook\DifferenceEngineViewHeaderHook; +use MediaWiki\Html\Html; use MediaWiki\MainConfigNames; use MediaWiki\Preferences\Hook\GetPreferencesHook; +use MediaWiki\Revision\RevisionRecord; +use MediaWiki\User\User; use MediaWiki\User\UserOptionsLookup; use Message; use OOUI\ButtonWidget; @@ -43,12 +45,6 @@ class RevisionSliderHooks implements DifferenceEngineViewHeaderHook, GetPreferen $oldRevRecord = $differenceEngine->getOldRevision(); $newRevRecord = $differenceEngine->getNewRevision(); - // sometimes the old revision can be null (e.g. missing rev), and perhaps also the - // new one (T167359) - if ( $oldRevRecord === null || $newRevRecord === null ) { - return; - } - // do not show on MobileDiff page // Note: Since T245172, DifferenceEngine::getTitle() is the title of the page being diffed. if ( $differenceEngine->getOutput()->getTitle()->isSpecial( 'MobileDiff' ) ) { @@ -59,21 +55,7 @@ class RevisionSliderHooks implements DifferenceEngineViewHeaderHook, GetPreferen * If the user is logged in and has explictly requested to disable the extension don't load. */ $user = $differenceEngine->getUser(); - if ( $user->isNamed() && $this->userOptionsLookup->getBoolOption( $user, 'revisionslider-disable' ) ) { - return; - } - - /** - * Do not show the RevisionSlider when revisions from two different pages are being compared - * - * Since RevisionRecord::getPageAsLinkTarget only returns a LinkTarget, which doesn't - * have an equals method, compare manually by namespace and text - */ - $oldTitle = $oldRevRecord->getPageAsLinkTarget(); - $newTitle = $newRevRecord->getPageAsLinkTarget(); - if ( $oldTitle->getNamespace() !== $newTitle->getNamespace() || - $oldTitle->getDBKey() !== $newTitle->getDBKey() - ) { + if ( $this->isDisabled( $user ) || !$this->isSamePage( $oldRevRecord, $newRevRecord ) ) { return; } @@ -100,6 +82,34 @@ class RevisionSliderHooks implements DifferenceEngineViewHeaderHook, GetPreferen $out->addJsConfigVars( 'extRevisionSliderTimeOffset', intval( $timeOffset ) ); $out->enableOOUI(); + $out->prependHTML( $this->getContainerHtml( $autoExpand ) ); + } + + public function isDisabled( User $user ): bool { + return $user->isNamed() && + $this->userOptionsLookup->getBoolOption( $user, 'revisionslider-disable' ); + } + + private function isSamePage( ?RevisionRecord $oldRevRecord, ?RevisionRecord $newRevRecord ): bool { + // sometimes the old revision can be null (e.g. missing rev), and perhaps also the + // new one (T167359) + if ( !$oldRevRecord || !$newRevRecord ) { + return false; + } + + /** + * Do not show the RevisionSlider when revisions from two different pages are being compared + * + * Since RevisionRecord::getPageAsLinkTarget only returns a LinkTarget, which doesn't + * have an equals method, compare manually by namespace and text + */ + $oldTitle = $oldRevRecord->getPageAsLinkTarget(); + $newTitle = $newRevRecord->getPageAsLinkTarget(); + return $oldTitle->getNamespace() === $newTitle->getNamespace() && + $oldTitle->getDBKey() === $newTitle->getDBKey(); + } + + private function getContainerHtml( bool $autoExpand ): string { $toggleButton = new ButtonWidget( [ 'label' => ( new Message( 'revisionslider-toggle-label' ) )->text(), 'indicator' => 'down', @@ -117,21 +127,17 @@ class RevisionSliderHooks implements DifferenceEngineViewHeaderHook, GetPreferen ) ); - $out->prependHTML( - Html::rawElement( - 'div', - [ 'class' => 'mw-revslider-container' ], - $toggleButton . - Html::rawElement( - 'div', - [ - 'class' => 'mw-revslider-slider-wrapper', - 'style' => ( !$autoExpand ? ' display: none;' : '' ), - ], - Html::rawElement( - 'div', [ 'class' => 'mw-revslider-placeholder' ], - $loadingSpinner - ) + return Html::rawElement( 'div', + [ 'class' => 'mw-revslider-container' ], + $toggleButton . + Html::rawElement( 'div', + [ + 'class' => 'mw-revslider-slider-wrapper', + 'style' => $autoExpand ? null : 'display: none;', + ], + Html::rawElement( 'div', + [ 'class' => 'mw-revslider-placeholder' ], + $loadingSpinner ) ) ); diff --git a/tests/phpunit/RevisionSliderHooksTest.php b/tests/phpunit/RevisionSliderHooksTest.php index f3169f2a..8edff107 100644 --- a/tests/phpunit/RevisionSliderHooksTest.php +++ b/tests/phpunit/RevisionSliderHooksTest.php @@ -10,12 +10,17 @@ use MediaWiki\User\StaticUserOptionsLookup; class RevisionSliderHooksTest extends \MediaWikiIntegrationTestCase { public function testShouldNotLoadWithoutRevisions() { - // Assert - $output = null; - // Arrange + $output = $this->createMock( OutputPage::class ); + $output->method( 'getTitle' ) + ->willReturn( $this->createMock( Title::class ) ); + $diffEngine = $this->newDiffEngine( null, $output ); + // Assert + $output->expects( $this->never() ) + ->method( 'addModules' ); + // Act $this->newInstance()->onDifferenceEngineViewHeader( $diffEngine ); } @@ -45,16 +50,13 @@ class RevisionSliderHooksTest extends \MediaWikiIntegrationTestCase { public function testShouldNotLoadWhenUserIsLoggedInAndDisabledExtension() { // Arrange $options = [ 'revisionslider-disable' => true ]; - $user = $this->createMock( User::class ); - $user->method( 'isNamed' ) - ->willReturn( true ); $output = $this->createMock( OutputPage::class ); $output->method( 'getTitle' ) ->willReturn( $this->createMock( Title::class ) ); $revision = $this->createMock( RevisionRecord::class ); - $diffEngine = $this->newDiffEngine( $revision, $output, $user ); + $diffEngine = $this->newDiffEngine( $revision, $output, true ); // Assert $output->expects( $this->never() ) @@ -94,19 +96,21 @@ class RevisionSliderHooksTest extends \MediaWikiIntegrationTestCase { private function newDiffEngine( ?RevisionRecord $revision, - ?OutputPage $output, - ?User $user = null + OutputPage $output, + bool $isNamed = false ): DifferenceEngine { + $user = $this->createMock( User::class ); + $user->method( 'isNamed' ) + ->willReturn( $isNamed ); + $diffEngine = $this->createMock( DifferenceEngine::class ); $diffEngine->method( 'getOldRevision' ) ->willReturn( $revision ); $diffEngine->method( 'getNewRevision' ) ->willReturn( $revision ); - $diffEngine->expects( $output ? $this->atLeastOnce() : $this->never() ) - ->method( 'getOutput' ) + $diffEngine->method( 'getOutput' ) ->willReturn( $output ); - $diffEngine->expects( $user ? $this->once() : $this->never() ) - ->method( 'getUser' ) + $diffEngine->method( 'getUser' ) ->willReturn( $user ); return $diffEngine; }