Bug 903390 - On reply to a own sent email the original sender identity is not chosen if other than the account default identity. r=jorgk
authorMagnus Melin <mkmelin+mozilla@iki.fi>
Tue, 21 Jun 2016 18:00:29 +0300
changeset 25341 cdecd4cf51a23b18fb58f1c4833511143f83c528
parent 25340 123f1bb494b7b254369fd549a764e77af9d09df5
child 25342 01df6045d101daecefbee660e0c8fc0836c699de
push id1725
push userclokep@gmail.com
push dateMon, 19 Sep 2016 17:35:08 +0000
treeherdercomm-beta@6ead1abf3817 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorgk
bugs903390
Bug 903390 - On reply to a own sent email the original sender identity is not chosen if other than the account default identity. r=jorgk
mail/components/compose/content/MsgComposeCommands.js
mail/test/mozmill/composition/test-reply-addresses.js
mail/test/mozmill/message-header/test-reply-identity.js
mailnews/compose/src/nsMsgCompose.cpp
--- a/mail/components/compose/content/MsgComposeCommands.js
+++ b/mail/components/compose/content/MsgComposeCommands.js
@@ -306,32 +306,53 @@ var stateListener = {
     case Components.interfaces.nsIMsgCompType.Redirect:
       break;
 
     default:
       dump("Unexpected nsIMsgCompType in NotifyComposeBodyReady (" +
            gComposeType + ")\n");
     }
 
+    // Setting the selected item in the identity list will cause an
+    // identity/signature switch. This can only be done once the message
+    // body has already been assembled with the signature we need to switch.
+    if (gMsgCompose.identity != gCurrentIdentity) {
+      // Since switching the signature loses the caret position, we record it
+      // and restore it later.
+      let editor = GetCurrentEditor();
+      let selection = editor.selection;
+      let range = selection.getRangeAt(0);
+      let start = range.startOffset;
+      let startNode = range.startContainer;
+
+      editor.enableUndo(false);
+      let identityList = document.getElementById("msgIdentity");
+      identityList.selectedItem = identityList.getElementsByAttribute(
+        "identitykey", gMsgCompose.identity.key)[0];
+      LoadIdentity(false);
+
+      editor.enableUndo(true);
+      editor.resetModificationCount();
+      selection.collapse(startNode, start);
+    }
     if (gMsgCompose.composeHTML)
       loadHTMLMsgPrefs();
     AdjustFocus();
   },
 
   NotifyComposeBodyReadyNew: function() {
     // Control insertion of line breaks.
     let useParagraph = Services.prefs.getBoolPref("mail.compose.default_to_paragraph");
     if (gMsgCompose.composeHTML && useParagraph) {
       let editor = GetCurrentEditor();
       editor.enableUndo(false);
 
       let pElement = editor.createElementWithDefaults("p");
-      let brElement = editor.createElementWithDefaults("br");
-      pElement.appendChild(brElement);
-      editor.insertElementAtSelection(pElement,true);
+      pElement.appendChild(editor.createElementWithDefaults("br"));
+      editor.insertElementAtSelection(pElement, false);
 
       document.getElementById("cmd_paragraphState").setAttribute("state", "p");
 
       editor.beginningOfDocument();
       editor.enableUndo(true);
       editor.resetModificationCount();
     } else {
       document.getElementById("cmd_paragraphState").setAttribute("state", "");
@@ -359,25 +380,35 @@ var stateListener = {
       if (range.startContainer != mailBody) {
         dump("Unexpected selection in NotifyComposeBodyReadyReply\n");
         return;
       }
 
       editor.enableUndo(false);
 
       // Delete a <br> if we see one.
+      let deleted2ndBR = false;
       let currentNode = mailBody.childNodes[start];
       if (currentNode.nodeName == "BR") {
         mailBody.removeChild(currentNode);
+        currentNode = mailBody.childNodes[start];
+        if (currentNode && currentNode.nodeName == "BR") {
+          mailBody.removeChild(currentNode);
+          deleted2ndBR = true;
+        }
       }
 
       let pElement = editor.createElementWithDefaults("p");
-      let brElement = editor.createElementWithDefaults("br");
-      pElement.appendChild(brElement);
-      editor.insertElementAtSelection(pElement,true);
+      pElement.appendChild(editor.createElementWithDefaults("br"));
+      editor.insertElementAtSelection(pElement, false);
+
+      if (deleted2ndBR) {
+        editor.insertElementAtSelection(
+          editor.createElementWithDefaults("br"), false);
+      }
 
       // Position into the paragraph.
       selection.collapse(pElement, 0);
 
       document.getElementById("cmd_paragraphState").setAttribute("state", "p");
 
       editor.enableUndo(true);
       editor.resetModificationCount();
@@ -4239,32 +4270,57 @@ function LoadIdentity(startup)
           {
             needToCleanUp = true;
             if (prevReplyTo != "")
               awRemoveRecipients(msgCompFields, "addr_reply", prevReplyTo);
             if (newReplyTo != "")
               awAddRecipients(msgCompFields, "addr_reply", newReplyTo);
           }
 
+          let toAddrs = new Set(msgCompFields.splitRecipients(
+            msgCompFields.to, true, {}));
+          let ccAddrs = new Set(msgCompFields.splitRecipients(
+            msgCompFields.cc, true, {}));
+          let bccAddrs = new Set(msgCompFields.splitRecipients(
+            msgCompFields.bcc, true, {}));
+
           if (newCc != prevCc)
           {
             needToCleanUp = true;
             if (prevCc != "")
               awRemoveRecipients(msgCompFields, "addr_cc", prevCc);
-            if (newCc != "")
+            if (newCc != "") {
+              // Ensure none of the Ccs are already in To.
+              let cc2 = msgCompFields.splitRecipients(newCc, true, {});
+              for (let i = cc2.length - 1; i >= 0; i--) {
+                if (toAddrs.has(cc2[i])) {
+                  cc2.splice(i, 1);
+                }
+              }
+              newCc = cc2.join(", ");
               awAddRecipients(msgCompFields, "addr_cc", newCc);
+            }
           }
 
           if (newBcc != prevBcc)
           {
             needToCleanUp = true;
             if (prevBcc != "")
               awRemoveRecipients(msgCompFields, "addr_bcc", prevBcc);
-            if (newBcc != "")
+            if (newBcc != "") {
+              // Ensure none of the Bccs are already in To or Cc.
+              let bcc2 = msgCompFields.splitRecipients(newBcc, true, {});
+              for (let i = bcc2.length - 1; i >= 0; i--) {
+                if (toAddrs.has(bcc2[i]) || ccAddrs.has(bcc2[i])) {
+                  bcc2.splice(i, 1);
+                }
+              }
+              newBcc = bcc2.join(", ");
               awAddRecipients(msgCompFields, "addr_bcc", newBcc);
+            }
           }
 
           if (needToCleanUp)
             awCleanupRows();
 
           try {
             gMsgCompose.identity = gCurrentIdentity;
           } catch (ex) { dump("### Cannot change the identity: " + ex + "\n");}
--- a/mail/test/mozmill/composition/test-reply-addresses.js
+++ b/mail/test/mozmill/composition/test-reply-addresses.js
@@ -18,16 +18,17 @@ var MODULE_REQUIRES = ["folder-display-h
 
 var folder;
 var i = 0;
 
 var myEmail = "me@example.com";
 var myEmail2 = "otherme@example.com";
 
 var identity;
+var identity2;
 
 Cu.import("resource:///modules/mailServices.js");
 
 function setupModule(module) {
   collector.getModule("folder-display-helpers").installInto(module);
   collector.getModule("window-helpers").installInto(module);
   collector.getModule("compose-helpers").installInto(module);
 
@@ -40,17 +41,17 @@ function setupModule(module) {
   folder = account.incomingServer.rootFolder
                   .QueryInterface(Ci.nsIMsgLocalMailFolder)
                   .createLocalSubfolder("Msgs4Reply");
 
   identity = acctMgr.createIdentity();
   identity.email = myEmail;
   account.addIdentity(identity);
 
-  let identity2 = acctMgr.createIdentity();
+  identity2 = acctMgr.createIdentity();
   identity2.email = myEmail2;
   account.addIdentity(identity2);
 
   // Let's add messages to the folder later as we go, it's hard to read
   // out of context what the expected results should be.
 }
 
 /**
@@ -77,16 +78,18 @@ function checkToAddresses(replyWinContro
     let addrTypePopup = addressingWidgetItems[i].querySelector("menupopup");
     let addrTextbox = addressingWidgetItems[i].querySelector("textbox");
 
     let selectedIndex = addrTypePopup.parentNode.selectedIndex;
     let typeMenuitems = addrTypePopup.childNodes;
     let addrType = (selectedIndex != -1) ?
       typeMenuitems[selectedIndex].value : typeMenuitems[0].value;
 
+    if (!addrTextbox.value)
+      continue;
     let addresses = obtainedFields[addrType];
     if (addresses)
       addresses.push(addrTextbox.value);
     else
       addresses = [addrTextbox.value];
     obtainedFields[addrType] = addresses;
   }
 
@@ -757,69 +760,69 @@ function testReplyToSelfNotOriginalSourc
     }
   });
   add_message_to_folder(folder, msg0);
 
   be_in_folder(folder);
   let msg = select_click_row(i++);
   assert_selected_and_displayed(mc, msg);
 
-  ensureNoAutoCc(identity);
-  useAutoBcc(identity, myEmail + ", smithers@example.com");
+  ensureNoAutoCc(identity2);
+  useAutoBcc(identity2, myEmail + ", smithers@example.com");
   checkReply(
      open_compose_with_reply_to_all,
     // To: original To
     // Cc: original Cc
     // Bcc: auto-bccs
     // Reply-To: original Reply-To
     {
       "addr_to": ["Bart <bart@example.com>",
                   "Maggie <maggie@example.com>"],
       "addr_cc": ["Lisa <lisa@example.com>"],
       "addr_bcc": [myEmail, "smithers@example.com"],
       "addr_reply": ["Flanders <flanders@example.com>"]
     }
   );
-  stopUsingAutoBcc(identity);
+  stopUsingAutoBcc(identity2);
 
-  useAutoCc(identity, myEmail + ", smithers@example.com");
-  useAutoBcc(identity, "moe@example.com,bart@example.com,lisa@example.com");
+  useAutoCc(identity2, myEmail + ", smithers@example.com");
+  useAutoBcc(identity2, "moe@example.com,bart@example.com,lisa@example.com");
   checkReply(
     open_compose_with_reply_to_all,
     // To: original To
     // Cc: original Cc (auto-Ccs would have been included here already)
     // Bcc: auto-bcc minus addressess already in To/Cc
     // Reply-To: original Reply-To
     {
       "addr_to": ["Bart <bart@example.com>",
                   "Maggie <maggie@example.com>"],
-      "addr_cc": ["Lisa <lisa@example.com>"],
+      "addr_cc": ["Lisa <lisa@example.com>", myEmail, "smithers@example.com"],
       "addr_bcc": ["moe@example.com"],
       "addr_reply": ["Flanders <flanders@example.com>"]
     }
   );
-  stopUsingAutoCc(identity);
-  stopUsingAutoBcc(identity);
+  stopUsingAutoCc(identity2);
+  stopUsingAutoBcc(identity2);
 
-  useAutoBcc(identity, myEmail2 + ", smithers@example.com");
+  useAutoBcc(identity2, myEmail2 + ", smithers@example.com");
   checkReply(
     open_compose_with_reply_to_all,
     // To: original To
     // Cc: original Cc (auto-Ccs would have been included here already)
     // Bcc: auto-bccs
     // Reply-To: original Reply-To
     {
       "addr_to": ["Bart <bart@example.com>",
                   "Maggie <maggie@example.com>"],
       "addr_cc": ["Lisa <lisa@example.com>"],
       "addr_bcc": [myEmail2, "smithers@example.com"],
       "addr_reply": ["Flanders <flanders@example.com>"]
     }
   );
-  stopUsingAutoBcc(identity);
+  stopUsingAutoBcc(identity2);
 }
 
 /**
  * Tests that a reply to an other identity isn't treated as a reply to self
  * followup.
  */
 function testReplyToOtherIdentity() {
   let msg0 = create_message({
@@ -832,17 +835,18 @@ function testReplyToOtherIdentity() {
     }
   });
   add_message_to_folder(folder, msg0);
 
   be_in_folder(folder);
   let msg = select_click_row(i++);
   assert_selected_and_displayed(mc, msg);
 
-  ensureNoAutoCc(identity);
+  ensureNoAutoCc(identity2);
+  ensureNoAutoBcc(identity2);
   checkReply(
     open_compose_with_reply_to_all,
     // To: from + to (except me2)
     // Cc: original Cc
     //
     {
       "addr_to": ["secretary@example.com", "barney@example.com"],
       "addr_cc": ["Lisa <lisa@example.com>"]
--- a/mail/test/mozmill/message-header/test-reply-identity.js
+++ b/mail/test/mozmill/message-header/test-reply-identity.js
@@ -1,16 +1,18 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /**
  * Tests that actions such as replying choses the most suitable identity.
  */
 
+// make SOLO_TEST=message-header/test-reply-identity.js mozmill-one
+
 var MODULE_NAME = "test-reply-identity";
 
 var RELATIVE_ROOT = "../shared-modules";
 var MODULE_REQUIRES = ["folder-display-helpers",
                          "window-helpers", "compose-helpers"];
 
 var folderHelper = null;
 var windowindowHelperelper = null;
@@ -26,60 +28,71 @@ var setupModule = function (module) {
   folderHelper = collector.getModule("folder-display-helpers");
   folderHelper.installInto(module);
   windowHelper = collector.getModule("window-helpers");
   windowHelper.installInto(module);
   composeHelper = collector.getModule("compose-helpers");
   composeHelper.installInto(module);
 
   addIdentitiesAndFolder();
+  // Msg #0
   add_message_to_folder(testFolder, create_message({
     from: "Homer <homer@example.com>",
     to: "workers@springfield.invalid",
     subject: "no matching identity, like bcc/list",
     body: {body: "Alcohol is a way of life, alcohol is my way of life, and I aim to keep it."},
     clobberHeaders: {
     }
   }));
+  // Msg #1
   add_message_to_folder(testFolder, create_message({
     from: "Homer <homer@example.com>",
     to: "powerplant-workers@springfield.invalid",
     subject: "only delivered-to header matching identity",
     body: {body: "Just because I don't care doesn't mean I don't understand."},
     clobberHeaders: {
       "Delivered-To" : "<" + identity2Email + ">"
     }
   }));
+  // Msg #2
   add_message_to_folder(testFolder, create_message({
     from: "Homer <homer@example.com>",
     to: "powerplant-workers@springfield.invalid, Apu <apu@test.invalid>",
     cc: "other." + identity2Email,
     subject: "subpart of cc address matching identity",
     body: {body: "Blame the guy who doesn't speak Engish."},
     clobberHeaders: {
     }
   }));
+  // Msg #3
   add_message_to_folder(testFolder, create_message({
     from: "Homer <homer@example.com>",
     to: "Lenny <" + identity2Email + ">",
     subject: "normal to:address match, with full name",
     body: {body: "Remember as far as anyone knows, we're a nice normal family."}
   }));
+  // Msg #4
   add_message_to_folder(testFolder, create_message({
-    from: ["Homer", "homer@example.com"],
+    from: "Homer <homer@example.com>",
     to: "powerplant-workers@springfield.invalid",
     subject: "delivered-to header matching only subpart of identity email",
     body: {body: "Mmmm...Forbidden donut"},
     clobberHeaders: {
       "Delivered-To" : "<other." + identity2Email + ">"
     }
   }));
+  // Msg #5
+  add_message_to_folder(testFolder, create_message({
+    from: identity2Email + " <" + identity2Email+ ">",
+    to: "Marge <marge@example.com>",
+    subject: "from second self",
+    body: {body: "All my life I've had one dream, to achieve my many goals."}
+  }));
 }
 
-
 var addIdentitiesAndFolder = function() {
   let server = MailServices.accounts.createIncomingServer("nobody",
                                                           "Reply Identity Testing", "pop3");
   testFolder = server.rootFolder.QueryInterface(Ci.nsIMsgLocalMailFolder)
                      .createLocalSubfolder("Replies");
 
   let identity = MailServices.accounts.createIdentity();
   identity.email = identity1Email;
@@ -156,8 +169,24 @@ function test_deliveredto_to_matching_on
   let msg = select_click_row(4);
   assert_selected_and_displayed(mc, msg);
 
   let replyWin = composeHelper.open_compose_with_reply();
   // Should have selected the (default) first id.
   checkReply(replyWin, identity1Email);
   close_compose_window(replyWin);
 }
+
+/**
+ * A reply from self is treated as a follow-up. And this self
+ * was the second identity, so the reply should also be from the second identity.
+ */
+function test_reply_to_self_second_id() {
+  be_in_folder(testFolder);
+
+  let msg = select_click_row(5);
+  assert_selected_and_displayed(mc, msg);
+
+  let replyWin = composeHelper.open_compose_with_reply();
+  // Should have selected the second id, which was in From.
+  checkReply(replyWin, identity2Email);
+  close_compose_window(replyWin, false /*no prompt*/);
+}
--- a/mailnews/compose/src/nsMsgCompose.cpp
+++ b/mailnews/compose/src/nsMsgCompose.cpp
@@ -72,21 +72,23 @@
 #include "nsIMutableArray.h"
 #include "nsArrayUtils.h"
 #include "nsIMsgWindow.h"
 #include "nsITextToSubURI.h"
 #include "nsIAbManager.h"
 #include "nsCRT.h"
 #include "mozilla/Services.h"
 #include "mozilla/mailnews/MimeHeaderParser.h"
+#include "mozilla/Preferences.h"
 #include "nsStreamConverter.h"
 #include "nsISelection.h"
 #include "nsJSEnvironment.h"
 #include "nsIObserverService.h"
 
+using namespace mozilla;
 using namespace mozilla::mailnews;
 
 static nsresult GetReplyHeaderInfo(int32_t* reply_header_type,
                                    nsString& reply_header_locale,
                                    nsString& reply_header_authorwrote,
                                    nsString& reply_header_ondateauthorwrote,
                                    nsString& reply_header_authorwroteondate,
                                    nsString& reply_header_originalmessage)
@@ -2573,31 +2575,34 @@ NS_IMETHODIMP QuotingOutputStreamListene
           rv = msgFolder->GetServer(getter_AddRefs(nsIMsgIncomingServer));
 
           if (NS_SUCCEEDED(rv) && nsIMsgIncomingServer)
             accountManager->GetIdentitiesForServer(nsIMsgIncomingServer, getter_AddRefs(identities));
         }
       }
 
       bool isReplyToSelf = false;
+      nsCOMPtr<nsIMsgIdentity> selfIdentity;
       if (identities)
       {
         // Go through the identities to see if any of them is the author of
         // the email.
         nsCOMPtr<nsIMsgIdentity> lookupIdentity;
 
         uint32_t count = 0;
         identities->GetLength(&count);
 
         for (uint32_t i = 0; i < count; i++)
         {
           lookupIdentity = do_QueryElementAt(identities, i, &rv);
           if (NS_FAILED(rv))
             continue;
 
+          selfIdentity = lookupIdentity;
+
           nsCString curIdentityEmail;
           lookupIdentity->GetEmail(curIdentityEmail);
 
           // See if it's a reply to own message, but not a reply between identities.
           if (curIdentityEmail.Equals(fromEmailAddress))
           {
             isReplyToSelf = true;
             // For a true reply-to-self, none of your identities are normally in
@@ -2633,21 +2638,25 @@ NS_IMETHODIMP QuotingOutputStreamListene
                 isReplyToSelf = false;
                 break;
               }
             }
             break;
           }
         }
       }
-
       if (type == nsIMsgCompType::ReplyToSender || type == nsIMsgCompType::Reply)
       {
         if (isReplyToSelf)
         {
+          // Cast to concrete class. We *only* what to change m_identity, not
+          // all the things compose->SetIdentity would do.
+          nsMsgCompose* _compose = static_cast<nsMsgCompose*>(compose.get());
+          _compose->m_identity = selfIdentity;
+          compFields->SetFrom(from);
           compFields->SetTo(to);
           compFields->SetReplyTo(replyTo);
         }
         else if (!mailReplyTo.IsEmpty())
         {
           // handle Mail-Reply-To (http://cr.yp.to/proto/replyto.html)
           compFields->SetTo(mailReplyTo);
           needToRemoveDup = true;
@@ -2661,16 +2670,21 @@ NS_IMETHODIMP QuotingOutputStreamListene
         else {
           compFields->SetTo(from);
         }
       }
       else if (type == nsIMsgCompType::ReplyAll)
       {
         if (isReplyToSelf)
         {
+          // Cast to concrete class. We *only* what to change m_identity, not
+          // all the things compose->SetIdentity would do.
+          nsMsgCompose* _compose = static_cast<nsMsgCompose*>(compose.get());
+          _compose->m_identity = selfIdentity;
+          compFields->SetFrom(from);
           compFields->SetTo(to);
           compFields->SetCc(cc);
           // In case it's a reply to self, but it's not the actual source of the
           // sent message, then we won't know the Bcc header. So set it only if
           // it's not empty. If you have auto-bcc and removed the auto-bcc for
           // the original mail, you will have to do it manually for this reply
           // too.
           if (!bcc.IsEmpty())