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

🐞 Marketplace: order time is displayed in UTC #1660

Closed
Tracked by #1331 ...
anaulin opened this issue Jul 13, 2023 · 9 comments
Closed
Tracked by #1331 ...

🐞 Marketplace: order time is displayed in UTC #1660

anaulin opened this issue Jul 13, 2023 · 9 comments
Assignees
Labels
🐞 bug Something isn't working

Comments

@anaulin
Copy link
Member

anaulin commented Jul 13, 2023

This order was placed at 2:15 AM local time, but in Convene it shows as:
image

We should try to display this timestamp in the user's timezone (as inferred from the browser).

@anaulin anaulin added the 🐞 bug Something isn't working label Jul 13, 2023
@zspencer
Copy link
Member

Do you have thoughts on how we can determine the appropriate time zone? Should we just presume PT as the application default for now? Or should we let it be set on the Person and Space level?

@anaulin
Copy link
Member Author

anaulin commented Jul 14, 2023

I was thinking we could use something like https://github.com/kbaum/browser-timezone-rails to set the timezone for each request.

@anaulin
Copy link
Member Author

anaulin commented Jul 14, 2023

Also, we could do one or all of:

  • let a Space configure a default timezone
  • let a Person configure their default timezone (and fallback to their browser)

@rosschapman
Copy link
Contributor

Converting the value automatically using the browser timezone seems sufficient to me unless Piikup mentioned they wanted configurability to handle some use case?

I tend to like an abstraction around 3rd-party lib helpers like humanized_money_with_symbol. Like, creating our own ViewComponent called DateTime or something. It cold parameterize TZ in case it does become dynamically set later.

@anaulin
Copy link
Member Author

anaulin commented Jul 18, 2023

@rosschapman agreed on starting simple, since nobody has asked for anything special, to the best of my knowledge.

I also like the idea of encapsulating the whole thing, although a helper method might be more lightweight than a ViewComponent, just so that in views we can do something like:

Order placed at <%= in_user_tz(date: date) %>

instead of:

Order placed at <%= render DateComponent.new(date: date) %>

Although if we use something like the gem I suggested above (which is used by one of the clients I work for), then we don't need to do anything special in views, because it sets the timezone on a per-request basis at the Rails level, so when you display a date in the Rails timezone it shows up in the correct timezone.

@zspencer
Copy link
Member

I believe localize(date, :format) is what you're looking for from a "how to format dates?" perspective; as this calls #to_formatted_s under the covers.

In most of the rails app I work on with time zones; we use Time.use_zone in an around_action to ensure the requests / response use a specified time zone in both ApplicationController and ApplicationMailer to call Time.with_zone.

Since emails are delivered off the thread that handles requests/responses; I don't know that the browser time zone gem will work quite right to fix this...

This leads me to think that for now, having a Neighborhood time zone set via ENV variable is probably the "cheapest thing that will work."

But I agree, we may want to support person and space level time zones once we start crossing those boundaries.

@zspencer
Copy link
Member

Aight, gonna tackle this now.

@zspencer zspencer self-assigned this Jul 26, 2023
zspencer added a commit that referenced this issue Jul 26, 2023
- #1660

This is a small step towards a better way for handling time; since we
currently only operate out of the SF Bay Area; it seems like a
reasonable place to start.

At some point we'll want to add in time zone support for browsers,
users, spaces, etc. but this gets us to a place where things are a
little better sooner rather than later
zspencer added a commit that referenced this issue Jul 26, 2023
- #1660

This is a small step towards a better way for handling time; since we
currently only operate out of the SF Bay Area; it seems like a
reasonable place to start.

At some point we'll want to add in time zone support for browsers,
users, spaces, etc. but this gets us to a place where things are a
little better sooner rather than later
@zspencer
Copy link
Member

Patch is out: #1706

zspencer added a commit that referenced this issue Jul 26, 2023
…imes (#1706)

`Neighborhood`: Use a default `TimeZone` everywhere

- #1660

This is a small step towards a better way for handling time; since we
currently only operate out of the SF Bay Area; it seems like a
reasonable place to start.

At some point we'll want to add in time zone support for browsers,
users, spaces, etc. but this gets us to a place where things are a
little better sooner rather than later
@zspencer
Copy link
Member

zspencer commented Aug 9, 2023

I'm closing this since it's been in prod for a while and appears to be working well.

@zspencer zspencer closed this as completed Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants