From 483e7e80d587901b443950855f36bb44ff77db39 Mon Sep 17 00:00:00 2001 From: Bsitu Date: Thu, 14 Aug 2014 18:46:26 +0000 Subject: [PATCH] Revert "Revert "Merge remote-tracking branch 'gerrit/master' into two_tabs"" This reverts commit b9574748a4abf6ac921b06a690d766965bc90fca. Change-Id: I8a8334db446dd6a2d5363a1f6ab12369b813b026 --- Echo.php | 2 + Hooks.php | 16 ++++- Makefile | 2 +- api/ApiEchoNotifications.php | 7 +- formatters/BasicFormatter.php | 4 +- includes/mapper/AbstractMapper.php | 24 +++++++ includes/mapper/EventMapper.php | 15 +--- includes/mapper/NotificationMapper.php | 64 ++++++++++++----- model/AbstractEntity.php | 14 ++++ model/Event.php | 7 +- model/Notification.php | 4 +- special/SpecialNotifications.php | 2 +- tests/api/ApiEchoNotificationsTest.php | 71 +++++++++++++++++++ tests/browser/features/flyout.feature | 2 +- tests/browser/features/flyout_nojs.feature | 2 +- tests/browser/features/notifications.feature | 39 +++++++++- .../features/step_definitions/common_steps.rb | 23 +++++- .../step_definitions/notifications_steps.rb | 60 ++++++++++++++++ .../mapper/NotificationMapperTest.php | 55 +++++++++----- 19 files changed, 345 insertions(+), 68 deletions(-) create mode 100644 includes/mapper/AbstractMapper.php create mode 100644 model/AbstractEntity.php create mode 100644 tests/api/ApiEchoNotificationsTest.php diff --git a/Echo.php b/Echo.php index 6e878efac..ac8f76569 100644 --- a/Echo.php +++ b/Echo.php @@ -49,6 +49,7 @@ $wgExtensionMessagesFiles['EchoAliases'] = $dir . 'Echo.alias.php'; // Basic Echo classes $wgAutoloadClasses['EchoHooks'] = $dir . 'Hooks.php'; +$wgAutoloadClasses['EchoAbstractEntity'] = $dir . 'model/AbstractEntity.php'; $wgAutoloadClasses['EchoEvent'] = $dir . 'model/Event.php'; $wgAutoloadClasses['EchoNotification'] = $dir . 'model/Notification.php'; $wgAutoloadClasses['MWEchoEmailBatch'] = $dir . 'includes/EmailBatch.php'; @@ -59,6 +60,7 @@ $wgAutoloadClasses['MWEchoEventLogging'] = $dir . 'includes/EventLogging.php'; $wgAutoloadClasses['EchoAttributeManager'] = $dir . 'includes/AttributeManager.php'; // Database mappers && gateways +$wgAutoloadClasses['EchoAbstractMapper'] = $dir . 'includes/mapper/AbstractMapper.php'; $wgAutoloadClasses['EchoEventMapper'] = $dir . 'includes/mapper/EventMapper.php'; $wgAutoloadClasses['EchoNotificationMapper'] = $dir . 'includes/mapper/NotificationMapper.php'; $wgAutoloadClasses['EchoUserNotificationGateway'] = $dir . 'includes/gateway/UserNotificationGateway.php'; diff --git a/Hooks.php b/Hooks.php index 8eb741bfb..ed5e7bc29 100644 --- a/Hooks.php +++ b/Hooks.php @@ -768,8 +768,22 @@ class EchoHooks { * @return bool true in all cases */ static function getUnitTests( &$files ) { - $files = array_merge( $files, glob( __DIR__ . '/tests/*Test.php' ) ); + // @codeCoverageIgnoreStart + $directoryIterator = new RecursiveDirectoryIterator( __DIR__ . '/tests/' ); + + /** + * @var SplFileInfo $fileInfo + */ + $ourFiles = array(); + foreach ( new RecursiveIteratorIterator( $directoryIterator ) as $fileInfo ) { + if ( substr( $fileInfo->getFilename(), -8 ) === 'Test.php' ) { + $ourFiles[] = $fileInfo->getPathname(); + } + } + + $files = array_merge( $files, $ourFiles ); return true; + // @codeCoverageIgnoreEnd } /** diff --git a/Makefile b/Makefile index c98c91a32..3b5da01ca 100644 --- a/Makefile +++ b/Makefile @@ -41,7 +41,7 @@ nodecheck: # Verify all javascript in the project has valid syntax and follows jshint rules jshint: nodecheck - @node_modules/.bin/jshint modules/* --config .jshintrc + @node_modules/.bin/jshint modules/* tests/qunit --config .jshintrc # Verify all less files are compilable checkless: diff --git a/api/ApiEchoNotifications.php b/api/ApiEchoNotifications.php index cb7776866..3027aa9bb 100644 --- a/api/ApiEchoNotifications.php +++ b/api/ApiEchoNotifications.php @@ -51,7 +51,10 @@ class ApiEchoNotifications extends ApiQueryBase { } if ( in_array( 'count', $prop ) ) { - $result += $this->getPropcount( $user, $params['sections'], $params['groupbysection'] ); + $result = array_merge_recursive( + $result, + $this->getPropcount( $user, $params['sections'], $params['groupbysection'] ) + ); } $this->getResult()->setIndexedTagName( $result, 'notification' ); @@ -110,7 +113,7 @@ class ApiEchoNotifications extends ApiQueryBase { ); // Fetch the result for the event types above - $notifMapper = new EchoNotificationMapper( MWEchoDbFactory::newFromDefault() ); + $notifMapper = new EchoNotificationMapper(); $notifs = $notifMapper->fetchByUser( $user, $limit + 1, $continue, $eventTypesToLoad ); foreach ( $notifs as $notif ) { $result['list'][$notif->getEvent()->getID()] = EchoDataOutputFormatter::formatOutput( $notif, $format, $user ); diff --git a/formatters/BasicFormatter.php b/formatters/BasicFormatter.php index 2cc71f41b..069fa353c 100644 --- a/formatters/BasicFormatter.php +++ b/formatters/BasicFormatter.php @@ -558,7 +558,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { return false; } - $eventMapper = new EchoEventMapper( MWEchoDbFactory::newFromDefault() ); + $eventMapper = new EchoEventMapper(); $events = $eventMapper->fetchByUserBundleHash( $user, $event->getBundleHash(), $this->distributionType, 'DESC', self::$maxRawBundleData ); @@ -811,7 +811,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { $data = $this->bundleData['last-raw-data']; // Then try to query the storage } else { - $eventMapper = new EchoEventMapper( MWEchoDbFactory::newFromDefault() ); + $eventMapper = new EchoEventMapper(); $data = $eventMapper->fetchByUserBundleHash( $user, $event->getBundleHash(), $this->distributionType, 'ASC', 1 ); diff --git a/includes/mapper/AbstractMapper.php b/includes/mapper/AbstractMapper.php new file mode 100644 index 000000000..77b45f3a9 --- /dev/null +++ b/includes/mapper/AbstractMapper.php @@ -0,0 +1,24 @@ +dbFactory = $dbFactory; + } + +} diff --git a/includes/mapper/EventMapper.php b/includes/mapper/EventMapper.php index a888a5d30..b9d3c32c0 100644 --- a/includes/mapper/EventMapper.php +++ b/includes/mapper/EventMapper.php @@ -4,20 +4,7 @@ * Database mapper for EchoEvent model, which is an immutable class, there should * not be any update to it */ -class EchoEventMapper { - - /** - * Echo database factory - * @param MWEchoDbFactory - */ - protected $dbFactory; - - /** - * @param MWEchoDbFactory - */ - public function __construct( MWEchoDbFactory $dbFactory ) { - $this->dbFactory = $dbFactory; - } +class EchoEventMapper extends EchoAbstractMapper { /** * Insert an event record diff --git a/includes/mapper/NotificationMapper.php b/includes/mapper/NotificationMapper.php index 970d73647..2e65cd9e9 100644 --- a/includes/mapper/NotificationMapper.php +++ b/includes/mapper/NotificationMapper.php @@ -3,20 +3,7 @@ /** * Database mapper for EchoNotification model */ -class EchoNotificationMapper { - - /** - * Echo database factory - * @param MWEchoDbFactory - */ - protected $dbFactory; - - /** - * @param MWEchoDbFactory - */ - public function __construct( MWEchoDbFactory $dbFactory ) { - $this->dbFactory = $dbFactory; - } +class EchoNotificationMapper extends EchoAbstractMapper { /** * Insert a notification record @@ -84,6 +71,49 @@ class EchoNotificationMapper { return $offset; } + /** + * Get unread notifications by user in the amount specified by limit. Based on existing + * requirements, we just need x amount ( 100 ) unread notifications to show on the + * overlay, so we don't need offset and ordering, we have an index to retrieve unread + * notifications but it's not optimized for ordering + * @param User $user + * @param int $limit + * @param string[] $eventTypes + * @return EchoNotification[] + */ + public function fetchUnreadByUser( User $user, $limit, array $eventTypes = array() ) { + $data = array(); + + if ( !$eventTypes ) { + return $data; + } + + $dbr = $this->dbFactory->getEchoDb( DB_SLAVE ); + $res = $dbr->select( + array( 'echo_notification', 'echo_event' ), + '*', + array( + 'notification_user' => $user->getID(), + 'event_type' => $eventTypes, + 'notification_bundle_base' => 1, + 'notification_read_timestamp' => NULL + ), + __METHOD__, + array( + 'LIMIT' => $limit, + ), + array( + 'echo_event' => array( 'LEFT JOIN', 'notification_event=event_id' ), + ) + ); + if ( $res ) { + foreach ( $res as $row ) { + $data[] = EchoNotification::newFromRow( $row ); + } + } + return $data; + } + /** * Get Notification by user in batch along with limit, offset etc * @param User the user to get notifications for @@ -92,10 +122,10 @@ class EchoNotificationMapper { * @param array Event types to load * @return EchoNotification[] */ - public function fetchByUser( User $user, $limit, $continue, array $eventTypesToLoad = array() ) { + public function fetchByUser( User $user, $limit, $continue, array $eventTypes = array() ) { $dbr = $this->dbFactory->getEchoDb( DB_SLAVE ); - if ( !$eventTypesToLoad ) { + if ( !$eventTypes ) { return array(); } @@ -109,7 +139,7 @@ class EchoNotificationMapper { // Look for notifications with base = 1 $conds = array( 'notification_user' => $user->getID(), - 'event_type' => $eventTypesToLoad, + 'event_type' => $eventTypes, 'notification_bundle_base' => 1 ); diff --git a/model/AbstractEntity.php b/model/AbstractEntity.php new file mode 100644 index 000000000..fed21f4e0 --- /dev/null +++ b/model/AbstractEntity.php @@ -0,0 +1,14 @@ +id = $eventMapper->insert( $this ); } @@ -242,7 +243,7 @@ class EchoEvent { * @param $fromMaster bool */ public function loadFromID( $id, $fromMaster = false ) { - $eventMapper = new EchoEventMapper( MWEchoDbFactory::newFromDefault() ); + $eventMapper = new EchoEventMapper(); $event = $eventMapper->fetchById( $id, $fromMaster ); // Copy over the attribute diff --git a/model/Notification.php b/model/Notification.php index 6e00ed6e2..f617e4095 100644 --- a/model/Notification.php +++ b/model/Notification.php @@ -1,6 +1,6 @@ fetchByUser( diff --git a/tests/api/ApiEchoNotificationsTest.php b/tests/api/ApiEchoNotificationsTest.php new file mode 100644 index 000000000..cd1a9732f --- /dev/null +++ b/tests/api/ApiEchoNotificationsTest.php @@ -0,0 +1,71 @@ +doApiRequest( array( + 'action' => 'query', + 'meta' => 'notifications', + 'notsections' => 'alert|message', + 'notgroupbysection' => 1, + 'notlimit' => 10, + 'notprop' => 'index|list|count' ) ); + + $this->assertArrayHasKey( 'query', $data[0] ); + $this->assertArrayHasKey( 'notifications', $data[0]['query'] ); + + $result = $data[0]['query']['notifications']; + + // General count + $this->assertArrayHasKey( 'count', $result ); + $this->assertArrayHasKey( 'rawcount', $result ); + + // Alert + $this->assertArrayHasKey( 'alert', $result ); + $alert = $result['alert']; + $this->assertArrayHasKey( 'list', $alert ); + $this->assertArrayHasKey( 'continue', $alert ); + $this->assertArrayHasKey( 'index', $alert ); + $this->assertArrayHasKey( 'rawcount', $alert ); + $this->assertArrayHasKey( 'count', $alert ); + + // Message + $this->assertArrayHasKey( 'message', $result ); + $message = $result['message']; + $this->assertArrayHasKey( 'list', $message ); + $this->assertArrayHasKey( 'continue', $message ); + $this->assertArrayHasKey( 'index', $message ); + $this->assertArrayHasKey( 'rawcount', $message ); + $this->assertArrayHasKey( 'count', $message ); + } + + public function testWithoutSectionGrouping() { + $data = $this->doApiRequest( array( + 'action' => 'query', + 'meta' => 'notifications', + 'notsections' => 'alert|message', + 'notlimit' => 10, + 'notprop' => 'index|list|count' ) ); + + $this->assertArrayHasKey( 'query', $data[0] ); + $this->assertArrayHasKey( 'notifications', $data[0]['query'] ); + + $result = $data[0]['query']['notifications']; + + $this->assertArrayHasKey( 'count', $result ); + $this->assertArrayHasKey( 'rawcount', $result ); + $this->assertArrayHasKey( 'list', $result ); + $this->assertArrayHasKey( 'continue', $result ); + $this->assertArrayHasKey( 'index', $result ); + + $this->assertTrue( !isset( $result['alert'] ) ); + $this->assertTrue( !isset( $result['message'] ) ); + } + +} diff --git a/tests/browser/features/flyout.feature b/tests/browser/features/flyout.feature index dfe722541..f6a638172 100644 --- a/tests/browser/features/flyout.feature +++ b/tests/browser/features/flyout.feature @@ -1,4 +1,4 @@ -@chrome @firefox @en.m.wikipedia.beta.wmflabs.org +@chrome @en.wikipedia.beta.wmflabs.org @firefox Feature: Flyout Background: diff --git a/tests/browser/features/flyout_nojs.feature b/tests/browser/features/flyout_nojs.feature index d64d9326d..e09dfec91 100644 --- a/tests/browser/features/flyout_nojs.feature +++ b/tests/browser/features/flyout_nojs.feature @@ -1,4 +1,4 @@ -@en.m.wikipedia.beta.wmflabs.org @firefox +@chrome @en.wikipedia.beta.wmflabs.org @firefox @login Feature: Flyout (nojs) Background: diff --git a/tests/browser/features/notifications.feature b/tests/browser/features/notifications.feature index 09a6784b4..e22ced979 100644 --- a/tests/browser/features/notifications.feature +++ b/tests/browser/features/notifications.feature @@ -1,8 +1,43 @@ -@chrome @firefox @en.m.wikipedia.beta.wmflabs.org +@chrome @en.wikipedia.beta.wmflabs.org @firefox @login Feature: Notification types + # Scenarios which trigger notifications + Scenario: Someone links to a page I created + Given I am logged in with no notifications + And another user has linked to a page I created from another page + And I come back from grabbing a cup of coffee + When I am on the "Selenium Echo flyout test page" page + Then I have new notifications + + Scenario: Mention message triggers notification + Given I am logged in with no notifications + And another user mentions me on the wiki + And I come back from grabbing a cup of coffee + When I am on the "Selenium Echo flyout test page" page + Then I have new notifications + + Scenario: Talk page message triggers talk notification + Given I am logged in with no notifications + # And I do not have Flow boards enabled on the user talk namespace + And another user writes on my talk page + And I come back from grabbing a cup of coffee + When I am on the "Selenium Echo flyout test page" page + Then I have new notifications + Scenario: New user gets a sign up notification Given I am logged in as a new user - And I am on the "Selenium Echo flyout test page" page + And I am on the "Selenium Echo flyout test page" page Then I have new notifications + Scenario: Change in user rights + # Too hard. Will do later. + + Scenario: Page revert + # Too hard. Will do later. + + # Scenarios which do not trigger notifications (but might be expected to) + Scenario: The @ message is not a keyword + Given I am logged in with no notifications + And another user @s me on "Talk:Echo at test" + When I am on the "Selenium Echo flyout test page" page + Then I have no new notifications diff --git a/tests/browser/features/step_definitions/common_steps.rb b/tests/browser/features/step_definitions/common_steps.rb index 4433495f6..744899737 100644 --- a/tests/browser/features/step_definitions/common_steps.rb +++ b/tests/browser/features/step_definitions/common_steps.rb @@ -1,3 +1,7 @@ +def get_session_username + return "#{ENV["MEDIAWIKI_USER"]}_#{@browser.name}" +end + # For use in Firefox browser tests only Given /^I am using user agent "(.+)"$/ do |user_agent| @user_agent = user_agent @@ -18,9 +22,24 @@ Given(/^I am on the "(.+)" page$/) do |title| visit(ArticlePage, :using_params => {:article_name => title}) end +Given(/^the user "(.*?)" exists$/) do |username| + on(APIPage).client.log_in(ENV["MEDIAWIKI_USER"], ENV["MEDIAWIKI_PASSWORD"]) + begin + on(APIPage).client.create_account(username, ENV["MEDIAWIKI_PASSWORD"]) + rescue MediawikiApi::ApiError + puts 'Assuming user ' + username + ' already exists since was unable to create.' + end +end + Given(/^I am logged in as the user "(.*?)"$/) do |username| - on(APIPage).client.create_account(@username, ENV["MEDIAWIKI_PASSWORD"]) - visit(LoginPage).login_with(@username, ENV["MEDIAWIKI_PASSWORD"]) + step 'the user "' + username +'" exists' + visit(LoginPage).login_with(username, ENV["MEDIAWIKI_PASSWORD"]) +end + +# Note Echo redefines this so that the user is unique to the current browser +Given(/^I am logged in my non-shared account$/) do + username = get_session_username() + step 'I am logged in as the user "' + username + '"' end Given(/^I am logged in as a new user$/) do diff --git a/tests/browser/features/step_definitions/notifications_steps.rb b/tests/browser/features/step_definitions/notifications_steps.rb index a65b0f54c..99340932a 100644 --- a/tests/browser/features/step_definitions/notifications_steps.rb +++ b/tests/browser/features/step_definitions/notifications_steps.rb @@ -1,3 +1,63 @@ +def make_page_with_user( title, text, username ) + client = on(APIPage).client + client.log_in(username, ENV["MEDIAWIKI_PASSWORD"]) + client.create_page(title, text) +end + +def make_page_with_user_b( title, text ) + username = "EchoUser" + step 'the user "' + username + '" exists' + make_page_with_user( title, text, username ) +end + +def make_page_with_user_a( title, text ) + make_page_with_user( title, text, get_session_username() ) +end + +Given(/^another user writes on my talk page$/) do + make_page_with_user_b("User talk:" + get_session_username(), + "== Barnstar ==\nHello Selenium, here is a barnstar for all your testing! " + @random_string + "~~~~\n") +end + +Given(/^another user @s me on "(.*?)"$/) do |title| + username = get_session_username().sub( '_', ' ' ) + text = "@" + username + " Cho cho cho. ~~~~" + make_page_with_user_b(title, text) +end + +Given(/^I come back from grabbing a cup of coffee$/) do + # Notifications can be extremely slow to trickle into beta labs so go to sleep for a bit + sleep 7 +end + +Given(/^another user mentions me on the wiki$/) do + title = 'Selenium Echo mention test ' + @random_string + username = get_session_username().sub( '_', ' ' ) + text = "== The walrus ==\n[[User:" + username + "]]: Cho cho cho. ~~~~\n" + make_page_with_user_b(title, text) +end + +Given(/^I am logged in with no notifications$/) do + step 'I am logged in my non-shared account' + # wait for JavaScript to have fully loaded + sleep 5 + on(ArticlePage).flyout_link_element.click + # wait for the API call that marks these as read and for UI to refresh + sleep 5 + on(ArticlePage).flyout_link_element.class_name.should_not match 'mw-echo-unread-notifications' +end + Then(/^I have new notifications$/) do on(ArticlePage).flyout_link_element.when_present.class_name.should match 'mw-echo-unread-notifications' end + +Then(/^I have no new notifications$/) do + on(ArticlePage).flyout_link_element.when_present.class_name.should_not match 'mw-echo-unread-notifications' +end + +Then(/^another user has linked to a page I created from another page$/) do + title = 'Selenium Echo link test ' + @random_string + make_page_with_user_a(title, "Selenium test page. Feel free to delete me.") + title2 = title + ' ' + @random_string + make_page_with_user_b(title2, "I am linking to [[" + title + "]].") +end diff --git a/tests/includes/mapper/NotificationMapperTest.php b/tests/includes/mapper/NotificationMapperTest.php index a88dfd281..147edcd86 100644 --- a/tests/includes/mapper/NotificationMapperTest.php +++ b/tests/includes/mapper/NotificationMapperTest.php @@ -9,17 +9,44 @@ class EchoNotificationMapperTest extends MediaWikiTestCase { $this->assertTrue( true ); } - public function testFetchByUser() { - global $wgEchoNotificationCategories; - $previous = $wgEchoNotificationCategories; + public function fetchUnreadByUser( User $user, $limit, array $eventTypes = array() ) { + // Unsuccessful select + $notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array ( 'select' => false ) ) ); + $res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, '' ); + $this->assertEmpty( $res ); - // Alter the category group so the user is always elegible to - // view some notification types. - foreach ( $wgEchoNotificationCategories as &$value ) { - $value['usergroups'] = array( 'echo_group' ); + // Successful select + $dbResult = array( + (object)array ( + 'event_id' => 1, + 'event_type' => 'test_event', + 'event_variant' => '', + 'event_extra' => '', + 'event_page_id' => '', + 'event_agent_id' => '', + 'event_agent_ip' => '', + 'notification_user' => 1, + 'notification_timestamp' => '20140615101010', + 'notification_read_timestamp' => '', + 'notification_bundle_base' => 1, + 'notification_bundle_hash' => 'testhash', + 'notification_bundle_display_hash' => 'testdisplayhash' + ) + ); + $notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array ( 'select' => $dbResult ) ) ); + $res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, '', array() ); + $this->assertEmpty( $res ); + + $notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array ( 'select' => $dbResult ) ) ); + $res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, '', array( 'test_event' ) ); + $this->assertTrue( is_array( $res ) ); + $this->assertGreaterThan( 0, count( $res ) ); + foreach ( $res as $row ) { + $this->assertInstanceOf( 'EchoNotification', $row ); } - unset( $value ); + } + public function testFetchByUser() { // Unsuccessful select $notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array ( 'select' => false ) ) ); $res = $notifMapper->fetchByUser( $this->mockUser(), 10, '' ); @@ -29,7 +56,7 @@ class EchoNotificationMapperTest extends MediaWikiTestCase { $dbResult = array( (object)array ( 'event_id' => 1, - 'event_type' => 'test', + 'event_type' => 'test_event', 'event_variant' => '', 'event_extra' => '', 'event_page_id' => '', @@ -55,19 +82,9 @@ class EchoNotificationMapperTest extends MediaWikiTestCase { $this->assertInstanceOf( 'EchoNotification', $row ); } - // Alter the category group so the user is not elegible to - // view any notification types. - foreach ( $wgEchoNotificationCategories as &$value ) { - $value['usergroups'] = array( 'sysop' ); - } - unset( $value ); - $notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array() ) ); $res = $notifMapper->fetchByUser( $this->mockUser(), 10, '' ); $this->assertEmpty( $res ); - - // Restore the default setting - $wgEchoNotificationCategories = $previous; } public function testFetchNewestByUserBundleHash() {