828294 - [Email] Email app does not warn that attachments will not be forwarded when forwarding a message

Status: RESOLVED FIXED

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

Description

[isabelrios]

Unagi: Gecko-29a48d1.Gaia-7c348f4

PROCEDURE
1. Set up an email account. For the test an gmail account was used (gmail.com)
2. Receive an email with an attachment
3 [review]. Go to the option to forward it and add a new email as a recipient
4. Verify whether the email is received correctly with the attachment

EXPECTED
When forwarding an email with an attachment, it should be correctly sent

ACTUAL
The email is forwarded ok but not the attachment. The attachment used was less than 300Kb

Comment 2

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

One major issue with forwarding and attachments is that the attachment may not be downloaded when the user chooses to forward the message.  Based on our current compose capabilities, we would need to download the message first, then re-upload it.  Download semantics could be complicated by inconsistencies relating to attachment storage.  (We can only store images/videos/music in devicestorage; we currently only store attachments there.  But we can store anything in our IndexedDB storage.)

If we extended our compose capabilities, then on certain server configurations we would be able to avoid downloading/uploading by having the server do much of the composition for us.  To my knowledge, this is only available on "real" IMAP servers supporting the LEMONADE suite of CATENATE/URLAUTH/URL-PARTIAL, accompanied by a BURL-supporting SMTP server.  Versions of dovecot, cyrus (notably used by fastmail.fm), and zimbra (as used by mozilla.com) support this.  Neither gmail nor yahoo support those extensions.  I don't see any ActiveSync support in SendMail/SmartReply/SmartForward, although it does seem ridiculous to me, so hopefully I'm just missing something.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Meh.  I assumed this was the case that this was a limitation due to the scope of not having attachments.  Same thing for reply too, right?

Comment 4

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

Replies usually don't include attachments by default, although gmail, for one, provides a link/button to easily attach the original attachments.  The same technical problems are why we don't have an affordance ourselves.

Comment 7

[Wayne Chang [:wchang]]

Dominic please assign appropriate person to take.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Dominic, please update the bug with dependency you mentioned in the meeting.

Comment 9

[Dominic Kuo [:dkuo]]

Tim, that's another bug, I am emailing that to you, thanks.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Andrew,

Dominic explained to me separately regarding comment 2. Could anyone beside you work on the download-and-upload feature in the e-mail libraries to achieve this nasty feature?

Comment 11

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

The only viable solution at this time is the lazy solution where we download the attachments for the file and then re-upload them.  That's a fairly easy implementation, although it has all kinds of UX issues; just download the attachments and once they're downloaded, and trigger the forward or notify the forward.  Modify the forwarding logic to allow attaching the attachments from the original.  

This bug lacks an explicit priority.  Unless it's a P1, everyone with experience at this type of back-end work (or who is gaining experience) is spoken for on performance work or P1's.

Suggest the bug be prioritized (realistically!) and UX consulted to figure out a flow that is acceptable to users where it could take us multiple minutes to download the attachments on a message while they are composing (or before they are composing).

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

adding UX/Product into CC for further comments

Comment 13

[Casey Yee]

(In reply to Andrew Sutherland (:asuth) from comment #11)
> The only viable solution at this time is the lazy solution where we download
> the attachments for the file and then re-upload them.  That's a fairly easy
> implementation, although it has all kinds of UX issues; just download the
> attachments and once they're downloaded, and trigger the forward or notify
> the forward.  Modify the forwarding logic to allow attaching the attachments
> from the original.  

Besides the issues as noted above, 

- We would want to adequately warn the user of potential extra charges when forwarding attachments.   If for example, the user forwards a largish 2MB PDF, the total transfer cost would be 4MB.  Which could be much more than the user expects. 

- Currently, messages are sent in a way that blocks the user from doing anything else in the UI.   A large forward could hang-up the interface for a significant amount of time.  I think that we would need to add some additional information to the progress screens so that the user has an idea of what is happening.

- Handling of any additional error/failure cases.


> Suggest the bug be prioritized (realistically!) and UX consulted to figure
> out a flow that is acceptable to users where it could take us multiple
> minutes to download the attachments on a message while they are composing
> (or before they are composing).

Because there is quite a bit of UX that needs to be considered and the real potential to upset users over unintended data charges.   I feel that we should stick with the current behavior and forgo any attachment forwarding until a later milestone when we have more time to work this out fully.

Comment 14

[Peter Dolanjski [:pdol]]

(In reply to Andrew Sutherland (:asuth) from comment #2)
> 
> If we extended our compose capabilities, then on certain server
> configurations we would be able to avoid downloading/uploading by having the
> server do much of the composition for us.  To my knowledge, this is only
> available on "real" IMAP servers supporting the LEMONADE suite of
> CATENATE/URLAUTH/URL-PARTIAL, accompanied by a BURL-supporting SMTP server. 
> Versions of dovecot, cyrus (notably used by fastmail.fm), and zimbra (as
> used by mozilla.com) support this.  Neither gmail nor yahoo support those
> extensions.  I don't see any ActiveSync support in
> SendMail/SmartReply/SmartForward, although it does seem ridiculous to me, so
> hopefully I'm just missing something.

Andrew, do you know if downloading and re-uploading is how this works on other platforms for Gmail/Yahoo?

Comment 15

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

(In reply to Peter Dolanjski [:pdol] from comment #14)
> Andrew, do you know if downloading and re-uploading is how this works on
> other platforms for Gmail/Yahoo?

Anyone using IMAP to talk to the server is going to be in the same situation we are.

It's extremely unlikely that the Google developed Gmail apps for devices speak IMAP.  They likely use a proprietary HTTPS-based API like the webmail UI (or the same of the same as the webmail UI) that can have the server do things for them.

Yahoo is probably able to do something similar with their dedicated apps, although it's conceivable they reused some of their Zimbra desktop client code, in which case maybe they are speaking IMAP.

A network capture of Android/iOS traffic for the apps on such a device (or if there's a firewall app for the device) should make it clear what network port is being used, and based on the device throughput how much information is being sent to the server in such a case.  But that's a little academic because all we have is IMAP.

Comment 16

[Peter Dolanjski [:pdol]]

The Product team discussed this further.  Given that we will be supporting attachments to a much greater extent in leo, not having forwarding of attachments would appear broken to the user (as there is an expectation set from every other email app that the attachment inclusion will occur).

I also want to be cognizant of the limitations and additional work required, as pointed out above.

Andrew/Casey, do you mind estimating how much effort would be needed to flesh this out and deliver? (so that we can try to prioritize)

Assume: 
- change behavior for sending emails to non-blocking or some other sort of indication as to what is happening
- user choice to include attachment in forward message or not
- error handling in the event of download or upload failure

Comment 17

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

Assuming the user-friendly pure async solution:

Switching to asynchronous/background sending is about 2 weeks of work because of offline op restoration we had skimped on and the UX need to have an outbox and need for a fair number of unit tests.  That's basically bug 834774.

Chaining the async download of attachments into the send process of a forwarded mail with attachments is about 1 week, noting that there are complicated failure cases where we'd have to end up just silently sending the message without attachments.  (If, say, the source message no longer exists.)

Causing forwarded HTML mails to undo our sanitization steps is about half a week.

Comment 18

[Casey Yee]

Hi there,
I'd estimate about 16 hours of UX work here.  This would include the Outbox, attachment forwarding and error handling as per Andrew.

Comment 19

[jachen]

Please let us know how to proceed in terms of scope.

If decide to go with just a message and do not include attachment on forward, we will need to provide a message that gives an explanation.

i.e. "Attachment forwarding currently not supported. Forward message without attachment?"

I think if the user just receives a message "Warning: no attachment will be forwarded" will really confuse people.

Comment 20

[Kai-Chih Hu [:khu]]

I agreed with jachen in comment #19. 

If we are going to save the attachments, and upload attachments when forwarding the message, just want to confirm that, the attachments saved will be deleted after the message is sent, right? 

Thanks.

Comment 21

[Kai-Chih Hu [:khu]]

Another question is, If we are going to save the attachments, and upload attachments when forwarding the message, we may also need to have an error message if there has no enough space to save the attachment, right?

Comment 22

[Kai-Chih Hu [:khu]]

Andrew, is this something you could be the owner? Thanks.

Comment 23

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

(In reply to khu from comment #22)
> Andrew, is this something you could be the owner? Thanks.

As far as I can tell, all the open questions right now are effectively UX decisions, like whether to only temporarily store the attachment or permanently download it.

Comment 24

[Peter Dolanjski [:pdol]]

Given the effort involved, actually forwarding the attachments is too risky for leo, but we should go ahead with the option that Jaime cited, warning the user.  

Jaime, can you assign someone to this?

Comment 25

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

In the spec please also remember to include the case for drafts having the attachment if the user decides to safe the draft.

Comment 26

[Stephany Wilkes]

Assigning to Rob for final copy and any required notification style direction.

Comment 28

[Rob MacDonald [:robmac]]

Sincerest apologies for the delay on this one...

I've posted a proposed temporary flow here:

https://www.dropbox.com/s/yjpbzx91hd8yb83/email-attachments-bug828294.pdf

Let me know if I've missed anything. Also flagging Matej for a tone of voice review.

Matej, the proposed strings are:

     Email attachment forwarding is not currently supported.

     

Forward this message without attachments?

- Rob

Comment 29

[Matej Novak [:matej]]

I's clear, but also a bit of a mouthful. I think we can simplify it a bit:


Attachments can not be forwarded at this time.

Forward this message without attachments?

Comment 30

[Rob MacDonald [:robmac]]

(In reply to Matej Novak [:matej] from comment #29)
> I's clear, but also a bit of a mouthful. I think we can simplify it a bit:
> 
> Attachments can not be forwarded at this time.
> 
> Forward this message without attachments?

Hi Matej...

Thanks again for your help with this. My only concern is "at this time" sounds like a temporary situation that may or may not be resolved the next time around. Do you agree? And, if so, any suggestions? 

- Rob

Comment 31

[Matej Novak [:matej]]

(In reply to Rob MacDonald [:robmac] from comment #30)
> (In reply to Matej Novak [:matej] from comment #29)
> > I's clear, but also a bit of a mouthful. I think we can simplify it a bit:
> > 
> > Attachments can not be forwarded at this time.
> > 
> > Forward this message without attachments?
> 
> Hi Matej...
> 
> Thanks again for your help with this. My only concern is "at this time"
> sounds like a temporary situation that may or may not be resolved the next
> time around. Do you agree? And, if so, any suggestions? 
> 
> - Rob

I wondered about that as well, but thought it was OK since the original string said "currently."

We could change it to this:

Attachments can not currently be forwarded.

But that sounds a bit clunky.

We could also take out the conditional wording altogether:

Attachments can not be forwarded.

But then it sounds pretty final, like we won't ever support it.

WDYT?

Comment 32

[Mark Shiao]

Hi Rob and Matej,

Any updates on the wording?  I've got the fix in place based on Rob's attached flow doc.  Will wait for the final wording before making my pull request.

Thanks,
Mark

Comment 33

[Stephany Wilkes]

In the interest of finishing this, let's go with "Attachments can not be forwarded." When we support them later, the strings can change. This is at least clear and easy for localization.

Comment 34

[Matej Novak [:matej]]

(In reply to Stephany Wilkes from comment #33)
> In the interest of finishing this, let's go with "Attachments can not be
> forwarded." When we support them later, the strings can change. This is at
> least clear and easy for localization.

Sounds good to me. Thanks.

Comment 35

[Mark Shiao]

Attachment: Link to pull request

Hi Andrew,

Can you please review this patch? I implemented a confirm dialog on message forwarding notifying user that attachments can not be forwarded.

Thanks,
Mark

Comment 36

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

Comment on attachment 752518 [details]
Link to pull request

Thanks for the patch!  The dialog looks good, but currently it is being displayed regardless of whether or not the message we are trying to forward actually has attachments.

The logic for determining whether to prompt should go something like this:
var needToPrompt = this.header.hasAttachments || this.body.embeddedImageCount > 0;

Note that this.body starts out undefined, so onForwarded really wants to return early "if (!this.body)".  It's already the case that a request to build the composer widget while the body has not yet been fetched will fail, so there's no (added) harm in that early return.  To make us all feel better, it might be good to add a comment too.  So:

// If we don't have a body yet, we can't forward the message.  In the future
// we should visibly disable the button until the body has been retrieved.
if (!this.body)
  return;


Once you've addressed this, please set review back to "?".

Comment 37

[Mark Shiao]

(In reply to Andrew Sutherland (:asuth) from comment #36)
> Comment on attachment 752518 [details]
> Link to pull request
> 
> Thanks for the patch!  The dialog looks good, but currently it is being
> displayed regardless of whether or not the message we are trying to forward
> actually has attachments.
> 
> The logic for determining whether to prompt should go something like this:
> var needToPrompt = this.header.hasAttachments ||
> this.body.embeddedImageCount > 0;
> 
> Note that this.body starts out undefined, so onForwarded really wants to
> return early "if (!this.body)".  It's already the case that a request to
> build the composer widget while the body has not yet been fetched will fail,
> so there's no (added) harm in that early return.  To make us all feel
> better, it might be good to add a comment too.  So:
> 
> // If we don't have a body yet, we can't forward the message.  In the future
> // we should visibly disable the button until the body has been retrieved.
> if (!this.body)
>   return;
> 
> 
> Once you've addressed this, please set review back to "?".

Hi Andrew,

Apologize for the obvious.  Didn't cross my at any point to check if an attachment was available.  Please let me know if there are additional issues.

Thanks,
Mark

Comment 38

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

Comment on attachment 752518 [details]
Link to pull request

Looks perfect now.  Thanks!  r=asuth

Comment 39

[Mark Shiao]

landed on master #3d2f650c5bed2aa2e6427c205743a18ebdc21332

Comment 40

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

(Updating subject to reflect actual fix implemented.)

Comment 41

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 3d2f650c5bed2aa2e6427c205743a18ebdc21332 to:
v1-train: 6b561998720685ec6b6fa0ff09daad51b880b734

Comment 42

[FangChen]

the issue has been reported again on FF0S1.1 during the Latam certification
Built ID 20130823171628
Device: Ikura
QC RIL version: "ro.build.firmware_revision=V1.01.00.01.019.180"
gaia commit: 4916f82 Merge branch 'zte/ics_strawberry_cdr' of ssh://10.67.16.41:29418/quic/lf/releases/gaia into ics_strawberry_cdr
gecko commit: a1c2bb0 ZRL modify ACCEPT Http head for MMS