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

[Special:Preferences] [PHP] Add HTMLSkinVersionField form field 

I177dad88 introduced the skin version user preference field and
associated configuration values. Per T242381, the field is to presented
as a checkbox with the implied storage type of a boolean where a string
is needed. A PreferencesFormPreSave hook handler was added to adapt
values of either data type to the other.  While this was a neat solution
to a minor nit, the adapter's implementation is incompatible with the
GlobalPreferences extension as the PreferencesFormPreSave hook isn't run
whilst saving global preferences.

Rather than adding an equivalent hook to the GlobalPreferences
extension, create a custom field based on a checkbox with the adapter
included. This allows us to:

- Separate the business logic concerned with preserving the user's
  VectorSkinVersion preference if they've simply disabled Vector from
  the adapter

- Simplify the adapter's implementation

- Forego adding hooks to the GlobalPreferences codebase

Additional changes:

- Replace repeated string literals with equivalent constants in
  tests/phpunit/integration/VectorHooksTest.php

Bug: T258493
Change-Id: I628435a4ad676f55534191b8c10147be28be5d73
This commit is contained in:
Sam Smith 2020-08-11 15:13:12 +01:00 committed by jdlrobson
parent f6de8a4a17
commit 4449235516
4 changed files with 232 additions and 121 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

92
includes/HTMLForm/Fields/HTMLLegacySkinVersionField.php Normal file
View File

@ -0,0 +1,92 @@
<?php
namespace Vector\HTMLForm\Fields;
use Vector\Constants;
use Wikimedia\Assert\Assert;
/**
* The field on Special:Preferences (and Special:GlobalPreferences) that allows the user to
* enable/disable the legacy version of the Vector skin. Per
* https://phabricator.wikimedia.org/T242381, the field is a checkbox, that, when checked, enables
* the legacy version of the Vector skin.
*
* `HTMLLegacySkinVersionField` adapts the boolean storage type of a checkbox field to the string
* storage type of the Vector skin version preference (e.g. see `Constants::SKIN_VERSION_LEGACY`).
*
* However, we cannot extend `HTMLCheckField` to inherit the behavior of a checkbox field.
* `HTMLCheckField::loadDataFromRequest` returns boolean values. Returning non-boolean values in
* `HTMLLegacySkinVersionField::loadDataFromRequest` would violate Liskov's Substitution Principle.
* Like `HTMLExpiryField`, `HTMLLegacySkinVersionField` proxies to a private instance of
* `HTMLCheckField`, adapting parameter and return values where necessary.
*
* @package Vector\HTMLForm\Fields
* @internal
*/
final class HTMLLegacySkinVersionField extends \HTMLFormField {
/**
* @var \HTMLCheckField $checkField
*/
private $checkField;
/**
* @inheritDoc
*/
public function __construct( $params ) {
Assert::precondition(
is_bool( $params['default'] ),
'The "default" param must be a boolean.'
);
parent::__construct( $params );
$this->checkField = new \HTMLCheckField( $params );
}
// BEGIN ADAPTER
/** @inheritDoc */
public function getInputHTML( $value ) {
return $this->checkField->getInputHTML( $value === Constants::SKIN_VERSION_LEGACY );
}
/** @inheritDoc */
public function getInputOOUI( $value ) {
return $this->checkField->getInputOOUI( (string)( $value === Constants::SKIN_VERSION_LEGACY ) );
}
/**
* @inheritDoc
*
* @return string If the checkbox is checked, then `Constants::SKIN_VERSION_LEGACY`;
* `Constants::SKIN_VERSION_LATEST` otherwise
*/
public function loadDataFromRequest( $request ) {
return $this->checkField->loadDataFromRequest( $request )
? Constants::SKIN_VERSION_LEGACY
: Constants::SKIN_VERSION_LATEST;
}
// END ADAPTER
/** @inheritDoc */
public function getLabel() {
return $this->checkField->getLabel();
}
// Note well that we can't invoke the following methods of `HTMLCheckField` directly because
// they're protected and `HTMLSkinVectorField` doesn't extend `HTMLCheckField`.
/** @inheritDoc */
protected function getLabelAlignOOUI() {
// See \HTMLCheckField::getLabelAlignOOUI
return 'inline';
}
/** @inheritDoc */
protected function needsLabel() {
// See \HTMLCheckField::needsLabel
return false;
}
}

28
includes/Hooks.php
View File

@ -12,6 +12,7 @@ use Skin;
use SkinTemplate;
use SkinVector;
use User;
use Vector\HTMLForm\Fields\HTMLLegacySkinVersionField;
/**
* Presentation hook handlers for Vector skin.
@ -158,7 +159,7 @@ class Hooks {
// Preferences to add.
$vectorPrefs = [
Constants::PREF_KEY_SKIN_VERSION => [
'type' => 'toggle',
'class' => HTMLLegacySkinVersionField::class,
// The checkbox title.
'label-message' => 'prefs-vector-enable-vector-1-label',
// Show a little informational snippet underneath the checkbox.
@ -166,11 +167,10 @@ class Hooks {
// The tab location and title of the section to insert the checkbox. The bit after the slash
// indicates that a prefs-skin-prefs string will be provided.
'section' => 'rendering/skin/skin-prefs',
// Convert the preference string to a boolean presentation.
'default' => self::isSkinVersionLegacy() ? '1' : '0',
'default' => self::isSkinVersionLegacy(),
// 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 ]
'hide-if' => [ '!==', 'wpskin', Constants::SKIN_NAME ],
],
Constants::PREF_KEY_SIDEBAR_VISIBLE => [
'type' => 'api',
@ -211,23 +211,15 @@ class Hooks {
&$result,
$oldPreferences
) {
$preference = null;
$isVectorEnabled = ( $formData[ 'skin' ] ?? '' ) === Constants::SKIN_NAME;
if ( $isVectorEnabled && array_key_exists( Constants::PREF_KEY_SKIN_VERSION, $formData ) ) {
// A preference was set. However, Special:Preferences converts the result to a boolean when a
// version name string is wanted instead. Convert the boolean to a version string in case the
// preference display is changed to a list later (e.g., a "_new_ new Vector" / '3' or
// 'alpha').
$preference = $formData[ Constants::PREF_KEY_SKIN_VERSION ] ?
Constants::SKIN_VERSION_LEGACY :
Constants::SKIN_VERSION_LATEST;
} elseif ( array_key_exists( Constants::PREF_KEY_SKIN_VERSION, $oldPreferences ) ) {
if ( !$isVectorEnabled && array_key_exists( Constants::PREF_KEY_SKIN_VERSION, $oldPreferences ) ) {
// The setting was cleared. However, this is likely because a different skin was chosen and
// the skin version preference was hidden.
$preference = $oldPreferences[ Constants::PREF_KEY_SKIN_VERSION ];
}
if ( $preference !== null ) {
$user->setOption( Constants::PREF_KEY_SKIN_VERSION, $preference );
$user->setOption(
Constants::PREF_KEY_SKIN_VERSION,
$oldPreferences[ Constants::PREF_KEY_SKIN_VERSION ]
);
}
}

107
tests/phpunit/integration/HTMLForm/Fields/HTMLLegacySkinVersionFieldTest.php Normal file
View File

@ -0,0 +1,107 @@
<?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
*/
namespace MediaWiki\Skins\Vector\Tests\Integration;
use Vector\Constants;
use Vector\HTMLForm\Fields\HTMLLegacySkinVersionField;
/**
* @group Vector
* @coversDefaultClass \Vector\HTMLForm\Fields\HTMLLegacySkinVersionField
*/
class HTMLLegacySkinVersionFieldTest extends \MediaWikiTestCase {
public function provideDefault() {
yield [ 'true' ];
yield [ 1 ];
}
/**
* @dataProvider provideDefault
* @covers ::__construct
*/
public function testConstructValidatesDefault( $default ) {
$this->expectException( \Wikimedia\Assert\PreconditionException::class );
new HTMLLegacySkinVersionField( [
'default' => $default
] );
}
public function provideGetInput() {
yield [ Constants::SKIN_VERSION_LEGACY, true ];
yield [ Constants::SKIN_VERSION_LATEST, false ];
}
/**
* @dataProvider provideGetInput
* @covers ::getInputHTML
* @covers ::getInputOOUI
*/
public function testGetInput( $skinVersionValue, $checkValue ) {
$params = [
'fieldname' => 'VectorSkinVersion',
'class' => HTMLLegacySkinVersionField::class,
'section' => 'rendering/skin/skin-prefs',
'label-message' => 'prefs-vector-enable-vector-1-label',
'help-message' => 'prefs-vector-enable-vector-1-help',
'default' => true,
'hide-if' => [ '!==', 'wpskin', Constants::SKIN_NAME ],
];
$skinVersionField = new HTMLLegacySkinVersionField( $params );
$checkField = new \HTMLCheckField( $params );
$this->assertSame(
$skinVersionField->getInputHTML( $skinVersionValue ),
$checkField->getInputHTML( $checkValue ),
'::getInputHTML matches HTMLCheckField::getInputHTML with mapped value'
);
$this->assertEquals(
$skinVersionField->getInputOOUI( $skinVersionValue ),
$checkField->getInputOOUI( $checkValue ),
'::getInputOOUI matches HTMLCheckField::getInputOOUI with mapped value'
);
}
public function provideLoadDataFromRequest() {
yield [ null, Constants::SKIN_VERSION_LEGACY ];
yield [ true, Constants::SKIN_VERSION_LEGACY ];
yield [ false, Constants::SKIN_VERSION_LATEST ];
}
/**
* @dataProvider provideLoadDataFromRequest
* @covers ::loadDataFromRequest
*/
public function testLoadDataFromRequest( $wpVectorSkinVersion, $expectedResult ) {
$skinVerionField = new HTMLLegacySkinVersionField( [
'fieldname' => 'VectorSkinVersion',
'default' => true,
] );
$request = new \WebRequest();
$request->setVal( 'wpVectorSkinVersion', $wpVectorSkinVersion );
$this->assertSame( $skinVerionField->loadDataFromRequest( $request ), $expectedResult );
}
}

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

