1080253 - notification count is wrong

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

Flame, nightly channel (master + mozilla-central), Oct 7 build

See screenshot.

STR:

1. start up phone, check for updates
2. once you see the update notification, go check
3. says zero notifications, even though there's a notification visible
4. take a screenshot
5. says zero notifications, even though there's a notification visible
6. take another screenshot
7. says one notification, even though there are three.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: 2014-10-08-15-48-36.png

Comment 2

[Dietrich Ayala (:dietrich)]

Sorry, clarification - it did count both screenshots. So the sequence was that it didn't count the update notification, so was always one notification short.

Comment 3

[Safwan Rahman (:safwan)]

It can be reproduced in latest maser V2.2

Environmental Variables:
Device: Keon V2.2
BuildID: 20141008012326
Gaia: 68b8bc9
Gecko: 652c5d4
Version: 35.0a1

Comment 4

[Dietrich Ayala (:dietrich)]

Thanks Safwan!

Comment 5

[Safwan Rahman (:safwan)]

Attachment: Screenshot in V2.1

Interesting fact is, Notification count is not available in Firefox OS 2.1

Environmental Variables:
Device: Keon V2.1
BuildID: 20141008125311
Gaia: d71f8804
Gecko: f4d4a81
Version: 34.0a2

Comment 6

[Manuel Casas Barrado [:mancas]]

Attachment: Notification count is displayed correctly

The notification count is updated from the |desktop-notifications-container|. However, system updates notification is placed in other container called |update-manager-container|. So we must check if this container is displayed and in that case we must increment the notification count.

Etienne, I'm not sure about if the approach I've made it's the best way to do this task. Take a look and let me know if you think in another solution for this.

Thanks

Comment 7

[Manuel Casas Barrado [:mancas]]

Attachment: notifications-count.png

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8503018 [details] [review]
Notification count is displayed correctly

Looks like bug 1073480 is going to render this one invalid :)

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8503018 [details] [review]
Notification count is displayed correctly

My bad, I read bug 1073480's summary too quickly :)

Anyway the way we get this count is rather fragile (currently), and update manager notifications are not the only other fake notifications so it could become quite complex (with tons of |if (fake notif displayed) count+++...|)

I wonder if we could do the |querySelectorAll('.notification')| on |this.notifications| instead and catch the correct number in a future-proof way. 

What do you think?

Comment 10

[Yeojin Chung [:YeojinC]]

