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

Match WHO Dashboard - Add one day to stats #1921

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

theswerd
Copy link
Collaborator

Closes #1917

Screenshots

Simulator Screen Shot - iPhone 12 Pro Max - 2021-01-10 at 21 47 20

How did you test the change?

  • iOS Simulator
  • iOS Device
  • Android Simulator
  • Android Device
  • curl to a dev App Engine server
  • other, please describe

Checklist:

@matthewblain
Copy link
Contributor

Is this a timezone issue, a server issue, or a parsing issue?

@brunobowden
Copy link
Collaborator

@matthewblain - timestamps are for midnight, so it's ambiguous which day it applies to. It looks as though the WHO dashboard uses the date of when the data was reported to WHO - this is different to the date on which the case or death occurred. Adding one day is consistent with this, so the data that appears suddenly gets the date on which it was reported.

@@ -75,7 +75,13 @@ class _RecentNumbersBarGraphState extends State<RecentNumbersBarGraph> {
final date = DateTime.fromMillisecondsSinceEpoch(
widget.timeseries[index].epochMsec.toInt());
// Abbr to fit on single line: e.g. "Oct 18, 2020"
final formattedDate = DateFormat.yMMMd().format(date);
final formattedDate = DateFormat.yMMMd().format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extract this to a shared method to avoid duplicated code.

This should also be heavily commented to explain why it exists.

@brunobowden
Copy link
Collaborator

@matthewblain - you could make a case for fixing this on the server.... but that would break the data consistency between the ArcGIS output and the rest API.

@matthewblain
Copy link
Contributor

I guess I think it should just be defined what it is. It's a time_t variant--are they all actual times, or midnight in some timezone, I looked at a sample of data from last month and they all appear to be midnight UTC. DateTime.fromMillisecondsSinceEpoch will generate an item in the local timezone, though there is an option to have be UTC. Then formatdate does... something with it.

What is the date on the WHO dashboard generated from? Does it take the UTC midnight time and generate a string date from that, then display that everywhere in the world?

@matthewblain
Copy link
Contributor

As a concrete example, for Nigeria, on the WHO dashboard web site, it shows 1145 new cases for Dec 18. This matches with data for timestamp 1608249600000. Which is Dec 18, 0:00 Zulu. Which also happens to be 2020-12-17T16:00:00 in Pacific time.

Has this bug been verified with the device timezone set on both sides of UTC?

@stale
Copy link

stale bot commented Jan 18, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label Jan 18, 2021
@stale stale bot removed the resolved:stale No recent activity on the issue or PR label Jan 25, 2021
@matthewblain
Copy link
Contributor

I just tested, and this is not the correct solution. It is a timezone issue.
Try this: Set the timezone on your device to, say, America/Los Angeles (currently UTC-8). It will show the date as being some particular day (in my case today, it shows the 24th). Change the timezone on your device to, say, Asia/Bangkok (currently UTC+7). Reload the stats page, and it will show the date as the next day (e.g. the 25th).

@theswerd
Copy link
Collaborator Author

@matthewblain, do you think setting the timezone to UTC will work?

@matthewblain
Copy link
Contributor

Yes, it's quite possible using the isUtc argument on the constructor will do the trick:
DateTime.fromMillisecondsSinceEpoch(

int millisecondsSinceEpoch,
{bool isUtc: false}

)

@brunobowden
Copy link
Collaborator

@matthewblain - I tip my hat to you. That would make more sense. Certainly, it shouldn't change depending on the local device time or time zone.

@brunobowden
Copy link
Collaborator

I would've preferred that the server API used dates instead of timestamps to avoid any ambiguity.... but that's not important for now.

Copy link
Collaborator

@brunobowden brunobowden left a comment

Choose a reason for hiding this comment

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

@theswerd - please look in to @matthewblain's comment about time zone. That is a plausible explanation to me. I don't know what time zone the data is in but I'll ask WHO.

@@ -75,7 +76,7 @@ class _RecentNumbersBarGraphState extends State<RecentNumbersBarGraph> {
final date = DateTime.fromMillisecondsSinceEpoch(
widget.timeseries[index].epochMsec.toInt());
// Abbr to fit on single line: e.g. "Oct 18, 2020"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move comment to the extension as the comment it common to both usages and not specific to line 78

@@ -0,0 +1,13 @@
import 'package:intl/intl.dart';

extension WHOFormat on DateTime {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you wanted to write an extension ;-) I would've included the DateTime.fromMillisecondsSinceEpoch step also as that's common to both usages.... and it's the msSinceEpoch that has midnight ambiguity. By the time it's converted to a date, it should be correct, so doing +1 in the format step doesn't fit so well to me.

I would've just extracted a single method and not bothered with a new file. We can move it to a separate file if something else uses it. I think though that this formatting may be specific to the WHO dashboard.... no idea if it extends to the

String whoDashboardDateFormat(int epochMS) {
  var date = DateTime.fromMillisecondsSinceEpoch(epochMS);
  // Timestamps are for midnight so the day it applies to is ambiguous.
  // Add 1 day to match the WHO Dashboard: http://covid19.who.int
  date.add(Duration(days: 1))
  // Abbr to fit on single line: e.g. "Oct 18, 2020"
  return DateFormat.yMMMd().format(date);
}

@stale
Copy link

stale bot commented Feb 2, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label Feb 2, 2021
@stale stale bot removed the resolved:stale No recent activity on the issue or PR label Feb 7, 2021
@stale
Copy link

stale bot commented Feb 14, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved:stale No recent activity on the issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stats associate numbers with prior day (off by one error)
3 participants