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

Button Expiry timezone is confused #624

Closed
cjyabraham opened this issue Dec 31, 2020 · 9 comments · Fixed by #630
Closed

Button Expiry timezone is confused #624

cjyabraham opened this issue Dec 31, 2020 · 9 comments · Fixed by #630
Assignees

Comments

@cjyabraham
Copy link
Collaborator

cjyabraham commented Dec 31, 2020

Before the WP Upgrade, things worked like this:
On my computer, chrome: I create a post and set the expiry date to one minute away in my local time. It correctly expires a minute later.

On Browserstack's Windows computer running chrome in Australia: I open the post and it shows the expiry time in PT, so that's neither my local timezone nor the computer's.

On Browserstack on MacOS running chrome in Australia: I open the post and it shows the expiry time in the Australia time zone.

After the WP Upgrade, things work like this:
On my computer, chrome: I create a post and set the expiry date to one minute away in my local time. It correctly expires a minute later. It shows the timezone as "EST" after the time for some reason.

On Browserstack's Windows computer running chrome in Australia: I open the post and it shows the expiry time in PT, so that's neither my local timezone nor the computer's.

On Browserstack on MacOS running chrome in Australia: I open the post and it shows the expiry time in the Australia time zone.

So the only change in the WP upgrade is the use of "EST" after the time. I assume the underlying issue is a bug that should be fixed in future versions.

Open question: The control in question is the DateTimePicker from @wordpress/components. Is that the correct control to be using for this or is it deprecated or something?

@cjyabraham cjyabraham self-assigned this Dec 31, 2020
@thetwopct
Copy link
Collaborator

There seems to be some history and updates here, continuing here, here and finally here. I think a general summary is that moment.js was introduced to localise the dateTimePicker component, but then removed as it was causing too many issues.

My own thoughts are that all times should be running to "WordPress time" - whatever the site timezone is in site settings - akin to "ship time". Introducing local times quickly gets confusing.

To add to these changes, in the Button with Expiry block plugin, the time() being used was UTC, no timezone was being specified. So an expiry time was being set in "site time" (maybe EST or possibly the users local time depending on how the dateTimePicker was working at the time) and being compared with a UTC time value.

I've now changed the Button with Expiry block to calculate in New York time, the same as what the WordPress LFE site is set too. Ship time.

The dateTimePicker is as the component is now distributed using the site time. Which for LFE is currently set to New York (EST).

After my update, when I see the dateTimePicker (using Chrome or Safari), it displays as EST (I am in UTC London).

2021-01-16-141843

When I use BrowserStack it also displays dateTimePicker in EST, even though the Geo-IP puts me in Netherlands and Google locate thinks I'm in Tbilisi (!). I think this confirms that the dateTimePicker is not geolocating based on the user timezone.

2021-01-16-142358

Now if I set an expiry date and time, it syncs with New York time, here's a video showing live clock, editor and frontend with the frontend refreshing after the time and the button now being expired to the second.

https://cln.sh/Z2tJ9C

@cjyabraham
Copy link
Collaborator Author

The two screenshots in your last comment show different times... is that because you snapped them exactly one hour apart? Or are they getting affected by their location?

@thetwopct
Copy link
Collaborator

thetwopct commented Jan 17, 2021

Nice spot. I just checked browser stack and that time does indeed change to be the current Hour and Minute based on the location, but this doesn't affect the timezone.

In the default for the block, the following is set:

		expireAt: {
			type: 'number',
			default: 60 * ( 1440 + Math.ceil( Date.now() / 60000 ) ), // 24 hours from Date.now
		},

The JS Date.now() probably needs NYC timezone attached to it...

@cjyabraham
Copy link
Collaborator Author

Setting things to NYC-time?? You're living in the past man!! I set my watch to Beijing time now... America is dead!!

@thetwopct
Copy link
Collaborator

thetwopct commented Jan 17, 2021

The block now pulls in the sites timezone to make its time decisions, so if the sites timezone is updated the block will reflect that.

When adding a button it now puts in the current time from sites timezone. i.e. It's 14:01 in London now where I am, 09:01 in New York, when I insert a button it displays as 09:01.

2021-01-17-140203@2x

I removed the previous default of "add 24 hours on to (current) users time" as I couldn't really see it being useful and it just adds complexity.

@cjyabraham
Copy link
Collaborator Author

A couple of weird things:

  1. If someone changes the site timezone, the numerical date and time doesn't adjust for the buttons so some which may have already expired may appear in the editor to be not expired. It's weird, however, as on the front-end they stay expired. I think this use-case is not going to occur for us so am fine leaving as is.
  2. I place a button and click "Button will expire". If I just save it then and refresh the editor, the time will continue to update to the current time. You have to explicitly set the time to have it "stick". Since this use-case of not setting the time at all is likely not going to happen we can probably leave it as is.

@cjyabraham
Copy link
Collaborator Author

This has been closed with the merge operation.

@cjyabraham
Copy link
Collaborator Author

This seems to be broken again.

When I open this post in my browser it shows this:
Screen Shot 2022-02-09 at 3 12 12 PM

But when I open it in a browserstack browser in India it shows this:
Screen Shot 2022-02-09 at 3 12 42 PM

So not showing EST time in either case.

@cjyabraham cjyabraham reopened this Feb 9, 2022
@cjyabraham cjyabraham removed the blocked label Feb 9, 2022
cjyabraham added a commit that referenced this issue Feb 13, 2022
cjyabraham added a commit that referenced this issue Feb 15, 2022
@cjyabraham
Copy link
Collaborator Author

this should be good now

cjyabraham added a commit that referenced this issue Feb 27, 2022
* Upgradeing to WP 5.9

* Remove defunct wp-cli.yml file

* Fix: Trying to get property 'post_parent' of non-object

* Make node nvrmc consistent with Lando local dev

* Updating wp-config layout to better distinguish between local dev and prod

* Updating NPM packages

* Fix: Upgrade Buttons with Expiry to work with WordPress 5.9

* Updating Image Box Block

* Updating Track Grid block

* Fix: Revert imagemin upgrade to match gulpfile

* Fix: Reverting change to lfe_get_event_parent_id as cannot reproduce PHP error

* Fix: Revert imagemin upgrade to match gulpfile

* Reset package files to diagnose build error

* fix expiry timing (#624)

* fix timezone issue (#624)

* improve logic for expiry of buttons; accommodate existing buttons

* simplify code a bit more

* change "View..." buttons in calendar to links (#677)

* Upgrade WP to 5.9.1

* made the past events link bolder and larger

* fix issue with setting country for KCDs

Co-authored-by: James Hunt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants