798282 - When reply all to self, automatic BCC addresses are not added

Status: RESOLVED INVALID

(Thunderbird :: Message Compose Window, defect)

Description

[Joachim Herb]

This was found during working on bug 525070 comment 20 and bug 525070 comment 45:

I ran the mozmill tests of the latest patch with the current Thunderbird release (14.0) and also with the current beta (15.0b4) and Earlybird (16.0a2). All produce the same error for the last test case: 
Reply to a mail which included bcc addresses (e.g. from the sent folder, so the bcc addresses were saved) from an account with automatic sending mails to bcc addresses. Then these automatic bcc addresses are not added to the mail, when reply to all is clicked/selected.

And by switching to another account and then back again, the missing automatic bcc address(es) are added.

Comment 1

[Joachim Herb]

Attachment: First fix plus mozmill tests

This fixes the problem and adds mozmill test(s) to confirm. It was build on the try server:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=8bd58669c40b

It's a start (and my first C++ patch).

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 668387 [details] [diff] [review]
First fix plus mozmill tests

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

Great work!  Just a few things - nothing major.

Also, I don't think I'm qualified to review the nsMsgCompose.cpp stuff - requesting review from Neil.

::: mail/test/mozmill/composition/test-reply-addresses.js
@@ +68,5 @@
> +    bcc: msgGen.makeNamesAndAddresses(2)});
> +  add_message_to_folder(gFolder2, msg2);
> +};
> +
> +function reply_selected_message_and_go_to_drafts_folder(f) {

Documentation header needed.

@@ +97,5 @@
> +  be_in_folder(draftsFolder);
> +}
> +
> +
> +function test_reply_without_cc_bcc () {

Documentation header regarding what this test is actually testing is needed.

@@ +138,5 @@
> +    "The BCC address \"" + replyBCCAddresses.value.join(',') + "\" of the " +
> +    "replied mail should be empty");
> +
> +  assert_equals(replyFromAddresses.value.join(','), kIdentity1Email,
> +      "The FROM address \"" + replyFromAddresses.value.join(',') + "\" of " +

Nit: Lines 142 - 144 should only be indented 2 spaces.

@@ +143,5 @@
> +      "the replied mail should be the same as the as the account email \"" +
> +      kIdentity1Email + "\"");
> +}
> +
> +function test_reply_with_cc_bcc () {

Documentation header regarding what this test is actually testing is needed.

The preparation of this test (lines 148 - 171) are almost (if not completely) identical to the prep in the last test. Can that be extracted to an external function?

@@ +196,5 @@
> +    kIdentity2Email + "\"");
> +}
> +
> +function test_reply_all_without_cc_bcc () {
> +  be_in_folder(gFolder1);

Documentation header regarding what this test is actually testing is needed.

Same as above wrt extracting this preparation stuff.

@@ +249,5 @@
> +    kIdentity1Email + "\"");
> +}
> +
> +function test_reply_all_with_cc_bcc () {
> +  be_in_folder(gFolder2);

Documentation header regarding what this test is actually testing is needed.

Same as above wrt test prep

@@ +306,5 @@
> +    "replied mail should be the same as the as the account email \"" +
> +    kIdentity2Email + "\"");
> +}
> +
> +function get_msg_addresses(aMsgHdr, aToAddresses, aCCAddresses, aBCCAddresses,

This helper function should go beneath the other helper function on line 72.  It also needs a documentation header.

::: mail/test/mozmill/shared-modules/test-compose-helpers.js
@@ +24,5 @@
>  const kTextNodeType = 3;
>  
>  var folderDisplayHelper;
>  var mc;
>  var windowHelper, domHelper;

Please put these newlines back, for readability.

@@ +92,2 @@
>    return wait_for_compose_window();
>  }

Newline before the comment block starts.

