From d18750b6fb9967f5cd1973cc8a088fe34f41674c Mon Sep 17 00:00:00 2001
From: Stephane Bisson <sbisson@wikimedia.org>
Date: Wed, 3 Aug 2016 11:41:50 -0400
Subject: [PATCH] Prevent duplicate thank-you-edit notifications

Avoid duplicate by looking if the notification
already exist.

Change-Id: Ie519d13cd0ab03919f70da90d58c82a214c74b49
Depends-On: I79194c41d6b2fd84ad658909a2941d9d3d28d94e
Bug: T128249
---
 Hooks.php                          | 39 ++++++++++----------
 autoload.php                       |  1 +
 tests/phpunit/ThankYouEditTest.php | 58 ++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 20 deletions(-)
 create mode 100644 tests/phpunit/ThankYouEditTest.php

diff --git a/Hooks.php b/Hooks.php
index 9d94c5f36..552f80a51 100644
--- a/Hooks.php
+++ b/Hooks.php
@@ -503,28 +503,27 @@ class EchoHooks {
 		// test for them reaching a congratulatory threshold
 		$thresholds = [ 1, 10, 100, 1000, 10000, 100000, 1000000 ];
 		if ( $user->isLoggedIn() && $status->value['revision'] ) {
-			// This edit hasn't been added to the edit count yet
-			$thresholdCount = $user->getEditCount() + 1;
+			$thresholdCount = $user->getEditCount();
 			if ( in_array( $thresholdCount, $thresholds ) ) {
-				$id = $user->getId();
-				DeferredUpdates::addCallableUpdate( function () use ( $id, $title, $thresholdCount ) {
-					// Fresh User object
-					$user = User::newFromId( $id );
-					$userEditCount = $user->getEditCount();
-					if ( (int)$userEditCount !== (int)$thresholdCount ) {
-						// Race condition with multiple simultaneous requests, skip
-						LoggerFactory::getInstance( 'Echo' )->debug(
-							'thank-you-edit race condition detected: {user} (id: {id}) should ' .
-							'have had {expectedCount} edits but has {actualCount}',
-							array(
-								'user' => $user->getName(),
-								'id' => $user->getId(),
-								'expectedCount' => $thresholdCount,
-								'actualCount' => $userEditCount,
-							)
-						);
-						return;
+				DeferredUpdates::addCallableUpdate( function () use ( $user, $title, $thresholdCount ) {
+
+					$notificationMapper = new EchoNotificationMapper();
+					$notifications = $notificationMapper->fetchByUser( $user, 10, null, array( 'thank-you-edit' ) );
+					/** @var EchoNotification $notification */
+					foreach ( $notifications as $notification ) {
+						if ( $notification->getEvent()->getExtraParam( 'editCount' ) === $thresholdCount ) {
+							LoggerFactory::getInstance( 'Echo' )->debug(
+								'{user} (id: {id}) has already been thanked for their {count} edit',
+								array(
+									'user' => $user->getName(),
+									'id' => $user->getId(),
+									'count' => $thresholdCount,
+								)
+							);
+							return;
+						}
 					}
+
 					LoggerFactory::getInstance( 'Echo' )->debug(
 						'Thanking {user} (id: {id}) for their {count} edit',
 						array(
diff --git a/autoload.php b/autoload.php
index 152db8edb..589275885 100644
--- a/autoload.php
+++ b/autoload.php
@@ -105,6 +105,7 @@ $wgAutoloadClasses += [
 	'MWEchoEventLogging' => __DIR__ . '/includes/EventLogging.php',
 	'MWEchoNotifUser' => __DIR__ . '/includes/NotifUser.php',
 	'MWEchoNotifUserTest' => __DIR__ . '/tests/phpunit/NotifUserTest.php',
+	'MWEchoThankYouEditTest' => __DIR__ . '/tests/phpunit/ThankYouEditTest.php',
 	'NotificationControllerTest' => __DIR__ . '/tests/phpunit/controller/NotificationControllerTest.php',
 	'NotificationPager' => __DIR__ . '/includes/special/NotificationPager.php',
 	'NotificationsTest' => __DIR__ . '/tests/NotificationsTest.php',
diff --git a/tests/phpunit/ThankYouEditTest.php b/tests/phpunit/ThankYouEditTest.php
new file mode 100644
index 000000000..1df66e206
--- /dev/null
+++ b/tests/phpunit/ThankYouEditTest.php
@@ -0,0 +1,58 @@
+<?php
+
+/**
+ * @group Echo
+ */
+class MWEchoThankYouEditTest extends MediaWikiTestCase {
+
+	protected function setUp() {
+		parent::setUp();
+	}
+
+	public function testFirstEdit() {
+		// setup
+		$user = $this->getMutableTestUser()->getUser();
+		$title = Title::newFromText( 'Help:MWEchoThankYouEditTest_testFirstEdit' );
+
+		// action
+		$this->edit( $title, $user, 'this is my first edit' );
+
+		// assertions
+		$notificationMapper = new EchoNotificationMapper();
+		$notifications = $notificationMapper->fetchByUser( $user, 10, null, array( 'thank-you-edit' ) );
+		$this->assertCount( 1, $notifications );
+
+		/** @var EchoNotification $notification */
+		$notification = reset( $notifications );
+		$this->assertEquals( 1, $notification->getEvent()->getExtraParam( 'editCount', 'not found' ) );
+	}
+
+	public function testTenthEdit() {
+		// setup
+		$user = $this->getMutableTestUser()->getUser();
+		$title = Title::newFromText( 'Help:MWEchoThankYouEditTest_testTenthEdit' );
+
+		// action
+		// we could fast-forward the edit-count to speed things up
+		// but this is the only way to make sure duplicate notifications
+		// are not generated
+		for ( $i = 0; $i < 12; $i++ ) {
+			$this->edit( $title, $user, "this is edit #$i" );
+		}
+
+		// assertions
+		$notificationMapper = new EchoNotificationMapper();
+		$notifications = $notificationMapper->fetchByUser( $user, 10, null, array( 'thank-you-edit' ) );
+		$this->assertCount( 2, $notifications );
+
+		/** @var EchoNotification $notification */
+		$notification = reset( $notifications );
+		$this->assertEquals( 10, $notification->getEvent()->getExtraParam( 'editCount', 'not found' ) );
+	}
+
+	private function edit( Title $title, User $user, $text ) {
+		$page = WikiPage::factory( $title );
+		$content = ContentHandler::makeContent( $text, $title );
+		$page->doEditContent( $content, 'test', 0, false, $user );
+	}
+}