Fails to import address lists from Eudora address book

VERIFIED FIXED in mozilla1.2alpha

Status

SeaMonkey
MailNews: Address Book & Contacts
P2
normal
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: Chuck Messenger, Assigned: Cavin Song)

Tracking

Trunk
mozilla1.2alpha
x86
Windows 2000
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: nab-imp, imp-failure)

Attachments

(1 attachment, 2 obsolete attachments)

6.92 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

17 years ago
actually, an address book problem, not a mail database problem.
Assignee: bienvenu → chuang
Component: Mail Database → Address Book

Updated

17 years ago
QA Contact: esther → nbaca

Comment 2

17 years ago
setting bug status to New
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

17 years ago
reassigning to cavin
Assignee: chuang → cavin

Updated

17 years ago
Keywords: nsbeta1
Whiteboard: nab-imp, imp-failure
(Assignee)

Comment 4

17 years ago
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     test1@foobar.com
2.  test2        Foo2      Bar     test2@foobar.com, test1
3.  test3        Foo3      Bar     test3@foobar.com, foo@bar.com

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

    Nickname     First     Last    Email               2nd Email
1.  Test1        Foo1      Bar     test1@foobar.com
2.  Test2        Foo2      Bar     test2@foobar.com    test1@foobar.com
3.  Test3        Foo3      Bar     test3@foobar.com    foo@bar.com

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.
(Reporter)

Comment 5

17 years ago
I tried again with 0.9.8, and it still fails in the same way.
(Assignee)

Comment 6

17 years ago
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.

Updated

17 years ago
Severity: enhancement → normal
Status: NEW → ASSIGNED
Keywords: nsbeta1 → nsbeta1+
Priority: -- → P2

Updated

17 years ago
Target Milestone: --- → mozilla1.0

Updated

17 years ago
Blocks: 126318

Updated

17 years ago
Blocks: 122274
Keywords: nsbeta1+ → nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.2

Comment 7

17 years ago
*** Bug 139511 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 8

17 years ago
Created attachment 81811 [details] [diff] [review]
Proposed patch, v1

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

Comment 9

16 years ago
Created attachment 82788 [details] [diff] [review]
Proposed patch, v2

Made some minor changes to v1 patch.
Attachment #81811 - Attachment is obsolete: true
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.
(Assignee)

Comment 11

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

Comment 12

16 years ago
Created attachment 83611 [details] [diff] [review]
Incorporated review comments.
Attachment #82788 - Attachment is obsolete: true
Comment on attachment 83611 [details] [diff] [review]
Incorporated review comments.

r=sspitzer
Attachment #83611 - Flags: review+
> 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 ji@netscape.com

Comment 15

16 years ago
+    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
(Assignee)

Comment 16

16 years ago
OK, I'll fix it before checking in the code.
(Assignee)

Comment 17

16 years ago
The issue of supporting non-ascii display names and email addresses is logged in
bug 145293.
(Assignee)

Comment 18

16 years ago
Fix landed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

16 years ago
Nominating for rtm.
Keywords: nsbeta1- → nsbeta1+
(Assignee)

Comment 20

16 years ago
Wrong keyword. Doing it again.
Keywords: nsbeta1+ → nsbeta1

Comment 21

16 years ago
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.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.