921050 - [User story] Send email in the background

Status: VERIFIED FIXED

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

Description

[Adam Rogers (:arog)]

User Story:

As a user, I want emails to be sent in the background so I do not need to wait while a message sends. If I exit the email app, the message should still send. This will save me time and allow me to perform other operations while email is sent.

Acceptance Criteria:

1. Once the user taps 'send' they will be returned to the email inbox (or where ever they initiated the compose action from)
2. the email will send in the background
3. while sending an email the email application can be exited
4. the sent email will appear in the sent folder
5. the user should be notified of any send errors

Comment 1

[Gareth Aye [:gaye] (back from PTO)]

Tentatively I'd like to take this to work on today (and perhaps the rest of the week), but I'd like some input from asuth and jrburke. Assigning to myself in the meantime :)

Comment 2

[James Burke [:jrburke]]

As mentioned in IRC, but with some extra details: some things to sort out, likely need UX guidance too:

* Is there an outbox separate from "drafts"?

* How to indicate errors when it fails?

* Do we need to proactively indicate email is sending in background, so user does not think email is sent if they immediately turning off the phone, something like "sending in background..."? Do we need positive indicators when sent, like as :asuth mentioned in the chat: a status bar indicator or even system notifications?

* Activity-based startup of the email app needs a new flow. Right now email blocks until it sends, but if background sending, then app could get closed before we actually send. Does this mean we need to set a new alarm for wakeup to try sending again? How will that work with any periodic sync alarm? Would be nice to coalesce alarm events in that case for example. Maybe activity sending is still synchronous, with option to send to background if it does not complete right away?
 
* Any new back end job setup for sending/batch sending.

Comment 3

[Adam Rogers (:arog)]

These are all great questions jrburke.  Adding in UX for the following:

1) 'outbox' experience - I think we will require an outbox of some sort.  This should probably include the ability to delete remove from the outbox (ie; queued when not online, then the user wants to cancel)
2) errors & notifications - do we need a 'sent' notification for emails sent in the background?  My first impression is that we do

Let me know if we should spin up some additional user stories here.

Comment 5

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

Relevant quotes from the bug about offline mail sending/outbox from bug 834774 that I duped to this bug:
===
- When do we remove a message from the outbox?  When we start sending it since it's now hard to stop it from sending?  Or should we show it in the outbox still while it's sending, but just make it non-cancelable.
- How do we let users cancel sending?
===

Comment 7

[Rob MacDonald [:robmac]]

Clearing flag.

Comment 9

[Juwei Huang[:juwei]]

Attachment: [Email] Outbox_1.1.pdf

Spec attached.

Comment 10

[Marcus Cavanaugh [:mcav]]

Juwei, question on the spec:

In the diagram here: http://mcav.com/temp/outbox-error.png

If there are errors in the e-mail addresses before send (that we can detect), the "Send anyway" choice doesn't make sense -- we already know that the message will fail if we try to send it.

I see a couple options to handle this:

- Remove the "Send" option from that dialog, instead just instruct the user to correct the addresses before we allow them to send

