1003384 - [B2G][1.4][SMS] - The phone type "Other" is not translated in any of the languages when suggested contacts are shown in SMS

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: Other.png

Description:
The phone type "Other" is not translated in any of the languages when a suggested contact is shown beneath the To: field of an SMS. 

Repro Steps:
1) Updated Buri to Build ID: 20140428000206
2) Change the phone's language to Swedish.
3) Create a contact that has a phone number listed as "Other".
3) Tap on "SMS". 
4) Tap on the "Compose New Message" icon.
5) Tap on the "To:" field and type a letter that matches the contact created in step 3. 
6) Observe the phone type "Other" string. 

Actual:
The phone type "Other" is not translated on all languages in SMS.

Expected:
The string "Other" is translated properly on all languages. 

Environmental Variables
Device: Buri 1.4.0 Mozilla RIL
Build ID: 20140428000206
Gecko: https://hg.mozilla.org/releases/mozilla-aurora/rev/36f67ce46855
Gaia: d23e479e8a4ce0bc620acb2d7e2f82801aa4d0ea
Platform Version: 30.0a2
Firmware Version: v1.2-device.cfg

Notes:
Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/12129/
See attached: screenshot

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue does not occur on the Buri 1.3 Moz RIL 

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140422024003
Gaia: cab7de82832d00696807ae48ec277bd642b82a0a
Gecko: c35695cc384c
Version: 28.0
Firmware Version: v1.2-device.cfg

The phone type is called "Another" in v1.3 but it is translated correctly on all languages.

Comment 2

[Julien Wajsberg [:julienw]]

It's strange, I thought we never translated any phone type until bug 931119 was fixed. But maybe we translated it in other places...

> The phone type "Other" is not translated on all languages in SMS.
Do you mean it's translates in some languages, or is it never translated?

Comment 3

[Preeti Raghunath(:Preeti)]

1.4+ for untranslated strings

Comment 4

[Julien Wajsberg [:julienw]]

NI reporter for comment 2.

Comment 5

[KTucker [:KTucker][Inactive 3/4/2016]]

I just went through all of the available languages on 1.4 and the phone type "Other" always appears in English in the SMS app. In the contacts app the word is translated. 

On 1.3, the word "Another" is used instead of "Other". "Another" is translated for multiple languages in the SMS app.

Comment 6

[Jason Smith [:jsmith]]

David - Can you find an assignee for this?

Comment 7

[David Scravaglieri [:scravag]]

Oleg, could you take a look at this please ?

Comment 8

[Oleg Zasypkin [:azasypkin]]

