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

Fixes: https://github.com/gitcoinco/web/issues/642 #728

Merged
merged 7 commits into from
Apr 4, 2018

Conversation

bakaoh
Copy link
Contributor

@bakaoh bakaoh commented Mar 28, 2018

Description

Enable Internationalization in the app

  • Migrate each of the HTML user-facing pieces of copy to {% blocktrans %} or {% trans %} fields
  • Migrate each of the javascript user-facing pieces of copy to ngettext fields
  • Migrate each of the views.py user-facing pieces of copy to gettext_lazy fields
  • Migrate each of the model fields and help_text
  • Repeat the above, for each email in marketing/mails.py
Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

No

Testing

Just add some simple code to enable internationalization. Nothing involves logic or business code.

Refers/Fixes

Fixes: #642

@codecov
Copy link

codecov bot commented Mar 28, 2018

Codecov Report

Merging #728 into master will increase coverage by 0.17%.
The diff coverage is 29.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #728      +/-   ##
==========================================
+ Coverage   33.79%   33.96%   +0.17%     
==========================================
  Files         101      101              
  Lines        5750     5765      +15     
  Branches      671      671              
==========================================
+ Hits         1943     1958      +15     
  Misses       3728     3728              
  Partials       79       79
Impacted Files Coverage Δ
app/app/urls.py 93.33% <100%> (+0.22%) ⬆️
app/external_bounties/models.py 55.17% <100%> (+0.78%) ⬆️
app/dashboard/models.py 69.41% <100%> (+0.12%) ⬆️
app/tdi/views.py 19.07% <13.33%> (+1.07%) ⬆️
app/external_bounties/views.py 18.62% <14.28%> (+0.8%) ⬆️
app/dashboard/views.py 17.64% <20%> (+0.21%) ⬆️
app/retail/views.py 42.01% <28.57%> (+0.99%) ⬆️
app/marketing/views.py 10.83% <33.33%> (+0.37%) ⬆️
app/faucet/views.py 26.31% <50%> (+0.98%) ⬆️
app/gitcoinbot/views.py 37.03% <50%> (+2.42%) ⬆️
... and 1 more

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 9d6c2e5...3f18d2b. Read the comment docs.

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

Phew!
@bakaoh Thank you for the contribution! Overall, the changes lgtm. I'm going to deploy this to staging at: https://stage.gitcoin.co following some local testing.

I'll ping back and approve the PR following QA.

Thanks again!

@@ -29,6 +29,7 @@
from django.db.models.signals import m2m_changed, post_delete, post_save, pre_save
from django.dispatch import receiver
from django.utils import timezone
from django.utils.translation import gettext_lazy as _
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

@owocki
Copy link
Contributor

owocki commented Mar 28, 2018

dat plus minus! strong effort @bakaoh !

@bakaoh could you talk a little bit about how you traversed the codebase to get this done? how can we be assured that you didnt miss anything from the initial scope?

looks like this still needs to be done:

each email in marketing/mails.py

(the subject lines need to be done.. looks like the templates are done)

@mbeacom we should put this on the fast track to merge so that we dont end up with endless merge conflicts!

@mbeacom
Copy link
Contributor

mbeacom commented Mar 28, 2018

@owocki Agreed! 💪 Merge conflicts will get out of control on this PR if we don't get it in soon. 😂 @bakaoh Would you mind resolving the conflicts here and addressing the marketing/mail.py subject lines? I'll pull this down and run through to see if we're missing anything else now.

@bakaoh
Copy link
Contributor Author

bakaoh commented Mar 29, 2018

@mbeacom i resolved the conflicts and commit the marketing/mail.py

@bakaoh could you talk a little bit about how you traversed the codebase to get this done? how can we be assured that you didnt miss anything from the initial scope?

i'd just gone through each file line by line, it took more time than expected but it gave me a change to know more about the codebase, i even found a minor bug at file dashboard/templates/bounty_details.html line 207 :))

@mbeacom
Copy link
Contributor

mbeacom commented Mar 29, 2018

@bakaoh Nice catch! Resolved in 28a39a9

Thanks for resolving the conflicts.
I'm going to comb over this branch a little more, test, and ping back here.

@owocki
Copy link
Contributor

owocki commented Mar 29, 2018

very excited to ship this soon. this unlocks translation which could be really huge for gitcoin in the future

@owocki
Copy link
Contributor

owocki commented Apr 4, 2018

@mbeacom any update here?

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

lgtm - Conflicts resolved.

@mbeacom mbeacom added frontend This needs frontend expertise. backend This needs backend expertise. translation labels Apr 4, 2018
@owocki
Copy link
Contributor

owocki commented Apr 4, 2018

just tagged the current master with pre-728

@mbeacom mbeacom merged commit fa43d1a into gitcoinco:master Apr 4, 2018
@gitcoinco gitcoinco deleted a comment from darkdarkdragon Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend This needs backend expertise. frontend This needs frontend expertise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants