-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add subscriber language and localize emails #803
Conversation
|
||
|
||
def l10n(msg_id: str, args: Union[Dict[str, Any], None] = None) -> str: | ||
"""Helper function to automatically call fluent.format_value from context""" | ||
def l10n(msg_id: str, args: Union[Dict[str, Any], None] = None, lang: str = None) -> str: |
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.
The l10n helper now accepts a third parameter for a custom language, independent from the request.
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.
Some small changes requested. But a big thank you for getting this done!
|
||
def upgrade() -> None: | ||
# Add language column to subscribers table | ||
op.add_column('subscribers', sa.Column('language', models.encrypted_type(sa.String), index=True)) |
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.
Can we swap this to nullable=False, and default=FALLBACK_LOCALE?
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.
Of course.
@@ -133,6 +134,7 @@ class Subscriber(HasSoftDelete, Base): | |||
|
|||
name = Column(encrypted_type(String), index=True) | |||
level = Column(Enum(SubscriberLevel), default=SubscriberLevel.basic, index=True) | |||
language = Column(encrypted_type(String), index=True) |
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.
Can we swap this to nullable=False, and default=FALLBACK_LOCALE?
self.confirmUrl = confirm_url | ||
self.denyUrl = deny_url | ||
self.schedule_name = schedule_name | ||
default_kwargs = {'subject': l10n('confirm-mail-subject', {'name': name})} | ||
lang = kwargs['lang'] if 'lang' in kwargs else None |
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.
Do we need to do this? kwargs is passed through to the constructor (and BaseBookingMail's constructor.) and it doesn't seem like it's explicitly used in the constructor elsewhere.
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.
I thought it would be a good idea to not require lang to be in the kwargs, but yeah, I see it makes sense that it's there. I will update it.
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.
Ohh gotcha. Yeah in cases where lang is required for mail we should probably try and be explicit about it.
Otherwise we'd have another Ryan bug report 😄
|
||
# Prefill new column with default value | ||
session = Session(op.get_bind()) | ||
subscribers: list[models.Subscriber] = session.query(models.Subscriber).all() |
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.
You'll have to confirm but having a default value and nullable=False should remove the need for this.
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.
I thought so too, in fact I first tried it with default values. But it didn't work, the language column would always stay empty... I thought maybe it's because of the encrypted type? Maybe you have another idea?
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.
I switched to db defaults, you can test the branch again, if you like
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.
Looks like it works with server_default (which means the default value defined in the table as opposed to the default value sqlalchemy provides), so switched it over to that.
I appear to have broken every test. Oops. |
Nice catch with the server_default, didn't think of that! But what's up with the tests now 😂 |
Will roll back to default= as server_default expects an encrypted string because we encrypt the field. We also can't encrypt it at the model definition time because the env / secret key isn't available then 🤔 |
…ation to remove nullable.
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.
Perfect, thanks for your hard work here!
Description of the Change
This PR adds a new field
language
for the subscribers table and extends our l10n middleware for using a custom locale.The tests were adjusted and extended accordingly.
Benefits
Subscribers should see emails sent to them in their configured language now.
Applicable Issues
Closes #766