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

feat: Email made translatable #7416

Merged
merged 17 commits into from
Nov 8, 2020

Conversation

codedsun
Copy link
Contributor

@codedsun codedsun commented Nov 5, 2020

Fixes #7405

Short description of what this resolves:

Changes proposed in this pull request:

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@auto-label auto-label bot added the feature label Nov 5, 2020
@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #7416 (0a5b601) into development (f0cf6a2) will increase coverage by 0.01%.
The diff coverage is 68.75%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #7416      +/-   ##
===============================================
+ Coverage        65.28%   65.29%   +0.01%     
===============================================
  Files              265      265              
  Lines            13205    13212       +7     
===============================================
+ Hits              8621     8627       +6     
- Misses            4584     4585       +1     
Impacted Files Coverage Δ
app/api/helpers/system_mails.py 100.00% <ø> (ø)
app/api/auth.py 24.15% <50.00%> (+0.32%) ⬆️
app/api/users.py 31.66% <75.00%> (+0.38%) ⬆️
app/instance.py 87.76% <80.00%> (-0.28%) ⬇️
config.py 89.02% <100.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0cf6a2...a76ee91. Read the comment docs.

@codedsun codedsun changed the title feat: Revamp user registration mail feat: Email made translatable Nov 6, 2020
app/api/auth.py Outdated
send_email(
to=user,
action=PASSWORD_RESET_AND_VERIFY,
html=render_template('email/password_reset_and_verify', **context),
Copy link
Member

Choose a reason for hiding this comment

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

pass context as keyword arguments

Copy link
Member

Choose a reason for hiding this comment

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

.html missing

app/api/auth.py Outdated
send_email(
to=user,
action=PASSWORD_RESET,
html=render_template('email/password_reset', **context),
Copy link
Member

Choose a reason for hiding this comment

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

pass context as keyword arguments

Copy link
Member

Choose a reason for hiding this comment

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

.html missing

app/api/users.py Outdated
to=user,
action=USER_REGISTER,
subject=MAILS[USER_REGISTER]['subject'].format(app_name=settings['app_name']),
html=render_template('email/user_register.html', **context),
Copy link
Member

Choose a reason for hiding this comment

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

pass context as keyword arguments

app/api/auth.py Outdated
subject=MAILS[PASSWORD_CHANGE]['subject'].format(
app_name=get_settings()['app_name']
),
html=render_template('email/password_change'),
Copy link
Member

Choose a reason for hiding this comment

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

.html missing

@@ -0,0 +1,3 @@
Please use the following link to reset your password.
<br/><a href='{link}' target='_blank'>{{ link }}</a>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<br/><a href='{link}' target='_blank'>{{ link }}</a>
<br/>{{ link }}

@@ -0,0 +1,2 @@
Please use the following link to reset your password and verify your account.
<br> <a href='{link}' target='_blank'> {{ link }} </a>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<br> <a href='{link}' target='_blank'> {{ link }} </a>
<br>{{ link }}

@codedsun
Copy link
Contributor Author

codedsun commented Nov 7, 2020

@iamareebjamal - mo file created.

{{ _('Hello') }},
<br/><br/>{{ _('Thank you for your ticket order for') }} {{ order.event.name }}.
<br/><br/>{{ _('This is your order confirmation. You can download your tickets at') }} : {{ order_view_url }}
<br/><br/>{{ _('Order Summary') }}:
<br/>Order: {{ order.invoice_number }}
<br/>Name: {{ order.user.full_name }}
Copy link
Member

Choose a reason for hiding this comment

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

_() missing here

<br/><br/>Ticket Summary:
{{ _('Hello') }},
<br/><br/>{{ _('This is a confirmation mail of your tickets for the event') }} {{ order.event.name }}. {{ _('You can download your tickets at') }}: {{ order_view_url }}
<br/><br/>{{ _('Ticket Summary') }}:
<br/>Name: {{ attendees.0.user.full_name }}
Copy link
Member

Choose a reason for hiding this comment

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

_() missing here

@@ -0,0 +1,7 @@
{{ _('Hello') }},
<br/><br/>{{ _('Your account has been created on') }} {{ settings.app_name }}. {{ _('Congratulations! ') }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<br/><br/>{{ _('Your account has been created on') }} {{ settings.app_name }}. {{ _('Congratulations! ') }}
<br/><br/>{{ _('Your account has been created on') }} {{ settings.app_name }}. {{ _('Congratulations!') }}

@@ -0,0 +1,7 @@
{{ _('Hello') }},
<br/><br/>{{ _('Your account has been created on') }} {{ settings.app_name }}. {{ _('Congratulations! ') }}
<br/><br/>{{ _('Your login is: {{ email }} ')}}
Copy link
Member

Choose a reason for hiding this comment

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

email should be out of _()

<br/><br/>{{ _('Your login is: {{ email }} ')}}
<br/><br/>{{ _('Please visit the following link to verify your email:') }} {{ link }}
<br/><br/>{{ _('Thank You') }},
<br/><br/>{{ settings.app_name }} Team
Copy link
Member

Choose a reason for hiding this comment

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

_() missing here

@codedsun
Copy link
Contributor Author

codedsun commented Nov 7, 2020

@iamareebjamal - While passing context as keyword argument in send_email function , the jinja html template doesn't recognize values and give error object not found. What should be done?

@iamareebjamal
Copy link
Member

You don't pass context as kwarg, you pass contents of context as kwargs

app/api/users.py Outdated
action=USER_REGISTER,
subject=MAILS[USER_REGISTER]['subject'].format(app_name=settings['app_name']),
html=render_template('email/user_register.html', context=context),
attachments=None,
Copy link
Member

Choose a reason for hiding this comment

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

If attachments is None, then this is not needed

config.py Outdated
@@ -49,6 +49,7 @@ class Config:
FLASK_ADMIN_SWATCH = 'lumen'

VERSION = VERSION_NAME
LANGUAGES = ['en', 'de', 'fr']
Copy link
Member

Choose a reason for hiding this comment

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

Add all languages present in the translations folder

messages.pot Outdated
@@ -0,0 +1,125 @@
# Translations template for PROJECT.
Copy link
Member

Choose a reason for hiding this comment

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

Delete this file

Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

Add required dependencies in requirements

@codedsun
Copy link
Contributor Author

codedsun commented Nov 7, 2020

@iamareebjamal - Done the changes, performed the testing

@@ -62,6 +62,7 @@ pyyaml==5.3.1
sendgrid==6.4.7
marshmallow==2.15.2
WeasyPrint==52.1
Flask-Babel=2.0.0
Copy link
Member

Choose a reason for hiding this comment

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

wrong syntax

@iamareebjamal
Copy link
Member

Please add a screenshot of the mail you received and fix the build

@iamareebjamal
Copy link
Member

Disable https://google.github.io/pytype/errors.html#mro-error on app/instance.py", line 243, in get_locale

@iamareebjamal
Copy link
Member

Please also write the command to extract to pot, merge to po, and generating mo files because I'll need that for testing

@codedsun
Copy link
Contributor Author

codedsun commented Nov 8, 2020

Extracting to pot
pybabel extract -F babel.cfg -k _l -o app/translations/messages.pot .

Updating po
pybabel update -i app/translations/messages.pot -d app/translations

Generating mo
pybabel compile -d app/translations

@codedsun codedsun force-pushed the translatable branch 2 times, most recently from d166be8 to 78c06ad Compare November 8, 2020 11:32
@codedsun
Copy link
Contributor Author

codedsun commented Nov 8, 2020

@iamareebjamal - Done!

@iamareebjamal
Copy link
Member

How did you test the translation was happening?

@codedsun
Copy link
Contributor Author

codedsun commented Nov 8, 2020

How did you test the translation was happening?

As you sugggested, In the de folder I changed the msgStr of Ticket purchased string Thank you for your ticket order for to Suneet Srivastava

i changed my browser display language in firefox to German (De), the website was translated to german now.
I logged in and purchased a ticket. Now in the mails table I gave you the exact text of the message here.
#7416 (comment)

@iamareebjamal
Copy link
Member

OK. Thank you

<br/>Quantity: {{ attendee | length }}
{% for i, attendee in attendees | groupby('ticket_id') %}
<br/>{{ _('Ticket') }}: {{ attendee.0.ticket.name }}
<br/>{ _('Quantity') }}: {{ attendee | length }}
Copy link
Member

Choose a reason for hiding this comment

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

{ missing

@iamareebjamal iamareebjamal merged commit 84d4f54 into fossasia:development Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make emails translatable
2 participants