963331 - [B2G][email]Recieved emails can be moved to 'Local Drafts' folder. Email body is erased and email is now treated as an unsent draft.

Status: VERIFIED FIXED

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

Description

[Ross Kuhlman ([e-mail])]

Attachment: EmailsMovedToLocalFolder.txt

If the user moves a recieved email from any folder into the 'Local Drafts' folder, the email body will be erased. The 'To', 'CC', 'BCC' and 'Subject' fields are preserved, but any attachments and the body content have been erased. The email is not treated as an unsent draft, pressing the back button will prompt the user to save or erase the draft.

Repro Steps:
*Prerequisite: Be logged into an email account on the device.

1) Updated Buri to BuildID: 20140122004001
2) Launch Email app.
3) Navigate to Inbox.
4) Tap the 'Edit' button. (Three horizontal dots and a checkmark)
5) Highlight an email, and tap the change folder button in the bottom right corner.
6) Tap on the 'Local Drafts' folder to place the highlighted email there.
7) Navigate to 'Local Drafts' folder.
8) Observe email that was moved in step 6.

Actual:
The email content has been erased, and the email is now treated as an unsent draft.

Expected:
User cannot place emails in the 'Local Draft' folder.
-OR-
Emails placed in the 'Local Draft' folder can still be read and treated like recieved emails.

Environmental Variables:
Device: Buri v1.3 Moz RIL
BuildID: 20140121004137
Gaia: 47049555282a9a01fb60d1e1421b57e2810c96f5
Gecko: 6f7dfe36ab6c
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Notes:
If the device is low on available memory, the emails will never appear in the 'Local Drafts' folder. They will disappear from the inbox but will not be placed in Local Drafts.

Repro frequency: 100%
See attached: logcat

Comment 1

[Ross Kuhlman ([e-mail])]

Issue also occurs in v1.2
Environmental Variables:
Device: Buri v1.2 Moz RIL
BuildID: 20140114004002
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 42a1c35fc831
Version: 26.0
Firmware Version: v1.2-device.cfg

Comment 2

[Jason Smith [:jsmith]]

What happens on 1.1?

Comment 3

[Ross Kuhlman ([e-mail])]

Attachment: EmailLog1.1.txt

Attaching log of User moving an email from the inbox to the Local Drafts folder in the latest available 1.1 build. The email never appears in the Local Drafts folder. The local folder displays a spinning throbber and claims to be searching for emails. This state persists indefinitely and no emails are displayed in the local dragts folder.

Comment 4

[Matthew Vaughan]

Removing qawanted per comment 3.

Comment 6

[Tony Chung [:tchung]]

nominating for 1.4? Triage team.

Comment 7

[Alex Wissmann [:awiss]]

Attachment: Frontend Patch

Comment 8

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

I agree that it makes sense to support a predicate for filtering out folders, but I think that forbidding moves to the localdrafts folder is arguably a domain/implementation problem that should be handled by the backend.

I'd propose we do something like add a property/getter on MailFolder in mailapi.js named something like "isValidMoveTarget" and then use that predicate in the front-end.  This is along the lines of the "selectable" property we compute, except its name is different. (Maybe we should name the thing "validMoveTarget" instead?)

Rather than eliding a folder from the list, it might be more appropriate to actually change from using 'selectable' to our 'validMoveTarget' predicate.  (And perhaps then !selectable implies !validMoveTarget).  This is relevant for hierarchy reasons; if we hide the parent folder of a folder things can get weird.  (Not a problem for localdrafts, though, of course.)

Comment 9

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8455619 [details] [review]
Frontend Patch

Clearing review per :asuth's comment about moving it into the backend. Also, probably best to flag him for review for this when it's time, since I'm vaguely gone this week and Andrew probably is itching to review things after his long break.

Comment 10

[Alex Wissmann [:awiss]]

