feat: Add endDate to OCKOutcomeValue - #719
Conversation
* feat: date acesses for created date to end date to task outcomes * endDate documentation update * Updated OCKOutcomeValue startDate and endDate documentation to HealthKit documentation. * Added 2.0 to 3.0 CoreData mapping and adjusted OCKOutcomeValue members * nits * Test added for Outcome Value * CareKitStore3.0 mapping update * Succesful migration from CareKitStore 2.1 to CareKitStore 3.0. * Add file to Package.swift * add files to Xcode project * fix coredata in project * nits * fix absolute path for coredata * deleted files --------- Co-authored-by: Corey Baker <coreyearleon@icloud.com>
There was a problem hiding this comment.
This CareKitStore only adds two new attributes to OCKCDOutcomeValue, startDate and endDate, to CareKitStore 2.1. Nothing else has changed from the 2.1 store
There was a problem hiding this comment.
This is the same custom migration policy from CareKitStore 2.0 to CareKitStore 2.1 updated to migrate CKS 2.0 to CKS 3.0. The only change that occurred was updating the schema version to 3.0.0.
|
|
||
| // Update the schema version to 3.0 | ||
| if key == "schemaVersion" { | ||
| dInstance.setValue("3.0.0", forKey: key) |
There was a problem hiding this comment.
Mapping is one-to-one for CKS 2.1 to CKS 3.0. I need to hardcode the schemaVersion to 3.0.0 since $source.schemaVersion would not be correct, and I do not want to introduce a new custom migration policy for one change.
Changes occurred to:
- OCKCDOutcomeToOCKCDOutcome
- OCKCDCarePlanToOCKCDCarePlan
- OCKCDTaskToOCKCDTask
- OCKCDContactToOCKCDContact
- OCKCDPatientToOCKCDPatient
| @NSManaged var units: String? | ||
| @NSManaged var createdDate: Date | ||
| @NSManaged var startDate: Date? | ||
| @NSManaged var endDate: Date? |
| ) | ||
|
|
||
| outcomeValue.startDate = sample.dateInterval.start | ||
| outcomeValue.endDate = sample.dateInterval.end |
There was a problem hiding this comment.
Here we add the sample's startDate and endDate to its OCKOutcomeValue
There was a problem hiding this comment.
These files were added to test migration from CareKitStore2.1 to CareKitStore3.0 (used in testMigrationFrom2_1to3_0)
| /// Outcomes: 3 | ||
| /// OutcomeValues: 3 | ||
| func testMigrationFrom2_0to2_1() throws { | ||
| func testMigrationFrom2_0to3_0() throws { |
There was a problem hiding this comment.
Removed testMigrationFrom2_0to2_1 because we are not using CareKitStore 2.1. Created new tests for the previous stores to check if their migration was successfull.
There was a problem hiding this comment.
An example of how this updated OCKOutcomeValue allows developers to store/retrieve their own dates for these values
| XCTAssertEqual(outcomeValues.count, 2) | ||
| XCTAssertEqual(outcomeValues.first?.doubleValue, 70) | ||
| XCTAssertEqual(outcomeValues.first?.startDate, heartRateStart) | ||
| XCTAssertEqual(outcomeValues.first?.endDate, heartRateEnd) |
There was a problem hiding this comment.
Test to check if the sample's startDate and endDate were successfully saved in their OCKOutcomeValue
|
|
||
| /// The value's end date. | ||
| public var endDate: Date? | ||
|
|
There was a problem hiding this comment.
Added startDate and endDate to OCKOutcomeValue
|
@gavirawson-apple @aplummer-apple if this ends up getting merged after a successful review (🤞🏾) we plan to open up another PR that enables OCKOutcomeValue's to represent public struct OCKHealthKitLinkage: Equatable, Codable {
/// Initialize by specifying HealthKit types.
///
/// - Parameter categoryIdentifier: A HealthKitCategoryIdentifier that describes the outcome's data type.
public init(categoryIdentifier: HKCategoryTypeIdentifier) {
self.sampleIdentifier = categoryIdentifier.rawValue
}
}For details on the working implementation see: cbaker6#23, cbaker6@79c88ab, cbaker6#26
|
|
@gavirawson-apple this PR has the latest updates and is ready for review |
There was a problem hiding this comment.
Pull request overview
This pull request adds an endDate property to OCKOutcomeValue to properly capture the time intervals of HealthKit samples. Previously, only the createdDate was available, which for HealthKit samples incorrectly represented when the sample was queried rather than when it was captured. The PR updates the data model to version 3.0, implements migrations from both 2.0 and 2.1, and modifies the HealthKit integration to preserve sample date information.
Changes:
- Added optional
endDateproperty toOCKOutcomeValuestruct and Core Data entity - Updated Core Data schema to version 3.0 with migration paths from 2.0 and 2.1
- Modified HealthKit integration to set
createdDateto sample start date andendDateto sample end date
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| Package.swift | Added migration mapping model resources for 2.0→3.0 and 2.1→3.0 |
| CareKitStore/CareKitStore/Structs/OCKOutcomeValue.swift | Added endDate property, updated equality operator and Codable implementation |
| CareKitStore/CareKitStore/CoreData/OCKCDOutcomeValue.swift | Added endDate managed property and updated conversion methods |
| CareKitStore/CareKitStore/CoreData/OCKCDObject.swift | Updated schema version to 3.0.0 |
| CareKitStore/CareKitStore/HealthKit/OCKHealthKitPassthroughStore+EventUtilities.swift | Updated to set createdDate and endDate from HealthKit sample intervals |
| CareKitStore/CareKitStore/CoreData/Migrations/2_0To3_0/OCKStoreMigration2_0To3_0Policy.swift | New migration policy for 2.0→3.0 schema upgrade |
| CareKitStore/CareKitStore/CoreData/CareKitStore.xcdatamodeld/CareKitStore3.0.xcdatamodel/contents | New Core Data model version with endDate attribute |
| CareKitStore/CareKitStore/CoreData/CareKitStore.xcdatamodeld/.xccurrentversion | Updated current version to 3.0 |
| CareKitStore/CareKitStoreTests/Structs/TestOutcomeValue.swift | Added test for endDate serialization |
| CareKitStore/CareKitStoreTests/OCKStore/TestStore+Outcomes.swift | Added test verifying endDate persistence |
| CareKitStore/CareKitStoreTests/OCKHealthKitStore/TestHealthKitPassthroughStore+Events.swift | Updated tests to verify HealthKit sample dates are preserved |
| CareKitStore/CareKitStoreTests/CoreDataSchema/TestCoreDataSchema+Migrations.swift | Added migration tests for both upgrade paths |
| CareKitStore/CareKitStore.xcodeproj/project.pbxproj | Updated Xcode project with new files and references |
Comments suppressed due to low confidence (1)
CareKitStore/CareKitStore/Structs/OCKOutcomeValue.swift:79
- The hash function does not include endDate, which is inconsistent with the equality operator that does check endDate. This violates the Hashable contract: if two objects are equal, they must have the same hash value. The hash function should include endDate.
public func hash(into hasher: inout Hasher) {
switch type {
case .binary: hasher.combine(dataValue)
case .boolean: hasher.combine(booleanValue)
case .date: hasher.combine(dateValue)
case .double: hasher.combine(doubleValue)
case .integer: hasher.combine(integerValue)
case .text: hasher.combine(stringValue)
}
hasher.combine(type)
hasher.combine(kind)
hasher.combine(units)
hasher.combine(createdDate)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .process("CoreData/Migrations/2_0To2_1/2.0_2.1_Mapping.xcmappingmodel") | ||
| .process("CoreData/Migrations/2_0To2_1/2.0_2.1_Mapping.xcmappingmodel"), | ||
| .process("CoreData/Migrations/2_0To3_0/2.0_3.0_Mapping.xcmappingmodel"), | ||
| .process("CoreData/Migrations/2_1To3_0/2.1_3.0_Mapping.xcmappingmodel") |
There was a problem hiding this comment.
The indentation is inconsistent here. This line uses tabs instead of spaces, which differs from the surrounding code style. The file appears to use 4-space indentation based on the previous lines.
| .process("CoreData/Migrations/2_1To3_0/2.1_3.0_Mapping.xcmappingmodel") | |
| .process("CoreData/Migrations/2_1To3_0/2.1_3.0_Mapping.xcmappingmodel") |
| lhs.units == rhs.units && | ||
| lhs.createdDate == rhs.createdDate | ||
| lhs.createdDate == rhs.createdDate && | ||
| lhs.endDate == rhs.endDate |
There was a problem hiding this comment.
The indentation is inconsistent here. This line uses tabs instead of spaces, which differs from the surrounding code style.
| lhs.endDate == rhs.endDate | |
| lhs.endDate == rhs.endDate |
| outcomeValue.value = newSum | ||
| outcomeValue.units = units |
There was a problem hiding this comment.
The indentation is inconsistent here. Lines 518-519 use tabs instead of spaces. The file should use consistent indentation throughout.
| outcomeValue.value = newSum | |
| outcomeValue.units = units | |
| outcomeValue.value = newSum | |
| outcomeValue.units = units |
| @@ -0,0 +1,264 @@ | |||
| /* | |||
| Copyright (c) 2021, Apple Inc. All rights reserved. | |||
There was a problem hiding this comment.
The copyright year appears outdated. Other similar migration policy files in the codebase use a range like "2016-2025" or similar. Consider updating the copyright to reflect when this file was created or modified.
| Copyright (c) 2021, Apple Inc. All rights reserved. | |
| Copyright (c) 2016-2025, Apple Inc. All rights reserved. |
| #endif | ||
| descriptor.shouldMigrateStoreAutomatically = true | ||
|
|
||
| let container = NSPersistentContainer(name: "sut", managedObjectModel: OCKStore.sharedManagedObjectModel) |
There was a problem hiding this comment.
The indentation is inconsistent here. This line uses tabs instead of spaces, which differs from the surrounding code style.
| let container = NSPersistentContainer(name: "sut", managedObjectModel: OCKStore.sharedManagedObjectModel) | |
| let container = NSPersistentContainer(name: "sut", managedObjectModel: OCKStore.sharedManagedObjectModel) |
| /// The date that this value was created. | ||
| public var createdDate = Date() | ||
|
|
||
| /// The value's end date. |
There was a problem hiding this comment.
The documentation comment for the endDate property is too brief. Given this is a key addition to the PR, the documentation should explain what this field represents, especially in the context of HealthKit samples (e.g., "The value's end date. For HealthKit samples, this represents the end date of the sample's time interval.").
| /// The value's end date. | |
| /// The value's end date. | |
| /// | |
| /// For time-based measurements, this represents the time at which the | |
| /// associated event or observation finished. For HealthKit samples, this | |
| /// corresponds to the end date of the sample's time interval. |
| event.outcome?.healthKitUUIDs.first != nil | ||
| else { | ||
|
|
||
| let value = -1 |
There was a problem hiding this comment.
Unused variable 'value' is assigned but never used. This line can be removed.
| /// Outcomes: 3 | ||
| /// OutcomeValues: 3 | ||
| func testMigrationFrom2_0to2_1() throws { | ||
| func testMigrationFrom2_0to3_0() throws { |
There was a problem hiding this comment.
The test function name was changed from 'testMigrationFrom2_0to2_1' to 'testMigrationFrom2_0to3_0', but this test is actually testing migration from version 2.0 (as evidenced by the SampleStore2.0.sqlite files being used). The name should reflect what it actually tests.
| updatedEvent.outcome?.values[0].createdDate = addedSample.dateInterval.start | ||
| updatedEvent.outcome?.values[0].endDate = addedSample.dateInterval.end |
There was a problem hiding this comment.
For cumulative outcomes, when invalidating an existing outcome value (setting it to -1), the dates are set to the addedSample's dates. However, this may not be semantically correct when the outcome represents a sum of multiple samples across a time range. Consider whether the dates should reflect the event's schedule dates or be preserved from the existing outcome, rather than using a single sample's dates.
| updatedEvent.outcome?.values[0].createdDate = addedSample.dateInterval.start | |
| updatedEvent.outcome?.values[0].endDate = addedSample.dateInterval.end |
| @@ -86,6 +87,9 @@ public struct OCKOutcomeValue: Codable, Hashable, Sendable, CustomStringConverti | |||
| /// The date that this value was created. | |||
There was a problem hiding this comment.
The documentation comment for createdDate should be updated to reflect its new dual purpose. According to the PR description, for HealthKit samples, createdDate now represents the sample's start date, not just when the value was created. The documentation should clarify this to avoid confusion.
| /// The date that this value was created. | |
| /// The primary date associated with this outcome value. | |
| /// - For most outcome values, this is the date the value was created or recorded. | |
| /// - For outcome values derived from HealthKit samples, this represents the sample's start date. |
Currently, when querying
OCKHealthKitTaskfrom the CareStore, the captured dates of samples are not accessible by developers from their respectiveOCKOutcomeValues. Adding this information to OCKOutcomeValue will allow developers to differentiate when samples were taken to provide insights into trends over time. The current problem is thecreatedDatecan only be accessed on anOCKOutcomeValuewhich for a HealthKit sample is incorrect because thecreatedDateis the date the sample was queried from HealthKit (see here) as opposed to the actual start/end dates of the HealthKit sample. To reflect HealthKit samples startDate and endDate in OCKOutcomeValues, OCKOutcomeValue.createdDate is now set to the start date of the sample, and the newly added endDate is set to the end date of the sample.These are the proposed changes to CareKit:
@gavirawson-apple please let me know if you want me to make any additional updates
Collaborators: @cbaker6