Remote 1: Action Models by gestrich · Pull Request #453 · LoopKit/LoopKit

gestrich · 2023-02-19T16:31:54Z

This will migrate away from the RemoteCommand type to the RemoteAction type.

See Loop PR for more details LoopKit/Loop#1930

Unit Tests

I added new unit tests to check that action validation is enforced.

gestrich · 2023-02-19T17:23:38Z

+//  Created by Bill Gestrich on 12/25/22.
+//  Copyright © 2022 LoopKit Authors. All rights reserved.
+//
+


Similar to RemoteCommand. But the associated types are specific to each command. This is in LoopKit as these models will be shared by Caregiver.

gestrich · 2023-02-19T17:24:04Z

+    public var description: String {
+        return "\(actionName) \(actionDetails)"
+    }
+


These are not used yet but helpful for Remote 2.0

gestrich · 2023-02-19T17:25:35Z

+    }
+}
+
+extension RemoteBolusAction {


Putting the validation logic needed to enact an action, in the action itself. I wrestled with keeping this logic in Loop instead, but I kind of like keeping the validation logic with the model.

Why are these in LoopKit at all? If it's for sharing types with plugins, maybe that would be necessary. If it's for sharing types between Loop and LCG, maybe pause on that for now, and keep them separate. Human input validation (LCG-only concern), network decoding and validation, and Loop's other validation concerns (like max bolus, max temp), are all separate things. Guessing there might be other differences in concerns between them that might make the types different too. Maybe Loop wants to have a received time?

If these types are strictly for representing a Swift version of the network encoding of these actions, then I guess that could be a shared concern.

I think I'm following the suggested separation here. These models are to include all the information needed to enact an action in Loop. For remote 2.0, they will be used for the network encoding/decoding both from the plugins and Caregiver.

I can move the validation pieces out of here and put into the Loop target. That does leave the question of where to put these things to keep them unit testable as they are. Making an extension, like here, keeps it testable without involving the DeviceDataManager test setup or dedicated validation structures for each action (i.e. RemoteBolusValidator..). My inclination is to move the extensions to the actions to the Loop target to provide some separation. But I can consider a validator or wrapping these actions into Loop domain actions if something more dedicated is desired. Note that in remote 2.0, the plan is for a RemoteCommand to hold onto a RemoteAction and other things like the RemoteCommandState.

For context, here is where I was planning to take things in remote 2.0 wrt actions. The RemoteCommand holds onto the action. Everything else in the RemoteCommand is specific to command management and lifecycle.

Rather than juggling a separate hook to the service that generated the command, the RemoteCommand itself has methods to interact with its service internally.. ex: RemoteCommand.markSuccess() rather than service.markSuccess(command)

public protocol RemoteCommand { var id: String {get} var action: RemoteAction {get} var status: RemoteCommandStatus {get} /* TODO: Should we support "future" commands? TODO: Should the expiration date be in here? TODO: Should creationDate be in here? */ func checkValidity() throws func markInProgress() async throws func markError(_ error: Error) async throws func markSuccess() async throws }

I think moving them to Loop sounds good. Yes, you will have a similar type in LCG, with similar fields, but I think this is one case where it's ok to avoid DRY. The responsibilities and things you will do with those types are going to be different.

Thanks for the context on eventual plans of wrapping RemoteAction with RemoteCommand. What do you think about RemoteAction -> Action? If a RemoteCommand has an action, the "remote" part can be implicit (if "actions" are things that could theoretically be triggered locally, by, say, a siri command), or could be namespaced by RemoteCommand (if they are not things that could be reused locally).

I think making the Actions decoupled from the remote aspect makes sense. I've dropped the Remote verbiage from actions for now.

ps2 · 2023-02-19T19:57:27Z

+    var actionName: String {
+        switch self {
+        case .carbsEntry:
+            return "Carb Entry"


Are these user displayed names? Localization?

Added - thanks

ps2 · 2023-02-19T23:06:06Z

+    }
+}
+
+extension RemoteBolusAction {


Why are these in LoopKit at all? If it's for sharing types with plugins, maybe that would be necessary. If it's for sharing types between Loop and LCG, maybe pause on that for now, and keep them separate. Human input validation (LCG-only concern), network decoding and validation, and Loop's other validation concerns (like max bolus, max temp), are all separate things. Guessing there might be other differences in concerns between them that might make the types different too. Maybe Loop wants to have a received time?

ps2 · 2023-02-19T23:08:35Z

+    }
+}
+
+extension RemoteBolusAction {


If these types are strictly for representing a Swift version of the network encoding of these actions, then I guess that could be a shared concern.

ps2 · 2023-02-20T18:41:38Z

        switch self {
        case .carbsEntry:
-            return "Carb Entry"
+            return NSLocalizedString("Carb Entry", comment: "Carb entry action name")


So this will lookup the string in the app bundle, not the LoopKit bundle. LocalizedString exists to let you look up strings in LoopKit. Not an issue if this moves into Loop. Also, I think I saw that this string is being combined with other (non-localized) strings?

I ended up dropping these strings for now in these pull requests but will keep these comments in mind in the future PRs where I may add back.

gestrich · 2023-02-23T00:13:40Z

Updates done - I think I responded to all the feedback but let me know if other questions.

…t#453) * extended bolus automatic to DoseActivationSource * naming update * refactor

ps2 · 2023-03-01T03:16:10Z

I still feel like sharing a model here is not great. There are going to be (at least) the following representations of these models:

Loop's internal representation (Swift dictionaries/types) after decoding from the notification.
The JSON format that Nightscout (or another service) produces when sending a remote notification, and that the Loop app accepts.
The JSON format that Nightscout accepts from the caregiver app.
LCG's internal representation before encoding to send to NS>

Decisions like whether to use var amountInGrams: Double or, say,var amount: HKQuantity as the internal representation is the responsibility/choice of the app itself. And evolution of the internal representations might change not quite in lockstep with the encoded JSON.

I know if probably feels like a violation of DRY to be repeating these models in two places, but it does seem to me like they are separate domains, that if not already, will sometime have different concerns, or want some flexibility.

This is just my opinion, and I needed to share it, I don't think this change will hurt Loop in a big way, and will not block on this if you really think it's the right way, or you really want to go this route for some reason.

gestrich · 2023-03-01T12:28:30Z

@ps2 I can understand the concerns about reusing these models in Remote 2.0 for decoding the payload. I'll keep that in mind for the upcoming Remote 2.0 PRs that introduce decoding.

Let me add a 5th item to the list that seems important to put in context with all of this:

Shared representation between Loop and the the remote plugins so that Loop can request a plugins actions. This needs to live in LoopKit to be seen by both.

While I added this as a 5th item, I feel this actually should be a further description on number 1 and should not be replicated, as this feels like the same domain (unlike decoding and such). I think my RemoteActions in this PR serve that role.

I think the upcoming PRs will add a lot of clarity when we get to domains 2-4.

ps2 · 2023-03-04T21:52:52Z

Yes, the Loop <-> Plugin interface is a shared domain, for sure and LoopKit is currently that kind of thing lives, so I'll merge this. Thanks!

gestrich · 2023-03-05T12:46:44Z

@ps2 Thanks - The next one I suggest reviewing/merging is this one here for the Loop repo LoopKit/Loop#1930

gestrich mentioned this pull request Feb 19, 2023

Remote 1: Action Models LoopKit/Loop#1930

Merged

gestrich commented Feb 19, 2023

View reviewed changes

Remote Action Models

335082e

gestrich force-pushed the feature/2023/02/bg/remote-action-models branch from 9b6d5a1 to 335082e Compare February 19, 2023 17:54

gestrich marked this pull request as ready for review February 19, 2023 17:55

ps2 reviewed Feb 19, 2023

View reviewed changes

Localize RemoteAction names

f6292c8

ps2 reviewed Feb 20, 2023

View reviewed changes

gestrich added 2 commits February 21, 2023 18:23

Move RemoteAction validation to extensions in Loop target

98859dd

Use Action verbiage rather than RemoteAction

3e736bf

gestrich changed the title ~~Remote Action Models~~ Remote 1: Action Models Feb 25, 2023

dybs pushed a commit to dybs/LoopKit that referenced this pull request Feb 28, 2023

[COASTAL-920] extended bolus automatic to BolusActivationType (LoopKi…

1985b9d

…t#453) * extended bolus automatic to DoseActivationSource * naming update * refactor

ps2 merged commit acd7817 into LoopKit:dev Mar 4, 2023

ps2 mentioned this pull request Apr 22, 2023

LOOP-887 Keycloak update and DIY Sync tidepool-org/LoopKit#496

Merged

Uh oh!

Conversation

gestrich commented Feb 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gestrich Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gestrich commented Feb 23, 2023

Uh oh!

ps2 commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gestrich commented Mar 1, 2023

Uh oh!

ps2 commented Mar 4, 2023

Uh oh!

gestrich commented Mar 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gestrich commented Feb 19, 2023 •

edited

Loading

gestrich Feb 20, 2023 •

edited

Loading

ps2 commented Mar 1, 2023 •

edited

Loading