ensure internalEnqueue can't add the same operation

This fixes a bug where loadSavedOperations would duplicate operations
in OperationRepo.queue. This is because it is possible for operations
to be added before it is started and these are also persisted to disk.

Improved OperationRepoTests by making mockOperationModelStore modify
a in memory list. This was required to write a test to ensure
loadSavedOperations wasn't duplicating operations.

Author: jkasten2

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

@@ -48,7 +48,7 @@ internal class OperationRepo(
     )
 
     private val executorsMap: Map<String, IOperationExecutor>
-    private val queue = mutableListOf<OperationQueueItem>()
+    internal val queue = mutableListOf<OperationQueueItem>()
     private val waiter = WaiterWithValue<LoopWaiterMessage>()
     private var paused = false
     private var coroutineScope = CoroutineScope(newSingleThreadContext(name = "OpRepo"))
@@ -143,6 +143,12 @@ internal class OperationRepo(
         addToStore: Boolean,
         index: Int? = null,
     ) {
+        val hasExisting = queue.any { it.operation.id == queueItem.operation.id }
+        if (hasExisting) {
+            Logging.debug("OperationRepo: internalEnqueue - operation.id: ${queueItem.operation.id} already exists in the queue.")
+            return
+        }
+
         synchronized(queue) {
             if (index != null) {
                 queue.add(index, queueItem)
@@ -402,7 +408,7 @@ internal class OperationRepo(
      * NOTE: Sometimes the loading might take longer than expected due to I/O reads from disk
      *      Any I/O implies executing time will vary greatly.
      */
-    private fun loadSavedOperations() {
+    internal fun loadSavedOperations() {
         _operationModelStore.loadOperations()
         for (operation in _operationModelStore.list().withIndex()) {
             internalEnqueue(

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt

@@ -27,18 +27,24 @@ import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withTimeoutOrNull
 import kotlinx.coroutines.yield
+import java.util.UUID
 
 // Mocks used by every test in this file
 private class Mocks {
     val configModelStore = MockHelper.configModelStore()
 
     val operationModelStore: OperationModelStore =
         run {
+            val operationStoreList = mutableListOf<Operation>()
             val mockOperationModelStore = mockk<OperationModelStore>()
             every { mockOperationModelStore.loadOperations() } just runs
-            every { mockOperationModelStore.list() } returns listOf()
-            every { mockOperationModelStore.add(any()) } just runs
-            every { mockOperationModelStore.remove(any()) } just runs
+            every { mockOperationModelStore.list() } returns operationStoreList
+            every { mockOperationModelStore.add(any()) } answers { operationStoreList.add(firstArg<Operation>()) }
+            every { mockOperationModelStore.remove(any()) } answers {
+                val id = firstArg<String>()
+                val op = operationStoreList.firstOrNull { it.id == id }
+                operationStoreList.remove(op)
+            }
             mockOperationModelStore
         }
 
@@ -117,9 +123,9 @@ class OperationRepoTests : FunSpec({
     test("enqueue operation executes and is removed when executed") {
         // Given
         val mocks = Mocks()
-
         val operationIdSlot = slot<String>()
         val operation = mockOperation(operationIdSlot = operationIdSlot)
+        val opId = operation.id
 
         // When
         mocks.operationRepo.start()
@@ -136,7 +142,7 @@ class OperationRepoTests : FunSpec({
                     it[0] shouldBe operation
                 },
             )
-            mocks.operationModelStore.remove("operationId")
+            mocks.operationModelStore.remove(opId)
         }
     }
 
@@ -151,6 +157,7 @@ class OperationRepoTests : FunSpec({
 
         val operationIdSlot = slot<String>()
         val operation = mockOperation(operationIdSlot = operationIdSlot)
+        val opId = operation.id
 
         // When
         opRepo.start()
@@ -174,7 +181,7 @@ class OperationRepoTests : FunSpec({
                     it[0] shouldBe operation
                 },
             )
-            mocks.operationModelStore.remove("operationId")
+            mocks.operationModelStore.remove(opId)
         }
     }
 
@@ -210,6 +217,7 @@ class OperationRepoTests : FunSpec({
 
         val operationIdSlot = slot<String>()
         val operation = mockOperation(operationIdSlot = operationIdSlot)
+        val opId = operation.id
 
         // When
         mocks.operationRepo.start()
@@ -226,7 +234,7 @@ class OperationRepoTests : FunSpec({
                     it[0] shouldBe operation
                 },
             )
-            mocks.operationModelStore.remove("operationId")
+            mocks.operationModelStore.remove(opId)
         }
     }
 
@@ -608,10 +616,24 @@ class OperationRepoTests : FunSpec({
             spyListener.onOperationRepoLoaded()
         }
     }
+
+    test("ensure loadSavedOperations doesn't duplicate existing OperationItems") {
+        // Given
+        val mocks = Mocks()
+        val op = mockOperation()
+        mocks.operationRepo.enqueue(op)
+
+        // When
+        mocks.operationRepo.loadSavedOperations()
+
+        // Then
+        mocks.operationRepo.queue.size shouldBe 1
+        mocks.operationRepo.queue.first().operation shouldBe op
+    }
 }) {
     companion object {
         private fun mockOperation(
-            id: String = "operationId",
+            id: String = UUID.randomUUID().toString(),
             name: String = "DUMMY_OPERATION",
             canStartExecute: Boolean = true,
             groupComparisonType: GroupComparisonType = GroupComparisonType.NONE,