(In reply to Andrew Sutherland [:asuth] from comment #8)
> I agree that it makes sense to support a predicate for filtering out
> folders, but I think that forbidding moves to the localdrafts folder is
> arguably a domain/implementation problem that should be handled by the
> backend.
> 
> I'd propose we do something like add a property/getter on MailFolder in
> mailapi.js named something like "isValidMoveTarget" and then use that
> predicate in the front-end.  This is along the lines of the "selectable"
> property we compute, except its name is different. (Maybe we should name the
> thing "validMoveTarget" instead?)
> 
> Rather than eliding a folder from the list, it might be more appropriate to
> actually change from using 'selectable' to our 'validMoveTarget' predicate. 
> (And perhaps then !selectable implies !validMoveTarget).  This is relevant
> for hierarchy reasons; if we hide the parent folder of a folder things can
> get weird.  (Not a problem for localdrafts, though, of course.)

Not sure how this would work in terms of the frontend. It seems like we would not want Local Drafts as an option on the move folder selector. However, it doesn't seem right to use this validMoveTarget property/getter in folderSelector, since folderSelector could be used for other things as well. My idea now is to have the user pass in a function to folderSelector() which takes a folder and then returns a true or false value depending on whether that folder should be included. How does this sound?

Comment 11

[Alex Wissmann [:awiss]]

(In reply to Andrew Sutherland [:asuth] from comment #8)

Another thing: What should the logic look like for determining whether a folder is a valid move target? Should it just be that particular folder types return false?

Comment 12

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

(In reply to Alex Wissmann [:awiss] from comment #10)
> Not sure how this would work in terms of the frontend. It seems like we
> would not want Local Drafts as an option on the move folder selector.

This is arguably somewhat of a UX decision here.

The benefit to showing the folder to the user but indicating that it is not a legal option by graying it out and refusing to let it be selected is that there's no ambiguity.  If we hide it, the user might scroll around looking for it.  Depending on the complexity of their folder hierarchy, this could be a lot of scrolling.

The benefit to hiding illegal options is that you potentially shorten the list a lot and avoid situations whether the user is tapping away at a disabled option not realizing that it's disabled.  The complicating factor is the noted one with hierarchy; it's only safe to hide an ancestor folder that's not a legal move target if it none of its children are legal move targets.

(Nothing again that localdrafts can never/will never have children.  But at some point we will stop using "localdrafts" and will be using real drafts folders, and at that point we are going to be faced with this complexity.  We'll also likely end up reusing the same UI component for filter targets and spam folder stuff and such.)

Juwei, what are your thoughts on hide versus show as disabled?


> However, it doesn't seem right to use this validMoveTarget property/getter
> in folderSelector, since folderSelector could be used for other things as
> well. My idea now is to have the user pass in a function to folderSelector()
> which takes a folder and then returns a true or false value depending on
> whether that folder should be included. How does this sound?

I think we're on the same page.  I didn't mean to convey that we'd hard-code the use of validMoveTarget.  I was thinking either we could say what attribute to check or use a function like you're proposing.  Certainly function is more flexible/idiomatic and less weird than specifying 'validMoveTarget'.


(In reply to Alex Wissmann [:awiss] from comment #11)
> Another thing: What should the logic look like for determining whether a
> folder is a valid move target? Should it just be that particular folder
> types return false?

That sounds right to me, but elaborating:

We already have 'account' and 'nomail' as non-selectable in that logic.  These are because:
- 'account' is just a hierarchy trick in the case where we're exposing all accounts and their folders in a single list
- 'nomail' is for IMAP folders that can't have mail in them and thus there's nothing to see in there and definitely not a place we can move things.

Since the overall idea of !selectable is that there's nothing to see in those folders, it does stand to reason that there's no reason to move things into them too.  So I'd chain off that existing logic.

Then for other folder types, we absolutely don't want to move messages into our own magical folder types.  Those are: 'localdrafts' and 'outbox'.

A case could be made for not letting messages be moved into the 'drafts' folder, but that gets into complicated edge cases relating to deletion/undeletion, etc.  I'd punt on this for now but would believe that it could make sense to add warnings for situations like moving into or out of a drafts folder when the trash folder is not involved.

Comment 13

[Juwei Huang[:juwei]]

I can see that the local draft folder is kind of an alternate solution because of the constraint that we cannot save the message to draft folder on server. 
So I suggest that we hide the local draft right now to prevent some confusion and possible issues.

Comment 14

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

So it's a given that we collapse when possible.  Then the question is how do we handle the interaction of selectable and validMoveTarget.  Running through the options, I could see us conflating !selectable as a node being important for hierarchy in our display.  I propose we avoid that and also introduce a property/getter "neededForHierarchy" or something like that.  It should currently be true when "selectable" is false.

So the pseudocode I think ends up like this:
  showTheFolderInTheList = neededForHierarchy || predicateSaidYes
  letTheFolderBeSelectable = predicateSaidYes

Comment 15

[Alex Wissmann [:awiss]]

Attachment: GELAM patch

Comment 16

[Alex Wissmann [:awiss]]

I just submitted a gelam patch and updated the new ones. The logic now requires !neededForHierarchy && !isValidMoveTarget for a particular folder to be excluded.

Comment 17

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

Comment on attachment 8456422 [details] [review]
GELAM patch

Looks good with small request fixed, but I think it would be nice if we could get some trivial test coverage here.

Bare minimum, in test_account_folder_logic.js we have a "syncFolderList created localdrafts folder" case that could be made to check that we're computing it correctly for localdrafts.  We could also potentially add an explicit testcase that exists just to test for what the MailFolders look like for our automatically created folders and/or what syncFolderList produces from our carefully controlled fake-server reported tree at least for IMAP.

Comment 18

[Alex Wissmann [:awiss]]

Could you take a look at the test I just pushed up, its the "bare minimum case" you just mentioned. (should I r? again here?) I'm not quite sure what the second test case idea you had would look like exactly, so we can discuss it next week.

Comment 19

[James Burke [:jrburke]]

Comment on attachment 8455619 [details] [review]
Frontend Patch

r+ for the front-end changes, with just one small nit mentioned in the pull request.

Comment 20

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

(In reply to Alex Wissmann [:awiss] from comment #18)
> Could you take a look at the test I just pushed up, its the "bare minimum
> case" you just mentioned. (should I r? again here?) I'm not quite sure what
> the second test case idea you had would look like exactly, so we can discuss
> it next week.

If you want to make sure people don't forget to look at things, it is definitely best to use some type of flag since then the bug will show up on their dashboard.  (So r? or f? on the patch, or ni? on the bug itself.)

Test-wise, you've got the check for the ones we want to be false to be false, but ideally we should also check that some of the cases we expect to be true are indeed true.  Super bare minimum for that would be to make sure the Inbox is a valid move target, but preferable would be to do a parametrized check where we verify the booleans are what we expect for all of our defined folder types.  Sort-of like https://github.com/awiss/gaia-email-libs-and-more/blob/localdrafts/test/unit/test_account_folder_logic.js#L205 but it's a somewhat different case.

After the current review feedback is addressed I think we're there.  Thanks!

Comment 21

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

Comment on attachment 8456422 [details] [review]
GELAM patch

Looks good!  Thanks for the thorough test coverage too!  Feel free to squash the gelam commits down as amuses you.  I'll wait to land until the gaia PR gets the ".selectable" case cleaned up and then I think it's good to go too!  (Please do request r? again for that so I know to check and then land.)

Comment 22

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

landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/321
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/7b131814f76f5acc96e516f3b4f10330e1c7a173

landed on gaia/master (note different pull request for squashing/landing expediency):
https://github.com/mozilla-b2g/gaia/pull/22298
https://github.com/mozilla-b2g/gaia/commit/81545451075a8610d827d4c5ef959d0e45c96c77

Comment 23

[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