Clean up preferences code

- Reference preview preferences should be defined inside Cite
- Don't use constants for seldom used strings to make the code
more readable.
- The lightweght ext.popups module is now always sent to the user
This was a micro-optimization and not necessary to do

Bug: T365538
Depends-On: Ic479c0a381ee16d1abcecfdd5ee48f0afccc1d3f
Change-Id: Ie8fa1672b9fdbb1c6b840dde5c9060a20a625adb

extension.json

@@ -22,7 +22,6 @@
 		"BeforePageDisplay": "PopupsHooks",
 		"ResourceLoaderGetConfigVars": "PopupsHooks",
 		"GetPreferences": "PopupsHooks",
-		"UserGetDefaultOptions": "PopupsHooks",
 		"MakeGlobalVariablesScript": "PopupsHooks"
 	},
 	"HookHandlers": {
@@ -58,10 +57,6 @@
 			"description": "@var bool: Whether the option to enable/disable Page Previews should be hidden on Preferences page. False by default",
 			"value": false
 		},
-		"PopupsOptInDefaultState": {
-			"description": "@var string:['1'|'0'] Default Page Previews visibility for old accounts. Has to be a string as a compatibility with beta feature settings. For more info see @T191888",
-			"value": "1"
-		},
 		"PopupsConflictingNavPopupsGadgetName": {
 			"description": "@var string: Navigation popups gadget name",
 			"value": "Navigation_popups"
@@ -179,9 +174,7 @@
 		"includes/ServiceWiring.php"
 	],
 	"DefaultUserOptions": {
-		"popups": "1",
-		"popupsreferencepreviews": "0",
-		"popups-reference-previews": "0"
+		"popups": "1"
 	},
 	"ConditionalUserOptions": {
 		"popups": [

i18n/en.json

@@ -22,9 +22,5 @@
 	"prefs-reading": "Reading preferences",
 	"popups-prefs-optin": "Enable page previews (get quick previews of a topic while reading a page)",
 	"popups-prefs-disable-nav-gadgets-info": "You have to [[$1|disable the Navigation popups gadget]] in your Gadgets preferences to enable page previews.",
-	"popups-prefs-conflicting-gadgets-info": "Certain gadgets and other customizations may affect the performance of this feature. If you experience problems please review your gadgets and user scripts, including global ones.",
-	"popups-prefs-navpopups-gadget-conflict-info": "You have the [[$1|Navigation popups]] gadget enabled, so you won't see previews provided by this feature. Depending on your wiki, the gadget may have a slightly different name. If you continue to experience issues, please review your gadgets and user scripts, including global ones.",
-	"popups-prefs-reftooltips-gadget-conflict-info": "You have the [[$1|Reference Tooltips]] gadget enabled, so you won't see reference previews but will still see page previews. Depending on your wiki, the gadget may have a slightly different name. If you continue to experience issues, please review your gadgets and user scripts, including global ones.",
-	"popups-prefs-reftooltips-and-navpopups-gadget-conflict-info": "You have the [[$1|Navigation popups]] and [[$1|Reference Tooltips]] gadgets enabled, so you won't see previews provided by this feature. Depending on your wiki, the gadgets may have slightly different names. If you continue to experience issues, please review your gadgets and user scripts, including global ones.",
-	"popups-refpreview-user-preference-label": "Enable reference previews (get quick previews of a reference while reading a page)"
+	"popups-prefs-conflicting-gadgets-info": "Certain gadgets and other customizations may affect the performance of this feature. If you experience problems please review your gadgets and user scripts, including global ones."
 }

i18n/qqq.json

@@ -38,9 +38,5 @@
 	"prefs-reading": "Title for 'Reading preferences' section on preferences page; it's a magic message derived from PREVIEWS_PREFERENCES_SECTION (see commit f597e34)",
 	"popups-prefs-optin": "Label for Page Previews option (description for Page Previews option)",
 	"popups-prefs-disable-nav-gadgets-info": "Help message telling to disable the \"Navigation popups\" gadget in order to enable page previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences",
-	"popups-prefs-conflicting-gadgets-info": "Help message informing about possible conflicts with other gadgets/customizations",
-	"popups-prefs-navpopups-gadget-conflict-info": "Help message telling to disable the \"Navigation popups\" gadget in order to allow page and reference previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences",
-	"popups-prefs-reftooltips-gadget-conflict-info": "Help message telling to disable the \"Reference Tooltips\" gadget in order to allow reference previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences",
-	"popups-prefs-reftooltips-and-navpopups-gadget-conflict-info": "Help message telling to disable the \"Navigation popups\" and \"Reference Tooltips\" gadgets in order to allow page and reference previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences",
-	"popups-refpreview-user-preference-label": "Label for the user option to enable or disable reference preview popups"
+	"popups-prefs-conflicting-gadgets-info": "Help message informing about possible conflicts with other gadgets/customizations"
 }

includes/PopupsContext.php

@@ -51,23 +51,10 @@ class PopupsContext {
 	 */
 	public const PREVIEWS_DISABLED = false;
 
-	/**
-	 * User preference key to enable/disable Page Previews
-	 */
-	public const PREVIEWS_OPTIN_PREFERENCE_NAME = 'popups';
-
-	/**
-	 * User preference key to enable/disable Reference Previews. Named
-	 * "mwe-popups-referencePreviews-enabled" in localStorage for anonymous users.
-	 */
-	public const REFERENCE_PREVIEWS_PREFERENCE_NAME = 'popups-reference-previews';
-
 	/**
 	 * Flags passed on to JS representing preferences
 	 */
 	private const NAV_POPUPS_ENABLED = 1;
-	private const REF_TOOLTIPS_ENABLED = 2;
-	private const REFERENCE_PREVIEWS_ENABLED = 4;
 
 	/**
 	 * @var Config
@@ -145,42 +132,12 @@ class PopupsContext {
 		return $this->config->get( 'PopupsHideOptInOnPreferencesPage' ) === false;
 	}
 
-	/**
-	 * @param User $user User whose preferences are checked
-	 * @return bool whether or not to show reference previews
-	 */
-	public function isReferencePreviewsEnabled( User $user ) {
-		return !$user->isNamed() || $this->userOptionsLookup->getBoolOption(
-			$user, self::REFERENCE_PREVIEWS_PREFERENCE_NAME
-		);
-	}
-
 	/**
 	 * @param User $user User whose preferences are checked
 	 * @return int
 	 */
 	public function getConfigBitmaskFromUser( User $user ) {
-		return ( $this->conflictsWithNavPopupsGadget( $user ) ? self::NAV_POPUPS_ENABLED : 0 ) |
-			( $this->conflictsWithRefTooltipsGadget( $user ) ? self::REF_TOOLTIPS_ENABLED : 0 ) |
-			( $this->isReferencePreviewsEnabled( $user ) ? self::REFERENCE_PREVIEWS_ENABLED : 0 );
-	}
-
-	/**
-	 * @param User $user User whose preferences are checked
-	 * @return bool
-	 */
-	public function shouldSendModuleToUser( User $user ) {
-		if ( !$user->isNamed() ) {
-			return true;
-		}
-
-		$shouldLoadPagePreviews = $this->userOptionsLookup->getBoolOption(
-			$user,
-			self::PREVIEWS_OPTIN_PREFERENCE_NAME
-		);
-		$shouldLoadReferencePreviews = $this->isReferencePreviewsEnabled( $user );
-
-		return $shouldLoadPagePreviews || $shouldLoadReferencePreviews;
+		return ( $this->conflictsWithNavPopupsGadget( $user ) ? self::NAV_POPUPS_ENABLED : 0 );
 	}
 
 	/**

includes/PopupsHooks.php

@@ -28,7 +28,6 @@ use MediaWiki\Output\Hook\MakeGlobalVariablesScriptHook;
 use MediaWiki\Output\OutputPage;
 use MediaWiki\Preferences\Hook\GetPreferencesHook;
 use MediaWiki\ResourceLoader\Hook\ResourceLoaderGetConfigVarsHook;
-use MediaWiki\User\Hook\UserGetDefaultOptionsHook;
 use MediaWiki\User\Options\UserOptionsManager;
 use MediaWiki\User\User;
 use Psr\Log\LoggerInterface;
@@ -43,7 +42,6 @@ class PopupsHooks implements
 	GetPreferencesHook,
 	BeforePageDisplayHook,
 	ResourceLoaderGetConfigVarsHook,
-	UserGetDefaultOptionsHook,
 	MakeGlobalVariablesScriptHook
 {
 
@@ -101,10 +99,7 @@ class PopupsHooks implements
 		}
 
 		$skinPosition = array_search( 'skin', array_keys( $prefs ) );
-		$readingOptions = array_merge(
-			$this->getPagePreviewPrefToggle( $user ),
-			$this->getReferencePreviewPrefToggle( $user )
-		);
+		$readingOptions = $this->getPagePreviewPrefToggle( $user );
 
 		if ( $skinPosition !== false ) {
 			$injectIntoIndex = $skinPosition + 1;
@@ -137,43 +132,7 @@ class PopupsHooks implements
 		}
 
 		return [
-			PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME => $option
-		];
-	}
-
-	/**
-	 * Get Reference Preview option
-	 *
-	 * @param User $user User whose preferences are being modified
-	 * @return array[]
-	 */
-	private function getReferencePreviewPrefToggle( User $user ) {
-		$option = [
-			'type' => 'toggle',
-			'label-message' => 'popups-refpreview-user-preference-label',
-			'help-message' => 'popups-prefs-conflicting-gadgets-info',
-			'section' => self::PREVIEWS_PREFERENCES_SECTION
-		];
-
-		$isNavPopupsGadgetEnabled = $this->popupsContext->conflictsWithNavPopupsGadget( $user );
-		$isRefTooltipsGadgetEnabled = $this->popupsContext->conflictsWithRefTooltipsGadget( $user );
-
-		if ( $isNavPopupsGadgetEnabled && $isRefTooltipsGadgetEnabled ) {
-			$option[ 'disabled' ] = true;
-			$option[ 'help-message' ] = [ 'popups-prefs-reftooltips-and-navpopups-gadget-conflict-info',
-				'Special:Preferences#mw-prefsection-gadgets' ];
-		} elseif ( $isNavPopupsGadgetEnabled ) {
-			$option[ 'disabled' ] = true;
-			$option[ 'help-message' ] = [ 'popups-prefs-navpopups-gadget-conflict-info',
-				'Special:Preferences#mw-prefsection-gadgets' ];
-		} elseif ( $isRefTooltipsGadgetEnabled ) {
-			$option[ 'disabled' ] = true;
-			$option[ 'help-message' ] = [ 'popups-prefs-reftooltips-gadget-conflict-info',
-				'Special:Preferences#mw-prefsection-gadgets' ];
-		}
-
-		return [
-			PopupsContext::REFERENCE_PREVIEWS_PREFERENCE_NAME => $option
+			'popups' => $option
 		];
 	}
 
@@ -194,10 +153,7 @@ class PopupsHooks implements
 			return;
 		}
 
