1105304 - [l10n] Privacy Panel: long string truncated, untranslated elements, strings reused

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Francesco Lodolo [:flod]]

Attachment: Screenshots of l10n issues

1. The same string should not be used for a menu item and the next header, we need 2 different strings. It's definitely the case for location-accuracy and remote-privacy-protection

2. Right now the panel it's a truncation festival. Can we make it better? For example the header should resize to fit, it doesn't.

3. Untranslated elements in the form (might be related to the use of .placeholder), please always test localizability with pseudolocales. App names are not localized, same goes for "User defined".

4. Adjust position: have we thought of non-metric countries?

5. As expected, the list of cities doesn't consider the translated names, resulting in a wrong alphabetical order. I don't see a point in localizing city names like this.

Comment 1

[Francesco Lodolo [:flod]]

I guess it makes more sense for this bug to block bug 1057675.

Comment 2

[Francesco Lodolo [:flod]]

Attachment: Passphrase placeholder

One more non localized string.

Comment 3

[Kevin Grandon :kgrandon]

Very nice bug filing here, the screenshot is very useful. The privacy panel definitely should've landed in a better state than this regarding translations.

Marta - are you able to work on these follow-ups? Who is going to be maintaining the privacy panel code going forward?

Comment 4

[Théo Chevalier]

I found a few more l10n bugs:

6. In the string about-description, please use "…" character instead of "..." and remove the space between data and …

7. Use curly quotes ’ instead of straight quotes ' for all the strings (I can see at least 3 straight quotes in privacypanel.en-US.properties and one in countries.en-US.properties)

8. AFAICT, on step 4 of guided tour (What is Custom Location), the text on the picture can't be localized.

Comment 5

[Théo Chevalier]

9. Also, "Remote Privacy Protection" should be “Remote Privacy Protection” (curly quotation marks)
And “RPP locate YOURPASSPHRASE“ should be “RPP locate YOURPASSPHRASE” (the last quotation mark is the wrong one)

And It‘s easy! should be It’s easy!
Again, wrong curly quote.

Comment 6

[Francesco Lodolo [:flod]]

When a PR lands 560 strings, asking someone from l10n to check would seem a necessary step. I wonder why nobody thought of that.

> 6. In the string about-description, please use "…" character instead of
> "..." and remove the space between data and …

I did report that in bug 1083953 (before realizing that it wasn't the real meta bug), but I think you're right, it makes sense to address all of them here. BTW, those '...' seem completely unnecessary in the context.

Some strings have a comma between subject and verb, afaik that's just wrong, even in English.

Also: we should avoid using so many abbreviations in localization comments, we have the space.

> 8. AFAICT, on step 4 of guided tour (What is Custom Location), the text on
> the picture can't be localized.

(noticed but forgot to report) I don't think there's an easy solution, besides managing to create an image completely without text. There's another image with OK, but I guess that's not as problematic.

Right now my main grudge is with the cities though, to the point that I'm against pushing this changeset out for localization until I get some clarifications.

1) The file contains duplicated keys. If I didn't miss any, they're:
blantyre = Blantyre
creston = Creston
istanbul = Istanbul
nicosia = Nicosia
shiprock = Shiprock

I understand they appear twice because shared between regions, but that doesn't mean we need 2 identical keys in a .properties file.

2) We're asking people to localize 444 city/area names. That's almost 9% of all the strings in Gaia! Maybe we should only have only regions localizable and have city names not localizable (like we do for FTU).

3) The order is wrong: it's alphabetical for English, not for the localized names.

3) A file like this doesn't belong to an app, it should be in /shared where it can be reused if necessary (I'm thinking of FTU).

4) I'm pretty sure there are typos around. I spotted 'Perto Rico', but city names are not really my forte.

Comment 7

[marta]

Hi, 
I am currently stuffed with writing the documentation for the Privacy Panel. Will fix the bugs next week, if you don't mind. But if you find anything more - please continue filing. 
Btw,  Francesco - Thanks for your thorough review! we had an r+ from Gandalf, so we did ask someone from the l10n team to have a look at it:)

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to marta from comment #7)
> Btw,  Francesco - Thanks for your thorough review! we had an r+ from
> Gandalf, so we did ask someone from the l10n team to have a look at it:)

Yup, I am to blame here. I did the tech-l10n review of how they use L10n API but didn't do functional testing against pseudolocale nor did I indicate that it's needed. Apologies for that.

