From 32be7421b1e0b5a7941c941df0160e37931fb896 Mon Sep 17 00:00:00 2001 From: Florian Schmidt Date: Fri, 18 May 2018 22:08:09 +0200 Subject: [PATCH] WatchAction::onSubmit return correct value, not always true The onSubmit method documentation states, that the caller can expect either true for a successfull run, false if not tried and an array of error messages in case of an failure. WatchAction however always returned false, even though a Status object is availble with all needed information. The behaviour of WatchAction::onSubmit is now changed to return the appropriate value taken from the returned Status object of WatchAction::doSelf. Also: * Added WatchAction test class to higher test coverage, especially for the static methods * Marked getUnwatchToken as deprecated, it's not used and a caller can easily switch to getWatchToken with "unwatch" as the action parameter Change-Id: I2c1b91e1884a0d5f27f5e7ab9eafd6173642c21c --- RELEASE-NOTES-1.32 | 2 + includes/actions/FormAction.php | 7 +- includes/actions/WatchAction.php | 6 +- .../includes/actions/WatchActionTest.php | 367 ++++++++++++++++++ 4 files changed, 378 insertions(+), 4 deletions(-) create mode 100644 tests/phpunit/includes/actions/WatchActionTest.php diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index fa7c962f2e..b474d15483 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -129,6 +129,8 @@ because of Phabricator reports. mediawiki.api.messages, and mediawiki.api.rollback. * ApiBase::truncateArray() is deprecated. No replacement, as nothing is known to use it. +* WatchAction::getUnwatchToken is deprecated. Use WatchAction::getWatchToken + with the 'unwatch' action parameter instead. === Other changes in 1.32 === * Soft hyphens (U+00AD) are now automatically removed from titles; these diff --git a/includes/actions/FormAction.php b/includes/actions/FormAction.php index 0141b9ec16..ec6391012c 100644 --- a/includes/actions/FormAction.php +++ b/includes/actions/FormAction.php @@ -109,8 +109,13 @@ abstract class FormAction extends Action { * * If you don't want to do anything with the form, just return false here. * + * This method will be passed to the HTMLForm as a submit callback (see + * HTMLForm::setSubmitCallback) and must return as documented for HTMLForm::trySubmit. + * + * @see HTMLForm::setSubmitCallback() + * @see HTMLForm::trySubmit() * @param array $data - * @return bool|array True for success, false for didn't-try, array of errors on failure + * @return bool|string|array|Status Must return as documented for HTMLForm::trySubmit */ abstract public function onSubmit( $data ); diff --git a/includes/actions/WatchAction.php b/includes/actions/WatchAction.php index 528e0e2ef1..aaccc0c489 100644 --- a/includes/actions/WatchAction.php +++ b/includes/actions/WatchAction.php @@ -40,9 +40,7 @@ class WatchAction extends FormAction { } public function onSubmit( $data ) { - self::doWatch( $this->getTitle(), $this->getUser() ); - - return true; + return self::doWatch( $this->getTitle(), $this->getUser() ); } protected function checkCanExecute( User $user ) { @@ -183,8 +181,10 @@ class WatchAction extends FormAction { * @param string $action Optionally override the action to 'watch' * @return string Token * @since 1.18 + * @deprecated since 1.32 Use WatchAction::getWatchToken() with action 'unwatch' directly. */ public static function getUnwatchToken( Title $title, User $user, $action = 'unwatch' ) { + wfDeprecated( __METHOD__, '1.32' ); return self::getWatchToken( $title, $user, $action ); } diff --git a/tests/phpunit/includes/actions/WatchActionTest.php b/tests/phpunit/includes/actions/WatchActionTest.php new file mode 100644 index 0000000000..044c30a667 --- /dev/null +++ b/tests/phpunit/includes/actions/WatchActionTest.php @@ -0,0 +1,367 @@ +testWikiPage = new WikiPage( $testTitle ); + $testContext = new DerivativeContext( RequestContext::getMain() ); + $testContext->setTitle( $testTitle ); + $this->watchAction = new WatchAction( $this->testWikiPage, $testContext ); + } + + /** + * @throws MWException + */ + protected function tearDown() { + parent::tearDown(); + + Hooks::clear( 'WatchArticle' ); + Hooks::clear( 'UnwatchArticle' ); + } + + /** + * @covers WatchAction::getName() + */ + public function testGetName() { + $this->assertEquals( 'watch', $this->watchAction->getName() ); + } + + /** + * @covers WatchAction::requiresUnblock() + */ + public function testRequiresUnlock() { + $this->assertFalse( $this->watchAction->requiresUnblock() ); + } + + /** + * @covers WatchAction::doesWrites() + */ + public function testDoesWrites() { + $this->assertTrue( $this->watchAction->doesWrites() ); + } + + /** + * @covers WatchAction::onSubmit() + * @covers WatchAction::doWatch() + */ + public function testOnSubmit() { + /** @var Status $actual */ + $actual = $this->watchAction->onSubmit( [] ); + + $this->assertTrue( $actual->isGood() ); + } + + /** + * @covers WatchAction::onSubmit() + * @covers WatchAction::doWatch() + */ + public function testOnSubmitHookAborted() { + Hooks::register( 'WatchArticle', function () { + return false; + } ); + + /** @var Status $actual */ + $actual = $this->watchAction->onSubmit( [] ); + + $this->assertInstanceOf( Status::class, $actual ); + $this->assertTrue( $actual->hasMessage( 'hookaborted' ) ); + } + + /** + * @covers WatchAction::checkCanExecute() + */ + public function testShowUserNotLoggedIn() { + $notLoggedInUser = new User(); + $testContext = new DerivativeContext( $this->watchAction->getContext() ); + $testContext->setUser( $notLoggedInUser ); + $watchAction = new WatchAction( $this->testWikiPage, $testContext ); + $this->setExpectedException( UserNotLoggedIn::class ); + + $watchAction->show(); + } + + /** + * @covers WatchAction::checkCanExecute() + */ + public function testShowUserLoggedInNoException() { + $loggedInUser = $this->getMock( User::class ); + $loggedInUser->method( 'isLoggedIn' )->willReturn( true ); + $testContext = new DerivativeContext( $this->watchAction->getContext() ); + $testContext->setUser( $loggedInUser ); + $watchAction = new WatchAction( $this->testWikiPage, $testContext ); + + $exception = null; + try { + $watchAction->show(); + } catch ( UserNotLoggedIn $e ) { + $exception = $e; + } + $this->assertNull( $exception, + 'UserNotLoggedIn exception should not be thrown if user is logged in.' ); + } + + /** + * @covers WatchAction::onSuccess() + */ + public function testOnSuccessMainNamespaceTitle() { + $testContext = $this->getMock( + DerivativeContext::class, + [ 'msg' ], + [ $this->watchAction->getContext() ] + ); + $testOutput = new OutputPage( $testContext ); + $testContext->setOutput( $testOutput ); + $testContext->method( 'msg' )->willReturnCallback( function ( $msgKey ) { + return new RawMessage( $msgKey ); + } ); + $watchAction = new WatchAction( $this->testWikiPage, $testContext ); + + $watchAction->onSuccess(); + + $this->assertEquals( '

