-
Notifications
You must be signed in to change notification settings - Fork 477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove unused Import Settings in JSON that cause errors decoding JSON #249
Remove unused Import Settings in JSON that cause errors decoding JSON #249
Conversation
Thanks for your contribution. The changes look good, but I am not overly familiar with these details, and don’t have other looping data to test from. Hoping someone with fresh profile data uploaded to NS from both Loop and AAPS will have a chance to test. Import of manual NS profile entries would also be good to test here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No comments regarding the code. Seems reasonable to exclude what's not needed from the fetch.
Successfully tested by following these steps:
- Applied suggested changes from PR In simulator build
- Cleared out basal/isf/cr/target and dia settings from simulator build
- Found old MongoDB Loop profile settings entry, cloned that in MongoDB and applied current date as startDate
- Saved this profile in Nightscout
- Run "Import settings" in Trio
- Successfully fetched all Loop profile setting (basals/isf/cr/dia and target)
- Deleted the cloned loop profile in MongoDB
- Saved most current Trio profile (before the loop clone) in Nightscout again
- Run "Import settings" in Trio
- Successfully fetched all Trio profile settings (basals/isf/cr/dia and target)
Works as expected regarding Loop Profile import. Doesn't break iAPS/Trio profile imports. ie Looks good to me. Approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this change - relying on others for code review.
I use Loop and have never been able to import settings into Trio before.
After applying the patch from this PR, I was able to import my Loop settings from my "real" nightscout site. I confirmed the imported settings in the Trio app of my test phone match my Loop settings: DIA, Basal Profile, ISF, CR and (low-end of) Target Range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review looks good to me. I couldn't find any references to the deleted variables. Looking at the profiles in Nightscout I can see that Loop uses Default
wheres Trio uses default
, so that section of the code looks good too.
I tested in a simulator by uploading settings from Loop to a spare Nightscout account, then trying to import settings into Trio, which failed. After applying this PR, though, the settings were properly imported into Trio.
My only suggestion to consider is maybe setting target to the middle of the target range from Loop rather than the bottom target, since Trio only uses a single target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at the code and LGTM
I don't use loop, so can't say anything about the function.
And maybe we need to make a separate PR for this?
@kskandis are you also up for that? |
So this looks good according to current reviews. Could someone also quickly check that NS-generated profiles are also imported correctly? |
And possibly AAPS-generated profiles? @t1dude ? |
I got to test this now, and can confirm that changes made from the NS profile editor are correctly imported. For target, only the lower bound is imported, the upper bound is ignored, as noted by others above. For me this is LGTM. Would be interesting to know if AAPS profiles are supported too, but that’s a little out of scope for this PR. |
I will merge this with 4 approvals and 4 testers. @t1dude if you could please test this with AAPS-uploaded profiles after the fact; if a fix for AAPS-profiles is necessary, we will record this as a new issue ticket. @MikePlante1 comment regarding not using just the lower bound of Loop's correction range can be handled via #252 . Thank you everyone for testing and reviewing. Thank you @kskandis for your contribution to Trio. |
That makes sense to me. Yes, I'll look into it. |
…ttingsLoop Remove unused Import Settings in JSON that cause errors decoding JSON
Resolves Nightscout CGM Import Settings fails with error by: