892528 - [User Story] Closing 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 close notifications even when the originating app is not open so that I can ensure that notifications, which are no longer relevant are removed.


Acceptance Criteria:

1. If my app issues a close command for a currently active (shown in utility tray) notification, the notification is dismissed and removed from view.
2. If my app issues a close command for a notification that has already been dismissed (no longer shown in utility tray), nothing happens.

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 close notifications so that I can ensure that notifications, which are no longer relevant are removed.

Comment 2

[Julien Wajsberg [:julienw]]

Note that there is one Gaia patch in bug 782211 that has not landed yet, I'll do it later today.

So (maybe) this bug will be fixed after this.

Comment 3

[Julien Wajsberg [:julienw]]

Just filed bug 899574 so that we can get the active notifications for an app, which is necessary to survive app restarts.

Comment 4

[Julien Wajsberg [:julienw]]

Added a bunch of bugs I filed as part of my tests in bug 855165. Some of them have simple patches if you feel like reviewing them.

Comment 5

[Candice Serran (:cserran)]

Candice Serran changed story state to started in Pivotal Tracker

Comment 6

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

Once bug 903547 lands, this should work for any app using the new notification API, since they can just call close() on the notification and the code is already in to check for tags and close accordingly.

Comment 7

[Julien Wajsberg [:julienw]]

Maybe in this bug you can add some code in the UITest test app to be able to close a notification ?

The code is in /test_apps/uitest/js/newnotification.js and /test_apps/uitest/js/newnotification.html (I added these files in bug 901856)

Comment 8

[Caio Lima(:caiolima)]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> Maybe in this bug you can add some code in the UITest test app to be able to
> close a notification ?
> 
> The code is in /test_apps/uitest/js/newnotification.js and
> /test_apps/uitest/js/newnotification.html (I added these files in bug 901856)

julienw, I've done it in bug https://bugzilla.mozilla.org/show_bug.cgi?id=907375

It worked well here.

Comment 9

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

Attachment: Patch 1 (v1) - Integration Tests for Closing Notifications

Comment 10

[James Lal [:lightsofapollo]]

Comment on attachment 794966 [details] [diff] [review]
Patch 1 (v1) - Integration Tests for Closing Notifications

See github (some minor changes requested)

Comment 11

[James Lal [:lightsofapollo]]

Comment on attachment 794966 [details] [diff] [review]
Patch 1 (v1) - Integration Tests for Closing Notifications

https://github.com/qdot/gaia/commit/64339f4bfd563ca0ecaf0698e66db979c8df6898

Comment 12

[James Lal [:lightsofapollo]]

wrong commit above...

in master: https://github.com/mozilla-b2g/gaia/commit/a3b860b817d4877c10b43c34efdc1d43695fd9f7

Comment 13

[James Lal [:lightsofapollo]]

So I landed this but you still need a test for .onclose probably see https://bugzilla.mozilla.org/show_bug.cgi?id=908978

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

[Candice Serran (:cserran)]

Kyle Machulis changed story state to accepted in Pivotal Tracker

Comment 17

[Jason Smith [:jsmith]]

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

Comment 18

[Jason Smith [:jsmith]]

Verified per completion of initial test pass.