-		$user = $out->getUser();
-		if ( $this->popupsContext->shouldSendModuleToUser( $user ) ) {
-			$out->addModules( [ 'ext.popups' ] );
-		}
+		$out->addModules( [ 'ext.popups' ] );
 	}
 
 	/**
@@ -225,8 +181,6 @@ class PopupsHooks implements
 	 * the users settings. These variables end in an inline <script> in the documents head.
 	 *
 	 * Variables added:
-	 * * `wgPopupsReferencePreviews' - The server's notion of whether or not the reference
-	 *   previews should be enabled. Depending on the general setting done on the wiki.
 	 * * `wgPopupsConflictsWithNavPopupGadget' - The server's notion of whether or not the
 	 *   user has enabled conflicting Navigational Popups Gadget.
 	 * * `wgPopupsConflictsWithRefTooltipsGadget' - The server's notion of whether or not the
@@ -238,15 +192,4 @@ class PopupsHooks implements
 	public function onMakeGlobalVariablesScript( &$vars, $out ): void {
 		$vars['wgPopupsFlags'] = $this->popupsContext->getConfigBitmaskFromUser( $out->getUser() );
 	}
-
-	/**
-	 * Called whenever a user wants to reset their preferences.
-	 *
-	 * @param array &$defaultOptions
-	 */
-	public function onUserGetDefaultOptions( &$defaultOptions ) {
-		$default = $this->config->get( 'PopupsOptInDefaultState' );
-		$defaultOptions[PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME] = $default;
-		$defaultOptions[PopupsContext::REFERENCE_PREVIEWS_PREFERENCE_NAME] = '1';
-	}
 }

