1270149 - Options applied to a template get lost when the template is used.

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[rrs]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160407164938

Steps to reproduce:

I use save templates so I don't have to recreate email information for various events. After this recent update, my templates are not holding my options, like "return receipt" or "delivery notification". One reason to use a template is to insure certain options are automatic with certain types of emails.


Actual results:

I update a template, save it, and then open the newly saved template to confirm that the options are still active. On my work computer, the options are not active. On my home computer, opening the same template, I find the options are active.
The draft feature holds the options; however, the template feature isn't holding the options.


Expected results:

Every time I save a template, I expect all of the options to exists when I actually use the template.

Comment 1

[Alexander Bergmann]

I can reproduce the problem!

If a new message is saved as a draft or as a template, the options "return receipt" and "delivery status notification" are both saved correctly in the created messages:
> X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=1; DSN=1; uuencode=0; attachmentreminder=0

However, if "edit as new" is used for the saved draft or template or the saved template is opened normally, then the options are lost.

Comment 2

[rrs]

They are not lost in the draft save feature, but they are lost in the template. 
This bug was created in the last update. What did your response answer? This is a bug and a problem. When is it going to be fixed? or how do we work around it. 

Please do not state the obvious... Give me an answer or a solution. Thank you.

Comment 3

[Jorg K (CEST = GMT+2)]

Starting from TB 45 we distinguish between "edit as new" and "edit draft" for a draft message.
"edit as new" does NOT restore the options, but "edit draft" does.

I can confirm that for a template for example the "delivery status notification" is not restored, but it was in TB 38. I find this a questionable feature, but obviously some people relied on it.

Aceman, do you have an opinion?

Comment 4

[Jorg K (CEST = GMT+2)]

Alice, if you have time, perhaps you can identify the regression. So the problem is that a template is saved with "delivery status notification" checked under options, but when you open it (double-click) it loses the setting. You can just prepare a template once and then open it with various versions of TB.

It might be bug 321355 or bug 1202165.

Comment 5

[Alice0775 White]

(In reply to Jorg K (PTO during summer, NI me) from comment #4)
> Alice, if you have time, perhaps you can identify the regression. So the
> problem is that a template is saved with "delivery status notification"
> checked under options, but when you open it (double-click) it loses the
> setting. You can just prepare a template once and then open it with various
> versions of TB.
> 
> It might be bug 321355 or bug 1202165.

Could you provide "Step by step STR" and "Actual and Expect Results"?

Comment 6

[rrs]

Response to Comment 3. Jorg K (?) 

The reason we rely on all of features chosen and saved in a template format is because it is a template. Template meaning an already formatted document... 

Since we do not require all of our emails to include delivery confirmation or return receipt, some emails do require these features. Since we cannot rely on ourselves to mark all options before sending each email, we rely on the template. I added this only to stress the importance of this feature.

Comment 7

[Jorg K (CEST = GMT+2)]

STR:
Create a new e-mail message. Under Options, set "Return Receipt" and "Delivery Status Notification".
Save as template.
Look at the template (message source), you'll see: receipt=1; DSN=1;
That's not broken.
I assume that you don't have these two options set by default.

Now, it various versions of TB, double-click the template.
Expected: "Return Receipt" and "Delivery Status Notification" ticked (like in TB 38).
Actual: "Return Receipt" and "Delivery Status Notification" not ticked (like in TB 45).

Comment 8

[Alice0775 White]

Regression window:
https://hg.mozilla.org/releases/mozilla-esr45/pushloghtml?fromchange=95e3b775e1ce11207577d6976f01de515b15f33b&tochange=a24504c4178baff24ec97fdb5eaf2a738ef2f51d
https://hg.mozilla.org/releases/comm-esr45/pushloghtml?fromchange=1f43b3761461&tochange=60354f9683ed

Suspect:Bug 321355

Comment 9

[Jorg K (CEST = GMT+2)]

Many thanks, Alice!!

Sigh, that was a one line change:
https://hg.mozilla.org/releases/comm-esr45/rev/60c360c6b4aa#l3.12

Comment 10

[rrs]

Does that mean you solved the bug? and it will be fixed on the next update?

I like the way you, all, work! Thanks for getting involved.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to rrs from comment #10)
> Does that mean you solved the bug? and it will be fixed on the next update?
This bug is a regression, that means that something that worked before doesn't work any more. We identified where the regression was caused, in bug 321355, but that doesn't mean there is an easy fix. Usually it's our aim to fix regressions in a release not to far in the future but I could name some which never got fixed ;-(

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Patch for debugging and investigation.

Magnus: Hiro, myself and you broke this with this one line change:
https://hg.mozilla.org/releases/comm-esr45/rev/60c360c6b4aa#l3.12

I didn't understand that change fully. So the question: Do you have a good idea? Yes, it's MIME, so we're stepping onto another planet.

With this patch I see:
Edit draft:
(code path not hit)

Edit draft as new:
Before setting URL name |imap-message://[e-mail]/INBOX/Drafts#110?fetchCompleteMessage=true|
Setting URL name |imap-message://[e-mail]/INBOX/Drafts#110?fetchCompleteMessage=true|

Use template (edit as new):
Before setting URL name |imap-message://[e-mail]/INBOX/Templates#4?fetchCompleteMessage=true|
Setting URL name |imap-message://[e-mail]/INBOX/Templates#4?fetchCompleteMessage=true|

So drafts and templates, when edited as new, run through the same code path.

However, with the patch:
- Saving a draft which was edited as new overwrites the draft - WRONG!!
- Saving a template (which was edited as new since there is no other option) does not overwrite the original.

So I'm coming to believe that Hiro's change was wrong and that not overwriting the edited-as-new draft should have been done some other way, maybe the way templates are handled.

Looks like we're back to square one. The line we removed needs to go back and we need to find a proper solution for bug 321355.

What do you think?

Comment 13

[Jorg K (CEST = GMT+2)]

More analysis:
If the draft ID isn't set, like with Hiro's patch, we initialise all the options do their default:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1025

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Proposed hacky solution (v1).

This patch restores the line removed by Hiro in bug 321355.

Instead of restoring
  fields->SetDraftId(mdd->url_name);
we restore
  fields->SetDraftId("fakeID");

The consequence is that draft and template properties are maintained. However, when an "edited-as-new" draft is saved, it is still saved as a new draft, so we don't regress bug 321355.

The test for all this is:
  mozmake SOLO_TEST=composition/test-drafts.js mozmill-one
and that works locally.

So whilst this is hacky, it fixes the regression from bug 321355 without having to back it out. Given that this is a regression and given that MIME land is somewhat treacherous, I'm be happy with this fix.

Comment 15

[Jorg K (CEST = GMT+2)]

I wanted to see what exactly happens to the draft ID, so I traced through it:

Edit as new message on draft:
1) set "fakeID" as seen in the patch.
2) read here https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1018
   to determine whether the options should be set from the default overriding the draft/template options.
