Merge pull request #937 from nextcloud/bugfix/noid/reduce-delete-push-spam

Only push delete-push to devices that also got the notification

4 files changed, 48 insertions, 32 deletions

diff --git a/lib/App.php b/lib/App.php
index 513a57d..ab8cb21 100644
--- a/lib/App.php
+++ b/lib/App.php
@@ -82,8 +82,8 @@ class App implements IDeferrableApp {
 			$this->push->deferPayloads();
 		}
 		foreach ($deleted as $user => $notifications) {
-			foreach ($notifications as $notificationId) {
-				$this->push->pushDeleteToDevice($user, $notificationId);
+			foreach ($notifications as $data) {
+				$this->push->pushDeleteToDevice($user, $data['id'], $data['app']);
 			}
 		}
 		if (!$isAlreadyDeferring) {
diff --git a/lib/Controller/EndpointController.php b/lib/Controller/EndpointController.php
index 0fc89c5..aca6a9e 100644
--- a/lib/Controller/EndpointController.php
+++ b/lib/Controller/EndpointController.php
@@ -181,10 +181,11 @@ class EndpointController extends OCSController {
 		}
 
 		try {
+			$notification = $this->handler->getById($id, $this->getCurrentUser());
 			$deleted = $this->handler->deleteById($id, $this->getCurrentUser());
 
 			if ($deleted) {
-				$this->push->pushDeleteToDevice($this->getCurrentUser(), $id);
+				$this->push->pushDeleteToDevice($this->getCurrentUser(), $id, $notification->getApp());
 			}
 		} catch (NotificationNotFoundException $e) {
 		}
diff --git a/lib/Handler.php b/lib/Handler.php
index 85c1661..ecca4ab 100644
--- a/lib/Handler.php
+++ b/lib/Handler.php
@@ -100,14 +100,17 @@ class Handler {
 				$deleted[$row['user']] = [];
 			}
 
-			$deleted[$row['user']][] = (int) $row['notification_id'];
+			$deleted[$row['user']][] = [
+				'id' => (int) $row['notification_id'],
+				'app' => $row['app'],
+			];
 			$notifications[(int) $row['notification_id']] = $this->notificationFromRow($row);
 		}
 		$statement->closeCursor();
 
-		foreach ($deleted as $user => $notificationIds) {
-			foreach ($notificationIds as $notificationId) {
-				$this->deleteById($notificationId, $user, $notifications[$notificationId]);
+		foreach ($deleted as $user => $entries) {
+			foreach ($entries as $entry) {
+				$this->deleteById($entry['id'], $user, $notifications[$entry['id']]);
 			}
 		}
 
diff --git a/lib/Push.php b/lib/Push.php
index 7e41b0f..1f7654f 100644
--- a/lib/Push.php
+++ b/lib/Push.php
@@ -124,6 +124,35 @@ class Push {
 		$this->sendNotificationsToProxies();
 	}
 
+	public function filterDeviceList(array $devices, string $app): array {
+		$isTalkNotification = \in_array($app, ['spreed', 'talk', 'admin_notification_talk'], true);
+
+		$talkDevices = array_filter($devices, static function ($device) {
+			return $device['apptype'] === 'talk';
+		});
+		$otherDevices = array_filter($devices, static function ($device) {
+			return $device['apptype'] !== 'talk';
+		});
+
+		$this->printInfo('Identified ' . count($talkDevices) . ' Talk devices and ' . count($otherDevices) . ' others.');
+
+		if (!$isTalkNotification) {
+			if (empty($otherDevices)) {
+				// We only send file notifications to the files app.
+				// If you don't have such a device, bye!
+				return [];
+			}
+			return $otherDevices;
+		}
+
+		if (empty($talkDevices)) {
+			// If you don't have a talk device,
+			// we fall back to the files app.
+			return $otherDevices;
+		}
+		return $talkDevices;
+	}
+
 	public function pushToDevice(int $id, INotification $notification, ?OutputInterface $output = null): void {
 		if (!$this->config->getSystemValueBool('has_internet_connection', true)) {
 			return;
@@ -173,30 +202,9 @@ class Push {
 		$this->printInfo('Public user key size: ' . strlen($userKey->getPublic()));
 
 		$isTalkNotification = \in_array($notification->getApp(), ['spreed', 'talk', 'admin_notification_talk'], true);
-		$talkDevices = array_filter($devices, function ($device) {
-			return $device['apptype'] === 'talk';
-		});
-		$otherDevices = array_filter($devices, function ($device) {
-			return $device['apptype'] !== 'talk';
-		});
-
-		$this->printInfo('Identified ' . count($talkDevices) . ' Talk devices and ' . count($otherDevices) . ' others.');
-
-		if (!$isTalkNotification) {
-			if (empty($otherDevices)) {
-				// We only send file notifications to the files app.
-				// If you don't have such a device, bye!
-				return;
-			}
-			$devices = $otherDevices;
-		} else {
-			if (empty($talkDevices)) {
-				// If you don't have a talk device,
-				// we fall back to the files app.
-				$devices = $otherDevices;
-			} else {
-				$devices = $talkDevices;
-			}
+		$devices = $this->filterDeviceList($devices, $notification->getApp());
+		if (empty($devices)) {
+			return;
 		}
 
 		// We don't push to devices that are older than 60 days
@@ -230,7 +238,7 @@ class Push {
 		}
 	}
 
-	public function pushDeleteToDevice(string $userId, int $notificationId): void {
+	public function pushDeleteToDevice(string $userId, int $notificationId, string $app = ''): void {
 		if (!$this->config->getSystemValueBool('has_internet_connection', true)) {
 			return;
 		}
@@ -241,6 +249,10 @@ class Push {
 		}
 
 		$devices = $this->getDevicesForUser($userId);
+		if ($notificationId !== 0 && $app !== '') {
+			// Only filter when it's not a single delete
+			$devices = $this->filterDeviceList($devices, $app);
+		}
 		if (empty($devices)) {
 			return;
 		}