Reply to list: automatically determine From: address

RESOLVED FIXED in Thunderbird 3.3a1

Status

--
enhancement
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: tessarakt, Assigned: kza3)

Tracking

(Depends on: 1 bug, {student-project})

unspecified
Thunderbird 3.3a1
student-project
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: good first bug, ucosp)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.0.3) Gecko/2008092813 Gentoo Firefox/3.0.3
Build Identifier: version 2.0.0.17 (20081022)

I installed the ReplyToList add-on, which adds the functionality asked for in Bug 45715.

However, I am missing one thing: That Thunderbird automatically figures out which From: address to use (so that the mail gets through to the list). I think this feature works for normal mail (by looking at the To: header, or whatever), right? For mailing list posts there are probably various headers that might be helpful ...

Reproducible: Always
This is a bug pertaining to an extension. Such bugs are not tracked here; please contact the extension author instead.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → INVALID
(Reporter)

Comment 2

10 years ago
No, it's not - I mentioned the extension just for illustration (sorry that I didn't make that clear). 

This is a followup to bug 45715 (on which it thus should depend).
(Reporter)

Updated

10 years ago
Status: RESOLVED → UNCONFIRMED
Depends on: 45715
Resolution: INVALID → ---
I'd love to see this done.  I have a couple identities and find that TB doesn't handle the reply list correctly.

Jens, if you have some time I don't think this would be too difficult to do.

With some modification to the getIdentityForHeader() function you could improve the default choice.
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#120

That function is called from ComposeMessage() and passed the type + mail header
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#164

Add a check for (type == Components.interfaces.nsIMsgCompType.ReplyToList) and then maybe pass in a better hint to the identity searching functions used.

e.g.
getIdentityForServer(server, hintForIdentity)
getBestIdentity(allIdentities, hintForIdentity)

As far as I can tell from my list messages you'd be looking to pass in the 'Delivered-To' header for your hint; though I believe there can be multiples so you might have to loop the call. 

AFAIK this shouldn't be too difficult, also this might make a good student project.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: student-project
Whiteboard: good first bug

Comment 4

9 years ago
Can this bug be reproduced without the ReplyToList add-on?
(Reporter)

Comment 5

9 years ago
(In reply to comment #4)
> Can this bug be reproduced without the ReplyToList add-on?

Now that "Reply list" is in TB3 without any add-on: Yes, sure.

Btw: It's not a bug, but an enhancement request.
Created attachment 411787 [details] [diff] [review]
not working WIP

This was harder than expected because you don't have the current headers of a message easily available and the getBestIdentity function looks for a hint string, instead of some kind of array of identities; a bit unexpected.  Also the getIdentityForHeaders function seems to be called when a message is displayed, not only during compose time so likely it has to be faster than calling out to the MimeMessage parser every time.  I was hoping that meant I'd have access to the currentHeaders data but it doesn't appear that way.

Might give this another try some other time but the fundamental issue seems to remain that multiple identities can be found in the Delivered-To.

Lets say I have an email address:
clarkbw@example.com

And a list identity (where all my mailing lists are sent to) of:
lists@example.com

My mailing lists messages could have the following headers:

Delivered-To: clarkbw@example.com
Delivered-To: lists@example.com
Delivered-To: list@lists
From: joe@lists
To: list@lists

The only path I have right now is to look at the extra identities first and hope to find them in the hint string.  Then check the default identity as the fallback.  This would likely fail if your mail bounced through more than one identity but would at least succeed if you have a default identity and a few others you use independently.

Comment 7

9 years ago
Are you going to work on a patch for the enhancement?
I think we're going to need the big guns here as I believe this change is more complicated than I had previously anticipated.  My patch is a start but this smells a lot like the Reply All Toggle in compose mode bug so I'm adding blake in.
(Assignee)

Comment 9

9 years ago
Trying to reproduce this bug. However, does this bug still fit in the "student-project" category?
Well, let's see how far you get on it.  :)

Thanks,
Blake.
Assignee: nobody → kza3
Status: NEW → ASSIGNED
Whiteboard: good first bug → good first bug, ucosp
(Assignee)

Comment 11

9 years ago
Hi. I am a little bit confused by this bug. As Jens mentioned, Thunderbird should automatically figures out which From: address to use in the “reply to list” feature. Actually, what I am not sure is which From address should be the correct one to use? There are multiple scenarios regarding to this bug:

1.	As far as I know, TB can know which account an email is associated with. Can we directly use this account as the “From” address no matter how many “Delivered-To” headers are found?
For example, I have an email in account 1 inbox. If I click reply, the “From” address will be account 1. However, if I drag this email into my account 2 inbox and click reply, the “From” address should be account 2, right? (this is what TB is currently doing)


2.	If I save an email as *.eml file and load it again, TB will has no idea which account it is associated with. In this way, “Delivered-To” and some other headers are used to decide which “From” address to use after clicking “reply”. As a result, I guess this “reply-yo-list” bug is talking about a saved email?

Please correct me if I am wrong.
(Reporter)

Comment 12

9 years ago
I am surely not the only person who gets mail addressed to different e-mail addresses into the same e-mail account. I have several of them configured as identities of my main account. This bug is e.g. about choosing the correct identity.
There's a bug for a per-folder setting of the From address. (I can't find it, though.)
This would help with mailing lists (without guessing, but manual user work), under the assumption that people typically filter mailing lists into a separate folder and can set their posting address as From on that folder.
(Assignee)