addedwatchtext +

', $testOutput->getHTML() ); + } + + /** + * @covers WatchAction::onSuccess() + */ + public function testOnSuccessTalkPage() { + $testContext = $this->getMock( + DerivativeContext::class, + [], + [ $this->watchAction->getContext() ] + ); + $testOutput = new OutputPage( $testContext ); + $testContext->method( 'getOutput' )->willReturn( $testOutput ); + $testContext->method( 'msg' )->willReturnCallback( function ( $msgKey ) { + return new RawMessage( $msgKey ); + } ); + $talkPageTitle = Title::newFromText( 'Talk:UTTest' ); + $testContext->setTitle( $talkPageTitle ); + $watchAction = new WatchAction( new WikiPage( $talkPageTitle ), $testContext ); + + $watchAction->onSuccess(); + + $this->assertEquals( '

addedwatchtext-talk +

', $testOutput->getHTML() ); + } + + /** + * @covers WatchAction::doWatch() + */ + public function testDoWatchNoCheckRights() { + $notPermittedUser = $this->getMock( User::class ); + $notPermittedUser->method( 'isAllowed' )->willReturn( false ); + + $actual = WatchAction::doWatch( $this->testWikiPage->getTitle(), $notPermittedUser, false ); + + $this->assertTrue( $actual->isGood() ); + } + + /** + * @covers WatchAction::doWatch() + */ + public function testDoWatchUserNotPermittedStatusNotGood() { + $notPermittedUser = $this->getMock( User::class ); + $notPermittedUser->method( 'isAllowed' )->willReturn( false ); + + $actual = WatchAction::doWatch( $this->testWikiPage->getTitle(), $notPermittedUser, true ); + + $this->assertFalse( $actual->isGood() ); + } + + /** + * @covers WatchAction::doWatch() + */ + public function testDoWatchCallsUserAddWatch() { + $permittedUser = $this->getMock( User::class ); + $permittedUser->method( 'isAllowed' )->willReturn( true ); + $permittedUser->expects( $this->once() ) + ->method( 'addWatch' ) + ->with( $this->equalTo( $this->testWikiPage->getTitle() ), $this->equalTo( true ) ); + + $actual = WatchAction::doWatch( $this->testWikiPage->getTitle(), $permittedUser ); + + $this->assertTrue( $actual->isGood() ); + } + + /** + * @covers WatchAction::doUnWatch() + */ + public function testDoUnWatchWithoutRights() { + $notPermittedUser = $this->getMock( User::class ); + $notPermittedUser->method( 'isAllowed' )->willReturn( false ); + + $actual = WatchAction::doUnWatch( $this->testWikiPage->getTitle(), $notPermittedUser ); + + $this->assertFalse( $actual->isGood() ); + } + + /** + * @covers WatchAction::doUnWatch() + */ + public function testDoUnWatchUserHookAborted() { + $permittedUser = $this->getMock( User::class ); + $permittedUser->method( 'isAllowed' )->willReturn( true ); + Hooks::register( 'UnwatchArticle', function () { + return false; + } ); + + $status = WatchAction::doUnWatch( $this->testWikiPage->getTitle(), $permittedUser ); + + $this->assertFalse( $status->isGood() ); + $errors = $status->getErrors(); + $this->assertEquals( 1, count( $errors ) ); + $this->assertEquals( 'hookaborted', $errors[0]['message'] ); + } + + /** + * @covers WatchAction::doUnWatch() + */ + public function testDoUnWatchCallsUserRemoveWatch() { + $permittedUser = $this->getMock( User::class ); + $permittedUser->method( 'isAllowed' )->willReturn( true ); + $permittedUser->expects( $this->once() ) + ->method( 'removeWatch' ) + ->with( $this->equalTo( $this->testWikiPage->getTitle() ) ); + + $actual = WatchAction::doUnWatch( $this->testWikiPage->getTitle(), $permittedUser ); + + $this->assertTrue( $actual->isGood() ); + } + + /** + * @covers WatchAction::getWatchToken() + */ + public function testGetWatchTokenNormalizesToWatch() { + $user = $this->getMock( User::class ); + $user->expects( $this->once() ) + ->method( 'getEditToken' ) + ->with( $this->equalTo( 'watch' ) ); + + WatchAction::getWatchToken( $this->watchAction->getTitle(), $user, 'INVALID_ACTION' ); + } + + /** + * @covers WatchAction::getWatchToken() + */ + public function testGetWatchTokenProxiesUserGetEditToken() { + $user = $this->getMock( User::class ); + $user->expects( $this->once() )->method( 'getEditToken' ); + + WatchAction::getWatchToken( $this->watchAction->getTitle(), $user ); + } + + /** + * @covers WatchAction::getUnwatchToken() + */ + public function testGetUnwatchToken() { + $user = $this->getMock( User::class ); + $user->expects( $this->once() )->method( 'getEditToken' ); + $this->hideDeprecated( 'WatchAction::getUnwatchToken' ); + + WatchAction::getUnWatchToken( $this->watchAction->getTitle(), $user ); + } + + /** + * @covers WatchAction::doWatchOrUnwatch() + */ + public function testDoWatchOrUnwatchUserNotLoggedIn() { + $user = $this->getLoggedInIsWatchedUser( false ); + $user->expects( $this->never() )->method( 'removeWatch' ); + $user->expects( $this->never() )->method( 'addWatch' ); + + $status = WatchAction::doWatchOrUnwatch( true, $this->watchAction->getTitle(), $user ); + + $this->assertTrue( $status->isGood() ); + } + + /** + * @covers WatchAction::doWatchOrUnwatch() + */ + public function testDoWatchOrUnwatchSkipsIfAlreadyWatched() { + $user = $this->getLoggedInIsWatchedUser(); + $user->expects( $this->never() )->method( 'removeWatch' ); + $user->expects( $this->never() )->method( 'addWatch' ); + + $status = WatchAction::doWatchOrUnwatch( true, $this->watchAction->getTitle(), $user ); + + $this->assertTrue( $status->isGood() ); + } + + /** + * @covers WatchAction::doWatchOrUnwatch() + */ + public function testDoWatchOrUnwatchSkipsIfAlreadyUnWatched() { + $user = $this->getLoggedInIsWatchedUser( true, false ); + $user->expects( $this->never() )->method( 'removeWatch' ); + $user->expects( $this->never() )->method( 'addWatch' ); + + $status = WatchAction::doWatchOrUnwatch( false, $this->watchAction->getTitle(), $user ); + + $this->assertTrue( $status->isGood() ); + } + + /** + * @covers WatchAction::doWatchOrUnwatch() + */ + public function testDoWatchOrUnwatchWatchesIfWatch() { + $user = $this->getLoggedInIsWatchedUser( true, false ); + $user->expects( $this->never() )->method( 'removeWatch' ); + $user->expects( $this->once() ) + ->method( 'addWatch' ) + ->with( $this->equalTo( $this->testWikiPage->getTitle() ), $this->equalTo( false ) ); + + $status = WatchAction::doWatchOrUnwatch( true, $this->watchAction->getTitle(), $user ); + + $this->assertTrue( $status->isGood() ); + } + + /** + * @covers WatchAction::doWatchOrUnwatch() + */ + public function testDoWatchOrUnwatchUnwatchesIfUnwatch() { + $user = $this->getLoggedInIsWatchedUser(); + $user->method( 'isAllowed' )->willReturn( true ); + $user->expects( $this->never() )->method( 'addWatch' ); + $user->expects( $this->once() ) + ->method( 'removeWatch' ) + ->with( $this->equalTo( $this->testWikiPage->getTitle() ) ); + + $status = WatchAction::doWatchOrUnwatch( false, $this->watchAction->getTitle(), $user ); + + $this->assertTrue( $status->isGood() ); + } + + /** + * @param bool $isLoggedIn Whether the user should be "marked" as logged in + * @param bool $isWatched The value any call to isWatched should return + * @return PHPUnit_Framework_MockObject_MockObject + */ + private function getLoggedInIsWatchedUser( $isLoggedIn = true, $isWatched = true ) { + $user = $this->getMock( User::class ); + $user->method( 'isLoggedIn' )->willReturn( $isLoggedIn ); + $user->method( 'isWatched' )->willReturn( $isWatched ); + + return $user; + } + +} -- 2.20.1