@@ +108,1 @@
>  /**

Newline before comment block.

::: mailnews/test/resources/messageGenerator.js
@@ +559,5 @@
> +   *     header.
> +   */
> +  get bcc() {
> +    return this._bcc;
> +  },

Newline before comment block

@@ +565,5 @@
> +   * Sets the Bcc header using a list of tuples containing [a display name,
> +   *  an e-mail address].
> +   *
> +   * @param aNameAndAddress A list of name-and-address tuples.  Each tuple is a
> +   *     list with two elements.  The first should be the

Weird linebreak here...

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #2)
> Comment on attachment 668387 [details] [diff] [review]
> First fix plus mozmill tests
> 
> Review of attachment 668387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work!  Just a few things - nothing major.
> 
> Also, I don't think I'm qualified to review the nsMsgCompose.cpp stuff -
> requesting review from Neil.

Rather, you should request review from Neil([e-mail]) once you've got another iteration up.

Comment 4

[Joachim Herb]

Attachment: Changes requested by reviewer

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 673733 [details] [diff] [review]
Changes requested by reviewer

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

The tests are looking really good! Just a few more suggestions, but I think I'm pretty happy with it. I'll only review the DRY'ing up you do in test-reply-address.js next time. The rest of it is r+, as far as I'm concerned (except for the nsMsgCompose.cpp stuff, which I, again, am not qualified to review).

