Merge "Revert "Revert "Merge remote-tracking branch 'gerrit/master' into two_tabs"""

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';

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
 	}
 
 	/**

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:

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 );

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
 				);

includes/mapper/AbstractMapper.php

@@ -0,0 +1,24 @@
+<?php
+
+/**
+ * Abstract mapper for model
+ */
+abstract class EchoAbstractMapper {
+
+	/**
+	 * Echo database factory
+	 * @var MWEchoDbFactory
+	 */
+	protected $dbFactory;
+
+	/**
+	 * @param MWEchoDbFactory|null
+	 */
+	public function __construct( MWEchoDbFactory $dbFactory = null ) {
+		if ( $dbFactory === null ) {
+			$dbFactory = MWEchoDbFactory::newFromDefault();
+		}
+		$this->dbFactory = $dbFactory;
+	}
+
+}

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

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
 		);
 

model/AbstractEntity.php

@@ -0,0 +1,14 @@
+<?php
+
+/**
+ * Abstract entity for Echo model
+ */
+abstract class EchoAbstractEntity {
+
+	/**
+	 * Convert an entity's property to array
+	 * @return array
+	 */
+	abstract public function toDbArray();
+
+}

model/Event.php

@@ -4,7 +4,8 @@
  * Immutable class to represent an event.
  * In Echo nomenclature, an event is a single occurrence.
  */
-class EchoEvent {
+class EchoEvent extends EchoAbstractEntity{
+
 	protected $type = null;
 	protected $id = null;
 	protected $variant = null;
@@ -194,7 +195,7 @@ class EchoEvent {
 	 * Inserts the object into the database.
 	 */
 	protected function insert() {
-		$eventMapper = new EchoEventMapper( MWEchoDbFactory::newFromDefault() );
+		$eventMapper = new EchoEventMapper();
 		$this->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

model/Notification.php

@@ -1,6 +1,6 @@
 <?php
 
-class EchoNotification {
+class EchoNotification extends EchoAbstractEntity {
 
 	/**
 	 * @var User
@@ -92,7 +92,7 @@ class EchoNotification {
 	protected function insert() {
 		global $wgEchoNotifications;
 
-		$notifMapper = new EchoNotificationMapper( MWEchoDbFactory::newFromDefault() );
+		$notifMapper = new EchoNotificationMapper();
 
 		// Get the bundle key for this event if web bundling is enabled
 		$bundleKey = '';

special/SpecialNotifications.php

@@ -40,7 +40,7 @@ class SpecialNotifications extends SpecialPage {
 
 		// Pull the notifications
 		$notif = array();
-		$notificationMapper = new EchoNotificationMapper( MWEchoDbFactory::newFromDefault() );
+		$notificationMapper = new EchoNotificationMapper();
 
 		$attributeManager = EchoAttributeManager::newFromGlobalVars();
 		$notifications = $notificationMapper->fetchByUser(

tests/api/ApiEchoNotificationsTest.php

@@ -0,0 +1,71 @@
+<?php
+
+/**
+ * @group medium
+ * @group API
+ * @covers ApiQuery
+ */
+class ApiEchoNotificationsTest extends ApiTestCase {
+
+	public function testWithSectionGrouping() {
+		// Grouping by section
+		$data = $this->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'] ) );
+	}
+
+}

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:

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:

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

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

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

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() {