1141479 - Fix new strings for default activities (followup bug 1073495)

Status: RESOLVED FIXED

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

Description

[Fernando Campo (:fcampo)]

+++ This bug was initially created as a clone of Bug #1073495 +++

Per bug 1073495 comment 26 and comment 27
We have unused , duplicated and wrong string IDs

Original list on  bug 1039386 attachment 8522777 [details]

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] fcampo:fix-strings-1141479 > mozilla-b2g:master

Comment 2

[Fernando Campo (:fcampo)]

Attachment: PR - https://github.com/mozilla-b2g/gaia/pull/28748

Comment 3

[Francesco Lodolo [:flod]]

I commented in the PR, but I'm missing a lot of context to understand these strings

> default-activity-newcontact = Open contact
> default-activity-updatecontact = Open contact

"Create/update contact" based on key name?

> default-activity-viewurl-email = Send email
If the activity is "view", should it be "Open link" or similar?

Comment 4

[Fernando Campo (:fcampo)]

(In reply to Francesco Lodolo [:flod] from comment #3)
> I commented in the PR, but I'm missing a lot of context to understand these
> strings
> 
> > default-activity-newcontact = Open contact
> > default-activity-updatecontact = Open contact
> 
> "Create/update contact" based on key name?
> 
> > default-activity-viewurl-email = Send email
> If the activity is "view", should it be "Open link" or similar?
Sorry I didn't explain this properly before asking for a review, I'll try now:

As you can see in the specs [https://bug1039386.bugzilla.mozilla.org/attachment.cgi?id=8522777 page 10], we have a list of Actions with different filters. Even if those actions are different for the OS (new contact, open contact, update contact), for this feature they are grouped together, as they will be opened with the same the moment any of them are associated with a default app.

So one proposal would be the one on the patch: using different L10nIds for each of the possible actions, but sharing the string. Not sure if that is correct for the translation team
A tweak for this would be having a master key for the ones shared, maybe it's better understandable like this:
open-contact = Open contact
default-activity-newcontact     = {{open-contact}}
default-activity-opencontact    = {{open-contact}}
default-activity-updatecontact  = {{open-contact}}


The second proposal would be to have less L10nIds than total actions, and re-use the IDs directly from the JS files
{
  name: 'update',
  type: ['webcontacts/contact'],
  l10nId: 'default-activity-opencontact',
  settingsId: 'activity.default.opencontact'
},
{
  name: 'open',
  type: ['webcontacts/contact'],
  l10nId: 'default-activity-opencontact',
  settingsId: 'activity.default.opencontact'
},
{
  name: 'new',
  type: ['webcontacts/contact'],
  l10nId: 'default-activity-opencontact',
  settingsId: 'activity.default.opencontact'
}

Hope I made it more clear now

Comment 5

[Francesco Lodolo [:flod]]

Thanks, a lot clearer with the specs.

I think the second approach is better: have one key for "Open contact", and use it everywhere is necessary.

I would also a localization note to the file, e.g.

# These strings are used to describe actions available to users, and which
# applications are associated as default handlers to these actions.
# For example: "Open link" -> Firefox.

Comment 6

[Stefan Plewako [:stef]]

A,B & C comments from last page of https://bug1039386.bugzilla.mozilla.org/attachment.cgi?id=8522777 should really be included as comment in activities.properties

Comment 7

[Fernando Campo (:fcampo)]

Strings updated for the second proposal and comments added, can you take a new look Francesco?

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8575228 [details] [review]
PR - https://github.com/mozilla-b2g/gaia/pull/28748

I'm still not sure I should review patches on Gaia. A

nyhow, the strings look good, I have some doubts on the localization comment, especially the second part: it's a bit confusing as it is, and I don't think it's really relevant in the localization file.

Comment 9

[Fernando Campo (:fcampo)]

(In reply to Francesco Lodolo [:flod] from comment #8)
> Comment on attachment 8575228 [details] [review]
> PR - https://github.com/mozilla-b2g/gaia/pull/28748
> 
> I'm still not sure I should review patches on Gaia. A
This patch has no changes in code, just strings, so I'd say we're safe with a review from l10n (tests passing).

> nyhow, the strings look good, I have some doubts on the localization
> comment, especially the second part: it's a bit confusing as it is, and I
> don't think it's really relevant in the localization file.
everything updated, code comments moved to the code and language nit added.

Comment 10

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 11

[Fernando Campo (:fcampo)]

(In reply to Autolander from comment #10)
> Autolander could not locate a review from a user within the suggested
> reviewer list. Either the patch author or the reviewer should be in the
> suggested reviewer list.
well, this answers our concerns :D

Comment 12

[Fernando Campo (:fcampo)]

Comment on attachment 8575227 [details] [review]
[gaia] fcampo:fix-strings-1141479 > mozilla-b2g:master

Autolander made me do it :p

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8575227 [details] [review]
[gaia] fcampo:fix-strings-1141479 > mozilla-b2g:master

:)

Comment 14

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 15

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/8d76df71f1cdd411724e23e755a6e21d9a35496f

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/a6b2d61cfda66faf4d00af194da2a05961370988 for Gu test failures on b2g-inbound:

https://treeherder.mozilla.org/logviewer.html#?job_id=1521972&repo=b2g-inbound

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] fcampo:fix-strings-1141479 > mozilla-b2g:master

Comment 18

[Fernando Campo (:fcampo)]

(In reply to Wes Kocher (:KWierso) from comment #16)
> I had to revert this in
> https://github.com/mozilla-b2g/gaia/commit/
> a6b2d61cfda66faf4d00af194da2a05961370988 for Gu test failures on b2g-inbound:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=1521972&repo=b2g-
> inbound
Thanks for reverting it, I had the impression that autolander only merged PRs when tests were green, but I'll be more careful in the future.

Tests are passing now [https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=9c9b386cc2f6], so I'll just carry the r+ from Francesco and Etienne and merge this manually.

master: https://github.com/mozilla-b2g/gaia/commit/491314497f2d220a78aa2818d0455fb395049d81