LOOP-1479: Implement first 2 screens of prescription flow + mock manager #11

novalegra · 2020-06-25T22:48:28Z

https://tidepool.atlassian.net/browse/LOOP-1479
Create Prescription data model
Create a mock prescription manager
Create a UICoordinator to navigate between views in the flow
Add button to TidepoolService setup view controller to trigger the onboarding flow
Create assets folder with Dexcom/Dash images
Create view model for initial part of flow
Create the first 2 screens of the onboarding flow (prescription code entry & device review)
- These are hosted in a DismissibleHostingController to prevent accidental dismissal, per discussions with design
- Because the TextField keyboard in the code entry screen may cover the field on smaller devices, I added a struct to keep track of the keyboard height and provide appropriate amounts of padding to keep the field in view

Add underlying infrastructure into Tidepool Service, a UI controller to coordinate prescription flow, & the prescription code entry screen

ps2

Looks really good! I think there are a couple of cleanup items that should be tackled, but otherwise this looks like a good first stab at implementing the prescription flow.

TidepoolServiceKit/Extensions/Prescription.swift

TidepoolServiceKit/Mocks/MockPrescriptionManager.swift

TidepoolServiceKitUI/Views/PrescriptionCodeEntryView.swift

ps2

LGTM! Would appreciate @darinkrauss taking a look before merge, though.

rickpasetto · 2020-06-26T17:10:10Z

@novalegra @ps2 I will need to utilize the Dexcom and Dash icons for my work. Would it make sense to put them into LoopKitUI?

ps2 · 2020-06-26T17:15:17Z

@rickpasetto Yours should come from the actual PumpManagers. Ideally Anna's should too, but we're not implementing device choosing in setup flow for v1, so I think just hardcoding them for now is fine.

darinkrauss

Looking great! 🚀

Please don't be alarmed at the number of nits as they are more a reflection of my particular-ness than anything else (and I add them to most all PRs 😄). Just consider them suggestions for your consideration. Feel free to ignore any or all of the nits.

Otherwise, I don't see a thing other than the nits, so LGTM! 🏎️

TidepoolServiceKit/Extensions/Prescription.swift

darinkrauss · 2020-06-26T16:33:08Z

TidepoolServiceKit/Extensions/TimeInterval.swift

+    public static func hours(_ hours: Double) -> TimeInterval {
+        return TimeInterval(hours * 60 /* minutes in hr */ * 60 /* seconds in minute */)
+    }
+}


[nit] Consider copy/paste this implementation from LoopKit:

static func minutes(_ minutes: Double) -> TimeInterval { return self.init(minutes: minutes) } static func hours(_ hours: Double) -> TimeInterval { return self.init(hours: hours) } init(minutes: Double) { self.init(minutes * 60) } init(hours: Double) { self.init(minutes: hours * 60) }

@darinkrauss doesn't TidepoolService have LoopKit as a dependency? Just curious...should she just make those LoopKit functions public and then use them?

darinkrauss · 2020-06-26T16:36:11Z

TidepoolServiceKit/Mocks/MockPrescription.swift

+    public let providerName: String // Name of clinician prescribing
+    public let cgm: CGMType // CGM type (manufacturer & model)
+    public let pump: PumpType // Pump type (manufacturer & model)
+    public let bloodGlucoseUnit: HKUnit // CGM type (manufacturer & model)


I know this object is a mock, but something similar will end up in TidepoolKit (using many of the types already implemented in TidepoolKit). Consider dropping use of HKUnit and just use another enum. This would allow you to drop the custom Codable implementation below. The enum could have a func/var that converts it into an HKUnit if you'd like.

This is a great suggestion, thanks!

darinkrauss · 2020-06-26T16:40:50Z

TidepoolServiceKit/Mocks/MockPrescriptionManager.swift

+            let timeZone = TimeZone(identifier: "America/Los_Angeles")!
+            let glucoseTargetRangeSchedule =  GlucoseRangeSchedule(
+                rangeSchedule: DailyQuantitySchedule(unit: .milligramsPerDeciliter,
+                    dailyItems: [RepeatingScheduleValue(startTime: .hours(0), value: DoubleRange(minValue: 100.0, maxValue: 110.0)), RepeatingScheduleValue(startTime: .hours(8), value: DoubleRange(minValue: 95.0, maxValue: 105.0)), RepeatingScheduleValue(startTime: .hours(14), value: DoubleRange(minValue: 95.0, maxValue: 105.0)), RepeatingScheduleValue(startTime: .hours(16), value: DoubleRange(minValue: 100.0, maxValue: 110.0)), RepeatingScheduleValue(startTime: .hours(18), value: DoubleRange(minValue: 90.0, maxValue: 100.0)), RepeatingScheduleValue(startTime: .hours(21), value: DoubleRange(minValue: 110.0, maxValue: 120.0))],


[nit] Consider breaking up into multiple lines as you did for the rest of this initializer.

darinkrauss · 2020-06-26T17:14:11Z

TidepoolServiceKitUI/View Models/PrescriptionCodeEntryViewModel.swift

+        if success {
+            didFinishStep()
+        } else {
+           // Handle error


[nit] TODO so it doesn't get missed?

darinkrauss · 2020-06-26T17:16:02Z

TidepoolServiceKitUI/View Models/PrescriptionCodeEntryViewModel.swift

+        }
+    }
+
+    func validateCode(prescriptionCode: String) -> Bool {


[nit] Consider how this reads "validate code prescription code". Perhaps validatePrescriptionCode(_ prescriptionCode: String)? Same for function below.

darinkrauss · 2020-06-26T17:17:21Z

TidepoolServiceKitUI/View Models/PrescriptionCodeEntryModel.swift

+    var prescription: Prescription?
+
+    func loadPrescriptionFromCode(prescriptionCode: String) {
+        #if targetEnvironment(simulator)


[nit] Consider just using Mock for simulator and non-simulator. After all, once TidepoolKit implements the backend API call we won't want the simulator to return a mock anymore.

darinkrauss · 2020-06-26T17:18:23Z

TidepoolServiceKitUI/View Controllers/PrescriptionReviewUICoordinator.swift

+
+class PrescriptionReviewUICoordinator: UINavigationController, CompletionNotifying, UINavigationControllerDelegate {
+    var screenStack = [PrescriptionReviewScreen]()
+    var completionDelegate: CompletionDelegate?


[nit] weak? Since the delegate is assigned to self below, won't this cause a retain cycle? Maybe not, but I generally assume all delegates should be weak to avoid the issue altogether.

darinkrauss · 2020-06-26T17:22:13Z

TidepoolServiceKitUI/View Controllers/PrescriptionReviewUICoordinator.swift

+    }
+
+    override func viewWillAppear(_ animated: Bool) {
+        screenStack = [determineFirstScreen()]


[nit] Will the first screen every vary?

Yes, we'll eventually be keeping track of progress within the flow in the case the screen quit

rickpasetto · 2020-06-26T19:15:48Z

TidepoolServiceKit/Extensions/TimeInterval.swift

+    public static func hours(_ hours: Double) -> TimeInterval {
+        return TimeInterval(hours * 60 /* minutes in hr */ * 60 /* seconds in minute */)
+    }
+}


@darinkrauss doesn't TidepoolService have LoopKit as a dependency? Just curious...should she just make those LoopKit functions public and then use them?

rickpasetto · 2020-06-26T19:24:30Z

TidepoolServiceKitUI/Views/PrescriptionDeviceView.swift

+        }
+    }
+
+    private var dashStack: some View {


Won't these icons and descriptions be vended from the device managers? Or is this just placeholder until that's in place?

This is a placeholder. The icons/descriptions would need to be static in order for this screen to be able to access them - @rickpasetto do you know if that's the case at the moment?

I am working on the ability to vend these images from the device managers. My recommendation is to put TODO comments in the code for both the device "buttons" (image, title, and description) and say that they are static placeholders for now

The device managers already do vend images for this use: https://github.com/tidepool-org/LoopKit/blob/dev/LoopKitUI/PumpManagerUI.swift#L18

But this PR should hardcode them for now. We have no mechanism for a Service plugin to access currently available device managers, and we don't need to as initially we are going to be not allowing device selection during onboarding.

@ps2 yes, but not before the devices are set up (in other words: as a static var instead of a member var). I'm working on changing that now.

rickpasetto · 2020-06-26T19:45:49Z

TidepoolServiceKitUI/Views/PrescriptionDeviceView.swift

+            UITableView.appearance().separatorStyle = .none // Remove lines between sections
+        }
+        .onPreferenceChange(WidthPreferenceKey.self) { widths in
+            if let width = widths.max() {


I'm curious...what does this do?

It aligns the images based on the max width

rickpasetto · 2020-06-26T20:05:34Z

TidepoolServiceKitUI/Views/PrescriptionDeviceView.swift

+        .resizable()
+        .aspectRatio(contentMode: ContentMode.fit)
+        .frame(height: 50)
+        .padding(5) // align with Dexcom


Hm...the way I did this in my screen was to fix the width of the image:

fileprivate struct LargeButton: View { let image: String let label: String let description: String static let spacing: CGFloat = 15 static let imageWidth: CGFloat = 48 static let topBottomPadding: CGFloat = 20 public var body: some View { return HStack(spacing: Self.spacing) { Image(frameworkImage: image, decorative: true) .frame(width: Self.imageWidth) VStack(alignment: .leading) { Text(label) DescriptiveText(label: description) } } .padding(EdgeInsets(top: Self.topBottomPadding, leading: 0, bottom: Self.topBottomPadding, trailing: 0)) } }

Looks like this:

rickpasetto · 2020-06-29T19:57:00Z

TidepoolServiceKit/Extensions/TimeInterval.swift

+import Foundation
+
+extension TimeInterval {
+    public static func minutes(_ minutes: Double) -> TimeInterval {


@novalegra why not use the existing LoopKit functions for this?

@rickpasetto We typically don't publicly export extensions to system types from our Frameworks. There are exceptions, but these are likely conflict sources; imagine if another framework did the same. I could easily see another framework thinking "wouldn't it be nice to implement minute/hour/day extensions for TimeInterval and export them for everyone?".

@ps2 I see. @novalegra in that case, it is fine to leave these as-is.

* Updating to new TidepoolKit with keycloak based auth * SettingsView using TidepoolService as ObservedObject * Tweak logo size * Fix issues with state restoration and re-logging in. Add alert when session loss is detected * Improve DataSetId caching * Do not allow environment switching when logged in * Tweak wording

* Embed TidepoolKit * Default to production environment * Use app provided client id * Tidepool backend does not support automated as bolus type * Fix upload of automatic boluses, and invalid pumpSettings. Updates for host identifier and version * Roundtrip localizations * Updates from Lokalise * Show login email on settings page * Updated translations from Lokalise on Thu Feb 9 13:30:18 CST 2023 * Do not upload Loop temp basals as automatic, to clean up Tidepool Web rendering * Move remote command parsing and validation to RemoteDataService * Revert "Remote PR Set #2: Introduce RemoteCommands" * Re-enable tracking of automatic flag for temp basal, and add for scheduled basal * Updated translations from Lokalise on Sat Mar 18 14:01:08 CDT 2023 * Updated translations from Lokalise on Sat Mar 18 15:11:44 CDT 2023 * Updated translations * Remove TidepoolKitUI reference from TidepoolServiceKitUITests * Tests compiling. Basal rate fixes. Not all tests passing yet * hostIdentifier/version updates for tests * Remove TidepoolKitUI references * Ensure food entries have name set * Nil names allow for meals, but not empty strings * Log errors during initialization * Mark automated boluses as such, and show environment in settings if not production * Update README.md * Update README.md * Updating to new TidepoolKit with keycloak based auth (#11) * Updating to new TidepoolKit with keycloak based auth * SettingsView using TidepoolService as ObservedObject * Tweak logo size * Fix issues with state restoration and re-logging in. Add alert when session loss is detected * Improve DataSetId caching * Do not allow environment switching when logged in * Tweak wording * Update tests * Remote PR Set 2: Introduce RemoteCommands * Support client-specific clientIds for auth * Remove unused code * Cleanup from review * Xcode project updates --------- Co-authored-by: Bill Gestrich <3207996+gestrich@users.noreply.github.com>

Anna added 23 commits June 18, 2020 09:47

LOOP 1269 - add prescription data model

04323bb

Undo commit to add project.pbx file changes

2d1bab6

Auto stash before revert of "LOOP 1269 - add prescription data model"

9af1488

LOOP-1269 Add prescription storage model

e0740c4

LOOP-1269 - add UI controller + first screen

0cc3b76

Add underlying infrastructure into Tidepool Service, a UI controller to coordinate prescription flow, & the prescription code entry screen

Add draft of 2nd screen

a2ce00e

LOOP-1269: complete first 2 screens

4b3fe92

Fix back button

63e84a0

Fix scrolling issue

21081ad

Widen text presentation area

6e1f2d2

Use mock prescription manager (for now) & make keyboard have letters

a1332ff

Remove second design from views

94136dd

Add colors to assets file

17a07f6

Add temporary correction ranges to prescription

74cafcc

Color dexcom icon & move delegates to right file

1067f7c

Add button to Tidepool setup controller to trigger flow

f34e32e

Use device specified in prescription

8cd0b68

Enforce a prescription code length

0aecd3d

Change copyright -> Tidepool Project

0f9faaa

Fix keyboard obstructing text field

30b176f

Fix keyboard swiping

98b5110

Fix animation issue and use return to dismiss

19a4734

Remove onboardingNeeded

1fec6b3

novalegra requested a review from ps2 June 25, 2020 22:48

Remove commented out code

5bd3617

novalegra requested review from rickpasetto and darinkrauss June 26, 2020 00:03

ps2 requested changes Jun 26, 2020

View reviewed changes

TidepoolServiceKit/Extensions/Prescription.swift Outdated Show resolved Hide resolved

TidepoolServiceKit/Mocks/MockPrescriptionManager.swift Outdated Show resolved Hide resolved

TidepoolServiceKitUI/Views/PrescriptionCodeEntryView.swift Outdated Show resolved Hide resolved

Anna added 2 commits June 26, 2020 09:30

PR-feedback changes

d2b7de9

Remove purple from assets

423e36f

novalegra requested a review from ps2 June 26, 2020 16:32

ps2 approved these changes Jun 26, 2020

View reviewed changes

darinkrauss approved these changes Jun 26, 2020

View reviewed changes

Feedback from Darin's review

2d6d53b

rickpasetto reviewed Jun 29, 2020

View reviewed changes

Anna added 2 commits June 29, 2020 12:47

Set width size

d1faedd

Add todo

ba933c5

rickpasetto reviewed Jun 29, 2020

View reviewed changes

novalegra merged commit 0471b79 into dev Jun 29, 2020

novalegra deleted the novalegra/LOOP-1479 branch June 29, 2020 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOOP-1479: Implement first 2 screens of prescription flow + mock manager #11

LOOP-1479: Implement first 2 screens of prescription flow + mock manager #11

novalegra commented Jun 25, 2020 •

edited

Loading

ps2 left a comment

ps2 left a comment

rickpasetto commented Jun 26, 2020

ps2 commented Jun 26, 2020

darinkrauss left a comment

darinkrauss Jun 26, 2020

rickpasetto Jun 26, 2020

darinkrauss Jun 26, 2020

novalegra Jun 26, 2020

darinkrauss Jun 26, 2020

darinkrauss Jun 26, 2020

darinkrauss Jun 26, 2020

darinkrauss Jun 26, 2020

darinkrauss Jun 26, 2020

novalegra Jun 26, 2020

darinkrauss Jun 26, 2020

novalegra Jun 26, 2020

rickpasetto Jun 26, 2020

rickpasetto Jun 26, 2020

novalegra Jun 29, 2020

rickpasetto Jun 29, 2020

ps2 Jun 29, 2020

ps2 Jun 29, 2020

rickpasetto Jun 29, 2020

rickpasetto Jun 26, 2020

novalegra Jun 29, 2020

rickpasetto Jun 26, 2020

rickpasetto Jun 29, 2020

ps2 Jun 29, 2020

rickpasetto Jun 29, 2020

LOOP-1479: Implement first 2 screens of prescription flow + mock manager #11

LOOP-1479: Implement first 2 screens of prescription flow + mock manager #11

Conversation

novalegra commented Jun 25, 2020 • edited Loading

ps2 left a comment

Choose a reason for hiding this comment

ps2 left a comment

Choose a reason for hiding this comment

rickpasetto commented Jun 26, 2020

ps2 commented Jun 26, 2020

darinkrauss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novalegra commented Jun 25, 2020 •

edited

Loading