[Stats Revamp] Changing "First day of week" results in incorrect numbers in Views & Visitors and Total Likes details

[staskus]

Description

Changing "First day of week" from Sunday to any other day in Language & Region, causes wrong numbers in Views & Visitors and Total Likes details pages after changing the date.

Stats views expect the week to be from Monday - Sunday but the method that calculates the date for this view uses the first day of week from settings.

Solution

The error happens after calling StatsPeriodHelper.calculateEndDate for a .week period.

After calculating the previous week, use the already created StatsPeriodHelper().weekIncludingDate to calculate the end of the week. After this fix, the date calculation works as expected.

Testing instructions

Enable "New Appearance for Stats" and "New Cards for Stats Insights" feature flags

Case 1 Views & Visitors:

1. Open Settings -> General -> Language & Region -> First Day of Week
2. Change day of week to "Sunday"
3. Open Stats
4. Click gear icon on top right
5. Add Views & Visitors card
6. Click "Week"
7. Go back in weeks and remember the numbers
8. Go back to settings and change day of week to "Monday"
9. Come back to Stats
10. Go back in weeks and make sure they are the same as in step 6

Case 2 Total Likes:

1. Repeat Case 1, but with "Total Likes" card

Case 3 Date and time close to the beginning or end of the week

1. Change the phone time to the beginning of the week or the end of the week (Monday 00:00 + site time zone difference, Sunday 23:59 + site time zone difference)
2. Make sure Case 1 and Case 2 report the same numbers

Regression Notes

1. Potential unintended areas of impact

Other parts of Stats that uses this calculation breaks

3. What I did to test those areas of impact (or what existing automated tests I relied on)

Manually testing old stats: Days / Weeks / Months and changing dates to make sure they continue working as expected.

4. What automated tests I added (or what prevented me from doing so)

None

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Images & Videos

Before the fix

First.Day.of.Week.-.before.fix.mp4

After the fix

First.Day.of.Week.-.after.fix.mov

[wpmobilebot]

You can test the changes in WordPress from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19829-a65ab7d on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[wpmobilebot]

You can test the changes in Jetpack from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19829-a65ab7d on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[staskus]

Updating the milestone to 21.6

[twstokes]

This LGTM @staskus! 🚀

• The week ranges stayed consistent in the "Views & Visitors" as well as the "Total Likes" cards.
• I didn't spot any regressions in other areas of the code that call calculateEndDate. I couldn't find any other paths that call it with the week unit.

It's interesting that there was a lot of overlap between the deleted code and the function we're calling now.

Some items to consider:

• ⚠️ I'm not certain about are the -12 hour offset in the DateComponents call. Would that introduce an edge case for certain time zones?
• The lastDayOfTheWeek seems like a really straightforward function to unit test - it would be really nice to consider that since dates are so tricky. 💣
• 📅 There are some other solutions floating around that appear to be a cleaner way of getting the end of the week.

[staskus]

@twstokes, thank you for the review! 🙏

It's interesting that there was a lot of overlap between the deleted code and the function we're calling now.

Yes, those were 2 identical pieces of code, so I 1) removed the duplication (by calling lastDayOfTheWeek) 2) made a change (by editing lastDayOfTheWeek)

⚠️ I'm not certain about are the -12 hour offset in the DateComponents call. Would that introduce an edge case for certain time zones?

That's an excellent question. I tried to track down and understand the reasoning. There's this comment that didn't spark too much confidence:

// (I *think* that's what's happening here. Doing Calendar math on this method has broken my brain.
// I spend like 10h on this ~50 LoC method. Beware.)

The lastDayOfTheWeek seems like a really straightforward function to unit test - it would be really nice to consider that since dates are so tricky.

That's a great suggestion. It would be a good way to trigger cases where this -12 is needed.

[sla8c]

@staskus I've taken a look at this and testing wise its fine. But I have a concern:

1. your change to StatsPeriodHelper also impacts non Stats Revamp ie current Stats screens and its not feature flagged. Additionally the change in StatsPeriodHelper.calculateEndDate for case .week used to always return a date (since it guard let'd to return currentDate) but the change you made can result in a nil being returned which is different + could be a bit worrisome re: other unknown impact(s)

2. I also tested across multiple timezones on device and didn't run into any issues but to echo what @twstokes said I think adding unit tests with variations on timezones would be a good thing to do since when I was first doing related code to this, I noticed that the change in timezone had a direct impact on this + keeping in mind our global userbase. Let me know if you'd like me to help with this! The unit tests can be a separate PR but we probably want to at least FeatureFlag your change for this PR

[staskus]

@sla8c, thanks for testing. It's a complicated issue, especially given the comments talking about corner case scenarios.

I added a couple of tests for what I think are important scenarios: Different first days of the week and dates being close to the beginning and end of the week.

When writing tests I couldn't find cases where this -12 change would be relevant. It was made in conjunction with other changes and I cannot reproduce the described issues anymore. There're more changes in the code after that and maybe this -12 is really not needed after all.

...but the change you made can result in a nil being returned which is different + could be a bit worrisome re: other unknown impact(s)

I could return currentDate in case the method fails. Although I couldn't think of a case where that would happen. As you mentioned, unknown impact(s), it's easy for me to revert this change.

adding unit tests with variations on timezones would be a good thing to do since when I was first doing related code to this, I noticed that the change in timezone had a direct impact on this + keeping in mind our global userbase. Let me know if you'd like me to help with this!

I haven't tested a variety of time zones, just a variety of different times and the different first days of the week. What kind of testing scenarios would you have in mind?

We probably want to at least FeatureFlag your change for this PR

I could do that just to be super-safe, although I couldn't find a single case where calculateEndDate would be used for .week period for older stats. After this change , "Week" tab takes data ranges from charts and not from this method. Other places, such as Weeks -> "Posts and Pages" uses this method but switch through the .day period for example.

[staskus]

@sla8c

Update:

• Returning currentDate instead of nil
• Added a Feature Flag check

[sla8c]

@staskus I agree that it is definitely a complicated issue

Thanks for adding some tests and adding the FeatureFlag! I'm approving + in a future PR, it would be good if maybe we add some more different timezone tests and maybe we should mention this (different timezone testing) as part of the stats test plan. Thanks for tackling this issue!

[staskus]

@staskus I agree that it is definitely a complicated issue

Thanks for adding some tests and adding the FeatureFlag! I'm approving + in a future PR, it would be good if maybe we add some more different timezone tests and maybe we should mention this (different timezone testing) as part of the stats test plan. Thanks for tackling this issue!

Time zones testing-wise are not playing a huge part, only the actual Date(). We call currentDateForSite() that uses the site timezone to create a new date by adding or subtracting time zone difference, so I think simply testing with different times is enough, and that includes unit tests.

I updated the testing scenarios to include time zone tests.