Closed Bug 947192 Opened 7 years ago Closed 7 years ago

[DSDS][Wap Push] Message notification including SIM

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
tracking-b2g backlog

People

(Reporter: wmathanaraj, Assigned: gsvelto)

References

Details

Attachments

(7 files, 6 obsolete files)

1.11 MB, application/pdf
Details
1.11 MB, application/pdf
Details
1.12 MB, application/pdf
Details
1.13 MB, application/pdf
Details
46 bytes, text/x-github-pull-request
Details | Review
28.48 KB, patch
julienw
: review+
Details | Diff | Splinter Review
25.07 KB, patch
julienw
: review+
Details | Diff | Splinter Review
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
Summary: [DSDS] [Wap Push] Message notification including SIM → [DSDS][Wap Push] Message notification including SIM
(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.
Flags: needinfo?(wmathanaraj)
in the Wap Push message I dont see any need to indicate if the message was received via SIM1 or SIM2 - does this help?
Flags: needinfo?(wmathanaraj)
(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!
UX spec updated
Add one page for normal process flow
Moving to the new Gaia::Wappush component.
Component: Gaia → Gaia::Wappush
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
feature does not block release
blocking-b2g: 1.4+ → backlog
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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8386803 - Flags: review?(felash)
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.
Attachment #8386806 - Flags: review?(felash)
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 :) )
Attachment #8386803 - Flags: review?(felash)
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.
Attachment #8386806 - Flags: review?(felash)
I had attached the wrong patch... Let me revisit the first patch with promises and then I'll ask for review again.
Attachment #8386806 - Attachment is obsolete: true
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 :)
Attachment #8386803 - Attachment is obsolete: true
Attachment #8390645 - Flags: review?(felash)
Attachment #8387773 - Attachment is obsolete: true
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?)
(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 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.
Attachment #8390645 - Flags: review?(felash) → review+
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 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 :)
Attachment #8390646 - Flags: review?(felash)
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.
Attachment #8390645 - Attachment is obsolete: true
Attachment #8402753 - Flags: review?(felash)
Updated patch with review comments addressed.
Attachment #8390646 - Attachment is obsolete: true
Attachment #8403133 - Flags: review?(felash)
With these patches Travis is green: https://travis-ci.org/mozilla-b2g/gaia/builds/22509701
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)
Attachment #8402753 - Flags: review?(felash)
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 /
Attachment #8403133 - Flags: review?(felash)
(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.
(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.
(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 +.
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.
Flags: needinfo?(felash)
Hey, you're the performance specialist here, I won't contradict you :)

Ask review again when you're ready !
Flags: needinfo?(felash)
Updated patch with the notification string modified to match SMS notifications. No other changes were done.
Attachment #8403133 - Attachment is obsolete: true
Attachment #8408356 - Flags: review?(felash)
Attachment #8402753 - Flags: review?(felash)
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
Attachment #8402753 - Flags: review?(felash) → review+
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
Attachment #8408356 - Flags: review?(felash) → review+
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
Merged to gaia/master df15cc78290ba2ef7bebc5178a78878b23aee590

https://github.com/mozilla-b2g/gaia/commit/df15cc78290ba2ef7bebc5178a78878b23aee590
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 1014638
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.