91295 - Fails to import address lists from Eudora address book

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P2)

Description

[Chuck Messenger]

I imported my Eudora address book into Mozilla mail.  It mostly worked, but it 
didn't import any of my mail lists.  In Eudora, each address book entry 
consists of one or more addresses and/or other address book entries.  Mozilla 
failed to import any of the address book entries which contained one or more 
other address book entries.

Comment 1

[David :Bienvenu]

actually, an address book problem, not a mail database problem.

Comment 2

[Asa Dotzler [:asa]]

setting bug status to New

Comment 3

[scottputterman]

reassigning to cavin

Comment 4

[Cavin Song]

Bug 82276 was just fixed and it was caused by entries not having any email 
address(es) (ie, entries without email address(es) were not imported).

The way it works now is that if you have the following addrbook entries efined 
in Eudora:

    Nickname     First     Last    Email
1.  Test1        Foo1      Bar     [e-mail]
2.  test2        Foo2      Bar     [e-mail], test1
3.  test3        Foo3      Bar     [e-mail], [e-mail]

Then after import finishes you'll have the following cards in NS6 addrbook:

    Nickname     First     Last    Email               2nd Email
1.  Test1        Foo1      Bar     [e-mail]
2.  Test2        Foo2      Bar     [e-mail]    [e-mail]
3.  Test3        Foo3      Bar     [e-mail]    [e-mail]

I'm not 100% sure if this bug may have been fixed by 82276, so, reporter, can 
you try it again with the latest build?  If it still does not work for you can 
you show us some sample entries which fail to be imported? This will help us 
resolve the problem quickly.

Comment 5

[Chuck Messenger]

I tried again with 0.9.8, and it still fails in the same way.

Comment 6

[Cavin Song]

Chuck, I should have said that the fix for bug 82276 was just checked in yesterday
so you'll need to run today's commercial build to verify it.  When you report 
errors or not working stuff next time, it would be great if you can also include
the build id of the mailnews program you run in the comment. Thanks for the help.

Comment 7

[Ninoschka Baca]

*** Bug 139511 has been marked as a duplicate of this bug. ***

Comment 8

[Cavin Song]

Attachment: Proposed patch, v1

If an Eudora contact contains two or more email addresses then it's mapped to a
list (or group).

Comment 9

[Cavin Song]

Attachment: Proposed patch, v2

Made some minor changes to v1 patch.

Comment 10

[(not reading, please use [e-mail] instead)]

Cavin, thanks for fixing all these bugs in our import code.

Some minor comments and suggestions:

1)

-    // Set 2nd email. If we have two/more email addresses then use the 2nd one 
on the list.
-    if (emailList.Count() > 1)
-    {
-      pData = (CAliasData *)emailList.ElementAt(1);
-      ADD_FIELD_TO_DB_ROW(pDb, Add2ndEmail, newRow, pData->m_email, uniStr);
-    }

forgive me for not understanding the import code.  do we loose any 
functionality by removing this code?  can you explain why it's safe to remove 
it?

2)

+  // Make sure group members exists before adding groups.
+  AddGroupMembersAsCards(membersArray, pDb);

you aren't checking rv, yet AddGroupMembersAsCards() can return failure.

3)

+void nsEudoraAddress::RememberGroupMembers(nsVoidArray &membersArray, 
nsVoidArray &emailList)
+{
+  PRInt32 i, j, memberCnt, cnt = emailList.Count();
+  CAliasData *pData;

As a style nit, I'd declare j, memberCnt and pData in the inner most proper 
scope.
You've declared them in an outer scope, where they are not needed.

4)

+  nsresult rv = NS_OK;
+  nsCOMPtr <nsIMdbRow> newRow;
+  nsAutoString uniStr;
+  nsCAutoString displayName;

These four are unused.

5)

+nsresult nsEudoraAddress::AddGroupMembersAsCards(nsVoidArray &membersArray, 
nsIAddrDatabase *pDb)
+{
+  PRInt32 i, max = membersArray.Count();
+  CAliasData *pData;
+  nsresult rv = NS_OK;
+  nsCOMPtr <nsIMdbRow> newRow;
+  nsAutoString uniStr;
+  nsCAutoString	displayName;

again, style nit on where these are declared.

6)

