Only add Popups code to output page where needed

If the Popups code is enabled in beta feature mode only then
only add it to the page if the beta feature is enabled.

isEnabledByUser now returns false if the user is anonymous
and Popups is restricted to beta mode.

Bug: T156290
Change-Id: If152d2a67a079050173c6d642e0960b59730bc6d

Popups.hooks.php

@@ -86,6 +86,7 @@ class PopupsHooks {
 
 	public static function onBeforePageDisplay( OutputPage &$out, Skin &$skin ) {
 		$module = PopupsContext::getInstance();
+		$user = $out->getUser();
 
 		if ( !$module->areDependenciesMet() ) {
 			$logger = $module->getLogger();
@@ -94,7 +95,9 @@ class PopupsHooks {
 			return true;
 		}
 
-		$out->addModules( [ 'ext.popups' ] );
+		if ( !$module->isBetaFeatureEnabled() || $module->isEnabledByUser( $user ) ) {
+			$out->addModules( [ 'ext.popups' ] );
+		}
 
 		return true;
 	}

includes/PopupsContext.php

@@ -148,13 +148,12 @@ class PopupsContext {
 	 * @return bool
 	 */
 	public function isEnabledByUser( \User $user ) {
-		if ( $user->isAnon() ) {
-			return true;
-		}
 		if ( $this->isBetaFeatureEnabled() ) {
-			return \BetaFeatures::isFeatureEnabled( $user, self::PREVIEWS_BETA_PREFERENCE_NAME );
-		};
-		return $user->getOption( self::PREVIEWS_OPTIN_PREFERENCE_NAME ) === self::PREVIEWS_ENABLED;
+			return $user->isAnon() ? false :
+				\BetaFeatures::isFeatureEnabled( $user, self::PREVIEWS_BETA_PREFERENCE_NAME );
+		}
+		return $user->isAnon() ? true :
+			$user->getOption( self::PREVIEWS_OPTIN_PREFERENCE_NAME ) === self::PREVIEWS_ENABLED;
 	}
 
 	/**

tests/phpunit/PopupsContextTest.php

@@ -158,18 +158,28 @@ class PopupsContextTest extends MediaWikiTestCase {
 	 * Check that Page Previews are disabled for anonymous user
 	 * @covers ::isEnabledByUser
 	 * @covers ::isBetaFeatureEnabled
+	 * @dataProvider providerAnonUserHasDisabledPagePreviews
 	 */
-	public function testAnonUserHasDisabledPagePreviews() {
+	public function testAnonUserHasDisabledPagePreviews( $betaFeatureEnabled, $expected ) {
 		$user = $this->getMutableTestUser()->getUser();
 		$user->setId( self::ANONYMOUS_USER );
 		$user->setOption( PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME,
 			PopupsContext::PREVIEWS_DISABLED );
 		$this->setMwGlobals( [
-			"wgPopupsBetaFeature" => false
+			"wgPopupsBetaFeature" => $betaFeatureEnabled,
 		] );
 
 		$context = PopupsContext::getInstance();
-		$this->assertEquals( true, $context->isEnabledByUser( $user ) );
+		$this->assertEquals( $expected, $context->isEnabledByUser( $user ) );
+	}
+
+	public static function providerAnonUserHasDisabledPagePreviews() {
+		return [
+			// If beta feature is enabled we can assume it's opt in only.
+			[ true, false ],
+			// If beta feature is disabled we can assume it's rolled out to everyone.
+			[ false, true ],
+		];
 	}
 	/**
 	 * @return array/

tests/phpunit/PopupsHooksTest.php

@@ -257,10 +257,24 @@ class PopupsHooksTest extends MediaWikiTestCase {
 		PopupsHooks::onBeforePageDisplay( $outPageMock, $skinMock );
 	}
 
+	public function providerOnBeforePageDisplay() {
+		return [
+			[ false, true, false ],
+			[ true, true, true ],
+			// if the user doesnt have the feature but the beta feature is disabled
+			// we can assume the user has it (as its rolled out to everyone)
+			[ false, false, true ],
+			// If the user has enabled it and the beta feature is disabled
+			// we can assume the code will be loaded.
+			[ true, false, true ]
+		];
+	}
+
 	/**
 	 * @covers ::onBeforePageDisplay
+	 * @dataProvider providerOnBeforePageDisplay
 	 */
-	public function testOnBeforePageDisplay() {
+	public function testOnBeforePageDisplay( $isEnabledByUser, $isBetaFeatureEnabled, $isCodeLoaded ) {
 		$skinMock = $this->getMock( Skin::class );
 
 		$outPageMock = $this->getMock(
@@ -270,18 +284,28 @@ class PopupsHooksTest extends MediaWikiTestCase {
 			'',
 			false
 		);
-		$outPageMock->expects( $this->once() )
+
+		$outPageMock->expects( $isCodeLoaded ? $this->once() : $this->never() )
 			->method( 'addModules' )
 			->with( [ 'ext.popups' ] );
 
 		$contextMock = $this->getMockBuilder( PopupsContextTestWrapper::class )
-			->setMethods( [ 'areDependenciesMet' ] )
+			->setMethods( [ 'areDependenciesMet', 'isBetaFeatureEnabled', 'isEnabledByUser' ] )
 			->disableOriginalConstructor()
 			->getMock();
+
 		$contextMock->expects( $this->once() )
 			->method( 'areDependenciesMet' )
 			->will( $this->returnValue( true ) );
 
+		$contextMock->expects( $this->any() )
+			->method( 'isBetaFeatureEnabled' )
+			->will( $this->returnValue( $isBetaFeatureEnabled ) );
+
+		$contextMock->expects( $this->any() )
+			->method( 'isEnabledByUser' )
+			->will( $this->returnValue( $isEnabledByUser ) );
+
 		PopupsContextTestWrapper::injectTestInstance( $contextMock );
 		PopupsHooks::onBeforePageDisplay( $outPageMock, $skinMock );
 	}