Merge pull request #938 from nextcloud/backport/937/stable21v21.0.2RC1v21.0.2

[stable21] Only push delete-push to devices that also got the notification

5 files changed, 50 insertions, 34 deletions

diff --git a/lib/App.php b/lib/App.php
index a7cfcc0..8b0555e 100644
--- a/lib/App.php
+++ b/lib/App.php
@@ -78,8 +78,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 8048e9e..df56b4b 100644
--- a/lib/Controller/EndpointController.php
+++ b/lib/Controller/EndpointController.php
@@ -174,10 +174,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 cd505f6..71b4606 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 902f414..116b4fc 100644
--- a/lib/Push.php
+++ b/lib/Push.php
@@ -121,6 +121,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;
@@ -170,30 +199,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
@@ -227,7 +235,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;
 		}
@@ -238,6 +246,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;
 		}
diff --git a/tests/Unit/PushTest.php b/tests/Unit/PushTest.php
index 63497ba..7ef71b7 100644
--- a/tests/Unit/PushTest.php
+++ b/tests/Unit/PushTest.php
@@ -646,11 +646,11 @@ class PushTest extends TestCase {
 			->willReturn('valid');
 
 		if ($isTalkNotification) {
-			$notification->expects($this->once())
+			$notification->expects($this->any())
 				->method('getApp')
 				->willReturn('spreed');
 		} else {
-			$notification->expects($this->once())
+			$notification->expects($this->any())
 				->method('getApp')
 				->willReturn('notifications');
 		}