Can't send emails to OS X mailing lists.

VERIFIED FIXED in mozilla1.9.1a1

Status

MailNews Core
Composition
P2
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Mitra Ardron, Assigned: standard8)

Tracking

Trunk
mozilla1.9.1a1
All
Mac OS X
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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 !
(Assignee)

Comment 1

10 years ago
Yep, I see this, not sure if its a regression or not, but its definitely not working.
Assignee: nobody → bugzilla
Status: UNCONFIRMED → NEW
Component: Address Book → MailNews: Composition
Ever confirmed: true
Product: Thunderbird → Core
QA Contact: address-book → composition
Summary: OSX Address Book mailing list support → Can't send to non-Mork (local) mailing lists
Version: unspecified → Trunk
(Assignee)

Comment 2

10 years ago
Sorry, reverting title back (almost) because I want to deal with this in two stages now I've looked at the code.
Summary: Can't send to non-Mork (local) mailing lists → Can't send emails to OS X mailing lists.
(Assignee)

Updated

10 years ago
Depends on: 437975
(Assignee)

Comment 3

10 years ago
We should get this fixed now we're enabling by default.
Flags: wanted-thunderbird3.0a2?
Flags: blocking-thunderbird3?

Comment 4

10 years ago
Yes, blocking‑thunderbird3+
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Hardware: Macintosh → All
(Assignee)

Updated

10 years ago
Priority: -- → P2
(Assignee)

Comment 5

10 years ago
Created attachment 328102 [details] [diff] [review]
[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.
Attachment #328102 - Flags: superreview?(bienvenu)
Attachment #328102 - Flags: review?(bienvenu)
(Assignee)

Updated

10 years ago
Blocks: 439470

Comment 6

10 years ago
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

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

r/sr=me, once the comments are addressesed...
Attachment #328102 - Flags: superreview?(bienvenu)
Attachment #328102 - Flags: superreview+
Attachment #328102 - Flags: review?(bienvenu)
Attachment #328102 - Flags: review+
(Assignee)

Comment 8

10 years ago
Patch checked in with comments addressed -> fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: wanted-thunderbird3.0a2?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
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?
Status: RESOLVED → VERIFIED
This is working fine for 1.9.0.2pre. So why the target milestone is set to 1.9.1a1?
(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.
(Reporter)

Comment 12

10 years ago
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 .... 
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

10 years ago
Created attachment 330004 [details] [diff] [review]
[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.
Attachment #330004 - Flags: superreview?(bienvenu)
Attachment #330004 - Flags: review?(bienvenu)
(Assignee)

Updated

10 years ago
Attachment #328102 - Attachment description: The fix → [checked in] The fix

Comment 15

10 years ago
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);
 }
Attachment #330004 - Flags: superreview?(bienvenu)
Attachment #330004 - Flags: superreview+
Attachment #330004 - Flags: review?(bienvenu)
Attachment #330004 - Flags: review+
(Assignee)

Comment 16

10 years ago
(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.
(Assignee)

Updated

10 years ago
Attachment #330004 - Attachment description: Fix initialisation of directories/mailing lists → [checked in] Fix initialisation of directories/mailing lists
(Assignee)

Comment 17

10 years ago
This should hopefully be fixed now :-)
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

10 years ago
Yes - it works for me now
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
Status: RESOLVED → VERIFIED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.