-
Notifications
You must be signed in to change notification settings - Fork 37
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
Send emails to users with localized timezone #61
Conversation
''' Get the meeting datetime for user. If user is specified, the timezone will be the user's | ||
timezone preference. If not specified, the timezone will be the meeting spec's timezone. | ||
''' | ||
if user and user.timezone: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ipwnponies I am a little concerned about this line. This doesn't seem backward compatible. I would expect this to blow up for older models that don't have a timezone property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should version beans - that way you can't run this code without also picking up the changes to the User model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we should version. Though I am not sure that addresses the problem here. I think that if we have old records that don't have the timezone entry (our current prod instance) and we update the model and attempt to access user.timezone, it will say that timezone does not exist on the user even though the model is defined properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't user.timezone
just return None then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is the case then we should be gtg. Just want to verify that behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to verify that behavior
I leave that in the very capable hands of @ipwnponies :)
I set something similar up for my fork for Twitch with an extra addition: I retrieve the user's timezone on the frontend and pass it to the backend for storage. Additionally, becoming timezone aware helps fix the breaking test_get_meeting_participants which starts to fail when clocks move back |
This adds support for sending emails to user's with localized timezone. Currently user timezone information does not exist so this change should not result in visible changes.
This still needs an end-to-end test but I'm unsure how to do this: