From 338e0e72ac0a6086c67233ac08d54cb7321418d9 Mon Sep 17 00:00:00 2001 From: Clare Ming Date: Fri, 18 Feb 2022 15:44:41 -0700 Subject: [PATCH] Fix language alert regression - Add test for SkinVector::shouldLanguageAlertBeInSidebar() method. - Change some methods to protected status -- acknowledging this is not an optimal change. Until we can pull out some of the language checks into its own class with injected dependencies (title, feature manager, languages), this is an interim solution to try to catch regressions since the conditional states for showing the alert are complex and ultimately temporary. Extracting the language checks into their own class can be done in a follow up ticket to help optimize testing language checks in isolation. Bug: T302018 Change-Id: I99b7df3029e0af86e4d67b3f446d4ce99260d33e --- includes/SkinVector.php | 8 +- tests/phpunit/integration/SkinVectorTest.php | 187 +++++++++++++++++++ 2 files changed, 191 insertions(+), 4 deletions(-) diff --git a/includes/SkinVector.php b/includes/SkinVector.php index 0e28c051..e2e0d648 100644 --- a/includes/SkinVector.php +++ b/includes/SkinVector.php @@ -186,7 +186,7 @@ class SkinVector extends SkinMustache { * Calls getLanguages with caching. * @return array */ - private function getLanguagesCached(): array { + protected function getLanguagesCached(): array { if ( $this->languages === null ) { $this->languages = $this->getLanguages(); } @@ -216,7 +216,7 @@ class SkinVector extends SkinMustache { * @param string $location Either 'top' or 'bottom' is accepted. * @return bool */ - private function isLanguagesInContentAt( $location ) { + protected function isLanguagesInContentAt( $location ) { if ( !$this->canHaveLanguages() ) { return false; } @@ -254,7 +254,7 @@ class SkinVector extends SkinMustache { * and the language array isn't empty. Hide it otherwise. * @return bool */ - private function shouldHideLanguages() { + protected function shouldHideLanguages() { // NOTE: T276950 - This should be revisited when an empty state for the language button is chosen. return $this->isLegacy() || !$this->isLanguagesInContent() || empty( $this->getLanguagesCached() ); } @@ -542,7 +542,7 @@ class SkinVector extends SkinMustache { $isMainPage = $this->getTitle() ? $this->getTitle()->isMainPage() : false; $shouldShowOnMainPage = $isMainPage && !empty( $this->getLanguagesCached() ) && $featureManager->isFeatureEnabled( Constants::FEATURE_LANGUAGE_IN_MAIN_PAGE_HEADER ); - return ( $this->isLanguagesInContentAt( 'top' ) && !$isMainPage && + return ( $this->isLanguagesInContentAt( 'top' ) && !$isMainPage && !$this->shouldHideLanguages() && $featureManager->isFeatureEnabled( Constants::FEATURE_LANGUAGE_ALERT_IN_SIDEBAR ) ) || $shouldShowOnMainPage; } diff --git a/tests/phpunit/integration/SkinVectorTest.php b/tests/phpunit/integration/SkinVectorTest.php index 9f4b0415..f047481b 100644 --- a/tests/phpunit/integration/SkinVectorTest.php +++ b/tests/phpunit/integration/SkinVectorTest.php @@ -1,7 +1,10 @@ [ + 'logged_in' => true, + 'logged_out' => true + ], + 'VectorLanguageInMainPageHeader' => [ + 'logged_in' => false, + 'logged_out' => false + ], + 'VectorLanguageAlertInSidebar' => [ + 'logged_in' => true, + 'logged_out' => true + ], + ]; + } + + public function providerLanguageAlertRequirements() { + $testTitle = Title::makeTitle( NS_MAIN, 'Test' ); + $testTitleMainPage = Title::makeTitle( NS_MAIN, 'MAIN_PAGE' ); + return [ + 'When none of the requirements are present, do not show alert' => [ + // Configuration requirements for language in header and alert in sidebar + [], + // Title instance + $testTitle, + // Cached languages + [], + // Is the language selector at the top of the content? + false, + // Should the language button be hidden? + false, + // Expected + false + ], + 'When the feature is enabled and languages should be hidden, do not show alert' => [ + $this->enableLanguageAlertFeatureConfig(), + $testTitle, + [], true, true, false + ], + 'When the language alert feature is disabled, do not show alert' => [ + [ + 'VectorLanguageInHeader' => [ + 'logged_in' => true, + 'logged_out' => true + ], + 'VectorLanguageAlertInSidebar' => [ + 'logged_in' => false, + 'logged_out' => false + ] + ], + $testTitle, + [ 'fr', 'en', 'ko' ], true, false, false + ], + 'When the language in header feature is disabled, do not show alert' => [ + [ + 'VectorLanguageInHeader' => [ + 'logged_in' => false, + 'logged_out' => false + ], + 'VectorLanguageAlertInSidebar' => [ + 'logged_in' => true, + 'logged_out' => true + ] + ], + $testTitle, + [ 'fr', 'en', 'ko' ], true, false, false + ], + 'When it is a main page, feature is enabled, and there are no languages, do not show alert' => [ + $this->enableLanguageAlertFeatureConfig(), + $testTitleMainPage, + [], true, true, false + ], + 'When it is a non-main page, feature is enabled, and there are no languages, do not show alert' => [ + $this->enableLanguageAlertFeatureConfig(), + $testTitle, + [], true, true, false + ], + 'When it is a main page, header feature is disabled, and there are languages, do not show alert' => [ + [ + 'VectorLanguageInHeader' => [ + 'logged_in' => false, + 'logged_out' => false + ], + 'VectorLanguageAlertInSidebar' => [ + 'logged_in' => true, + 'logged_out' => true + ] + ], + $testTitleMainPage, + [ 'fr', 'en', 'ko' ], true, true, false + ], + 'When it is a non-main page, alert feature is disabled, there are languages, do not show alert' => [ + [ + 'VectorLanguageInHeader' => [ + 'logged_in' => true, + 'logged_out' => true + ], + 'VectorLanguageAlertInSidebar' => [ + 'logged_in' => false, + 'logged_out' => false + ] + ], + $testTitle, + [ 'fr', 'en', 'ko' ], true, true, false + ], + 'When most requirements are present but languages are not at the top, do not show alert' => [ + $this->enableLanguageAlertFeatureConfig(), + $testTitle, + [ 'fr', 'en', 'ko' ], false, false, false + ], + 'When most requirements are present but languages should be hidden, do not show alert' => [ + $this->enableLanguageAlertFeatureConfig(), + $testTitle, + [ 'fr', 'en', 'ko' ], true, true, false + ], + 'When it is a main page, features are enabled, and there are languages, show alert' => [ + $this->enableLanguageAlertFeatureConfig(), + $testTitleMainPage, + [ 'fr', 'en', 'ko' ], true, false, true + ], + 'When all the requirements are present on a non-main page, show alert' => [ + $this->enableLanguageAlertFeatureConfig(), + $testTitle, + [ 'fr', 'en', 'ko' ], true, false, true + ], + ]; + } + + /** + * @dataProvider providerLanguageAlertRequirements + * @covers ::shouldLanguageAlertBeInSidebar + * @param array $requirements + * @param Title $title + * @param array $getLanguagesCached + * @param bool $isLanguagesInContentAt + * @param bool $shouldHideLanguages + * @param bool $expected + * @throws Exception + */ + public function testShouldLanguageAlertBeInSidebar( + array $requirements, + Title $title, + array $getLanguagesCached, + bool $isLanguagesInContentAt, + bool $shouldHideLanguages, + bool $expected + ) { + $config = new HashConfig( array_merge( $requirements, [ + 'DefaultSkin' => 'vector-2022', + 'VectorDefaultSkinVersion' => '2', + 'VectorSkinMigrationMode' => true, + ] ) ); + $this->installMockMwServices( $config ); + + $mockSkinVector = $this->getMockBuilder( SkinVector::class ) + ->disableOriginalConstructor() + ->onlyMethods( [ 'getTitle', 'getLanguagesCached','isLanguagesInContentAt', 'shouldHideLanguages' ] ) + ->getMock(); + $mockSkinVector->method( 'getTitle' ) + ->willReturn( $title ); + $mockSkinVector->method( 'getLanguagesCached' ) + ->willReturn( $getLanguagesCached ); + $mockSkinVector->method( 'isLanguagesInContentAt' )->with( 'top' ) + ->willReturn( $isLanguagesInContentAt ); + $mockSkinVector->method( 'shouldHideLanguages' ) + ->willReturn( $shouldHideLanguages ); + + $shouldLanguageAlertBeInSidebarMethod = new ReflectionMethod( + SkinVector::class, + 'shouldLanguageAlertBeInSidebar' + ); + $shouldLanguageAlertBeInSidebarMethod->setAccessible( true ); + + $this->assertSame( + $shouldLanguageAlertBeInSidebarMethod->invoke( $mockSkinVector ), + $expected + ); + } + }