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

Send emails to users with localized timezone #61

Closed
wants to merge 3 commits into from

Conversation

ipwnponies
Copy link
Contributor

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:

  • Send test email to ensure timezone works
  • Do dataprovider sync to test the model change.

''' 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:
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

@kentwills kentwills May 30, 2017

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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 :)

@ny2ko
Copy link
Contributor

ny2ko commented Dec 24, 2020

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

@ymilki ymilki added python Pull requests that update Python code Type: Enhancement labels Jul 30, 2021
@kentwills kentwills closed this Jul 30, 2021
@ymilki
Copy link
Member

ymilki commented Jul 30, 2021

beans has changed a lot since 2017. We do need to fix the timezone issues, but at this point, a new PR would be more appropriate. #145 #141 #122 are tracking some specific symptoms related to timezone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants