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

Post Scheduled Date Time Zone Conversions #13128

Merged
merged 11 commits into from Dec 17, 2019
Merged

Conversation

@bjtitus
Copy link
Contributor

bjtitus commented Dec 17, 2019

Fixes #12992
Fixes #13080
Fixes #13132

This adds date formatters which consider the site's time zone instead of relying on device time zone settings. After this change, times shown in the Settings screen will show the time according to the site's time zone.

Example Post (w/ screenshots)

Here are some screenshots illustrating the same scheduled post (Scheduled for 10:00AM Central Standard Time, 9:00AM in my local Mountain Standard Time) across the web and iOS products after this change:

Screen Shot 2019-12-17 at 8 31 22 AM Screen Shot 2019-12-17 at 8 31 04 AM

Screen Shot 2019-12-17 at 8 31 04 AM  Screen Shot 2019-12-17 at 8 31 04 AM

Here are the key changes:

  • Added testDisplayDate method to test whether the display date is real. Verified this by changing the date formatter to use the incorrect time zone and the test was able to catch it.
  • Adds SiteDateFormatters to pull blog's time zone from BlogService
  • Adds new Date Formatter for use in PostSettingsViewController this will change the formatting behavior for that cell regardless of the feature flag
  • Sets the time picker to the current time of the day in the site's time zone

Reviewing Other Platforms

I checked this behavior against the web client behavior and there are a few things I wanted to point out:

  • Typically, the Gutenberg editor shows the date in the site's time zone. One exception to this is when the site's time zone is changed after a post is scheduled. In that case, the timestamp from the original time zone is still shown. I believe this is a bug and could lead to some user confusion since the timestamp is neither in the site time zone or the device time zone. iOS handles this appropriately by showing the date in the site's current time zone.

  • The list of posts on the web displays the time in the device time zone. This is consistent with the iOS post list as well, although web uses relative date formats so it's a little clearer when the post is about to be published (i.e. "in 5 minutes").

Testing

  • Set the Site Time Zone to a different Time Zone than the device
  • Schedule a post
  • Check that the displayed scheduled time matches the site's time zone and is published on that date

Future Considerations

  • The new SiteDateFormatters struct could be augmented to automatically cache the date formatters. These settings screens don't have a lot of performance concerns, but caching the formatters might be desirable in the future. The ActivityDateFormatting struct is very similar and a more unified and updated date approach
  • Gutenberg uses the date and time format set in site settings. I think it would be valuable to use this on mobile somewhere, but it might not fit in the cell. It should be possible to convert the PHP date format strings to Unicode date format strings. This would probably be a bigger task to consider for a larger date overhaul.

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. This is a smaller bug fix for a feature that was not released to production.

bjtitus added 3 commits Dec 16, 2019
* Adds date formatters which consider the time zone.
* Adds localized time zone string to make it more obvious which time zone will be used.
* Adds past published message for footer text
* Moves the time zone specific date formatters or publishing to their own location
* Adds unit tests around screens to ensure date values are correctly passed
* Adds `testDisplayDate` to verify that the timezone conversion (as shown in the Publish Settings screen, at least) handles basic time zone conversion
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Dec 17, 2019

You can trigger an installable build for these changes by visiting CircleCI here.

@bjtitus bjtitus added this to the 14.0 milestone Dec 17, 2019
bjtitus added 3 commits Dec 17, 2019
Sets the time picker to the selected date's time (current or previously selected time depending on the situation)
@ScoutHarris

This comment has been minimized.

Copy link
Contributor

ScoutHarris commented Dec 17, 2019

Hey @bjtitus . Regarding your comment about release notes -

This is a smaller bug fix for a feature that was not released to production.

#13080 is actually a live bug, and if you've fixed that here maybe we could add a release note specifically for that being fixed?

@ScoutHarris ScoutHarris self-requested a review Dec 17, 2019
@bjtitus

This comment has been minimized.

Copy link
Contributor Author

bjtitus commented Dec 17, 2019

@ScoutHarris So I went back and checked version 13.4.0 so I think you're right that the incorrect timestamp issue may go back a ways. What do you think of this for the Release Notes?

* Fixed a bug that displayed incorrect time stamps for scheduled posts.
@ScoutHarris

This comment has been minimized.

Copy link
Contributor

ScoutHarris commented Dec 17, 2019

What do you think of this for the Release Notes?

That'll work!

Copy link
Contributor

ScoutHarris left a comment

I have a couple requests, but since you're going to make additional changes I'll stop and finish the review afterwards.

//
// Created by Brandon Titus on 12/16/19.
// Copyright © 2019 WordPress. All rights reserved.
//

This comment has been minimized.

Copy link
@ScoutHarris

ScoutHarris Dec 17, 2019

Contributor

Could you remove this comment block please? We don't normally leave them.

footerText = String.localizedStringWithFormat("Post will be published on %@ in your site timezone (%@)", publishedOnString, offsetLabel)
let publishedOnString = viewModel.dateTimeFormatter.string(from: date)

let nameStyle: NSTimeZone.NameStyle = viewModel.timeZone.isDaylightSavingTime(for: date) ? .daylightSaving : .standard

This comment has been minimized.

Copy link
@ScoutHarris

ScoutHarris Dec 17, 2019

Contributor

The display format of the timezone has changed. As originally designed and implemented:

Screen Shot 2019-12-17 at 1 21 16 PM

If we revert back to that display, checking isDaylightSavingTime should not be necessary.

This comment has been minimized.

Copy link
@bjtitus

bjtitus Dec 17, 2019

Author Contributor

I thought this format may match the "Site Timezone" setting a little better. i.e. if you set to UTC-7 it will reflect that as GMT-07:00 but if you use Chicago it will properly reflect that the timezone is in use and would use whatever rules apply to that zone.

I think that avoids some confusion when you might wonder why "UTC-7" is shown when you did not pick a Manual Offset option.

@ScoutHarris ScoutHarris changed the title Fixes #12992, #13080: Post Scheduled Date Time Zone Conversions Post Scheduled Date Time Zone Conversions Dec 17, 2019
@bjtitus

This comment has been minimized.

Copy link
Contributor Author

bjtitus commented Dec 17, 2019

@ScoutHarris Thanks for the feedback! I addressed all of those requests. Should be ready for final review!

@bjtitus bjtitus requested a review from ScoutHarris Dec 17, 2019
Copy link
Contributor

ScoutHarris left a comment

I think you've got it!

:shipit:

@bjtitus bjtitus merged commit bf1eaa8 into develop Dec 17, 2019
7 checks passed
7 checks passed
Hound No violations found. Woof!
Peril All green. Nice work.
Details
ci/circleci: Build Tests Your tests passed on CircleCI!
Details
ci/circleci: Installable Build/Hold Your job is on hold on CircleCI!
Details
ci/circleci: UI Tests (iPad Air 3rd generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone 11) Your tests passed on CircleCI!
Details
ci/circleci: Unit Tests Your tests passed on CircleCI!
Details
@bjtitus bjtitus deleted the issue/12992-post-scheduled-date branch Dec 17, 2019
@bjtitus bjtitus mentioned this pull request Dec 17, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.