-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update routes to limit range to 30 days #118
Conversation
mpgxvii
commented
Mar 5, 2024
- Fix hrv, breathing rate, and skin temp routes to limit request date range to 30 days
- Solves Limit the date range of Fitbit requests #117
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.
Please find a comment below. Can you also add the link to the documentation that states a limit of 30 days on these endpoints?
|
||
public class FitbitBreathingRateRoute extends FitbitPollingRoute { | ||
private final FitbitBreathingRateAvroConverter converter; | ||
protected static final Duration REQUEST_INTERVAL = Duration.ofDays(30); |
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 would rename this to THIRTY_DAYS
and put it in the top level class, then it can be returned in downstream classes getDateRangeInterval()
function. That way you don't have to define it more than once and make the code more readable.
From what i can see, seems like we don't request for more than 30 days in a single request anyways? |
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 (let's do some final testing in stage).
Oh I was thinking if the start date was > than 30 days before today, it would be the case? One of the users reported these logs:
|
oh ok, could be the case. But we also see these same logs in our connector (which is not collecting any of these endpoints), so not sure what causes it. Maybe we need better reporting of failed requests in the logs (like the code, message, etc returned from failed HTTP request to determine the root cause). This is a worthy feature update regardless, so i would proceed to merge. |