::: mail/test/mozmill/composition/test-reply-addresses.js
@@ +113,5 @@
> + */
> +function test_reply_without_cc_bcc () {
> +  be_in_folder(gFolder1);
> +
> +  let origAddresses = get_addresses_from_header(select_click_row(0));

Besides folder that we start in, and the function passed to reply_selected_message_and_go_to_drafts_folder, I'm seeing a lot of similarities between this and the other tests.

Can they not be DRY'd up a little? A helper function to take care of this stuff?

@@ +119,5 @@
> +  let replyAddresses = get_addresses_from_header(select_click_row(0));
> +
> +  press_delete(mc);
> +
> +  assert_equals(replyAddresses.to.join(','),

Since replyAddresses.to is an array, the toString method does the comma-seperated join for us automatically, so you don't need to do it yourself.

::: mail/test/mozmill/shared-modules/test-message-helpers.js
@@ +65,5 @@
> +  let CCAddresses = {};
> +  let BCCAddresses = {};
> +  let FromAddresses = {};
> +
> +  let fullNames = {};

Let's just pass empty objects to parseHeadersWithArray each time instead of aliasing the same ones.

So, let numAddrsTo = MailServices.headerParser.parseHeadersWithArray(
  aMsgHdr.recipients, ToAddresses, {}, {}); will do.

@@ +69,5 @@
> +  let fullNames = {};
> +  let names = {};
> +
> +  let numAddrsTo = MailServices.headerParser.parseHeadersWithArray(
> +  aMsgHdr.recipients, ToAddresses, names, fullNames);

Lines 73, 75, 77, and 79 need proper indentation - 2 spaces please.

@@ +82,5 @@
> +  CCAddresses.value = CCAddresses.value.sort();
> +  BCCAddresses.value = BCCAddresses.value.sort();
> +  FromAddresses.value = FromAddresses.value.sort();
> +
> +  return {to: ToAddresses.value,

Let's just do the sorting inline here.

::: mailnews/test/resources/messageGenerator.js
@@ +417,5 @@
>    },
>  
>    /**
>     * Given a tuple containing [a display name, an e-mail address], returns a
> +   *  string suitable for use in a to/from/cc/bcc header line.

nit: no space before string.

@@ +583,5 @@
> +      return;
> +    }
> +    this._bcc = aNameAndAddresses;
> +    this.headers["Bcc"] = this._commaize(
> +                            [this._formatMailFromNameAndAddress(nameAndAddr)

Let's indent lines 587 and 788 a little less - I think 2 spaces indented from the above line is enough.

@@ +584,5 @@
> +    }
> +    this._bcc = aNameAndAddresses;
> +    this.headers["Bcc"] = this._commaize(
> +                            [this._formatMailFromNameAndAddress(nameAndAddr)
> +                             for each (nameAndAddr in aNameAndAddresses)]);

Of course, this line will need one extra space of indentation since it starts inside square parentheses.

Comment 6

[Joachim Herb]

(In reply to Mike Conley (:mconley) from comment #5)
> Comment on attachment 673733 [details] [diff] [review]
> Changes requested by reviewer
> 
> Review of attachment 673733 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests are looking really good! Just a few more suggestions, but I think
> I'm pretty happy with it. I'll only review the DRY'ing up you do in
> test-reply-address.js next time. The rest of it is r+, as far as I'm
> concerned (except for the nsMsgCompose.cpp stuff, which I, again, am not
> qualified to review).
Should I really do another round of the patch before Neil([e-mail]) reviews the c++ part? Or should I look for another reviewer, because the patch has been waiting for review since 3 weeks?

Comment 7

[Joachim Herb]

The C++ part of the patch is still not reviewed (since more than two month). Should I ask somebody else for review?

Comment 8

[[e-mail]]

Comment on attachment 673733 [details] [diff] [review]
Changes requested by reviewer

First of all let me apologise for not getting around to reviewing your patch earlier. At least I have the excuse of being a volunteer.

Now unfortunately shortly after you created your patch bug 250187 rewrote large swathes of relevant code so I can only comment based on looking at the patch.

>+          compFields->GetBcc(replyBccCompValue);
>+          if (!replyBccCompValue.IsEmpty() && !bcc.IsEmpty())
>+            replyBccCompValue.AppendLiteral(", ");
>+          replyBccCompValue.Append(bcc);
It's unclear what this code is doing, if anything. My best guess is that you should be using bcc instead of replyBccCompValue in your subsequent code.

>+            rv = parser->RemoveDuplicateAddresses(nsDependentCString(_compFields->GetTo()),
>+                                                  myEmail, resultStr);
>+            if (NS_SUCCEEDED(rv))
>+            {
>+              if (type == nsIMsgCompType::ReplyAll && !mailFollowupTo.IsEmpty())
>+                _compFields->SetTo(resultStr.get());
>+            }
This block looks like it was copied as part of the code that cleans up the CC field, but it doesn't belong here. I don't know whether the code would be simpler with separate To/CC/BCC blocks or a single block that tries to do all three headers, but having a mixture of styles is a little confusing.

Comment 9

[[e-mail]]

(In reply to comment #8)
> Now unfortunately shortly after you created your patch
Well, shortly is a relative term... that was simply the next bug to alter that file, although I see it was actually a month later.

Comment 10

[rsx11m]

Per http://forums.mozillazine.org/viewtopic.php?f=29&t=2648633 the "Cc" address isn't added in replies if it matches the replying sender's identity. Is the a different case (i.e., new bug) or related to the issue handled here?

And, why is this bug still "unconfirmed" if it had a patch up for review?  ;-)

Comment 11

[Joachim Herb]

Bug 250187 was the bug, which changed the behavior of the CC field.

Comment 12

[rsx11m]

Thanks.

Comment 13

[Magnus Melin [:mkmelin]]

I'm not sure I'd actually consider this a bug. 
You'd only have the BCC:s in the first place if it's a reply-to-self thing (updating summary). And for that case, it's intended to be a followup preserving all the addresses in the original mail - so the auto-bccs would have been added ORIGINALLY, and only missing if you a) removed them manually, b) added auto-BCC to the account between the original mail and the reply.

We can't really detect case a.

For refrenece: http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-reply-addresses.js#693

Comment 14

[Joachim Herb]

According to the discussion in bug 968270 comment #14 and bug 968270 comment #33 the current behavior of Thunderbird is the intended one and this bug is invalid and will therefore not be fixed.