Remote PR Set #2: Introduce RemoteCommands #1934
Conversation
gestrich
force-pushed
the
feature/2023/02/bg/remote-command-service
branch
from
dac31aa to
f973d54
Compare
| A9DCF32A25B0FABF00C89088 /* LoopUIColorPalette+Default.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9DCF2D525B0F3C500C89088 /* LoopUIColorPalette+Default.swift */; }; | ||
| A9DF02CB24F72B9E00B7C988 /* CriticalEventLogTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9DF02CA24F72B9E00B7C988 /* CriticalEventLogTests.swift */; }; | ||
| A9DFAFB324F0415E00950D1E /* CarbBackfillRequestUserInfoTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9DFAFB224F0415E00950D1E /* CarbBackfillRequestUserInfoTests.swift */; }; | ||
| A9DFAFB524F048A000950D1E /* WatchHistoricalCarbsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9DFAFB424F048A000950D1E /* WatchHistoricalCarbsTests.swift */; }; |
There was a problem hiding this comment.
The RemoteCommand type will now be in LoopKit. This is the common model that is shared between plugins and Loop so commands can be fetched from plugins by Loop.
The tests that were previously part of RemoteCommandTests are not relevant here anymore. I've added equivalent tests to rileylink_ios which will validate push notification parsing.
| log.error("Remote Notification: Remote Commands not enabled.") | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
I consider the validation of remote commands to be owned by the domain of the service (plugin) that produced the command (ex: OTP and Expiration date). This allows the providers to have their own validation criteria using different mechanisms appropriate for the server that generated it.
I think there are certain validation things should be common across all services like dose hashing since they need to be validated in Loop. Also, if we validated an "issueDate", this may make sense to stay in Loop as Loop could maintain the policy for the oldest issued commands it would accept (maybe different for different action types)
The "expirationDate" here though seems different than my suggested "issueDate", as it is based on a Nightscout server-side policy (I believe 5 minutes) so it seems like a specific concept to Nightscout that can be confined to its plugin.
| return | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The remote provider will now be responsible for push notification parsing which may vary across providers and provider versions.
| func handleRemoteCommand(_ command: RemoteCommand) async { | ||
|
|
||
| log.default("Remote Notification: Handling command %{public}@", String(describing: command)) | ||
|
|
There was a problem hiding this comment.
The RemoteCommand can communicate with its remote service. This will be helpful in the future where command sequencing will be reported back to the remote service.
I'm leveraging this capability of RemoteCommands here where validate() is now called directly on the command without needing to look up its originating service in this code, as was previously done in validatePushNotificationSource.
There was a problem hiding this comment.
The only reason I validate the command in each switch case below, rather than in just one place above: I need to use the appropriate error handler which vary for remote carbs vs remote bolus. The overall error handling of commands below is something to consider refactoring in the future but I wanted to keep this PR scoped.
| } | ||
|
|
||
| extension RemoteDataServicesManager { | ||
|
|
There was a problem hiding this comment.
Getting rid of validatePushNotificationSource as mentioned above.
The method serviceForPushNotification will find the appropriate service for a push notification. We will use that service then to ask for a RemoteCommand from the push notification.
| } | ||
|
|
||
| extension RemoteDataServicesManager: RemoteCommandSource { | ||
|
|
There was a problem hiding this comment.
This is async as someday the service will make an API request to put a remote lock on the command state (i.e. Set to in-progress state)
gestrich
force-pushed
the
feature/2023/02/bg/remote-command-service
branch
from
f973d54 to
6a54dab
Compare
gestrich
changed the title
gestrich
changed the title
| @@ -1,59 +0,0 @@ | |||
| // | |||
There was a problem hiding this comment.
These were replaced by Codable models in rileylink_ios
| @@ -1,108 +0,0 @@ | |||
| // | |||
There was a problem hiding this comment.
These tests were refactored/expanded and moved to rileylink_ios
2 participants
This PR is part of the below set that should be merged together. This one will use "RemoteCommands", a protocol for interacting with the remote services that is the source of the RemoteCommand. This will lay the groundwork for remote 2.0 commands.