@ -7,6 +7,7 @@
use Vector\Constants;
use Vector\FeatureManagement\FeatureManager;
use Vector\Hooks;
use Vector\HTMLForm\Fields\HTMLLegacySkinVersionField;
const SKIN_PREFS_SECTION = 'rendering/skin/skin-prefs';
@ -45,7 +46,8 @@ class VectorHooksTest extends \MediaWikiTestCase {
* @covers ::onGetPreferences
*/
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionFoundLegacy() {
$this->setFeatureLatestSkinVersionIsEnabled( false );
$isLegacy = true;
$this->setFeatureLatestSkinVersionIsEnabled( !$isLegacy );
$prefs = [
'foo' => [],
@ -53,25 +55,24 @@ class VectorHooksTest extends \MediaWikiTestCase {
'bar' => []
];
Hooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs );
$this->assertSame(
$this->assertEquals(
$prefs,
[
'foo' => [],
'skin' => [],
'VectorSkinVersion' => [
'type' => 'toggle',
'class' => HTMLLegacySkinVersionField::class,
'label-message' => 'prefs-vector-enable-vector-1-label',
'help-message' => 'prefs-vector-enable-vector-1-help',
'section' => SKIN_PREFS_SECTION,
// '1' is enabled which means Legacy.
'default' => '1',
'hide-if' => [ '!==', 'wpskin', 'vector' ]
'default' => $isLegacy,
'hide-if' => [ '!==', 'wpskin', Constants::SKIN_NAME ]
],
'VectorSidebarVisible' => [
'type' => 'api',
'default' => true
],
'bar' => []
'bar' => [],
],
'Preferences are inserted directly after skin.'
);
@ -81,26 +82,26 @@ class VectorHooksTest extends \MediaWikiTestCase {
* @covers ::onGetPreferences
*/
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionMissingLegacy() {
$this->setFeatureLatestSkinVersionIsEnabled( false );
$isLegacy = false;
$this->setFeatureLatestSkinVersionIsEnabled( !$isLegacy );
$prefs = [
'foo' => [],
'bar' => []
];
Hooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs );
$this->assertSame(
$this->assertEquals(
$prefs,
[
'foo' => [],
'bar' => [],
'VectorSkinVersion' => [
'type' => 'toggle',
'class' => HTMLLegacySkinVersionField::class,
'label-message' => 'prefs-vector-enable-vector-1-label',
'help-message' => 'prefs-vector-enable-vector-1-help',
'section' => SKIN_PREFS_SECTION,
// '1' is enabled which means Legacy.
'default' => '1',
'hide-if' => [ '!==', 'wpskin', 'vector' ]
'default' => $isLegacy,
'hide-if' => [ '!==', 'wpskin', Constants::SKIN_NAME ]
],
'VectorSidebarVisible' => [
'type' => 'api',
@ -111,88 +112,13 @@ class VectorHooksTest extends \MediaWikiTestCase {
);
}
/**
* @covers ::onGetPreferences
*/
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionMissingLatest() {
$this->setFeatureLatestSkinVersionIsEnabled( true );
$prefs = [
'foo' => [],
'bar' => [],
];
Hooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs );
$this->assertSame(
$prefs,
[
'foo' => [],
'bar' => [],
'VectorSkinVersion' => [
'type' => 'toggle',
'label-message' => 'prefs-vector-enable-vector-1-label',
'help-message' => 'prefs-vector-enable-vector-1-help',
'section' => SKIN_PREFS_SECTION,
// '0' is disabled (which means latest).
'default' => '0',
'hide-if' => [ '!==', 'wpskin', 'vector' ]
],
'VectorSidebarVisible' => [
'type' => 'api',
'default' => true
],
],
'Legacy skin version is disabled.'
);
}
/**
* @covers ::onPreferencesFormPreSave
*/
public function testOnPreferencesFormPreSaveVectorEnabledLegacyNewPreference() {
$formData = [
'skin' => 'vector',
// True is Legacy.
'VectorSkinVersion' => true,
];
$form = $this->createMock( HTMLForm::class );
$user = $this->createMock( \User::class );
$user->expects( $this->once() )
->method( 'setOption' )
// '1' is Legacy.
->with( 'VectorSkinVersion', '1' );
$result = true;
$oldPreferences = [];
Hooks::onPreferencesFormPreSave( $formData, $form, $user, $result, $oldPreferences );
}
/**
* @covers ::onPreferencesFormPreSave
*/
public function testOnPreferencesFormPreSaveVectorEnabledLatestNewPreference() {
$formData = [
'skin' => 'vector',
// False is latest.
'VectorSkinVersion' => false,
];
$form = $this->createMock( HTMLForm::class );
$user = $this->createMock( \User::class );
$user->expects( $this->once() )
->method( 'setOption' )
// '2' is latest.
->with( 'VectorSkinVersion', '2' );
$result = true;
$oldPreferences = [];
Hooks::onPreferencesFormPreSave( $formData, $form, $user, $result, $oldPreferences );
}
/**
* @covers ::onPreferencesFormPreSave
*/
public function testOnPreferencesFormPreSaveVectorEnabledNoNewPreference() {
$formData = [
'skin' => 'vector',
'VectorSkinVersion' => Constants::SKIN_VERSION_LEGACY,
];
$form = $this->createMock( HTMLForm::class );
$user = $this->createMock( \User::class );
@ -209,8 +135,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
*/
public function testOnPreferencesFormPreSaveVectorDisabledNoOldPreference() {
$formData = [
// False is latest.
'VectorSkinVersion' => false,
'VectorSkinVersion' => Constants::SKIN_VERSION_LATEST,
];
$form = $this->createMock( HTMLForm::class );
$user = $this->createMock( \User::class );
@ -227,8 +152,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
*/
public function testOnPreferencesFormPreSaveVectorDisabledOldPreference() {
$formData = [
// False is latest.
'VectorSkinVersion' => false,
'VectorSkinVersion' => Constants::SKIN_VERSION_LATEST,
];
$form = $this->createMock( HTMLForm::class );
$user = $this->createMock( \User::class );
@ -248,16 +172,14 @@ class VectorHooksTest extends \MediaWikiTestCase {
*/
public function testOnLocalUserCreatedLegacy() {
$config = new HashConfig( [
// '1' is Legacy.
'VectorDefaultSkinVersionForNewAccounts' => '1',
'VectorDefaultSkinVersionForNewAccounts' => Constants::SKIN_VERSION_LEGACY,
] );
$this->setService( 'Vector.Config', $config );
$user = $this->createMock( \User::class );
$user->expects( $this->once() )
->method( 'setOption' )
// '1' is Legacy.
->with( 'VectorSkinVersion', '1' );
->method( 'setOption' )
->with( 'VectorSkinVersion', Constants::SKIN_VERSION_LEGACY );
$isAutoCreated = false;
Hooks::onLocalUserCreated( $user, $isAutoCreated );
}
@ -267,16 +189,14 @@ class VectorHooksTest extends \MediaWikiTestCase {
*/
public function testOnLocalUserCreatedLatest() {
$config = new HashConfig( [
// '2' is latest.
'VectorDefaultSkinVersionForNewAccounts' => '2',
'VectorDefaultSkinVersionForNewAccounts' => Constants::SKIN_VERSION_LATEST,
] );
$this->setService( 'Vector.Config', $config );
$user = $this->createMock( \User::class );
$user->expects( $this->once() )
->method( 'setOption' )
// '2' is latest.
->with( 'VectorSkinVersion', '2' );
->method( 'setOption' )
->with( 'VectorSkinVersion', Constants::SKIN_VERSION_LATEST );
$isAutoCreated = false;
Hooks::onLocalUserCreated( $user, $isAutoCreated );
}