I'll make sure to recognize such review requests as both, tech and functional review next time.

Comment 9

[Théo Chevalier]

Just to bike shed a little bit, and make sure we are all on the same page, I think in the future we'll need 3 types of review for huge landings like this one:
- Tech review (l10n API, etc.)
- Functional review (Shall we localize 450 city names, pseudo-l10n testing, duplicated strings, truncations, all kind of localizability (l12y) issues)
- A copy review from UX or Matej Novak (en-US typography, grammar, consistency…)


Also, (I think it has not been noticed yet), we shouldn't hardcode "Firefox OS" but should use the proper variable instead: {{brandShortName}}.

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Théo Chevalier [:tchevalier] from comment #9)
> - Tech review (l10n API, etc.)
> - Functional review (Shall we localize 450 city names, pseudo-l10n testing,
> duplicated strings, truncations, all kind of localizability (l12y) issues)
> - A copy review from UX or Matej Novak (en-US typography, grammar,
> consistency…)

I absolutely agree. And by looking at the other bug, I honestly thought than galdalf was asked to review only the technical build bits, maybe I was fooled by the bug subject (move X, not implement X). I'm definitely available for the second type of review, while I defer the first type to gandalf and stas unless trivial.

> Also, (I think it has not been noticed yet), we shouldn't hardcode "Firefox
> OS" but should use the proper variable instead: {{brandShortName}}.

Correct

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Francesco Lodolo [:flod] from comment #10)
> I absolutely agree. And by looking at the other bug, I honestly thought than
> gandalf was asked to review only the technical build bits, maybe I was
> fooled by the bug subject (move X, not implement X). 

Oh, yes, I was. But the same way you usually CC me to bugs you're working on wrt. API, I should CC you wrt. QA and l12y :) (or review that myself).

Once again, apologies for that omission. I got this review between four tech reviews and just jumped through code.

Comment 12

[Francesco Lodolo [:flod]]

Any update, especially on the city part?

Comment 13

[marta]

Attachment: Bug1105304.patch

first part of changes. We will work the rest later in December

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8534299 [details] [diff] [review]
Bug1105304.patch

Review of attachment 8534299 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/privacy-panel/js/ala/define_custom_location.js
@@ +146,5 @@
> +      var keys = Object.keys(this.regionsAndCities);
> +
> +      keys.sort(function(a, b) {
> +        return (navigator.mozL10n.get(a) > navigator.mozL10n.get(b));
> +      });

This will not work for anything non-latin.

@@ +221,2 @@
>          option.value = cityName;
> +        option.textContent = city.city;

Moving from declarative l10n to manual textContent is synchronous, and will prevent retranslations.

See: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Using_a_declarative_API

Comment 15

[marta]

Attachment: bug_1105304.patch

Comment 16

[marta]

All of the changes requested in this bug. A few comments:
- We have decided to not translate the countries.
- We are using the same sort mechanism as for the date&time
- There is only one field that is overflowed in the test, but so are some in the settings
- no localization of the texts on the pictures. I believe we cannot do that.
Thanks everyone for detailed review!

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8539125 [details] [diff] [review]
bug_1105304.patch

Review of attachment 8539125 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor nits and the warning about sorting. I really don't know how to help with sorting until we have Ecma 402 enabled in Firefox OS :(

