LOOP-1938: Acknowledge all Alerts given an identifier (#259)

* LOOP-1938: Acknowledge all given an identifier

I thought it would be sufficient to only acknowledge the latest Alert, but LOOP-1938 points out that if an alert repeats without (prior issued Alerts) getting acknowledged, those prior Alerts get "stuck" and will replay on restart until they all are acknowledged.
Becky convinced me that, conceptually, if you get multiple issues of the _same_ alert, recording an acknowledgement date on all of them is appropriate.

* checkpoint

* Unit tests & refactoring

Author: Rick Pasetto

Loop/Managers/Alerts/AlertStore.swift

@@ -11,6 +11,8 @@ import LoopKit
 
 public class AlertStore {
 
+    static let totalFetchLimit = 500
+    
     public enum AlertStoreError: Error {
         case notFound
     }
@@ -87,18 +89,18 @@ public class AlertStore {
 
     public func recordAcknowledgement(of identifier: Alert.Identifier, at date: Date = Date(),
                                       completion: ((Result<Void, Error>) -> Void)? = nil) {
-        recordUpdateOfLatest(of: identifier,
-                             addingPredicate: NSPredicate(format: "acknowledgedDate == nil"),
-                             with: {
-                                $0.acknowledgedDate = date
-                                return .save
-                             },
-                             completion: completion)
+        recordUpdateOfAll(identifier: identifier,
+                          addingPredicate: NSPredicate(format: "acknowledgedDate == nil"),
+                          with: {
+                              $0.acknowledgedDate = date
+                              return .save
+                          },
+                          completion: completion)
     }
 
     public func recordRetraction(of identifier: Alert.Identifier, at date: Date = Date(),
                                  completion: ((Result<Void, Error>) -> Void)? = nil) {
-        recordUpdateOfLatest(of: identifier,
+        recordUpdateOfLatest(identifier: identifier,
                              addingPredicate: NSPredicate(format: "retractedDate == nil"),
                              with: {
                                 // if the alert was retracted before it was ever shown, delete it.
@@ -135,29 +137,40 @@ public class AlertStore {
 // MARK: Private functions
 
 extension AlertStore {
-
-    private func recordUpdateOfLatest(of identifier: Alert.Identifier,
+    
+    private func recordUpdateOfAll(identifier: Alert.Identifier,
+                                   addingPredicate predicate: NSPredicate,
+                                   with updateBlock: @escaping ManagedObjectUpdateBlock,
+                                   completion: ((Result<Void, Error>) -> Void)?) {
+        managedObjectContext.perform {
+            self.lookupAll(identifier: identifier, predicate: predicate) {
+                switch $0 {
+                case .success(let objects):
+                    if objects.count > 0 {
+                        let result = self.update(objects: objects, with: updateBlock)
+                        completion?(result)
+                    } else {
+                        self.log.error("Alert not found for update: %{public}@", identifier.value)
+                        completion?(.failure(AlertStoreError.notFound))
+                    }
+                case .failure(let error):
+                    completion?(.failure(error))
+                }
+            }
+        }
+    }
+    
+    private func recordUpdateOfLatest(identifier: Alert.Identifier,
                                       addingPredicate predicate: NSPredicate,
-                                      with block: @escaping ManagedObjectUpdateBlock,
+                                      with updateBlock: @escaping ManagedObjectUpdateBlock,
                                       completion: ((Result<Void, Error>) -> Void)?) {
-        self.managedObjectContext.perform {
+        managedObjectContext.perform {
             self.lookupLatest(identifier: identifier, predicate: predicate) {
                 switch $0 {
                 case .success(let object):
                     if let object = object {
-                        let shouldDelete = block(object) == .delete
-                        do {
-                            if shouldDelete {
-                                self.managedObjectContext.delete(object)
-                            }
-                            try self.managedObjectContext.save()
-                            self.log.default("%{public}@ alert: %{public}@", shouldDelete ? "Deleted" : "Recorded", identifier.value)
-                            self.purgeExpired()
-                            completion?(.success)
-                        } catch {
-                            self.log.error("Could not store alert: %{public}@, %{public}@", identifier.value, String(describing: error))
-                            completion?(.failure(error))
-                        }
+                        let result = self.update(objects: [object], with: updateBlock)
+                        completion?(result)
                     } else {
                         self.log.error("Alert not found for update: %{public}@", identifier.value)
                         completion?(.failure(AlertStoreError.notFound))
@@ -168,6 +181,41 @@ extension AlertStore {
             }
         }
     }
+    
+    private func update(objects: [StoredAlert], with updateBlock: @escaping ManagedObjectUpdateBlock) -> Result<Void, Error> {
+        objects.forEach { alert in
+            let shouldDelete = updateBlock(alert) == .delete
+            if shouldDelete {
+                self.managedObjectContext.delete(alert)
+            }
+            self.log.default("%{public}@ alert: %{public}@", shouldDelete ? "Deleted" : "Recorded", alert.identifier.value)
+        }
+        do {
+            try self.managedObjectContext.save()
+        } catch {
+            return .failure(error)
+        }
+        self.purgeExpired()
+        return .success
+    }
+    
+
+    private func lookupAll(identifier: Alert.Identifier, predicate: NSPredicate, completion: @escaping (Result<[StoredAlert], Error>) -> Void) {
+        managedObjectContext.perform {
+            do {
+                let fetchRequest: NSFetchRequest<StoredAlert> = StoredAlert.fetchRequest()
+                fetchRequest.predicate = NSCompoundPredicate(andPredicateWithSubpredicates: [
+                    identifier.equalsPredicate,
+                    predicate
+                ])
+                fetchRequest.fetchLimit = Self.totalFetchLimit
+                let result = try self.managedObjectContext.fetch(fetchRequest)
+                completion(.success(result))
+            } catch {
+                completion(.failure(error))
+            }
+        }
+    }
 
     private func lookupLatest(identifier: Alert.Identifier, predicate: NSPredicate, completion: @escaping (Result<StoredAlert?, Error>) -> Void) {
         managedObjectContext.perform {

LoopTests/Managers/Alerts/AlertStoreTests.swift

@@ -135,7 +135,7 @@ class AlertStoreTests: XCTestCase {
 
     func testRecordAcknowledgedOfInvalid() {
         let expect = self.expectation(description: #function)
-        self.alertStore.recordAcknowledgement(of: Self.identifier1, at:  Self.historicDate) {
+        self.alertStore.recordAcknowledgement(of: Self.identifier1, at: Self.historicDate) {
             switch $0 {
             case .failure: break
             case .success: XCTFail("Unexpected success")
@@ -504,9 +504,9 @@ class AlertStoreTests: XCTestCase {
         let expect = self.expectation(description: #function)
         let now = Date()
         fillWith(startDate: Self.historicDate, data: [
-            (alert1, false, false),
+            (alert1, true, false),
             (alert2, false, false),
-            (alert1, true, false)
+            (alert1, false, false)
         ]) {
             self.alertStore.recordAcknowledgement(of: self.alert1.identifier, at: now, completion: self.expectSuccess {
                 self.alertStore.fetch(completion: self.expectSuccess { storedAlerts in
@@ -515,7 +515,7 @@ class AlertStoreTests: XCTestCase {
                     XCTAssertNotNil(storedAlerts.last)
                     if let last = storedAlerts.last {
                         XCTAssertEqual(Self.identifier1, last.identifier)
-                        XCTAssertEqual(Self.historicDate, last.issuedDate)
+                        XCTAssertEqual(Self.historicDate + 4, last.issuedDate)
                         XCTAssertEqual(now, last.acknowledgedDate)
                         XCTAssertNil(last.retractedDate)
                     }
@@ -526,6 +526,28 @@ class AlertStoreTests: XCTestCase {
         wait(for: [expect], timeout: 1)
     }
 
+    func testAcknowledgeMultiple() {
+        let expect = self.expectation(description: #function)
+        let now = Date()
+        fillWith(startDate: Self.historicDate, data: [
+            (alert1, false, false),
+            (alert2, false, false),
+            (alert1, false, false)
+        ]) {
+            self.alertStore.recordAcknowledgement(of: self.alert1.identifier, at: now, completion: self.expectSuccess {
+                self.alertStore.fetch(completion: self.expectSuccess { storedAlerts in
+                    XCTAssertEqual(3, storedAlerts.count)
+                    for alert in storedAlerts where alert.identifier == Self.identifier1 {
+                        XCTAssertEqual(now, alert.acknowledgedDate)
+                        XCTAssertNil(alert.retractedDate)
+                    }
+                    expect.fulfill()
+                })
+            })
+        }
+        wait(for: [expect], timeout: 1)
+    }
+    
     func testLookupAllUnacknowledgedEmpty() {
         let expect = self.expectation(description: #function)
         alertStore.lookupAllUnacknowledged(completion: expectSuccess { alerts in
@@ -561,12 +583,12 @@ class AlertStoreTests: XCTestCase {
     func testLookupAllUnacknowledgedSomeNot() {
         let expect = self.expectation(description: #function)
         fillWith(startDate: Self.historicDate, data: [
-            (alert1, false, false),
+            (alert1, true, false),
             (alert2, false, false),
-            (alert1, true, false)
+            (alert1, false, false),
         ]) {
             self.alertStore.lookupAllUnacknowledged(completion: self.expectSuccess { alerts in
-                self.assertEqual([self.alert1, self.alert2], alerts)
+                self.assertEqual([self.alert2, self.alert1], alerts)
                 expect.fulfill()
             })
         }