892530 - [User Story] Replacing a Notification

Status: VERIFIED FIXED

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

Description

[Peter Dolanjski [:pdol]]

User Story:

As a developer I want the ability to replace an existing notification with an updated one even when the originating app is not open so that I can ensure that notifications are updated with the latest information as it becomes available.


Acceptance Criteria:

1. If my app issues a replace command for a currently active (shown in utility tray) notification, the notification is updated to reflect the information contained in the update.
2. If my app issues a replace command for a notification that has already been dismissed (no longer shown in utility tray), a new notification reflecting the information continaed in the update is shown in the utility tray.

Assumption: W3C spec is adhered to.

Comment 1

[Peter Dolanjski [:pdol]]

After further discussion with engineering, the user story has been updated to only cover the scenario where the app is open (it is assumed that the app will open via system timers or an equivalent mechanism):

As a developer I want the ability to replace an existing notification with an updated one so that I can ensure that notifications are updated with the latest information as it becomes available.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

UX Question: Is there any sort of animation required to show a user a notification is being replaced (i.e. sliding a new notification into the slot the old one was in), or are we simply updating the text in the notification?

Comment 3

[Candice Serran (:cserran)]

Candice Serran changed story state to started in Pivotal Tracker

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

It looks like the code to do the actual replacement landed as part of bug 900279, so we just need to add a call to allow replacement that just calls addNotification and makes sure the details have a matching notification id.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Actually, we don't even need to do that. As long as the new notifications API is used, we can pass a notification tag, for which there is already code based on tag matching landed for replacing notifications. So once 903547 lands, this should "just work". There's already unit tests in gaia, also. We'll still need to implement integration tests.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #5)
> Actually, we don't even need to do that. As long as the new notifications
> API is used, we can pass a notification tag, for which there is already code
> based on tag matching landed for replacing notifications. So once 903547
> lands, this should "just work". There's already unit tests in gaia, also.
> We'll still need to implement integration tests.

FWIW - I just tested this on trunk with my test app here (http://mozilla.github.io/qa-testcase-data/webapi/notifications/index.html) and confirmed that bare bone smoke tests show this feature is already implemented on trunk. Note that the testing done here isn't extensive, but that implies the basics are implemented here.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Integration Tests for Notification Firing and Replacement

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Integration Tests for Notification Firing and Replacement

First patch was completely wrong file. Oops.

Comment 9

[James Lal [:lightsofapollo]]

Comment on attachment 794401 [details] [diff] [review]
Patch 1 (v2) - Integration Tests for Notification Firing and Replacement

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

I made some comments (most of them just to clear up the text or remove functionality you don't use).

In general I would like to see us build out a robust interface for testing notifications (outside of simply verifying the gecko <-> gaia glue works) but that is a bigger issue that we can build on top of this. Lots of apps use notifications and probably want to verify they are showing up as expected so sharing that code with some abstraction is a good idea.

::: apps/system/test/marionette/notifications.js
@@ +1,3 @@
> +var assert = require('assert');
> +
> +var systemapp = 'app://system.gaiamobile.org';

you can remove this the default context will always be the system app

@@ +5,5 @@
> +marionette('notification tests', function() {
> +  var client = marionette.client();
> +
> +  setup(function() {
> +    client.switchToFrame();

unless you plan on switching outside of the system app (which I don't see here in this test) you can remove this setup block

@@ +10,5 @@
> +  });
> +
> +  test('fire notification', function() {
> +    var url = client.getUrl();
> +    assert.ok(url.indexOf(systemapp) !== -1);

remove the system app assertions

@@ +14,5 @@
> +    assert.ok(url.indexOf(systemapp) !== -1);
> +    client.executeScript(function() {
> +      new Notification('testing', { body: 'testing', tag: '123' });
> +    });
> +    assert.ok(client.findElement(

assert body too?

@@ +20,5 @@
> +  });
> +
> +  test('replace notification', function() {
> +    var url = client.getUrl();
> +    assert.ok(url.indexOf(systemapp) !== -1);

same as above

@@ +30,5 @@
> +                       { body: 'replacement', tag: '123' });
> +    });
> +    var el = client.findElement(
> +      '[data-notification-id="app://system.gaiamobile.org#tag:123"] > .detail');
> +    assert.ok(el);

if you have multiple assertions add a description so we know what happens when it fails:

assert.ok(el, 'notification is in dom');

@@ +31,5 @@
> +    });
> +    var el = client.findElement(
> +      '[data-notification-id="app://system.gaiamobile.org#tag:123"] > .detail');
> +    assert.ok(el);
> +    var txt = el.getAttribute('innerHTML');

I think el.text(); is what you want here

@@ +32,5 @@
> +    var el = client.findElement(
> +      '[data-notification-id="app://system.gaiamobile.org#tag:123"] > .detail');
> +    assert.ok(el);
> +    var txt = el.getAttribute('innerHTML');
> +    assert.ok(txt == 'replacement');

assert.equal(txt, 'replacement')

Comment 10

[James Lal [:lightsofapollo]]

A more general comment I would have is even if you prefer the "review as a patch" model I would suggest creating a pull request. Our travis ci setup will run your new test on the latest master + your soon to be merged code.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to James Lal [:lightsofapollo] from comment #9)
> @@ +31,5 @@
> > +    });
> > +    var el = client.findElement(
> > +      '[data-notification-id="app://system.gaiamobile.org#tag:123"] > .detail');
> > +    assert.ok(el);
> > +    var txt = el.getAttribute('innerHTML');
> 
> I think el.text(); is what you want here

I was getting an empty string back when I tried that. That's why I went with innerHTML.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Integration Tests for Notification Firing and Replacement

Comment 13

[Candice Serran (:cserran)]

Kyle Machulis changed story state to delivered in Pivotal Tracker

Comment 14

[Candice Serran (:cserran)]

Kyle Machulis changed story state to accepted in Pivotal Tracker

Comment 15

[Candice Serran (:cserran)]

Kyle Machulis changed story state to accepted in Pivotal Tracker

Comment 16

[James Lal [:lightsofapollo]]

Comment on attachment 794928 [details] [review]
Patch 1 (v3) - Integration Tests for Notification Firing and Replacement

Stealing whole review (per in person conversation)...

Comment 17

[James Lal [:lightsofapollo]]

https://github.com/mozilla-b2g/gaia/commit/42d88f26420b6c58621d1b68675d512fd10c9fcb

Comment 18

[Jason Smith [:jsmith]]

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2226

Comment 19

[Jason Smith [:jsmith]]

Verified per completion of initial test pass.