diff --git a/includes/Constants.php b/includes/Constants.php index e00dedb7..782bda2f 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 00000000..91b0518b --- /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 e14ee3f9..46dff357 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 8851f983..06f8016d 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 bb47d79b..94baead3 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 69dd0ffc..409c7410 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 717924c7..0d7da2c7 100644 --- a/resources/skins.vector.es6/main.js +++ b/resources/skins.vector.es6/main.js @@ -215,6 +215,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 1bd582b3..239fc34d 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 00000000..baf92d2c --- /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 ); + } +}