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 Stats information fails to load when date cannot be parsed when returned from API #22910

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Mar 27, 2024

Fixes #22859

  • Make sure WPKit Podspec is correctly updated

We need to pass yyyy-MM-dd date to API when fetching stats/visits and other similar endpoints. However, in some cases, date conversions to date string fails.

The bug is related to TimeZone and DTS. For example, Paraguay has a DTS change at midnight 2023-10-01 which is ambiguous to DateFormatter and nil is returned.

Date formatter we use:

private var periodDataQueryDateFormatter: DateFormatter {
    let df = DateFormatter()
    df.locale = Locale(identifier: "en_US_POSIX")
    df.dateFormat = "yyyy-MM-dd"
    return df
}

Result for "America/Asuncion" time zone:

df.timeZone = TimeZone(identifier: "America/Asuncion")
df.date(from: "2023-10-01") // result nil

Result for "Europe/Vilnius" time zone:

po df.timeZone = TimeZone(identifier: "Europe/Vilnius")
po df.date(from: "2023-10-01") // result <Date> 2023-09-30 21:00:00 +0000

In documentation Apple suggests to set the timezone to UTC:

When working with fixed format dates, such as RFC 3339, you set the dateFormat property to specify a format string. For most fixed formats, you should also set the locale property to a POSIX locale ("en_US_POSIX"), and set the timeZone property to UTC.

RFC3339DateFormatter.timeZone = TimeZone(secondsFromGMT: 0)

However, this is not a straightforward solution, since in Stats we're working with dates that are in a different time zone.

Solution

Created RFC339NoTimeDateFormatter that sets:

df.locale = Locale(identifier: "en_US_POSIX")
df.dateFormat = "yyyy-MM-dd"
df.timeZone = TimeZone(secondsFromGMT: 0)

but also recalculates the data to the current time zone after conversion. Therefore, the dates remain unchanged but conversions also succeed. I added RFC339NoTimeDateFormatterTests as an example.

To test:

The easiest way to reproduce and confirm that the issue is fixed is to follow the steps from #22859

  1. Set defaultPeriodCount from 14 to 50 in SiteStatsTableHeaderView view so we can go back further back in dates
  2. Switch the time zone to Paraguay/Asuncion
  3. Open Stats Traffic tab
  4. Select weeks
  5. Come back to Sep 25 - Oct 1, 2023 week
  6. Confirm data loads

Video before the fix

Date.bug.mov

Video after the fix

Date.fix.mov

Regression Notes

  1. Potential unintended areas of impact

There shouldn't be any. WPKit-PR: wordpress-mobile/WordPressKit-iOS#771 adds unit tests to make sure that a new date formatter behaves in exact same way but additionally doesn't fail during DST changes.

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

Unit tests, manual tests

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

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.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 27, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22910-478d749
Version25.1
Bundle IDorg.wordpress.alpha
Commit478d749
App Center BuildWPiOS - One-Offs #10214
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 27, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22910-478d749
Version25.1
Bundle IDcom.jetpack.alpha
Commit478d749
App Center Buildjetpack-installable-builds #9263
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus staskus requested a review from guarani March 27, 2024 08:55
@staskus staskus modified the milestones: 24.6, 24.7 Mar 27, 2024
@staskus staskus modified the milestones: 24.7, Pending Apr 14, 2024
@staskus
Copy link
Contributor Author

staskus commented Jun 19, 2024

Update after #23366 is merged

@staskus staskus added the [Status] Blocked Waiting for a different task to be able to proceed label Jun 19, 2024
RFC339NoTimeDateFormatter creates yyyy-MM-dd date from string while taking into account DST (Daylight Savings Time)
@staskus staskus force-pushed the fix/22859-stats-traffic-card-information-fails-to-load-when-date-cannot-be-parsed branch from 46dd232 to 478d749 Compare June 21, 2024 11:59
@staskus staskus added [Pri] Low and removed [Status] Blocked Waiting for a different task to be able to proceed labels Jun 21, 2024
@staskus staskus requested a review from kean June 21, 2024 12:32
@staskus
Copy link
Contributor Author

staskus commented Jun 21, 2024

This is a fix for a low-priority corner case. I only found this bug to be reproducible for Paraguay/Asuncion time zones during daylight savings date. 🤔 Therefore, I'm conflicted about whether it merits significant date formatter updates, although I tried to be careful and cover corner cases with tests.

@staskus staskus removed the request for review from kean June 21, 2024 12:37
@staskus staskus added the [Status] Blocked Waiting for a different task to be able to proceed label Jun 21, 2024
@staskus staskus removed the [Status] Blocked Waiting for a different task to be able to proceed label Jun 21, 2024
@staskus staskus requested a review from kean June 21, 2024 12:55
@kean
Copy link
Contributor

kean commented Jun 21, 2024

Dropping a note here as well, as it was discussed internally: it might be worth switching to native date here to eliminate any potential issues with timezones, time, and Date conversions.

@staskus
Copy link
Contributor Author

staskus commented Jun 25, 2024

I'm closing this PR based on the previous discussion. This is an extremely rare corner case that only happens when parsing a specific date in a timezone on which DST starts at midnight. Other more generic solutions are preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stats Traffic: Card information fails to load when date cannot be parsed when returned from API
3 participants