(In reply to David Scravaglieri [:scravag] from comment #7)
> Oleg, could you take a look at this please ?

Ok, will take care of it!

Comment 9

[Oleg Zasypkin [:azasypkin]]

Ok, looks like the issue was caused by patch for bug 925566 and fix is pretty trivial.

Marina, as you was assigned to the bug 925566, can you please let us know whether phone type 'another' is still can be chosen by user via contact creation UI? I see that you've removed it from code but left in the locale file. We also have 'another' label in locale file in SMS app and I'm wondering whether we can remove it.

Julien, this patch will involve updating locale file in 1.4 branch, do I need to update English version only as usual, or there is some other workflow in case of v1.3/v1.4 uplifting?

Thanks!

Comment 10

[Marina Rodríguez [:mai]]

Hi, 

It's not possible automaticaly choose the phone type 'another', although the user can putting a custom tag with this label. I didn't remove the label from the locale file to provide backward compatibility, for the old contacts. 

Julien, make any sense keeping these labels yet?

Regards

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to marina rodríguez [:mai] from comment #10)
> Hi, 
> 
> It's not possible automaticaly choose the phone type 'another', although the
> user can putting a custom tag with this label. I didn't remove the label
> from the locale file to provide backward compatibility, for the old
> contacts. 
> 
> Julien, make any sense keeping these labels yet?

Yes, I think we should keep the label, because as you said, some users might still have "another".


(In reply to Oleg Zasypkin [:azasypkin] from comment #9)
> Ok, looks like the issue was caused by patch for bug 925566 and fix is
> pretty trivial.
> 
> Marina, as you was assigned to the bug 925566, can you please let us know
> whether phone type 'another' is still can be chosen by user via contact
> creation UI? I see that you've removed it from code but left in the locale
> file. We also have 'another' label in locale file in SMS app and I'm
> wondering whether we can remove it.

Note that all locales keep all keys for all versions anyway (see [1]), so this is not really a concern.

[1] http://hg.mozilla.org/gaia-l10n/en-US/file/0adbdc3958c5/apps/sms/sms.properties

> 
> Julien, this patch will involve updating locale file in 1.4 branch, do I
> need to update English version only as usual, or there is some other
> workflow in case of v1.3/v1.4 uplifting?
> 

Same workflow, we should update only the English. But Francesco (:flod) on IRC told me it's too late to change strings in 1.4 anyway... So I'm renominating here.


What disturbs me here is that in bug 897942 we already added "Other", probably because that's what Contacts used back at the time. I think we should share the translation file for these labels. Do you think it's possible ?

Comment 12

[Jason Smith [:jsmith]]

This is too user facing to be punted from the release, so this stays as a blocker.

Can we remove the string being displayed to the user here instead to avoid adding a string?

Comment 13

[Julien Wajsberg [:julienw]]

Yeah, we can try to reuse the existing string we have for "Other" or "another" (yeah, I know). This won't make a nice code but for a branch patch it should work just good enough.

Comment 14

[Axel Hecht [:Pike]]

Let's add back the late-l10n keyword until we actually have a patch that doesn't have l10n impact.

late-l10n isn't about "good to add strings", it's "oh my gosh, this probably has string impact, all scream and shout and run in circles"

There's a few things here:

We can't us "Another", as at least German translated that as "Weitere", which is more like "further items", that's really different.

https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/sms/locales/sms.en-US.properties#L180 makes me think that we do have "Other" as key, but not "other". Which is what I see in the screenshot, btw.

We might be good to just map one key to another, pun intended.

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Axel Hecht [:Pike] from comment #14)
> Let's add back the late-l10n keyword until we actually have a patch that
> doesn't have l10n impact.
> 
> late-l10n isn't about "good to add strings", it's "oh my gosh, this probably
> has string impact, all scream and shout and run in circles"
> 
> There's a few things here:
> 
> We can't us "Another", as at least German translated that as "Weitere",
> which is more like "further items", that's really different.

I could argue it was a bad translation, because "another" was really used in the same case !

> 
> https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/sms/locales/sms.en-US.
> properties#L180 makes me think that we do have "Other" as key, but not
> "other". Which is what I see in the screenshot, btw.
> 
> We might be good to just map one key to another, pun intended.

Yep, let's do this.

Comment 16

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #11)
> (In reply to marina rodríguez [:mai] from comment #10)
> What disturbs me here is that in bug 897942 we already added "Other",
> probably because that's what Contacts used back at the time. I think we
> should share the translation file for these labels. Do you think it's
> possible ?

I thought that if even Contacts app doesn't have 'Other' l10n id in their locale files (at least I didn't find anything like that in code\localization in master) we shouldn't bother either. Or do you think we should keep both 'Other' and 'other' labels for the master branch?

Comment 17

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Patch for master branch

Comment 18

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (v1.4)

Patch for v1.4 branch

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #16)
> (In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #11)
> > (In reply to marina rodríguez [:mai] from comment #10)
> > What disturbs me here is that in bug 897942 we already added "Other",
> > probably because that's what Contacts used back at the time. I think we
> > should share the translation file for these labels. Do you think it's
> > possible ?
> 
> I thought that if even Contacts app doesn't have 'Other' l10n id in their
> locale files (at least I didn't find anything like that in code\localization
> in master) we shouldn't bother either. Or do you think we should keep both
> 'Other' and 'other' labels for the master branch?

Actually I couldn't find a reason why Steve added "Other" in Bug 897942. Maybe it would be a good idea to wait for his return if you want to remove it in the master patch.

One possible reason is that some phone numbers from old versions had "Other" instead of "other" (because the Contacts app would set the translation for Other instead of the tag for unknown tags), and so for backward compatibility we had to add it.

Comment 20

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #19)
> 
> Actually I couldn't find a reason why Steve added "Other" in Bug 897942.
> Maybe it would be a good idea to wait for his return if you want to remove
> it in the master patch.

> One possible reason is that some phone numbers from old versions had "Other"
> instead of "other" (because the Contacts app would set the translation for
> Other instead of the tag for unknown tags), and so for backward
> compatibility we had to add it.

Ok, let's rather wait for Steve for master branch patch. Don't want to leave legacy stuff without a reason :)

Comment 21

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8418404 [details] [review]
GitHub pull request URL (v1.4)

Added JS-only workaround that maps 'other' l10n id to 'Other' one. Or did you mean another more elegant solution to map l10n ids that I don't know about?

Skimmed through phone.type usages in SMS code base, looks like it's always used as l10n id, so looks more or less safe to replace 'other' with 'Other' :)

Comment 22

[Julien Wajsberg [:julienw]]

So, there is at least one place where it seems that we don't translate the type at all; it's in getCarrierTag (used in ThreadUI.updateCarrier). Could be a good idea to translate it too while we're at it?

Also, in the l10n properties files, we can use "{{ XXX }}" to use other texts in this translation, but then we wouldn't be able to change it on master, so I'd rather use the JS solution in v1.4 only.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8418404 [details] [review]
GitHub pull request URL (v1.4)

Removing the r request until the previous question is answered. If you think it's not necessary to do it in v1.4 (or just that I'm plain wrong), just raise the review request again, and we'll be fine.

Still I think we should do it in v2.

Comment 24

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #22)
> So, there is at least one place where it seems that we don't translate the
> type at all; it's in getCarrierTag (used in ThreadUI.updateCarrier). Could
> be a good idea to translate it too while we're at it?

Agree that it should be fixed, but I'd rather avoid fixing this in current patch. At the first look it requires more changes than just translate phone type: we should change how getCarrierTag works as currently it returns string that should be _partially_ localized + we should support automatic localized label update on language change. Regarding to the latter I see another bug with career label:
 
1. Open thread where career header is shown (eg. "mobile | carrier_name");
2. Press Home button;
3. Go to the language settings and change the language;
4. Switch back to the SMS app;

Expected result: you should see "localized_phone_type | carrier_name" or at least the same label as on step #1.

Actual result: "localized_label_for_carrier_unknown" is shown.

IMO it's a good candidate for a separate carrier label specific bug, probably a v1.4 blocker. What do you think?

> Also, in the l10n properties files, we can use "{{ XXX }}" to use other
> texts in this translation, but then we wouldn't be able to change it on
> master, so I'd rather use the JS solution in v1.4 only.

Yes, JS workaround is definitely for v1.4 only, for master patch I'm planning to change 'Other'->'other' or just add 'other' in addition to 'Other' depending on Steve input.

Comment 25

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #16)
> > (In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #11)
> > > (In reply to marina rodríguez [:mai] from comment #10)
> > > What disturbs me here is that in bug 897942 we already added "Other",
> > > probably because that's what Contacts used back at the time. I think we
> > > should share the translation file for these labels. Do you think it's
> > > possible ?
> > 
> > I thought that if even Contacts app doesn't have 'Other' l10n id in their
> > locale files (at least I didn't find anything like that in code\localization
> > in master) we shouldn't bother either. Or do you think we should keep both
> > 'Other' and 'other' labels for the master branch?
> 
> Actually I couldn't find a reason why Steve added "Other" in Bug 897942.
> Maybe it would be a good idea to wait for his return if you want to remove
> it in the master patch.
> 
> One possible reason is that some phone numbers from old versions had "Other"
> instead of "other" (because the Contacts app would set the translation for
> Other instead of the tag for unknown tags), and so for backward
> compatibility we had to add it.

The 'Other' should be used in the past synced from contacts app, but since the contact code base already changed(lowercase 'other' id is added after that), the only thing we could confirm is 'other' is used in current contacts app[1]. Maybe we should just change the id because there seems not possible to return a tag with 'Other' type for now.

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L213

Comment 26

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck](PTO until 5/12) from comment #25)
> (In reply to Julien Wajsberg [:julienw] from comment #19)
> > (In reply to Oleg Zasypkin [:azasypkin] from comment #16)
> > > (In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #11)
> > > > (In reply to marina rodríguez [:mai] from comment #10)
> > > > What disturbs me here is that in bug 897942 we already added "Other",
> > > > probably because that's what Contacts used back at the time. I think we
> > > > should share the translation file for these labels. Do you think it's
> > > > possible ?
> > > 
> > > I thought that if even Contacts app doesn't have 'Other' l10n id in their
> > > locale files (at least I didn't find anything like that in code\localization
> > > in master) we shouldn't bother either. Or do you think we should keep both
> > > 'Other' and 'other' labels for the master branch?
> > 
> > Actually I couldn't find a reason why Steve added "Other" in Bug 897942.
> > Maybe it would be a good idea to wait for his return if you want to remove
> > it in the master patch.
> > 
> > One possible reason is that some phone numbers from old versions had "Other"
> > instead of "other" (because the Contacts app would set the translation for
> > Other instead of the tag for unknown tags), and so for backward
> > compatibility we had to add it.
> 
> The 'Other' should be used in the past synced from contacts app, but since
> the contact code base already changed(lowercase 'other' id is added after
> that), the only thing we could confirm is 'other' is used in current
> contacts app[1]. Maybe we should just change the id because there seems not
> possible to return a tag with 'Other' type for now.
> 
> [1]:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> js/contacts.js#L213

Agree, thanks!

Comment 27

[Steve Chung [:steveck]]

Comment on attachment 8418401 [details] [review]
GitHub pull request URL

For the master, changing the id should be enough since you might create another bug for carrier tag fixing, thanks!

Comment 28

[Julien Wajsberg [:julienw]]

Comment on attachment 8418404 [details] [review]
GitHub pull request URL (v1.4)

r=me, thanks !

Comment 29

[Julien Wajsberg [:julienw]]

master: ae15cfb697cc9127f9b909a5ba911186157f617c
v1.4: 15da4fde475db99626c8a47964b12715775cd5c1

(I put 'r=julien' in both because I landed the "master" patch thinking it was the v1.4 patch, sorry Steve)

Comment 30

[bmorgan]

This issue has been fixed on the Buri 1.4 MOZ RIL

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140508000201
Gaia: 4ce973ef0732b0d52cb043210db598aa176b2ce9
Gecko: 16ab7f6b18f8
Version: 30.0
Firmware Version: v1.2-device.cfg