golem
/
VectorGOLEM
9
0
Fork 0
Code Issues 1 Pull Requests Releases Wiki Activity

Merge "[fix] "Existing account only" skin version config"

This commit is contained in:
jenkins-bot 2020-04-29 18:55:30 +00:00 committed by Gerrit Code Review
parent d4c2e2b441 709772fa12
commit ec6307e669
8 changed files with 350 additions and 164 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php
View File

@ -22,11 +22,9 @@
namespace Vector\FeatureManagement\Requirements;
use Config;
use User;
use Vector\Constants;
use Vector\FeatureManagement\Requirement;
use WebRequest;
use Vector\SkinVersionLookup;
/**
* Retrieve the skin version for the request and compare it with `Constants::SKIN_VERSION_LATEST`.
@ -58,29 +56,17 @@ use WebRequest;
final class LatestSkinVersionRequirement implements Requirement {
/**
* @var WebRequest
* @var SkinVersionLookup
*/
private $request;
/**
* @var User
*/
private $user;
/**
* @var Config
*/
private $config;
private $skinVersionLookup;
/**
* This constructor accepts all dependencies needed to obtain the skin version.
*
* @param WebRequest $request
* @param User $user
* @param Config $config
* @param SkinVersionLookup $skinVersionLookup
*/
public function __construct( WebRequest $request, User $user, Config $config ) {
$this->request = $request;
$this->user = $user;
$this->config = $config;
public function __construct( SkinVersionLookup $skinVersionLookup ) {
$this->skinVersionLookup = $skinVersionLookup;
}
/**
@ -95,27 +81,6 @@ final class LatestSkinVersionRequirement implements Requirement {
* @throws \ConfigException
*/
public function isMet() : bool {
return $this->getVersion() === Constants::SKIN_VERSION_LATEST;
}
/**
* The skin version as a string. E.g., `Constants::SKIN_VERSION_LATEST`,
* `Constants::SKIN_VERSION_LATEST`, or maybe 'beta'. Note: it's likely someone will put
* arbitrary strings in the query parameter which means this function returns those strings as
* is.
*
* @return string
* @throws \ConfigException
*/
private function getVersion() : string {
// Obtain the skin version from the `useskinversion` URL query parameter override, the user
// preference, or the configured default.
return (string)$this->request->getVal(
Constants::QUERY_PARAM_SKIN_VERSION,
$this->user->getOption(
Constants::PREF_KEY_SKIN_VERSION,
$this->config->get( Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION )
)
);
return $this->skinVersionLookup->getVersion() === Constants::SKIN_VERSION_LATEST;
}
}

31
includes/Hooks.php
View File

