-
Notifications
You must be signed in to change notification settings - Fork 4
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
[MRG] adding support for apple health #44
[MRG] adding support for apple health #44
Conversation
) | ||
new_analysis.save() | ||
except: | ||
pass |
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.
in which case would an exception be thrown here? maybe add a comment
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.
Good point, it's a rather general catch-all as the merging of Fitbit & Fitbit Intraday is really fickle and easily crashes (e.g. I stopped using my Fitbit some time ago but still have my data in my account. Fitbit still updates the data even and reports 0
for each day, this leads the whole pipeline to crash 🙈 ).
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.
not ideal but better than a server error ruining other processing/response? wouldn't hold this up based on improving this further
if i["source"] == "direct-sharing-453" and i["basename"].startswith( | ||
"heartrate_samples" | ||
): | ||
missing_sources.pop("apple_health", None) |
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.
Alright so this PR handles the case of the data having been imported by OH already, and doesn't offer an option to import them through QF. Is this planned for future?
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.
Nope, the Apple Health
data is a bit special, as you can't export it through an API or anything, but just through having the corresponding iPhone app installed that exports data. Which makes integrating it much harder.
To integrate this in QF we'd need to generate a whole new API to which to export the data (not to mention the fact that it took 2 months to get the app into the appstore :D).
So using the app we got now seems the way to go (plus it doesn't make much difference, as people need to use an external tool in any case).
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 see :D
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.
this bit of code reminds me that supporting the google fit data from the separate activity is missing :) I created #46
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.
Thanks @madprime!
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.
LGTM! I only looked at the code, didn't try running it. I made a minor PR that's just formatting the template code for readability (at least, that's what I intended), and added an issue that occurred to me while looking over the code. :)
if i["source"] == "direct-sharing-453" and i["basename"].startswith( | ||
"heartrate_samples" | ||
): | ||
missing_sources.pop("apple_health", None) |
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.
this bit of code reminds me that supporting the google fit data from the separate activity is missing :) I created #46
@@ -203,6 +214,26 @@ <h3>Manage Oura</h3> | |||
<a href="https://oura.openhumans.org">oura.openhumans.org</a> | |||
</p> | |||
{% endif %} | |||
</div> | |||
{% endif %} |
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 opened up the code wondering if this was an extra endif (it's not) & have a minor code formatting PR to share. just readability, as I was trying to check this over :)
) | ||
new_analysis.save() | ||
except: | ||
pass |
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.
not ideal but better than a server error ruining other processing/response? wouldn't hold this up based on improving this further
(minor) improve template code formatting
This PR:
RetrospectiveEvent
andSymptomReport
objects as is done for all other wearables.I've tested it with my data and it worked for all cases. I think it should be ready to be deployed!