3) read here: https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2421

When saving the draft:
4) https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#345
   That's in nsMsgCompose::ResetUrisForEmbeddedObjects. There the URIs are transferred from the
   old draft to the new. That copes with the fake ID, but that might cause more problems.
5) https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3845
   That's in RemoveCurrentDraftMessage where a draft is overwritten.
   Also copes with the fake ID.

So the fake ID is really ugly. We only need it to trigger no overriding the options in 2).

Do you have a better idea?

Comment 16

[Jorg K (CEST = GMT+2)]

The fake ID also causes a problem in MsgComposeCommands.js.

There we have:
gEditingDraft = gMsgCompose.compFields.draftId;
and when closing the window and discarding any changes:
      case 2: //Don't Save
        // don't delete the draft if we didn't start off editing a draft
        // and the user hasn't explicitly saved it.
        if (!gEditingDraft && gAutoSaveKickedIn)
          RemoveDraft();

Now if I "edit as new" a draft and autosave kicked in, I then close the window and choose to discard changes, the saved draft doesn't get removed since gEditingDraft=="fakeID".

And here we see another bug in TB 38 which had the draft ID set for templates. An edit based on a template with and autosave which is later discarded, won't clean up the autosaved draft.

So what's a good option to trigger 2) without setting the draft ID?

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8750769 [details] [diff] [review]
Proposed hacky solution (v1).

I have a better idea. Wait an hour ;-)

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2).

OK, here is my proposal.

Were we set
  fields->SetDraftId(mdd->url_name);
before that got removed in bug 321355 I now set a special indicator:
nsMsgCompFields::MsgHeaderID::MSG_DRAFT_TEMPLATE_INDICATOR

Due to the
nsMsgCompFields* _fields = static_cast<nsMsgCompFields*>(fields.get());
this is still a little on the hacky side, but to do it properly I'd have to open up nsIMsgCompFields.idl which is not an option.

I've also enhanced the test to make sure that the options are maintained, even for "edit as new" on the draft:
  mozmake SOLO_TEST=composition/test-drafts.js mozmill-one

I'm happy with this solution. It is fully understood and the test will ensure that it won't break again.

Comment 19

[Jorg K (CEST = GMT+2)]

Wayne, you wanted to see regressions, so here you go.

Comment 20

[Kent James (:rkent)]

Let's try to get this in the next release.

Comment 21

[Jorg K (CEST = GMT+2)]

That depends entirely on the reviewer ;-)

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 8750959 [details] [diff] [review]
Proposed solution (v2).

Review of attachment 8750959 [details] [diff] [review]:
-----------------------------------------------------------------

Why not just a private member in nsMsgCompFields instead of the "header" MSG_DRAFT_TEMPLATE_INDICATOR?

