From 444923551655eaba9ff1162a229f5a6e75480db1 Mon Sep 17 00:00:00 2001 From: Sam Smith Date: Tue, 11 Aug 2020 15:13:12 +0100 Subject: [PATCH] [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 --- .../Fields/HTMLLegacySkinVersionField.php | 92 +++++++++++++ includes/Hooks.php | 28 ++-- .../Fields/HTMLLegacySkinVersionFieldTest.php | 107 +++++++++++++++ tests/phpunit/integration/VectorHooksTest.php | 126 ++++-------------- 4 files changed, 232 insertions(+), 121 deletions(-) create mode 100644 includes/HTMLForm/Fields/HTMLLegacySkinVersionField.php create mode 100644 tests/phpunit/integration/HTMLForm/Fields/HTMLLegacySkinVersionFieldTest.php diff --git a/includes/HTMLForm/Fields/HTMLLegacySkinVersionField.php b/includes/HTMLForm/Fields/HTMLLegacySkinVersionField.php new file mode 100644 index 000000000..0acd7aa14 --- /dev/null +++ b/includes/HTMLForm/Fields/HTMLLegacySkinVersionField.php @@ -0,0 +1,92 @@ +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; + } +} diff --git a/includes/Hooks.php b/includes/Hooks.php index 10dde4eac..b4cc915cc 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -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 ] + ); } } diff --git a/tests/phpunit/integration/HTMLForm/Fields/HTMLLegacySkinVersionFieldTest.php b/tests/phpunit/integration/HTMLForm/Fields/HTMLLegacySkinVersionFieldTest.php new file mode 100644 index 000000000..5bb6282e6 --- /dev/null +++ b/tests/phpunit/integration/HTMLForm/Fields/HTMLLegacySkinVersionFieldTest.php @@ -0,0 +1,107 @@ +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 ); + } +} diff --git a/tests/phpunit/integration/VectorHooksTest.php b/tests/phpunit/integration/VectorHooksTest.php index 9280e2ec5..9dedbb299 100644 --- a/tests/phpunit/integration/VectorHooksTest.php +++ b/tests/phpunit/integration/VectorHooksTest.php @@ -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 ); }