892519 - [User Story] Email Notifications - Email App Not Foreground

Status: VERIFIED FIXED

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

Description

[Peter Dolanjski [:pdol]]

User Story:

As a user, I want to be notified when a new email arrives in my inbox, even when the email application is not currently in the foreground so that I do not need to open the app or manually refresh to check if an email has arrived. 


Acceptance Criteria:

1. If I am not currently viewing the Email app and a new email is detected to have arrived in my mailbox, I am made aware that I have received an email (even if the email app is not open)
2. If my device's screen is off, and a new email is detected to have arrived in my mailbox, I am made aware that I have received an email
3. If my device is locked, and a new email is detected to have arrived in my mailbox, I am made aware that I have received an email

Comment 1

[Dylan Oliver [:doliver]]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/53237791

Comment 3

[James Burke [:jrburke]]

Attachment: Pointer to pull request https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/232

Preliminary GELAM pull request. While this code works and has had some end-to-end testing on a phone, there is a high likelihood it will need some changes before it is merge-worthy. Most importantly, it is lacking tests. Doing a pull request now though to work through any higher level design issues first.

Terms:

* BE: back end: usually means GELAM itself, including the main-frame-setup.js and code under that, the worker code.
* FE: front end: the code in gaia for showing the email UI and front end logic.

### General notes

The general approach: use cronsync as an instance property of mailuniverse to manage syncing. cronsyn-main is outside the worker, so it can set alarms and receive notifications of triggered alarms.

mailuniverse is the facade in front of the cron work, with it handling the triggering of "cron start" and "cron stop" events that go through the mailbridge and mailapi that are then visible to the FE. The "cron stop" event will have any data about new messsages. Right now that data is: for each account that has new messages, how many new and then

There are debug statements that should probably be removed or at least
switched off.

### evt.js

evt.js from the FE was added to the BE, and used by cronsync-main.js to send out the list of wake locks. When copying the files into gaia, the evt.js in GELAM is not copied over, the FE copy is used.

This was done because acquiring the wake locks need to happen during the alarm event loop tick as further ones may be shut down by the system, but the release of the locks need to occur after the FE has figured out how to notify. So a evt.js is used as a pub/sub method in this case with the receiver not being known to GELAM (but wired up by the gaia FE).

### New account properties

**syncInterval**: defaults to 0 for upgrades, meaning no syncing.

**notifyOnNew**: defaults to false for upgrades (if new account is created by FE
after this patch lands, FE defaults to true)

### cron changes

Updates cronslice to latest slice expectations, and pulls it out as a separate file from cronsync.

It also uses $sync.INITIAL_FILL_SIZE for desiredHeaders, felt it was better to use an existing constant than one that may not match if that constant it is related to changes later.

cronsync talks to cronsync-main over postMessage via $router.

Syncing tries to group account IDs that are on the same interval together, so all the cron-related methods accept an array of account IDs.

cronsync is driven by two mechanisms:

* **mailuniverse**: on account startup, if there is a syncInterval on the account, then ensureSync will be called to make sure alarm is still set and set correctly for the account. This should help in cases where the mail app gets killed or dies at inopportune times before the next sync alarm is set.

* as mozAlarms fire, **cronsync-main** listens for them, and tells cronsync about it.

### mailapi/malapi

* passes through the new sync-related account variables.
* Removes modifyConfig, as it only seemed to be used for syncInterval when that value was global. This may have been too aggressive.
* exposes an oncronsyncstart and oncronsyncstop that the FE can use to get notifications of when cronsyncing starts and stops.

### mailuniverse

* makeBridgeFn: just encapsulates some boilerplate for setting up some pass-through messaging functions.
* Renamed _cronSyncer to _cronSync to better match module name. I wonder if this should go further and perhaps cronsync should be a singleton as the cronsync-main.js sends messages in a way that it assumes a static entity receiving them.
* Calls _cronSync.ensureSync if syncInterval is in play, otherwise, it does the old syncFolderList call.
* Bridges the notifications coming out of _cronSync with what is sent over the mailbridge, as the "cronstop" state needs to wait for a final DB account save (usually for the snippets) before sending out the cronstop event. Without this, the FE was shutting down the app before final DB write was done.

Comment 4

[James Burke [:jrburke]]

Attachment: Pointer to pull request: https://github.com/mozilla-b2g/gaia/pull/11499

Preliminary Gaia pull request. While this code works and has had some end-to-end testing on a phone, there is a high likelihood it will need some changes before it is merge-worthy. Most importantly, it is lacking tests. Doing a pull request now though to work through any higher level design issues first.

Terms:

