From 12e2e9edbc5f97b49d85194af5fff316d5bf4a90 Mon Sep 17 00:00:00 2001 From: addshore Date: Tue, 22 Mar 2016 18:07:49 +0000 Subject: [PATCH] Switch Signature of WatchedItemStore::addWatchBatch Adding batches of watched items per users makes much more sense. Only the deprecated static WatchedItem method needed the old silly way of passing in objects. Change-Id: I90f9583b66bd3b5afcf07faefedb38a8a0149f6e --- includes/WatchedItem.php | 30 +++++---- includes/WatchedItemStore.php | 30 ++++----- includes/user/User.php | 7 +- .../includes/WatchedItemStoreUnitTest.php | 64 ++++--------------- .../phpunit/includes/WatchedItemUnitTest.php | 42 +++++++----- 5 files changed, 74 insertions(+), 99 deletions(-) diff --git a/includes/WatchedItem.php b/includes/WatchedItem.php index 5b4a4fcfe1..0495536a26 100644 --- a/includes/WatchedItem.php +++ b/includes/WatchedItem.php @@ -179,23 +179,31 @@ class WatchedItem { */ public static function batchAddWatch( array $items ) { // wfDeprecated( __METHOD__, '1.27' ); - $userTargetCombinations = []; + if ( !$items ) { + return false; + } + + $targets = []; + $users = []; /** @var WatchedItem $watchedItem */ foreach ( $items as $watchedItem ) { - if ( $watchedItem->checkRights && !$watchedItem->getUser()->isAllowed( 'editmywatchlist' ) ) { + $user = $watchedItem->getUser(); + if ( $watchedItem->checkRights && !$user->isAllowed( 'editmywatchlist' ) ) { continue; } - $userTargetCombinations[] = [ - $watchedItem->getUser(), - $watchedItem->getTitle()->getSubjectPage() - ]; - $userTargetCombinations[] = [ - $watchedItem->getUser(), - $watchedItem->getTitle()->getTalkPage() - ]; + $userId = $user->getId(); + $users[$userId] = $user; + $targets[$userId][] = $watchedItem->getTitle()->getSubjectPage(); + $targets[$userId][] = $watchedItem->getTitle()->getTalkPage(); } + $store = WatchedItemStore::getDefaultInstance(); - return $store->addWatchBatch( $userTargetCombinations ); + $success = true; + foreach ( $users as $userId => $user ) { + $success &= $store->addWatchBatchForUser( $user, $targets[$userId] ); + } + + return $success; } /** diff --git a/includes/WatchedItemStore.php b/includes/WatchedItemStore.php index 4eea54deec..c4340ad5e1 100644 --- a/includes/WatchedItemStore.php +++ b/includes/WatchedItemStore.php @@ -615,30 +615,30 @@ class WatchedItemStore { * @param LinkTarget $target */ public function addWatch( User $user, LinkTarget $target ) { - $this->addWatchBatch( [ [ $user, $target ] ] ); + $this->addWatchBatchForUser( $user, [ $target ] ); } /** - * @param array[] $userTargetCombinations array of arrays containing [0] => User [1] => LinkTarget + * @param User $user + * @param LinkTarget[] $targets * * @return bool success */ - public function addWatchBatch( array $userTargetCombinations ) { + public function addWatchBatchForUser( User $user, array $targets ) { if ( $this->loadBalancer->getReadOnlyReason() !== false ) { return false; } + // Only loggedin user can have a watchlist + if ( $user->isAnon() ) { + return false; + } + + if ( !$targets ) { + return true; + } $rows = []; - foreach ( $userTargetCombinations as list( $user, $target ) ) { - /** - * @var User $user - * @var LinkTarget $target - */ - - // Only loggedin user can have a watchlist - if ( $user->isAnon() ) { - continue; - } + foreach ( $targets as $target ) { $rows[] = [ 'wl_user' => $user->getId(), 'wl_namespace' => $target->getNamespace(), @@ -648,10 +648,6 @@ class WatchedItemStore { $this->uncache( $user, $target ); } - if ( !$rows ) { - return false; - } - $dbw = $this->getConnection( DB_MASTER ); foreach ( array_chunk( $rows, 100 ) as $toInsert ) { // Use INSERT IGNORE to avoid overwriting the notification timestamp diff --git a/includes/user/User.php b/includes/user/User.php index 027edf929b..a272b37d5f 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -3467,10 +3467,9 @@ class User implements IDBAccessObject { */ public function addWatch( $title, $checkRights = self::CHECK_USER_RIGHTS ) { if ( !$checkRights || $this->isAllowed( 'editmywatchlist' ) ) { - WatchedItemStore::getDefaultInstance()->addWatchBatch( [ - [ $this, $title->getSubjectPage() ], - [ $this, $title->getTalkPage() ], - ] + WatchedItemStore::getDefaultInstance()->addWatchBatchForUser( + $this, + [ $title->getSubjectPage(), $title->getTalkPage() ] ); } $this->invalidateCache(); diff --git a/tests/phpunit/includes/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/WatchedItemStoreUnitTest.php index afde88cddf..5f07dbf430 100644 --- a/tests/phpunit/includes/WatchedItemStoreUnitTest.php +++ b/tests/phpunit/includes/WatchedItemStoreUnitTest.php @@ -934,7 +934,7 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { ); } - public function testAddWatchBatch_nonAnonymousUser() { + public function testAddWatchBatchForUser_nonAnonymousUser() { $mockDb = $this->getMockDb(); $mockDb->expects( $this->once() ) ->method( 'insert' ) @@ -974,52 +974,14 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $mockUser = $this->getMockNonAnonUserWithId( 1 ); $this->assertTrue( - $store->addWatchBatch( - [ - [ $mockUser, new TitleValue( 0, 'Some_Page' ) ], - [ $mockUser, new TitleValue( 1, 'Some_Page' ) ], - ] - ) - ); - } - - public function testAddWatchBatch_anonymousUserCombinationsAreSkipped() { - $mockDb = $this->getMockDb(); - $mockDb->expects( $this->once() ) - ->method( 'insert' ) - ->with( - 'watchlist', - [ - [ - 'wl_user' => 1, - 'wl_namespace' => 0, - 'wl_title' => 'Some_Page', - 'wl_notificationtimestamp' => null, - ] - ] - ); - - $mockCache = $this->getMockCache(); - $mockCache->expects( $this->once() ) - ->method( 'delete' ) - ->with( '0:Some_Page:1' ); - - $store = new WatchedItemStore( - $this->getMockLoadBalancer( $mockDb ), - $mockCache - ); - - $this->assertTrue( - $store->addWatchBatch( - [ - [ $this->getMockNonAnonUserWithId( 1 ), new TitleValue( 0, 'Some_Page' ) ], - [ $this->getAnonUser(), new TitleValue( 0, 'Other_Page' ) ], - ] + $store->addWatchBatchForUser( + $mockUser, + [ new TitleValue( 0, 'Some_Page' ), new TitleValue( 1, 'Some_Page' ) ] ) ); } - public function testAddWatchBatchReturnsFalse_whenOnlyGivenAnonymousUserCombinations() { + public function testAddWatchBatchForUser_anonymousUsersAreSkipped() { $mockDb = $this->getMockDb(); $mockDb->expects( $this->never() ) ->method( 'insert' ); @@ -1033,18 +995,16 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $mockCache ); - $anonUser = $this->getAnonUser(); $this->assertFalse( - $store->addWatchBatch( - [ - [ $anonUser, new TitleValue( 0, 'Some_Page' ) ], - [ $anonUser, new TitleValue( 1, 'Other_Page' ) ], - ] + $store->addWatchBatchForUser( + $this->getAnonUser(), + [ new TitleValue( 0, 'Other_Page' ) ] ) ); } - public function testAddWatchBatchReturnsFalse_whenGivenEmptyList() { + public function testAddWatchBatchReturnsTrue_whenGivenEmptyList() { + $user = $this->getMockNonAnonUserWithId( 1 ); $mockDb = $this->getMockDb(); $mockDb->expects( $this->never() ) ->method( 'insert' ); @@ -1058,8 +1018,8 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $mockCache ); - $this->assertFalse( - $store->addWatchBatch( [] ) + $this->assertTrue( + $store->addWatchBatchForUser( $user, [] ) ); } diff --git a/tests/phpunit/includes/WatchedItemUnitTest.php b/tests/phpunit/includes/WatchedItemUnitTest.php index 58984cf389..b4eaa765e9 100644 --- a/tests/phpunit/includes/WatchedItemUnitTest.php +++ b/tests/phpunit/includes/WatchedItemUnitTest.php @@ -165,25 +165,37 @@ class WatchedItemUnitTest extends PHPUnit_Framework_TestCase { } public function testBatchAddWatch() { - /** @var WatchedItem[] $items */ - $items = [ - new WatchedItem( User::newFromId( 1 ), new TitleValue( 0, 'Title1' ), null ), - new WatchedItem( User::newFromId( 3 ), Title::newFromText( 'Title2' ), '20150101010101' ), - ]; - - $userTargetCombinations = []; - foreach ( $items as $item ) { - $userTargetCombinations[] = [ $item->getUser(), $item->getTitle()->getSubjectPage() ]; - $userTargetCombinations[] = [ $item->getUser(), $item->getTitle()->getTalkPage() ]; - } + $itemOne = new WatchedItem( User::newFromId( 1 ), new TitleValue( 0, 'Title1' ), null ); + $itemTwo = new WatchedItem( + User::newFromId( 3 ), + Title::newFromText( 'Title2' ), + '20150101010101' + ); $store = $this->getMockWatchedItemStore(); - $store->expects( $this->once() ) - ->method( 'addWatchBatch' ) - ->with( $userTargetCombinations ); + $store->expects( $this->exactly( 2 ) ) + ->method( 'addWatchBatchForUser' ); + $store->expects( $this->at( 0 ) ) + ->method( 'addWatchBatchForUser' ) + ->with( + $itemOne->getUser(), + [ + $itemOne->getTitle()->getSubjectPage(), + $itemOne->getTitle()->getTalkPage(), + ] + ); + $store->expects( $this->at( 1 ) ) + ->method( 'addWatchBatchForUser' ) + ->with( + $itemTwo->getUser(), + [ + $itemTwo->getTitle()->getSubjectPage(), + $itemTwo->getTitle()->getTalkPage(), + ] + ); $scopedOverride = WatchedItemStore::overrideDefaultInstance( $store ); - WatchedItem::batchAddWatch( $items ); + WatchedItem::batchAddWatch( [ $itemOne, $itemTwo ] ); ScopedCallback::consume( $scopedOverride ); } -- 2.20.1