@ -5,6 +5,7 @@ namespace Vector;
use HTMLForm;
use MediaWiki\MediaWikiServices;
use OutputPage;
use RequestContext;
use SkinTemplate;
use SkinVector;
use User;
@ -44,6 +45,10 @@ class Hooks {
return;
}
$skinVersionLookup = new SkinVersionLookup(
RequestContext::getMain()->getRequest(), $user, self::getServiceConfig()
);
// Preferences to add.
$vectorPrefs = [
Constants::PREF_KEY_SKIN_VERSION => [
@ -56,10 +61,7 @@ class Hooks {
// indicates that a prefs-skin-prefs string will be provided.
'section' => 'rendering/skin/skin-prefs',
// Convert the preference string to a boolean presentation.
'default' =>
$user->getOption( Constants::PREF_KEY_SKIN_VERSION ) === Constants::SKIN_VERSION_LATEST ?
'0' :
'1',
'default' => $skinVersionLookup->isLegacy() ? '1' : '0',
// Only show this section when the Vector skin is checked. The JavaScript client also uses
// this state to determine whether to show or hide the whole section.
'hide-if' => [ '!==', 'wpskin', Constants::SKIN_NAME ]
@ -119,16 +121,6 @@ class Hooks {
}
}
/**
* Called on each pageview to populate preference defaults for existing users.
*
* @param array &$defaultPrefs
*/
public static function onUserGetDefaultOptions( array &$defaultPrefs ) {
$default = self::getConfig( Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION_FOR_EXISTING_ACCOUNTS );
$defaultPrefs[ Constants::PREF_KEY_SKIN_VERSION ] = $default;
}
/**
* Called one time when initializing a users preferences for a newly created account.
*
@ -150,8 +142,13 @@ class Hooks {
* @throws \ConfigException
*/
private static function getConfig( $name ) {
/* @var Config */ $service =
MediaWikiServices::getInstance()->getService( Constants::SERVICE_CONFIG );
return $service->get( $name );
return self::getServiceConfig()->get( $name );
}
/**
* @return \Config
*/
private static function getServiceConfig() {
return MediaWikiServices::getInstance()->getService( Constants::SERVICE_CONFIG );
}
}

9
includes/ServiceWiring.php
View File

@ -27,6 +27,7 @@ use Vector\Constants;
use Vector\FeatureManagement\FeatureManager;
use Vector\FeatureManagement\Requirements\DynamicConfigRequirement;
use Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement;
use Vector\SkinVersionLookup;
return [
Constants::SERVICE_CONFIG => function ( MediaWikiServices $services ) {
@ -49,9 +50,11 @@ return [
$featureManager->registerRequirement(
new LatestSkinVersionRequirement(
$context->getRequest(),
$context->getUser(),
$context->getConfig()
new SkinVersionLookup(
$context->getRequest(),
$context->getUser(),
$services->getService( Constants::SERVICE_CONFIG )
)
)
);

119
includes/SkinVersionLookup.php Normal file
View File

@ -0,0 +1,119 @@
<?php
/**
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
* http://www.gnu.org/copyleft/gpl.html
*
* @file
* @since 1.35
*/
namespace Vector;
use Config;
use User;
use WebRequest;
/**
* Given initial dependencies, retrieve the current skin version. This class does no parsing, just
* the lookup.
*
* Skin version is evaluated in the following order:
*
* - useskinversion URL query parameter override. See readme.
*
* - User preference. The User object for new accounts is updated (persisted as a user preference)
* by hook according to VectorDefaultSkinVersionForNewAccounts. See Hooks and skin.json. The user
* may then change the preference at will.
*
* - Site configuration default. The default is controlled by VectorDefaultSkinVersion and
* VectorDefaultSkinVersionForExistingAccounts based on login state. The former is used
* for anonymous users and as a fallback configuration, the latter is logged in users (existing
* accounts). See skin.json.
*
* @unstable
*
* @package Vector
* @internal
*/
final class SkinVersionLookup {
/**
* @var WebRequest
*/
private $request;
/**
* @var User
*/
private $user;
/**
* @var Config
*/
private $config;
/**
* This constructor accepts all dependencies needed to obtain the skin version. The dependencies
* are lazily evaluated, not cached, meaning they always return the current results.
*
* @param WebRequest $request
* @param User $user
* @param Config $config
*/
public function __construct( WebRequest $request, User $user, Config $config ) {
$this->request = $request;
$this->user = $user;
$this->config = $config;
}
/**
* Whether or not the legacy skin is being used.
*
* @return bool
* @throws \ConfigException
*/
public function isLegacy(): bool {
return $this->getVersion() === Constants::SKIN_VERSION_LEGACY;
}
/**
* The skin version as a string. E.g., `Constants::SKIN_VERSION_LATEST`,
* `Constants::SKIN_VERSION_LATEST`, or maybe 'beta'. Note: it's likely someone will put arbitrary
* strings in the query parameter which means this function returns those strings as is.
*
* @return string
* @throws \ConfigException
*/
public function getVersion(): string {
// Obtain the skin version from the 1) `useskinversion` URL query parameter override, 2) the
// user preference, 3) the configured default for logged in users, 4) or the site default.
//
// The latter two configurations cannot be set by `Hooks::onUserGetDefaultOptions()` as user
// sessions are unavailable at that time so it's not possible to determine whether the
// preference is for a logged in user or an anonymous user. Since new users are known to have
// had their user preferences initialized in `Hooks::onLocalUserCreated()`, that means all
// subsequent requests to `User->getOption()` that do not have a preference set are either
// existing accounts or anonymous users. Login state makes the distinction.
return (string)$this->request->getVal(
Constants::QUERY_PARAM_SKIN_VERSION,
$this->user->getOption(
Constants::PREF_KEY_SKIN_VERSION,
$this->config->get(
$this->user->isLoggedIn()
? Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION_FOR_EXISTING_ACCOUNTS
: Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION
)
)
);
}
}

7
skin.json
View File

@ -35,7 +35,6 @@
"BeforePageDisplayMobile": "Vector\\Hooks::onBeforePageDisplayMobile",
"GetPreferences": "Vector\\Hooks::onGetPreferences",
"PreferencesFormPreSave": "Vector\\Hooks::onPreferencesFormPreSave",
"UserGetDefaultOptions": "Vector\\Hooks::onUserGetDefaultOptions",
"LocalUserCreated": "Vector\\Hooks::onLocalUserCreated"
},
"@note": "When modifying skins.vector.styles definition, make sure the installer still works",
@ -122,15 +121,15 @@
},
"VectorDefaultSkinVersion": {
"value": "2",
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use for anonymous users and as a fallback."
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use for anonymous users and as a fallback. The value is _not_ persisted."
},
"VectorDefaultSkinVersionForExistingAccounts": {
"value": "2",
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use when an existing user has not specified a preference. This configuration is not used for new accounts (see VectorDefaultSkinVersionForNewAccounts) and is impermanent. In the future, this field may contains versions such as \"beta\" which when specified and the BetaFeatures extension is installed, and the user is enrolled, the latest version is used otherwise legacy."
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use when an existing user has not specified a preference. This configuration is not used for new accounts (see VectorDefaultSkinVersionForNewAccounts) and is impermanent. In the future, this field may contains versions such as \"beta\" which when specified and the BetaFeatures extension is installed, and the user is enrolled, the latest version is used otherwise legacy. The value is _not_ persisted."
},
"VectorDefaultSkinVersionForNewAccounts": {
"value": "2",
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to **set** for newly created user accounts. This configuration is not used for preexisting accounts (see VectorDefaultSkinVersion) and only ever executed once at new account creation time."
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to **set** for newly created user accounts. **The value is persisted as a user preference.** This configuration is not used for preexisting accounts (see VectorDefaultSkinVersion) and only ever executed once at new account creation time."
}
},
"ServiceWiringFiles": [

69
tests/phpunit/integration/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php
View File

@ -20,6 +20,7 @@
*/
use Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement;
use Vector\SkinVersionLookup;
/**
* @group Vector
@ -29,73 +30,27 @@ class LatestSkinVersionRequirementTest extends \MediaWikiTestCase {
/**
* @covers ::isMet
* @covers ::getVersion
*/
public function testRequest() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->expects( $this->exactly( 1 ) )
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( '1' ) )
->willReturn( 'beta' );
public function testUnmet() {
$config = new HashConfig( [ 'VectorDefaultSkinVersionForExistingAccounts' => '1' ] );
$user = $this->createMock( \User::class );
$user
->expects( $this->exactly( 1 ) )
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '1' );
$config = new \HashConfig( [ 'VectorDefaultSkinVersion' => '2' ] );
$requirement = new LatestSkinVersionRequirement( $request, $user, $config );
$this->assertFalse(
$requirement->isMet(),
'WebRequest::getVal takes precedence. "beta" isn\'t considered latest.'
$requirement = new LatestSkinVersionRequirement(
new SkinVersionLookup( new WebRequest(), $this->getTestUser()->getUser(), $config )
);
$this->assertFalse( $requirement->isMet(), '"1" isn\'t considered latest.' );
}
/**
* @covers ::isMet
* @covers ::getVersion
*/
public function testUserPreference() {
$request = new WebRequest();
public function testMet() {
$config = new HashConfig( [ 'VectorDefaultSkinVersionForExistingAccounts' => '2' ] );
$user = $this->createMock( \User::class );
$user
->expects( $this->exactly( 1 ) )
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '1' );
$config = new \HashConfig( [ 'VectorDefaultSkinVersion' => '2' ] );
$requirement = new LatestSkinVersionRequirement( $request, $user, $config );
$this->assertFalse(
$requirement->isMet(),
'User preference takes second place. "1" (legacy) isn\'t considered latest.'
$requirement = new LatestSkinVersionRequirement(
new SkinVersionLookup( new WebRequest(), $this->getTestUser()->getUser(), $config )
);
}
/**
* @covers ::isMet
* @covers ::getVersion
*/
public function testConfig() {
$request = new WebRequest();
$user = \MediaWikiTestCase::getTestUser()
->getUser();
$config = new HashConfig( [ 'VectorDefaultSkinVersion' => '2' ] );
$requirement = new LatestSkinVersionRequirement( $request, $user, $config );
$this->assertTrue(
$requirement->isMet(),
'Config takes third place. "2" is considered latest.'
);
$this->assertTrue( $requirement->isMet(), '"2" is considered latest.' );
}
}

184
tests/phpunit/integration/SkinVersionLookupTest.php Normal file
View File

@ -0,0 +1,184 @@
<?php
/**
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
* http://www.gnu.org/copyleft/gpl.html
*
* @file
* @since 1.35
*/
use Vector\SkinVersionLookup;
/**
* @group Vector
* @coversDefaultClass \Vector\SkinVersionLookup
*/
class SkinVersionLookupTest extends \MediaWikiTestCase {
/**
* @covers ::isLegacy
* @covers ::getVersion
*/
public function testRequest() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( 'beta' ) )
->willReturn( 'alpha' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( false );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( 'beta' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'alpha',
'Query parameter is the first priority.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
false,
'Version is non-Legacy.'
);
}
/**
* @covers ::getVersion
* @covers ::isLegacy
*/
public function testUserPreference() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( 'beta' ) )
->willReturn( 'beta' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( false );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( 'beta' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'beta',
'User preference is the second priority.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
false,
'Version is non-Legacy.'
);
}
/**
* @covers ::getVersion
* @covers ::isLegacy
*/
public function testConfigLoggedIn() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( '1' ) )
->willReturn( '1' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( true );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '1' ) )
->willReturn( '1' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'1',
'Config is the third priority and distinguishes logged in users from anonymous users.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
true,
'Version is Legacy.'
);
}
/**
* @covers ::getVersion
* @covers ::isLegacy
*/
public function testConfigAnon() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '2' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( false );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '2' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'2',
'Config is the third priority and distinguishes anonymous users from logged in users.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
false,
'Version is non-Legacy.'
);
}
}

