Resolve issue #353 (#359)

* Resolve issue #353

* Typo fix

CareKit/CareKit/Synchronized View Controllers/Chart/Controller/OCKChartController.swift

@@ -46,16 +46,16 @@ open class OCKChartController: OCKChartControllerProtocol, ObservableObject {
 
     // MARK: Properties
 
-    private let weekOfDate: Date
-    private var subscription: AnyCancellable?
+    private let eventQuery: OCKEventQuery
+    private var cancellables: Set<AnyCancellable> = Set()
 
     // MARK: - Life Cycle
 
     /// Initialize the controller.
     /// - Parameter weekOfDate: A date in the week of the insights range.
     /// - Parameter storeManager: Wraps the store that contains the insight data.
     public required init(weekOfDate: Date, storeManager: OCKSynchronizedStoreManager) {
-        self.weekOfDate = weekOfDate
+        self.eventQuery = OCKEventQuery(dateInterval: Calendar.current.dateIntervalOfWeek(for: weekOfDate))
         self.storeManager = storeManager
         self.objectWillChange = .init([])
     }
@@ -67,44 +67,31 @@ open class OCKChartController: OCKChartControllerProtocol, ObservableObject {
     ///   - configurations: An array of configurations to be plotted.
     open func fetchAndObserveInsights(forConfigurations configurations: [OCKDataSeriesConfiguration],
                                       errorHandler: ((Error) -> Void)? = nil) {
-
-        // Fetch tasks, then fetch events for the tasks and set the view model
-        let eventQuery = OCKEventQuery(dateInterval: Calendar.current.dateIntervalOfWeek(for: weekOfDate))
-        fetchTasks(eventQuery: eventQuery, configurations: configurations, errorHandler: errorHandler)
-    }
-
-    private func fetchTasks(eventQuery: OCKEventQuery, configurations: [OCKDataSeriesConfiguration],
-                            errorHandler: ((Error) -> Void)? = nil) {
-
-        // Build up the task query
-        var taskQuery = OCKTaskQuery(for: Date())
-        taskQuery.ids = configurations.map { $0.taskID }
-
-        storeManager.store.fetchAnyTasks(query: taskQuery, callbackQueue: .main) { [weak self] result in
-            guard let self = self else { return }
-            switch result {
-            case .success(let tasks):
-
-                // Fetch events and set the view model. Also set the view model when the events change
-                self.refetchEvents(eventQuery: eventQuery, configurations: configurations) { result in
-                    if case let .failure(error) = result {
-                        errorHandler?(error)
-                    }
-                }
-
-                self.subscribeTo(tasks: tasks, eventQuery: eventQuery, configurations: configurations) { result in
-                    if case let .failure(error) = result {
-                        errorHandler?(error)
+        cancellables = Set()
+        configurations.forEach { config in
+            store.fetchAnyEvents(taskID: config.taskID, query: eventQuery, callbackQueue: .main) { result in
+                switch result {
+                case let .failure(error): errorHandler?(error)
+                case let .success(events):
+                    self.refetchEvents(configurations: configurations, completion: nil)
+                    events.forEach { event in
+                        self.storeManager
+                            .publisher(forEvent: event, categories: [.add, .update, .delete])
+                            .sink(receiveValue: { _ in
+                                self.refetchEvents(configurations: configurations) { result in
+                                    if case let .failure(error) = result {
+                                        errorHandler?(error)
+                                    }
+                                }
+                            })
+                            .store(in: &self.cancellables)
                     }
                 }
-
-            case .failure(let error):
-                errorHandler?(error)
             }
         }
     }
 
-    private func refetchEvents(eventQuery: OCKEventQuery, configurations: [OCKDataSeriesConfiguration],
+    private func refetchEvents(configurations: [OCKDataSeriesConfiguration],
                                completion: OCKResultClosure<[OCKDataSeries]>?) {
         var allDataSeries = [OCKDataSeries]()
         let group = DispatchGroup()
@@ -138,25 +125,4 @@ open class OCKChartController: OCKChartControllerProtocol, ObservableObject {
             completion?(.success(allDataSeries))
         }
     }
-
-    private func subscribeTo(tasks: [OCKAnyTask],
-                             eventQuery: OCKEventQuery, configurations: [OCKDataSeriesConfiguration],
-                             completion: OCKResultClosure<[OCKDataSeries]>?) {
-        // Set the view model when the tasks change
-        let taskSubscriptions = tasks.map { task in
-            return storeManager.publisher(forTask: task, categories: [.update, .delete], fetchImmediately: false)
-                .sink { _ in self.refetchEvents(eventQuery: eventQuery, configurations: configurations, completion: completion) }
-        }
-
-        // Set the view model when the events for the tasks change
-        let eventSubscriptions = tasks.map { task in
-            return self.storeManager.publisher(forEventsBelongingToTask: task, categories: [.update, .add, .delete])
-                .sink { _ in self.refetchEvents(eventQuery: eventQuery, configurations: configurations, completion: completion) }
-        }
-
-        subscription = AnyCancellable {
-            taskSubscriptions.forEach { $0.cancel() }
-            eventSubscriptions.forEach { $0.cancel() }
-        }
-    }
 }

CareKitStore/CareKitStore/CoreData/OCKCDVersionedObject.swift

@@ -61,6 +61,10 @@ class OCKCDVersionedObject: OCKCDObject, OCKCDManageable {
         }.compactMap { $0 as? T }
     }
 
+    static var notDeletedPredicate: NSPredicate {
+        NSPredicate(format: "%K == nil", #keyPath(OCKCDVersionedObject.deletedDate))
+    }
+
     static func headerPredicate(for ids: [String]) -> NSPredicate {
         return NSCompoundPredicate(andPredicateWithSubpredicates: [
             NSPredicate(format: "%K IN %@", #keyPath(OCKCDVersionedObject.id), ids),
@@ -70,30 +74,25 @@ class OCKCDVersionedObject: OCKCDObject, OCKCDManageable {
 
     static func headerPredicate() -> NSPredicate {
         return NSCompoundPredicate(andPredicateWithSubpredicates: [
-            NSPredicate(format: "%K == nil", #keyPath(OCKCDVersionedObject.next)),
+            notDeletedPredicate,
             NSPredicate(format: "%K == nil", #keyPath(OCKCDVersionedObject.deletedDate))
         ])
     }
 
     static func newestVersionPredicate(in interval: DateInterval) -> NSPredicate {
-        let notDeletedYet = NSPredicate(format: "%K < %@ AND %K == nil",
-                                        #keyPath(OCKCDVersionedObject.effectiveDate),
-                                        interval.end as NSDate,
-                                        #keyPath(OCKCDVersionedObject.deletedDate))
-        let deletedAfterQueryStart = NSPredicate(format: "%K < %@ AND %K > %@",
+        let startsBeforeEndOfQuery = NSPredicate(format: "%K < %@",
                                                  #keyPath(OCKCDVersionedObject.effectiveDate),
-                                                 interval.end as NSDate,
-                                                 #keyPath(OCKCDVersionedObject.deletedDate),
-                                                 interval.start as NSDate)
-        let noNextVersion = NSPredicate(format: "%K == nil OR %K.effectiveDate > %@",
+                                                 interval.end as NSDate)
+
+        let noNextVersion = NSPredicate(format: "%K == nil OR %K.effectiveDate >= %@",
                                         #keyPath(OCKCDVersionedObject.next),
                                         #keyPath(OCKCDVersionedObject.next),
                                         interval.end as NSDate)
-        let existsDuringQuery = NSCompoundPredicate(orPredicateWithSubpredicates: [
-            notDeletedYet, deletedAfterQueryStart])
 
         return NSCompoundPredicate(andPredicateWithSubpredicates: [
-            existsDuringQuery, noNextVersion])
+            startsBeforeEndOfQuery,
+            noNextVersion
+        ])
     }
 
     static func validateNewIDs(_ ids: [String], in context: NSManagedObjectContext) throws {

CareKitStore/CareKitStore/CoreData/OCKStore+CarePlans.swift

@@ -68,7 +68,7 @@ extension OCKStore {
         context.perform {
             do {
                 try OCKCDCarePlan.validateNewIDs(plans.map { $0.id }, in: self.context)
-                let persistablePlans = plans.map (self.createCarePlan)
+                let persistablePlans = plans.map(self.createCarePlan)
                 try self.context.save()
                 let addedPlans = persistablePlans.map(self.makePlan)
                 callbackQueue.async {
@@ -150,9 +150,7 @@ extension OCKStore {
     }
 
     private func buildPredicate(for query: OCKCarePlanQuery) throws -> NSPredicate {
-        var predicate = NSPredicate(value: true)
-        let notDeletedPredicate = NSPredicate(format: "%K == nil", #keyPath(OCKCDVersionedObject.deletedDate))
-        predicate = NSCompoundPredicate(andPredicateWithSubpredicates: [predicate, notDeletedPredicate])
+        var predicate = OCKCDVersionedObject.notDeletedPredicate
 
         if let interval = query.dateInterval {
             let intervalPredicate = OCKCDVersionedObject.newestVersionPredicate(in: interval)
@@ -165,7 +163,7 @@ extension OCKStore {
         }
 
         if !query.versionIDs.isEmpty {
-            let versionPredicate = NSPredicate(format: "self IN %@", try query.versionIDs.map(objectID(for:)))
+            let versionPredicate = NSPredicate(format: "self IN %@", try query.versionIDs.map(objectID))
             predicate = NSCompoundPredicate(andPredicateWithSubpredicates: [predicate, versionPredicate])
         }
 

CareKitStore/CareKitStore/CoreData/OCKStore+Contacts.swift

@@ -186,9 +186,7 @@ extension OCKStore {
     }
 
     private func buildPredicate(for query: OCKContactQuery) throws -> NSPredicate {
-        var predicate = NSPredicate(value: true)
-        let notDeletedPredicate = NSPredicate(format: "%K == nil", #keyPath(OCKCDVersionedObject.deletedDate))
-        predicate = NSCompoundPredicate(andPredicateWithSubpredicates: [predicate, notDeletedPredicate])
+        var predicate = OCKCDVersionedObject.notDeletedPredicate
 
         if let interval = query.dateInterval {
             let intervalPredicate = OCKCDVersionedObject.newestVersionPredicate(in: interval)
@@ -201,7 +199,7 @@ extension OCKStore {
         }
 
         if !query.versionIDs.isEmpty {
-            let versionPredicate = NSPredicate(format: "self IN %@", try query.versionIDs.map(objectID(for:)))
+            let versionPredicate = NSPredicate(format: "self IN %@", try query.versionIDs.map(objectID))
             predicate = NSCompoundPredicate(andPredicateWithSubpredicates: [predicate, versionPredicate])
         }
 

CareKitStore/CareKitStore/CoreData/OCKStore+Patients.swift

@@ -147,10 +147,7 @@ extension OCKStore {
     }
 
     private func buildPredicate(for query: OCKPatientQuery) throws -> NSPredicate {
-        var predicate = NSPredicate(value: true)
-
-        let notDeletedPredicate = NSPredicate(format: "%K == nil", #keyPath(OCKCDVersionedObject.deletedDate))
-        predicate = NSCompoundPredicate(andPredicateWithSubpredicates: [predicate, notDeletedPredicate])
+        var predicate = OCKCDVersionedObject.notDeletedPredicate
 
         if let interval = query.dateInterval {
             let intervalPredicate = OCKCDVersionedObject.newestVersionPredicate(in: interval)

CareKitStore/CareKitStore/Protocols/CoreData/OCKCoreDataTaskStore.swift

@@ -273,7 +273,7 @@ extension OCKCoreDataTaskStoreProtocol {
     }
 
     func buildPredicate(for query: OCKTaskQuery) throws -> NSPredicate {
-        var predicate = NSPredicate(format: "%K == nil", #keyPath(OCKCDVersionedObject.deletedDate)) // Not deleted
+        var predicate = OCKCDVersionedObject.notDeletedPredicate
 
         if let interval = query.dateInterval {
             let headPredicate = OCKCDVersionedObject.newestVersionPredicate(in: interval)

CareKitStore/CareKitStore/Protocols/Events/OCKEventStore.swift

@@ -154,6 +154,13 @@ public extension OCKReadOnlyEventStore where Task: OCKAnyVersionableTask, Outcom
             case .failure(let error): completion(.failure(error))
             case .success(let outcomes):
                 let events = self.join(task: task, with: outcomes, and: scheduleEvents) + previousEvents
+
+                // If the query doesn't go back in time beyond the start of this version of the task, we're done.
+                guard query.dateInterval.start < task.effectiveDate else {
+                    completion(.success(events))
+                    return
+                }
+
                 self.fetchNextValidPreviousVersion(for: task, callbackQueue: callbackQueue) { result in
                     switch result {
                     case .failure(let error): completion(.failure(error))

CareKitStore/CareKitStoreTests/OCKStore/TestStore+Tasks.swift

@@ -285,6 +285,23 @@ class TestStoreTasks: XCTestCase {
         XCTAssertNoThrow(try store.updateTaskAndWait(task))
     }
 
+    func testQueryUpdatedTasksEvents() throws {
+        let schedule = OCKSchedule.mealTimesEachDay(start: Date(), end: nil) // 7:30AM, 12:00PM, 5:30PM
+        let original = try store.addTaskAndWait(OCKTask(id: "meds", title: "Original", carePlanID: nil, schedule: schedule))
+
+        var updated = original
+        updated.effectiveDate = schedule[5].start // 5:30PM tomorrow
+        updated.title = "Updated"
+        updated = try store.updateTaskAndWait(updated)
+        let query = OCKEventQuery(for: schedule[5].start) // 0:00AM - 23:59.99PM tomorrow
+        let events = try store.fetchEventsAndWait(taskID: "meds", query: query)
+
+        XCTAssert(events.count == 3)
+        XCTAssert(events[0].task.localDatabaseID == original.localDatabaseID)
+        XCTAssert(events[1].task.localDatabaseID == original.localDatabaseID)
+        XCTAssert(events[2].task.localDatabaseID == updated.localDatabaseID)
+    }
+
     func testUpdateFailsForUnsavedTasks() {
         let task = OCKTask(id: "meds", title: "Medication", carePlanID: nil, schedule: .mealTimesEachDay(start: Date(), end: nil))
         XCTAssertThrowsError(try store.updateTaskAndWait(task))
@@ -327,6 +344,18 @@ class TestStoreTasks: XCTestCase {
         XCTAssert(fetched.first?.title == taskA.title)
     }
 
+    func testTaskQueryStartingExactlyOnEffectiveDateOfNewVersion() throws {
+        let schedule = OCKSchedule.dailyAtTime(hour: 0, minutes: 0, start: Date(), end: nil, text: nil)
+        let query = OCKTaskQuery(dateInterval: DateInterval(start: schedule[5].start, end: schedule[5].end))
+
+        var task = try store.addTaskAndWait(OCKTask(id: "meds", title: "Medication", carePlanID: nil, schedule: schedule))
+        task.effectiveDate = task.schedule[5].start
+        task = try store.updateTaskAndWait(task)
+
+        let fetched = try store.fetchTasksAndWait(query: query)
+        XCTAssert(fetched.first == task)
+    }
+
     func testTaskQuerySpanningVersionsReturnsNewestVersionOnly() throws {
         let schedule = OCKSchedule.mealTimesEachDay(start: Date(), end: nil)
 

CareKitStore/CareKitStoreTests/Structs/TestSchedule.swift

@@ -58,15 +58,15 @@ class TestSchedule: XCTestCase {
     }
 
     func testAllDayEventsCapturedByEventsBetweenDates() {
-         let morning = Calendar.current.startOfDay(for: Date())
-         let breakfast = Calendar.current.date(byAdding: .hour, value: 7, to: morning)!
-         let lunch = Calendar.current.date(byAdding: .hour, value: 12, to: morning)!
-         let dinner = Calendar.current.date(byAdding: .hour, value: 18, to: morning)!
-         let allDay = OCKScheduleElement(start: breakfast, end: nil, interval: DateComponents(day: 1), text: "Daily", duration: .allDay)
-         let schedule = OCKSchedule(composing: [allDay])
-         let events = schedule.events(from: lunch, to: dinner)
-         XCTAssert(events.count == 1, "Expected 1 all day event, but got \(events.count)")
-     }
+        let morning = Calendar.current.startOfDay(for: Date())
+        let breakfast = Calendar.current.date(byAdding: .hour, value: 7, to: morning)!
+        let lunch = Calendar.current.date(byAdding: .hour, value: 12, to: morning)!
+        let dinner = Calendar.current.date(byAdding: .hour, value: 18, to: morning)!
+        let allDay = OCKScheduleElement(start: breakfast, end: nil, interval: DateComponents(day: 1), text: "Daily", duration: .allDay)
+        let schedule = OCKSchedule(composing: [allDay])
+        let events = schedule.events(from: lunch, to: dinner)
+        XCTAssert(events.count == 1, "Expected 1 all day event, but got \(events.count)")
+    }
 
     func testWeeklySchedule() {
         let schedule = OCKSchedule.weeklyAtTime(weekday: 1, hours: 0, minutes: 0, start: Date(), end: nil, targetValues: [], text: nil)
@@ -181,6 +181,14 @@ class TestSchedule: XCTestCase {
         XCTAssert(events[2].occurrence == 5)
     }
 
+    func testScheduleIntervalsHaveInclusiveLowerBoundAndExclusiveUpperBound() {
+        let element = OCKScheduleElement(start: Date(), end: nil, interval: DateComponents(day: 1), text: nil, targetValues: [], duration: .allDay)
+        let schedule = OCKSchedule(composing: [element])
+        let events = schedule.events(from: schedule[1].start, to: schedule[1].end)
+        XCTAssert(events.count == 1)
+        XCTAssert(events.first?.occurrence == 1)
+    }
+
     // Measure how long it takes to generate 10 years worth of events for a highly complex schedule with hourly events.
     // Results in the computatin of about 100,000 events.
     func testEventGenerationPerformanceHeavySchedule() {

CareKitStore/CareKitStoreTests/TestStoreProtocolExtensions.swift

@@ -217,6 +217,20 @@ class TestStoreProtocolExtensions: XCTestCase {
         XCTAssert(events.first?.task.title == versionA.title)
     }
 
+    func testFetchEventsReturnsOnlyTheNewerOfTwoEventsWhenTwoVersionsOfATaskHaveEventsAtQueryStart() throws {
+        let element = OCKScheduleElement(start: Date(), end: nil, interval: DateComponents(day: 1),
+                                         text: nil, targetValues: [], duration: .allDay)
+        let schedule = OCKSchedule(composing: [element])
+        let versionA = OCKTask(id: "123", title: "A", carePlanID: nil, schedule: schedule)
+        try store.addTaskAndWait(versionA)
+        var versionB = OCKTask(id: "123", title: "B", carePlanID: nil, schedule: schedule)
+        versionB.effectiveDate = schedule[4].start
+        try store.updateTaskAndWait(versionB)
+        let events = try store.fetchEventsAndWait(taskID: "123", query: .init(for: schedule[4].start))
+        XCTAssert(events.count == 1, "Expected 1, but got \(events.count)")
+        XCTAssert(events.first?.task.title == "B")
+    }
+
     // MARK: Adherence and Insights
 
     func testFetchAdherenceAggregatesEventsAcrossTasks() throws {