Skip to content

Conversation

ewollesen
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Jul 22, 2025

PR Preview Action v1.6.2

🚀 View preview at
https://developer.tidepool.org/TidepoolApi/pr-preview/pr-166/

Built to branch gh-pages at 2025-09-08 18:19 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

tags:
$ref: ./clinic/models/patientTagIds.v1.yaml
glycemicRanges:
$ref: ./clinic/models/glycemicRanges.v1.yaml
Copy link
Contributor

@toddkazakov toddkazakov Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that clinics will be able to define their own glycemic ranges (post MVP). This should be a ref to that custom configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this slack message/thread for more context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this slack message/thread for more context.

I guess my thinking was that when the custom ranges are added, a new value will be added to the enum custom or something, and a new field could be added with whatever info is needed for the custom ranges.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clinicians will be able to define more than one custom range (at the clinic level), but will be able assign a single one to a patient. Each custom range can have a custom name. The enum abstraction won't work in this case.

I think the data model should look like:

GlycemicRange
 - id [string]
 - name [string]
 - thresholds [GlycemicRangeThresholds]
 - ...
Clinic
  - id [string]
  - name [string]
  - ...
  - glycemicRanges [GlycemicRange]

Patient
  - id [string]
  - ...
  - activeGlycemicRangeId [string] (optional)

This will accommodate current needs and future iterations

@ewollesen ewollesen force-pushed the eric-glycemic-range-selection branch from 29d2de1 to ca150c8 Compare July 30, 2025 14:39
@ewollesen ewollesen requested a review from toddkazakov July 30, 2025 18:29
@ewollesen ewollesen force-pushed the eric-glycemic-range-selection branch from ca150c8 to 72e762e Compare September 8, 2025 18:15
toddkazakov
toddkazakov previously approved these changes Sep 9, 2025
Copy link
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of changes and some thoughts.

@ewollesen ewollesen force-pushed the eric-glycemic-range-selection branch from 07bcde8 to 3e7fc96 Compare October 10, 2025 16:23
darinkrauss
darinkrauss previously approved these changes Oct 10, 2025
Copy link
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Clinicians will be able to select from one of a limited set of
possible glycemic ranges which will in the future affect summary
calculations.

BACK-3879
Clinicians will be able to select from one of a limited set of
diagnosis types which will in the future be used for filtering.

BACK-3872
The result of code review, the glycemic ranges model is expanded for
the future eventuality of a list of custom defined thresholds.

This required some gymnastics to get oapi-codegen to generate a type
that would reliably parse from JSON and reasonably be represented in
BSON.

BACK-3879
As per code review, the spaces have been removed so they're more like
code values than plain text words.

BACK-3879
I guess these aren't allowed here, but that fact was hidden by other
errors, and so wasn't visible until now.

It doesn't appear to make a difference to the generated code, so I
guess it wasn't having any effect either way.
@ewollesen
Copy link
Contributor Author

Latest force push was just to rebase onto latest master, which fixes linting errors. /cc @lostlevels

@ewollesen ewollesen requested a review from lostlevels October 14, 2025 20:47
@@ -0,0 +1,13 @@
properties:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an id here?

Copy link
Contributor Author

@ewollesen ewollesen Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an id here?

I don't believe so. Based on our most recent discussion, since we're only using presets for now, this should be OK.

Future work for clinic-custom-presets might change that in some way, but for now, the presets are all just enums, so no id is needed.

x-omitzero: true
items:
$ref: ./clinic/models/site.v1.yaml
glycemicRanges:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we can have at most one glycemic range active per patient. Did the requirements change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed in the past that it's better if this is denormalized and is a references a glycemic range defined at the clinic level by id. Did anything change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed in the past that it's better if this is denormalized and is a references a glycemic range defined at the clinic level by id. Did anything change?

From our latest discussion I think we've decided that this is OK for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants