3.0: Reports\get_filter_value( 'dates' ) returns incorrect date range

[pippinsplugins]

Bug Report

The Reports\get_filter_value( 'dates' ) method returns an incorrect date range when passed a value of today.

Expected behavior

Date range returned should be 00:00:00 of the current day until 23:59:59 of the current day.

Actual behavior

A multi-day range is returned.

Example return: Array ( [from] => 2018-11-06 [to] => 2018-11-07 [range] => today

[pippinsplugins]

Seems this actually affects all ranges.

For example:

Last Month returns a range that includes the last day of the month before the previous month (I.E September 30 - October 31).

This Month returns October 31 to November 30.

Today returns November 5 to November 7.

Yesterday returns November 4 to November 6.

Custom with 2018-11-05 and 2018-11-05 selected actually returns November 4 to November 7.

[pippinsplugins]

I believe this has to do with the setTimezone() method that used a lot:

'start' => $date->copy()->setTimezone( edd_get_timezone_id() )->startOfDay()->setTimezone( 'UTC' ),
'end'   => $date->copy()->setTimezone( edd_get_timezone_id() )->endOfDay()->setTimezone( 'UTC' ),

We if remove the usage of setTimezone( edd_get_timezone_id() ) the dates are closer to what is expected.

@DrewAPicture can you weigh in on this please?

[sunnyratilal]

This is actually correct behaviour. @JJJ and I spent a lot of time with the issue of time zones.

As of 3.0, all dates will be stored in UTC and as part of that we need to apply offsets to dates/times to ensure the correct offsets get passed into the query classes.

For example, if your server offset is UTC - 2 and we are using the “today” range, we need to query the previous day. The database will return the correct records and then we apply further offset calculations to ensure the rows have your server time displayed.

If the offset calculations are removed it’ll cause all the reports to break and subsequently report inaccurate data so these steps had to be taken.

The offset calculations are applied all across Core now, even on the list tables to ensure the correct data is returned from the database.

FWIW, @JJJ and I have extensively tested this to ensure that it does indeed work and I wrote a plethora of unit tests to verify that this is indeed the correct behaviour.

Happy to elaborate further.

[DrewAPicture]

I think part of the confusion here actually has to do with the use of setTimezone() in the code. DateTime/Carbon doesn't actually convert times when you change the timezone of the date, it just changes the timezone of the object, if that makes sense.

This is why there is (was?) code that messed with the timestamps before generating a DateTime/Carbon object.

The only ways to actually convert the dates and times pulled from the database in UTC into "WP time" is to generate the object and then mess with the timestamp and/or timezone, or manually convert it first then instantiate a DateTime/Carbon object with that value and timezone.

The use of objects were in large part added to aid in calculations and formatting but conversions have to happen externally.

[DrewAPicture]

Yeah, it's still in there in the Date class. Really it comes down to the use of setTimezone(). This is probably not the droid you're looking for ;-)

From Date::__construct():

<?php
	// Apply the WP offset based on the WP timezone that was set.
	$offset   = $this->getOffset();
	$interval = \DateInterval::createFromDateString( "{$offset} seconds" );
	$this->add( $interval );

[pippinsplugins]

This will sound harsh, so excuse me please.

If a site admin requests a report to display values for Today and they are shown a 2-3 day range instead, that is simply not okay.

It doesn't matter if that's technically correct, even if there are dozens of unit tests that are passing, because that's never going to be accepted by store owners. If they ask for sales from today, they should rightly expect to get sales from today, not a multi-day range.

@sunnyratilal am I mis-understanding what you described? If so, is there just a problem with the date display not being converted to local time?

[JJJ]

Since all of the times in the data are UTC, we should be querying for the correct offset, then formatting it to display the offset dates and times.

The string problem is largely between Carbon and PHP’s own strtotime(), which has numerous oddities when it comes to words like “Today”, “Yesterday” and “Tomorrow”, especially regarding return values and length-of-day math.

I’ve avoided using the Carbon implementation that Drew wrote up, because the magic-method-math never seems to do what I expect it to. I’ve said this in Slack and other Github issues, so I don’t wanna repeat myself, but I generally agree with Pippin that the helpers haven’t helped me yet.

Sunny did implement them in a few places, with tests.

If the dates are coming back with incorrect/invalid values, like Drew eluded to, we need to make sure we are using the correct methods in the correct ways and their intended places. We may not be, as there is a lot of calculating happening.

[pippinsplugins]

I think it may just be a display problem.

@sunnyratilal @DrewAPicture, when displaying a human-readable date, how should I be doing it? Can you show me how I should convert a UTC date to a local date with the newly built methods?

[DrewAPicture]

OK, so I looked through the updated code a bit, and it seems like there's a few things going on here.

1. At some point, edd()->utils->date() was changed to default to instantiating objects in UTC instead of WP Time. This came in with the addition of the optional $localize parameter.

The API was originally built with ease of conversion in mind, so once this change was made, stuff got way more complicated. That's neither here nor there, but I think that was probably the catalyst.

2. Some date manipulations use direct manipulations with newly-instantiated Carbon instances and the Carbon->subSeconds() method and others use the standard interval juggling in the Date constructor I referenced before, i.e. $something = edd()->utils->date(). Add in the wrinkle of $localize defaulting to false, we see a lot of use of edd()->utils->date( something, null, true ) which is what is needed to trigger a date conversion to WP.

Edit: Actually I had that last part wrong. $localize has to be set to false for the date to be converted based on the offset of the supplied timezone. So the only way to trigger conversion from UTC to WP is to literally supply the $timezone parameter and leave $localize as false.

I think this is probably because the original code was merged sans Carbon. Now that Date extends Carbon in master, we should standardize on using Carbon->subSeconds() in the Date constructor for timestamp manipulation. We should also pretty much never instantiate a Carbon object directly. Everything should flow through Date or edd()->utils->date(), if for nothing else, access to the quick formats in Date::format().

3. Looking through the usage of utils->date(), there seems to be a lack of standardization on one good method to get from a stored UTC date to a WP time version and vice versa. See above.

4. I think we're going to need to audit all uses of edd()->utils->date(). I see a some problem areas where
   i. Dates displayed in the UI aren't being converted before display (Example)
   iii. Dates are first applying the offset then removing it (Example)
   iv. etc.

Takeaways: I think we need to reconsider tying localization to WP time conversion in this API. Yes, both are related in some respects to display, but the latter has a much higher likelihood of needing to be used all over the place – this just a reality of storing in UTC and converting out to WP time and vice versa.

I'm not intending to point out flaws in thinking or place blame or anything, I think Pippin has just pointed that we need to tighten this stuff up and standardize as much as possible.

We have Carbon now, so let's standardize on leveraging its methods. Let's make sure that we're always displaying WP time in the UI and converting to UTC for queries and db manipulations. Let's make sure that we're always running dates through a single pipe so behavior can be predictable.

[DrewAPicture]

@pippinsplugins to answer your question directly

when displaying a human-readable date, how should I be doing it? Can you show me how I should convert a UTC date to a local date with the newly built methods?

With the current code, this is how it works in both directions:

<?php
// UTC to WP
$tz   = edd()->utils->get_time_zone();
$date = edd()->utils->date( 'today', $tz );

$date->format( 'datetime' ) // date_format time_format

// WP to UTC
$date = edd()->utils->date( $date_from_UI );

$date->format( 'mysql' ) // YYYY-MM-DD HH:MM:SS

[JJJ]

The problem with displaying everything in WP time, is that might not correspond to the time the user is in. They enter their time, now we have to convert it 3 ways, and we don’t know what time zone they are in.

This is how all of WordPress’s GMT issues have always lurked around.

Site set to New York.
I live in Chicago.
I enter January 1, 2019 1:00am Chicago time.
Our code does the UTC - New York math.
Saves as UTC - 5.
Should be UTC - 6.
Everyone thinks the data is OK, because they never see the error, because we format it back out to UTC - 5 without saying so.

This is why, I think, all the formatting and juggling of time zones in the admin area is risky.

Anywhere we ask users to input a time, we should be denoting to them what offset we will be magically applying.

[JJJ]

I would never have figured all of that out, and I’m still not sure what the context is to need to do any of that. 😃

[DrewAPicture]

I guess I never really considered localization to be an option in terms of input. When the Date class was committed, I deliberately left localization to the new edd_date_i18n() functionality because it's not really related to time conversion in terms of storage or retrieval. It's display functionality, right?

Just like we've essentially drawn a line in the sand in regard to treat all pre-3.0 dates as "UTC" regardless of whether they are or not, I think we should probably choose what types of input we want to support in the long term. I would be in favor of denoting the timezone any input represents if you think that would be beneficial.

[pippinsplugins]

There is a number of key details to determine here, but here's one I'm certain on: user-facing dates should always be displayed in WP user local time unless it explicitly states to the user on the screen that the displayed date is in UTC or any other timezone.

[cklosowski]

I agree with @pippinsplugins here. If we're giving a report for today, then the report needs to be for the end user, not UTC. If the site is set to a timezone for WP time, then the report needs to be in that WP Time. For instance if I look at a report on the EDD site, it is set to US CST, I should see the report based off that site time, not my own timezone (MST which is -1 hour right now) or UTC.

[JJJ]

Unfortunately, there is no such thing as WP user local time.

We have server time, WP site time, and UTC. We do not know the time from the browser being used by the user. We can ask for it if we want it, and I think that’s ultimately the best thing to do.

Use some JavaScript to convert the time stamps from UTC to client browser timezone.

At least we store everything as UTC. 😊

If the WP site time is UTC - 13 and I live in UTC - 6, then what? If we don’t handle this client side, it’s a mess.

[cklosowski]

@JJJ, I guess what I mean by User time is WP Site time.

[JJJ]

The problem with using the WordPress Site time for display, is orders and things end up looking as if they happened in the future. The WordPress "local time" is local to the server, not any 1 user.

Example

1. I live in UTC-6, and you live UTC-7. In the screenshot below, my site is UTC+10:

2. I created an order, with a date_completed value of 2018-12-14 16:48:00.

3. In our Order Completed column as a UTC-6 user, the order took place in the future:

4. ...and as a UTC-7 user:

Epilogue

Notice that regardless of your or my timezone, the order is still many hours in the future.

---

If time is critical, and I believe it is, we need to do these things:

• Know the timezone of the client browser
• The logged in user tells WordPress their preferred timezone

If we use the WordPress time, nothing is ever correct.

[JJJ]

Here's a single screenshot that shows off why we can't rely on WordPress Site Time:

• Customer registered on 12-16, at 05:29 UTC
• WordPress Site Time is UTC+6
• My time is 9:35 PM (UTC-6)
• List table shows them as having been created 12 hours in my future

[JJJ]

Related 7 year old WordPress Trac ticket: https://core.trac.wordpress.org/ticket/18146

[cklosowski]

Given that we have to move on something here and the trac ticket for user based time settings is...no where near ready, I think we have to go with the devil we know for the display time.

We've dealt for years with times being the site time not my time. It happens every time I publish a blog post on the EDD site, since it's set to central and i'm on MST. I just know that the store is 2 hours (or 1 hour) ahead of my timezone.

Displaying that a customer was created 12 hours in the future for the current user is just the nature of the WP interface at this point. It is based off the store's timezone.

[JJJ]

I'll be over here saying "I told you so" from UTC-6 in the future. :D

[mintplugins]

I think sticking with what WordPress does for now is probably the unfortunate-but-best-current-option, and if/when they add user-level date picking, we should flip to that.

If WordPress was a javascript generated UI like Gutenberg, passing the date to the frontend in UTC, and then converting+outputting it by running through the browser's toLocaleDateString method would be the most accurate localization of the date. But we aren't there yet.

So my vote is that we just stick to what WordPress does for now.

I add this next part because it's a personal pet peeve of mine with the internet in general. My personal view is that a date should never be shown to the user without a timezone beside it.

It's a Murphy's Law thing:

• Don't append a timezone = possible (and thus definite) user confusion about which timezone
• Append a timezone = no possible user confusion about which timezone

I'm not saying we need to add the timezone here, but more that I would rather WordPress do that across the board.

[mintplugins]

Here's an interesting thing I just discovered. Charts.js seems to use your browser's timezone setting. So you pass it UTC timestamps, and no matter what, it converts the time of the events into your browser's timezone.

See: chartjs/Chart.js#4334

[mintplugins]

Here's some of the changes you'll notice.

1. When viewing reports the orders on the chart shoudld now show in your timezone, and you'll also see your timezone's abbreviation wherever times or dates are shown to provide more clarity:

You can then change your timezone in WordPress, and the date of the sale should now be accurately reported in that new timezone:

I was in Toronto, and now I am changing to Vancouver, which is 3 hours earlier:

Additionally, the Orders page now consistently outputs the order date in the user's timezone, and converts it to UTC prior to saving.

[mintplugins]

For the solutions I've added here, I went through several different iterations, one of which was much more intense than this one. I'm not 100% happy with this approach, but it was what I was able to do at the current time. Ultimately I think we need to address our dates/times handling on a larger scale, but I believe this should resolve the issue as stated in the original scope.

[ashleyfae]

I pushed up one minor fix that was the exact same as the last one I pointed out. c643bbc I did a bunch of testing and all the graph dates look good to me.

One thing: I'm not sure if it's still within the scope of this issue because it's not graph-related, but it looks like the Date column in Reports > Customers (for "Top Five Customers" and "Most Valuable Customers") is only displayed in UTC and not converted to the site time zone. It definitely should be because everything else in this whole area is.

[mintplugins]

@nosegraze Those should be showing the in the current timezone now:

[ashleyfae]

@mintplugins All good now!