diff --git a/Popups.hooks.php b/Popups.hooks.php index 3dbb8bd92..ee9ea9757 100644 --- a/Popups.hooks.php +++ b/Popups.hooks.php @@ -50,9 +50,9 @@ class PopupsHooks { * @param array $prefs */ static function onGetPreferences( User $user, array &$prefs ) { - $module = PopupsContext::getInstance(); + $context = PopupsContext::getInstance(); - if ( !$module->showPreviewsOptInOnPreferencesPage() ) { + if ( !$context->showPreviewsOptInOnPreferencesPage() ) { return; } $option = [ @@ -66,6 +66,11 @@ class PopupsHooks { ], 'section' => self::PREVIEWS_PREFERENCES_SECTION ]; + if ( $context->conflictsWithNavPopupsGadget( $user ) ) { + $option[ 'disabled' ] = true; + $option[ 'help' ] = wfMessage( 'popups-prefs-disable-nav-gadgets-info', + 'Special:Preferences#mw-prefsection-gadgets' ); + } $skinPosition = array_search( 'skin', array_keys( $prefs ) ); if ( $skinPosition !== false ) { diff --git a/extension.json b/extension.json index 72cb9a77c..b6b4bc8ba 100644 --- a/extension.json +++ b/extension.json @@ -10,7 +10,8 @@ "type": "betafeatures", "AutoloadClasses": { "PopupsHooks": "Popups.hooks.php", - "Popups\\PopupsContext": "includes/PopupsContext.php" + "Popups\\PopupsContext": "includes/PopupsContext.php", + "Popups\\PopupsGadgetsIntegration": "includes/PopupsGadgetsIntegration.php" }, "ConfigRegistry": { "popups": "GlobalVarConfig::newInstance" diff --git a/i18n/en.json b/i18n/en.json index df2d6dd60..595fa1c27 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -24,5 +24,6 @@ "prefs-reading": "Reading preferences", "popups-prefs-optin-title": "Page previews\n\nGet quick previews of a topic while reading an article", "popups-prefs-optin-enabled-label": "Enable", - "popups-prefs-optin-disabled-label": "Disable" + "popups-prefs-optin-disabled-label": "Disable", + "popups-prefs-disable-nav-gadgets-info": "You have to [[$1 | disable Navigation Popups Gadget]] from Gadgets tab to enable Page Previews" } diff --git a/i18n/qqq.json b/i18n/qqq.json index 02b1e6c57..98a86c952 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -30,5 +30,6 @@ "prefs-reading": "Title for 'Reading preferences' section on preferences page", "popups-prefs-optin-title": "Title for Page Previews option\n\nDescription for Page previews option", "popups-prefs-optin-enabled-label": "Label for Page Previews opt in", - "popups-prefs-optin-disabled-label": "Label for Page previews opt out" + "popups-prefs-optin-disabled-label": "Label for Page previews opt out", + "popups-prefs-disable-nav-gadgets-info": "Help message telling to disable Navigation Popups gadget in order to enable Page Previews. Parameters: $1 - link to Preferences page" } diff --git a/includes/PopupsContext.php b/includes/PopupsContext.php index 151b796a0..cc2b9ac6d 100644 --- a/includes/PopupsContext.php +++ b/includes/PopupsContext.php @@ -77,9 +77,12 @@ class PopupsContext { * Module constructor. * @param ExtensionRegistry $extensionRegistry */ - protected function __construct( ExtensionRegistry $extensionRegistry ) { + protected function __construct( ExtensionRegistry $extensionRegistry, + PopupsGadgetsIntegration $gadgetsIntegration ) { /** @todo Use MediaWikiServices Service Locator when it's ready */ $this->extensionRegistry = $extensionRegistry; + $this->gadgetsIntegration = $gadgetsIntegration; + $this->config = MediaWikiServices::getInstance()->getConfigFactory() ->makeConfig( PopupsContext::EXTENSION_NAME ); } @@ -91,10 +94,20 @@ class PopupsContext { */ public static function getInstance() { if ( !self::$instance ) { - self::$instance = new PopupsContext( ExtensionRegistry::getInstance() ); + $registry = ExtensionRegistry::getInstance(); + self::$instance = new PopupsContext( $registry, + new PopupsGadgetsIntegration( $registry ) ); } return self::$instance; } + + /** + * @param \User $user + * @return bool + */ + public function conflictsWithNavPopupsGadget( \User $user ) { + return $this->gadgetsIntegration->conflictsWithNavPopupsGadget( $user ); + } /** * Is Beta Feature mode enabled * diff --git a/includes/PopupsGadgetsIntegration.php b/includes/PopupsGadgetsIntegration.php new file mode 100644 index 000000000..9a9d6c2bc --- /dev/null +++ b/includes/PopupsGadgetsIntegration.php @@ -0,0 +1,67 @@ +. +* +* @file +* @ingroup extensions +*/ +namespace Popups; + +/** +* Gadgets integration +* +* @package Popups +*/ +class PopupsGadgetsIntegration { + const NAVIGATION_POPUPS_NAME = 'Navigation_popups'; + + /** + * @var \ExtensionRegistry + */ + private $extensionRegistry; + + /** + * PopupsGadgetsIntegration constructor. + * @param \ExtensionRegistry $extensionRegistry + */ + public function __construct( \ExtensionRegistry $extensionRegistry ) { + $this->extensionRegistry = $extensionRegistry; + } + + /** + * @return bool + */ + private function isGadgetExtensionEnabled() { + return $this->extensionRegistry->isLoaded( 'Gadgets' ); + } + /** + * Check if Page Previews conflicts with Nav Popups Gadget + * If user enabled Nav Popups PagePreviews are not available + * + * @param \User $user + * @return bool + */ + public function conflictsWithNavPopupsGadget( \User $user ) { + if ( $this->isGadgetExtensionEnabled() ) { + $gadgetsRepo = \GadgetRepo::singleton(); + $match = array_search( self::NAVIGATION_POPUPS_NAME, $gadgetsRepo->getGadgetIds() ); + if ( $match !== false ) { + return $gadgetsRepo->getGadget( self::NAVIGATION_POPUPS_NAME )->isEnabled( $user ); + } + } + return false; + } +} diff --git a/tests/phpunit/PopupsContextTest.php b/tests/phpunit/PopupsContextTest.php index 35d00f95a..406dd4236 100644 --- a/tests/phpunit/PopupsContextTest.php +++ b/tests/phpunit/PopupsContextTest.php @@ -280,4 +280,24 @@ class PopupsContextTest extends MediaWikiTestCase { ] ); $this->assertEquals( "2", PopupsContext::getInstance()->getDefaultIsEnabledState() ); } + + /** + * @covers ::conflictsWithNavPopupsGadget + */ + public function testConflictsWithNavPopupsGadget() { + $integrationMock = $this->getMockBuilder( \Popups\PopupsGadgetsIntegration::class ) + ->disableOriginalConstructor() + ->setMethods( [ 'conflictsWithNavPopupsGadget' ] ) + ->getMock(); + + $user = $this->getTestUser()->getUser(); + + $integrationMock->expects( $this->once() ) + ->method( 'conflictsWithNavPopupsGadget' ) + ->with( $user ) + ->willReturn( true ); + + $context = new PopupsContextTestWrapper( ExtensionRegistry::getInstance(), $integrationMock ); + $this->assertEquals( true, $context->conflictsWithNavPopupsGadget( $user ) ); + } } diff --git a/tests/phpunit/PopupsContextTestWrapper.php b/tests/phpunit/PopupsContextTestWrapper.php index 938452946..12bc27772 100644 --- a/tests/phpunit/PopupsContextTestWrapper.php +++ b/tests/phpunit/PopupsContextTestWrapper.php @@ -31,15 +31,23 @@ use \Popups\PopupsContext; * Used for testing only * @codeCoverageIgnore */ +use Popups\PopupsGadgetsIntegration; + class PopupsContextTestWrapper extends PopupsContext { /** * Override constructor so we can create new instances for testing - * . + * * @param ExtensionRegistry $extensionRegistry + * @param PopupsGadgetsIntegration $gadgetsIntegration */ - public function __construct( ExtensionRegistry $extensionRegistry ) { - parent::__construct( $extensionRegistry ); + public function __construct( ExtensionRegistry $extensionRegistry, + PopupsGadgetsIntegration $gadgetsIntegration = null ) { + + $gadgetsIntegration = $gadgetsIntegration ? $gadgetsIntegration : + new PopupsGadgetsIntegration( $extensionRegistry ); + + parent::__construct( $extensionRegistry, $gadgetsIntegration ); } /** diff --git a/tests/phpunit/PopupsGadgetsIntegrationTest.php b/tests/phpunit/PopupsGadgetsIntegrationTest.php new file mode 100644 index 000000000..6ae8d8cdb --- /dev/null +++ b/tests/phpunit/PopupsGadgetsIntegrationTest.php @@ -0,0 +1,141 @@ +. +* +* @file +* @ingroup extensions +*/ + +use Popups\PopupsGadgetsIntegration; + +/** +* Popups module tests +* +* @group Popups +* @coversDefaultClass Popups\PopupsGadgetsIntegration +*/ +class PopupsGadgetsIntegrationTest extends MediaWikiTestCase +{ + /** + * Helper constants for easier reading + */ + const GADGET_ENABLED = true; + /** + * Helper constants for easier reading + */ + const GADGET_DISABLED = false; + + /** + * Checks if Gadgets extension is available + */ + private function checkRequiredDependencies() { + if ( !ExtensionRegistry::getInstance()->isLoaded( 'Gadgets' ) ) { + $this->markTestSkipped( 'Skipped as Gadgets extension is not available' ); + } + } + /** + * @param bool $gadgetsEnabled + * @return ExtensionRegistry + */ + private function getExtensionRegistryMock( $gadgetsEnabled ) { + $mock = $this->getMock( ExtensionRegistry::class, [ 'isLoaded' ] ); + $mock->expects( $this->any() ) + ->method( 'isLoaded' ) + ->with( 'Gadgets' ) + ->willReturn( $gadgetsEnabled ); + return $mock; + } + /** + * @covers ::conflictsWithNavPopupsGadget + * @covers ::isGadgetExtensionEnabled + * @covers ::__construct + */ + public function testConflictsWithNavPopupsGadgetIfGadgetsExtensionIsNotLoaded() { + $user = $this->getTestUser()->getUser(); + $integration = new PopupsGadgetsIntegration( $this->getExtensionRegistryMock( false ) ); + $this->assertEquals( false, $integration->conflictsWithNavPopupsGadget( $user ) ); + } + + /** + * @covers ::conflictsWithNavPopupsGadget + */ + public function testConflictsWithNavPopupsGadgetIfGadgetNotExists() { + $this->checkRequiredDependencies(); + + $user = $this->getTestUser()->getUser(); + + $gadgetRepoMock = $this->getMock( GadgetRepo::class, [ 'getGadgetIds', 'getGadget' ] ); + + $gadgetRepoMock->expects( $this->once() ) + ->method( 'getGadgetIds' ) + ->willReturn( [] ); + + $this->executeConflictsWithNavPopupsGadgetSafeCheck( $user, $gadgetRepoMock, + self::GADGET_DISABLED ); + } + + /** + * @covers ::conflictsWithNavPopupsGadget + */ + public function testConflictsWithNavPopupsGadgetIfGadgetExists() { + $this->checkRequiredDependencies(); + + $user = $this->getTestUser()->getUser(); + + $gadgetMock = $this->getMockBuilder( Gadget::class ) + ->setMethods( [ 'isEnabled', 'getGadget' ] ) + ->disableOriginalConstructor() + ->getMock(); + + $gadgetMock->expects( $this->once() ) + ->method( 'isEnabled' ) + ->with( $user ) + ->willReturn( self::GADGET_ENABLED ); + + $gadgetRepoMock = $this->getMock( GadgetRepo::class, + [ 'getGadgetIds', 'getGadget' ] ); + + $gadgetRepoMock->expects( $this->once() ) + ->method( 'getGadgetids' ) + ->willReturn( [ 'Navigation_popups' ] ); + + $gadgetRepoMock->expects( $this->once() ) + ->method( 'getGadget' ) + ->with( 'Navigation_popups' ) + ->willReturn( $gadgetMock ); + + $this->executeConflictsWithNavPopupsGadgetSafeCheck( $user, $gadgetRepoMock, + self::GADGET_ENABLED ); + } + + /** + * Execute test and restore orGadgetRepo + * + * @param $user + * @param $repoMock + * @param $expected + */ + private function executeConflictsWithNavPopupsGadgetSafeCheck( $user, $repoMock, $expected ) { + $origGadgetsRepo = GadgetRepo::singleton(); + GadgetRepo::setSingleton( $repoMock ); + + $integration = new PopupsGadgetsIntegration( $this->getExtensionRegistryMock( true ) ); + $this->assertEquals( $expected, $integration->conflictsWithNavPopupsGadget( $user ) ); + + GadgetRepo::setSingleton( $origGadgetsRepo ); + } + +} diff --git a/tests/phpunit/PopupsHooksTest.php b/tests/phpunit/PopupsHooksTest.php index 6c79b032f..9dc0ccfbf 100644 --- a/tests/phpunit/PopupsHooksTest.php +++ b/tests/phpunit/PopupsHooksTest.php @@ -19,6 +19,7 @@ * @ingroup extensions */ require_once ( 'PopupsContextTestWrapper.php' ); +use Popups\PopupsContext; /** * Integration tests for Page Preview hooks @@ -54,7 +55,7 @@ class PopupsHooksTest extends MediaWikiTestCase { PopupsHooks::onGetBetaPreferences( $this->getTestUser()->getUser(), $prefs ); $this->assertCount( 2, $prefs ); - $this->assertArrayHasKey( \Popups\PopupsContext::PREVIEWS_BETA_PREFERENCE_NAME, $prefs ); + $this->assertArrayHasKey( PopupsContext::PREVIEWS_BETA_PREFERENCE_NAME, $prefs ); } /** @@ -75,6 +76,36 @@ class PopupsHooksTest extends MediaWikiTestCase { $this->assertEquals( 'notEmpty', $prefs[ 'someNotEmptyValue'] ); } + /** + * @covers ::onGetPreferences + */ + public function testOnGetPreferencesNavPopupGadgetIsOn() { + $userMock = $this->getTestUser()->getUser(); + $contextMock = $this->getMock( PopupsContextTestWrapper::class, + [ 'showPreviewsOptInOnPreferencesPage', 'conflictsWithNavPopupsGadget' ], + [ ExtensionRegistry::getInstance() ] ); + + $contextMock->expects( $this->once() ) + ->method( 'showPreviewsOptInOnPreferencesPage' ) + ->will( $this->returnValue( true ) ); + + $contextMock->expects( $this->once() ) + ->method( 'conflictsWithNavPopupsGadget' ) + ->with( $userMock ) + ->will( $this->returnValue( true ) ); + + PopupsContextTestWrapper::injectTestInstance( $contextMock ); + $prefs = []; + + PopupsHooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs ); + $this->assertArrayHasKey( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, $prefs ); + $this->assertArrayHasKey( 'disabled', + $prefs[ PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME ] ); + $this->assertEquals( true, + $prefs[ PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME]['disabled'] ); + $this->assertNotEmpty( $prefs[ PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME]['help'] ); + } + /** * @covers ::onGetPreferences */ @@ -95,7 +126,7 @@ class PopupsHooksTest extends MediaWikiTestCase { PopupsHooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs ); $this->assertCount( 4, $prefs ); $this->assertEquals( 'notEmpty', $prefs[ 'someNotEmptyValue'] ); - $this->assertArrayHasKey( \Popups\PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, $prefs ); + $this->assertArrayHasKey( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, $prefs ); $this->assertEquals( 1, array_search( \Popups\PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, array_keys( $prefs ) ), ' PagePreviews preferences should be injected after Skin select' ); } @@ -118,8 +149,8 @@ class PopupsHooksTest extends MediaWikiTestCase { PopupsHooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs ); $this->assertCount( 3, $prefs ); - $this->assertArrayHasKey( \Popups\PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, $prefs ); - $this->assertEquals( 2, array_search( \Popups\PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, + $this->assertArrayHasKey( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, $prefs ); + $this->assertEquals( 2, array_search( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME, array_keys( $prefs ) ), ' PagePreviews should be injected at end of array' ); }