915643 - [Email] Clear old notifications once Notification.get is available

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[James Burke [:jrburke]]

If the user enters the email app through the email app icon and views the inbox with the messages that were part of a notification, then the email app should remove the notification from the system notification overlay.

This is will be possible to do once bug 899574 lands.

Comment 1

[Tony Chung [:tchung]]

James, do you need this bug to be koi blocker?  if so, please nom the flag to be blocking-b2g=koi?

Comment 2

[James Lal [:lightsofapollo]]

This is very important so marking koi? for Monday triage.

Basically we will end up in a situation where tons of notifications are showing because we cannot find them after the email app closes [so if you have 1k new emails you may end up with 1k notifications]. I didn't directly work on this feature but I believe that is the current state without Notification.get at least in some situations.

jrburke can you please confirm?

Comment 3

[James Burke [:jrburke]]

Since the fix for bug 892522 (email notification grouping) was landed, it should not result in multiple notifications, just one notification per email account configured.

However, the count will likely be wrong in the notification, or give the impression something else new is in the inbox when there is not anything new, which will be confusing. So I would still like to see koi? on this bug.

Comment 4

[Candice Serran (:cserran)]

Peter Dolanjski deleted the linked story in Pivotal Tracker

Comment 5

[Dylan Oliver [:doliver]]

triage: koi+ to get notifications up to 1.2 spec

Comment 6

[Jason Smith [:jsmith]]

I don't see how we can block on this for 1.2 - we are not support Notification.get in the 1.2 timeframe. It's targeted to land for 1.3. Renom to discuss if there's a workaround to fix this in 1.2 or if this needs to be punted to 1.3.

Comment 7

[Dylan Oliver [:doliver]]

Candice Serran deleted the linked story in Pivotal Tracker

Comment 8

[James Burke [:jrburke]]

Current work being done in this branch:

https://github.com/jrburke/gaia/compare/bug915643-email-clear-notifications

Basics are working, and includes the changes for bug 922722. Still to do:

* Confirm the multiple from name merging from previous notifications is still working.
* Remove the extraneous logs used for dev tracing.
* Write integration test(s).

Ideally the changeset for this bug should be separate from bug 922722, but there was enough refactoring that I wanted to pick up both at the same time, since they can both share the refactoring. However, with bug 922722 blocked on bug 930794, there may come a time when these two bugs should be separated with this bug going first.

This is just a checkpoint state as I am losing coherence due to jetlag.

Comment 10

[James Burke [:jrburke]]

Now that bug 930794 is fixed, we should be able to fix this, and we should also remove the workaround that was done in bug 966147 as part of fixing this bug.

Comment 11

[:gerard-majax]

James, any news?

Comment 12

[James Burke [:jrburke]]

:gerard-majax - definitely in our backlog short list. I want to get to this and bug 922722 before the end of 2.0 stabilization. Currently I have some visual refresh cleanup and some l10n stuff, so it may not be until the next stabilization sprint. However, if I get lucky this first week on bugs, could be next week.

However, if this is blocking you from some other notification work, please let me know, and I'll see if I can push some scheduled bugs for this sprint into next. We may have some other engineers too we can tap. I was going to do it because I had a half of a fix in a branch, but might make more sense to just hand it off.

Comment 13

[James Burke [:jrburke]]

Attachment: GitHub pull request

See pull request for changeset description. Asking :asuth for review since I have overloaded :mcav lately with review requests, and would appreciate :asuth's views on some of the data normalization around names in the notifications.

I also understand if this has a lower priority for review given other 2.0 work. I can flip review off until later if that is preferred. I also added some groundwork for bug 922722 in this pull request, but it does not fix the bug, due to bug 1033933 blocking full resolution of bug 922722.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8450304 [details] [review]
GitHub pull request

r=asuth, some nits.

Comment 16

[Beatriz Rodríguez [:brg]]

James, I can see your patch got r+ some days ago, is it ready for checkin?

Besides, could you please ask for approval in 2.0 branch? This bug was waived in certification and we would like to see if the upload is feasible.
Thanks in advance!

Comment 17

[James Burke [:jrburke]]

There were some nits I wanted to address and had to do a bit of code adjustment due to outbox changes on master. All done now:

Merged to master:
https://github.com/mozilla-b2g/gaia/commit/bf95162abe34eb5775866f5e27fa84782016c54c

from pull request:
https://github.com/mozilla-b2g/gaia/pull/21356