- Instead of the dialog, disable the send button and show the black error bar (like we show when we've sent an e-mail and it failed in the outbox).

Thoughts?

Comment 11

[Juwei Huang[:juwei]]

You're right. It doesn't make sense if the only address is invalid and we still send out the email. The original idea for "Send anyway" is that if the message is going to send to 10 people, some are valid address some are not, I think we should still send out the email because at least some of them will receive it.

So can we do it in two ways:

1. All the addresses are invalid: Grey out the send button. Users cannot send the message.
(i'm pretty sure users will know why they cannot send the mail if they see the red address )

2. At least one address is correct: Send the message and a confirm window to ask user to send it anyway or not.

What do you think about this? Let me know :)

Comment 12

[Marcus Cavanaugh [:mcav]]

I think that we should completely disable the send button if any of the addresses are invalid. An email being sent with only some bad addresses will still fail to send (to all recipients, not just to those who are invalid), and we'd be left in the same spot as before. In other words, it doesn't do the user any good to try to send a message where some of the recipients are syntactically invalid.

So I'd suggest always disabling the send button, and making users correct addresses before sending no matter what. If we think it's not obvious enough to just have the send button disabled, maybe we could also make it more obvious which addresses are invalid.

What do you think?

Comment 13

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/307

Comment 14

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19516

Comment 15

[Marcus Cavanaugh [:mcav]]

Attachment: Mail-fakeservers PR for tests

Comment 16

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8426706 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/307

Flagging you for preliminary feedback. There is still some work to be done, but it's worth having you take a look early so that there's time to address any major issues in the implementation.

Specifically, this feature includes a number of architectural decisions, like where stuff lives in GELAM, how the outbox hooks in to the jobs & syncing processes, etc. I'm happy to move stuff around if you think anything's not ideal.

There are a couple of things that I'm still unhappy with in this patch:

- I'm a little concerned that the outbox isn't noticeable enough, that people would not realize that there are messages pending. I wired up the "refresh" button to send pending outbox messages even if you're viewing the inbox, as that seems like the only way anyone would otherwise ever have their messages try to be sent again without finding the outbox, but it feels sloppy. On the bright side, if someone actually does use periodic sync, the outbox also syncs there too.

- I've patched up all the existing GELAM tests, so we still have comparable sending coverage, but I'd like to have more coverage on both frontend and backend. Specifically, the outbox doesn't have any new backend tests yet. Still wrangling with Marionette to get a better offline test going for the frontend.

I'll keep working and cleaning stuff up in the meantime; feel free to take a light feedback/review pass at your leisure.

Bugzilla Notes:

- The Gaia changes do not include the GELAM changes, and the mail-fakeservers change is only for the marionette tests (to allow the SMTP server to reject a message). 

- Flagging you only on this attachment, rather than all three, because we all get so much bugmail as it is.

Comment 18

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

(In reply to Marcus Cavanaugh [:mcav] <[e-mail]> from comment #16)
> - I'm a little concerned that the outbox isn't noticeable enough, that
> people would not realize that there are messages pending. I wired up the
> "refresh" button to send pending outbox messages even if you're viewing the
> inbox, as that seems like the only way anyone would otherwise ever have
> their messages try to be sent again without finding the outbox, but it feels
> sloppy. On the bright side, if someone actually does use periodic sync, the
> outbox also syncs there too.

I believe it's a desired feature to show the number of new/unread messages in each folder.  While there are still all kinds of server implications for that, we could fast-track outbox and drafts to get the badges in the drawer so it's very bright/obvious that they have stuff in them.  We could maybe even badge the menu hamburger button or have it change color like Chrome/friends do to tell you they really want you to restart the browser.  All UX calls, of course.

But that I think wants to be a follow-up for scoping/sanity reasons.

Comment 20

[Juwei Huang[:juwei]]

I'm sort of convinced by you....I used to thinking about a very edge case like hundreds of email addresses, then it may hard to find the specific email address which is wrong.
But you're right, we should correct every single email address before we send out the message.

Somehow I am not recommend to disable the send button all the time in case users don't know why they can't send the message if the wrong email address is in the middle of all the correct addresses. So if we are going to go this direction, I would let the send button still tappable, and show a confirmation window when user try to send the message.

The confirmation window states:
----------------------------------
Invalid email address

The message could not be 
delivered to one or more recipients. 
Try sending again after fixing it.

   [ O K ]
---------------------------------

And after tapping "OK" button, the screen backs to the compose page.

In terms of outbox discoverability, I think my original proposal was to have a badge beside "Outbox" in the drawer to indicate how many messages are pending right now. It would be look like this in the drawer:

Inbox (2)
Drafts
Local drafts
Outbox (2)
Send
...

It would more obvious if there are any message pending in the outbox. So if there's any consideration in terms of discoverability, badges can be the proper way to solve the issue.


What do you think about them? Let me know :)

Comment 21

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8426706 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/307

I've updated the patches with fairly substantial changes. I left the original commits intact for Gaia and GELAM, with the second commit containing these new changes.

Highlights:

- Addressed your comments from the original feedback request.

- Refactored the backend's outbox job to become a self-scheduling, one-email-per-job operation. See docs both in jobmixins.js and a new helper file `mailapi/jobs/outbox.js` I created to pull out and clear up much of that logic. (Happy to move that wherever, if you think there's a better place.)

- Overhauled message_list.js outbox handling, to be less hacky and take syncing cues from the backend. Notably, it should correctly display refresh icons consistent with the state of the backend, even though outbox ops are not guaranteed to immediately run one after another.

- Minor GELAM test tweaks: I had to tweak a few test assertions and unrelated POP3-induced inaccuracies (mainly dealing with asserting account saves during ops).

