1417792 - Save draft composition as template creates a new template every time

Status: RESOLVED DUPLICATE of bug 1389062

(MailNews Core :: Composition, defect)

Description

[Thomas D. (:thomas8)]

STR

1) compose a msg with subject "My subject" and msg text "foo"
2) Save > Template
3) add "bar" to msg text
4) Save > Template again (or Ctrl+S)
5) Check templates folder

Actual results

Two templates with subject "My subject"
- one with msg text "foo" (from first save)
- another one with msg text "foo bar" (from second save)
This is unexpected because for the other two modes of saving (draft or file), we always overwrite the item of the current save mode.
There's no point in creating another template every time you save, and end up with a pile of old-version templates.

Expected result

Only one template with subject "My subject" and msg text "foo bar"

Comment 1

[Thomas D. (:thomas8)]

Jörg, I think this goes wrong in C++ code, could you have a look into it?

We'll also need this to get "Edit Template" command right (my Bug 1417788, has UI patch but currently stalled because I can't do those backend changes).

In fact, if I'm reading this code correctly, we are trying but failing to delete the old version of a saved template, while it seems to work for saved drafts:

https://dxr.mozilla.org/comm-central/rev/9d2ede676b3bceaa3fbf860324ec6fd7c24463c0/mailnews/compose/src/nsMsgCompose.cpp#3803-3810

      if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft ||
          mDeliverMode == nsIMsgSend::nsMsgSaveAsTemplate)
      {
        msgCompose->NotifyStateListeners(nsIMsgComposeNotificationType::SaveInFolderDone, aStatus);
        // Remove the current draft msg when saving as draft/template is done.
        msgCompose->SetDeleteDraft(true);
        RemoveCurrentDraftMessage(msgCompose, true);
      }


(In reply to Thomas D. from bug 1417788 comment #6)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #5)
> 
> > To-do:
> > *** 1) 'save-as-template' flavor must be preselected in Save-button dropdown
> 
> We touched that code in bug 507103 (attachment 740225 [details] [diff] [review]
> [review]), and there's some extensive related analysis from WADA in Bug
> 870313.
> 
> > *** 2) 'save-as-Template' must overwrite the existing template, not create a new
> > template (probably something about message-id and/or mime internals).
> 
> Bug 288702 - saving an edited template should replace existing template, and
> keep same message-id
> -> Attachment 179331 [details] [diff] - we might be able to borrow code from
> there. Note that bug 288702 has been mostly undone because somehow it did
> the wrong thing, probably by conflating "Edit as New Message" and "Edit
> Template/Draft".
> 
> > if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft || mDeliverMode == nsIMsgSend::nsMsgSaveAsTemplate)
> 
> > fields->SetMessageId(id); // keep same message id for editing template.
> 
> > *** 3) Ensure backend to do the right thing wrt 2) and Shift modifier.
> 
> That's Jörg's domain, as in bug 731688, attachment 8899166 [details] [diff] [review]
> [review].
> 
> > Any help on those would be appreciated

Comment 2

[Thomas D. (:thomas8)]

Jörg, looking at the old and mostly backed out patch Attachment 179331 [details] [diff] from bug 288702, the part about keeping the same msg ID when saving template is in mimedrft.cpp:

https://dxr.mozilla.org/comm-central/rev/9d2ede676b3bceaa3fbf860324ec6fd7c24463c0/mailnews/mime/src/mimedrft.cpp#1405-1410

    // Keep the same message id when editing a draft unless we're
    // editing a message "as new message" (template) or forwarding inline.
    if (mdd->format_out != nsMimeOutput::nsMimeMessageEditorTemplate &&
        fields && !forward_inline) {
      fields->SetMessageId(id);
    }

According to the comment, that actually looks wrong in the new dispensation where "Edit as New Message" (with new msg id) and "Edit template" (with same msg id) are now two different things.

Comment 3

[Thomas D. (:thomas8)]

Jörg, as a caveat, there's exactly one time where we want a NEW msg ID and NOT do delete the existing draft, namely for the first time when an existing draft is saved as template. But for all subsequent saves of the template, msg ID must be same, and old version of same template be deleted or updated.

Comment 4

[Jorg K (CEST = GMT+2)]

Thomas, the code you're quoting
https://dxr.mozilla.org/comm-central/rev/9d2ede676b3bceaa3fbf860324ec6fd7c24463c0/mailnews/mime/src/mimedrft.cpp#1405-1410
runs when you're editing a draft (ID stored on the new message created from the draft) or editing a message as new (ID *not* stored on the new message) or creating a message from a template, since that's also a new message.
It was introduced here: https://hg.mozilla.org/comm-central/rev/516a17742688#l1.14 (##) in bug 1279869 which was about maintaining message IDs. That's unrelated to the "go and delete the previous guy", see below. During the STR you gave (create message, save multiple times as template), that code isn't run.

There is special code in draft saving the nukes the previous draft:
https://dxr.mozilla.org/comm-central/rev/ea30a7f29acc01eb0bdc971a435adf4c36e600d0/mailnews/compose/src/nsMsgCompose.cpp#3867
That code is using a draft ID, which is a URL, not a message ID.

Bug 321355 which fixed "edit as new" for messages therefore removing setting the draft ID:
https://hg.mozilla.org/comm-central/rev/1a06d4cf1ad8#l3.12 (**)
That then caused a regression which was fixed here:
https://hg.mozilla.org/comm-central/rev/29b988292b20#l2.13

Even in TB 52 repeatedly saving as template creates multiple templates. So this is nothing that got broken recently.

Looking at bug 288702 "saving an edited template should replace existing template, and keep same message-id", that has regressed big time due to the changes at (##) and (**). I filed bug 1421736 for that.

Comment 5

[Jorg K (CEST = GMT+2)]

This bug can be fixed as part of bug 1421736. With a new compose type "edit template" we can set the compose type of the current composition to that type once it has been saved once as a template. Any subsequent saves will then overwrite. It becomes the same as pulling up a template for editing and then saving it again.

Sadly we now have spread the work over three bugs, this one, bug 1421736 and bug 1389062 :-(

Comment 6

[Jorg K (CEST = GMT+2)]

Will be fixed by implementing a proper "Edit Template" function in bug 1389062.

If you're interested where the fix is, look at the hunks in mailnews/compose/src/nsMsgCompose.cpp. We allow templates to be deleted and we set a "draft id" for the templates so it can find it's predecessor.