Didn't have time to try it yet. But, since edit as new and compose as template are the same, doesn't this still break on of the cases? 
For edit as new you want the settings from the current identity, for template the ones from the template

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v3).

(In reply to Magnus Melin from comment #22)
> Why not just a private member in nsMsgCompFields instead of the "header"
> MSG_DRAFT_TEMPLATE_INDICATOR?
That's an option too, I've done it now. I was just inspired by the draft ID header ;-)

> Didn't have time to try it yet. But, since edit as new and compose as
> template are the same, doesn't this still break on of the cases? 
> For edit as new you want the settings from the current identity, for
> template the ones from the template.
I'll explain it again:
Before Hiro removed the line
  fields->SetDraftId(mdd->url_name);
anything that was edited as new, draft, template, whatever, got the options set from whatever it was that got edited as new. That was expected behaviour.

As a consequence of having a draft ID set, the previous version of the draft was deleted here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3865
Please see detailed analysis in comment #15.

As detailed in comment #16, the draft ID being set also lead to another bug:
      case 2: //Don't Save
        // don't delete the draft if we didn't start off editing a draft
        // and the user hasn't explicitly saved it.
        if (!gEditingDraft && gAutoSaveKickedIn)
          RemoveDraft();
If the user closed the composition window without saving and a draft ID was set, then any autosaved draft is not deleted. So you edit something as new, a draft gets autosaved, you abandon the edit and the saved draft hangs around. I've just tried it in TB 38.

When a draft is edited as new, we don't want to delete the previous version, that was bug 321355. So Hiro's fix was legitimate, and he accidentally (or knowingly) fixed the auto-save problem described above.

So it is correct not to have a draft ID for anything edited as new. Now, the problem is, how I communicate that I still want to maintain the options from my reused object. The solution is to pass them along in some other hacky way.

The test that makes sure that everything is OK is
  mozmake SOLO_TEST=composition/test-drafts.js mozmill-one

(Note that this is an urgent review since we want to fix the regression in the next release.)

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 8751496 [details] [diff] [review]
Proposed solution (v3).

Review of attachment 8751496 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +1030,5 @@
>      // Set return receipt flag and type, and if we should attach a vCard
>      // by checking the identity prefs - but don't clobber the values for
>      // drafts and templates as they were set up already by mime when
>      // initializing the message.
> +    if (m_identity && setFields)

Hmm, seems to me you could just have

if (m_identity && draftId.IsEmpty() && (type != nsIMsgCompType::Draft || type != nsIMsgCompType::Template))

... or is that type!=template only..?

Or am I missing something?

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v4).

(In reply to Magnus Melin from comment #24)
> if (m_identity && draftId.IsEmpty() && (type != nsIMsgCompType::Draft ||
> type != nsIMsgCompType::Template))
> ... or is that type!=template only..?
Template only since "edit as new" of a draft goes as template.
Edit of a draft has the draft ID set, so it can be deleted later.

mozmake SOLO_TEST=composition/test-drafts.js mozmill-one
passes with the patch.

> Or am I missing something?
No, I missed a whole lot apparently.

Well, you missed one bit:
(type != nsIMsgCompType::Draft || type != nsIMsgCompType::Template) is always true ;-)

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 8752340 [details] [diff] [review]
Proposed solution (v4).

Review of attachment 8752340 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=mkmelin

Comment 27

[Jorg K (CEST = GMT+2)]

Thanks for the inspiration ;-)

Landed on TB 49:
https://hg.mozilla.org/comm-central/rev/29b988292b20

Uplifts coming.

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 8752340 [details] [diff] [review]
Proposed solution (v4).

[Approval Request Comment]
Regression caused by (bug #): Bug 321355.
User impact if declined: Nasty, some of the template functionality stops working.
Testing completed (on c-c, etc.): Manual and with Mozmill test.
Risk to taking this patch (and alternatives if risky):
Very low risk. Very well understood one line change which has a test.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8752340 [details] [diff] [review]
Proposed solution (v4).

Oops, beta and esr45 need different patches.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: Patch for beta.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: Patch for ESR45

Sadly in ESR45 there is no test since the test was introduced in bug 1202165 which landed on TB 47.

Comment 32

[Jorg K (CEST = GMT+2)]

Aurora (TB 48):
https://hg.mozilla.org/releases/comm-aurora/rev/1800cce8869a

Comment 33

[Kent James (:rkent)]

Comment on attachment 8752478 [details] [diff] [review]
Patch for beta.

http://hg.mozilla.org/releases/comm-beta/rev/f707a3dcecc8
http://hg.mozilla.org/releases/comm-beta/rev/3399f3fdf71d

(I missed at first that there was a separate patch for beta, so pushed aurora patch without the test at first)

Comment 34

[Kent James (:rkent)]

Comment on attachment 8752479 [details] [diff] [review]
Patch for ESR45

http://hg.mozilla.org/releases/comm-esr45/rev/f2abc926a364