Given the entanglement with the outbox changes, which are only on master, not 2.0, if this was uplifted to 2.0, it would need a custom commit created, it likely will not auto-uplift.

I do not think I am the right person to ask for 2.0 uplift, given that I understand the 2.0 tree to be fairly locked down, only for blocking bugs. If this bug does get approval for the 2.0 branch, I will do a custom commit, so setting NO_UPLIFT in whiteboard to prevent auto-uplift.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

I agree this doesn't seem blocker-worthy, especially since we don't expect this to be (semantically) clean to uplift.  And it seems like the user has sufficient workaround options available to get the notification gone:
- swipe-to-the-right on the notification to clear an individual notification
- 'clear all' to clear all notifications
- just tap on the notification

(I do agree that it would have been nice to have.)

Comment 19

[Beatriz Rodríguez [:brg]]

[Blocking Requested - why for this release]: This is a cert waiver in 1.3.
I understand it is late for asking(sorry) but we need the fix in 2.0. There are several inconsistencies across notifications behaviour. The understanding about when they are removed or not make it a certification waiver.

Comment 20

[Stephany Wilkes]

[Blocking Requested - why for this release]:

All - First, the bad news: I understand that this was waived, but it's too late to land in 2.0. It's too risky to take on all but the most necessary work. The good news is that this has already landed and is fixed for 2.1, so we at least know when we're getting it.

If this really will block 2.0 cert, we can re-nom, but it is fixed for 2.1. Marking for 2.1 in case this should be reopened.

Comment 21

[Beatriz Rodríguez [:brg]]

[Blocking Requested - why for this release]: the lack of consistency about notifications behaviour won't be waived in 2.0. Behaviour was modified from 1.3 to 2.0 and this part of the work will improve the experience and make it more similar across apps. 
It will be a blocker in certification in few months when we start the cert process of 2.0... I think it is worthy to take the risk now instead of several months later.

Comment 22

[Jason Smith [:jsmith]]

Preeti - Do you know what the conclusion is from product if we are taking this bug or not to 2.0?

Comment 24

[Preeti Raghunath(:Preeti)]

Wilfred to follow up on next steps.

Comment 25

[Wilfred Mathanaraj [:WDM]]

still waiting on feedback from tef side

Comment 26

[Edward Chen[:edchen]]

[Environment]
Gaia      3584b2723412ed3299c6761f465885d80651c87e
Gecko     https://hg.mozilla.org/mozilla-central/rev/e7806c9c83f3
BuildID   20140820160201
Version   34.0a1
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014


[Result]
PASS

Comment 27

[Beatriz Rodríguez [:brg]]

[Blocking Requested - why for this release]: read comment 19 and 21. Private email with more info sent to Wilfred.

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Beatriz Rodríguez [:brg] from comment #27)
> [Blocking Requested - why for this release]: read comment 19 and 21. Private
> email with more info sent to Wilfred.

We've read the comments, but it is very very late to take this in 2.0 given this risk and we cannot accommodate that request at this point(iterating this again). Let's take this offline with wilfred.

Comment 29

[bhavana bajaj [:bajaj]]

We had a lot of back-forth and offline discussions with Tef on this and keeping in mind that this was raised as a cert issue in one of the countries we've never launched and could not get a waiver for 2.0 release, we ended up on making this a blocker here :( 

Whenever a 2.0 patch is available lets flag :brg for help on testing and highlight the areas that need to be tested so they can help with testing here.

Comment 30

[Jason Smith [:jsmith]]

