Improve Remote failure error messaging#1867
Conversation
| @@ -1329,12 +1329,18 @@ extension DeviceDataManager { | |||
| log.error("Remote Notification: Overrides not enabled.") | |||
| return | |||
| } | |||
There was a problem hiding this comment.
This is the start of routing a remote notification validation to the service that generated it (i.e. Nightscout being a service). Through now, we've been allowing any service to validate remote notifications (i.e. OTP codes), even those that have nothing to do with it which could lead to weird issues. I think they should only be routed to the service that generated them. This is also useful for my work in this PR where I need to get the errors associated with remote validation so I only want to route them to the appropriate service.
Since we are not yet injecting the notification on the server-side (Nightscout), I'm making "NightscoutService" the fallback here.
There was a problem hiding this comment.
Yes, agree 100%. We should only try to validate commands against the service that issued them. Right now, a service integration in Loop sends a device id to the remote service, so that it can route the commands to this device correctly. I think the service integration should also send a service id, so that Loop can know which service to use to validate the command.
| if let expirationStr = notification["expiration"] as? String { | ||
| let formatter = ISO8601DateFormatter() | ||
| formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] | ||
| if let expiration = formatter.date(from: expirationStr) { |
There was a problem hiding this comment.
Show an error when the notification has expired.
| @@ -1365,8 +1371,14 @@ extension DeviceDataManager { | |||
| log.default("Remote Notification: Enacting bolus entry: %{public}@", String(describing: bolusAmount)) | |||
|
|
|||
| //Remote bolus requires validation from its remote source | |||
There was a problem hiding this comment.
Capture the error from the validation service to give the user more information on why something failed and how to fix it.
| @@ -586,15 +586,13 @@ extension RemoteDataServicesManager { | |||
|
|
|||
| extension RemoteDataServicesManager { | |||
|
|
|||
There was a problem hiding this comment.
As mentioned above, we will validate a push notification by the service that generated it.
be2a91b to
61d9b1a
Compare
|
|
||
| let defaultServiceIdentifier = "NightscoutService" | ||
| let serviceIdentifer = notification["serviceIdentifier"] as? String ?? defaultServiceIdentifier | ||
|
|
There was a problem hiding this comment.
A side point as I was working through this handlePushNotification code...
I'm thinking an evolution of this code, someday, may be to send a raw notification to each service for handling. They could handle all the validation, like this expiration check below, which may vary by service.
There are some remote actions maybe only they can handle like activating a service setting or mode.
On that matter, maybe they could handle all notifications (i.e. deliver the bolus/carbs/etc). We could allow services to perform such via service delegation and add controls on the Loop side to restrict things for safety.
Anyways, just a few thoughts.
There was a problem hiding this comment.
I think validating the command is the service's responsibility. Not sure executing the command is, though. I think the "command set" is a Loop domain. Maybe we're talking about the same thing, though.
ps2
left a comment
There was a problem hiding this comment.
This is polishing a dead-end use case, I hope. I'm counting on this being short term. It's not just the code that gets harder to change with time, but user behavior/expectations.
We have had some reports of failed remote deliveries and the current error messaging isn't very helpful for understanding why a remote push failed. This information is captured in diagnostic logs but ideally this would be surfaced to a user. This PR enhances our user-facing errors messages.
Associated PRs:
LoopKit: #1867
NightscoutService: ps2/NightscoutService#24