Merge "Fix contradictory RC filters and add back-compat"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Thu, 4 May 2017 18:16:36 +0000 (18:16 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Thu, 4 May 2017 18:16:36 +0000 (18:16 +0000)
includes/specialpage/ChangesListSpecialPage.php
includes/specials/SpecialRecentchanges.php
tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php
tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php
tests/phpunit/includes/specials/SpecialRecentchangesTest.php

index 4d02ddc..268f0b0 100644 (file)
@@ -960,7 +960,87 @@ abstract class ChangesListSpecialPage extends SpecialPage {
         * @param FormOptions $opts
         */
        public function validateOptions( FormOptions $opts ) {
-               // nothing by default
+               if ( $this->fixContradictoryOptions( $opts ) ) {
+                       $query = wfArrayToCgi( $this->convertParamsForLink( $opts->getChangedValues() ) );
+                       $this->getOutput()->redirect( $this->getPageTitle()->getCanonicalURL( $query ) );
+               }
+       }
+
+       /**
+        * Fix invalid options by resetting pairs that should never appear together.
+        *
+        * @param FormOptions $opts
+        * @return bool True if any option was reset
+        */
+       private function fixContradictoryOptions( FormOptions $opts ) {
+               $contradictorySets = [];
+
+               $fixed = $this->fixBackwardsCompatibilityOptions( $opts );
+
+               foreach ( $this->filterGroups as $filterGroup ) {
+                       if ( $filterGroup instanceof ChangesListBooleanFilterGroup ) {
+                               $filters = $filterGroup->getFilters();
+                               $allInGroupEnabled = array_reduce(
+                                       $filters,
+                                       function ( $carry, $filter ) use ( $opts ) {
+                                               return $carry && $opts[ $filter->getName() ];
+                                       },
+                                       /* initialValue */ count( $filters ) > 0
+                               );
+
+                               if ( $allInGroupEnabled ) {
+                                       foreach ( $filters as $filter ) {
+                                               $opts->reset( $filter->getName() );
+                                       }
+
+                                       $fixed = true;
+                               }
+                       }
+               }
+
+               return $fixed;
+       }
+
+       /**
+        * Fix a special case (hideanons=1 and hideliu=1) in a special way, for backwards
+        * compatibility.
+        *
+        * This is deprecated and may be removed.
+        *
+        * @param FormOptions $opts
+        * @return bool True if this change was mode
+        */
+       private function fixBackwardsCompatibilityOptions( FormOptions $opts ) {
+               if ( $opts['hideanons'] && $opts['hideliu'] ) {
+                       $opts->reset( 'hideanons' );
+                       if ( !$opts['hidebots'] ) {
+                               $opts->reset( 'hideliu' );
+                               $opts['hidehumans'] = 1;
+                       }
+
+                       return true;
+               }
+
+               return false;
+
+       }
+
+       /**
+        * Convert parameters values from true/false to 1/0
+        * so they are not omitted by wfArrayToCgi()
+        * Bug 36524
+        *
+        * @param array $params
+        * @return array
+        */
+       protected function convertParamsForLink( $params ) {
+               foreach ( $params as &$value ) {
+                       if ( $value === false ) {
+                               $value = '0';
+                       }
+               }
+               unset( $value );
+               return $params;
        }
 
        /**
@@ -1049,15 +1129,6 @@ abstract class ChangesListSpecialPage extends SpecialPage {
                        ''
                );
 
-               // It makes no sense to hide both anons and logged-in users. When this occurs, try a guess on
-               // what the user meant and either show only bots or force anons to be shown.
-
-               // -------
-
-               // XXX: We're no longer doing this handling.  To preserve back-compat, we need to complete
-               // T151873 (particularly the hideanons/hideliu/hidebots/hidehumans part) in conjunction
-               // with merging this.
-
                if ( !$this->runMainQueryHook( $tables, $fields, $conds, $query_options, $join_conds,
                        $opts )
                ) {
index a47d91b..f9164a1 100644 (file)
@@ -814,16 +814,7 @@ class SpecialRecentChanges extends ChangesListSpecialPage {
         * @return string
         */
        function makeOptionsLink( $title, $override, $options, $active = false ) {
-               $params = $override + $options;
-
-               // T38524: false values have be converted to "0" otherwise
-               // wfArrayToCgi() will omit it them.
-               foreach ( $params as &$value ) {
-                       if ( $value === false ) {
-                               $value = '0';
-                       }
-               }
-               unset( $value );
+               $params = $this->convertParamsForLink( $override + $options );
 
                if ( $active ) {
                        $title = new HtmlArmor( '<strong>' . htmlspecialchars( $title ) . '</strong>' );
index 03e341a..b101857 100644 (file)
@@ -28,6 +28,20 @@ abstract class AbstractChangesListSpecialPageTestCase extends MediaWikiTestCase
                $wgGroupPermissions['patrollers'] = [
                        'patrol' => true,
                ];
+
+               // Deprecated
+               $this->setTemporaryHook(
+                       'ChangesListSpecialPageFilters',
+                       null
+               );
+
+               # setup the ChangesListSpecialPage (or subclass) object
+               $this->changesListSpecialPage = $this->getPage();
+               $context = $this->changesListSpecialPage->getContext();
+               $context = new DerivativeContext( $context );
+               $context->setUser( $this->getTestUser( [ 'patrollers' ] )->getUser() );
+               $this->changesListSpecialPage->setContext( $context );
+               $this->changesListSpecialPage->registerFilters();
        }
 
        protected function tearDown() {
@@ -44,13 +58,6 @@ abstract class AbstractChangesListSpecialPageTestCase extends MediaWikiTestCase
         * @dataProvider provideParseParameters
         */
        public function testParseParameters( $params, $expected ) {
-               $context = $this->changesListSpecialPage->getContext();
-               $context = new DerivativeContext( $context );
-               $context->setUser( $this->getTestUser( [ 'patrollers' ] )->getUser() );
-               $this->changesListSpecialPage->setContext( $context );
-
-               $this->changesListSpecialPage->registerFilters();
-
                $opts = new FormOptions();
                foreach ( $expected as $key => $value ) {
                        // Register it as null so sets aren't rejected.
@@ -73,4 +80,65 @@ abstract class AbstractChangesListSpecialPageTestCase extends MediaWikiTestCase
                        /** named= */ true
                );
        }
+
+       /**
+        * @dataProvider validateOptionsProvider
+        */
+       public function testValidateOptions( $optionsToSet, $expectedRedirect, $expectedRedirectOptions ) {
+               $redirectQuery = [];
+               $redirected = false;
+               $output = $this->getMockBuilder( OutputPage::class )
+                       ->disableProxyingToOriginalMethods()
+                       ->disableOriginalConstructor()
+                       ->getMock();
+               $output->method( 'redirect' )->willReturnCallback(
+                       function ( $url ) use ( &$redirectQuery, &$redirected ) {
+                               $urlParts = wfParseUrl( $url );
+                               $query = isset( $urlParts[ 'query' ] ) ? $urlParts[ 'query' ] : '';
+                               parse_str( $query, $redirectQuery );
+                               $redirected = true;
+                       }
+               );
+               $ctx = new RequestContext();
+
+               // Give users patrol permissions so we can test that.
+               $user = $this->getTestSysop()->getUser();
+               $ctx->setUser( $user );
+
+               // Disable this hook or it could break changeType
+               // depending on which other extensions are running.
+               $this->setTemporaryHook(
+                       'ChangesListSpecialPageStructuredFilters',
+                       null
+               );
+
+               $ctx->setOutput( $output );
+               $clsp = $this->changesListSpecialPage;
+               $clsp->setContext( $ctx );
+               $opts = $clsp->getDefaultOptions();
+
+               foreach ( $optionsToSet as $option => $value ) {
+                       $opts->setValue( $option, $value );
+               }
+
+               $clsp->validateOptions( $opts );
+
+               $this->assertEquals( $expectedRedirect, $redirected, 'redirection' );
+
+               if ( $expectedRedirect ) {
+                       if ( count( $expectedRedirectOptions ) > 0 ) {
+                               $expectedRedirectOptions += [
+                                       'title' => $clsp->getPageTitle()->getPrefixedText(),
+                               ];
+                       }
+
+                       $this->assertArrayEquals(
+                               $expectedRedirectOptions,
+                               $redirectQuery,
+                               /* $ordered= */ false,
+                               /* $named= */ true,
+                               'redirection query'
+                       );
+               }
+       }
 }
index 6028573..c8c65dc 100644 (file)
@@ -15,23 +15,26 @@ use Wikimedia\TestingAccessWrapper;
  * @covers ChangesListSpecialPage
  */
 class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase {
-       protected function setUp() {
-               parent::setUp();
-
-               # setup the rc object
-               $this->changesListSpecialPage = $this->getPage();
-       }
-
        protected function getPage() {
-               return TestingAccessWrapper::newFromObject(
-                       $this->getMockForAbstractClass(
-                               'ChangesListSpecialPage',
+               $mock = $this->getMockBuilder( ChangesListSpecialPage::class )
+                       ->setConstructorArgs(
                                [
                                        'ChangesListSpecialPage',
                                        ''
                                ]
                        )
+                       ->setMethods( [ 'getPageTitle' ] )
+                       ->getMockForAbstractClass();
+
+               $mock->method( 'getPageTitle' )->willReturn(
+                       Title::makeTitle( NS_SPECIAL, 'ChangesListSpecialPage' )
                );
+
+               $mock = TestingAccessWrapper::newFromObject(
+                       $mock
+               );
+
+               return $mock;
        }
 
        /** helper to test SpecialRecentchanges::buildMainQueryConds() */
@@ -48,6 +51,7 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
                }
 
                $this->changesListSpecialPage->setContext( $context );
+               $this->changesListSpecialPage->filterGroups = [];
                $formOptions = $this->changesListSpecialPage->setup( null );
 
                #  Filter out rc_timestamp conditions which depends on the test runtime
@@ -230,22 +234,6 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
                );
        }
 
-       public function testRcHidemyselfHidebyothersFilter() {
-               $user = $this->getTestUser()->getUser();
-               $this->assertConditions(
-                       [ # expected
-                               "rc_user_text != '{$user->getName()}'",
-                               "rc_user_text = '{$user->getName()}'",
-                       ],
-                       [
-                               'hidemyself' => 1,
-                               'hidebyothers' => 1,
-                       ],
-                       "rc conditions: hidemyself=1 hidebyothers=1 (logged in)",
-                       $user
-               );
-       }
-
        public function testRcHidepageedits() {
                $this->assertConditions(
                        [ # expected
@@ -372,22 +360,6 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
                );
        }
 
-       public function testRcHidepatrolledHideunpatrolledFilter() {
-               $user = $this->getTestSysop()->getUser();
-               $this->assertConditions(
-                       [ # expected
-                               "rc_patrolled = 0",
-                               "rc_patrolled = 1",
-                       ],
-                       [
-                               'hidepatrolled' => 1,
-                               'hideunpatrolled' => 1,
-                       ],
-                       "rc conditions: hidepatrolled=1 hideunpatrolled=1",
-                       $user
-               );
-       }
-
        public function testHideCategorization() {
                $this->assertConditions(
                        [
@@ -576,6 +548,8 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
        }
 
        public function testGetStructuredFilterJsData() {
+               $this->changesListSpecialPage->filterGroups = [];
+
                $definition = [
                        [
                                'name' => 'gub-group',
@@ -893,4 +867,47 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
                        $this->changesListSpecialPage->areFiltersInConflict()
                );
        }
+
+       public function validateOptionsProvider() {
+               return [
+                       [
+                               [ 'hideanons' => 1, 'hideliu' => 1, 'hidebots' => 1 ],
+                               true,
+                               [ 'hideliu' => 1, 'hidebots' => 1, ],
+                       ],
+
+                       [
+                               [ 'hideanons' => 1, 'hideliu' => 1, 'hidebots' => 0 ],
+                               true,
+                               [ 'hidebots' => 0, 'hidehumans' => 1 ],
+                       ],
+
+                       [
+                               [ 'hidemyself' => 1, 'hidebyothers' => 1 ],
+                               true,
+                               [],
+                       ],
+                       [
+                               [ 'hidebots' => 1, 'hidehumans' => 1 ],
+                               true,
+                               [],
+                       ],
+                       [
+                               [ 'hidepatrolled' => 1, 'hideunpatrolled' => 1 ],
+                               true,
+                               [],
+                       ],
+                       [
+                               [ 'hideminor' => 1, 'hidemajor' => 1 ],
+                               true,
+                               [],
+                       ],
+                       [
+                               // changeType
+                               [ 'hidepageedits' => 1, 'hidenewpages' => 1, 'hidecategorization' => 1, 'hidelog' => 1, ],
+                               true,
+                               [],
+                       ],
+               ];
+       }
 }
index e9c7d4b..8f3f585 100644 (file)
@@ -10,15 +10,15 @@ use Wikimedia\TestingAccessWrapper;
  * @covers SpecialRecentChanges
  */
 class SpecialRecentchangesTest extends AbstractChangesListSpecialPageTestCase {
-       protected function setUp() {
-               parent::setUp();
-
-               # setup the CLSP object
-               $this->changesListSpecialPage = TestingAccessWrapper::newFromObject(
+       protected function getPage() {
+               return TestingAccessWrapper::newFromObject(
                        new SpecialRecentchanges
                );
        }
 
+       // Below providers should only be for features specific to
+       // RecentChanges.  Otherwise, it should go in ChangesListSpecialPageTest
+
        public function provideParseParameters() {
                return [
                        [ 'limit=123', [ 'limit' => '123' ] ],
@@ -32,4 +32,15 @@ class SpecialRecentchangesTest extends AbstractChangesListSpecialPageTestCase {
                        [ 'tagfilter=foo', [ 'tagfilter' => 'foo' ] ],
                ];
        }
+
+       public function validateOptionsProvider() {
+               return [
+                       [
+                               // hidebots=1 is default for Special:RecentChanges
+                               [ 'hideanons' => 1, 'hideliu' => 1 ],
+                               true,
+                               [ 'hideliu' => 1 ],
+                       ],
+               ];
+       }
 }