947192 - [DSDS][Wap Push] Message notification including SIM

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Wappush, defect, P1)

Description

[Wilfred Mathanaraj [:WDM]]

As a user, when I receive a WAP Push message, I want to see which SIM the WAP Push message was received on

Acceptance criteria

AC1: Only want to see the recevied SIM inidcator in the notification
AC2: I dont want to see any SIM notification directly in the message

Comment 1

[Enpei]

(In reply to Wilfred Mathanaraj [:WDM] from comment #0)
> As a user, when I receive a WAP Push message, I want to see which SIM the
> WAP Push message was received on
> 
> Acceptance criteria
> 
> AC1: Only want to see the recevied SIM inidcator in the notification
> AC2: I dont want to see any SIM notification directly in the message

Hi Wilfred,

Could you kindly explain AC2 more? Do you mean no SIM indicator in WAP push message?

Thanks.

Comment 2

[Wilfred Mathanaraj [:WDM]]

in the Wap Push message I dont see any need to indicate if the message was received via SIM1 or SIM2 - does this help?

Comment 3

[Enpei]

(In reply to Wilfred Mathanaraj [:WDM] from comment #2)
> in the Wap Push message I dont see any need to indicate if the message was
> received via SIM1 or SIM2 - does this help?

Thank you!

Comment 4

[Neo Hsieh [:neo]]

Attachment: [DSDS] WAP_Push_140110.pdf

Comment 5

[Neo Hsieh [:neo]]

Attachment: [DSDS] WAP_Push_140121.pdf

UX spec updated

Comment 6

[Neo Hsieh [:neo]]

Attachment: [DSDS] WAP_Push_140121_fixed.pdf

Add one page for normal process flow

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Moving to the new Gaia::Wappush component.

Comment 8

[Neo Hsieh [:neo]]

Attachment: [DSDS] WAP_Push_140128.pdf

Comment 9

[Wilfred Mathanaraj [:WDM]]

feature does not block release

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2] Simplify the way notifications are sent, simplify the tests accordingly

This patch greatly simplifies the code used to send notifications by taking out the request for the app's own reference and moving it into the initialization phase. This simplifies the unit tests too and paves the way for the next patch that adds SIM-specific tagging and more thorough testing of the notifications.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2] If multiple SIMs are present add the SIM number to the notification

This tags notifications with the SIM number if more than one SIM is present. The existing and new functionality is covered by additional tests that ensure the right text is put in the notification. I've tested this both in the desktop build by simulating the delivery of the messages and in the emulator with an actual message.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8386803 [details] [diff] [review]
[PATCH 1/2] Simplify the way notifications are sent, simplify the tests accordingly

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

I'd like to see how a Promise-enabled init would work, so reserving my r for now.

::: apps/wappush/js/wappush.js
@@ +69,4 @@
>  
> +        req = navigator.mozSettings.createLock().get(wapPushEnableKey);
> +        req.onsuccess = function wpm_settingsLockSuccess() {
> +          wapPushEnabled = req.result[wapPushEnableKey];

I feel like we could do both req in parallel.

You could do this quite easily by doing 2 different functions returning promises: one that gets the app object, the other that gets the setting. They would reject the promise in the onerror callback.

You can also create a method to set the message handler.

Then here, you just need to do 

  return Promise.all([fetchAppObject(), fetchSetting()]
    .then(setMessageHandler)
    .catch(function(err) {
      err = err || 'UnknownError';
      console.error('Got an error while initing: ', err);
    });

(and you get rid of the "done" callback by returning a promise too :) )

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8386806 [details] [diff] [review]
[PATCH 2/2] If multiple SIMs are present add the SIM number to the notification

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

I don't understand this patch. Is it the right file ? :)

you can have a look to bug 947180 if necessary.

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2] If multiple SIMs are present add the SIM number to the notification

I had attached the wrong patch... Let me revisit the first patch with promises and then I'll ask for review again.

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v2] Simplify the way notifications are sent, simplify the tests accordingly

Updated patch using promises for initialization. They're way too cool! I'm probably going to rewrite significant other chunks of the app using them :)

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2 v2] If multiple SIMs are present add the SIM number to the notification

Comment 17

[Julien Wajsberg [:julienw]]

Gabriele, since it has lesser priority than other work that I'm doing, I'll need to keep it for after my holidays. Otherwise you can try to find another reviewier (maybe jaoo?)

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #17)
> Gabriele, since it has lesser priority than other work that I'm doing, I'll
> need to keep it for after my holidays. Otherwise you can try to find another
> reviewier (maybe jaoo?)

Take your time, this isn't high priority for me either but it taught me how to use promises :)

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Add SIM number to the WAP Push message notification

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8390645 [details] [diff] [review]
[PATCH 1/2 v2] Simplify the way notifications are sent, simplify the tests accordingly

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

r=me with nits, a green travis, and provided you tested this works as expected in the emulator.

Feels better!

