922722 - [email] notifications count will only list most recent sync count

Status: RESOLVED FIXED

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

Description

[James Burke [:jrburke]]

Right now, the group notification for email, "N New Emails", will only show the count for the latest sync. So if there are the following syncs before the user opens email or looks at notifications:

* 3 new messages fetched
* 5 new messages fetched
* 2 new messages fetched

The notification will only say "2 New Emails", instead of "10 New Emails".

The ideal fix is to use Notification.get(), tracked in bug 899574, so the code could get the latest notification to correctly do the count.

Another possibility may be for the email app to track the count internally, but this would require setting a new storage of the info, and could be inaccurate if the user clicks "Clear All" in the system notification tray, which would mean the user saw the count and was basically asking for a reset.

So will prefer to depend on bug 899574 for now.

Comment 1

[James Burke [:jrburke]]

Asking for koi+, as it is similar to the notifications cleanup being done in bug 915643.

Comment 2

[James Burke [:jrburke]]

I suppose a workaround would be to rely on notification close events to keep solution #2, in-app email storage of notification info, as a way to sync up with changes to the notification tray display, but it feels hacky, and it would mean not asking for bug 921678 to be fixed, which has its own undesired side effects.

So still prefer the Notification.get() approach for most efficient long term solution.

Comment 3

[Jason Smith [:jsmith]]

FYI to the Productivity triage - notification.get() is being implemented for 1.3, not 1.2. So if you are to block on this, you'll need a way to fix this without relying on notification.get.

Comment 4

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

(In reply to Jason Smith [:jsmith] from comment #3)
> FYI to the Productivity triage - notification.get() is being implemented for
> 1.3, not 1.2. So if you are to block on this, you'll need a way to fix this
> without relying on notification.get.

Ugh.  For simplicity I suggest we just use another cookie, then.  Reset the cookie on UI load, and if we're feeling nice, for 'onclose'?

Comment 6

[James Burke [:jrburke]]

Due to bug 930794, this will not result in correct counts even if we do the email fix. So wait for that bug to be fixed before fixing this one.

Comment 7

[James Burke [:jrburke]]

Currently bug 915643 has a branch with changes to fix this bug once bug 930794 lands. However, depending on that time window, the branch for bug 915643 may need to separate the work for the two bugs. Just be sure to check that bug before starting any new branch for this bug.

Comment 8

[Olof Wickström]

This must be a degrade. This is a serious bug related to user's feel of security and trust in the device. The number of unread messages may not change in this way. Please fix

Comment 9

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

(Leaving the 2.1 nom issue to product management and just dealing with the implementation now)

I think the mechanism for addressing this feature would now be to track "newCount" on the folder's folderMeta like we do for unreadCount.  There are ways we could implicitly reset the newCount back to zero, but I'd favor an explicit API.  We've have the message_list card trigger in when showing a folder.  It could even trigger the new message count overlay in the future if UX wanted.

Comment 10

[James Burke [:jrburke]]

That change seems like a larger one to do for 2.1. We would also have some complication around the "single message notification jump straight to message reader" case. We could probably sort out the logic for it, but just adds more complication.

We already have the seeds to do this correctly in the email code (in sync.js), but we have it commented out because we do not get a consistent world view from notification events, bug 1033933.

Since other apps would benefit from getting correct notification events, I would rather see the underlying system notification fixes get done to get this issue solved than to add more hoops for us to track it internally.

Comment 11

[Dylan Oliver [:doliver]]

Based on comment #10 and the dependency chain for this bug, this fix does not seem feasible for 2.1 at this point. 

Although the notification only shows the number of new emails that were retrieved in the most recent sync, the user will be able see the correct number of total unread messages in the folder badge when they enter the email application. There is an inconsistency here but this is not a regression and so does not meet the criteria for a 2.1 blocker at this time.

Bumping this forward to take on for 2.2.

Comment 12

[Francis Lee [:frlee]]

hi Joe,

could you please check if this is feasible for 2.2 scope planning?

Comment 13

[Dylan Oliver [:doliver]]

Not blocking, this is dependent on a couple of other bugs before we can fix but we need to check on the priority with those teams.

Comment 14

[James Burke [:jrburke]]

Attachment: GitHub pull request

There was already commented out code for this in the current master, but I expanded the change to switch to using Notification.data to store the data for the notification. This removed the goofy \n work around the names in the notification. Hooray!

As for an upgrade plan, if the user installs this 2.2 email app when a notification for a pre-2.2 email app exists, and if they then tap on it: in this case the data object will not have any data in it (the data would have been in the iconURL fragment ID). I opted for just ignoring the data and going to the message list.

Mechanics of the upgrade path: mail_app was changed to default to message_list if no data.type. If no account, then uses currently viewed or default account for email app.

I tested the upgrade path by temporarily manually modifying app_messages onNotification to just set data to an empty object, and the notification tap worked as expected, going to the message list.

The biggest effect of this upgrade plan is on outbox error notifications -- the user is not taken to the outbox in that case. However, if the user is not able to manually navigate to the outbox to fix, the next time a sync happens, the user will get a notification that will go to the correct place, it is not an unrecoverable situation.

This upgrade plan was chosen since notifications are very ephemeral. The multiple email sync would go to message_list anyway, the single email notification tap will go to the right message_list in the majority case so the user will see the new email. Outbox may require a bit more manual work or at least waiting for next sync to get a direct link, but given the small likelihood of overlap on time of upgrade of email and an outbox failure, seemed like a reasonable tradeoff to avoid bloating the code long term for these small edge cases.

I manually ran the email integration tests and they all still pass as expected with this change. I did manual testing of multiple single email syncs, to confirm the correct count and email name grouping/deduping, and single email sync test, and a sync of multiple emails.

Comment 15

[James Burke [:jrburke]]

I should have noted, this bug can be worked on because bug 1033933 seems to be fixed by other changes on master. However, I do not recommend trying to uplift to other branches unless the test case in bug 1033933 also works in the target uplift branch.

Comment 16

[James Burke [:jrburke]]

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/487cc491e75fda6e368bc8b6a8bc9400ee0a66c8

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