Closed Bug 965854 Opened 7 years ago Closed 7 years ago

Display SIM indication in voicemail notification

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S6 (25apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: rik, Assigned: drs)

References

Details

Attachments

(1 file, 3 obsolete files)

They should now read "SIM 1 - New voicemail"
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 975918
blocking-b2g: 1.4? → ---
Blocks: b2g-dsds-1.4
Status: RESOLVED → REOPENED
blocking-b2g: --- → 1.4?
Resolution: DUPLICATE → ---
Blocks DSDS and hence moving to 1.4+
blocking-b2g: 1.4? → 1.4+
set target milestone to 1.4 S4 for now
Target Milestone: 1.4 S2 (28feb) → 1.4 S4 (28mar)
Keywords: late-l10n
Rik, will you be working on this in 1.4 S5? if so, can you move this to 1.4 S5? Thanks
Flags: needinfo?(anthony)
This was already working actually, but it wasn't being localized and was in an incorrect format. I did a small refactor to make testing this easier, too.
Assignee: anthony → drs+bugzilla
Attachment #8401475 - Flags: review?(anthony)
Flags: needinfo?(anthony)
This seems to use existing strings, but possibly in a new context.

If this is the right way to do this, let's drop the late-l10n keyword.

It'd be good to get some screenshots of existing use of the strings, and the new use, so that we're able to tell if this might be exposing new problems.
Keywords: late-l10n
Whiteboard: exposing existing strings
(In reply to Axel Hecht [:Pike] from comment #6)
> This seems to use existing strings, but possibly in a new context.
> 
> If this is the right way to do this, let's drop the late-l10n keyword.
> 
> It'd be good to get some screenshots of existing use of the strings, and the
> new use, so that we're able to tell if this might be exposing new problems.

Yes, it's using existing strings in a new context. Suffice it to say, the strings were previously used mostly in the DSDS SIM picker, which is an action menu presented to the user whenever they long press on a button that triggers an action which could take place on multiple SIMs.
Comment on attachment 8401475 [details] [diff] [review]
Display SIM indication in voicemail notification.

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

Sorry for the slow review.

::: apps/system/js/voicemail.js
@@ +79,5 @@
>      }
>  
> +    if (SIMSlotManager.isMultiSIM()) {
> +      var _ = window.navigator.mozL10n.get;
> +      title = _('sim-picker-button', { n: simIndex }) + ' - ' + title;

This is not the format we want. We want "(SIM n) Bla bla voicemail".
I've also been told that concatenating strings is generally a bad idea when it comes to L10n. Maybe it's ok in that case, I don't know.
Depending on Pike's recommendation, you may want to do something like https://github.com/mozilla-b2g/gaia/blob/e9ab2febbab83dcbef737dac2f51ea9b635f0082/apps/communications/dialer/locales/dialer.en-US.properties#L49-L50

::: apps/system/test/unit/voicemail_test.js
@@ +154,5 @@
>  
> +              test('should/should not localize SIM indicator', function() {
> +                var getSpy = this.sinon.spy(MockL10n, 'get');
> +                Voicemail.showNotification('aaaa', 'bbbb', '1111', serviceId);
> +                if (serviceId == 0) {

You should use this.isMultiSIM.

@@ +159,5 @@
> +                  sinon.assert.notCalled(getSpy);
> +                } else {
> +                  sinon.assert.calledWith(
> +                    getSpy, 'sim-picker-button', { n: serviceId });
> +                }

This is not testing that we send the proper string to showNotification.
Attachment #8401475 - Flags: review?(anthony) → review-
Re-adding late-l10n based on comment 8.
Keywords: late-l10n
Whiteboard: exposing existing strings
Updated PR with a separate "review fixes" commit.
Attachment #8401475 - Attachment is obsolete: true
Attachment #8406239 - Flags: review?(anthony)
(In reply to Doug Sherk (:drs) from comment #10)
> Created attachment 8406239 [details] [diff] [review]
> Display SIM indication in voicemail notification.
> 
> Updated PR with a separate "review fixes" commit.

Actually, I basically rewrote it, so you might want to just read the patch posted here as an attachment instead.
Duplicate of this bug: 996521
Renom - we're string frozen, so we might not be able to take this for 1.4.
blocking-b2g: 1.4+ → 1.4?
(In reply to Jason Smith [:jsmith] from comment #13)
> Renom - we're string frozen, so we might not be able to take this for 1.4.

We can go back to using the old string so it would look like "(SIM1) New voicemail". This would be less ideal but at least it would be localized.
triage: with UX, let's proceed with old string then there's no localization issue.
blocking-b2g: 1.4? → 1.4+
Comment on attachment 8406239 [details] [diff] [review]
Display SIM indication in voicemail notification.

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

::: apps/system/locales/system.en-US.properties
@@ +37,5 @@
>  newVoicemails[few] = {{n}} new voicemails
>  newVoicemails[many] = {{n}} new voicemails
>  newVoicemails[other] = {{n}} new voicemails
>  newVoicemailsUnknown = New voicemail
> +voicemailNotificationMultiSim = (SIM {{n}}) {{message}}

Note for reviewer - per triage, this bug has to be fixed without introducing a new string.
Keywords: late-l10n
Just to know the blocking rationale:

The cost of not fixing this bug is that we don't report which SIM applies to the voicemail notification shown in the utility tray, right?
(In reply to Jason Smith [:jsmith] from comment #17)
> Just to know the blocking rationale:
> 
> The cost of not fixing this bug is that we don't report which SIM applies to
> the voicemail notification shown in the utility tray, right?

We currently report the SIM, but it isn't localized. So it will always say "SIM n - xxx". We can't fix any ordering issues here, such as it needing to be "xxx - SIM n" without a new string, but we can at least make sure that the "SIM n" part is localized.
(In reply to Doug Sherk (:drs) from comment #11)
> (In reply to Doug Sherk (:drs) from comment #10)
> > Created attachment 8406239 [details] [diff] [review]
> > Display SIM indication in voicemail notification.
> > 
> > Updated PR with a separate "review fixes" commit.
> 
> Actually, I basically rewrote it, so you might want to just read the patch
> posted here as an attachment instead.

@rik, since 1.4 is going to be finished, can you help to review the patch as soon as possible? Thank you.
Flags: needinfo?(anthony)
(In reply to Kevin Hu [:khu] from comment #19)
> @rik, since 1.4 is going to be finished, can you help to review the patch as
> soon as possible? Thank you.

I had to post a new version before he could review it. :)

Here's a new version, doing what I discussed in comment 14. I also updated the PR. I'm not actually sure why I asked Anthony to review this since he's not a System peer, so I'm moving review to Etienne.
Attachment #8406239 - Attachment is obsolete: true
Attachment #8406239 - Flags: review?(anthony)
Attachment #8409694 - Flags: review?(etienne)
Flags: needinfo?(anthony)
Filed bug 998972 for the followup fix.
Blocks: 998972
Comment on attachment 8409694 [details] [diff] [review]
Display SIM indication in voicemail notification.

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

r+ with nits

::: apps/system/test/unit/voicemail_test.js
@@ +222,5 @@
>                });
>  
> +              test('should/should not display SIM indicator', function() {
> +                var baseTitle = 'aaaa';
> +                var expectedTitle =

nit -> |var multiSimTitle|

@@ +225,5 @@
> +                var baseTitle = 'aaaa';
> +                var expectedTitle =
> +                  '(' + MockL10n.get('sim-picker-button', { n: 2 }) + ') ' +
> +                  baseTitle;
> +                var getSpy = this.sinon.spy(MockL10n, 'get');

don't think you're using this spy
Attachment #8409694 - Flags: review?(etienne) → review+
With review fixes, carrying r+.
Attachment #8409694 - Attachment is obsolete: true
Attachment #8410350 - Flags: review+
https://github.com/mozilla-b2g/gaia/commit/160752ee0fb3de6b27c401da73f2e5b87966c51a
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1090332
You need to log in before you can comment on or make changes to this bug.