Skip to content
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

Fix incorrect dates shown at ends of time ranges #149

Merged
merged 2 commits into from
Apr 11, 2018

Conversation

pluehne
Copy link
Contributor

@pluehne pluehne commented Apr 9, 2018

Because of a mistake, the end date of time ranges as shown in the dashboard was not copied by value but by reference. As an effect, displayed dates would be repeatedly decremented by one second every time users hovered data points. For this reason, displayed dates could vary when hovering them multiple times.

This is easily fixed by copying the affected variable by value.

@pluehne pluehne added the bug label Apr 9, 2018
@pluehne pluehne self-assigned this Apr 9, 2018
@pluehne
Copy link
Contributor Author

pluehne commented Apr 9, 2018

@toddocon: I discovered this issue while debugging your bug report in #147. I think this one could be related or even the explanation for #147, but I’m not entirely sure. I manually set my time zone to PST, and all dates look fine to me now.

So if you could try out this patch, this would be much appreciated 😄.

Also, when testing this, could you check whether the bug in #118 still persists on your side?

@pluehne pluehne changed the title Fix incorrect dates at range ends Fix incorrect dates shown at ends of time ranges Apr 9, 2018
@@ -429,7 +429,7 @@ function createHistoryChart(canvas, actionBar)

// The date range is of the form [t0, t1), inclusive of t0 but exclusive of t1.
// Hence, subtract one second from t1 to obtain the previous date in UTC
let t1 = dateRange[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you see an easy way to cover that with a test case? E.g. move tooltipTitleCallback to a "real" function and then test this function with some demo data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this as well, but now that you think of unit-testing this as well, I probably should do this tomorrow 🙂.

This adds Moment time zone support. While this isn’t needed from the
dashboard, we’ll need the library for mocking up custom time zones,
which is useful when unit-testing functions related to date and time.
Because of a mistake, the end date of time ranges as shown in the
dashboard was not copied by value but by reference. As an effect,
displayed dates would be repeatedly decremented by one second every time
users hovered data points. For this reason, displayed dates could vary
when hovering them multiple times.

This is easily fixed by copying the affected variable by value. Multiple
test cases ensure that no off-by-one errors or local time zone issues
occur.
@pluehne pluehne force-pushed the patrick/fix-incorrect-dates-at-range-end branch from f8e1e83 to 03e647d Compare April 10, 2018 15:09
@pluehne
Copy link
Contributor Author

pluehne commented Apr 10, 2018

@larsxschneider: I added some unit tests, so please review this again 🙂.

@codecov-io
Copy link

Codecov Report

Merging #149 into master will increase coverage by 1.3%.
The diff coverage is 83.33%.

@larsxschneider larsxschneider merged commit 9366cee into master Apr 11, 2018
@larsxschneider larsxschneider deleted the patrick/fix-incorrect-dates-at-range-end branch April 11, 2018 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants