Closed Bug 892530 Opened 7 years ago Closed 7 years ago

[User Story] Replacing a Notification

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+)

VERIFIED FIXED
blocking-b2g koi+

People

(Reporter: pdol, Assigned: qdot)

References

Details

(Keywords: feature, Whiteboard: [ucid:System36, FT:systems-fe, KOI:P1][systemsfe])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 892522
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.
Whiteboard: [ucid:System36] → [ucid:System36, FT:systems-fe, KOI:P1]
Assignee: nobody → kyle
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?
Candice Serran changed story state to started in Pivotal Tracker
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.
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.
Flags: in-moztrap?(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.
First patch was completely wrong file. Oops.
Attachment #794400 - Attachment is obsolete: true
Attachment #794400 - Flags: review?(jlal)
Attachment #794400 - Flags: review?(anygregor)
Attachment #794401 - Flags: review?(jlal)
Attachment #794401 - Flags: review?(anygregor)
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')
Attachment #794401 - Flags: review?(jlal) → review-
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.
(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.
Attachment #794401 - Attachment is obsolete: true
Attachment #794401 - Flags: review?(anygregor)
Attachment #794928 - Flags: review?(jlal)
Attachment #794928 - Flags: review?(anygregor)
Kyle Machulis changed story state to delivered in Pivotal Tracker
Kyle Machulis changed story state to accepted in Pivotal Tracker
Kyle Machulis changed story state to accepted in Pivotal Tracker
Comment on attachment 794928 [details] [review]
Patch 1 (v3) - Integration Tests for Notification Firing and Replacement

Stealing whole review (per in person conversation)...
Attachment #794928 - Flags: review?(jlal)
Attachment #794928 - Flags: review?(anygregor)
Attachment #794928 - Flags: review+
https://github.com/mozilla-b2g/gaia/commit/42d88f26420b6c58621d1b68675d512fd10c9fcb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Whiteboard: [ucid:System36, FT:systems-fe, KOI:P1] → [ucid:System36, FT:systems-fe, KOI:P1][systemsfe]
blocking-b2g: koi? → koi+
Verified per completion of initial test pass.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.