1094632 - [Woodduck][Email]No attachment icon in the right of the email which has attahments in local draft folder

Status: VERIFIED FIXED

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

Description

[Josh Cheng [:josh]]

+++ This bug was initially created as a clone of Bug #1092945 +++

REPRODUCING PROCEDURES:
 1.Enter E-Mail from idle
 2.Configure a valid email account
 2.Compose a email with some attachments,and save it to local draft,then go to local draft to view--KO
 
  EXPECTED BEHAVIOUR:
 should have attachment icon

Ps. I found an another issue like this, 912525, but I didn't sure that version is work for Woodduck, so I have to submit a new one, sorry.

Comment 2

[Josh Cheng [:josh]]

per https://bugzilla.mozilla.org/show_bug.cgi?id=1092945#c6

Comment 3

[Josh Cheng [:josh]]

Hi Gary,
It's reported by partner. Do you think we have a temp solution for now and formal solution in future release?

Comment 4

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

Thanks for spinning off the public bug.  Quoting myself from that bug:

===
I can confirm the drafts implementation does not update the "hasAttachments" flag on the header in response to changes in attachments existing on the draft.  I believe this was an oversight in UX specification and implementation that constitutes a low-priority bug.
===

The temp solution and the formal solution would be the same thing for this since the real fix is relatively easy.

Comment 5

[Sherman Chen [:chens] (inactive)]

Hi Andrew,

I think saveDraft() should be the place to update attachment flags, but I found it hard to realize the connection between message header and composer, can you shed some light on how to take care of this? thanks!

Comment 6

[qinghao.wu]

Hi 
  any ideas? could you give me some copy from bug1092945 ? 
  i am not authorized to access bug #1093425.

Comment 7

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

The buggy line is basically https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/js/drafts/draft_rep.js#L50

Test coverage checking hasAttachments should be added to https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/test/unit/test_compose_detach.js

Comment 8

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

(In reply to qinghao.wu from comment #6)
>   any ideas? could you give me some copy from bug1092945 ? 
>   i am not authorized to access bug #1093425.

That bug is the same as this bug but contains partner-identifying information.  There's no useful discussion or anything happening on it; we filed this bug and duped that one to this when it was clear it was a real bug.

Comment 9

[Josh Cheng [:josh]]

Dear Andrew,
Do you think you can take this bug as partner is requesting the fix? Thanks!

Comment 10

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request to 2.0m

Hi Andrew, 
Thanks for the suggestion, but I didn't find 2.0/2.0m branch on gaia-email-libs-and-more repo, if I'm going to fix this on 2.0m branch, may I change the library directly in gaia repo?

Comment 11

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

Unfortunately, there is no 2.0/2.0m branch for gaia-email-libs-and-more.  Our normal policy is to fix whatever needs to be fixed on the trunk, and then formulate an uplift manually if required.  So in this case a fix would be prepared for gaia-email-libs-and-more including tests.  The gaia patch would be derived from those changes via "make install-into-gaia" (as documented in gaia-email-libs-and-more's README.md) for trunk/v2.2.  Then those changes would be manually applied to a v2.0 checkout.

Comment 12

[Sherman Chen [:chens] (inactive)]

Hi Andrew, 

I found there's significant changes when "make install-into-gaia", seems the structure is totally different and I can't apply these on 2.0. If there's no 2.0 branch on gaia-email-libs-and-more repo, how should I apply these change on gaia 2.0?

Comment 13

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

Right.  It is an awkward situation.

Elaborating on comment 11, the way we would normally fix this on trunk and v2.0 is:

1) Formulate a trunk fix in gaia-email-libs-and-more including a unit test.
2) Install the changes into trunk gaia using "make install-into-gaia".
3) Test the changes in trunk gaia.
4) Prepare a v2.0 branch fix by grepping for the changed code in a 2.0/2.0m branch under gaia/apps/email/js/ext and directly making the change there.  (Using emacs or other tools it may be possible to apply the hunk directly.)

Comment 14

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

Comment on attachment 8525813 [details] [review]
Pull request to 2.0m

r=asuth by inspection.  I believe this is the correct fix.  I should be able to "port" this to trunk and provide some unit tests over the weekend, but please do feel free to do that if you've got the spare cycles and want to learn more about the email backend tests :). 

(The only tricky thing is I think there might be a cavalier unit test that abuses the mergeDraftStates function.  It'll show up in the test run though.)

Comment 15

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

Uh, which is to say, you have my blessing to land the fix on v2.0 if you can get approval.  I don't want to make any claims about riskiness until we have the unit test coverage on trunk, but realistically I don't expect any risk, especially if you manually test attaching and detaching an attachment and nothing breaks or shows errors in the logs.

Comment 16

[Sherman Chen [:chens] (inactive)]

Thanks Andrew :)

2.0m: https://github.com/mozilla-b2g/gaia/commit/d742e375aca6dc1bf3a36638000ad7f5338ef457

Comment 17

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

Let's leave this open since it's not fixed on trunk yet.

Comment 18

[Josh Cheng [:josh]]

Hi Chens,
Can you also provide patch for 2.1 and m/c? Thanks!

Comment 19

[Josh Cheng [:josh]]

Hi Norry,
Please verify on Woodduck 2.0M. Thanks!

Comment 20

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

Please don't mark this bug fixed until we have landed a fix on trunk.