tests/phpunit/PopupsContextTest.php

@@ -94,20 +94,6 @@ class PopupsContextTest extends MediaWikiIntegrationTestCase {
 		];
 	}
 
-	/**
-	 * @covers ::shouldSendModuleToUser
-	 */
-	public function testShouldSendToAnonUser() {
-		$user = $this->createMock( User::class );
-		$user->method( 'getId' )->willReturn( self::ANONYMOUS_USER );
-
-		$context = $this->getContext();
-		$this->assertTrue(
-			$context->shouldSendModuleToUser( $user ),
-			'The module is always sent to anonymous users.'
-		);
-	}
-
 	/**
 	 * @covers ::areDependenciesMet
 	 * @covers ::__construct
@@ -235,30 +221,20 @@ class PopupsContextTest extends MediaWikiIntegrationTestCase {
 	 * @covers ::getConfigBitmaskFromUser
 	 * @dataProvider provideTestGetConfigBitmaskFromUser
 	 * @param bool $navPops
-	 * @param bool $refTooltips
-	 * @param bool $refEnabled
 	 * @param int $expected
 	 */
 	public function testGetConfigBitmaskFromUser(
 		$navPops,
-		$refTooltips,
-		$refEnabled,
 		$expected
 	) {
 		$contextMock = $this->createPartialMock(
 			PopupsContext::class,
 			[
-				'conflictsWithNavPopupsGadget',
-				'conflictsWithRefTooltipsGadget',
-				'isReferencePreviewsEnabled',
+				'conflictsWithNavPopupsGadget'
 			]
 		);
 		$contextMock->method( 'conflictsWithNavPopupsGadget' )
 			->willReturn( $navPops );
-		$contextMock->method( 'conflictsWithRefTooltipsGadget' )
-			->willReturn( $refTooltips );
-		$contextMock->method( 'isReferencePreviewsEnabled' )
-			->willReturn( $refEnabled );
 
 		$this->assertSame(
 			$expected,
@@ -270,25 +246,9 @@ class PopupsContextTest extends MediaWikiIntegrationTestCase {
 		return [
 			[
 				true,
-				true,
-				true,
-				7,
+				1,
 			],
 			[
-				false,
-				true,
-				false,
-				2,
-			],
-			[
-				true,
-				false,
-				true,
-				5,
-			],
-			[
-				false,
-				false,
 				false,
 				0,
 			],

tests/phpunit/PopupsHooksTest.php

@@ -69,7 +69,7 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 		$contextMock->expects( $this->once() )
 			->method( 'showPreviewsOptInOnPreferencesPage' )
 			->willReturn( true );
-		$contextMock->expects( $this->exactly( 2 ) )
+		$contextMock->expects( $this->once() )
 			->method( 'conflictsWithNavPopupsGadget' )
 			->with( $userMock )
 			->willReturn( true );
@@ -83,16 +83,16 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 			$this->getServiceContainer()->getUserOptionsManager()
 		) )
 			->onGetPreferences( $userMock, $prefs );
-		$this->assertArrayHasKey( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME,
+		$this->assertArrayHasKey( 'popups',
 			$prefs,
 			'The opt-in preference is retrieved.' );
 		$this->assertArrayHasKey( 'disabled',
-			$prefs[ PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME ],
+			$prefs[ 'popups' ],
 			'The opt-in preference has a status.' );
 		$this->assertTrue(
-			$prefs[ PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME]['disabled'],
+			$prefs[ 'popups']['disabled'],
 			'The opt-in preference\'s status is disabled.' );
-		$this->assertNotEmpty( $prefs[ PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME]['help-message'],
+		$this->assertNotEmpty( $prefs[ 'popups']['help-message'],
 			'The opt-in preference has a help message.' );
 	}
 
@@ -104,7 +104,7 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 		$contextMock->expects( $this->once() )
 			->method( 'showPreviewsOptInOnPreferencesPage' )
 			->willReturn( true );
-		$contextMock->expects( $this->exactly( 2 ) )
+		$contextMock->expects( $this->once() )
 			->method( 'conflictsWithNavPopupsGadget' )
 			->willReturn( false );
 
@@ -125,11 +125,11 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 		$this->assertSame( 'notEmpty',
 			$prefs[ 'someNotEmptyValue'],
 			'Unretrieved preferences are unchanged.' );
-		$this->assertArrayHasKey( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME,
+		$this->assertArrayHasKey( 'popups',
 			$prefs,
 			'The opt-in preference is retrieved.' );
 		$this->assertSame( 1,
-			array_search( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME,
+			array_search( 'popups',
 				array_keys( $prefs ) ),
 			'The opt-in preference is injected after Skin select.' );
 	}
@@ -142,7 +142,7 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 		$contextMock->expects( $this->once() )
 			->method( 'showPreviewsOptInOnPreferencesPage' )
 			->willReturn( true );
-		$contextMock->expects( $this->exactly( 2 ) )
+		$contextMock->expects( $this->once() )
 			->method( 'conflictsWithNavPopupsGadget' )
 			->willReturn( false );
 
@@ -159,11 +159,11 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 		) )
 			->onGetPreferences( $this->createMock( User::class ), $prefs );
 		$this->assertGreaterThan( 2, count( $prefs ), 'A preference is retrieved.' );
-		$this->assertArrayHasKey( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME,
+		$this->assertArrayHasKey( 'popups',
 			$prefs,
 			'The opt-in preference is retrieved.' );
 		$this->assertSame( 2,
-			array_search( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME,
+			array_search( 'popups',
 				array_keys( $prefs ) ),
 			'The opt-in preference is appended.' );
 	}
@@ -229,12 +229,12 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 
 	public static function providerOnBeforePageDisplay() {
 		return [
-			[ false, false, false ],
-			[ true, true, false ],
+			[ true, false ],
+			[ true, false ],
 			// Code not sent if title is excluded
-			[ true, false, true ],
+			[ false, true ],
 			// Code not sent if title is excluded
-			[ false, false, true ]
+			[ false, true ]
 		];
 	}
 
@@ -242,7 +242,7 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 	 * @covers ::onBeforePageDisplay
 	 * @dataProvider providerOnBeforePageDisplay
 	 */
-	public function testOnBeforePageDisplay( $shouldSendModuleToUser,
+	public function testOnBeforePageDisplay(
 			$isCodeLoaded, $isTitleExcluded ) {
 		$skinMock = $this->createMock( Skin::class );
 
@@ -262,9 +262,6 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 				->willReturn( true );
 		}
 
-		$contextMock->method( 'shouldSendModuleToUser' )
-			->willReturn( $shouldSendModuleToUser );
-
 		$contextMock->expects( $this->once() )
 			->method( 'isTitleExcluded' )
 			->willReturn( $isTitleExcluded );
@@ -307,26 +304,4 @@ class PopupsHooksTest extends MediaWikiIntegrationTestCase {
 			'The wgPopupsFlags global is present and 0.' );
 	}
 
-	/**
-	 * @covers ::onUserGetDefaultOptions
-	 */
-	public function testOnUserGetDefaultOptions() {
-		$userOptions = [
-			'test' => 'not_empty'
-		];
-
-		( new PopupsHooks(
-			new HashConfig( [
-				'PopupsOptInDefaultState' => '1',
-			] ),
-			$this->getServiceContainer()->getService( 'Popups.Context' ),
-			$this->getServiceContainer()->getService( 'Popups.Logger' ),
-			$this->getServiceContainer()->getUserOptionsManager()
-		) )
-			->onUserGetDefaultOptions( $userOptions );
-		$this->assertCount( 3, $userOptions );
-		$this->assertSame( '1', $userOptions[ PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME ] );
-		$this->assertSame( '1', $userOptions[ PopupsContext::REFERENCE_PREVIEWS_PREFERENCE_NAME ] );
-	}
-
 }