From 49f2b25737aa59494ceb1318361f5a2c65e15d8f Mon Sep 17 00:00:00 2001 From: Nicholas Ray Date: Wed, 28 Apr 2021 18:47:25 -0600 Subject: [PATCH] Create A/B test harness for Language in header feature * Adds ab test config to enable/disable the ab test. Defaults to `false` (ab test disabled). * Adds a `languageinheader` query param which only takes effect when the ab test is enabled. The query param is cast to a bool and determines which treatment is shown. For example, set query param to `languageinheader=1` to see the new treatment. Set query param to `languageinheader=0` to see the old treatment. To bucket based on the user's id or global user's id, don't set the query param. * Moves the language in header config work that was previously in ServiceWiring into a `LanguageInHeaderTreatmentRequirement` class so that unit tests can be done on most of the logic that determines whether the language in header will show. * Adds logic to bucket user based on [global] user id. Bug: T280825 Change-Id: Id538fe6e09002fae6c371109769f3b7d61e7ac6d --- includes/Constants.php | 23 ++ .../LanguageInHeaderTreatmentRequirement.php | 134 +++++++++ includes/ServiceWiring.php | 27 +- skin.json | 4 + ...nguageInHeaderTreatmentRequirementTest.php | 279 ++++++++++++++++++ 5 files changed, 448 insertions(+), 19 deletions(-) create mode 100644 includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php create mode 100644 tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php diff --git a/includes/Constants.php b/includes/Constants.php index eaa14581f..dbe1ebfcf 100644 --- a/includes/Constants.php +++ b/includes/Constants.php @@ -121,7 +121,30 @@ final class Constants { */ public const REQUIREMENT_LANGUAGE_IN_HEADER = 'LanguageInHeader'; + /** + * Defines whether or not the Language in header A/B test is running. See + * https://phabricator.wikimedia.org/T280825 for additional detail about the test. + * + * Note well that if the associated config value is falsy, then we fall back to choosing the + * language treatment based on the `VectorLanguageInHeader` config variable. + * + * @var string + */ + public const CONFIG_LANGUAGE_IN_HEADER_TREATMENT_AB_TEST = 'VectorLanguageInHeaderTreatmentABTest'; + // These are used for query parameters. + /** + * If undefined and AB test enabled, user will be bucketed as usual. + * + * If set, overrides the language in header AB test config: + * + * 'languageinheader=0' will show existing treatment. + * 'languageinheader=1' will show new treatment. + * + * @var string + */ + public const QUERY_PARAM_LANGUAGE_IN_HEADER = 'languageinheader'; + /** * Override the skin version user preference and site Config. See readme. * @var string diff --git a/includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php b/includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php new file mode 100644 index 000000000..6ce703cd3 --- /dev/null +++ b/includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php @@ -0,0 +1,134 @@ +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 ( + (bool)$this->config->get( Constants::CONFIG_LANGUAGE_IN_HEADER_TREATMENT_AB_TEST ) && + $this->user->isRegistered() + ) { + + if ( $this->request->getCheck( Constants::QUERY_PARAM_LANGUAGE_IN_HEADER ) ) { + return $this->request->getBool( Constants::QUERY_PARAM_LANGUAGE_IN_HEADER ); + } + + $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/ServiceWiring.php b/includes/ServiceWiring.php index c5bb9b34c..fdcc577b3 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -26,6 +26,7 @@ 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\WvuiSearchTreatmentRequirement; use Vector\SkinVersionLookup; @@ -69,25 +70,13 @@ return [ // Feature: Languages in sidebar // ================================ - $config = $services->getMainConfig(); - $languageInHeaderConfig = $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, - ]; - } - - $featureManager->registerSimpleRequirement( - Constants::REQUIREMENT_LANGUAGE_IN_HEADER, - $languageInHeaderConfig[ $context->getUser()->isRegistered() ? 'logged_in' : 'logged_out' ] + $featureManager->registerRequirement( + new LanguageInHeaderTreatmentRequirement( + $services->getMainConfig(), + $context->getUser(), + $context->getRequest(), + CentralIdLookup::factoryNonLocal() + ) ); $featureManager->registerFeature( diff --git a/skin.json b/skin.json index 69d30b929..ad2fa3b10 100644 --- a/skin.json +++ b/skin.json @@ -283,6 +283,10 @@ }, "description": "@var array Moves the language links from the sidebar into a menu beside the page title. Also moves the indicators to the line below, next to the tagline (siteSub)." }, + "VectorLanguageInHeaderTreatmentABTest": { + "value": false, + "description": "@var boolean Enables or disables the language in header treatment A/B test. See https://phabricator.wikimedia.org/T280825 and associated tasks for additional detail." + }, "VectorDisableSidebarPersistence": { "value": false, "description": "@var boolean Temporary feature flag that disables saving the sidebar expanded/collapsed state as a user-preference (triggered via clicking the main menu icon). This is intended as a temporary kill-switch in the event that the DB is overloaded with writes to the user_options table." diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php new file mode 100644 index 000000000..a042989c4 --- /dev/null +++ b/tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php @@ -0,0 +1,279 @@ + false, + 'logged_out' => false, + ], + // is A-B test enabled + false, + // note 0 = anon user + 0, + // use central id lookup? + false, + // `languageinheader` query param + null, + false, + 'If nothing enabled, nobody gets new treatment' + ], + [ + // Is language enabled + [ + 'logged_in' => false, + 'logged_out' => true, + ], + // is A-B test enabled + false, + // note 0 = anon user + 0, + // use central id lookup? + false, + // `languageinheader` query param + null, + true, + 'Anon users should get new treatment if enabled when A/B test disabled' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + false, + 2, + // use central id lookup? + false, + // `languageinheader` query param + null, + true, + 'Logged in users should get new treatment if enabled when A/B test disabled' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + false, + 1, + // use central id lookup? + false, + // `languageinheader` query param + null, + true, + 'All odd logged in users should get new treatent when A/B test disabled' + ], + [ + // Is language enabled + [ + 'logged_in' => false, + 'logged_out' => true, + ], + // is A-B test enabled + true, + // note 0 = anon user + 0, + // use central id lookup? + false, + // `languageinheader` query param + null, + true, + // Ab test is only for logged in users + 'Anon users with a/b test enabled should see new treatment when config enabled' + ], + [ + // Is language enabled + [ + 'logged_in' => false, + 'logged_out' => false, + ], + // is A-B test enabled + true, + // note 0 = anon user + 0, + // use central id lookup? + false, + // `languageinheader` query param + null, + false, + // Ab test is only for logged in users + 'Anon users with a/b test enabled should see old treatment when config disabled' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + true, + 2, + // use central id lookup? + false, + // `languageinheader` query param + null, + true, + 'Even logged in users get new treatment when A/B test enabled' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + true, + 1, + // use central id lookup? + false, + // `languageinheader` query param + null, + false, + 'Odd logged in users do not get new treatment when A/B test enabled' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + true, + 2, + // use central id lookup? + true, + // `languageinheader` query param + null, + true, + 'With CentralIdLookup, even logged in users get new treatment when A/B test enabled' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + true, + 1, + // use central id lookup? + true, + // `languageinheader` query param + null, + false, + 'With CentralIdLookup, odd logged in users do not get new treatment when A/B test enabled' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + true, + 1, + // use central id lookup? + false, + // `languageinheader` query param + "1", + true, + 'Odd logged in users get new treatment when A/B test enabled and query param set to "1"' + ], + [ + // Is language enabled + [ + 'logged_in' => true, + 'logged_out' => false, + ], + // is A-B test enabled + true, + 1, + // use central id lookup? + false, + // `languageinheader` query param + "0", + false, + 'Even logged in users get old treatment when A/B test enabled and query param set to "0"' + ], + ]; + } + + /** + * @covers ::isMet + * @dataProvider providerLanguageInHeaderTreatmentRequirement + * @param bool $configValue + * @param bool $abValue + * @param int $userId + * @param bool $useCentralIdLookup + * @param string|null $queryParam + * @param bool $expected + * @param string $msg + */ + public function testLanguageInHeaderTreatmentRequirement( + $configValue, $abValue, $userId, $useCentralIdLookup, $queryParam, $expected, $msg + ) { + $config = new HashConfig( [ + Constants::CONFIG_KEY_LANGUAGE_IN_HEADER => $configValue, + Constants::CONFIG_LANGUAGE_IN_HEADER_TREATMENT_AB_TEST => $abValue, + ] ); + + $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 ); + + $centralIdLookup = $this->createMock( CentralIdLookup::class ); + $centralIdLookup->method( 'centralIdFromLocalUser' )->willReturn( $userId ); + + $requirement = new LanguageInHeaderTreatmentRequirement( + $config, + $user, + $request, + $useCentralIdLookup ? $centralIdLookup : null + ); + + $this->assertSame( $expected, $requirement->isMet(), $msg ); + } +}