diff --git a/includes/Constants.php b/includes/Constants.php index 904b4e7d..0a6cf039 100644 --- a/includes/Constants.php +++ b/includes/Constants.php @@ -166,6 +166,11 @@ final class Constants { */ public const REQUIREMENT_USE_WVUI_SEARCH = 'VectorUseWvuiSearch'; + /** + * @var string + */ + public const QUERY_PARAM_CONSOLIDATE_USER_LINKS = 'vectoruserlinks'; + /** * @var string */ diff --git a/includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php b/includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php deleted file mode 100644 index 128620e0..00000000 --- a/includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php +++ /dev/null @@ -1,133 +0,0 @@ -config = $config; - $this->user = $user; - $this->request = $request; - $this->centralIdLookup = $centralIdLookup; - } - - /** - * @inheritDoc - */ - public function getName() : string { - return Constants::REQUIREMENT_LANGUAGE_IN_HEADER; - } - - /** - * If A/B test is enabled check whether the user is logged in and bucketed. - * Fallback to `VectorLanguageInHeader` config value. - * - * @inheritDoc - * @throws \ConfigException - */ - public function isMet() : bool { - if ( $this->request->getCheck( Constants::QUERY_PARAM_LANGUAGE_IN_HEADER ) ) { - return $this->request->getBool( Constants::QUERY_PARAM_LANGUAGE_IN_HEADER ); - } - - if ( - (bool)$this->config->get( Constants::CONFIG_LANGUAGE_IN_HEADER_TREATMENT_AB_TEST ) && - $this->user->isRegistered() - ) { - $id = null; - if ( $this->centralIdLookup ) { - $id = $this->centralIdLookup->centralIdFromLocalUser( $this->user ); - } - - // $id will be 0 if the central ID lookup failed. - if ( !$id ) { - $id = $this->user->getId(); - } - - return $id % 2 === 0; - } - - // If AB test is not enabled, fallback to checking config state. - $languageInHeaderConfig = $this->config->get( Constants::CONFIG_KEY_LANGUAGE_IN_HEADER ); - - // Backwards compatibility with config variables that have been set in production. - if ( is_bool( $languageInHeaderConfig ) ) { - $languageInHeaderConfig = [ - 'logged_in' => $languageInHeaderConfig, - 'logged_out' => $languageInHeaderConfig, - ]; - } else { - $languageInHeaderConfig = [ - 'logged_in' => $languageInHeaderConfig['logged_in'] ?? false, - 'logged_out' => $languageInHeaderConfig['logged_out'] ?? false, - ]; - } - - return $languageInHeaderConfig[ $this->user->isRegistered() ? 'logged_in' : 'logged_out' ]; - } -} diff --git a/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php b/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php new file mode 100644 index 00000000..892a5626 --- /dev/null +++ b/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php @@ -0,0 +1,201 @@ +registerRequirement( + * new OverridableConfigRequirement( + * $config, + * $user, + * $request, + * $centralIdLookup, + * 'configName', + * 'requirementName', + * 'overrideName', + * 'configTestName', + * ) + * ); + * ``` + * + * registers a requirement that will evaluate to true only when `mediawiki/includes/Setup.php` has + * finished executing (after all service wiring has executed). I.e., every call to + * `Requirement->isMet()` re-interrogates the request, user authentication status, + * and config object for the current state and returns it. Contrast to: + * + * ```lang=php + * $featureManager->registerRequirement( + * 'Foo', + * $config->get( 'Sitename' ) + * ); + * ``` + * + * wherein state is evaluated only once at registration time and permanently cached. + * + * NOTE: This API hasn't settled. It may change at any time without warning. Please don't bind to + * it unless you absolutely need to + * + * @package Vector\FeatureManagement\Requirements + */ +final class OverridableConfigRequirement implements Requirement { + + /** + * @var Config + */ + private $config; + + /** + * @var User + */ + private $user; + + /** + * @var WebRequest + */ + private $request; + + /** + * @var CentralIdLookup|null + */ + private $centralIdLookup; + + /** + * @var string + */ + private $configName; + + /** + * @var string + */ + private $requirementName; + + /** + * @var string + */ + private $overrideName; + + /** + * @var string|null + */ + private $configTestName; + + /** + * This constructor accepts all dependencies needed to determine whether + * the overridable config is enabled for the current user and request. + * + * @param Config $config + * @param User $user + * @param WebRequest $request + * @param CentralIdLookup|null $centralIdLookup + * @param string $configName Any `Config` key. This name is used to query `$config` state. + * @param string $requirementName The name of the requirement presented to FeatureManager. + * @param string $overrideName The name of the override presented to FeatureManager, i.e. query parameter. + * @param string|null $configTestName The name of the AB test config presented to FeatureManager if applicable. + */ + public function __construct( + Config $config, + User $user, + WebRequest $request, + ?CentralIdLookup $centralIdLookup, + string $configName, + string $requirementName, + string $overrideName, + ?string $configTestName + ) { + $this->config = $config; + $this->user = $user; + $this->request = $request; + $this->centralIdLookup = $centralIdLookup; + $this->configName = $configName; + $this->requirementName = $requirementName; + $this->overrideName = $overrideName; + $this->configTestName = $configTestName; + } + + /** + * @inheritDoc + */ + public function getName() : string { + return $this->requirementName; + } + + /** + * Check query parameter to override config or not. + * Then check for AB test value. + * Fallback to config value. + * + * @inheritDoc + */ + public function isMet() : bool { + // Check query parameter. + if ( $this->request->getCheck( $this->overrideName ) ) { + return $this->request->getBool( $this->overrideName ); + } + + // Check if there's config for AB testing. + if ( + !empty( $this->configTestName ) && + (bool)$this->config->get( $this->configTestName ) && + $this->user->isRegistered() + ) { + $id = null; + if ( $this->centralIdLookup ) { + $id = $this->centralIdLookup->centralIdFromLocalUser( $this->user ); + } + + // $id will be 0 if the central ID lookup failed. + if ( !$id ) { + $id = $this->user->getId(); + } + + return $id % 2 === 0; + } + + // If AB test is not enabled, fallback to checking config state. + $thisConfig = $this->config->get( $this->configName ); + + // Backwards compatibility with config variables that have been set in production. + if ( is_bool( $thisConfig ) ) { + $thisConfig = [ + 'logged_in' => $thisConfig, + 'logged_out' => $thisConfig, + ]; + } else { + $thisConfig = [ + 'logged_in' => $thisConfig['logged_in'] ?? false, + 'logged_out' => $thisConfig['logged_out'] ?? false, + ]; + } + + // Fallback to config. + return $thisConfig[ $this->user->isRegistered() ? 'logged_in' : 'logged_out' ]; + } +} diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 96433d29..554239ae 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -26,8 +26,8 @@ use MediaWiki\MediaWikiServices; use Vector\Constants; use Vector\FeatureManagement\FeatureManager; use Vector\FeatureManagement\Requirements\DynamicConfigRequirement; -use Vector\FeatureManagement\Requirements\LanguageInHeaderTreatmentRequirement; use Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement; +use Vector\FeatureManagement\Requirements\OverridableConfigRequirement; use Vector\FeatureManagement\Requirements\WvuiSearchTreatmentRequirement; use Vector\SkinVersionLookup; @@ -71,11 +71,15 @@ return [ // Feature: Languages in sidebar // ================================ $featureManager->registerRequirement( - new LanguageInHeaderTreatmentRequirement( + new OverridableConfigRequirement( $services->getMainConfig(), $context->getUser(), $context->getRequest(), - CentralIdLookup::factoryNonLocal() + CentralIdLookup::factoryNonLocal(), + Constants::CONFIG_KEY_LANGUAGE_IN_HEADER, + Constants::REQUIREMENT_LANGUAGE_IN_HEADER, + Constants::QUERY_PARAM_LANGUAGE_IN_HEADER, + Constants::CONFIG_LANGUAGE_IN_HEADER_TREATMENT_AB_TEST ) ); @@ -108,15 +112,24 @@ return [ // Feature: Consolidate personal menu links // ================================ - $consolidateUserLinksConfig = $services->getMainConfig()->get( Constants::CONFIG_CONSOLIDATE_USER_LINKS ); - $featureManager->registerSimpleRequirement( - Constants::REQUIREMENT_CONSOLIDATE_USER_LINKS, - $consolidateUserLinksConfig[ $context->getUser()->isRegistered() ? 'logged_in' : 'logged_out' ] + $featureManager->registerRequirement( + new OverridableConfigRequirement( + $services->getMainConfig(), + $context->getUser(), + $context->getRequest(), + null, + Constants::CONFIG_CONSOLIDATE_USER_LINKS, + Constants::REQUIREMENT_CONSOLIDATE_USER_LINKS, + Constants::QUERY_PARAM_CONSOLIDATE_USER_LINKS, + null + ) ); $featureManager->registerFeature( Constants::FEATURE_CONSOLIDATE_USER_LINKS, [ + Constants::REQUIREMENT_FULLY_INITIALISED, + Constants::REQUIREMENT_LATEST_SKIN_VERSION, Constants::REQUIREMENT_CONSOLIDATE_USER_LINKS ] ); diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php similarity index 51% rename from tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php rename to tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php index df0a04c6..ecde7013 100644 --- a/tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php +++ b/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php @@ -24,15 +24,15 @@ use CentralIdLookup; use HashConfig; use User; use Vector\Constants; -use Vector\FeatureManagement\Requirements\LanguageInHeaderTreatmentRequirement; +use Vector\FeatureManagement\Requirements\OverridableConfigRequirement; use WebRequest; /** * @group Vector * @group FeatureManagement - * @coversDefaultClass \Vector\FeatureManagement\Requirements\LanguageInHeaderTreatmentRequirement + * @coversDefaultClass \Vector\FeatureManagement\Requirements\OverridableConfigRequirement */ -class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { +class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { public function providerLanguageInHeaderTreatmentRequirement() { return [ @@ -50,6 +50,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param null, + // AB test name + null, false, 'If nothing enabled, nobody gets new treatment' ], @@ -67,6 +69,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', true, 'Anon users should get new treatment if enabled when A/B test disabled' ], @@ -83,6 +87,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', true, 'Logged in users should get new treatment if enabled when A/B test disabled' ], @@ -99,6 +105,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', true, 'All odd logged in users should get new treatent when A/B test disabled' ], @@ -116,6 +124,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', true, // Ab test is only for logged in users 'Anon users with a/b test enabled should see new treatment when config enabled' @@ -128,12 +138,15 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { ], // is A-B test enabled true, + // // note 0 = anon user 0, // use central id lookup? false, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', false, // Ab test is only for logged in users 'Anon users with a/b test enabled should see old treatment when config disabled' @@ -151,6 +164,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', true, 'Even logged in users get new treatment when A/B test enabled' ], @@ -167,6 +182,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', false, 'Odd logged in users do not get new treatment when A/B test enabled' ], @@ -183,6 +200,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { true, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', true, 'With CentralIdLookup, even logged in users get new treatment when A/B test enabled' ], @@ -199,6 +218,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { true, // `languageinheader` query param null, + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', false, 'With CentralIdLookup, odd logged in users do not get new treatment when A/B test enabled' ], @@ -215,6 +236,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param "1", + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', true, 'Odd logged in users get new treatment when A/B test enabled and query param set to "1"' ], @@ -231,6 +254,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param "0", + // AB test name + 'VectorLanguageInHeaderTreatmentABTest', false, 'Even logged in users get old treatment when A/B test enabled and query param set to "0"' ], @@ -247,6 +272,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param "1", + // AB test name + null, true, 'Users get new treatment when query param set to "1" regardless of state of A/B test or config flags' ], @@ -263,6 +290,8 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { false, // `languageinheader` query param "0", + // AB test name + null, false, 'Users get old treatment when query param set to "0" regardless of state of A/B test or config flags' ], @@ -277,11 +306,19 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { * @param int $userId * @param bool $useCentralIdLookup * @param string|null $queryParam + * @param string|null $testName * @param bool $expected * @param string $msg */ public function testLanguageInHeaderTreatmentRequirement( - $configValue, $abValue, $userId, $useCentralIdLookup, $queryParam, $expected, $msg + $configValue, + $abValue, + $userId, + $useCentralIdLookup, + $queryParam, + $testName, + $expected, + $msg ) { $config = new HashConfig( [ Constants::CONFIG_KEY_LANGUAGE_IN_HEADER => $configValue, @@ -299,11 +336,223 @@ class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase { $centralIdLookup = $this->createMock( CentralIdLookup::class ); $centralIdLookup->method( 'centralIdFromLocalUser' )->willReturn( $userId ); - $requirement = new LanguageInHeaderTreatmentRequirement( + $requirement = new OverridableConfigRequirement( $config, $user, $request, - $useCentralIdLookup ? $centralIdLookup : null + $useCentralIdLookup ? $centralIdLookup : null, + 'VectorLanguageInHeader', + 'LanguageInHeader', + 'languageinheader', + $testName ?? null + ); + + $this->assertSame( $expected, $requirement->isMet(), $msg ); + } + + public function providerUserLinksTreatmentRequirement() { + return [ + [ + // Is consolidate user links enabled (backward compatibility with boolean values) + false, + // note 0 = anon user + 0, + // `vectoruserlinks` query param + null, + false, + 'If nothing enabled (old config), nobody gets new treatment' + ], + [ + // Is consolidate user links enabled + [ + 'logged_in' => false, + 'logged_out' => false, + ], + // note 0 = anon user + 0, + // `vectoruserlinks` query param + null, + false, + 'If nothing enabled (new config), nobody gets new treatment' + ], + [ + // Is consolidate user links enabled (backward compatibility with boolean values) + true, + // note 0 = anon user + 0, + // `vectoruserlinks` query param + null, + true, + 'Anon users should get new treatment if enabled (old config)' + ], + [ + // Is consolidate user links enabled + [ + 'logged_in' => true, + 'logged_out' => true, + ], + // note 0 = anon user + 0, + // `vectoruserlinks` query param + null, + true, + 'Anon users should get new treatment if enabled (new config)' + ], + [ + // Is consolidate user links enabled (backward compatibility with boolean values) + true, + // Logged in user + 2, + // `vectoruserlinks` query param + null, + true, + 'Logged in users should get new treatment if enabled (old config)' + ], + [ + // Is consolidate user links enabled + [ + 'logged_in' => true, + 'logged_out' => true, + ], + // Logged in user + 2, + // `vectoruserlinks` query param + null, + true, + 'Logged in users should get new treatment if enabled (new config)' + ], + [ + // Is consolidate user links enabled (backward compatibility with boolean values) + false, + // note 0 = anon user + 0, + // `vectoruserlinks` query param + "1", + true, + 'Anon users get new treatment when query param set to "1" regardless of config (old config)' + ], + [ + // Is consolidate user links enabled + [ + 'logged_in' => false, + 'logged_out' => false, + ], + // note 0 = anon user + 0, + // `vectoruserlinks` query param + "1", + true, + 'Anon users get new treatment when query param set to "1" regardless of config (new config)' + ], + [ + // Is consolidate user links enabled (backward compatibility with boolean values) + true, + // note 0 = anon user + 0, + // `vectoruserlinks` query param + "0", + false, + 'Anon user get old treatment when query param set to "0" regardless of config (old config)' + ], + [ + // Is consolidate user links enabled + [ + 'logged_in' => true, + 'logged_out' => true, + ], + // note 0 = anon user + 0, + // `vectoruserlinks` query param + "0", + false, + 'Anon user get old treatment when query param set to "0" regardless of config (new config)' + ], + [ + // Is consolidate user links enabled (backward compatibility with boolean values) + false, + // Logged in user + 2, + // `vectoruserlinks` query param + "1", + true, + 'Logged in users get new treatment when query param set to "1" regardless of config (old config)' + ], + [ + // Is consolidate user links enabled + [ + 'logged_in' => false, + 'logged_out' => false, + ], + // Logged in user + 2, + // `vectoruserlinks` query param + "1", + true, + 'Logged in users get new treatment when query param set to "1" regardless of config (new config)' + ], + [ + // Is consolidate user links enabled (backward compatibility with boolean values) + true, + // Logged in user + 2, + // `vectoruserlinks` query param + "0", + false, + 'Logged in user get old treatment when query param set to "0" regardless of config (old config)' + ], + [ + // Is consolidate user links enabled + [ + 'logged_in' => true, + 'logged_out' => true, + ], + // Logged in user + 2, + // `vectoruserlinks` query param + "0", + false, + 'Logged in user get old treatment when query param set to "0" regardless of config (new config)' + ], + ]; + } + + /** + * @covers ::isMet + * @dataProvider providerUserLinksTreatmentRequirement + * @param bool $configValue + * @param int $userId + * @param string|null $queryParam + * @param bool $expected + * @param string $msg + */ + public function testUserLinksTreatmentRequirement( + $configValue, + $userId, + $queryParam, + $expected, + $msg + ) { + $config = new HashConfig( [ + Constants::CONFIG_CONSOLIDATE_USER_LINKS => $configValue, + ] ); + + $user = $this->createMock( User::class ); + $user->method( 'isRegistered' )->willReturn( $userId !== 0 ); + $user->method( 'getID' )->willReturn( $userId ); + + $request = $this->createMock( WebRequest::class ); + $request->method( 'getCheck' )->willReturn( $queryParam !== null ); + $request->method( 'getBool' )->willReturn( (bool)$queryParam ); + + $requirement = new OverridableConfigRequirement( + $config, + $user, + $request, + null, + 'VectorConsolidateUserLinks', + 'ConsolidateUserLinks', + 'vectoruserlinks', + null ); $this->assertSame( $expected, $requirement->isMet(), $msg );