Flod: can you take a look as well? (see? I'm learning!)

::: apps/privacy-panel/index.html
@@ +82,4 @@
>          </li>
>          <li>
>            <a id="menu-item-rpp" class="menu-item pp-link" data-icon="messages" href="#rpp-main">
> +            <span data-l10n-id="remote-privacy-protection">Remote Protection</span>

You can safely remove all strings from HTML. 

According to https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices - we don't need to store en-US strings in HTML files, since they are added during build time.

::: apps/privacy-panel/js/ala/define_custom_location.js
@@ +145,5 @@
> +      // sort regions by their localized name
> +      var keys = Object.keys(this.regionsAndCities);
> +
> +      keys.sort(function(a, b) {
> +        return (navigator.mozL10n.get(a) > navigator.mozL10n.get(b));

If I'm correct, JS comparison operator doesn't handle utf8 strings very well. I don't think it'll work the way it's intended.

::: apps/privacy-panel/locales/privacypanel.en-US.properties
@@ +17,5 @@
>  
>  # ALA-4 (ALA: Exception List)
>  app-list-description = This list shows all the apps, which are allowed to use your location.
> +type-user-defined = User defined
> +type-blur = {{blur}} blur

is that the right variable name? The entity looks cryptic

Comment 18

[Francesco Lodolo [:flod]]

Comment on attachment 8539125 [details] [diff] [review]
bug_1105304.patch

Review of attachment 8539125 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately I can't test the patch. When I try to apply it to Gaia I get a bunch of errors on icons. Is there a branch with these changes or a PR somewhere?

String changes: it's painful, but if you a land a string on master, and you need to change it, you need to assign a new ID to the string. 
Otherwise for people who already localized these strings (or follow each changeset in the history to localize) is extremely hard to understand what's changed.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Having said that, I'm not seeing:
* 2 separate entities for button/headers
* code to resize font in the header (originally introduced in bug 908300)
* fix on curly apostrophes vs straight for English (this kind of changes, as explained in the link above, doesn't require a new string ID)

And I agree with Gandalf, I don't think the comparison will work. We have localeCompare, but it's also Gecko>29
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare

Comment 19

[marta]

Hi,
Thanks guys, looking into it. Two quick comments:
 * we are not resizing the header  - this is a UI no-go. We have fixed the lengths and tested it with pseudo locales and now all fit in.
 * we don't quite get the point about curly vs. straight - on the device it shows exactly as you requested.

Comment 20

[marta]

as for the sort - it is a copy paste from the date&time. Should we go on with implementing something new?

Comment 21

[Francesco Lodolo [:flod]]

(In reply to marta from comment #19)
>  * we are not resizing the header  - this is a UI no-go. We have fixed the
> lengths and tested it with pseudo locales and now all fit in.
Let me clarify: that solution automatically resizes the font *only* if the text doesn't fit, I'm not asking to resize the header for everyone. And testing pseudo locale is not enough, some locales have a much larger expansion for some words.

>  * we don't quite get the point about curly vs. straight - on the device it
> shows exactly as you requested.
Typographical consistency with the rest of the strings in Gaia's repository (think of it as a coding style rule if you prefer).

Comment 22

[marta]

Attachment: bug_1105304.patch

Patch with all requested changes but the resizable header. Still fighting with that one.

Comment 23

[Francesco Lodolo [:flod]]

Comment on attachment 8539259 [details] [diff] [review]
bug_1105304.patch

Review of attachment 8539259 [details] [diff] [review]:
-----------------------------------------------------------------

Pending changed strings that require a new ID, it seems to work a lot better.

Outstanding issues:
* can we do something for the truncations, e.g. display radio buttons and menu items on two lines?
* app names are still untranslated

::: apps/privacy-panel/locales/privacypanel.en-US.properties
@@ +1,4 @@
>  # Main Page
>  privacy-panel = Privacy Panel
>  location-accuracy = Location Accuracy
> +remote-privacy-protection = Remote Protection

Need a new ID

@@ +6,5 @@
>  
>  
>  # ALA-3 (ALA: Adjust Location Accuracy)
> +ala-panel = Location Accuracy
> +use-geolocation = Geolocation

Need a new ID

@@ +7,5 @@
>  
>  # ALA-3 (ALA: Adjust Location Accuracy)
> +ala-panel = Location Accuracy
> +use-geolocation = Geolocation
> +use-location-adjustment = Location adjustment

Need a new ID

@@ +8,5 @@
>  # ALA-3 (ALA: Adjust Location Accuracy)
> +ala-panel = Location Accuracy
> +use-geolocation = Geolocation
> +use-location-adjustment = Location adjustment
> +location-adjustment-information = Remember: Location Adjustment influences the location {{brandShortName}} provides to apps. It will not affect your IP-address or locale settings, so some services might still be able to locate you.

Need a new ID

@@ +48,4 @@
>  
>  
>  # ALA-10 (ALA: Define Custom Location)
> +define-custom-location = Custom location

Need a new ID

@@ +101,2 @@
>  remote-locate = Remote Locate
> +remote-locate-desc = You can locate this device via SMS as following: RP locate YOURPASSPHRASE

Need a new ID (the SMS text has changed). Same for the other 2 strings.

@@ +213,4 @@
>  version = Version
>  build-id = Build-ID
>  about-header = About the Privacy Panel
> +about-description = Together with Deutsche Telecom the Mozilla Foundation developed the Privacy Panel to enable the user to take back control of the personal data…

Do you actually need ellipsis in this sentence?

Comment 24

[Théo Chevalier]

I have a little typo to add, spotted by Goofy while reviewing the French l10n:

"Deutsche Telecom" should be "Deutsche Telekom" :)

Comment 25

[Francesco Lodolo [:flod]]

Just pointing out that we're approaching branching (less than 2 weeks), the code currently on master is not good enough and I don't think we want to start working with approvals and multi branch landings.

Comment 26

[marta]

I will do a pull request today. Should I r? you? I still cannot figure out the resizable header - apparently they use some custom code.

Comment 27

[marta]

I will do a pull request today. Should I r? you? I still cannot figure out the resizable header - apparently they use some custom code. 
As for app names - FxOS does not translate them in places where list of apps is displayed. Should I do it in PP?

Comment 28

[Francesco Lodolo [:flod]]

I can give feedback on the string (see comment 23), not much help for the actual code.

App names are definitely localized in Settings->App permissions or when you switch between apps, not sure if the approach is reusable.

Comment 29

[marta]

Attachment: Localization fixes to PP

Did the changes requested. I cannot figure how to fix the headers and translate the app names. Gandalf can you help with the code?

Comment 30

[Francesco Lodolo [:flod]]

At first sight strings look OK, but with the patch applied Gaia doesn't build.

Exception: L10nError: "privacy-protection" not found in en-US in app://privacy-panel.gaiamobile.org

Comment 31

[marta]

fixed both

Comment 32

[Francesco Lodolo [:flod]]

Comment on attachment 8542510 [details] [review]
Localization fixes to PP

Strings look good to me.

One more nit found when testing: put a space between the number and the unit ("1 km", not "1km"). This is valid both for the slider and exceptions' description when set to User defined ({{blurRadius}}).

One more note on the "Forgot/Change your passphrase?". Now I can type more than 4 digits, but my PIN (6 digits) is still not recognized. If I switch to a 4 digits PIN it works as expected.

Comment 33

[marta]

Fixed the nit. As for 4 digit thing, I checked and it is not our code - something with fxos code?

Comment 34

[Francesco Lodolo [:flod]]

(In reply to marta from comment #33)
> Fixed the nit. As for 4 digit thing, I checked and it is not our code -
> something with fxos code?

That would be strange, considering that I can manage my PIN correctly in settings (enable, disable, change).

Comment 35

[marta]

4 digit should be fixed, please check

Comment 36

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to marta from comment #29)
> Created attachment 8542510 [details] [review]
> Localization fixes to PP
> 
> I cannot figure how to fix the headers and
> translate the app names. Gandalf can you help with the code?

Hi Marta,

I did not work on the header autosizing so I'm not sure how to add it to the Privacy Panel header. - adding :gmarty and :mhenretty. They've been working on the patch in bug 908300.

I also don't understand how we translate app names fully (heh). I've been hacking some bits around it, but it seems that we have a pretty complex system to get App's name. NI on :kgrandon to point you to the right code for localized app names.

I should be able to help you implement the solutions, but I need pointers from those guys on both issues. :)

Comment 37

[Guillaume Marty [:gmarty]]

From what I can see, this app doesn't use gaia-header. We must use it to benefit from the centering/autoresizing/truncating logic.

Comment 38

[Michael Henretty [:mikehenrty][:mhenretty]]

Echoing what Guillaume said, the privacy panel will have to use the gaia-header web component to take advantage of the resize logic. To do so, you will need to include the following registration script:

<script src="shared/elements/gaia-header/dist/gaia-header.js"></script>

...and then use the gaia header element, something like:
<gaia-header>
  <h1>My Header</h1>
  <a id="some-button"></a>
</gaia-header>

Comment 39

[Kevin Grandon :kgrandon]

(In reply to Zibi Braniecki [:gandalf] from comment #36)
> (In reply to marta from comment #29)
> > Created attachment 8542510 [details] [review]
> > Localization fixes to PP
> > 
> > I cannot figure how to fix the headers and
> > translate the app names. Gandalf can you help with the code?
> 
> Hi Marta,
> 
> I did not work on the header autosizing so I'm not sure how to add it to the
> Privacy Panel header. - adding :gmarty and :mhenretty. They've been working
> on the patch in bug 908300.
> 
> I also don't understand how we translate app names fully (heh). I've been
> hacking some bits around it, but it seems that we have a pretty complex
> system to get App's name. NI on :kgrandon to point you to the right code for
> localized app names.

It looks like the settings app is currently using ManifestHelper in the app permissions list: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/manifest_helper.js

For consistency, you may want to use that as well.

Comment 40

[marta]

So I have checked again and there is no localization of the apps in apps permissions. See attachement.

I also cannot reproduce the bug with 6 digit pin - I enter it, enter my new passphrase for the RPP and everything works just fine. Could someone check it as well?

New pull request with gaia headers.

Comment 41

[marta]

Attachment: Changes for review

Comment 42

[Francesco Lodolo [:flod]]

Opening a new PR means losing the comments in the existing one. For example: Gandalf had a good point about localizing distance values.
https://github.com/mozilla-b2g/gaia/pull/27060

Tried the patch, localization is gone because of the header changes in index.html already pointed out by Kevin on Github.

(In reply to marta from comment #40)
> I also cannot reproduce the bug with 6 digit pin - I enter it, enter my new
> passphrase for the RPP and everything works just fine. Could someone check
> it as well?

I've just realized that there are two separate issues here.

With the current code it's not possible to enter more than 4 digits in the PIN request dialog (maxlength="4"). This is fixed in this PR (maybe you should dupe bug 1105328 to this one at this point). But:

1. I have a 6 digits PIN. Go into settings, enable the PIN, enter my 6 digits, PIN is enabled (just checking if it's correct)
2. Disable the PIN, since I don't usually keep it enabled.
3. Go into Privacy Panel, try to change the passphrase, I'm asked to enter the PIN. PIN is always wrong.

The only scenario in which this works is if I skip #2. With the SIM PIN enabled #3 works.

Comment 43

[Francesco Lodolo [:flod]]

Attachment: App permissions

(In reply to marta from comment #40)
> So I have checked again and there is no localization of the apps in apps
> permissions. See attachement.

I don't see an attachment, but as you can see from mine they're localized.
I guess that Privacy Panel should display an icon there too.

Comment 44

[marta]

Hi so latest updated request has localization.
As for the PIN - this is a feature - you are not supposed to be able to change your password if your SIM is empty - then you should use your screen passlock as verification method.

Could we integrate the current pull request? I will not change the bug, but this PR fixes a bunch of other more important issues...

Comment 45

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8546902 [details] [review]
Changes for review

I'm sorry, but this code introduces API regressions and still doesn't fix the non-localizable sorting.

In particular, the way you introduce a bunch of mozL10n.get and build an API that works against: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs for unit localization is an r- from me.

Comment 46

[marta]

ok thanks, will take care of that tomorrow

Comment 47

[Kevin Grandon :kgrandon]

Comment on attachment 8546902 [details] [review]
Changes for review

Feel free to re-flag once Zibi's comments are addressed.

Comment 48

[Francesco Lodolo [:flod]]

(In reply to marta from comment #44)
> As for the PIN - this is a feature - you are not supposed to be able to
> change your password if your SIM is empty - then you should use your screen
> passlock as verification method.

I'll file a different bug, since there are a lot of issues in that part of the feature and this bug has already way too much stuff in it.

Comment 49

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #48)
> I'll file a different bug, since there are a lot of issues in that part of
> the feature and this bug has already way too much stuff in it.

For reference: filed bug 1120164, which will probably need more strings for a proper fix.

Comment 50

[Francesco Lodolo [:flod]]

Multi-locale build is broken, the text remains in English because of the header changes in index.html

Checked application.zip and it has only en-US.json in it. In /build_stage/locales I see *.it.properties files, but not it.json in /build_stage/locales-obj

Comment 51

[marta]

Fixed the header problem (I hope) and other issues. Kevin, waiting for review

Comment 52

[Stefan Plewako [:stef]]

Patch from attachment 8546902 [details] [review] duplicates custom-location id in apps/privacy-panel/locales/privacypanel.en-US.properties:
>  # ALA-6 (ALA: Custom Location)
>  custom-location = Custom Location

>  # ALA-10 (ALA: Define Custom Location)
> -define-custom-location = Define custom location
> +custom-location = Custom location

Comment 53

[marta]

corrected

Comment 54

[Francesco Lodolo [:flod]]

Tested the current version of the patch, I don't see particular l10n issues, but I can't manage exceptions anymore, so I'm not sure if the App names issue is localization (they're correctly localized in the new Transparency panel).

Comment 55

[Kevin Grandon :kgrandon]

Comment on attachment 8546902 [details] [review]
Changes for review

Zibi - do you want to take another pass at this now that most of your comments have been addressed?

Comment 56

[Kevin Grandon :kgrandon]

Comment on attachment 8546902 [details] [review]
Changes for review

Looking good, but I have to leave an R- due to test failures - it seems some of the unit tests are failing from the last run. I've also closed and re-opened the pull request to re-trigger a newer gaia try as it seems we missed a run. I'll continue to take a look and leave comments on the PR if I see anything. Please re-flag me after the tests are fixed.

Comment 57

[marta]

fixed tests

Comment 58

[Kevin Grandon :kgrandon]

Comment on attachment 8546902 [details] [review]
Changes for review

Looking better, but we're still seeing a failure. It looks like there is a file, rpp_features, that was not moved into the correct location. Please address and re-flag me. Also left a comment on github and will keep looking for things.

Comment 59

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8546902 [details] [review]
Changes for review

yeah, the L10n API use is good

Comment 60

[marta]

Kevin, fixed last elements....

Comment 61

[Kevin Grandon :kgrandon]

Comment on attachment 8546902 [details] [review]
Changes for review

I think we are close, but I left a comment about a key that I think needs to be updated, and I think we're also missing some images in the PR. Please address my comments and re-flag me.

Comment 62

[Martin Shuman [:Marty]]

Adding privacy keyword.

Comment 63

[marta]

reflagging, tests are passing:)

Comment 64

[Kevin Grandon :kgrandon]

Comment on attachment 8546902 [details] [review]
Changes for review

I'm sure there's some things in here that we missed, but this is getting unweildly and long lived. Let's get this landed and iterate on it further from there. Nice work.

Comment 65

[Francesco Lodolo [:flod]]

Please DO NOT land this patch in its current state. What exactly is this?

sim-invalid-txt = Wrong SIM PIN! {{ value }} trie{{ plural }} left!

And there's nothing in the code that manages any of the variables.

Plural forms:
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

One more note: changing the patch significantly after it's been checked it's not the right way to go. This specific string was radically modified way after l10n reviewed the patch, I noticed it only because Kevin picked up the unchanged string ID in the last review.

Comment 66

[marta]

revised the string to the original form, it's mostly for another patch, so I will land it separately. Sorry for the inconvenience.

Comment 67

[Francesco Lodolo [:flod]]

Not sure if we're waiting for tests to pass (last run doesn't look good) or something else at this point.

Comment 68

[marta]

The try-run looks good. The last one did not start properly, that's why it didnt look good. https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=9078be4e8766
R? Kevin, hope we can land it today so that other bugs are not blocked

Comment 69

[marta]

Oh, Kevin gave r+ already. ni? him to land it.

Comment 71

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/ff8273baba92aa59b811c29b48b0ec61ac9f975f

Comment 72

[Delphine Lebédel [:delphine - Use needinfo]]

Hi Marta, can you please request approval on this patch for 2.2?
We would really appreciate that this gets in asap. Thanks!

Comment 73

[marta]

How do I do it?

Comment 74

[Delphine Lebédel [:delphine - Use needinfo]]

Go to Attachments, click on Detail next to the patch that needs to be uplifted to 2.2. Then choose the approval gaia 2.2 flag from the list. Put :bajaj as approver (1st name that comes to mind in order to do this ;) ) and fill in the Approval Request Comment section with the needed information. thanks

Comment 75

[Delphine Lebédel [:delphine - Use needinfo]]

Marta: this might be helpful for future reference https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_6

Comment 76

[Francesco Lodolo [:flod]]

Comment on attachment 8546902 [details] [review]
Changes for review

(since nobody else is asking)

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: Privacy Panel on 2.2 includes more than 400 unnecessary strings and localization issues fixed in this patch
[Testing completed]: on master
[Risk to taking this patch] (and alternatives if risky): poor localizability
[String changes made]: 486 strings removed, 50 added

Comment 77

[marta]

Thank for doing it, flod!

Comment 78

[Francesco Lodolo [:flod]]

Setting checkin-needed for 2.2, not aware of a different flag for branch checkin.

Comment 79

[Kevin Grandon :kgrandon]

The uplift queries should already pick this up based on the approval and blocking status. You don't need to add checkin-needed here.

Comment 80

[Kevin Grandon :kgrandon]

I can go ahead and uplift this though.

In v2.2: https://github.com/mozilla-b2g/gaia/commit/7c8dfd36336fb87848b5668dbaa3bc78ac758a71