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

Set TZ in transformer to America/New_York from EDT #188

Merged

Conversation

ThorntonMatthewD
Copy link
Collaborator

@ThorntonMatthewD ThorntonMatthewD commented Nov 13, 2023

Summary

Switches the timezone used for casting event times in EventDataTransformer from using a type 2 "EDT" timezone to a type 3 "America/New_York" timezone that will automatically account for daylight savings time.

Testing Plan

  1. Before checking out this branch run php artisan pull:events
  2. Launch the application and navigate to /events
  3. The times, since we've fallen back from EDT to EST, will appear an hour later than they're actually scheduled for. Compare with the OpenWorks events dashboard.
  4. Also be sure to stop by the /calendar page to see that the same issue exists there.
  5. Checkout the branch for this PR and re-run php artisan pull:events
  6. Refresh the /events and /calendar pages. The times should now appear correctly.

@allella
Copy link
Member

allella commented Nov 13, 2023

Thanks for working on this @ThorntonMatthewD

We do have the TZ .env variable that I think becomes the config/app.php as the 'timezone'.

I suspect we shouldn't hard code the timezone. Or, we could pull from the config, and use the New York as the default value if the config isn't set?

https://laravel.com/docs/10.x/configuration#accessing-configuration-values

@allella
Copy link
Member

allella commented Nov 13, 2023

I ran Matt's PR as a patch on stage and it fixed the issue, so I ran it on the live site too.

I guess there's a chance we have a problem again when daylight savings reverts, but the hardcoded EDT in the app/Data/EventDataTransformer.php seems like a fair chance of being the issue.

I think we should pull the America/New_York from the Laravel config, instead of hard coding it. So, that's the main thing I think we should change on this PR.

Thanks Matt.

@ThorntonMatthewD ThorntonMatthewD marked this pull request as ready for review November 15, 2023 04:22
@bogdankharchenko bogdankharchenko self-requested a review November 15, 2023 13:25
@allella
Copy link
Member

allella commented Nov 15, 2023

@ThorntonMatthewD were you trying to do anything more on this PR, or is it ready to go?

@ThorntonMatthewD
Copy link
Collaborator Author

@allella The couple of unit tests were all I was after since yesterday. Since those are up I think this iteration of the TZ fix is ready for review.

@ThorntonMatthewD
Copy link
Collaborator Author

^ I actually forgot to change back one last thing. NOW things are good. 😄

@allella allella merged commit 6d4ecd0 into hackgvl:develop Nov 15, 2023
1 check passed
@ThorntonMatthewD ThorntonMatthewD deleted the adjust-event-date-transformer-tz branch November 15, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants