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 #3983 adds Greek language #3984

Merged
merged 17 commits into from
Dec 11, 2018
Merged

Fixes #3983 adds Greek language #3984

merged 17 commits into from
Dec 11, 2018

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Dec 10, 2018

Greek is 100% translated and reviewed.

Status

Ready for review

Description of Changes

Fixes #3983

Changes proposed in this pull request:

Testing

Follow the steps at https://docs.securedrop.org/en/release-0.10.0/development/i18n.html#merging-translations-back-to-develop and we should see files related to el.

Next,

  • Add Greek as a supported language via securedrop-admin sdconfig, the tester should verify site-specific contains the locale for Greek
  • Verify that change can be applied to the server, i.e. securedrop-admin install updates config.py
  • The menu item appears as expected in the source/journalist interfaces
  • Follow steps below for page layout tests

These are new translations files (covered by a generic apparmor inclusion) and updates that can be verified if you speak the language by switching to the corresponding language using the menu. Ensuring there is no breakage because of placeholders incorrectly copied over can be done with:

bin/dev-shell bash
source bin/dev-deps
run_xvfb & run_redis & run_x11vnc & run_sass --watch & urandom & maybe_create_config_py
./i18n_tool.py translate-messages --compile
setting config.py line SUPPORTED_LOCALES = ['el','ar','de_DE','es_ES','fr_FR','nb_NO','nl','pt_BR','zh_Hant','en_US','it_IT','tr', 'hi', 'ru', 'sv']
running
$ PAGE_LAYOUT_LOCALES='el,ar,de_DE,es_ES,fr_FR,nb_NO,nl,pt_BR,zh_Hant,it_IT,tr,hi,ru,sv' pytest -v --page-layout tests/pageslayout
...

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

Can you go back through and double check we're updating everywhere we need to?

We should also update:

  1. The Makefile.
  2. We should run the i10n tool which will update this file which should then be committed.

What about adding these steps to the test plan:

  1. Add Greek as a supported language via securedrop-admin sdconfig, the tester should verify site-specific contains the locale for Greek
  2. Verify that change can be applied to the server, i.e. securedrop-admin install updates config.py
  3. The menu item appears as expected in the source/journalist interfaces

@@ -38,6 +38,7 @@ class I18NTool(object):
'de_DE': {'name': 'German', 'desktop': 'de_DE', },
'es_ES': {'name': 'Spanish', 'desktop': 'es_ES', },
'fr_FR': {'name': 'French', 'desktop': 'fr', },
'el': {'name': 'French', 'desktop': 'el', },
Copy link
Contributor

Choose a reason for hiding this comment

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

the name should be "Greek"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, and eeks :(

@kushaldas
Copy link
Contributor Author

We should run the i10n tool which will update this file which should then be committed.

@redshiftzero do you want that l10n.txt file also to be included in this PR?

Greek is 100% translated and reviewed.
@redshiftzero
Copy link
Contributor

do you want that l10n.txt file also to be included in this PR?

Yes, unless there's a reason not to?

@kushaldas
Copy link
Contributor Author

Yes, unless there's a reason not to?

I was just asking :)

@codecov-io
Copy link

codecov-io commented Dec 10, 2018

Codecov Report

Merging #3984 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3984   +/-   ##
========================================
  Coverage    84.67%   84.67%           
========================================
  Files           43       43           
  Lines         2760     2760           
  Branches       299      299           
========================================
  Hits          2337     2337           
  Misses         355      355           
  Partials        68       68
Impacted Files Coverage Δ
securedrop/securedrop/i18n_tool.py 94.14% <0%> (ø) ⬆️

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 03fe4a1...8b2b87f. Read the comment docs.

@kushaldas
Copy link
Contributor Author

Pushed those two changes.

@kushaldas kushaldas changed the title Fixes #3983 adds greek language Fixes #3983 adds Greek language Dec 10, 2018
@redshiftzero
Copy link
Contributor

The tests here helpfully inform us that we will need to also add the translation directory for Greek for this PR. Let's append the translation updates (not just for Greek but for all supported languages) to this PR when they are 100% translated/reviewed or when it's approaching 10 AM Pacific time, whichever comes first. @kushaldas please run through the test plan at that time to ensure we did not forget anything that needs to be updated.

Then others in the morning (Pacific time) shift should also run through the testing as described in this PR once those translation files are in.

heartsucker
heartsucker previously approved these changes Dec 10, 2018
@heartsucker heartsucker dismissed their stale review December 10, 2018 21:34

dismissing review beuase i was looking at a stale page :/

@redshiftzero
Copy link
Contributor

Just writing this out explicitly as there is some confusion about the purpose of the test plan and who should be testing what (perhaps we can discuss this at the engineering meeting on Thursday): for each PR, at least two people need to verify that changes do what is advertised. The submitter should test their own changes do what is advertised unless there is a compelling reason not to, i.e. "I had to sign off for the day and I did not get a chance to run through full testing here" is a fine reason, just note that on the PR so another person can pick up your work. We have a checkbox in the PR template in order to indicate whether or not this was done.

In addition to the submitter, either the reviewer, or a QA participant should also test that changes do what is advertised (or that automated tests that have been added sufficiently test the change). If we think that the manual test plan is too onerous, we should discuss how to strike the right balance in the PR as part of review. The test plan is part of the PR template to clearly indicate what has been tested, which aids us both in collaboration and for the purpose of retrospectives.

@redshiftzero redshiftzero dismissed their stale review December 11, 2018 01:32

I'm dismissing my review here to unblock team members working prior to the start of the Pacific Time business day.

localizers: Dimitris Maroulidis, boublis, A. Nonymous, pierwill, Adrian, Weblate, Kushal Das, Loic Dachary

https://github.com/freedomofpress/securedrop-i18n
copied from acf2dfe
localizers: Andrey, Kushal Das, Adham Kurbanov

https://github.com/freedomofpress/securedrop-i18n
copied from acf2dfe
localizers: erinm, Kushal Das, Nick Bouwhuis

https://github.com/freedomofpress/securedrop-i18n
copied from acf2dfe
localizers: A. Nonymous, Kushal Das, Kaya Zeren

https://github.com/freedomofpress/securedrop-i18n
copied from acf2dfe
localizers: Jonas Franzén, Kushal Das, Allan Nordhøy

https://github.com/freedomofpress/securedrop-i18n
copied from acf2dfe
localizers: Adolfo Jayme-Barrientos, erinm, Zuhualime Akoochimoya, Kushal Das

https://github.com/freedomofpress/securedrop-i18n
copied from acf2dfe
localizers: Øyvind Bye Skille, Kushal Das, Allan Nordhøy

https://github.com/freedomofpress/securedrop-i18n
copied from acf2dfe
@zenmonkeykstop
Copy link
Contributor

On Tails 3.10.1, checked out add_greek branch and confirmed that:

  • 'el' is listed as a language option during sdconfig
  • if selected during sdconfig, 'el' is added as a supported language to site-specific YAML

@kushaldas
Copy link
Contributor Author

In my local stage test, Greek is showing well, http://lnbnwqtzlo6uzei5.onion/?l=el

@redshiftzero
Copy link
Contributor

So to be clear @kushaldas, this represents the outcome of your test?

  • Add Greek as a supported language via securedrop-admin sdconfig, the tester should verify site-specific contains the locale for Greek
  • Verify that change can be applied to the server, i.e. securedrop-admin install updates config.py (not tested)
  • The menu item appears as expected in the source/journalist interfaces

@kushaldas
Copy link
Contributor Author

So to be clear @kushaldas, this represents the outcome of your test?

Correct.

  • Add Greek as a supported language via securedrop-admin sdconfig, the tester should verify site-specific contains the locale for Greek
  • Verify that change can be applied to the server, i.e. securedrop-admin install updates config.py (not tested)
  • The menu item appears as expected in the source/journalist interfaces

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

✅ I've manually inspected the diff
✅ All page layout tests pass in all languages
✅ Two testers report securedrop-admin sdconfig adding the language to site-specific without issue
✅ One tester reports successfully adding Greek to config.py and the translation appearing on the interfaces without issue

Thanks @kushaldas and @zenmonkeykstop for testing - since we should proceed with the release process, I'm going to approve this so we can backport it into the release branch, but I encourage anyone still testing to finish the manual testing (since this part of the release got relatively little testing, good to have more eyes) in case an issue turns up prior to pre-flight checks.

@redshiftzero redshiftzero merged commit 4240852 into develop Dec 11, 2018
@redshiftzero redshiftzero deleted the add_greek branch December 11, 2018 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants