WordPress-iOS icon indicating copy to clipboard operation
WordPress-iOS copied to clipboard

Fix Stats information fails to load when date cannot be parsed when returned from API

Open staskus opened this issue 1 year ago • 2 comments

Fixes #22859 WPKit-PR: https://github.com/wordpress-mobile/WordPressKit-iOS/pull/771

All of the changes and explanations are within WPKit PR. In short, DateFormatter fails to convert a date string (2023-10-01) string to Date on rare occasions when DST happens on that particular date at midnight.

This PR also contains additional changes due to a state of WordPressKit and WordPressAuthenticator. I'll update WP-iOS PR once WPKit-iOS changes are merged.

To test:

The easiest way to reproduce and confirm that the issue is fixed is to follow the steps from https://github.com/wordpress-mobile/WordPress-iOS/issues/22859

  1. Switch the time zone to Paraguay/Asuncion
  2. Enable Stats Traffic feature flag
  3. Open Stats Traffic tab
  4. Select weeks
  5. Come back to Sep 25 - Oct 1, 2023 week
  6. Confirm data loads

Regression Notes

  1. Potential unintended areas of impact

There shouldn't be any. WPKit-PR: https://github.com/wordpress-mobile/WordPressKit-iOS/pull/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.
  • [x] I have considered adding unit tests for my changes.
  • [x] I have considered adding accessibility improvements for my changes.
  • [x] 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)

staskus avatar Mar 27 '24 07:03 staskus

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
Commit478d7498df458d6f40f3f71a852f251797efc55e
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 avatar Mar 27 '24 07:03 wpmobilebot

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
Commit478d7498df458d6f40f3f71a852f251797efc55e
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.

wpmobilebot avatar Mar 27 '24 07:03 wpmobilebot

Update after https://github.com/wordpress-mobile/WordPress-iOS/pull/23366 is merged

staskus avatar Jun 19 '24 10:06 staskus

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 avatar Jun 21 '24 12:06 staskus

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.

kean avatar Jun 21 '24 18:06 kean

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.

staskus avatar Jun 25 '24 06:06 staskus