+    displayName.Truncate();
+    if (!pData->m_realName.IsEmpty())
+      displayName = pData->m_realName;
+    else if (!pData->m_nickName.IsEmpty())
+      displayName = pData->m_nickName;

why not:

+    if (!pData->m_realName.IsEmpty())
+      displayName = pData->m_realName;
+    else if (!pData->m_nickName.IsEmpty())
+      displayName = pData->m_nickName;
+    else
+      displayName.Truncate();

7)

+  // Create a list.
+  nsresult rv;
+  nsCOMPtr <nsIMdbRow> newRow;
+  rv = pDb->GetNewListRow(getter_AddRefs(newRow)); 

just a minor style nit, consider:

+  nsCOMPtr <nsIMdbRow> newRow;
+  nsresult rv = pDb->GetNewListRow(getter_AddRefs(newRow)); 

8)

+  nsCAutoString value;

again, declared in a wider scope than necessary.

9)

+    value.Assign("mail=");
+    value.Append(pData->m_email);
+    rv = pDb->AddLdifListMember(newRow, value.get());

consider:

nsCAutoString ldifValue = NS_LITERAL_CSTRING("mail=") + nsDependentCString
(pData->m_email);

(or something similar).

10)

some other things to test or consider (these might become spin off bugs)

a)  how does your code handle non-ASCII display names?

b) will this code work with IDNs in email addresses?  (does eudora support 
that?)
see http://bugzilla.mozilla.org/show_bug.cgi?id=127399 

c) when you import on a debug build, do you assert?  for a related asserting 
when importing, see http://bugzilla.mozilla.org/show_bug.cgi?id=142123

11)

please include some notes on what scenarios you tested (lists with 1 email, 
lists with multiple emails, lists with 0 emails, etc.) for QAs consideration.

Comment 11

[Cavin Song]

(1)
It's safe to remove the setting of 2nd email address code because before the fix 
we always mapped Eudora address entries to NS cards so we would set the 2nd 
email address for a card if an Eudora address entry contains two or more email 
addresses. But now we map those Eudora entries to lists/groups, so the code to 
set 2nd email address is no longer needed.

(2) - (9)
Will make the changes.

(10)
a) - The code currently only works for ascii display names and email addresses.
b) - It won't work with IDNs in email addresses. Not sure if Eudora currently 
supports IDNs in email addresses, but my guess is not.
c) - Yes, I did see the same thread safety assertions as recorded in 
http://bugzilla.mozilla.org/show_bug.cgi?id=142123.

(11)
To test the fix, create a new Eudora address entry with two or more email 
addresses (ie, in the space where it says "This nickname will expand to the 
following addresses:" enter two or more addresses). Then import this Eudora 
addressbook and you should see the Eudora address entry is turned into an NS 
list, not card, with the right number of member addresses. In addition, create 
some more new Eudora address entries with 0 or 1 email address and import the 
addressbook again. These Eudora entries should be turned into NS cards.

A new patch is coming.

Comment 12

[Cavin Song]

Attachment: Incorporated review comments.

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 83611 [details] [diff] [review]
Incorporated review comments.

r=sspitzer

Comment 14

[(not reading, please use [e-mail] instead)]

> The code currently only works for ascii display names and email addresses.

Assuming you can have non ascii display names in Eudora, we should log a bug on
this, since we know a problem exists.  when you do, please cc [e-mail]

Comment 15

[David :Bienvenu]

+    rv = pDb->GetNewRow(getter_AddRefs(newRow)); 
+    if (!newRow)
+      return NS_ERROR_FAILURE;

you're not checking rv, and you're not returning rv either...if you fix that,
sr=bienvenu

Comment 16

[Cavin Song]

OK, I'll fix it before checking in the code.

Comment 17

[Cavin Song]

The issue of supporting non-ascii display names and email addresses is logged in
bug 145293.

Comment 18

[Cavin Song]

Fix landed on trunk.

Comment 19

[Cavin Song]

Nominating for rtm.

Comment 20

[Cavin Song]

Wrong keyword. Doing it again.

Comment 21

[Ninoschka Baca]

Trunk build 2002-05-28: WinMe, Mac 9.1
Verified Fixed. 
- Individual entries are imported with all the data.
- Entries with more than 1 email address become a list after importing.