Comment 14

9 years ago
Created attachment 432956 [details] [diff] [review]
First patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

First patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)
Attachment #432956 - Flags: review?(bwinton)
Comment on attachment 432956 [details] [diff] [review]
First patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

>+++ b/mail/base/content/mailCommands.js
>@@ -32,25 +32,32 @@
>+Components.utils.import("resource://app/modules/gloda/mimemsg.js");

So, you import this, but I can't see where you use it.

>+  //dump("in getBestIdentity\n");
>+  //dump("optionalHint: " + optionalHint + "\n");

Instead of commenting these statements out, we should just remove them.

>-
>+  
>+  //dump("\nIdentity count: " + identitiesCount + "\n"); 
>+   

Ditto, and watch out for trailing spaces.

>@@ -94,16 +101,17 @@ function getBestIdentity(identities, opt
>+  dump("Identity returned by getBestIdentity. Identity: " + identity.email + "\n");

We probably want to delete this line, and others like it.  ;)

>@@ -112,48 +120,106 @@ function getIdentityForServer(server, op
>         // dump("identities = " + identities + "\n");
>         // Try and find the best one.
>         identity = getBestIdentity(identities, optionalHint);
>     }
> 
>     return identity;
> }
> 
>-function getIdentityForHeader(hdr, type)
>-{
>-  // If we treat reply from sent specially, do we check for that folder flag here ?
>-  var isTemplate = (type == Components.interfaces.nsIMsgCompType.Template);
>-  var hintForIdentity = isTemplate ? hdr.author : hdr.recipients + hdr.ccList;
>-  var identity = null;
>-  var server;
>+function getIdentityForHeader(hdr, type)

I'm seeing a lot of ^Ms at the end of this function.  Could you make sure
you save it with Unix line-endings?

>+{
>+  // If we treat reply from sent specially, do we check for that folder flag here ?
>+  var isTemplate = (type == Components.interfaces.nsIMsgCompType.Template);
>+  var isReplyToList = (type == Components.interfaces.nsIMsgCompType.ReplyToList);

Since you only use these once each, in an if test, I think you can inline
these variables.

>+  var server;
>+  var hintForIdentity;

I think we should change these to "let"s.

>+  //dump("entering getIdentityForHeader\n");
>+  //dump("hdr.folder:\n");
>+  
>+  if(isReplyToList)
>+  {

The coding standard we use puts the {s on the same line as the if (or
while, or for), and a space between the if and the (.

(But, the closing } is on a different line than the else.)

>+    var index = 0;
>+    var key = "delivered-to";
>+    var allIdentities = accountManager.allIdentities;
>+    
>+    var tempIdentity = "";
>+    
>+    // get the last "delivered-to" that is in the defined identities
>+    while((tempIdentity = currentHeaderData[key]) != undefined)
>+    {
>+        for each (var tempID in fixIterator(allIdentities,
>+                                          Components.interfaces.nsIMsgIdentity)) 
>+        {
>+            dump("\ncurrent identity in tb: " + tempID.email + "\n");
>+            if(tempIdentity.headerValue.toLowerCase().indexOf(tempID.email.toLowerCase()) >=0 )

indexOf is defined to return -1 if it's not found, so we should probably
check that the return value is != -1 instead of >= 0.

>+            {
>+                hintForIdentity = tempID.email;
>+                dump("looping for identity: " + hintForIdentity);
>+                break;
>+            }                                                                           
So, you know that this only breaks out of the for loop, right?

Maybe it would be a better idea to loop through all the headers in
currentHeaderData, extract the ones that start with "delivered-to", sort
and reverse that list, then go through it, and pick the first one with an
identity.

>+        }
>+        
>+        key = String("delivered-to" + index++);

I don't think you need to wrap this in a call to "String"…

>+  if (server && !identity)
>+  {
>+    dump("getIdentityForServer\n");
>+    identity = getIdentityForServer(server, hintForIdentity);
>+  }

If you take out the dump statement, you can also take out the { and }s.

>+++ b/mail/test/mozmill/message-header/test-reply-to-list-from-address-selection.js
>@@ -0,0 +1,196 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging, Inc.

I've been told that this should be "The Mozilla Foundation".

>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Blake Winton <bwinton@latte.ca>
>+ *   Dan Mosedale <dmose@mozillamessaging.com>

I don't remember writing anything in this file…  ;)
(You should probably remove Dan's and my names, and add in yours instead.

>+/*
>+ * Test that TB can automatically select the "From" address in the "reply-to-list" mode.
>+ */

You could wrap that comment so that it wasn't longer than 80 characters.

>+var MODULE_NAME = 'test-message-header';

I think you want to pick a different name for this module.

>+var RELATIVE_ROOT = '../shared-modules';
>+var MODULE_REQUIRES = ['folder-display-helpers', 'window-helpers', 'compose-helpers'];

This line can also wrap.

>+var addMessageToFolder = function(aFolder){
>+  var msgId = Components.classes["@mozilla.org/uuid-generator;1"].getService(Components.interfaces.nsIUUIDGenerator).generateUUID() +"@mozillamessaging.invalid";
>+
>+   let source = "From - Sat Nov  1 12:39:54 2008\n" +

Three-space indent, eh?  That's a curious choice.  ;)

>+function addIdentities() {
>+  // this doesn't work

I think you should remove it if you got something else working.
Also, I wonder if you needed to call this before you called any
AccountManager stuff.  I know Bienvenu had some test code that did
something extremely similar, and it worked.

>+function test_addFolderAndMailAndIdentity(){
>+
>+  //mc.sleep(20000);
>+  
>+  dump("\nbefore add identites\n");
>+  //mc.sleep(3000);
>+  addIdentities();
>+  dump("\nafter add identites\n");
>+  
>+  addMessageToFolder(testFolder);  
>+}
>+
>+function test_Reply_To_List(){
>+  be_in_folder(testFolder);
>+    
>+  dump("\n\nbefore open the message\n\n");
>+
>+  select_click_row(0);
>+  plan_for_new_window("mail:messageWindow");
>+  open_selected_message();
>+  
>+  dump("\n\nbefore clicking reply to list\n\n");
>+  replyToListWindow = composeHelper.open_compose_with_reply_to_list();
>+}

I don't really like it when one test depends on another running before it.
Perhaps you could just have this be one test that adds the identities, and
then the message, and then checks the reply-to-list.  Or put the identities
and message adding code in setupModule, maybe, and open and close the
replyToListWindow in each test.

Also, neither of the above two things are really tests, I don't think.

>+function test_From_Address()
>+{ 
>+   var identityList = replyToListWindow.e("msgIdentity");
>+   dump("\nselected identity: " + identityList.selectedItem.label + "\n");
>+   
>+   // see if it's the correct identity selected
>+   
>+   if(identityList.selectedItem.label.indexOf(identityString1) < 0)
>+        throw new Error("The From address is not correctly selected! Expected: " + identityString1 + "; Actual: " + identityList.selectedItem.label);

I'm sure you can break this up into more than one line of less than 80
characters each.  ;)

>+   else
>+       dump("Correct identity selected!");
>+}

So, I'm going to give it an r-, mainly because I want to see the changes
before you ask someone else for a review, but I like the direction you're
going, and think that most of my complaints are stylistic things.

Later,
Blake.
Attachment #432956 - Flags: review?(bwinton) → review-
(Assignee)

Comment 16

9 years ago
Created attachment 434799 [details] [diff] [review]
Revised Patch
Attachment #432956 - Attachment is obsolete: true
Attachment #434799 - Flags: review?(bwinton)
Comment on attachment 434799 [details] [diff] [review]
Revised Patch

>+++ b/mail/base/content/mailCommands.js
>@@ -114,52 +114,90 @@ function 
>+  // If we treat reply from sent specially, do we check for that fol
>+  // der flag here ?

You shouldn't really split words across lines like that.  ;)  (Just move all of "folder" to the next line.  And get rid of the space before the ?, while you're changing stuff.)

>+    while((tempIdentity = currentHeaderData[key])!=undefined) {

We need spaces around the "!=".

>+    // reverse the array so that the last delivered-to header will show at front

We use full sentences for our comments, with capital letters at the start, and periods at the end.  :)

>+    let identityFound = false;
>+    for (let i=0; i<deliveredTos.length; i++) {
>+        for each (var tempID in fixIterator(allIdentities,

Two space indents, please.

>+++ b/mail/base/content/msgHdrViewOverlay.js
>@@ -471,17 +471,18 @@ var messageHeaderSink = {
>+          else if (lowerCaseHeaderName == "delivered-to")
>+            this.mDummyMsgHeader.deliveredTo = header.headerValue;          

You've got a lot of extra space at the end of this line.

>+++ b/mail/test/mozmill/message-header/test-reply-to-list-from-address-selection.js
>@@ -0,0 +1,146 @@
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation, Inc.

Hmm…  I don't know if the Mozilla Foundation is incorporated.  Maybe just leave it as "The Mozilla Foundation." instead.

There are also some lines with trailing spaces in this file which should be removed.

>+  var server = acctMgr.createIncomingServer("nobody", 
>+                             "Test Local Folders", "pop3");

The "Test Local Folders" should line up with the "nobody" on the previous line.

So, in summary, there's some cleanup for you to do, but no major changes, so you get an r+ from me.  Now you need to set the "addl. review" flag down below to ? to get your second review.

Good luck,
Blake.
(Assignee)

Comment 18

9 years ago
Created attachment 437632 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)
Attachment #434799 - Attachment is obsolete: true
Attachment #434799 - Flags: review?(bwinton)
(Assignee)

Updated

9 years ago
Attachment #437632 - Flags: review?(bugzilla)
Comment on attachment 437632 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

I'm a bit busy at the moment, I'm going to pass this to Magnus to see if he has time to review. It'll need superreview once it has review, so feel free to come back to me for that.
Attachment #437632 - Flags: review?(bugzilla) → review?(mkmelin+mozilla)
Comment on attachment 437632 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)


>+  let server="";
>+  let hintForIdentity="";
>+
>+  if(type == Components.interfaces.nsIMsgCompType.ReplyToList) {

Style nit; always put space around = and for if/else and other clauses. There are a lot of those, but i'll only comment this one. So, like

let server = ""
let hintForIdentity = ""

if (type == ....)

Please add a code comment about what this block is trying to achieve.

>+    var index = 0;

Please be consistent with var/let. Only use var if you need it.

>+    var key = "delivered-to";
>+    var allIdentities = accountManager.allIdentities;
>+
>+    var tempIdentity = "";
>+
>+    // Get the delivered-to headers.
>+    var deliveredTos = new Array();
>+    while((tempIdentity = currentHeaderData[key]) != undefined) {

You should be able to write just
while (tempIdentity = currentHeaderData[key])

>+    for (let i=0; i<deliveredTos.length; i++) {
>+      for each (var tempID in fixIterator(allIdentities,
>+                Components.interfaces.nsIMsgIdentity)) {
>+        if(deliveredTos[i].indexOf(tempID.email.toLowerCase())!=-1) {
>+          hintForIdentity = tempID.email;
>+          identityFound = true;
>+          break;
>+        }
>+      }
>+      if(identityFound)
>+        break;

You don't need the identityFound variable. Just check |if (hintForIdentity)|



>+ * The Original Code is Thunderbird Mail Client.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation.

You?

>+ * Portions created by the Initial Developer are Copyright (C) 2009

2010?


>+/*
>+ * Test that TB can automatically select the "From" address 
>+ * in the "reply-to-list" mode.
>+ */

"the most suitable identity From address for reply-to-list"

>+var MODULE_NAME = 'test-reply-to-list-from-address-selection';

Minor nit: for code consitency it's preferable to use double quotes except when when you need single quotes.


>+var addMessageToFolder = function (aFolder) {
>+  var msgId = Components.classes["@mozilla.org/uuid-generator;1"]
>+              .getService(Components.interfaces.nsIUUIDGenerator).generateUUID()+


We typlically line these up as
 Components.classes
           .getService

..., but then again. Cc is defined here.

>+  dump("\nSelected identity: " + identityList.selectedItem.label + "\n");

Remove this and the other dump.

>\ No newline at end of file

Whops.

Other than those, seems to work fine.
(Assignee)

Comment 21

9 years ago
Created attachment 439744 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

Hi Magnus,

Thank you so much for reviewing my patch. 
I have created a new patch which takes your suggestions. Please give it a look when you have time. 

Thanks again.

Kefu
Attachment #439744 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

9 years ago
Attachment #437632 - Attachment is obsolete: true
Attachment #437632 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

9 years ago
Attachment #439744 - Flags: superreview?(bugzilla)
Comment on attachment 411787 [details] [diff] [review]
not working WIP

(Marking this obsolete, cause Kefu seems to have a better patch.)
Attachment #411787 - Attachment is obsolete: true
Comment on attachment 439744 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

David knows a bit more about how libmime is working there, so I'm going to let him sr it.
Attachment #439744 - Flags: superreview?(bugzilla) → superreview?(bienvenu)
(In reply to comment #21)
> I have created a new patch which takes your suggestions. Please give it a look

You only addressed part of them
For instance "if(" should always be "if (", and you still mix "var" and "let" oddly, there's still a dump statement. 
Also there's a few lines with spaces on empty lines and spaces at the end of some lines - please remove those.
(Assignee)

Comment 25

9 years ago
Created attachment 440022 [details] [diff] [review]
Revised Patch with all the "dump" removed

I am sorry that I didn't clean up the code thoroughly last time, but here is my revised patch with all the "dump" statement removed. Also, some extra space at the end of some lines are removed.
Attachment #439744 - Attachment is obsolete: true
Attachment #440022 - Flags: superreview?(bienvenu)
Attachment #440022 - Flags: review?(mkmelin+mozilla)
Attachment #439744 - Flags: superreview?(bienvenu)
Attachment #439744 - Flags: review?(mkmelin+mozilla)
Comment on attachment 440022 [details] [diff] [review]
Revised Patch with all the "dump" removed

Looks good, thx! r=mkmelin
Attachment #440022 - Flags: review?(mkmelin+mozilla) → review+
Minor nit: i<deliveredTos.length should be i < deliveredTos.length

Comment 28

9 years ago
Comment on attachment 440022 [details] [diff] [review]
Revised Patch with all the "dump" removed

sr=me for the mime changel
Attachment #440022 - Flags: superreview?(bienvenu) → superreview+
(In reply to comment #27)
> Minor nit: i<deliveredTos.length should be i < deliveredTos.length

Kefu can you provide an updated patch with that change ?
(Assignee)

Comment 30

9 years ago
Created attachment 446946 [details] [diff] [review]
checkin-needed. Final patch with the nits fixed

Hey guys, I am sorry for the late reply, but here is the final patch with the nits fixed.
Attachment #440022 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/8d62b8a94643

Thanks Kefu.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
Depends on: 581921

Updated

8 years ago
Blocks: 654009

Comment 32

8 years ago
Could this be made configurable? In my case the last delivered-to entry that matches any of my identities is due to mail forwarding from my "official" address to my mail storage provider.

I want to reply to the mailing list from the "official" address which is registered and has posting privileges. This one is also my default identity and I don't want to pick that one each time manually (and half the times I forget and need to cancel moderated posts...)

I think, the implemented approach is too heuristical to work in all circumstances, so it should be optional.
Depends on: 675598

Comment 33

7 years ago
This "enhancement" seriously affects my user experience. Could you please reopen and reconsider it?
(In reply to Tilman Vogel from comment #33)
> This "enhancement" seriously affects my user experience. Could you please
> reopen and reconsider it?

I think the best you could do is file another "enhancement" bug mentioning this one, to make the behaviour configurable. But you should prepare very convincing and detailed arguments if you don't want it to be resolved WONTFIX.

Comment 35

7 years ago
(In reply to Tony Mechelynck [:tonymec] from comment #34)
> I think the best you could do is file another "enhancement" bug mentioning
> this one, to make the behaviour configurable. But you should prepare very
> convincing and detailed arguments if you don't want it to be resolved
> WONTFIX.

Have you read my comment #32? Is it plausible that this enhancement does not work as intended in my case and that it affects the usability of Thunderbird with mailing lists and redirected accounts in general?
Tilman: That's possible, but it's better to create a new bug for discussing and finding a resolution to your problem. Please cc on the bug.

Comment 37

7 years ago
I just opened bug 679295. How can I cc this bug?
Depends on: 679295
Typo, I meant "please cc me" :)
Duplicate of this bug: 230247
You need to log in before you can comment on or make changes to this bug.