Comment 21

[Norry.L.F(Leave from Mozilla)]

Attachment: Verify.png

The bug is verified successfully on Woodduck 2.0m

Attachment: Verify.png
Reproducing rate: 0/5
Reproducing steps：
 1.Enter E-Mail from idle
 2.Configure a valid email account
 2.Compose a email with some attachments,and save it to local draft,then go to local draft to view

Woodduck build:
Gaia-Rev        8e17537f9e6d02b7271d92134671414d1fb88386
Gecko-Rev       71be912cc4fedd423869ad9dce72880de170851f
Build-ID        20141128050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1417122322
FW-Date         Fri Nov 28 05:05:45 CST 2014

Comment 22

[Josh Cheng [:josh]]

Need to add test for landing this on m/c

Comment 23

[Keven Kuo [:kkuo]]

Hi! Chens,

Will you do uplift things to m-c?

--
Keven

Comment 24

[Sherman Chen [:chens] (inactive)]

I've already have a patch but still need to add tests, can't land on master until got review+ from module owner. Email backend test is completely different than gaia and I haven't figured it out yet.

Comment 25

[Josh Cheng [:josh]]

Hi Chens,
Can you take this bug since you already provide patch for 2.0M? 
Thanks!

Comment 26

[Sherman Chen [:chens] (inactive)]

Took it, but I didn't have enough bandwidth to provide PR with tests for master. If anyone interested in it please feel free to take it.

Comment 27

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request to GELAM

Hi Andrew, 

This patch tries to fix hasAttachmens in draft header, and add test on it. Would you review it and give some feedback? thanks!

Comment 28

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request to master

Hi Andrew, would you review this patch and give some feedback? thanks!

Comment 29

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

Comment on attachment 8554448 [details] [review]
Pull request to GELAM

Thanks very much for providing the test; I know it was probably a somewhat daunting undertaking!  Test looks good and passes with the fix and fails without the fix, so we're all good.  I'm landing it now.

Comment 30

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

landed in gaia/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/363
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/5d193a2cc170c5b4dbb0998d7608ff72cf834281

https://github.com/mozilla-b2g/gaia/pull/27675
https://github.com/mozilla-b2g/gaia/commit/4a1c90517ce8f133dc7ea96155436cb7304794ad

:chens/:josh, I'm going to leave it to you to request any appropriate uplift (or just make branch landings.)

From a risk perspective for approvers, this is extremely low risk, especially since it's already on v2.0m.

Comment 31

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8554452 [details] [review]
Pull request to master

Thanks Andrew, carry over r+ and requesting for 2.1/2.2 approval

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Pre-existed situation.
[User impact] if declined: Local draft will not have proper header indicates
there's attachment inside the draft.
[Testing completed]: Manual test and unit test.
[Risk to taking this patch] (and alternatives if risky): Quote from comment 30, this should be extremely low risk, especially since it's already on v2.0m.
[String changes made]: None

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 8554452 [details] [review]
Pull request to master

Approving on 2.1 as its low risk enough to take it.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/b928887bec52e11de9af41d22ef391a9990b9776
v2.1: https://github.com/mozilla-b2g/gaia/commit/6ff41a0928e62bcfaa1b4126b4422ca51147d9c3

Comment 34

[Shine(Leave from Mozilla)]

Attachment: Flame_master_verify1.3gp

This problem is verified as "pass" on latest build of Flame2.1/2.1S/2.2/master and Nexus 2.2/master by the STR in comment 0.
Actual result: The attachment icon can be displayed on the right of the email in local draft folder.
See attachment: Flame_master_verify1.3gp
Rate: 0/5

Device information:
Flame 2.1 (Pass)
Build ID               20150713001203
Gaia Revision          d13826b20b4a45e3f5cd4b25a30a737d8be7f1b9
Gaia Date              2015-07-02 23:36:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/92049b3c4bb5
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150713.035945
Firmware Date          Mon Jul 13 03:59:57 EDT 2015
Bootloader             L1TC000118D0

2.1S (Pass)
Build ID               20150713001205
Gaia Revision          8a614350a1f45f28e5a300bbd0233ee6d0a4738a
Gaia Date              2015-07-09 20:07:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/75dd72198612
Gecko Version          34.0
Device Name            scx15_sp7715ea
Firmware(Release)      4.4.2
Firmware(Incremental)  145
Firmware Date          Thu Apr 16 22:58:57 CST 2015

Flame 2.2 (Pass)
Build ID               20150713002503
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8d59402ba85a
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150713.041147
Firmware Date          Mon Jul 13 04:11:59 EDT 2015
Bootloader             L1TC000118D0

Flame master (Pass)
Build ID               20150713010204
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gaia Date              2015-07-10 14:29:23
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab21ec484bb
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150713.043935
Firmware Date          Mon Jul 13 04:39:47 EDT 2015
Bootloader             L1TC000118D0

N5 2.2 (Pass)
Build ID               20150713002503
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8d59402ba85a
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150713.040339
Firmware Date          Mon Jul 13 04:04:00 EDT 2015
Bootloader             HHZ12f

N5 master (Pass)
Build ID               20150713010204
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gaia Date              2015-07-10 14:29:23
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab21ec484bb
Gecko Version          42.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150713.043623
Firmware Date          Mon Jul 13 04:36:39 EDT 2015
Bootloader             HHZ12f