Merge "Avoid event insertion if possible"

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 ] : [] );

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;

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" );
 	}
 
 	/**

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

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

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

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

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

tests/phpunit/integration/EventIntegrationTest.php

@@ -0,0 +1,87 @@
+<?php
+
+use MediaWiki\Extension\Notifications\Mapper\EventMapper;
+use MediaWiki\Extension\Notifications\Model\Event;
+use MediaWiki\User\UserIdentityValue;
+
+/**
+ * @covers \MediaWiki\Extension\Notifications\Model\Event
+ * @covers \MediaWiki\Extension\Notifications\Controller\NotificationController
+ * @covers \MediaWiki\Extension\Notifications\Jobs\NotificationJob
+ * @group Database
+ */
+class EventIntegrationTest extends MediaWikiIntegrationTestCase {
+
+	public function testEventInsertionImmediate() {
+		$this->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() ) );
+	}
+
+}