437903 - Can't send emails to OS X mailing lists.

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[Mitra Ardron]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9) Gecko/2008053008 Firefox/3.0
Build Identifier: version 3.0a2pre (2008060803)

OSX Address Book mailing lists aren't supported.

Reproducible: Always

Steps to Reproduce:
1.Create a list in Mac OSX, add one or more cards to it.
2.Create a new message
3.From Contact sidepane select the list
4.Click "Add to To"
5.Send
Actual Results:  
Error message: testgroup <testgroup> is not a valid e-mail address because it is not of the form user@host. You must correct it before sending the e-mail.

Expected Results:  
Sends to the people in the list !

Comment 1

[Mark Banner (:standard8)]

Yep, I see this, not sure if its a regression or not, but its definitely not working.

Comment 2

[Mark Banner (:standard8)]

Sorry, reverting title back (almost) because I want to deal with this in two stages now I've looked at the code.

Comment 3

[Mark Banner (:standard8)]

We should get this fixed now we're enabling by default.

Comment 4

[Magnus Melin [:mkmelin]]

Yes, blocking‑thunderbird3+

Comment 5

[Mark Banner (:standard8)]

Attachment: [checked in] The fix

This is the fix for this bug and the second half of bug 439470.

I think the best way to explain what I'm doing here is with the comment I've added to nsAbOSXDirectory.h:

  // This is a list of nsIAbCards, kept separate from m_AddressList because:
  // - nsIAbDirectory items that are mailing lists, must keep a list of
  //   nsIAbCards in m_AddressList, however
  // - nsIAbDirectory items that are address books, must keep a list of
  //   nsIAbDirectory (i.e. mailing lists) in m_AddressList, AND no nsIAbCards.
  //
  // This wasn't too bad for mork, as that just gets a list from its database,
  // but because we store our own copy of the list, we must store a separate
  // list of nsIAbCards here. nsIMutableArray is used, because then it is
  // interchangable with m_AddressList.
  nsCOMPtr<nsIMutableArray> mCardList;

So basically, we have two possibilities for storage, and we have to use the right one at the right time.

This patch fixes the bugs and gets the notification updates correct. I'm sure there could be improvements made to how we do this, but I'm hoping we can just separate out mailing lists from nsIAbDirectory at some stage soon which would make handling this sort of thing a lot easier.

Comment 6

[David :Bienvenu]

Comment on attachment 328102 [details] [diff] [review]
[checked in] The fix

some nits:

+    // It is ok to use m_AddressList here as only top-level directories have
+    // gropus, and they will be in m_AddressList

"groups"

we can use ? operator here:

+  nsresult rv;
+  if (m_IsMailList)
+    rv = m_AddressList->AppendElement(aCard, PR_FALSE);
+  else
+    rv = mCardList->AppendElement(aCard, PR_FALSE);

and as discussed over IRC:
+  if (m_IsMailList)
+  {
+    if (m_AddressList)
+      rv = m_AddressList->IndexOf(0, aCard, &index);
+  }
+  else
+  {
+    if (mCardList)
+      rv = m_AddressList->IndexOf(0, aCard, &index);
+  }

Comment 7

[David :Bienvenu]

Comment on attachment 328102 [details] [diff] [review]
[checked in] The fix

r/sr=me, once the comments are addressesed...

Comment 8

[Mark Banner (:standard8)]

Patch checked in with comments addressed -> fixed.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008070703 Thunderbird/3.0a2pre ID:2008070703

I still have to claim about one big remaining issue. All recipients of the given list are added to the To: field! That shouldn't happen due to privacy reasons. Is there already a bug about adding them via BCC?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is working fine for 1.9.0.2pre. So why the target milestone is set to 1.9.1a1?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> I still have to claim about one big remaining issue. All recipients of the
> given list are added to the To: field! That shouldn't happen due to privacy
> reasons. Is there already a bug about adding them via BCC?

Filed bug 444004.

Comment 12

[Mitra Ardron]

I am still seeing the problem in version 3.0a2pre (2008070703) 

I'm following exactly the steps where the bug was reported above, 

I'm not sure if we are doing something different that would make it clearer where the bug is?

Henrik - can you recheck following those steps to see if we are doing something different.

I don't know how to generate the rest of the version string in your comment#9 above.

Sorry I don't seem to have permissions to set it back to a status indicating it doesn't work .... 

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for mentioning that. There is something strange. Sending a message to a mailing list works but you have to initialize it via the address book. Otherwise following error message appears:

Test-Group <Test-Group> is not a valid e-mail address because it is not of the form user@host. You must correct it before sending the e-mail.

Following steps have to be done to be able to send a message to a mailing list from anywhere.

1. Open the address book
2. Select the desired mailing list
3. Choose "Write" from the toolbar
=> All contained contacts are filled in automatically.

Now you can also do following:
4. Hit Cmd-N for a new message
5. Use autocomplete to find the mailing list used above
6. Send message.

I reopen this bug because of the missing initialization. It looks like that the mailing list is empty on startup and isn't filled with the contacts when only using steps 4-6.

Further I filed bug 444211 for the missing mailing list name within the contacts sidebar.

Comment 14

[Mark Banner (:standard8)]

Attachment: [checked in] Fix initialisation of directories/mailing lists

So the problem with the code at the moment, is that the maintained lists (of
cards) are only filled in when GetChildCards is called. This is giving the
effect that if you don't show the properties of the list before you send the
email (aka call GetChildCards), then it won't send because it doesn't know
about any items in that list.

This patch fixes that by moving the initialisation to the Init function (how
original!), and doing the appropriate returns in GetChildCards. I think its
still sensible to leave the GetChildCards function sorting out what is actually
in the list for queries as that's how the rest of the address book types do it.

Comment 15

[David :Bienvenu]

Comment on attachment 330004 [details] [diff] [review]
[checked in] Fix initialisation of directories/mailing lists

nit: i can be declared in the for loop:

+  unsigned int i;
+  unsigned int nbCards = [cards count];
+  nsCOMPtr<nsIAbCard> card;
+  for (i = 0; i < nbCards; ++i)

Do we need to free +  NSArray *cards or does it get garbage collected? It looks like the original code doesn't free it.

typo in "appropriate". Also, could use the ? operator w/ return if you wanted.
+  // Not a search, so just return the appropraite list of items.
+  if (m_IsMailList)
+    return NS_NewArrayEnumerator(aCards, m_AddressList);
+
+  return NS_NewArrayEnumerator(aCards, mCardList);
 }

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> (From update of attachment 330004 [details] [diff] [review])

> Do we need to free +  NSArray *cards or does it get garbage collected? It looks
> like the original code doesn't free it.

I've just taken a look at other areas of the code base - NSArray items such as this don't get released, so I assume they get collected in the auto collection.

Comment 17

[Mark Banner (:standard8)]

This should hopefully be fixed now :-)

Comment 18

[Mitra Ardron]

Yes - it works for me now

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008071803 Thunderbird/3.0a2pre ID:2008071803