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

Fix test get meeting participants #110

Merged
merged 4 commits into from
Jan 30, 2018

Conversation

kentwills
Copy link
Contributor

fixes #64.

@ipwnponies I don't think we have you as a reviewer, but could you take a quick look at this timezone fix.

I verified the emails:
screen shot 2018-01-01 at 5 16 44 pm

I verified the frontend:
screen shot 2018-01-01 at 5 17 38 pm

Additionally I added/fixed tests.

@kentwills kentwills self-assigned this Jan 2, 2018
Copy link
Contributor

@ipwnponies ipwnponies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, if I understand this correctly. We're getting the subscription's timezone (America/Los Angeles) and then converting the meeting's datetime from utc to local using this timezone. Since America/Los Angeles is UTC offset agnostic, this will work correctly.

This will give us the local time of the meeting. What's the plan to localize this to individual users? That is, once we decide that 2018-01-04 13:00 America/Los Angeles is when we want meeting, how do we handle changing to local time for a participant who is in Phoenix?

@kentwills
Copy link
Contributor Author

@ipwnponies

screen shot 2018-01-05 at 11 00 34 am

I wish NDB had a Time property (timezone agnostic property), but it doesn't. I am dropping all of the Date data for the field:

Subscription DateTime = 1/2/2017 13:00 <--Date is dropped along with any timezone information, we just pull out 13:00

When we move to Meeting Datetime we create a new datetime, set the time to 13:00 and apply the timezone which looks like this on 2/2/2018:

Meeting datetime 2/2/2018 13:00 PDT

Because a timezone hasn't been set on the new date, the .localize function just tags on the PDT without modifying the original time stored in the database. This is a bit confusing as time's that are stored for subscriptions are different than times stored for meetings, but it is the only way that I can get the right time throughout the year.

This is stored in the database in UTC so if we wanted to handle local time, we can transform from the UTC value stored in the DB.

@ipwnponies
Copy link
Contributor

@kentwills Refresh my memory here but I think this is going to cause issues.

This is a table that shows the three actors: meeting subscription, yelp beans user 1 in America/Los Angeles, and yelp beans user 2 in America/Phoenix.

Name Location Meeting time preference Meeting local time in 2018-02-01 Meeting local time in 2018-07-01
Meeting Subscription SF 21:00 UTC 21:00 UTC 21:00 UTC
User1 SF 13:00 PST (-8) 13:00 PST 13:00 or 14:00 PDT (-7) ???
User2 Phoenix 14:00 MST (-7) 14:00 MST 14:00 MST

How does the logic work out against this use case? I've assumed meeting time as UTC because that's the "base" timezone from you localize to each user's specific timezone.

Can we add tz info to the datastore as a new text? This will allow us to use UTC everywhere and have timezone information that can be used for localizing time just before presentation (email or preference page).

@kentwills
Copy link
Contributor Author

Name Location Meeting time preference Meeting local time in 2018-02-01 Meeting local time in 2018-07-01
Meeting Subscription SF 21:00 UTC 21:00 UTC 21:00 UTC

This example provided oversimplifies the situation.

There is a Meeting Subscription Datetime - this represents the time we want to have the meeting when we create a new one it is stored in UTC in the database, but when it is retrieved, it is timezone agnostic.
This line doesn't convert the time, instead it adds the timezone without conversion. We ignore timezone for storing the preference and add the timezone when creating the individual meeting instance. When we store this back in the database it is converted to UTC. This means that we have a central expectation for when the meeting will happen and we adjust to meet that expectation every time the meeting is created.

Given the situation (with a Meeting Subscription Datetime of 21:00) above, we would instead see this:

Name Location Subscription Meeting Time preference Meeting Instance Time Date
Meeting Instance SF 21:00 UTC 2018-02-01
Meeting Instance SF 22:00 UTC 2018-07-01

Does this make sense? Does this use of the localize function create confusion?

@ipwnponies
Copy link
Contributor

Apologies for the confusion and back-and-forth. The changes are very straightforward, after reading the pytz.timezone.localize() documentation. I'm also guilty of not re-reading my very own notes for the fix in #64.

@kentwills
Copy link
Contributor Author

Noted Pre-commit failure on master branch, but not reproducible on local or personal travis account.

The 'pre-commit-hooks==0.7.0' distribution was not found and is required by the application

Will follow up with a fix if the error continues.

@kentwills kentwills merged commit 8a3c9bd into Yelp:master Jan 30, 2018
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.

Daylight Savings Time Bug
2 participants