[DSDS][Wap Push] Message notification including SIM

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: wmathanaraj, Assigned: gsvelto)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

Attachments

(7 attachments, 6 obsolete attachments)

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 | Splinter Review
28.48 KB, patch
julienw
: review+
Details | Diff | Splinter Review
25.07 KB, patch
julienw
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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

Comment 1

5 years ago
(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)
(Reporter)

Comment 2

5 years ago
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)

Comment 3

5 years ago
(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!
Depends on: 951999

Comment 4

5 years ago
Created attachment 8361423 [details]
[DSDS] WAP_Push_140110.pdf

Comment 5

5 years ago
Created attachment 8362806 [details]
[DSDS] WAP_Push_140121.pdf

UX spec updated

Comment 6

5 years ago
Created attachment 8362810 [details]
[DSDS] WAP_Push_140121_fixed.pdf

Add one page for normal process flow
(Assignee)

Comment 7

5 years ago
Moving to the new Gaia::Wappush component.
Component: Gaia → Gaia::Wappush
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM

Comment 8

5 years ago
Created attachment 8366551 [details]
[DSDS] WAP_Push_140128.pdf
No longer blocks: 945641
No longer blocks: 942446
(Reporter)

Comment 9

5 years ago
feature does not block release
blocking-b2g: 1.4+ → backlog
(Assignee)

Comment 10

5 years ago
Created attachment 8386803 [details] [diff] [review]
[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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8386803 - Flags: review?(felash)
(Assignee)

Comment 11

5 years ago
Created attachment 8386806 [details] [diff] [review]
[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.
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)
(Assignee)

Comment 14

5 years ago
Created attachment 8387773 [details] [diff] [review]
[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.
Attachment #8386806 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 8390645 [details] [diff] [review]
[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 :)
Attachment #8386803 - Attachment is obsolete: true
Attachment #8390645 - Flags: review?(felash)
(Assignee)

Comment 16

5 years ago
Created attachment 8390646 [details] [diff] [review]
[PATCH 2/2 v2] If multiple SIMs are present add the SIM number to the notification
Attachment #8390646 - Flags: review?(felash)
(Assignee)

Updated

5 years ago
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?)
(Assignee)

Comment 18

5 years ago
(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 :)
(Assignee)

Comment 19

5 years ago
Created attachment 8391344 [details] [review]
[PULL REQUEST] Add SIM number to the WAP Push message notification
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)
(Assignee)

Comment 23

5 years ago
Created attachment 8402753 [details] [diff] [review]
[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.
Attachment #8390645 - Attachment is obsolete: true
Attachment #8402753 - Flags: review?(felash)
(Assignee)

Comment 24

5 years ago
Created attachment 8403133 [details] [diff] [review]
[PATCH 2/2 v3] If multiple SIMs are present add the SIM number to the notification

Updated patch with review comments addressed.
Attachment #8390646 - Attachment is obsolete: true
Attachment #8403133 - Flags: review?(felash)
(Assignee)

Comment 25

5 years ago
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)
(Assignee)

Comment 28

5 years ago
(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.
(Assignee)

Comment 29

5 years ago
(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 +.
(Assignee)

Comment 31

5 years ago
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)
(Assignee)

Comment 33

5 years ago
Created attachment 8408356 [details] [diff] [review]
[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.
Attachment #8403133 - Attachment is obsolete: true
Attachment #8408356 - Flags: review?(felash)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 36

5 years ago
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
(Assignee)

Comment 37

5 years ago
Merged to gaia/master df15cc78290ba2ef7bebc5178a78878b23aee590

https://github.com/mozilla-b2g/gaia/commit/df15cc78290ba2ef7bebc5178a78878b23aee590
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 1014638
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.