(In reply to bhavana bajaj [:bajaj] from comment #29)
> We had a lot of back-forth and offline discussions with Tef on this and
> keeping in mind that this was raised as a cert issue in one of the countries
> we've never launched and could not get a waiver for 2.0 release, we ended up
> on making this a blocker here :( 
> 
> Whenever a 2.0 patch is available lets flag :brg for help on testing and
> highlight the areas that need to be tested so they can help with testing
> here.

Note - make sure we get qa-approval flagged here for the 2.0-specific patch here so that someone looks at this before it gets uplifted.

Comment 31

[James Burke [:jrburke]]

Attachment: v2.0 GitHub pull request

2.0 pull request. I created a zip file that can be installed and tried by QA before I uplift for 2.0:

http://jrburke.com/work/gaia/email-915643-v20.zip

I created it based on the current v2.0 code, so it should be good to test. It can be installed on a device using the process described here:

https://github.com/jrburke/gaia-dev-zip#for-the-ux-person

I tried on 2.0 on the device and it did not explode. Seemed to work fine for me.

Comment 32

[Beatriz Rodríguez [:brg]]

(In reply to James Burke [:jrburke] from comment #31)
> 2.0 pull request. I created a zip file that can be installed and tried by QA
> before I uplift for 2.0:
> 
> http://jrburke.com/work/gaia/email-915643-v20.zip
> 
Thanks a lot James. We had tried it in 2.0 branch(with Flame: Gecko ef26d06 and Gaia ddec117) and it is working fine. Tomorrow we will continue testing and provide more feedback.

Comment 33

[Beatriz Rodríguez [:brg]]

We did more test to the patch and it worked fine in 2.0. Please land it in 2.0 when you get QA-approval+

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Someone on QA needs to properly set the qa-approval+ flag (rather than just putting it in a comment) and remove the NO_UPLIFT from the whiteboard. And James will need to request Gaia v2.0 approval on the patch per the policy change of a few weeks ago.

Comment 35

[James Burke [:jrburke]]

Comment on attachment 8487577 [details] [review]
v2.0 GitHub pull request

[Bug caused by] (feature/regressing bug #):
New feature

[User impact] if declined:
Opening the message_list will not clear notifications that correspond to that message list. This is apparently a cert blocker for a country.

[Testing completed]:
Test on device and by partner.

[Risk to taking this patch] (and alternatives if risky):
Low. It has been on the 2.1 branch for a while, and the code change for 2.0 was to remove the tie-in with the outbox, so just a deletion of the outbox callback section.

[String changes made]:
none

Comment 36

[Tony Chung [:tchung]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #34)
> Someone on QA needs to properly set the qa-approval+ flag (rather than just
> putting it in a comment) and remove the NO_UPLIFT from the whiteboard. And
> James will need to request Gaia v2.0 approval on the patch per the policy
> change of a few weeks ago.

ni? Edchen to handle qa-approval and remove NO_UPLIFT when done.  When uplift is done, please remember to follow up by retesting on 2.1 branch and marking status-b2g-2.1 = verified.

Comment 37

[James Burke [:jrburke]]

Just to clarify: this is an uplift for the 2.0 branch, and only the "v2.0 GitHub pull request" should be uplifted to the 2.0 branch. This change is already in 2.1.

Comment 38

[Tony Chung [:tchung]]

I retested this against 2.0 Open 2 device.   The following scenarios are working as expected with the patch from comment 31:

Environment:
Gaia-Rev        31434a3949556171f3565ca47ac2b44e810e95e6
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/989a723d7e2e
Build ID        20140917000200
Version         32.0
Device Name     ZTE_P821A20
FW-Release      4.3
FW-Incremental  eng.chencong.20140715.162404
FW-Date         Tue Jul 15 16:25:44 CST 2014

REpro
* Received new email notifications in status bar. Launched email app to see unread messages.  Verified notification status bar removed the email notifications
* repeated the above step, but also against lockscreen notifications.   Verified lockscreen is cleared

Please uplift when ready.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/d904bc0edc5bda2c29a2ed5dca3c1244f26136e5

Comment 40

[Juanjo Iglesias]

I have tested using Flame 2.0 (Gecko d87b9af Gaia 0658006) and its working properly.

When the user launches the email app, the new mails notifications, that have been received, are removed from the status bar.

Comment 41

[Beatriz Rodríguez [:brg]]

Bug marked as Verified in branch 2.0.
Thanks for the work done here!

Comment 42

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/d904bc0edc5bda2c29a2ed5dca3c1244f26136e5

Comment 43

[Coler]

Verify passed, this issue can't be repro on Woodduck 2.0, Flame2.1.
Attached: Verify_Woodduck_Email.mp4
Reproducing rate: 0/5
Build Version:
Woodduck 2.0:
Gaia-Rev        ee5cf148b4c546beea9bfb799d2a3ee62074957d
Gecko-Rev       73601b71861cbc2f180c4d2653cec3e9fbb39db5
Build-ID        20141114050313
Version         32.0
Flame 2.1:
Gaia-Rev        569a299ca446f714cd98d5881cc058fd6f6e257b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d188e92aa5a6
Build-ID        20141113001200
Version         34.0

Comment 44

[Coler]

Attachment: Verify_Woodduck_Email.MP4