From 411f9ce69a26258bdb50f45386a53da9f3140634 Mon Sep 17 00:00:00 2001 From: Jon Robson Date: Mon, 2 Oct 2023 11:15:18 -0700 Subject: [PATCH] Allow user preferences to be overriden by query string parameter Currently only features that are associated with LocalSettings configuration can be overriden by query string. Going forward we'd like to expand this to apply to user preference too. A generic OverrideableRequirement is created. The existing OverridableConfigRequirement is refactored to extend OverrideableRequirement The /UserPreferenceRequirement now extends it This allows http://localhost:8888/wiki/Spain?vectortypographysurvey=1 to work Bug: T347900 Change-Id: I11efd6b07192d5d2333f4506e9d87a8c0638d657 --- .../OverridableConfigRequirement.php | 21 ++--- .../OverrideableRequirementHelper.php | 84 +++++++++++++++++++ .../UserPreferenceRequirement.php | 12 ++- includes/ServiceWiring.php | 21 +++-- .../OverridableConfigRequirementTest.php | 3 +- .../UserPreferenceRequirementTest.php | 3 + 6 files changed, 119 insertions(+), 25 deletions(-) create mode 100644 includes/FeatureManagement/Requirements/OverrideableRequirementHelper.php diff --git a/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php b/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php index 3fe3a3b36..4de2049bf 100644 --- a/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php +++ b/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php @@ -75,11 +75,6 @@ final class OverridableConfigRequirement implements Requirement { */ private $user; - /** - * @var WebRequest - */ - private $request; - /** * @var string */ @@ -91,9 +86,9 @@ final class OverridableConfigRequirement implements Requirement { private $requirementName; /** - * @var string + * @var OverrideableRequirementHelper */ - private $overrideName; + private $helper; /** * This constructor accepts all dependencies needed to determine whether @@ -114,10 +109,9 @@ final class OverridableConfigRequirement implements Requirement { ) { $this->config = $config; $this->user = $user; - $this->request = $request; $this->configName = $configName; $this->requirementName = $requirementName; - $this->overrideName = strtolower( $configName ); + $this->helper = new OverrideableRequirementHelper( $request, $requirementName ); } /** @@ -135,12 +129,9 @@ final class OverridableConfigRequirement implements Requirement { * @inheritDoc */ public function isMet(): bool { - // Check query parameter. - if ( $this->request->getCheck( $this->overrideName ) ) { - return $this->request->getBool( $this->overrideName ); - } - if ( $this->request->getCheck( $this->configName ) ) { - return $this->request->getBool( $this->configName ); + $isMet = $this->helper->isMet(); + if ( $isMet !== null ) { + return $isMet; } // If AB test is not enabled, fallback to checking config state. diff --git a/includes/FeatureManagement/Requirements/OverrideableRequirementHelper.php b/includes/FeatureManagement/Requirements/OverrideableRequirementHelper.php new file mode 100644 index 000000000..a074bcf0a --- /dev/null +++ b/includes/FeatureManagement/Requirements/OverrideableRequirementHelper.php @@ -0,0 +1,84 @@ +request = $request; + $this->overrideName = 'vector' . strtolower( $requirementName ); + $this->requirementName = $requirementName; + } + + /** + * Check query parameter to override config or not. + * Then check for AB test value. + * Fallback to config value. + * + * @return bool|null + */ + public function isMet() { + // Check query parameter. + if ( $this->request->getCheck( $this->overrideName ) ) { + return $this->request->getBool( $this->overrideName ); + } + $vectorReq = 'Vector' . $this->requirementName; + if ( $this->request->getCheck( $vectorReq ) ) { + return $this->request->getBool( $vectorReq ); + } + return null; + } +} diff --git a/includes/FeatureManagement/Requirements/UserPreferenceRequirement.php b/includes/FeatureManagement/Requirements/UserPreferenceRequirement.php index 99a618cfa..7fcc083a0 100644 --- a/includes/FeatureManagement/Requirements/UserPreferenceRequirement.php +++ b/includes/FeatureManagement/Requirements/UserPreferenceRequirement.php @@ -25,6 +25,7 @@ use MediaWiki\Skins\Vector\FeatureManagement\Requirement; use MediaWiki\Title\Title; use MediaWiki\User\UserOptionsLookup; use User; +use WebRequest; /** * @package MediaWiki\Skins\Vector\FeatureManagement\Requirements @@ -56,6 +57,11 @@ final class UserPreferenceRequirement implements Requirement { */ private $title; + /** + * @var OverrideableRequirementHelper + */ + private $helper; + /** * This constructor accepts all dependencies needed to determine whether * the overridable config is enabled for the current user and request. @@ -64,6 +70,7 @@ final class UserPreferenceRequirement implements Requirement { * @param UserOptionsLookup $userOptionsLookup * @param string $optionName The name of the user preference. * @param string $requirementName The name of the requirement presented to FeatureManager. + * @param WebRequest $request * @param Title|null $title */ public function __construct( @@ -71,6 +78,7 @@ final class UserPreferenceRequirement implements Requirement { UserOptionsLookup $userOptionsLookup, string $optionName, string $requirementName, + WebRequest $request, $title = null ) { $this->user = $user; @@ -78,6 +86,7 @@ final class UserPreferenceRequirement implements Requirement { $this->optionName = $optionName; $this->requirementName = $requirementName; $this->title = $title; + $this->helper = new OverrideableRequirementHelper( $request, $requirementName ); } /** @@ -113,6 +122,7 @@ final class UserPreferenceRequirement implements Requirement { * @inheritDoc */ public function isMet(): bool { - return $this->isPreferenceEnabled(); + $override = $this->helper->isMet(); + return $override ?? $this->isPreferenceEnabled(); } } diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 57cb85796..b25ac94e2 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -50,6 +50,7 @@ return [ ); $context = RequestContext::getMain(); + $request = $context->getRequest(); // Feature: Languages in sidebar // ================================ @@ -57,7 +58,7 @@ return [ new OverridableConfigRequirement( $services->getMainConfig(), $context->getUser(), - $context->getRequest(), + $request, Constants::CONFIG_KEY_LANGUAGE_IN_HEADER, Constants::REQUIREMENT_LANGUAGE_IN_HEADER ) @@ -89,7 +90,7 @@ return [ new OverridableConfigRequirement( $config, $context->getUser(), - $context->getRequest(), + $request, Constants::CONFIG_KEY_LANGUAGE_IN_HEADER, $requirementName ) @@ -111,7 +112,7 @@ return [ new OverridableConfigRequirement( $services->getMainConfig(), $context->getUser(), - $context->getRequest(), + $request, Constants::CONFIG_LANGUAGE_IN_MAIN_PAGE_HEADER, Constants::REQUIREMENT_LANGUAGE_IN_MAIN_PAGE_HEADER ) @@ -138,7 +139,7 @@ return [ new OverridableConfigRequirement( $services->getMainConfig(), $context->getUser(), - $context->getRequest(), + $request, Constants::CONFIG_STICKY_HEADER, Constants::REQUIREMENT_STICKY_HEADER ) @@ -167,6 +168,7 @@ return [ $services->getUserOptionsLookup(), Constants::PREF_KEY_PAGE_TOOLS_PINNED, Constants::REQUIREMENT_PAGE_TOOLS_PINNED, + $request, $context->getTitle() ) ); @@ -188,6 +190,7 @@ return [ $services->getUserOptionsLookup(), Constants::PREF_KEY_TOC_PINNED, Constants::REQUIREMENT_TOC_PINNED, + $request, $context->getTitle() ) ); @@ -208,6 +211,7 @@ return [ $services->getUserOptionsLookup(), Constants::PREF_KEY_MAIN_MENU_PINNED, Constants::REQUIREMENT_MAIN_MENU_PINNED, + $request, $context->getTitle() ) ); @@ -229,6 +233,7 @@ return [ $services->getUserOptionsLookup(), Constants::PREF_KEY_LIMITED_WIDTH, Constants::REQUIREMENT_LIMITED_WIDTH, + $request, $context->getTitle() ) ); @@ -245,7 +250,7 @@ return [ $featureManager->registerRequirement( new LimitedWidthContentRequirement( $services->getMainConfig(), - $context->getRequest(), + $request, $context->getTitle() ) ); @@ -263,7 +268,7 @@ return [ new OverridableConfigRequirement( $services->getMainConfig(), $context->getUser(), - $context->getRequest(), + $request, Constants::CONFIG_ZEBRA_DESIGN, Constants::REQUIREMENT_ZEBRA_DESIGN ) @@ -285,6 +290,7 @@ return [ $services->getUserOptionsLookup(), Constants::PREF_KEY_FONT_SIZE, Constants::REQUIREMENT_FONT_SIZE, + $request, $context->getTitle() ) ); @@ -304,7 +310,7 @@ return [ new OverridableConfigRequirement( $services->getMainConfig(), $context->getUser(), - $context->getRequest(), + $request, Constants::CONFIG_KEY_CLIENT_PREFERENCES, Constants::REQUIREMENT_CLIENT_PREFERENCES ) @@ -326,6 +332,7 @@ return [ $services->getUserOptionsLookup(), Constants::PREF_KEY_TYPOGRAPHY_SURVEY, Constants::REQUIREMENT_TYPOGRAPHY_SURVEY, + $request, $context->getTitle() ) ); diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php index 43e2f4f78..3e8e90b60 100644 --- a/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php +++ b/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php @@ -133,8 +133,7 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { $user, $request, 'VectorLanguageInHeader', - 'LanguageInHeader', - 'languageinheader' + 'LanguageInHeader' ); $this->assertSame( $expected, $requirement->isMet(), $msg ); diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/UserPreferenceRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/UserPreferenceRequirementTest.php index 3d08faad8..2bb2a62f5 100644 --- a/tests/phpunit/unit/FeatureManagement/Requirements/UserPreferenceRequirementTest.php +++ b/tests/phpunit/unit/FeatureManagement/Requirements/UserPreferenceRequirementTest.php @@ -2,6 +2,7 @@ namespace MediaWiki\Skins\Vector\Tests\Unit\FeatureManagement\Requirements; +use FauxRequest; use MediaWiki\Skins\Vector\FeatureManagement\Requirements\UserPreferenceRequirement; use MediaWiki\Title\Title; use MediaWiki\User\UserOptionsLookup; @@ -85,6 +86,7 @@ final class UserPreferenceRequirementTest extends \MediaWikiUnitTestCase { ) { $user = $this->createMock( User::class ); $title = $isTitlePresent ? $this->createMock( Title::class ) : null; + $request = new FauxRequest(); $userOptionsLookup = $this->createMock( UserOptionsLookup::class ); $userOptionsLookup->method( 'getOption' )->willReturn( $isEnabled ); @@ -94,6 +96,7 @@ final class UserPreferenceRequirementTest extends \MediaWikiUnitTestCase { $userOptionsLookup, 'userOption', 'userRequirement', + $request, $title );