- General cleanup and docs.


CI won't show success yet, both because it won't cleanly merge as of now (likely something trivial), and because the Gaia changes do not include the GELAM changes. I didn't rebase yet, as you may want the original commit for reference.

Clearing the feedback flag and asking for review, both to catch your attention and to indicate that I think this is ready for a real review pass. Also, because I'd like to give you as much time as possible to review before you travel next week.

If you'd like to use your fancy ReviewBoard thing, I'm down, just let me know what I need to do for that. (Leaving the gaia/mail-fakeservers flags unset for sanity, but feel free to ?/+/- them if you'd like.)

Comment 22

[Marcus Cavanaugh [:mcav]]

Uh, and, on preview, I missed Juwei's compose comment 20 above. I will push that up momentarily.

Comment 23

[Marcus Cavanaugh [:mcav]]

And, I've re-pushed the patch with Juwei's request included.

Comment 24

[Marcus Cavanaugh [:mcav]]

(For reference, I've been typing "asdf@" as an example of an invalid address that passes `parseMailbox`'s filter but will actually fail to hit the SMTP server, in order to get stuck in the outbox for testing.)

Comment 25

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

Comment on attachment 8426721 [details] [review]
Mail-fakeservers PR for tests

(feel free to land/release the mail-fakeservers changes now/before the other stuff if you are so inclined)

Comment 26

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

Commenting here for GELAM since this may want to be a follow-up bug and would likely get lost in github:

outbox.js' sendMessage treats FooAccount.sendMessage as a conceptually atomic thing.  But for CompositeAccount with ImapAccount there's our under-the-hood call to ImapAccount.saveSentMessage which will schedule a non-persisted appendMessage operation for non-gmail IMAP accounts.  We don't wait for that to complete.  I think it would be ideal if we were able to track the send status so that there's an additional state representing "sent but not yet appended to the server".  Right now our failure mode is that we may fail to append the message.

Comment 27

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

Comment on attachment 8426720 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19516

My review is still ongoing here, but :jrburke, you should do a review pass wearing your front-end owner hat.  Don't feel a need to go deep on the back-end stuff, but there is of course no need to avoid it.  I would be appreciative if you could do more device testing assuming I'm going to skimp on that a bit because I'm busy-ish with this bootcamp thing.

No super rush on this, I won't be able to finish the front-end review until very late tonight, but it's also possible it could be very late tomorrow.

Comment 28

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8426720 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19516

I've updated Gaia, GELAM, and mail-fakeservers with the changes as discussed on the various GitHub threads. I'm pretty sure I addressed all the comments so far, but do let me know if I've missed something or if you'd like further changes.

Again, you'll need to push the GELAM changes into Gaia, as they are not included in the Gaia PR. Also, mail-fakeservers if you're going to run the GELAM tests.

I rebased Gaia and tested locally with both an IMAP and ActiveSync account; to my eyes everything looks and works as it should, apart from the playSoundOnSend pref being missing on Accounts (which, with this Gaia, should work when :jrburke merges his GELAM branch for that). I tested sending valid messages, sending messages to invalid addresses, and, through the fortune of Hotmail being angry at me for not verifying my account, seeing an external transient failure->resolution allow all the messages to get sent from the outbox. So that was nice.

On the backend, I had a pretty rough time trying to get the new GELAM outbox tests to pass, largely due to trying to pin down expectations that the outbox test doesn't really care about (like connection state, etc). While I believe it properly covers the cases :asuth requested, it was done in a rush, so it's not as clean or pristine as would be ideal. I don't think that will matter in practice; it could be revisited in the future when we have extra time (either if we miss the 2.0 train or decide to clean it up in the stabilization period), if we decide it works for now.

Working on those tests today really brought to light how nice it would be to get in some of that GELAM-test API refactoring we've talked about elsewhere.

Unrelated, but the new visual design really does look nice, good work James.

Comment 29

[James Burke [:jrburke]]

Comment on attachment 8426720 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19516

Really neat to see this coming together! I like it more than the existing behavior. I finished my current review pass, but would like to do another, likely shorter pass on diffs from this point and some other follow on on-device experiments. So feel free to flip a front end review back to me when ready.

Comment 30

[Marcus Cavanaugh [:mcav]]

I've pushed the changes from your previous round of reviews except for one thing: I'm spinning my wheels on combining the test_outbox.js "fail send, shutdown the universe, then start a new one and stuff should send" two tests into one. I'd like to take you up on your offer to help, whether that's you taking a look independently or us getting together to work through it.

I've pushed a branch with my current attempt[1], but you may or may not find that helpful to look at. I'm (ashamedly) at the point where I'm just flipping flags around rather than making any real progress understanding what's going on. 

Since this is for 2.1, there's no rush to land (other than bitrot); we could alternately just hold off until the workweek and go over it in person if you'd rather.

[1] https://github.com/mcav/gaia-email-libs-and-more/commit/a7272e3a6531959d44f876c76b7a92958467f48c

Comment 31

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

Bit-rot is the mind-killer; I should be able to help with the test this week.

Comment 32

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

I landed the mail-fakeservers stuff after addressing the test issue.  I rebased it and bumped the rev, so it ended up as a different pull request.

landed on mozilla-b2g/master:
https://github.com/mozilla-b2g/mail-fakeservers/pull/17
https://github.com/mozilla-b2g/mail-fakeservers/commit/212d0d505b799c90756bfe80d7cd240eb415e71e

$ npm publish
npm http PUT https://registry.npmjs.org/mail-fakeservers
npm http 201 https://registry.npmjs.org/mail-fakeservers
+ mail-fakeservers@0.0.20


I've made the outbox tests happy.  This had some cascading cleanup effects.  I ended up doing the following:

- Using expect_runOp in more places within the outbox and compose tests.  In various places we had skimped on these and just used lazy loggers.  The lazy loggers weren't enough to bound all of the operations in a step so that they happened in the step.  As a result they were smearing into other steps, resulting in intermittent failures or just breaking the outbox tests.  (Additionally, in some cases the steps weren't actually doing what we thought they were doing.)

