Merge "Fix bug in SkinVersionLookup" into wmf/1.38.0-wmf.18

An error in both of these modules. This module is additive (it doesn't
replace the existing user module) so only needs to add new pages, not
append to existing pages.

Bug: T300070
Change-Id: I3ba2ce82ba924972d0f9fea763328510aef41f8e

includes/Constants.php

@@ -162,6 +162,12 @@ final class Constants {
 	 */
 	public const QUERY_PARAM_SKIN_VERSION = 'useskinversion';
 
+	/**
+	 * Override the skin user preference and site Config. See readme.
+	 * @var string
+	 */
+	public const QUERY_PARAM_SKIN = 'useskin';
+
 	/**
 	 * @var string
 	 */

includes/Hooks.php

@@ -208,23 +208,28 @@ class Hooks {
 	}
 
 	/**
-	 * Updates personal navigation menu (user links) for modern Vector wherein user page, create account and login links
-	 * are removed from the dropdown to be handled separately. In legacy Vector, the custom "user-page" bucket is
-	 * removed to preserve existing behavior.
+	 * Updates personal navigation menu (user links) dropdown for modern Vector:
+	 *  - Adds icons
+	 *  - Makes user page and watchlist collapsible
 	 *
 	 * @param SkinTemplate $sk
 	 * @param array &$content_navigation
 	 */
-	private static function updateUserLinksItems( $sk, &$content_navigation ) {
+	private static function updateUserLinksDropdownItems( $sk, &$content_navigation ) {
 		// For logged-in users in modern Vector, rearrange some links in the personal toolbar.
 		if ( $sk->getUser()->isRegistered() ) {
-			// Remove user page from personal menu dropdown for logged in users at higher resolutions.
+			// Remove user page from personal menu dropdown for logged in use
 			self::makeMenuItemCollapsible(
 				$content_navigation['user-menu']['userpage']
 			);
-			self::makeMenuItemCollapsible(
-				$content_navigation['user-menu']['watchlist']
-			);
+			// watchlist may be disabled if $wgGroupPermissions['*']['viewmywatchlist'] = false;
+			// See [[phab:T299671]]
+			$wlItem = $content_navigation['user-menu']['watchlist'] ?? false;
+			if ( $wlItem ) {
+				self::makeMenuItemCollapsible(
+					$wlItem
+				);
+			}
 			// Remove logout link from user-menu and recreate it in SkinVector,
 			unset( $content_navigation['user-menu']['logout'] );
 			// Don't show icons for anon menu items (besides login and create account).
@@ -246,7 +251,21 @@ class Hooks {
 			// are set to false.
 			unset( $content_navigation['user-menu']['login-private'] );
 		}
+	}
 
+	/**
+	 * Updates personal navigation menu (user links) for modern Vector wherein user page, create account and login links
+	 * are removed from the dropdown to be handled separately. In legacy Vector, the custom "user-page" bucket is
+	 * removed to preserve existing behavior.
+	 *
+	 * @param SkinTemplate $sk
+	 * @param array &$content_navigation
+	 */
+	private static function updateUserLinksItems( $sk, &$content_navigation ) {
+		$hasUserMenu = $content_navigation['user-menu'] ?? false;
+		if ( $hasUserMenu ) {
+			self::updateUserLinksDropdownItems( $sk, $content_navigation );
+		}
 		// ULS and user page links are hidden at lower resolutions.
 		if ( $content_navigation['user-interface-preferences'] ) {
 			self::makeMenuItemCollapsible(

includes/ResourceLoader/VectorResourceLoaderUserModule.php

@@ -16,7 +16,6 @@ class VectorResourceLoaderUserModule extends ResourceLoaderUserModule {
 		$user = $context->getUserObj();
 		$pages = [];
 		if ( $config->get( 'AllowUserCss' ) && !$user->isAnon() && ( $skin === Constants::SKIN_NAME_MODERN ) ) {
-			$pages = parent::getPages( $context );
 			$userPage = $user->getUserPage()->getPrefixedDBkey();
 			$pages["$userPage/vector.js"] = [ 'type' => 'script' ];
 		}

includes/ResourceLoader/VectorResourceLoaderUserStylesModule.php

@@ -16,7 +16,6 @@ class VectorResourceLoaderUserStylesModule extends ResourceLoaderUserStylesModul
 		$user = $context->getUserObj();
 		$pages = [];
 		if ( $config->get( 'AllowUserCss' ) && !$user->isAnon() && ( $skin === Constants::SKIN_NAME_MODERN ) ) {
-			$pages = parent::getPages( $context );
 			$userPage = $user->getUserPage()->getPrefixedDBkey();
 			$pages["$userPage/vector.css"] = [ 'type' => 'style' ];
 		}

includes/SkinVersionLookup.php

@@ -107,6 +107,23 @@ final class SkinVersionLookup {
 	 * @throws \ConfigException
 	 */
 	public function getVersion(): string {
+		$migrationMode = $this->config->get( 'VectorSkinMigrationMode' );
+		$useSkin = $this->request->getVal(
+			Constants::QUERY_PARAM_SKIN
+		);
+		// In migration mode, the useskin parameter is the source of truth.
+		if ( $migrationMode ) {
+			if ( $useSkin ) {
+				return $useSkin === Constants::SKIN_NAME_LEGACY ?
+					Constants::SKIN_VERSION_LEGACY :
+					Constants::SKIN_VERSION_LATEST;
+			}
+		}
+		// [[phab:T299971]]
+		if ( $useSkin === Constants::SKIN_NAME_MODERN ) {
+			return Constants::SKIN_VERSION_LATEST;
+		}
+
 		// If skin key is not vector, then version should be considered legacy.
 
 		// If skin is "Vector" invoke additional skin versioning detection.
@@ -138,9 +155,12 @@ final class SkinVersionLookup {
 			)
 		);
 
-		// If we are in migration mode, we must check the skin version preference.
-		if ( $this->config->get( 'VectorSkinMigrationMode' ) ) {
+		// If we are in migration mode...
+		if ( $migrationMode ) {
+			// ... we must check the skin version preference for logged in users.
+			// No need to check for anons as wgDefaultSkin has already been consulted at this point.
 			if (
+				$this->user->isRegistered() &&
 				$skin === Constants::SKIN_NAME_LEGACY &&
 				$skinVersionPref === Constants::SKIN_VERSION_LATEST
 			) {

tests/phpunit/integration/SkinVersionLookupTest.php

@@ -36,8 +36,13 @@ class SkinVersionLookupTest extends \MediaWikiIntegrationTestCase {
 		$request = $this->getMockBuilder( \WebRequest::class )->getMock();
 		$request
 			->method( 'getVal' )
-			->with( $this->anything(), 'beta' )
-			->willReturn( 'alpha' );
+			->willReturnCallback( static function ( $key ) {
+				if ( $key === Constants::QUERY_PARAM_SKIN ) {
+					return null;
+				} else {
+					return 'alpha';
+				}
+			} );
 
 		$user = $this->createMock( \User::class );
 		$user
@@ -76,8 +81,13 @@ class SkinVersionLookupTest extends \MediaWikiIntegrationTestCase {
 		$request = $this->getMockBuilder( \WebRequest::class )->getMock();
 		$request
 			->method( 'getVal' )
-			->with( $this->anything(), 'beta' )
-			->willReturn( 'beta' );
+			->willReturnCallback( static function ( $key ) {
+				if ( $key === Constants::QUERY_PARAM_SKIN ) {
+					return null;
+				} else {
+					return 'beta';
+				}
+			} );
 
 		$user = $this->createMock( \User::class );
 		$user
@@ -116,8 +126,13 @@ class SkinVersionLookupTest extends \MediaWikiIntegrationTestCase {
 		$request = $this->getMockBuilder( \WebRequest::class )->getMock();
 		$request
 			->method( 'getVal' )
-			->with( $this->anything(), '1' )
-			->willReturn( '1' );
+			->willReturnCallback( static function ( $key ) {
+				if ( $key === Constants::QUERY_PARAM_SKIN ) {
+					return null;
+				} else {
+					return '1';
+				}
+			} );
 
 		$user = $this->createMock( \User::class );
 		$user
@@ -148,6 +163,114 @@ class SkinVersionLookupTest extends \MediaWikiIntegrationTestCase {
 		);
 	}
 
+	public function providerAnonUserMigrationMode() {
+		return [
+			// When no query string just return DefaultSkin version.
+			[
+				Constants::SKIN_NAME_LEGACY,
+				null,
+				Constants::SKIN_VERSION_LEGACY,
+			],
+			[
+				Constants::SKIN_NAME_MODERN,
+				null,
+				Constants::SKIN_VERSION_LATEST,
+			],
+			// When useskin=vector return legacy Vector version.
+			[
+				Constants::SKIN_NAME_LEGACY,
+				Constants::SKIN_NAME_LEGACY,
+				Constants::SKIN_VERSION_LEGACY,
+			],
+			[
+				Constants::SKIN_NAME_MODERN,
+				Constants::SKIN_NAME_LEGACY,
+				Constants::SKIN_VERSION_LEGACY,
+			],
+			// When useskin=vector-2022 return modern Vector.
+			[
+				Constants::SKIN_NAME_MODERN,
+				Constants::SKIN_NAME_MODERN,
+				Constants::SKIN_VERSION_LATEST,
+			],
+			[
+				Constants::SKIN_NAME_LEGACY,
+				Constants::SKIN_NAME_MODERN,
+				Constants::SKIN_VERSION_LATEST,
+			],
+		];
+	}
+
+	/**
+	 * @covers ::getVersion
+	 * @dataProvider providerAnonUserMigrationMode
+	 */
+	public function testVectorAnonUserMigrationModeWithUseSkinVector(
+		string $defaultSkin,
+		$useSkin,
+		$expectedVersion
+	) {
+		$request = $this->getMockBuilder( \WebRequest::class )->getMock();
+		$request
+			->method( 'getVal' )
+			->with( 'useskin' )
+			->willReturn( $useSkin );
+		$user = $this->createMock( \User::class );
+		$user
+			->method( 'isRegistered' )
+			->willReturn( false );
+
+		$config = new HashConfig( [
+			'DefaultSkin' => $defaultSkin,
+			'VectorSkinMigrationMode' => true,
+			'VectorDefaultSkinVersion' => '2',
+			'VectorDefaultSkinVersionForExistingAccounts' => '2'
+		] );
+		$userOptionsLookup = $this->getUserOptionsLookupMock( $user, '2', [
+			'skin' => $defaultSkin,
+		] );
+
+		$skinVersionLookup = new SkinVersionLookup( $request, $user, $config, $userOptionsLookup );
+
+		$this->assertSame(
+			$expectedVersion,
+			$skinVersionLookup->getVersion(),
+			'useskin=vector query string yields legacy skin in migration mode'
+		);
+	}
+
+	/**
+	 * @covers ::getVersion
+	 */
+	public function testVectorRegisteredUserMigrationMode() {
+		$request = $this->getMockBuilder( \WebRequest::class )->getMock();
+		$request
+			->method( 'getVal' )
+			->willReturn( null );
+		$user = $this->createMock( \User::class );
+		$user
+			->method( 'isRegistered' )
+			->willReturn( true );
+
+		$config = new HashConfig( [
+			'DefaultSkin' => 'vector',
+			'VectorSkinMigrationMode' => true,
+			'VectorDefaultSkinVersion' => '1',
+			'VectorDefaultSkinVersionForExistingAccounts' => '1'
+		] );
+		$userOptionsLookup = $this->getUserOptionsLookupMock( $user, '2', [
+			'skin' => Constants::SKIN_NAME_LEGACY
+		] );
+
+		$skinVersionLookup = new SkinVersionLookup( $request, $user, $config, $userOptionsLookup );
+
+		$this->assertSame(
+			'2',
+			$skinVersionLookup->getVersion(),
+			'If legacy skin is set with skin version modern, then the user gets modern skin still'
+		);
+	}
+
 	/**
 	 * @covers ::getVersion
 	 */
@@ -188,8 +311,13 @@ class SkinVersionLookupTest extends \MediaWikiIntegrationTestCase {
 		$request = $this->getMockBuilder( \WebRequest::class )->getMock();
 		$request
 			->method( 'getVal' )
-			->with( $this->anything(), '2' )
-			->willReturn( '2' );
+			->willReturnCallback( static function ( $key ) {
+				if ( $key === Constants::QUERY_PARAM_SKIN ) {
+					return null;
+				} else {
+					return '2';
+				}
+			} );
 
 		$user = $this->createMock( \User::class );
 		$user