* BE: back end: usually means GELAM itself, including the main-frame-setup.js and code under that, the worker code.
* FE: front end: the code in gaia for showing the email UI and front end logic.


### General notes

Existing accounts are treated to be "manual" sync (syncInterval of 0) and not notified of new mail. Any new account created with this branch of the code will be set to "notify of new mail", but sync is set to "manual" by default.

There are some UI controls in the settings for an account to turn on syncing. However, this UI is **just temporary**, with the real UI coming in [Bug 892518](https://bugzilla.mozilla.org/show_bug.cgi?id=892518). The temporary hacky UI also has to "DEV" settings at the bottom, for 20 secs and 60 secs to make testing some things easier. **Be sure to set it back to manual** if you use one of those settings, to save the forests and perhaps bad consequences from your email provider from hammering them with too many syncs.

Right now notifications are only text based, and do not allow any formatting. If the sync results in 1 or 2 emails, then a notification for each email is generated showing the subject and the author. If more than two, then there is just one notification that tells the number of new messages, and then a comma separated list of unique authors.

That layout seemed to work well: there was not much room for much else.

There is no revoking or removing of notifications as that is not available in the platform at the moment.

Clicking on the notifications should go somewhere useful: if multi-email notification, to the message list for that email account. If a single email notification, then jumping to that message.

If the email app is visible when the new message comes in:

* If in the account for the message, no notification.
* If for an account that is not the current one being shown in the app, a system notification is sent.

All alarm and notification work goes through the index.html file. This is partly because right now alarms can only be fired [for the same page that sets them]( https://bugzilla.mozilla.org/show_bug.cgi?id=800431). Once that alarm bug is fixed and SharedWorkers are available, a more efficient approach may be possible.

Still to do:

* Add tests.
* There are new l10n string work to wire up.
* See if sync time can be reduced. It takes about 10 seconds right now from start to finish on a wifi network for an account that is already synced.

Related GELAM changeset pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/232

Notes on the implementation:

### app_messages

It now listens for 'notification' messages, and converts the data passed via the iconURL into a data structure that is then passed to app code.

### app_self

module for wrapping the mozApps.getSelf() call, as it is needed to get an iconURL for the notification. It uses the evt so that other code can use the "latest" callback structure to get the mozApps value.

### account_picker, folder_picker

They no longer look for related cards and prod them to update to new account/folder values, but instead, the other cards now just listen to the `model` module for changes.

### message_list

Listens to model now for changes instead of waiting for a direct call from folder_picker.

### settings_account.js/.html

Has the hacky UI for the syncInterval and notifyOnNew setting. That UI was stolen from the hidden portions that were in settings_main.html.

### cards/setup_progress.js

Sets some defaults for the new account values.

### evt.js

Had a bug around "once" notifications. New unit test confirms the fix.

### html_cache_restore.js

Checks for alarm and notification pending messages as it did before for activities, but now sets an object specifying what was detected. This is done since calling mozHasPendingMessage only seems to work once per type.

### mail_app.js

Loads the new modules that handle the sync events and wake locks, then registers for 'notification' messages to know how to start up the UI.

### model.js

Refactored a bit to allow setting account and folder more granularly. Used mostly for the notification listening in mail_app.js to switch to the correct account/folder to use for the UI.

### query_string.js

Handle converting an object to a querystring value to add to iconURL, and then to hydrate that querystring back into an object.

### sync.js

Handles the "cron start" and "cron stop" messages from the back end. Determines how to notify the user, and then emits a "cronSyncStop" that is used by the wake_locks module to know when to release the wake locks. sync.js also will shut down the app if it has not been made visible during the time it was started.

### wake_locks.js

Handles the 'conrSyncWakeLocks' event that is broadacast from the BE, and then listens for 'cronSyncStop' from sync.js to know when to release the locks. Also sets up a timer for 45 seconds as a max time for the locks, to prevent a bug from keeping the wake locks operational forever. This means syncs neeed to finish within 45 seconds or risk going into sleep state.

Comment 5

[James Burke [:jrburke]]

Attachment: bug892519-notify.zip

Zip of in-progress branch. Can be used for early UX and product verification of functionality. To install it, see directions here:
https://github.com/jrburke/gaia-dev-zip#for-the-ux-person

Comment 6

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

Preliminary pass.  My focus was on larger things, but everything generally looked very good, so instead you just get a bunch of nits or things to keep in mind :)

- sync/cronslice: I propose we eliminate cronslice from existence.  Instead we use a real MailSlice but hand it a no-op-y SliceBridgeProxy-alike or give it a SlideBridgeProxy that has a dummy bridge with a no-op sendMessage.  My rationale here is that MailSlice has a reasonably big surface area to imitate and we aren't really gaining any performance or memory usage benefits from having a no-opy slice implementation at this time.

I'm not crazy about adding onNewHeader to the mix; I would prefer that cronsync just hook up to _onAddingHeader directly.  However, that would leave the messy issue of how to get the FolderStorage talking to the cronsyncer which the slice thing addresses.  I guess just keep an eye on if there's a way to clean this up in the next user story where we overhaul the in-app display too.  I think the right thing to do here will fall out when we end up implementing some type of derived data source like Thunderbird's virtual folders that would also want to listen for very notable database changes.  (Note that our virtual-folders-esque implementation would not work this way!)

- docs: GAIA/js/model.js really wants an overview doc-block that either explains how it is in charge of a lot of the UI state or explicitly calls out a markdown file that explains what it gets up to.  Since we're definitely going with yuidoc for reals, the events that it emits should probably also be defined using the @event syntax yuidoc supports: http://yui.github.io/yuidoc/syntax/index.html

- upgrades: GAIA/js/cards/settings_account.js and any other UI should not be making upgrade decisions.  All migration logic should either happen in the hard upgrade configurator's recreateAccount() method or an explicit soft-upgrade mechanism.  (The simplest soft-upgrade is probably to have the accountDef store a version number in the accountDef that the account constructor sees and then applies simple fix-ups before it boosts the version number.)  For the sake of avoiding scope-creep, we can bump our database version number since we're going to end up bumping it during the 1.2 cycle again anyways.

- l10n dev nit: Please make sure all hard-coded strings in our HTML or JS look stupid by doing the "FirsT AnD LasT LetterS ArE CapitalizeD" thing.  The TODO's are good, but they can be overlooked.  (I realize in the HTML case you transplanted the enumeration and this user story was not about that UI at all.)

- on modifyConfig: I wouldn't have removed it, but you don't need to put it back in since it is indeed unused right now and in general we are going to bias towards per-account settings in all cases.

- nit: don't link to pivotal tracker; only bugzilla bugs are canonical

- wake locks: I need to think more about how to integrate the streaming wakelock stuff with your impl.  I definitely like that you added a failsafe timer!

Comment 7

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

Comment on attachment 789255 [details] [review]
Pointer to pull request: https://github.com/mozilla-b2g/gaia/pull/11499

Clearing review because I did pretty thorough passes over both on the github pull requests, and combined with my previous comments, I think this covers most architectural and nit stuff enough to keep :jrburke busy for a while.  Feel free to request specific feedback about certain things directly, otherwise I'd like to do my next review pass after we've got some tests and the bulk of my requests have been addressed (by being implemented or counter-proposals/etc.)

I actually gave more feedback than I was expecting; I think it's probably worth roping in :lightsofapollo explicitly to help out with writing some of the more black-box style integration tests that are likely appropriate for time reasons.

Comment 9

[James Lal [:lightsofapollo]]

Hey guys- Porting over the fakeimap/stmp server into a standalone package and helping out on tests here is my top priority for this week... Once we have the servers I think the rest is trivial.

Comment 10

[James Burke [:jrburke]]

Attachment: Zip of email app for ux review

Updated app zip that includes UX changes for bug 892518 (UI to change sync options) and bug 892523 (clicking on notifications to launch app). Asking for UX review for those bugs as well as this one. Product can also review.

While this bug in particular has minimal UX/Product feedback (just does background syncing), the other bugs mentioned above do have more visible feedback. I will put notes in those other bugs about any deviations from the specs, and will flag them also for UX review, but will point to this zip in those other bugs.

Installing the zip can be done using this process:
https://github.com/jrburke/gaia-dev-zip#for-the-ux-person

Dev review is still ongoing and still may have some tweaks, but the larger behaviors that are supported are now visible with the current state of the dev branch. Any further dev review will be around code organization, and should not impact items that would affect UX/Product review.

Comment 11

[James Burke [:jrburke]]

The GELAM pull request has been updated and rebased:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/232

I started just keeping the incremental review feedback commits separate, but by the end of the changes, it was different enough that the incremental changes would have been more confusing.

The main differences from the previous version of the pull request:

* accountmixins for saveAccountState and runAfterSaves.
* The database version was revved to 22, to make sure upgrade paths are taken to seed the defaults for the new sync-related account settings.
* No more cronslice, just a regular slice is used by cronsync, and it just does some customizations of the slice for cronsync's uses.
* SliceBridgeProxy was separated out as a separate file so that cronsync could use it for its slice construction.
* The alarm setting has been consolidated into once ensureSync call, which does the alarm checking for all accounts that have a sync enabled. It also checks the interval times to make sure they are still valid.
* mailuniverse no longer has the extra logic to hold oncronsyncstart and end events based on save events, but rather, each account type now has a runAfterSaves that is called.... Imminent thing
* mailuniverse now has a mode of 'interactive' or 'cron', and if not 'interactive', the deferred operations are deferred longer.
* No more wifi lock, bug 907028 was filed to track any changes around using a wifi lock.

Waiting on asking for a formal review until there are tests, but given the size of changes, if people want to review before then, it may help the review-change feedback cycle. While I believe the code is in a better place than the previous pull request, state, I understand there could be another round of changes.

Travis is reporting a failed test case, but when I run locally, that test passes for me. Oddly enough, I get a failure in test_imap_bad_servers, but that one passes in travis.

Comment 12

[James Burke [:jrburke]]

The Gaia pull request has been updated and rebased:
https://github.com/mozilla-b2g/gaia/pull/11499

I started just keeping the incremental review feedback commits separate, but by the end of the changes, it was different enough that the incremental changes would have been more confusing.

See the GELAM pull request for the description of updates in the email/js/ext directory. For the other files, the main differences from the previous version of the pull request are:

* More thorough use of `model` for tracking model changes across cards. `model` was recently introduced in bug 892069, but the use of it was not wired into many of the main cards completely. This lead to a few bugs, a notable one was a consistent 1969 bug when switching from an ActiveSync to IMAP account, so those should be fixed now.
* Additionally, some cards now use evt to publish a UI action that needs to occur, and mail_app listens for those and then does the work to update the card structures. This is in line with the general pub/sub refactor story we want to move to longer term. It also means it is clearer to track in the main controller, mail_app, what is resetting the model and the card states.
* It includes the UI changes tracked for "ability to change sync interval", bug 892518. There is an account_prefs_mixins module that is used by settings_account, and there is a new card, setup_account_prefs, that is shown after setup_progress, that allows the user to change the sync options before completing account setup.
* Part of the changes for bug 892518 were showing of the last sync time in the account_picker card.
* A date module to hold date-related functions. Longer term, the common date functions in mail_common should be moved to that module. That refactor was avoided to reduce the already considerable amount of changes in this pull request.
* wake_locks was changed to track wake locks for each "account key", the set of account IDs that are tied to a particular sync interval. This should allow overlapping syncs of different accounts without the first one finishing and releasing all the locks.
* The 20 second and 60 second "DEV" sync intervals have been removed. However, they can be restored by using the secret debug mode (https://wiki.mozilla.org/Gaia/Email/SecretDebugMode). Additionally, if the sync interval from the BE does not match an existing preset, it is added to the options list.
* Localization wiring.

Waiting on asking for a formal review until there are more tests, but given the size of changes, if people want to review before then, it may help the review-change feedback cycle. While I believe the code is in a better place than the previous pull request, state, I understand there could be another round of changes.

Comment 13

[James Burke [:jrburke]]

Comment on attachment 794115 [details]
Zip of email app for ux review

Uploading latest version that matches latest code state.

Comment 14

[James Burke [:jrburke]]

Attachment: Zip of email app for ux review

Latest app zip. See Comment 10 for details:
https://bugzilla.mozilla.org/show_bug.cgi?id=892519#c10

UX should review this version of the app, as it has a couple of bug fixes.

Comment 15

[James Burke [:jrburke]]

Comment on attachment 789255 [details] [review]
Pointer to pull request: https://github.com/mozilla-b2g/gaia/pull/11499

Some unit tests are now in the changeset. Still going to add integration tests, but they will be validating what has already been manually tested with this code, so asking for technical review now on the code.

Comment 16

[Stephany Wilkes]

Flagging for UX review (per comment 14) by Rob or Jacqueline as this involves email notifications.

Comment 19

[Tony Chung [:tchung]]

cc'ing QA to watch for this bug to land.

Comment 20

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

Comment on attachment 789255 [details] [review]
Pointer to pull request: https://github.com/mozilla-b2g/gaia/pull/11499

Wooooooooo! r=asuth

Comment 21

[James Burke [:jrburke]]

Committed to Gaia master:
https://github.com/mozilla-b2g/gaia/commit/2d240c619e54801dd5a4290adb14b5529133ef36

Pull request:
https://github.com/mozilla-b2g/gaia/pull/11499

Committed to GELAM:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/25ba2035b2100c5e8da28265f9e350bc2f1304b2

Pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/232

Comment 22

[Tony Chung [:tchung]]

Verified working on master and aurora branch, 9/22 build