jenkins-bot has submitted this change and it was merged.
Change subject: Allow multiple target pages per event
......................................................................
Allow multiple target pages per event
Bug: T85064
Change-Id: I338f3d73efb98a3bb66ef64fdeeb66e752a453c2
---
M Hooks.php
A RELEASE_NOTES
A db_patches/patch-multiple_target_pages.sql
A db_patches/patch-multiple_target_pages.sqlite.sql
M echo.sql
M includes/DataOutputFormatter.php
M includes/mapper/NotificationMapper.php
M includes/mapper/TargetPageMapper.php
M model/Notification.php
M tests/phpunit/includes/mapper/NotificationMapperTest.php
M tests/phpunit/includes/mapper/TargetPageMapperTest.php
M tests/phpunit/model/NotificationTest.php
12 files changed, 174 insertions(+), 55 deletions(-)
Approvals:
Matthias Mullie: Looks good to me, approved
jenkins-bot: Verified
diff --git a/Hooks.php b/Hooks.php
index 5e425d0..6e9334a 100644
--- a/Hooks.php
+++ b/Hooks.php
@@ -115,6 +115,7 @@
if ( $updater->getDB()->getType() === 'sqlite' ) {
$updater->modifyExtensionField( 'echo_event',
'event_agent', "$dir/db_patches/patch-event_agent-split.sqlite.sql" );
$updater->modifyExtensionField( 'echo_event',
'event_variant', "$dir/db_patches/patch-event_variant_nullability.sqlite.sql" );
+ $updater->addExtensionField( 'echo_target_page',
'etp_id', "$dir/db_patches/patch-multiple_target_pages.sqlite.sql" );
// There is no need to run the patch-event_extra-size
or patch-event_agent_ip-size because
// sqlite ignores numeric arguments in parentheses that
follow the type name (ex: VARCHAR(255))
// see http://www.sqlite.org/datatype3.html Section 2.2
for more info
@@ -123,6 +124,7 @@
$updater->modifyExtensionField( 'echo_event',
'event_variant', "$dir/db_patches/patch-event_variant_nullability.sql" );
$updater->modifyExtensionField( 'echo_event',
'event_extra', "$dir/db_patches/patch-event_extra-size.sql" );
$updater->modifyExtensionField( 'echo_event',
'event_agent_ip', "$dir/db_patches/patch-event_agent_ip-size.sql" );
+ $updater->addExtensionField( 'echo_target_page',
'etp_id', "$dir/db_patches/patch-multiple_target_pages.sql" );
}
$updater->addExtensionField( 'echo_notification',
'notification_bundle_base',
@@ -663,10 +665,7 @@
$mapper = new EchoTargetPageMapper();
$targetPages = $mapper->fetchByUserPageId( $user,
$title->getArticleID() );
if ( $targetPages ) {
- $eventIds = array();
- foreach ( $targetPages as $targetPage ) {
- $eventIds[] = $targetPage->getEventId();
- }
+ $eventIds = array_keys( $targetPages );
$notifUser = MWEchoNotifUser::newFromUser(
$user );
$notifUser->markRead( $eventIds );
}
diff --git a/RELEASE_NOTES b/RELEASE_NOTES
new file mode 100644
index 0000000..c6d0fcf
--- /dev/null
+++ b/RELEASE_NOTES
@@ -0,0 +1,7 @@
+March, 2015
+-----------
+
+* Echo events now support multiple target pages. As a result The api output
of individual
+ notifications within api.php?action=query&meta=notifications no longer has a
'targetpage'
+ property. This has been replaced with a 'targetpages' property which is the
same as
+ 'targetpage' was, but now as an array.
diff --git a/db_patches/patch-multiple_target_pages.sql
b/db_patches/patch-multiple_target_pages.sql
new file mode 100644
index 0000000..5d574f9
--- /dev/null
+++ b/db_patches/patch-multiple_target_pages.sql
@@ -0,0 +1,3 @@
+ALTER TABLE /*_*/echo_target_page ADD etp_id int unsigned not null primary key
auto_increment;
+DROP INDEX /*i*/echo_target_page_user_event ON /*_*/echo_target_page;
+CREATE INDEX /*i*/echo_target_page_user_event ON /*_*/echo_target_page
(etp_user, etp_event);
diff --git a/db_patches/patch-multiple_target_pages.sqlite.sql
b/db_patches/patch-multiple_target_pages.sqlite.sql
new file mode 100644
index 0000000..0943bca
--- /dev/null
+++ b/db_patches/patch-multiple_target_pages.sqlite.sql
@@ -0,0 +1,27 @@
+-- Sqlite can't add a primary key to an existing table
+
+-- give current table temporary name
+ALTER TABLE /*_*/echo_target_page RENAME TO /*_*/temp_echo_target_page;
+
+-- recreate table with our new setup
+CREATE TABLE /*_*/echo_target_page (
+ etp_id int unsigned not null primary key auto_increment,
+ etp_user int unsigned not null default 0,
+ etp_page int unsigned not null default 0,
+ etp_event int unsigned not null default 0
+) /*$wgDBTableOptions*/;
+
+-- copy over old data into new table
+INSERT INTO /*_*/echo_target_page
+ (etp_user, etp_page, etp_event)
+SELECT
+ etp_user, etp_page, etp_event
+FROM
+ /*_*/temp_echo_target_page;
+
+-- drop the original table
+DROP TABLE /*_*/temp_echo_target_page;
+
+-- recreate indexes
+CREATE INDEX /*i*/echo_target_page_user_event ON /*_*/echo_target_page
(etp_user, etp_event);
+CREATE INDEX /*i*/echo_target_page_user_page_event ON /*_*/echo_target_page
(etp_user, etp_page, etp_event);
diff --git a/echo.sql b/echo.sql
index 02871a6..910203a 100644
--- a/echo.sql
+++ b/echo.sql
@@ -43,10 +43,11 @@
CREATE INDEX /*i*/echo_email_batch_user_hash_priority ON /*_*/echo_email_batch
(eeb_user_id, eeb_event_hash, eeb_event_priority);
CREATE TABLE /*_*/echo_target_page (
+ etp_id int unsigned not null primary key auto_increment,
etp_user int unsigned not null default 0,
etp_page int unsigned not null default 0,
etp_event int unsigned not null default 0
) /*$wgDBTableOptions*/;
-CREATE UNIQUE INDEX /*i*/echo_target_page_user_event ON /*_*/echo_target_page
(etp_user, etp_event);
+CREATE INDEX /*i*/echo_target_page_user_event ON /*_*/echo_target_page
(etp_user, etp_event);
CREATE INDEX /*i*/echo_target_page_user_page_event ON /*_*/echo_target_page
(etp_user, etp_page, etp_event);
diff --git a/includes/DataOutputFormatter.php b/includes/DataOutputFormatter.php
index 68fadf1..06ce147 100644
--- a/includes/DataOutputFormatter.php
+++ b/includes/DataOutputFormatter.php
@@ -93,10 +93,11 @@
// This is only meant for unread notifications, if a
notification has a target
// page, then it shouldn't be auto marked as read unless the
user visits
// the target page or a user marks it as read manully ( coming
soon )
- if ( $notification->getTargetPage() ) {
- $output['targetpage'] =
$notification->getTargetPage()->getPageId();
- } else {
- $output['targetpage'] = '';
+ $output['targetpages'] = array();
+ if ( $notification->getTargetPages() ) {
+ foreach ( $notification->getTargetPages() as
$targetPage ) {
+ $output['targetpages'][] =
$targetPage->getPageId();
+ }
}
if ( $format ) {
diff --git a/includes/mapper/NotificationMapper.php
b/includes/mapper/NotificationMapper.php
index 8844c60..a3fef50 100644
--- a/includes/mapper/NotificationMapper.php
+++ b/includes/mapper/NotificationMapper.php
@@ -6,6 +6,22 @@
class EchoNotificationMapper extends EchoAbstractMapper {
/**
+ * @var EchoTargetPageMapper
+ */
+ protected $targetPageMapper;
+
+ public function __construct(
+ MWEchoDbFactory $dbFactory = null,
+ EchoTargetPageMapper $targetPageMapper = null
+ ) {
+ parent::__construct( $dbFactory );
+ if ( $targetPageMapper === null ) {
+ $targetPageMapper = new EchoTargetPageMapper(
$this->dbFactory );
+ }
+ $this->targetPageMapper = $targetPageMapper;
+ }
+
+ /**
* Insert a notification record
* @param EchoNotification
* @return null
@@ -175,17 +191,37 @@
)
);
- $data = array();
- if ( $res ) {
- foreach ( $res as $row ) {
- try {
- $data[$row->event_id] =
EchoNotification::newFromRow( $row );
- } catch ( Exception $e ) {
- $id = isset( $row->event_id ) ?
$row->event_id : 'unknown event';
- wfDebugLog( 'Echo', __METHOD__ . ":
Failed initializing event: $id" );
- MWExceptionHandler::logException( $e );
+ // query failure of some sort
+ if ( !$res ) {
+ return array();
+ }
+
+ $events = array();
+ foreach ( $res as $row ) {
+ $events[$row->event_id] = $row;
+ }
+
+ // query returned no events
+ if ( !$events ) {
+ return array();
+ }
+
+ $targetPages = $this->targetPageMapper->fetchByUserPageId(
$user, array_keys( $events ) );
+
+ $data = array();
+ foreach ( $events as $eventId => $row ) {
+ try {
+ if ( isset( $targetPages[$row->event_id] ) ) {
+ $targets = $targetPages[$row->event_id];
+ } else {
+ $targets = null;
}
+ $data[$row->event_id] =
EchoNotification::newFromRow( $row, $targets );
+ } catch ( Exception $e ) {
+ $id = isset( $row->event_id ) ? $row->event_id
: 'unknown event';
+ wfDebugLog( 'Echo', __METHOD__ . ": Failed
initializing event: $id" );
+ MWExceptionHandler::logException( $e );
}
}
return $data;
diff --git a/includes/mapper/TargetPageMapper.php
b/includes/mapper/TargetPageMapper.php
index beae448..b4a2108 100644
--- a/includes/mapper/TargetPageMapper.php
+++ b/includes/mapper/TargetPageMapper.php
@@ -16,11 +16,13 @@
);
/**
- * Fetch an EchoTargetPage instance by user & page_id
+ * Fetch EchoTargetPage instances by user & page_id. The resulting
+ * array is indexed by the event id. Each entry contains an array
+ * of EchoTargetPage instances.
*
* @param User $user
- * @param int $pageId
- * @return EchoTargetPage[]|boolean
+ * @param int|int[] $pageId One or more page ids to fetch target pages
of
+ * @return EchoTargetPage[][]|boolean
*/
public function fetchByUserPageId( User $user, $pageId ) {
$dbr = $this->dbFactory->getEchoDb( DB_SLAVE );
@@ -37,7 +39,7 @@
if ( $res ) {
$targetPages = array();
foreach ( $res as $row ) {
- $targetPages[] = EchoTargetPage::newFromRow(
$row );
+ $targetPages[$row->etp_event][] =
EchoTargetPage::newFromRow( $row );
}
return $targetPages;
} else {
diff --git a/model/Notification.php b/model/Notification.php
index a0f808b..17f2e0e 100644
--- a/model/Notification.php
+++ b/model/Notification.php
@@ -13,10 +13,12 @@
protected $event;
/**
- * The target page object for the notification if there is one
- * @var EchoTargetPage|null
+ * The target page object for the notification if there is one. Null
means
+ * the information has not been loaded.
+ *
+ * @var EchoTargetPage[]|null
*/
- protected $targetPage;
+ protected $targetPages;
/**
* @var string
@@ -133,20 +135,14 @@
// Create a target page object if specified by event
$event = $this->event;
$user = $this->user;
- if ( $event->getExtraParam( 'target-page' ) ) {
- $notifMapper->attachListener( 'insert',
'add-target-page', function() use ( $event, $user, $eventIds ) {
- $targetPageId = $event->getExtraParam(
'target-page' );
- // Make sure the target-page id is a valid id
- $title = Title::newFromID( $targetPageId );
- // Try master if there is no match
- if ( !$title ) {
- $title = Title::newFromID(
$targetPageId, Title::GAID_FOR_UPDATE );
+ $targetPages = self::resolveTargetPages( $event->getExtraParam(
'target-page' ) );
+ if ( $targetPages ) {
+ $notifMapper->attachListener( 'insert',
'add-target-page', function() use ( $event, $user, $eventIds, $targetPages ) {
+ $targetMapper = new EchoTargetPageMapper();
+ if ( $eventIds ) {
+ $targetMapper->deleteByUserEvents(
$user, $eventIds );
}
- if ( $title ) {
- $targetMapper = new
EchoTargetPageMapper();
- if ( $eventIds ) {
-
$targetMapper->deleteByUserEvents( $user, $eventIds );
- }
+ foreach ( $targetPages as $title ) {
$targetPage = EchoTargetPage::create(
$user, $title, $event );
if ( $targetPage ) {
$targetMapper->insert(
$targetPage );
@@ -173,11 +169,38 @@
}
/**
+ * @param int[]|int|false $targetPageIds
+ * @return Title[]
+ */
+ protected static function resolveTargetPages( $targetPageIds ) {
+ if ( !$targetPageIds ) {
+ return array();
+ }
+ if ( !is_array( $targetPageIds ) ) {
+ $targetPageIds = array( $targetPageIds );
+ }
+ $result = array();
+ foreach ( $targetPageIds as $targetPageId ) {
+ // Make sure the target-page id is a valid id
+ $title = Title::newFromID( $targetPageId );
+ // Try master if there is no match
+ if ( !$title ) {
+ $title = Title::newFromID( $targetPageId,
Title::GAID_FOR_UPDATE );
+ }
+ if ( $title ) {
+ $result[] = $title;
+ }
+ }
+ return $result;
+ }
+
+ /**
* Load a notification record from std class
* @param stdClass
+ * @param EchoTargetPage[]|null An array of EchoTargetPage instances,
or null if not loaded.
* @return EchoNotification
*/
- public static function newFromRow( $row ) {
+ public static function newFromRow( $row, $targetPages = null ) {
$notification = new EchoNotification();
if ( property_exists( $row, 'event_type' ) ) {
@@ -186,9 +209,7 @@
$notification->event = EchoEvent::newFromID(
$row->notification_event );
}
- if ( property_exists( $row, 'etp_event' ) && $row->etp_event ) {
- $notification->targetPage = EchoTargetPage::newFromRow(
$row );
- }
+ $notification->targetPages = $targetPages;
$notification->user = User::newFromId( $row->notification_user
);
// Notification timestamp should never be empty
$notification->timestamp = wfTimestamp( TS_MW,
$row->notification_timestamp );
@@ -276,10 +297,12 @@
}
/**
- * Getter method
- * @return EchoTargetPage|null
+ * Getter method. Returns an array of EchoTargetPages, or null if they
have
+ * not been loaded.
+ *
+ * @return EchoTargetPage[]|null
*/
- public function getTargetPage() {
- return $this->targetPage;
+ public function getTargetPages() {
+ return $this->targetPages;
}
}
diff --git a/tests/phpunit/includes/mapper/NotificationMapperTest.php
b/tests/phpunit/includes/mapper/NotificationMapperTest.php
index b131767..3a67219 100644
--- a/tests/phpunit/includes/mapper/NotificationMapperTest.php
+++ b/tests/phpunit/includes/mapper/NotificationMapperTest.php
@@ -53,7 +53,7 @@
$this->assertEmpty( $res );
// Successful select
- $dbResult = array(
+ $notifDbResult = array(
(object)array (
'event_id' => 1,
'event_type' => 'test_event',
@@ -70,11 +70,25 @@
'notification_bundle_display_hash' =>
'testdisplayhash'
)
);
- $notifMapper = new EchoNotificationMapper(
$this->mockMWEchoDbFactory( array ( 'select' => $dbResult ) ) );
+
+ $tpDbResult = array(
+ (object)array(
+ 'etp_user' => 1, // userid
+ 'etp_page' => 7, // pageid
+ 'etp_event' => 1, // eventid
+ ),
+ );
+
+ $notifMapper = new EchoNotificationMapper(
$this->mockMWEchoDbFactory( array ( 'select' => $notifDbResult ) ) );
$res = $notifMapper->fetchByUser( $this->mockUser(), 10, '',
array() );
$this->assertEmpty( $res );
- $notifMapper = new EchoNotificationMapper(
$this->mockMWEchoDbFactory( array ( 'select' => $dbResult ) ) );
+ $notifMapper = new EchoNotificationMapper(
+ $this->mockMWEchoDbFactory( array ( 'select' =>
$notifDbResult ) ),
+ new EchoTargetPageMapper(
+ $this->mockMWEchoDbFactory( array( 'select' =>
$tpDbResult ) )
+ )
+ );
$res = $notifMapper->fetchByUser( $this->mockUser(), 10, '',
array( 'test_event' ) );
$this->assertTrue( is_array( $res ) );
$this->assertGreaterThan( 0, count( $res ) );
diff --git a/tests/phpunit/includes/mapper/TargetPageMapperTest.php
b/tests/phpunit/includes/mapper/TargetPageMapperTest.php
index f0a3c8b..6358df3 100644
--- a/tests/phpunit/includes/mapper/TargetPageMapperTest.php
+++ b/tests/phpunit/includes/mapper/TargetPageMapperTest.php
@@ -23,8 +23,10 @@
$res = $targetMapper->fetchByUserPageId( User::newFromId( 1 ),
2 );
$this->assertTrue( is_array( $res ) );
$this->assertCount( 2, $res );
- foreach ( $res as $row ) {
- $this->assertInstanceOf( 'EchoTargetPage', $row );
+ foreach ( $res as $targetPages ) {
+ $this->assertTrue( is_array( $targetPages ) );
+ $this->assertCount( 1, $targetPages );
+ $this->assertInstanceOf( 'EchoTargetPage', reset(
$targetPages ) );
}
}
diff --git a/tests/phpunit/model/NotificationTest.php
b/tests/phpunit/model/NotificationTest.php
index 8bb4cf2..267bd25 100644
--- a/tests/phpunit/model/NotificationTest.php
+++ b/tests/phpunit/model/NotificationTest.php
@@ -14,7 +14,7 @@
wfTimestamp( TS_MW, $row['notification_timestamp'] )
);
$this->assertInstanceOf( 'EchoEvent', $notif->getEvent() );
- $this->assertNull( $notif->getTargetPage() );
+ $this->assertNull( $notif->getTargetPages() );
// Provide a read timestamp
$row['notification_read_timestamp'] = time() + 1000;
@@ -25,9 +25,13 @@
wfTimestamp( TS_MW, $row['notification_read_timestamp']
)
);
- $row += $this->mockTargetPageRow();
- $notif = EchoNotification::newFromRow( (object)$row );
- $this->assertInstanceOf( 'EchoTargetPage',
$notif->getTargetPage() );
+ $notif = EchoNotification::newFromRow( (object)$row, array(
+ EchoTargetPage::newFromRow(
(object)$this->mockTargetPageRow() )
+ ) );
+ $this->assertGreaterThan( 0, count( $notif->getTargetPages() )
);
+ foreach ( $notif->getTargetPages() as $targetPage ) {
+ $this->assertInstanceOf( 'EchoTargetPage', $targetPage
);
+ }
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/197069
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I338f3d73efb98a3bb66ef64fdeeb66e752a453c2
Gerrit-PatchSet: 7
Gerrit-Project: mediawiki/extensions/Echo
Gerrit-Branch: master
Gerrit-Owner: EBernhardson <ebernhard...@wikimedia.org>
Gerrit-Reviewer: EBernhardson <ebernhard...@wikimedia.org>
Gerrit-Reviewer: Mattflaschen <mflasc...@wikimedia.org>
Gerrit-Reviewer: Matthias Mullie <mmul...@wikimedia.org>
Gerrit-Reviewer: Springle <sprin...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