46
tests/phpunit/integration/VectorHooksTest.php
View File

@ -35,7 +35,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionFoundLegacy() {
$config = new HashConfig( [
'VectorShowSkinPreferences' => true,
// Required by test user's onUserGetDefaultOptions() hook but unused for this test.
// '1' is Legacy.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
] );
$this->setService( 'Vector.Config', $config );
@ -72,7 +72,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionMissingLegacy() {
$config = new HashConfig( [
'VectorShowSkinPreferences' => true,
// Required by test user's onUserGetDefaultOptions() hook but unused for this test.
// '1' is Legacy.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
] );
$this->setService( 'Vector.Config', $config );
@ -107,8 +107,8 @@ class VectorHooksTest extends \MediaWikiTestCase {
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionMissingLatest() {
$config = new HashConfig( [
'VectorShowSkinPreferences' => true,
// Required by test user's onUserGetDefaultOptions() hook but unused for this test.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
// '2' is latest.
'VectorDefaultSkinVersionForExistingAccounts' => '2',
] );
$this->setService( 'Vector.Config', $config );
@ -116,13 +116,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
'foo' => [],
'bar' => [],
];
$user = $this->createMock( \User::class );
$user->expects( $this->once() )
->method( 'getOption' )
->with( 'VectorSkinVersion' )
// '2' is latest.
->will( $this->returnValue( '2' ) );
Hooks::onGetPreferences( $user, $prefs );
Hooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs );
$this->assertSame(
$prefs,
[
@ -240,36 +234,6 @@ class VectorHooksTest extends \MediaWikiTestCase {
Hooks::onPreferencesFormPreSave( $formData, $form, $user, $result, $oldPreferences );
}
/**
* @covers ::onUserGetDefaultOptions
*/
public function testOnUserGetDefaultOptionsLegacy() {
$config = new HashConfig( [
// '1' is Legacy.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
] );
$this->setService( 'Vector.Config', $config );
$prefs = [];
Hooks::onUserGetDefaultOptions( $prefs );
$this->assertSame( $prefs, [ 'VectorSkinVersion' => '1' ], 'Version is Legacy.' );
}
/**
* @covers ::onUserGetDefaultOptions
*/
public function testOnUserGetDefaultOptionsLatest() {
$config = new HashConfig( [
// '2' is latest.
'VectorDefaultSkinVersionForExistingAccounts' => '2',
] );
$this->setService( 'Vector.Config', $config );
$prefs = [];
Hooks::onUserGetDefaultOptions( $prefs );
$this->assertSame( $prefs, [ 'VectorSkinVersion' => '2' ], 'Version is latest.' );
}
/**
* @covers ::onLocalUserCreated
*/