::: apps/wappush/js/wappush.js
@@ +64,5 @@
> +      req.onsuccess = function wpm_gotApp(event) {
> +        resolve(event.target.result);
> +      };
> +      req.onerror = function wpm_getAppError() {
> +        reject('Could not obtain the app object');

you probably have an error in this.error that you can use. Maybe this.error.name contains something useful, but you need to check the code (or IDL)

@@ +82,5 @@
> +      req.onsuccess = function wpm_settingsLockSuccess() {
> +        resolve(req.result[wapPushEnableKey]);
> +      };
> +      req.onerror = function wpm_settingsLockError() {
> +        reject('Could not read the pref');

same here

@@ +124,5 @@
> +    }).catch(function(error) {
> +      // If we encountered an error don't process messages
> +      wapPushEnabled = false;
> +      error = error || 'Unknown error';
> +      console.error('Could not initialize: ' + error);

nit: "console.error('Could not initialize:', error);"

::: apps/wappush/test/unit/wappush_test.js
@@ +92,5 @@
>        }
>      });
> +
> +    MockNavigatormozSetMessageHandler.mSetup();
> +    WapPushManager.init().then(done);

always use ".then(done, done)" so that errors are caught too.

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8390645 [details] [diff] [review]
[PATCH 1/2 v2] Simplify the way notifications are sent, simplify the tests accordingly

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

::: apps/wappush/js/wappush.js
@@ +218,5 @@
>  
> +        var notification = new Notification(message.sender, options);
> +        notification.addEventListener('click',
> +          function wpm_onNotificationClick(event) {
> +            app.launch();

I'm not sure it's that good to cache the app because we use it only here. How about using `wpm_getApp().then(function(app) { app.launch(); });` ?

I worry that caching the app at startup is unnecessary and will take a not so small amount of memory.

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 8390646 [details] [diff] [review]
[PATCH 2/2 v2] If multiple SIMs are present add the SIM number to the notification

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

Hey, here are my comments, sorry for the delay!

Please request review once you're ready again :)

::: apps/wappush/js/wappush.js
@@ +201,5 @@
> +
> +    /* If the phone has more than one SIM prepend the number of the SIM on
> +     * which this message was received */
> +    if (navigator.mozMobileConnections &&
> +        (navigator.mozMobileConnections.length > 1)) {

This doesn't work because when the device has 2 SIM slots but 1 SIM only, we should not prepend the SIM. In that case mozMobileConnections is still 2. In the multi sim action button, we do [2], I think this is currently the best way. In the SMS app we do something different [3] but I think it's not necessary here.

[2] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/multi_sim_action_button.js#L24-L25
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/settings.js#L110-L131

@@ +202,5 @@
> +    /* If the phone has more than one SIM prepend the number of the SIM on
> +     * which this message was received */
> +    if (navigator.mozMobileConnections &&
> +        (navigator.mozMobileConnections.length > 1)) {
> +      title = _('sim') + (+message.serviceId + 1) + ' - ' + title;

I think you want to put the whole string in the locale, instead of concatenating. This is better for localizations, especially right to left.

See what I did for SMS: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/locales/sms.en-US.properties#L215-L217

(BTW the SIM indication should be "SIM 1" instead of "SIM1", see bug 985273 comment 5)

::: apps/wappush/test/unit/wappush_test.js
@@ +214,5 @@
> +
> +      test('the notification is populated correctly for SIM1', function() {
> +        var notificationSpy = this.sinon.spy(window, 'Notification');
> +        MockNavigatormozSetMessageHandler.mTrigger('wappush-received', message);
> +        assert.isTrue(notificationSpy.calledOnce);

use sinon.assert :)

Comment 23

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v3] Simplify the way notifications are sent, simplify the tests accordingly

Thanks for the review! Sorry for asking yet-another review but I made a further change that I had forgotten (I've also addressed all of your review comments). In the wpm_onNotification() function I was still calling navigator.mozApps.getSelf() to retrieve the app object but that's pointless because we retrieve it during initialization so I simplified that bit of code accordingly. This had a side-effect however in that I needed to make sure that the notification listener was called after initialization so that everything would be properly in place. For good measure I've moved registering all event listeners at the end of the initialization procedure as this makes them robust in face of potential race conditions with initialization. Now basically nothing happens until all the asynchronous stuff has been dealt with properly. No changes were needed in the unit-tests and this was re-tested in the emulator to ensure everything works correctly.

Comment 24

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2 v3] If multiple SIMs are present add the SIM number to the notification

Updated patch with review comments addressed.

Comment 25

[Gabriele Svelto [:gsvelto] (PTO)]

With these patches Travis is green: https://travis-ci.org/mozilla-b2g/gaia/builds/22509701

Comment 26

[Julien Wajsberg [:julienw]]

Comment on attachment 8402753 [details] [diff] [review]
[PATCH 1/2 v3] Simplify the way notifications are sent, simplify the tests accordingly

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

::: apps/wappush/js/wappush.js
@@ +33,5 @@
>    /** Enable/disable WAP Push notifications */
>    var wapPushEnabled = true;
>  
> +  /** A reference to the app's object */
> +  var app = null;

Have you read my comment 21?

I think it isn't useful to cache the app reference because we use it only once in the life of the application (even if it's twice in the code base)

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8403133 [details] [diff] [review]
[PATCH 2/2 v3] If multiple SIMs are present add the SIM number to the notification

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

mostly nits but I'd like one more round !
Thanks :)

::: apps/wappush/locales/wappush.en-US.properties
@@ +7,5 @@
>  try-again        = Try again
>  finish           = Finish
> +sim              = SIM {{id}}
> +
> +dsds-notification-title-with-sim = ({{sim}}) - {{title}}

I think we want no "dash", only the parenthesis

::: apps/wappush/test/unit/wappush_test.js
@@ +203,5 @@
> +      test('the notification is populated correctly for SIM1', function() {
> +        this.sinon.spy(window, 'Notification');
> +        MockNavigatormozSetMessageHandler.mTrigger('wappush-received', message);
> +        sinon.assert.calledOnce(Notification);
> +        sinon.assert.calledWithMatch(Notification, new RegExp('.*1.*'), {

why don't you use the value returned by the L10n mock? Because it's too complicated?

if you want to use a regexp, just use the usual syntax with /

Comment 28

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Have you read my comment 21?

Awww, no, I missed it :(

> I think it isn't useful to cache the app reference because we use it only
> once in the life of the application (even if it's twice in the code base)

OK, I'll use the promise instead.

Comment 29

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #27)
> mostly nits but I'd like one more round !
> Thanks :)

Excellent, thanks for the review.

> I think we want no "dash", only the parenthesis

The spec in attachment 8366551 [details] shows a dash but no parenthesis (and states SIM1 instead of SIM 1) but if SMS uses "(SIM 1) title" I think WAP Push should be consistent with it so I'll remove the dash.

> why don't you use the value returned by the L10n mock? Because it's too
> complicated?

It's a huge string of JSON-ified objects and the only thing that changes and which we care about is the SIM number :)

> if you want to use a regexp, just use the usual syntax with /

OK, I'll do that.

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Gabriele Svelto [:gsvelto] from comment #29)
> 
> > I think we want no "dash", only the parenthesis
> 
> The spec in attachment 8366551 [details] shows a dash but no parenthesis
> (and states SIM1 instead of SIM 1) but if SMS uses "(SIM 1) title" I think
> WAP Push should be consistent with it so I'll remove the dash.

Yes, that's been changed in latest spec (I don't have a ref right now), because the dash makes it ugly when the title is a number starting with +.

Comment 31

[Gabriele Svelto [:gsvelto] (PTO)]

Actually when I went back to modify the first patch I suddenly realized why it was so important to read the app object in the initialization procedure and not later: it makes sending the notifications a lot simpler and allowed me to cut a large amount of testing code. If you don't mind I'd like to keep that, the app object is pretty small anyway and is already present in the app, what you get is just a reference to it.

Comment 32

[Julien Wajsberg [:julienw]]

Hey, you're the performance specialist here, I won't contradict you :)

Ask review again when you're ready !

Comment 33

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2 v4] If multiple SIMs are present add the SIM number to the notification

Updated patch with the notification string modified to match SMS notifications. No other changes were done.

Comment 34

[Julien Wajsberg [:julienw]]

Comment on attachment 8402753 [details] [diff] [review]
[PATCH 1/2 v3] Simplify the way notifications are sent, simplify the tests accordingly

r=me, good work

Comment 35

[Julien Wajsberg [:julienw]]

Comment on attachment 8408356 [details] [diff] [review]
[PATCH 2/2 v4] If multiple SIMs are present add the SIM number to the notification

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

r=me with the following test change; please check that travis is still running after this.

::: apps/wappush/test/unit/wappush_test.js
@@ +203,5 @@
> +      test('the notification is populated correctly for SIM1', function() {
> +        this.sinon.spy(window, 'Notification');
> +        MockNavigatormozSetMessageHandler.mTrigger('wappush-received', message);
> +        sinon.assert.calledOnce(Notification);
> +        sinon.assert.calledWithMatch(Notification, new RegExp('.*1.*'), {

nit: just use `/1/` instead of the Regexp constructor. You don't need `.*` either.

I verified this works in http://jsfiddle.net/9hFn6/ (open the console and run it) :)

@@ +213,5 @@
> +        this.sinon.spy(window, 'Notification');
> +        message.serviceId = 1;
> +        MockNavigatormozSetMessageHandler.mTrigger('wappush-received', message);
> +        sinon.assert.calledOnce(Notification);
> +        sinon.assert.calledWithMatch(Notification, new RegExp('.*2.*'), {

same here

Comment 36

[Gabriele Svelto [:gsvelto] (PTO)]

Thanks for the review! It took me a while to figure out that I had to literally use /1/ and not the '/1/' string; I'm still way to influenced by my low-level C background to get really fluent in JS :-P

Anyway, the Travis run is here: https://travis-ci.org/mozilla-b2g/gaia/builds/232951124

Comment 37

[Gabriele Svelto [:gsvelto] (PTO)]

Merged to gaia/master df15cc78290ba2ef7bebc5178a78878b23aee590

https://github.com/mozilla-b2g/gaia/commit/df15cc78290ba2ef7bebc5178a78878b23aee590