Merge "Create A/B test harness for Language in header feature"

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

includes/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirement.php

@@ -0,0 +1,134 @@
+<?php
+
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ * @since 1.37
+ */
+
+namespace Vector\FeatureManagement\Requirements;
+
+use CentralIdLookup;
+use Config;
+use User;
+use Vector\Constants;
+use Vector\FeatureManagement\Requirement;
+use WebRequest;
+
+/**
+ * Checks whether or not Language button in header should be used.
+ *
+ * @unstable
+ *
+ * @package Vector\FeatureManagement\Requirements
+ * @internal
+ */
+final class LanguageInHeaderTreatmentRequirement implements Requirement {
+	/**
+	 * @var Config
+	 */
+	private $config;
+
+	/**
+	 * @var User
+	 */
+	private $user;
+
+	/**
+	 * @var WebRequest
+	 */
+	private $request;
+
+	/**
+	 * @var CentralIdLookup
+	 */
+	private $centralIdLookup;
+
+	/**
+	 * @param Config $config
+	 * @param User $user
+	 * @param WebRequest $request
+	 * @param CentralIdLookup|null $centralIdLookup
+	 */
+	public function __construct(
+		Config $config,
+		User $user,
+		WebRequest $request,
+		?CentralIdLookup $centralIdLookup
+	) {
+		$this->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' ];
+	}
+}

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();
+		$featureManager->registerRequirement(
-		$languageInHeaderConfig = $config->get( Constants::CONFIG_KEY_LANGUAGE_IN_HEADER );
+			new LanguageInHeaderTreatmentRequirement(
-
+				$services->getMainConfig(),
-		// Backwards compatibility with config variables that have been set in production.
+				$context->getUser(),
-		if ( is_bool( $languageInHeaderConfig ) ) {
+				$context->getRequest(),
-			$languageInHeaderConfig = [
+				CentralIdLookup::factoryNonLocal()
-				'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->registerFeature(

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."

tests/phpunit/unit/FeatureManagement/Requirements/LanguageInHeaderTreatmentRequirementTest.php

@@ -0,0 +1,279 @@
+<?php
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+namespace Vector\FeatureManagement\Tests;
+
+use CentralIdLookup;
+use HashConfig;
+use User;
+use Vector\Constants;
+use Vector\FeatureManagement\Requirements\LanguageInHeaderTreatmentRequirement;
+use WebRequest;
+
+/**
+ * @group Vector
+ * @group FeatureManagement
+ * @coversDefaultClass \Vector\FeatureManagement\Requirements\LanguageInHeaderTreatmentRequirement
+ */
+class LanguageInHeaderTreatmentRequirementTest extends \MediaWikiUnitTestCase {
+
+	public function providerLanguageInHeaderTreatmentRequirement() {
+		return [
+			[
+				// Is language enabled
+				[
+					'logged_in' => 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 );
+	}
+}