From eb597645c318373dc94bd6f7cc954dda4d9c7af2 Mon Sep 17 00:00:00 2001 From: Clare Ming Date: Mon, 1 Aug 2022 12:23:14 -0600 Subject: [PATCH] Refactor TOC A/B test to bucket users on backend - Include temporary feature requirement for TOC A/B test. - Assumes 100% of logged-in users with even/odd user ids being assigned to treatment/control buckets respectively. - Sampling rates passed in by config are not considered during bucketing. - Update hook for adding needed TOC A/B test body classes. - Add test for temp feature. Note: the temporary feature requirement and associated hooks should be removed once the 2nd TOC A/B test concludes. Bug: T313435 Change-Id: If9c75235614af289cd50182baab29bec3155eb81 --- includes/Constants.php | 10 ++ .../TableOfContentsTreatmentRequirement.php | 84 ++++++++++++++ includes/Hooks.php | 12 +- includes/ServiceWiring.php | 20 ++++ includes/SkinVector22.php | 15 ++- resources/skins.vector.AB.styles.less | 9 +- resources/skins.vector.es6/main.js | 1 + .../components/TableOfContents.less | 8 -- ...ableOfContentsTreatmentRequirementTest.php | 105 ++++++++++++++++++ 9 files changed, 246 insertions(+), 18 deletions(-) create mode 100644 includes/FeatureManagement/Requirements/TableOfContentsTreatmentRequirement.php create mode 100644 tests/phpunit/unit/FeatureManagement/Requirements/TableOfContentsTreatmentRequirementTest.php diff --git a/includes/Constants.php b/includes/Constants.php index e00dedb7c..782bda2f1 100644 --- a/includes/Constants.php +++ b/includes/Constants.php @@ -206,6 +206,16 @@ final class Constants { */ public const FEATURE_LANGUAGE_ALERT_IN_SIDEBAR = 'LanguageAlertInSidebar'; + /** + * @var string + */ + public const FEATURE_TABLE_OF_CONTENTS = 'TableOfContents'; + + /** + * @var string + */ + public const REQUIREMENT_TABLE_OF_CONTENTS = 'TableOfContents'; + /** * @var string */ diff --git a/includes/FeatureManagement/Requirements/TableOfContentsTreatmentRequirement.php b/includes/FeatureManagement/Requirements/TableOfContentsTreatmentRequirement.php new file mode 100644 index 000000000..91b0518bb --- /dev/null +++ b/includes/FeatureManagement/Requirements/TableOfContentsTreatmentRequirement.php @@ -0,0 +1,84 @@ +config = $config; + $this->user = $user; + $this->centralIdLookup = $centralIdLookup; + } + + /** + * @inheritDoc + */ + public function getName(): string { + return Constants::REQUIREMENT_TABLE_OF_CONTENTS; + } + + /** + * If A/B test is enabled check whether the user is logged in and bucketed. + * + * @inheritDoc + * @throws \ConfigException + */ + public function isMet(): bool { + $currentAbTest = $this->config->get( Constants::CONFIG_WEB_AB_TEST_ENROLLMENT ); + if ( $currentAbTest['enabled'] && $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(); + } + + // This assume 100% sampling of logged-in users with roughly half + // in control or treatment buckets based on even or odd user ids. + // This does not cover unsampled users nor does it consider the + // sampling rates of any given bucket passed in via config. + return $id % 2 === 0; + } + return false; + } +} diff --git a/includes/Hooks.php b/includes/Hooks.php index e14ee3f9e..46dff357f 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -557,14 +557,18 @@ class Hooks implements } $classes = []; - $isTocABTestEnabled = $sk->isTOCABTestEnabled(); - if ( + $sk->isTOCABTestEnabled() && $sk->isTableOfContentsVisibleInSidebar() && - $isTocABTestEnabled + !$sk->getUser()->isAnon() ) { + $userBucket = !$sk->isUserInTocTreatmentBucket() + ? 'control' + : 'treatment'; $experimentConfig = $config->get( Constants::CONFIG_WEB_AB_TEST_ENROLLMENT ); - $classes[] = $experimentConfig[ 'name' ]; + $experimentName = $experimentConfig[ 'name' ]; + $classes[] = $experimentName; + $classes[] = "$experimentName-$userBucket"; } return $classes; diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 8851f9837..06f8016d7 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -27,6 +27,7 @@ use MediaWiki\Skins\Vector\Constants; use MediaWiki\Skins\Vector\FeatureManagement\FeatureManager; use MediaWiki\Skins\Vector\FeatureManagement\Requirements\DynamicConfigRequirement; use MediaWiki\Skins\Vector\FeatureManagement\Requirements\OverridableConfigRequirement; +use MediaWiki\Skins\Vector\FeatureManagement\Requirements\TableOfContentsTreatmentRequirement; return [ Constants::SERVICE_FEATURE_MANAGER => static function ( MediaWikiServices $services ) { @@ -186,6 +187,25 @@ return [ ] ); + // T313435 Feature: Table of Contents + // Temporary - remove after TOC A/B test is finished. + // ================================ + $featureManager->registerRequirement( + new TableOfContentsTreatmentRequirement( + $services->getMainConfig(), + $context->getUser(), + $services->getCentralIdLookupFactory()->getNonLocalLookup() + ) + ); + + $featureManager->registerFeature( + Constants::FEATURE_TABLE_OF_CONTENTS, + [ + Constants::REQUIREMENT_FULLY_INITIALISED, + Constants::REQUIREMENT_TABLE_OF_CONTENTS, + ] + ); + return $featureManager; } ]; diff --git a/includes/SkinVector22.php b/includes/SkinVector22.php index bb47d79b1..94baead36 100644 --- a/includes/SkinVector22.php +++ b/includes/SkinVector22.php @@ -2,8 +2,6 @@ namespace MediaWiki\Skins\Vector; -use MediaWiki\MediaWikiServices; - /** * @ingroup Skins * @package Vector @@ -37,8 +35,17 @@ class SkinVector22 extends SkinVector { $experimentConfig = $this->getConfig()->get( Constants::CONFIG_WEB_AB_TEST_ENROLLMENT ); return $experimentConfig['name'] === self::TOC_AB_TEST_NAME && - $experimentConfig['enabled'] && - MediaWikiServices::getInstance()->hasService( Constants::WEB_AB_TEST_ARTICLE_ID_FACTORY_SERVICE ); + $experimentConfig['enabled']; + } + + /** + * Check whether the user is bucketed in the treatment group for TOC. + * + * @return bool + */ + public function isUserInTocTreatmentBucket(): bool { + $featureManager = VectorServices::getFeatureManager(); + return $featureManager->isFeatureEnabled( Constants::FEATURE_TABLE_OF_CONTENTS ); } /** diff --git a/resources/skins.vector.AB.styles.less b/resources/skins.vector.AB.styles.less index 69dd0ffce..409c7410c 100644 --- a/resources/skins.vector.AB.styles.less +++ b/resources/skins.vector.AB.styles.less @@ -1,7 +1,12 @@ .skin-vector-toc-experiment-control .mw-table-of-contents-container, -.skin-vector-toc-experiment-treatment #toc, -.skin-vector-toc-experiment-unsampled .mw-table-of-contents-container { +.skin-vector-toc-experiment-control #vector-toc-collapsed-button, +body:not( .skin-vector-toc-experiment-control ) #toc { // This trumps any layout rules e.g. vector-layout-grid /* stylelint-disable-next-line declaration-no-important */ display: none !important; } + +// T313435 Show legacy toc for control group. +.skin-vector-toc-experiment-control #toc { + display: table; +} diff --git a/resources/skins.vector.es6/main.js b/resources/skins.vector.es6/main.js index 1f72d915e..953a11530 100644 --- a/resources/skins.vector.es6/main.js +++ b/resources/skins.vector.es6/main.js @@ -209,6 +209,7 @@ const main = () => { document.body.classList.contains( ABTestConfig.name ) && // eslint-disable-next-line compat/compat window.URLSearchParams && + !mw.user.isAnon() && initExperiment( ABTestConfig ); const isInTreatmentBucket = !!experiment && experiment.isInTreatmentBucket(); diff --git a/resources/skins.vector.styles/components/TableOfContents.less b/resources/skins.vector.styles/components/TableOfContents.less index 1bd582b3f..239fc34d6 100644 --- a/resources/skins.vector.styles/components/TableOfContents.less +++ b/resources/skins.vector.styles/components/TableOfContents.less @@ -123,11 +123,3 @@ .client-js body.rtl .sidebar-toc .sidebar-toc-toggle { transform: rotate( 90deg ); } - -// T300975 following media query for TOC experiment treatment -// class can be removed once associated A/B test is over. -@media ( max-width: @max-width-tablet ) { - .skin-vector-toc-experiment-treatment #toc { - display: table; - } -} diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/TableOfContentsTreatmentRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/TableOfContentsTreatmentRequirementTest.php new file mode 100644 index 000000000..baf92d2ce --- /dev/null +++ b/tests/phpunit/unit/FeatureManagement/Requirements/TableOfContentsTreatmentRequirementTest.php @@ -0,0 +1,105 @@ + [ + 'name' => 'skin-vector-toc-experiment', + 'enabled' => $abValue, + 'buckets' => [ + 'control' => [ + 'samplingRate' => 0.5, + ], + 'treatment' => [ + 'samplingRate' => 0.5, + ] + ] + ], + ] ); + + $user = $this->createMock( User::class ); + $user->method( 'isRegistered' )->willReturn( $userId !== 0 ); + $user->method( 'getID' )->willReturn( $userId ); + + $centralIdLookup = $this->createMock( CentralIdLookup::class ); + $centralIdLookup->method( 'centralIdFromLocalUser' )->willReturn( $userId ); + + $requirement = new TableOfContentsTreatmentRequirement( + $config, + $user, + $useCentralIdLookup ? $centralIdLookup : null + ); + + $this->assertSame( $expected, $requirement->isMet(), $msg ); + } +}