Remote 1: Action Models #1930
Remote 1: Action Models #1930
Conversation
| @@ -812,6 +812,18 @@ extension DeviceDataManager { | |||
| self.loopManager.updateRemoteRecommendation() | |||
| } | |||
| } | |||
|
|
|||
gestrich
force-pushed
the
feature/2023/02/bg/remote-action-models
branch
from
99519a7 to
ba6c7f3
Compare
| @@ -1341,8 +1353,15 @@ extension Notification.Name { | |||
|
|
|||
| // MARK: - Remote Notification Handling | |||
| extension DeviceDataManager { | |||
There was a problem hiding this comment.
High-level changes in this class:
- Make an async variant of
handleRemoteNotification. This will be especially helpful for remote commands 2 later. - Use the new RemoteAction models for enacting a command.
- Add methods for handling async background uploads
|
|
||
| do { | ||
| command = try RemoteCommand.createRemoteCommand(notification: notification, allowedPresets: loopManager.settings.overridePresets, defaultAbsorptionTime: carbStore.defaultAbsorptionTimes.medium).get() | ||
| action = try RemoteAction.createRemoteAction(notification: notification).get() | ||
| } catch { | ||
| log.error("Remote Notification: Parse Error: %{public}@", String(describing: error)) | ||
| return |
There was a problem hiding this comment.
There were all sorts of validation logic for specific command types in this method previously. The new paradigm here is to move the validation logic into the RemoteAction. That helps hide this complexity and makes it more easy to unit test.
| @@ -1372,156 +1388,144 @@ extension DeviceDataManager { | |||
| } | |||
| } | |||
|
|
|||
| let command: RemoteCommand | |||
| let action: RemoteAction | |||
|
|
|||
There was a problem hiding this comment.
RemoteCommand was renamed to RemoteAction. Later on I plan to reintroduce a RemoteCommand, but it will instead have a RemoteAction. The distinction is a command will include the action and all the ancillary things related to commands (security validation, status reporting, etc).
Separating these concepts should make more sense with Remote 2.0.
| try await handleRemoteCarbAction(carbAction) | ||
| } catch { | ||
| await NotificationManager.sendRemoteCarbEntryFailureNotification(for: error, amountInGrams: carbAction.amountInGrams) | ||
| log.error("Remote Notification: Carb Action Error: %{public}@", String(describing: notification)) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
These "handle" methods below all take an action and attempt to enact it.
| } | ||
|
|
||
| //Remote Overrides | ||
|
|
There was a problem hiding this comment.
The paradigm here is each RemoteAction is asked for its data required to enact it (i.e. TemporaryOverride, CarbEntry, etc).
That call allows the action to validate that its data is normalized wrt to Loop's expectations (ex: carbs not exceeding reasonable max)
| @@ -209,6 +209,14 @@ final class RemoteDataServicesManager { | |||
| completion() | |||
| } | |||
| } | |||
| @@ -8,78 +8,34 @@ | |||
|
|
|||
| import Foundation | |||
| import LoopKit | |||
| import HealthKit | |||
|
|
|||
There was a problem hiding this comment.
Not needed as each remote action type will own its own errors.
| case bolusEntry(Double) | ||
| case carbsEntry(NewCarbEntry) | ||
| } | ||
|
|
||
|
|
||
| // Push Notifications |
There was a problem hiding this comment.
Generally like before, this method parses a notification and extracts a remote action (previously command).
The difference is Loop valid action checks are not done here (ex: Carb amount within reasonable limits). That will instead be managed by the client.
Separating notification payload parsing and validation seems like a good idea here. This will go away when I use Codable to do all this parsing in Remote 2.0.
|
|
||
| var absorptionTime = defaultAbsorptionTime | ||
| var absorptionTime: TimeInterval? = nil | ||
| if let absorptionOverrideInHours = notification["absorption-time"] as? Double { | ||
| absorptionTime = TimeInterval(hours: absorptionOverrideInHours) | ||
| } | ||
|
|
There was a problem hiding this comment.
Carb food type was missing from the APIs. This will require a change to the Nightscout codebase in order to support this but should be ok when empty too.
| @@ -0,0 +1,108 @@ | |||
| // | |||
There was a problem hiding this comment.
I needed to update the RemoteCommandTests with these and change a few that are unnecessary.
I added more tests to LoopKit, which test the individual remote action types themselves which should cover all the cases I know of.
|
Updates done - I think I responded to all the feedback but let me know if other questions. |
|
@ps2
@ps2 Disregard - I had my repo in a weird state and see it did make it to dev just fine. |
This will migrate away from the RemoteCommand type to the RemoteAction type. I'd like to separate actions, things that get enacted in Loop, from the RemoteCommand that delivered the action. This should help with unit testing and Remote Command 2.0 where this distinction is more meaningful. For now, I've remote the RemoteCommand type in favor of RemoteAction, however I plan to reintroduce it when Remote Commands 2.0 will bring the queuing capabilities (and responsibilities for a command)
Note that the related LoopKit PR will need merged with this LoopKit/LoopKit#453