- Making us wait for database saves to complete before declaring operations complete or sync complete.  Accordingly, saving now has async begin/end logs instead of just an event when the save starts.  This was needed to correct persistent-related tests where we tore down the universe before the save actually managed to happen, resulting in our reborn universe missing important data or still having data that should no longer exist.  (For the outbox tests, the first test ended up leaving its sent message in the outbox, so the second test got confused and tried to send it again.  I believe; there was some other confusion around that test later on so I may have been confused at the time.)  This corrects bug 987931 which I now believe may be involved in our current cronsync mega-sadness.  This is a particularly regrettable thing to have rolled into this patch, but there are non-trivial cascade effects and I already had a lot of other test cleanups under way.  I'm just going to have to uplift this separately for 2.0 I fear.

- We now describe why we're saving state a bit more, we also explicitly expect on this some.

- We collect call-stacks when we generate expectations.  The idea is that if you're looking at the log and it says "unexpected event FOO" and you're like "gah! there's 40 call-sites that could be doing that!" you can now just see the "stack" thing and click on it and be able to track down the exact source of the expectation.  (Although we're not doing any long-stack cleverness related to parameterized test steps, although we could.)  This requires pulling down a change to ArbPL to see this, but it should be harmless-if-confusing if you forget to do this.

- Some POP3/testing cleanup.  In general, this resulted in a migration of hacks into th_main and a reduction in custom 'expect' functions being triggered in the tests.
 - A bunch of ignore_* calls were removed...
 - Connection management was "cleaned up" in that it's pretty clear when POP3 wants a connection, so we just generate those.  This seems to work okay.
 - We don't generate 'sync' log entries in cases where we're not actually synchronizing.
 - We don't save when it makes no sense

- I split the outbox tests into 3 separate test files and one shared file so we could avoid any bogus database sharing concerns or having to worry about the restored state too much.  The bad news is git does not seem to have viewed this as a copy with modifications, so the diffs suggest the files are entirely new.

The commit on top of your current pull request (which needs some rebasing, regrettably) is https://github.com/asutherland/gaia-email-libs-and-more/commit/59f90727f91244d78a5cf18b631f988fdffca722.  It lives inside the pull request at https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/319 that I created because I wanted travis to run the tests, but travis doesn't seem to want to run them, I guess because an automatic merge isn't possible.

:mcav, if you can verify that:
- I fixed the outbox tests
- all other tests are green
- the wait-for-save logic looks sane
and then rebase your stuff, then I think we can land this stuff, although I should probably do a run on my Flame device tomorrow to sanity check.

Note: I would request that you not squash my test changes in with your stuff because I think the history will be valuable at least to me if it turns out I screwed any of this stuff up.  I have no strong opinion about whether you squash your commits under that; I'm fine with keeping them if the history helps you.

Comment 33

[Marcus Cavanaugh [:mcav]]

Thanks so much for cleaning all that up.

With your explanations here, the changes you've made seem coherent and correct.

I've now done the following:

- One small typo fix
- Rebased (both the Gaia and GELAM patches) onto master
- GELAM tests pass locally for me, with one intermittent failure in test_activesync_recreate (probably unrelated; unexpected accountSave operation); the relevant Travis run also has a couple seemingly unrelated intermittents, which I'm rerunning now.

I've pushed these GELAM changes into Gaia and have been testing the flows on my Flame. So far it works as expected. I'm monitoring the relevant CI results for both patches currently.

https://github.com/mozilla-b2g/gaia/pull/19516

https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/320

If the tests pass, do we have r+ to land? James hasn't formally verified the Gaia changes (though he did go through a review round earlier, and the code is the same), but given the relatedness to the blocker stuff, his vacation, and our interest in landing, would he be all right with landing before he gets back? I'm not familiar with the procedural or blood-pact kind of thing you two have set up as owners of the email app.

Comment 34

[Marcus Cavanaugh [:mcav]]

After IRC discussion with :asuth, it's time to land this thing.

gaia master: https://github.com/mozilla-b2g/gaia/commit/8fa862440bdbe128c2cd34bf65e7fce7b0c4f5a6

gelam master: https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/63ab3efc11cb38743bb8896483dd28797fbae9cd

with a green Gaia-Try: https://tbpl.mozilla.org/?rev=830f2f461515cbe80f4560e2fd5dea05df39b603&tree=Gaia-Try

Comment 35

[Francesco Lodolo [:flod]]

background-send-sending=Sending mail…
background-send-error-title=Mail failed to send

For consistency with the other strings these should be "email", not "mail" (no need to change IDs for fixing them).

In general, the file is not very consistent: mail, e-mail, email are all used.

Comment 36

[Jordan de Geus(JordanD)]

- When an email is pending/inprocess of sending, upon returning to the Homescreen users will eventually hear an audible trigger their message has been sent successful.
 
- If users do not have a connection, the user will see a error message within the notification menu stating "Network unavailable. Email saved to Outbox". This will occur both in the Email app as well as when the user is on the Homescreen. If the user is on the Homescreen, the notification will display within the Notification menu.

- When users have a email pending/in-process of sending, upon viewing the Outbox, they will see the email remains within the Outbox menu until the message has been sent. Once the message has been sent successfully, users will be able to observe the message moves to Sent Mail box.

- When users attempt to send an email to an invalid email address such as "hello@" users will see a popup message stating "Invalid Address - Your message contains one more more invalid email address. Please correct them before sending."

--------------------------------------------------------------------------------------

Based on Comment 18 I am unable to verify this issue. The menu within Email should display new/unread messages 

This issue has been written Bug 1063196

Comment 37

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue cannot be verified as fixed because:

bug 871897 is a backend fix that we cannot verify.

bug 949270 and bug 949498 cannot be verified as fixed because email is not being sent out in the correct order out of the outbox. Please see bug 1058443

bug 949502 cannot be verified as fixed because the wrong error icons are being shown for messages in the outbox. Please see bug 1064509

Comment 38

[Jason Smith [:jsmith]]

Both issues found during verification aren't blockers, so let's finish off the verification here taking into account that those two issues won't block signoff.

Comment 39

[KTucker [:KTucker][Inactive 3/4/2016]]

Verifying this issue as fixed on the Flame 2.1(319mb) and the Flame 2.2(319mb)

Flame 2.2

Environmental Variables:
Device: Flame Master (319mb)
Build ID: 20140908040204
Gaia: c71fd5d8c9c7cb021c97e5e9fbb29f92b50a084d
Gecko: 892768985915
Version: 35.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1

Environmental Variables:
Device: Flame 2.1(319mb)
Build ID: 20140908000204
Gaia: a8e4d26555e5713ec6c72270cfd0cfabc096a0d3
Gecko: 746f24f9d21d
Version: 34.0a2
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Once the user taps 'send' they will be returned to the email inbox (or where ever they initiated the compose action from). 

The email does send in the background so the user can perform other tasks.

The user can exit the email app while an email is being sent. 

Sent emails appear in the sent folder.

The user is notified when an error is encountered while sending an email.