(In reply to Safwan Rahman from comment #5)
> Created attachment 8502683 [details]
> Screenshot in V2.1
> 
> Interesting fact is, Notification count is not available in Firefox OS 2.1
> 
> Environmental Variables:
> Device: Keon V2.1
> BuildID: 20141008125311
> Gaia: d71f8804
> Gecko: f4d4a81
> Version: 34.0a2

I have filed a bug on this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1081278

Comment 11

[Manuel Casas Barrado [:mancas]]

(In reply to Etienne Segonzac (:etienne) from comment #9)
> Comment on attachment 8503018 [details] [review]
> Notification count is displayed correctly
> 
> My bad, I read bug 1073480's summary too quickly :)
> 
> Anyway the way we get this count is rather fragile (currently), and update
> manager notifications are not the only other fake notifications so it could
> become quite complex (with tons of |if (fake notif displayed) count+++...|)
> 
> I wonder if we could do the |querySelectorAll('.notification')| on
> |this.notifications| instead and catch the correct number in a future-proof
> way. 
> 
> What do you think?

That's what I thought firstly. However we need to add |notification| class to the fake notifications in order to get the correct number. Or we can make two different calls:
|this.notifications.querySelectorAll('.notification')| and |this.notifications.querySelectorAll('.fake-notification')|

What do you think is the best way?

Comment 12

[Manuel Casas Barrado [:mancas]]

Sorry, but we can't add the |notification| class to fake notifications. If we do that, the styles will be overlapped

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #11)
> (In reply to Etienne Segonzac (:etienne) from comment #9)
> > Comment on attachment 8503018 [details] [review]
> > Notification count is displayed correctly
> > 
> > My bad, I read bug 1073480's summary too quickly :)
> > 
> > Anyway the way we get this count is rather fragile (currently), and update
> > manager notifications are not the only other fake notifications so it could
> > become quite complex (with tons of |if (fake notif displayed) count+++...|)
> > 
> > I wonder if we could do the |querySelectorAll('.notification')| on
> > |this.notifications| instead and catch the correct number in a future-proof
> > way. 
> > 
> > What do you think?
> 
> That's what I thought firstly. However we need to add |notification| class
> to the fake notifications in order to get the correct number. Or we can make
> two different calls:
> |this.notifications.querySelectorAll('.notification')| and
> |this.notifications.querySelectorAll('.fake-notification')|
> 
> What do you think is the best way?

|this.notifications.querySelectorAll('.notification, .fake-notification')| :)

Comment 14

[Manuel Casas Barrado [:mancas]]

(In reply to Etienne Segonzac (:etienne) from comment #13)
> |this.notifications.querySelectorAll('.notification, .fake-notification')| :)

Hi! I've made the changes and it seems it works fine. Take a look when you want.

Just a comment on github:
https://github.com/mozilla-b2g/gaia/pull/25017/files#diff-690ac4f812d69b66c808d3e430042345R417

Thanks!

Comment 15

[Dietrich Ayala (:dietrich)]

Please make sure this patch lands with an accompanying automated regression test.

We should ever be able to regress basic functionality like this without our test suites instantly turning into a fiery inferno of failure warnings.

Comment 16

[Dietrich Ayala (:dietrich)]

Do we know what the regressing commit was?

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8503018 [details] [review]
Notification count is displayed correctly

Hey Manuel,

Dietrich makes a good point, here's what you'll need to cover this:

* if you're not set up yet to run marionette-js tests locally -> [1]
* you'll need to add a Selector/getter to get the notification count as displayed reliably, a bit like this [2]
* the notifications are pretty well covered so there's a lot of inspiration to draw upon, see [3]

You're test will need to
* check that the count is 0
* send a real notification
* check that the count is 1
* trigger a fake system update, for this you'll need to execute the following JS
```
UpdateManager.addToUpdatesQueue(UpdateManager.systemUpdatable);
UpdateManager.displayNotificationAndToaster();
```
we should extract this in a helper method in lib/system.js
* check that the count is 2

You can ping myself or kgrandon on IRC to get help (try kgrandon first ;))


[1] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_integration_tests
[2] https://github.com/mozilla-b2g/gaia/blob/4f86c631e0465c0e56ccebeb1324fd28be9ea32f/apps/system/test/marionette/lib/system.js#L40
[3] https://github.com/mozilla-b2g/gaia/blob/4f86c631e0465c0e56ccebeb1324fd28be9ea32f/apps/system/test/marionette/notification_behavior_test.js

Comment 18

[Manuel Casas Barrado [:mancas]]

Hey etienne!

I've made the integration test you required. However the others tests in |notifications_behavior_test.js|, are failing for this reason:

[marionette error] dummy file:67 evt.detail.mozbehavior is undefined

And I don't really know what it's happening

Thanks!

Comment 19

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #18)
> Hey etienne!
> 
> I've made the integration test you required. However the others tests in
> |notifications_behavior_test.js|, are failing for this reason:
> 
> [marionette error] dummy file:67 evt.detail.mozbehavior is undefined
> 
> And I don't really know what it's happening
> 

Looks nice!
You should create a separate test file for your test (notification_count_test.js), because the test file you're editing is specifically intended at covering the optional mozbehavior attribute of notifications.

Good thing is: it will probably fix your issue too :)

Comment 20

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8503018 [details] [review]
Notification count is displayed correctly

I've created a new file for this bug tests but the test of |notification_behavior.js| are failing.

Take a look when you have time

Thanks etienne!

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8503018 [details] [review]
Notification count is displayed correctly

they work fine on my machine :)
r=me with the 2 small comments on github addressed, thank you!

(and gaia-try will confirm that the tests are working :))

Comment 22

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/099ab1aea0edb8d88a85cebb299b81ad471506e5

Comment 23

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2.
The notification count is displayed correctly, including the update notification.

Flame 2.2 

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141022040201
Gaia: 4d7f051cede6544f4c83580253c743c22b0cb279
Gecko: ae4d9b4ff2ee
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0