diff --git a/includes/Controller/NotificationController.php b/includes/Controller/NotificationController.php index 22228c0c7..2ee8693e0 100644 --- a/includes/Controller/NotificationController.php +++ b/includes/Controller/NotificationController.php @@ -150,7 +150,7 @@ class NotificationController { } $hookRunner->onEchoGetNotificationTypes( $user, $event, $userNotifyTypes ); - // types such as web, email, etc + // types such as web, email, etc. foreach ( $userNotifyTypes as $type ) { self::doNotification( $event, $user, $type ); } @@ -191,9 +191,9 @@ class NotificationController { if ( !$rev ) { $logger = LoggerFactory::getInstance( 'Echo' ); $logger->debug( - 'Notifying for event {eventId}. Revision \'{revId}\' not found.', + 'Notifying for event type \'{eventType}\'. Revision \'{revId}\' not found.', [ - 'eventId' => $event->getId(), + 'eventType' => $event->getType(), 'revId' => $revId, ] ); @@ -253,7 +253,10 @@ class NotificationController { $rootJobSignature = $event->getExtraParam( 'rootJobSignature' ); $rootJobTimestamp = $event->getExtraParam( 'rootJobTimestamp' ); - return [ 'eventId' => $event->getId() ] + $params = $event->getId() + ? [ 'eventId' => $event->getId() ] + : [ 'eventData' => $event->toDbArray() ]; + return $params + ( $delay ? [ 'jobReleaseTimestamp' => (int)wfTimestamp() + $delay ] : [] ) + ( $rootJobSignature ? [ 'rootJobSignature' => $rootJobSignature ] : [] ) + ( $rootJobTimestamp ? [ 'rootJobTimestamp' => $rootJobTimestamp ] : [] ); diff --git a/includes/Jobs/NotificationJob.php b/includes/Jobs/NotificationJob.php index e2401fba2..ca3e7d058 100644 --- a/includes/Jobs/NotificationJob.php +++ b/includes/Jobs/NotificationJob.php @@ -5,6 +5,7 @@ namespace MediaWiki\Extension\Notifications\Jobs; use Job; use MediaWiki\Extension\Notifications\Controller\NotificationController; use MediaWiki\Extension\Notifications\Mapper\EventMapper; +use MediaWiki\Extension\Notifications\Model\Event; use MediaWiki\Title\Title; class NotificationJob extends Job { @@ -15,8 +16,12 @@ class NotificationJob extends Job { } public function run() { - $eventMapper = new EventMapper(); - $event = $eventMapper->fetchById( $this->params['eventId'], true ); + if ( isset( $this->params['eventId'] ) ) { + $eventMapper = new EventMapper(); + $event = $eventMapper->fetchById( $this->params['eventId'], true ); + } else { + $event = Event::newFromArray( $this->params['eventData'] ); + } NotificationController::notify( $event, false ); return true; diff --git a/includes/Model/Event.php b/includes/Model/Event.php index 69d3f38b2..cf83b3884 100644 --- a/includes/Model/Event.php +++ b/includes/Model/Event.php @@ -84,6 +84,9 @@ class Event extends AbstractEntity implements Bundleable { */ protected $deleted = 0; + /** For use in tests */ + public static bool $alwaysInsert = false; + /** * You should not call the constructor. * Instead, use one of the factory functions: @@ -198,19 +201,14 @@ class Event extends AbstractEntity implements Bundleable { } // @Todo - Database insert logic should not be inside the model - $obj->insert(); - - $hookRunner->onEventInsertComplete( $obj ); + if ( self::$alwaysInsert ) { + $obj->insert(); + } global $wgEchoUseJobQueue; NotificationController::notify( $obj, $wgEchoUseJobQueue ); - $stats = $services->getStatsdDataFactory(); - $type = $info['type']; - $stats->increment( 'echo.event.all' ); - $stats->increment( "echo.event.$type" ); - return $obj; } @@ -250,6 +248,34 @@ class Event extends AbstractEntity implements Bundleable { return $data; } + /** + * Creates an Event from an array. The array should be output from ::toDbArray(). + * + * @param array $data + * @return self + */ + public static function newFromArray( $data ) { + $obj = new self(); + if ( isset( $data['event_id'] ) ) { + $obj->id = $data['event_id']; + } + $obj->type = $data['event_type']; + $obj->variant = $data['event_variant']; + $obj->extra = $data['event_extra'] ? unserialize( $data['event_extra'] ) : []; + if ( isset( $data['event_page_id'] ) ) { + $obj->pageId = $data['event_page_id']; + } + $obj->deleted = $data['event_deleted']; + + if ( $data['event_agent_id'] ?? 0 ) { + $obj->agent = User::newFromId( $data['event_agent_id'] ); + } elseif ( isset( $data['event_agent_ip'] ) ) { + $obj->agent = User::newFromName( $data['event_agent_ip'], false ); + } + + return $obj; + } + /** * Check whether the echo event is an enabled event * @return bool @@ -259,6 +285,19 @@ class Event extends AbstractEntity implements Bundleable { return isset( $wgEchoNotifications[$this->getType()] ); } + /** + * Insert this event into the database if it hasn't been yet, and return its id. + * Subsequent calls on this instance will not cause repeated insertion + * and will always return the same id. + * @return int + */ + public function acquireId() { + if ( !$this->id ) { + $this->insert(); + } + return $this->id; + } + /** * Inserts the object into the database. */ @@ -276,6 +315,15 @@ class Event extends AbstractEntity implements Bundleable { } } } + + $services = MediaWikiServices::getInstance(); + $hookRunner = new HookRunner( $services->getHookContainer() ); + $hookRunner->onEventInsertComplete( $this ); + + $stats = $services->getStatsdDataFactory(); + $type = $this->getType(); + $stats->increment( 'echo.event.all' ); + $stats->increment( "echo.event.$type" ); } /** diff --git a/includes/Model/Notification.php b/includes/Model/Notification.php index 7aabffcd1..f99144635 100644 --- a/includes/Model/Notification.php +++ b/includes/Model/Notification.php @@ -129,6 +129,7 @@ class Notification extends AbstractEntity implements Bundleable { } ); + $this->event->acquireId(); $notifMapper->insert( $this ); if ( $this->event->getCategory() === 'edit-user-talk' ) { diff --git a/includes/Notifier.php b/includes/Notifier.php index a955705d9..bd345533b 100644 --- a/includes/Notifier.php +++ b/includes/Notifier.php @@ -81,7 +81,7 @@ class Notifier { } } } elseif ( $event->getExtraParam( 'noemail' ) ) { - // Could be set for API triggered notifications were email is not + // Could be set for API triggered notifications where email is not // requested in API request params return false; } @@ -114,14 +114,15 @@ class Notifier { $bundleHash = md5( $bundleString ); } - // email digest notification ( weekly or daily ) + // email digest notification (weekly or daily) if ( $wgEchoEnableEmailBatch && $userOptionsLookup->getOption( $user, 'echo-email-frequency' ) > 0 ) { // always create a unique event hash for those events don't support bundling // this is mainly for group by + $eventId = $event->acquireId(); if ( !$bundleHash ) { - $bundleHash = md5( $event->getType() . '-' . $event->getId() ); + $bundleHash = md5( $event->getType() . '-' . $eventId ); } - EmailBatch::addToQueue( $user->getId(), $event->getId(), $priority, $bundleHash ); + EmailBatch::addToQueue( $user->getId(), $eventId, $priority, $bundleHash ); return true; } diff --git a/includes/Push/PushNotifier.php b/includes/Push/PushNotifier.php index 770d99b7b..12b4c7b12 100644 --- a/includes/Push/PushNotifier.php +++ b/includes/Push/PushNotifier.php @@ -33,7 +33,6 @@ class PushNotifier { $params = [ 'centralId' => $centralId ]; // below params are only needed for debug logging (T255068) if ( $event !== null ) { - $params['eventId'] = $event->getId(); $params['eventType'] = $event->getType(); if ( $event->getAgent() !== null ) { $params['agent'] = $event->getAgent()->getId(); diff --git a/tests/phpunit/Controller/NotificationControllerTest.php b/tests/phpunit/Controller/NotificationControllerTest.php index 04e732369..55fa73a90 100644 --- a/tests/phpunit/Controller/NotificationControllerTest.php +++ b/tests/phpunit/Controller/NotificationControllerTest.php @@ -270,8 +270,13 @@ class NotificationControllerTest extends MediaWikiIntegrationTestCase { ->method( 'getTitle' ) ->willReturn( Title::newFromText( 'test-title' ) ); $event->expects( $this->once() ) - ->method( 'getId' ) - ->willReturn( 42 ); + ->method( 'toDbArray' ) + ->willReturn( [ + 'event_type' => 'test', + 'event_extra' => [ + 'extra-key' => 'extra' + ], + ] ); NotificationController::enqueueEvent( $event ); $jobQueueGroup = $this->getServiceContainer()->getJobQueueGroup(); $queues = $jobQueueGroup->getQueuesWithJobs(); @@ -279,7 +284,15 @@ class NotificationControllerTest extends MediaWikiIntegrationTestCase { $this->assertEquals( 'EchoNotificationJob', $queues[0] ); $job = $jobQueueGroup->pop( 'EchoNotificationJob' ); $this->assertEquals( 'Test-title', $job->params[ 'title' ] ); - $this->assertEquals( 42, $job->params[ 'eventId' ] ); + $this->assertEquals( + [ + 'event_type' => 'test', + 'event_extra' => [ + 'extra-key' => 'extra' + ], + ], + $job->params[ 'eventData' ] + ); } public function testNotSupportedDelay() { @@ -315,12 +328,26 @@ class NotificationControllerTest extends MediaWikiIntegrationTestCase { [ 'rootJobTimestamp', null, $rootJobTimestamp ] ] ); $event->expects( $this->once() ) - ->method( 'getId' ) - ->willReturn( 42 ); + ->method( 'toDbArray' ) + ->willReturn( [ + 'event_type' => 'test', + 'event_extra' => [ + 'delay' => 10, + 'rootJobSignature' => 'test-signature', + 'rootJobTimestamp' => $rootJobTimestamp, + ], + ] ); $params = NotificationController::getEventParams( $event ); $expectedParams = [ - 'eventId' => 42, + 'eventData' => [ + 'event_type' => 'test', + 'event_extra' => [ + 'delay' => 10, + 'rootJobSignature' => 'test-signature', + 'rootJobTimestamp' => $rootJobTimestamp, + ], + ], 'rootJobSignature' => 'test-signature', 'rootJobTimestamp' => $rootJobTimestamp, 'jobReleaseTimestamp' => 10 diff --git a/tests/phpunit/TalkPageFunctionalTest.php b/tests/phpunit/TalkPageFunctionalTest.php index 34bc5e5ca..0a628fa4b 100644 --- a/tests/phpunit/TalkPageFunctionalTest.php +++ b/tests/phpunit/TalkPageFunctionalTest.php @@ -11,12 +11,20 @@ use MediaWiki\Tests\Api\ApiTestCase; * @group medium */ class TalkPageFunctionalTest extends ApiTestCase { + + protected function tearDown(): void { + Event::$alwaysInsert = false; + parent::tearDown(); + } + /** * Creates and updates a user talk page a few times to ensure proper events are * created. * @covers \MediaWiki\Extension\Notifications\DiscussionParser */ public function testAddCommentsToTalkPage() { + Event::$alwaysInsert = true; + $editor = $this->getTestSysop()->getUser(); $talkTitle = $this->getTestSysop()->getUser()->getTalkPage(); $talkPage = $this->getServiceContainer()->getWikiPageFactory()->newFromTitle( $talkTitle ); diff --git a/tests/phpunit/integration/EventIntegrationTest.php b/tests/phpunit/integration/EventIntegrationTest.php new file mode 100644 index 000000000..d5d27ea70 --- /dev/null +++ b/tests/phpunit/integration/EventIntegrationTest.php @@ -0,0 +1,87 @@ +clearHook( 'BeforeEchoEventInsert' ); + $this->overrideConfigValue( 'EchoUseJobQueue', false ); + + $user = $this->getTestUser()->getUser(); + $event = Event::create( [ + 'type' => 'welcome', + 'agent' => $user, + 'extra' => [ 'key' => 'value' ] + ] ); + $eventId = $event->getId(); + $this->assertNotFalse( $eventId ); + $this->assertSame( $eventId, $event->acquireId() ); + $this->assertSelect( + 'echo_event', + [ 'count' => 'COUNT(*)' ], + [ 'event_type' => 'welcome' ], + [ [ '1' ] ] + ); + } + + public function testEventNotInserted() { + $this->clearHook( 'BeforeEchoEventInsert' ); + $this->overrideConfigValue( 'EchoUseJobQueue', false ); + + $event = Event::create( [ + 'type' => 'welcome', + // anons cannot be notified + 'agent' => UserIdentityValue::newAnonymous( '1.2.3.4' ), + 'extra' => [ 'key' => 'value' ] + ] ); + $this->assertFalse( $event->getId() ); + $this->assertSelect( + 'echo_event', + [ 'count' => 'COUNT(*)' ], + [ 'event_type' => 'welcome' ], + [ [ '0' ] ] + ); + } + + public function testEventInsertionDeferred() { + $this->clearHook( 'BeforeEchoEventInsert' ); + $this->clearHook( 'PageSaveComplete' ); + $this->overrideConfigValue( 'EchoUseJobQueue', true ); + + $user = $this->getTestUser()->getUser(); + $title = $this->getExistingTestPage()->getTitle(); + $this->runJobs(); + + $event = Event::create( [ + 'type' => 'welcome', + 'agent' => $user, + 'title' => $title, + 'extra' => [ 'key' => 'value' ] + ] ); + $this->assertFalse( $event->getId() ); + unset( $event ); + + $jobQueueGroup = $this->getServiceContainer()->getJobQueueGroup(); + $queues = $jobQueueGroup->getQueuesWithJobs(); + $this->assertSame( [ 'EchoNotificationJob' ], $queues ); + $job = $jobQueueGroup->pop( 'EchoNotificationJob' ); + $job->run(); + + $eventMapper = new EventMapper(); + $events = $eventMapper->fetchByPage( $title->getArticleID() ); + $this->assertCount( 1, $events ); + [ $event ] = $events; + $this->assertSame( 'welcome', $event->getType() ); + $this->assertTrue( $user->equals( $event->getAgent() ) ); + } + +}