From 4c875027a6ec1c0ca11931d2f3ad6d4874226d65 Mon Sep 17 00:00:00 2001
From: cketti <cketti@gmail.com>
Date: Thu, 3 Feb 2022 02:16:06 +0100
Subject: [PATCH] Crash app when trying to add duplicate notification on debug
 builds

This is not a critical error. So we're not crashing release builds. We could skip the duplicate notification in release builds. But the hope is that users will notice them and report a bug, allowing us to find and fix the root cause rather than the symptom.
---
 .../k9/notification/NotificationDataStore.kt  | 15 ++++++--
 .../notification/NotificationDataStoreTest.kt | 34 +++++++++++++++++++
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/app/core/src/main/java/com/fsck/k9/notification/NotificationDataStore.kt b/app/core/src/main/java/com/fsck/k9/notification/NotificationDataStore.kt
index 51372b0a9..50cd1be99 100644
--- a/app/core/src/main/java/com/fsck/k9/notification/NotificationDataStore.kt
+++ b/app/core/src/main/java/com/fsck/k9/notification/NotificationDataStore.kt
@@ -2,6 +2,7 @@ package com.fsck.k9.notification
 
 import com.fsck.k9.Account
 import com.fsck.k9.controller.MessageReference
+import com.fsck.k9.core.BuildConfig
 
 internal const val MAX_NUMBER_OF_NEW_MESSAGE_NOTIFICATIONS = 8
 
@@ -31,6 +32,11 @@ internal class NotificationDataStore {
     @Synchronized
     fun addNotification(account: Account, content: NotificationContent, timestamp: Long): AddNotificationResult {
         val notificationData = getNotificationData(account)
+        val messageReference = content.messageReference
+
+        if (BuildConfig.DEBUG && notificationData.contains(messageReference)) {
+            throw AssertionError("Notification for message $messageReference already exists")
+        }
 
         return if (notificationData.isMaxNumberOfActiveNotificationsReached) {
             val lastNotificationHolder = notificationData.activeNotifications.last()
@@ -41,7 +47,7 @@ internal class NotificationDataStore {
 
             val operations = listOf(
                 NotificationStoreOperation.ChangeToInactive(lastNotificationHolder.content.messageReference),
-                NotificationStoreOperation.Add(content.messageReference, notificationId, timestamp)
+                NotificationStoreOperation.Add(messageReference, notificationId, timestamp)
             )
 
             val newNotificationData = notificationData.copy(
@@ -56,7 +62,7 @@ internal class NotificationDataStore {
             val notificationHolder = NotificationHolder(notificationId, timestamp, content)
 
             val operations = listOf(
-                NotificationStoreOperation.Add(content.messageReference, notificationId, timestamp)
+                NotificationStoreOperation.Add(messageReference, notificationId, timestamp)
             )
 
             val newNotificationData = notificationData.copy(
@@ -169,6 +175,11 @@ internal class NotificationDataStore {
         throw AssertionError("getNewNotificationId() called with no free notification ID")
     }
 
+    private fun NotificationData.contains(messageReference: MessageReference): Boolean {
+        return activeNotifications.any { it.content.messageReference == messageReference } ||
+            inactiveNotifications.any { it.content.messageReference == messageReference }
+    }
+
     private fun NotificationHolder.toInactiveNotificationHolder() = InactiveNotificationHolder(timestamp, content)
 
     private fun InactiveNotificationHolder.toNotificationHolder(notificationId: Int): NotificationHolder {
diff --git a/app/core/src/test/java/com/fsck/k9/notification/NotificationDataStoreTest.kt b/app/core/src/test/java/com/fsck/k9/notification/NotificationDataStoreTest.kt
index 9bb2c110b..a07463909 100644
--- a/app/core/src/test/java/com/fsck/k9/notification/NotificationDataStoreTest.kt
+++ b/app/core/src/test/java/com/fsck/k9/notification/NotificationDataStoreTest.kt
@@ -3,8 +3,12 @@ package com.fsck.k9.notification
 import com.fsck.k9.Account
 import com.fsck.k9.RobolectricTest
 import com.fsck.k9.controller.MessageReference
+import com.fsck.k9.core.BuildConfig
 import com.google.common.truth.Truth.assertThat
 import kotlin.test.assertNotNull
+import org.junit.Assert.fail
+import org.junit.Assume.assumeFalse
+import org.junit.Assume.assumeTrue
 import org.junit.Test
 
 private const val ACCOUNT_UUID = "1-2-3"
@@ -127,6 +131,36 @@ class NotificationDataStoreTest : RobolectricTest() {
         assertThat(addResult.notificationData.activeNotifications.first()).isEqualTo(addResult.notificationHolder)
     }
 
+    @Test
+    fun `adding notification for same message twice should throw in debug build`() {
+        assumeTrue(BuildConfig.DEBUG)
+
+        val content1 = createNotificationContent("1")
+        val content2 = createNotificationContent("1")
+        notificationDataStore.addNotification(account, content1, TIMESTAMP)
+
+        try {
+            notificationDataStore.addNotification(account, content2, TIMESTAMP)
+            fail("Exception expected")
+        } catch (e: AssertionError) {
+            assertThat(e).hasMessageThat().matches("Notification for message .+ already exists")
+        }
+    }
+
+    @Test
+    fun `adding notification for same message twice should add another notification in release build`() {
+        assumeFalse(BuildConfig.DEBUG)
+
+        val content1 = createNotificationContent("1")
+        val content2 = createNotificationContent("1")
+
+        val addResult1 = notificationDataStore.addNotification(account, content1, TIMESTAMP)
+        val addResult2 = notificationDataStore.addNotification(account, content2, TIMESTAMP)
+
+        assertThat(addResult1.notificationHolder.notificationId)
+            .isNotEqualTo(addResult2.notificationHolder.notificationId)
+    }
+
     private fun createAccount(): Account {
         return Account("00000000-0000-4000-0000-000000000000").apply {
             accountNumber = ACCOUNT_NUMBER