From a76e198523b4e2c997578e65d596e31e2837d4ca Mon Sep 17 00:00:00 2001 From: Jon Robson Date: Tue, 18 Apr 2023 14:11:30 -0700 Subject: [PATCH] Remove A/B testing element from OverridableConfigRequirement Superseded by ABRequirement. Having this code here is confusing and might lead to non-standard A/B tests being defined. Change-Id: Ifd9d2b7249250a73e7f6e4f9d6b51c322ef2759d --- .../OverridableConfigRequirement.php | 39 +--- includes/ServiceWiring.php | 6 - .../OverridableConfigRequirementTest.php | 220 +----------------- 3 files changed, 6 insertions(+), 259 deletions(-) diff --git a/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php b/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php index 88234d94e..b9e0edff3 100644 --- a/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php +++ b/includes/FeatureManagement/Requirements/OverridableConfigRequirement.php @@ -21,7 +21,6 @@ namespace MediaWiki\Skins\Vector\FeatureManagement\Requirements; -use CentralIdLookup; use Config; use MediaWiki\Skins\Vector\FeatureManagement\Requirement; use User; @@ -37,7 +36,6 @@ use WebRequest; * $config, * $user, * $request, - * $centralIdLookup, * MainConfigNames::Sitename, * 'requirementName', * 'overrideName', @@ -82,11 +80,6 @@ final class OverridableConfigRequirement implements Requirement { */ private $request; - /** - * @var CentralIdLookup|null - */ - private $centralIdLookup; - /** * @var string */ @@ -102,11 +95,6 @@ final class OverridableConfigRequirement implements Requirement { */ 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. @@ -114,31 +102,25 @@ final class OverridableConfigRequirement implements Requirement { * @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|null $overrideName The name of the override presented to FeatureManager, i.e. query parameter. * When not set defaults to lowercase version of config key. - * @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 = null, - ?string $configTestName = null + ?string $overrideName = null ) { $this->config = $config; $this->user = $user; $this->request = $request; - $this->centralIdLookup = $centralIdLookup; $this->configName = $configName; $this->requirementName = $requirementName; $this->overrideName = $overrideName === null ? strtolower( $configName ) : $overrideName; - $this->configTestName = $configTestName; } /** @@ -161,25 +143,6 @@ final class OverridableConfigRequirement implements Requirement { 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 ); diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 0616ab79e..0d119e6c4 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -58,7 +58,6 @@ return [ $services->getMainConfig(), $context->getUser(), $context->getRequest(), - $services->getCentralIdLookupFactory()->getNonLocalLookup(), Constants::CONFIG_KEY_LANGUAGE_IN_HEADER, Constants::REQUIREMENT_LANGUAGE_IN_HEADER, null @@ -92,7 +91,6 @@ return [ $config, $context->getUser(), $context->getRequest(), - $services->getCentralIdLookupFactory()->getNonLocalLookup(), Constants::CONFIG_KEY_LANGUAGE_IN_HEADER, $requirementName ) @@ -115,7 +113,6 @@ return [ $services->getMainConfig(), $context->getUser(), $context->getRequest(), - null, Constants::CONFIG_LANGUAGE_IN_MAIN_PAGE_HEADER, Constants::REQUIREMENT_LANGUAGE_IN_MAIN_PAGE_HEADER ) @@ -143,7 +140,6 @@ return [ $services->getMainConfig(), $context->getUser(), $context->getRequest(), - null, Constants::CONFIG_LANGUAGE_ALERT_IN_SIDEBAR, Constants::REQUIREMENT_LANGUAGE_ALERT_IN_SIDEBAR ) @@ -165,7 +161,6 @@ return [ $services->getMainConfig(), $context->getUser(), $context->getRequest(), - null, Constants::CONFIG_STICKY_HEADER, Constants::REQUIREMENT_STICKY_HEADER ) @@ -291,7 +286,6 @@ return [ $services->getMainConfig(), $context->getUser(), $context->getRequest(), - null, Constants::CONFIG_ZEBRA_DESIGN, Constants::REQUIREMENT_ZEBRA_DESIGN, Constants::REQUIREMENT_ZEBRA_AB_TEST diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php index 27ce94604..cc0b1596a 100644 --- a/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php +++ b/tests/phpunit/unit/FeatureManagement/Requirements/OverridableConfigRequirementTest.php @@ -20,7 +20,6 @@ namespace MediaWiki\Skins\Vector\Tests\Unit\FeatureManagement\Requirements; -use CentralIdLookup; use HashConfig; use MediaWiki\Skins\Vector\Constants; use MediaWiki\Skins\Vector\FeatureManagement\Requirements\OverridableConfigRequirement; @@ -42,16 +41,10 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { '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, - // AB test name - null, false, 'If nothing enabled, nobody gets new treatment' ], @@ -61,18 +54,12 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { '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, - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', true, - 'Anon users should get new treatment if enabled when A/B test disabled' + 'Anon users should get new treatment if enabled' ], [ // Is language enabled @@ -80,184 +67,11 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { 'logged_in' => true, 'logged_out' => false, ], - // is A-B test enabled - false, - 2, - // use central id lookup? - false, - // `languageinheader` query param - null, - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', - 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, - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', - 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, - // 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' - ], - [ - // 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, - // 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' - ], - [ - // 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, - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', - 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, - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', - 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, - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', - 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, - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', - 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", - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', - 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", - // AB test name - 'VectorLanguageInHeaderTreatmentABTest', false, - 'Even logged in users get old treatment when A/B test enabled and query param set to "0"' + 'Even logged in users get old treatment when query param set to "0"' ], [ // Is language enabled @@ -265,17 +79,11 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { 'logged_in' => false, 'logged_out' => false, ], - // is A-B test enabled - false, 1, - // use central id lookup? - 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' + 'Users get new treatment when query param set to "1" regardless of state of config flags' ], [ // Is language enabled @@ -283,17 +91,11 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { 'logged_in' => false, 'logged_out' => false, ], - // is A-B test enabled - false, 1, - // use central id lookup? - 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' + 'Users get old treatment when query param set to "0" regardless of state of config flags' ], ]; } @@ -302,27 +104,20 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { * @covers ::isMet * @dataProvider providerLanguageInHeaderTreatmentRequirement * @param bool $configValue - * @param bool $abValue * @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, - $testName, $expected, $msg ) { $config = new HashConfig( [ Constants::CONFIG_KEY_LANGUAGE_IN_HEADER => $configValue, - $testName => $abValue, ] ); $user = $this->createMock( User::class ); @@ -333,18 +128,13 @@ class OverridableConfigRequirementTest extends \MediaWikiUnitTestCase { $request->method( 'getCheck' )->willReturn( $queryParam !== null ); $request->method( 'getBool' )->willReturn( (bool)$queryParam ); - $centralIdLookup = $this->createMock( CentralIdLookup::class ); - $centralIdLookup->method( 'centralIdFromLocalUser' )->willReturn( $userId ); - $requirement = new OverridableConfigRequirement( $config, $user, $request, - $useCentralIdLookup ? $centralIdLookup : null, 'VectorLanguageInHeader', 'LanguageInHeader', - 'languageinheader', - $testName ?? null + 'languageinheader' ); $this->assertSame( $expected, $requirement->isMet(), $msg );