83585 - Convert addressbook from NS4.x to NS 6.1

Status: VERIFIED FIXED

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

Description

[Tiantian Kong]

Spec: ( This is a description of the requirements as you understand so far. 
 Please be as detailed as possible. Please use short sentences.)

The following are the requirements :
1. Update the Tools/Import Utility UI (dialog box) to display an option for
importing 4.x Communicator address book (AB)
2. Throw up a file dialog to display the existing 4.x AB, take the user
automatically to the directory where it resides and display the appropriate file
type in the file dialog
3. Before starting the conversion resolve the discrepancies in the 4.x and 6.x
fields. An example is - 4.x has only 1 address where as 6.x has 2 addresses for
each record (address card). Take input from the user to decide the action for
such fields
4. Deal with any preferences for 4.x AB.
5. Convert the 4.x format to 6.x format. Existing AB class can be used however
will require modifications for the above point, 3.
6. Save the converted 6.x AB file in the default AB directory for 6.x.
7. The feature should work on all three platforms : Unix (Linux), Mac & Windows.

Dev Time: (12 man-days, [2 to find all reqs + 8 to develop + 2 for integration]) 
Learning time ? 5-7 days (XUL(XML, Javascript), XPCOM, DOM/AOM) 
? Contingency ? 2 days

Hand-off to QA:  (06/04/01) 
External Dependencies: Need access to Mac and Linux platform

Comment 1

[Rajiv Dayal]

- created a new module (Comm4x) as part of the Import utility module for this 
feature which means implemented the following :
      - a factory class for (Comm4x)
      - a sub-class to implement nsIImportAddressBook
           - the functions for this interface  
      - a StringBundle and a properties file for title strings and messages
- finished requirements item # 1 and 2. Initially seemed a simple stuff to 
do...but hey no .. it forces u to understand the nitty-gritty of XUL, 
Javascript, XPCOM and their working with multiple threads. Good for me, I am new 
and learning !

Comment 2

[Rajiv Dayal]

- there exists a class called Upgrader whose job is to convert Communicator 4.x 
address book stored using Neomagic DB into LDIF (text based) format. Using this 
class implemented part 1 of item # 5. Implemented the class for converting LDIF 
to Mork based .mab (N6.x) format using some existing code from text AB to N6.x 
AB Import, for part 2 of item # 5.
- the above uses Address book code (ABMDBDirectory, ...) which is throwing 
repeated assertion failures for several 'intl\uconv' classes being not thread 
safe as well as assertion failures for calls from native code to Javascript ???
- the above happens 50% of time but 90% of times the import is successful. In 
process of figuring out whats going on !

Comment 3

[Rajiv Dayal]

- modified the implementation of couple of 'uconv' internationalization classes 
for testing purposes which removed the assertions for thread safety. The 
question now is - do these classes need to be thread safe or they have to be 
used in a different way ? Other members in the team think that the 
internationalization classes should ideally be thread safe. Will follow up with 
Internationalization people. 
- In any case the good thing is if the above multiple thread assertions are 
ignored the import code still works fine and hence I can go ahead with 
implementation.
- the assertions error in the Addres book code while making calls to Javascript 
from native code mostly happens in the AddressBookSession class and the 
AddressBookDir class when notifying the listeners for some UI items which is 
part of Javascript code. This happens because the XUL/Javascript UI code 
should run in the main thread and since the ImportAddress function runs as a 
separate thread this throws an exception when the Addressbook classes call the 
UI code. Need to follow this up with the Addressbook developer. 
- However, again the good news is that when these assertion are ignored the 
import code goes ahead and works fine, so I can proceed with the implementation.

Comment 4

[Rajiv Dayal]

- implemented the requirements items # 3, 5 and 6. Implemeted the dialog which 
is displayed to the user to get the input to deal with the address location 
field discrepencies and implemented the code in the nsImportGenericAddressBook 
to get this input and pass on to the Comm4x import class. Implemented the code 
to take care of the discrepencies while saving the imported address book 
records into the N6.x Mork database.

Comment 5

[Rodney Velasco]

assigning QA to luke.

Comment 6

[Rajiv Dayal]

Attachment: HandOff Notes - User Guide / Build Instr / Execution Notes / Known Issues

Comment 7

[Rajiv Dayal]

- well .. the implementation is done, time for testing. To build and run the 
code :- use my machine's shared dir "ImportAB4.xTo6.x" to copy the new files and 
follow the instructions in the attachment (id=37431) in the previous comment.
- Handing over to QA with 2 known issues (see attachment (id=37431) above).

Comment 8

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

jglick, we need your UI input.

we have this problem (I think it is covered in the comments in this bug report)
where importing 4.x addressbooks always map the 4.x card entries to 6.x work
entries.  (the entries for a given addressbook card has changed, so when it is
not clear what where values go, we put them as "work" entries, instead of
"personal".)

for manual import of 4.x addressbooks (not automatic import during migration),
rajev wants to prompt the user to ask them if they want us to default entries to
"work" or "personal".

right now, he's got a dialog, but I suggested making it another page in the
import wizard.  what do you think?

note, we'll still default (when migrating) to "work".  we don't want any dialogs
popping up for each addressbook during migration.

Comment 9

[jglick]

When a user imports an AB manually (from the AB File menu, Import) why not do 
the same as during migration? Save them as "work" entries, instead of 
"personal".  I don't see the benefit of asking users since they mostly likely 
have a mix of personal and work data.

In addition, 4.x didn't distinguish between work and personal data, so asking 
users "do you want this data as personal or work?" might confuse them. Instead 
of asking them to answer a question they might not userstand, why not just pick 
a logical default?

Comment 10

[Rajiv Dayal]

The most critical known issue/problem that we were facing were the XPConnect 
assertions during the import. It however also seemed to appear even with the 
Text based AB import. In fact there is already a bug for the same assertion 
error happening  when doing other operations in AB : Bug id - 79852. Candice 
Huang (AB developer) has been able to track it down and has the fix for it 
(included as an attachment in the bug - 79852).

Comment 11

[Rajiv Dayal]

We had a meeting for the requirements for this feature and that is when it was 
decided that it will be better to resolve field discrepencies between 4.x and 
6.x during the separate import (manual) process. To do so we will take user 
input. Mitch Green (CTO, eClient) who also deals with the customers suggested 
this. 

Mitch, can you please give your comments on this.

I guess users have separate address books for home and personal and it might be 
better to ask user when importing each address book manully where he wants to 
store the address location for each address book (note that the Q is asked for 
each address book and not each card).  Also in order to  not  confuse the user 
we show a detailed message (3 lines) the text of which is below :

"Netscape 6 Address Book has two address locations (Home and Work) for each 
entry(card), Communicator had only one, 
Do you want to store the imported address location as Home or Work ?"

Please feel free to change this display text.

Comment 12

[jglick]

If you do want to ask the user, I would agree with Seth that asking them as part 
of the Wizard is a better option. Its just cleaner as part of the Wizard than 
popping up a dialog from a Wizard.

As for the wording, hopefully jatin can provide his feedback, since you'll want 
to be sure what you're asking of users is clear to them.  

Use "Previous versions of Netscape" or something similar since users don't know 
what "Communicator" is.

Comment 13

[Mitch Green]

Many users of Communicator have more than one Personal Address Book (e.g.,
Co-workers and Friends), or they might only put their friends into their
Personal Address Book, or they might only put co-workers into their Personal
Address Book. When I migrated my Profile from 4.X to 6.X, I was very angry that
400 the addresses in my Address Book got imported into the wrong place.  I had
to move them all by hand.  I hated it.  That will end up as a showstopper
somewhere, I'm sure.

So, let's be polite and ask whether the address fields are for "work" or for "home"?

"Communicator's Address Book allowed you to store 1 street/mailing address per
user, Netscape 6.1 allows 2 (Home and Work). Would you like to store this
Address Book's imported addresses as a Home or Work?"

Comment 14

[lohphat]

Before I file a new bug, does this bug# reflect that a NS 4.x -> Moz 0.9.1
profile conversion misses the abook?

Comment 15

[Hong Kwon]

If what you mean is profile migration, this is a different feature entirely. 
File a bug against profile migration.  This is for address book conversion at
run time.

Comment 16

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

[e-mail]:

4.x -> 0.9.1 will not migrate your 4.x address book.  even after this fix, you
won't be able to do it by hand.

the reason is the 4.x addressbook migration code is based on some non-public
code (that was originally part of 4.x) that we don't own.

you'd have to use Netscape 6.x to get that feature.  you can do it by hand by
exporting your ab in 4.x to ldif, and then importing the ldif back into to mozilla.

Comment 17

[lohphat]

Then the GUI shoukd warn you during the conversion "This step will convert
everything except your address book" as opposed to leading the user to believe
that "converting existing profile" means exactly that.

I'm not bitter.

Comment 18

[Rajiv Dayal]

I think the **profile migration** GUI should warn during the conversion "This 
step will convert everything except your address book". Can u please file the 
bug there, this feature is different than profile migration. Thanks.

Comment 19

[Tiantian Kong]

When will users use this feature?

Chris: please make sure you document this. If you have additional comments,
please let us know.

(1) If the migration of address book failed at the time of profile migration,
the user can use this utility to import the address book, instead of deleting
the entire profile, then performing another migration. ( Question relating to
this. What will user do if the migration of bookmark, email, etc. failed at the
time of migration? Right now, looks like they need to do another profile
migration. Should we provide some alteratives here as well? Maybe profile
migration can be modified to call this feature automatically if there is a
address book migration failure? Anyone has any comments?)

(2) This feature provides user the choice to store the address as either home or
work address. ( Suggestion. Profile migration needs to be modified to provide
the same choice. I'll open a bug about this.)

(3) This feature can be used to import address book from different user profile
after the completion of profile migration.

(4) This feature can be used to import updated address book from 4.x for the
same profile, after the completion of profile migration. This is especially
useful when the user are using 4.x and 6.x side by side, for example, during
alpha/beta testing of 6.x.

Comments: Utility to import AOL addressbook into Netscape 6.x address book would
be nice to have. 

Comment 20

[Rodney Velasco]

adding nsenterprise keyword

Comment 21

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

this bug needs to move to bugscape.  

this will not be a mozilla feature, since it depends on the 4.x addressbook code
which only ships with the Netscape commercial build.

asa, can you help me to move it?

Comment 22

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

Attachment: the mozilla part of the fix.

Comment 23

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

Attachment: the ns part of the fix (I'm sure the .mcp file is horked.)

Comment 24

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

Attachment: screen shot of the new dialog. (this needs to be fixed.)

Comment 25

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

todo list:

1) get this patch to build on win32 and mac, and test it.

2) we don't need whichLocationDialog.* (see the screen shot).  we can use the
common dialog for this.

3) fix packager-mac and packager-win  (see what I did to packager-unix)

4) fix the userAgent / filePicker code in importDialog.js to be XP.

5) figure out how to not duplicate importMsgs.properties in the ns tree

Comment 26

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

let me elaborate on that todo list:

> 2) we don't need whichLocationDialog.* (see the screen shot).  we can use the
common dialog for this.

rajiv, take a look at the nsIPromptService.idl interface.  you should be able to
put up a simple dialog and get the users choice using the "common dialogs".

> 3) fix packager-mac and packager-win  (see what I did to packager-unix)

the 4.x -> 6.x import code lives in the a new component.  you have to package
it.

> 4) fix the userAgent / filePicker code in importDialog.js to be XP.

There is some code in nsProfileAccess::Get4xProfileInfo() that already
determines the users 4.x profile root, but it's not accessible.  I suggest you
remove your existing code until we have a XP way to get the user's 4.x profile
root.

> 5) figure out how to not duplicate importMsgs.properties in the ns tree

having two versions keeps this feature out of mozilla.  but it means we have to
keep the two versions in sync.

one dirty solution is to add the new strings (you added) to the
importMsg.properties in the mozilla tree, and not have a importMsg.properties in
the ns tree.

we can talk about this more later.

for later:

6)  as chuang and I have both mentioned, there's a lot of duplicate code in
ns/mailnews/import/comm4x/src/*.  I'm sure we can re-use common code to do this,
but the other items on this todo list are more important.

Comment 27

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

> 5) figure out how to not duplicate importMsgs.properties in the ns tree

I've thought of a clean way this can be done.  it involves moving the modules
specific strings out of imporMsg.properties and into a seperate string bundle.

then, in importDialog.js, when we need those modules specific strings, we do this:

url = "chrome://messenger/locale/" + module-id + ".properties";
and get the string bundle for that url.

rajiv, don't worry about doing that cleanup before landing this fix.  that
cleanup can happen after.  

instead of adding importMsgs.properties to the ns tree, we should add your two
strings to the mozilla importMsgs.properties.

Comment 28

[Rajiv Dayal]

Seth I saw the attachments you have put, it seems to be for my new files as well 
as the code that I had modified, is that true ? If so thanks for doing it for 
me. I will test if the patch works on mac as u suggested.

Comment 29

[Asa Dotzler [:asa]]

sspitzer, we're probably better served by leaving the bug here. If we moved it
we'd have to create a new Bugzilla bug for the mozilla part of the fix and
moving doesn't carry over attachments so we'd have to manually do that in the
Bugscape bug.  Probably more work than it's worth.  The alternative (if it is
useful) is to create a new Bugscape bug to do the rest of the work for this and
leave this bug open as coverage and explanation of the bugzilla checkin. 

Comment 30

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

asa, works for me.  let's just leave it here.

Comment 31

[chuang]

Attachment: Proposed patch for resolving dup ldif code

Comment 32

[chuang]

I have attached the diff for reuse nsAddressBook::ConvertLDIFtoMAB.
The changes in nsComm4xImport.cpp, ImportAddressImpl::ImportAddressBook()

line 587, 588
-     IMPORT_LOG0( "NA2 to LDIF conversion done.\n");
-     NS_RELEASE(proxyABObject);

line 601
-     rv = m_comm4x.ImportLDIF( &addrAbort, name.GetUnicode(), tmpLDIFFileSpec, 
pDestination, error);
+     rv = proxyABObject->ConvertLDIFtoMAB(tmpLDIFFileSpec, PR_FALSE, 
pDestination, PR_TRUE);
+     IMPORT_LOG0( "NA2 to LDIF conversion done.\n");
+     NS_RELEASE(proxyABObject);

I don't feel this fix is risky,  but it's up to Rajiv and Seth to decide if they 
want to take it at this point.  I have test it on WIn32 for both migarting and 
import 4.x addressbook to 6.0.

Comment 33

[chuang]

I have filed a new bug 87342 for removing the dup ldif code in Import/text and 
addrbook.  If we want to clean up other dup code in Outlook, OE and Eudora, we 
should have a separate bug.  

Comment 34

[lrg]

The fix seems to be working on Win 2000 and NT, though we are still having a 
small problem on win 98.  There are a few major issues on Linux that still need 
to be resolved, primarily handling of files from another user's directory; also 
certain .na2 files will cause failure or a crash, we need to find out the 
situations that cause this.  I haven't seen a mac build yet so i have no 
comments for that yet.

Comment 35

[lrg]

upon further examination of behavior on linux, i think that the failure 
which i have been seeing occurs when an import of an empty address book is 
attempted, or when an address book with blank cards is imported.

Comment 36

[Brian Nesse (gone)]

Attachment: Stuffed Mac project file for Commercial tree

Comment 37

[Brian Nesse (gone)]

Attachment: Mac build list changes for Commercial tree

Comment 38

[Brian Nesse (gone)]

With a couple of fixes, I got this to compile and run on the Mac. I have attached 
a (functional) mcp file and the build script changes to build it. 

The other change was that I removed the #include "nsAddressBook.h" from 
nsComm4xImport.cpp [approx line 53] because it wasn't necessary and required 
access paths back to the Mozilla tree.

Comment 39

[Brian Nesse (gone)]

Forgot to mention. The import appears to have functioned correctly but there were 
some fairly major visual issues with some of the dialogs.

Comment 40

[jglick]

Attachment: Suggestion for additional wizard screen and wording

Comment 41

[jglick]

Suggested that an additional screen be added to the Import wizard to ask users 
if they want the data stored as Home or Work info.  Suggested wording is in the 
attachment above.  Could someone from Tech Pubs checkout the wording and make 
sure the capitialization is correct?  Thanx.

Comment 42

[Rajiv Dayal]

All the items as suggested by Seth has been completed. 

1. The patch has been tested on MAC (please see comments from Brian above) and 
windows.
2. The whichDialog has been removed, earlier used the common dialog but since 
the common dialog on win looked ugly so implemented the screen as part of the 
import wizard itself as suggested by jglick and also by Seth earlier.
3. done.
4. done.

Am in the process of creating the new patches with the latest build.

Comment 43

[Rajiv Dayal]

Attachment: The new wizard page to take user input to store mailing address as either home or work

Comment 44

[Rajiv Dayal]

Attachment: this one is the image instead of the previous one

Comment 45

[Rajiv Dayal]

Attachment: the mozilla patch file with the changes for above wizard page

Comment 46

[Rajiv Dayal]

5) Seth, for the suggestion # 5) above to not duplicate the 
importMsgs.properties in the ns tree, in fact we donot have to. When I tested on 
Windows by moving the comm4x to ns commercial tree instead of mozilla tree I did 
not have to copy the importMsgs.properties to the ns tree. I tested the ns build 
and it works fine with only comm4x dir in the mailnews/import in ns. I guess 
since the ns build copies the mozilla dist/bin dir it might not be required to 
really have a copy in the ns tree too.

Comment 47

[chuang]

I only see jar.mn in the last patch (6.25.01 15:25) and it's not good. Aren't 
those files should be in commercial tree according to Seth's patch?

In commercial tree, mailnews/import/comm4x/src/makefile.win
+ $(DIST)\lib\xpcom.lib \
Should be
+ $(MOZ_DIST)\lib\xpcom.lib \
or it won't build on Win32.

Please post a final patch with the new files.

Comment 48

[Brian Nesse (gone)]

Attachment: New Mac project file that doesn't require extraneous header files.

Comment 49

[Rajiv Dayal]

Attachment: please disregard the earlier patch attachment, i attached the wrong file earlier

Comment 50

[Tiantian Kong]

Attachment: Problem with btn alignment in the final screen

Comment 51

[Tiantian Kong]

Per Seth's comment, I've copied below some email conversations.

************************
Hi, Seth and Scot:

Thanks so much for all your help in the address book converter.

Over the weekend, Rajiv has implemented all of Seth's suggestions concerning the
address book converter. 

At this moment, Rajiv is producing patch for NS commercial build. He has turned
over the patch on Mozilla for building and testing on Win and Mac to other team
members. QA will test his new patch on windows today. And Rajiv build and test
on Linux.

Known issues with address book converter:

   1. crash on Linux when importing an empty address book. ( Otherwise the
feature works as expected.)
   2. button misalignment on Mac and Win 98. for two buttons on one screen,
after the completion of import.

We are trying to check in code by COB today. Suppose no new bugs are found in
the tests today, can we check in with the 2 known issues above?

If we put in additional effort to debug the two known issues above, (a) we may
not be able to find the fix. (b) Even if we got fixes, we may not have time to
retest on the 3 platforms. ( Rajiv has been debugging them for quite some time,
no clue yet.  )

What's the process for us to check in fixes for these two issues, and the code
clean up later on?

Tiantian

********************************
Reply from Scot: 

I think we can live with #1 for the initial checkin as long as it's only import
that's affected.  I think it needs to be fixed by RTM though.  Without seeing #2
I don't know how important it is, but it sounds like it can also be skipped for
this checkin.  You should file bugs for them and then get approval from PDT to
check them in when you have fixes.  My understanding is that this week will be
the easiest to get bugs in.

Scot

********************************
reply from Seth:

Rajiv, please post a screen shots of the new wizard panel (on mac and win98) so
we can see what it looks like.

As far as landing this by COB, Rajiv's patch still needs to be reviewed (by
chuang) and super-reviewed (by me).

Instead of sending direct email, please bring up issues in the bug report. (so
we don't lose track of them.)

-Seth

Comment 52

[Rajiv Dayal]

Attachment: ns patch for modified files

Comment 53

[lrg]

Attachment: Jpeg image file of final screen of Import Wizard with overlapping buttons

Comment 54

[Rajiv Dayal]

Attachment: the ns patch for the new files

Comment 55

[chuang]

I applied the new patches (I have to apply the ns tree by hand, hopefully it's
all correct) and run the import on Win2000.  It works fine with the known button
misalignment.  Only one minor thing -

Not all new files have NPL copy right(2 Makefile.in).  The year should be 2001
instead of 1998 or 1999.  Unless the copy right is not important on commercial
tree, or we should fix it.  Can someone find out?

r=chuang pending the copy right issue.  

Comment 56

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

here are some initial comments.  I'll continue reviewing tomorrow morning.

1)

the latest fix involves nsComm4xAddress::ImportLDIF()

what happened to chuang's patch for resolving duplicate ldif code

see her comments on 2001-06-22 

2)  spelling mistake:

+Comm4xFiles=Communicator Adress Book files (*.na2)

"Adress" should be "Address"

3) working problems (you need [e-mail] to review your new wordings.)

a)

"Netscape Communicator 4.7"

what if they used 4.5? or 4.6?  Don't use version numbers.  Just say "Netscape
Communicator"

b)

"store 1 mailing" should be "store one mailing"
"allows 2" should be "allows two"

c)

Don't use "Netscape 6.1".  (I don't want to have to back and change that on
every release.)  I'm not sure how you should generically refer to the current
product, but doing 6.1 is not right.

d)

Communicator 4_x / Netscape 4_x

"4_x" is no good.  I'd suggest just refering to previous versions as "Netscape
Commuciator".

4)

+#Netscape Communicator 4.x directories
+NetscapeWinDir=C:\\Program Files\\Netscape\\Users

+        if (selectedModuleName == gImportMsgsBundle.getString('Comm4xImportName'))
+
    {
+
	var agt=navigator.userAgent.toLowerCase();
+
	var is_win = ( (agt.indexOf("win")!=-1) || (agt.indexOf("16bit")!=-1) );
+
	if (is_win)
+
	{
+
		var aDir =
Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile);
+
		if (!aDir) alert("Call to dir createInstance failed");
+
		aDir.initWithPath(gImportMsgsBundle.getString('NetscapeWinDir'));
+
		filePicker.displayDirectory=aDir;
+
	}
+
	filePicker.appendFilter(gImportMsgsBundle.getString('Comm4xFiles'),"*.na2");
+
}

no go on this.  as I mentioned in previous comments, this is not XP code. 
specifying the path like that in the .properties file is not the right solution.

you should not add this code.  later, we can worry about adding some XP way of
getting to the user's 4.x profile root.

Specifying the filter as "*.na2" is ok, but the rest has to go.

Please address these issues and attach a new patch.

Comment 57

[Brian Nesse (gone)]

Attachment: Yet another mac project file with the correct code fragment name this time.

Comment 58

[Rajiv Dayal]

1. When I added the patch from Candice and changed the code for removing 
duplicate code, it creates two AB files, for eg : when I import Test1.na2 two 
imported files are created Test1.ldif and Test1. Sometimes it doesnot import the 
entries at all and sometimes it imports the entries to Test1.ldif. I have 
retested twice once couple of days back and again today applying the patch to 
the latest tip. I donot think we can debug this right away. Besides we will need 
to test import and also migration on all three platforms for which our QA does 
not have time right away. So as we had earlier decided we will do the real clean 
up later rather than trying to hurry this up.

2. corrected the spelling mistake

3. i have sent the mail to jatin to review the wordings.

a) changed it to say Netscape Communicator
b) changed the display text to use the words one and two instead of 1 and 2
c) I had earlier got comments that users dont know that the previous version is 
called Communicator, they are more familiar to the term Netscape 4.5/4.7 (or 
just Netscape 4). Will let Jatin decide.
d)Let Jatin decide.

4. removed the code to take user to default Netscape 4 profile dir on windows 
from the javascript file.

Comment 59

[chuang]

Rajiv, I don't see two files created with my patch.  Is these two files stay in
the user's profile or did it show as two address books in the address book window?

Comment 60

[Rajiv Dayal]

Candice, it shows as two address books in the address book window. Besides, 
although it returns success no entries are displayed in these address books in 
the address book window. 

Comment 61

[chuang]

Well, the patch works fine on my win2000 anyway.  The ldif file is created when 
converting na2 to ldif not by the dup code.  I'm not sure about the other one. 

I found a bug that will make the import 4.x useless on Win32.   In the new page 
asking for home or work, if the file path is really long, the buttons are off 
the window frame.  I cat only use the close box to close the import wizard.

Please see the attached image file.

Comment 62

[chuang]

Attachment: image file for the page with buttons off the window

Comment 63

[Jatin Billimoria]

emailed rdayal, but including suggested edits here too:
---
Netscape Communicator 4.x has one mailing address for each card, while Netscape
6.1 has two (Home and Work).

Select the category in which you want to store the imported mailing address:

This category will be used for all the entries of the selected address book:
----

Comment 64

[Rajiv Dayal]

Candice, the ldif file is created in the temp directory and not mork db so i
donot know why it is appearing in the address book window. Anyway, debugging all
this will take some time and i explained in my earlier emails that the import
utility base class creates new address book for the imported ab so i will have
to debug that which may impact other imports too. So we will not checkin the
code cleanup to remove dups now in a hurry as we had decided earlier. 

I have changed the screen you posted to display the address book name instead of
the full path. I had put it there so that the user knows for which address book
he/she is making the choice for the work and home category.

Comment 65

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

more code review:

1) CIDs vs Contract IDs

+ m_pService = do_GetService( kImportServiceCID);

use the contract ID, not the CID 

if you use CIDs in other place, replace them with the a
appropriate contract IDs.

2) don't use NS_WITH_SERVICE, use do_GetService()


NS_WITH_SERVICE( nsIProxyObjectManager, proxyMgr, kProxyObjectManagerCID, &rv);

should be something like:

nsCOMPtr <nsIProxyObjectManager> proxyMgr = do_GetService(<proxy object manager
contract id>, &rv);

3) remove dead code.

example:

+/*
+// {9263B3F2-53D2-11d5-B69B-00B0D06E5F27}
+DEFINE_GUID(<<NS_COMM4XIMPORT_CID>>, 
+0x9263b3f2, 0x53d2, 0x11d5, 0xb6, 0x9b, 0x0, 0xb0, 0xd0, 0x6e, 0x5f, 0x27);
+*/


4) use nsCOMPtr.

an example from your code:

nsIStringBundle *pBundle = nsComm4xStringBundle::GetStringBundleProxy();
...
NS_IF_RELEASE( pBundle);

nsCOMPtr <nsIStringBundle> pBundle;
rv = nsComm4xStringBundle::GetStringBundle(getter_AddRefs(pBundle));

another example:

+
	nsIFileSpec *pSpec = nsnull;
+
	desc->GetFileSpec( &pSpec);
+
	if (pSpec) {
+
		pSpec->FromFileSpec( pLoc);
+
		NS_RELEASE( pSpec);
+
	}


no need to call release.

5)  use the proper macros, example:

replace this:

+    NS_PRECONDITION(_retval != nsnull, "null ptr");
+
if (!_retval)
+
	return( NS_ERROR_NULL_POINTER);

with

NS_ENSURE_ARG_POINTER(_retval);

6) see if you can remove nsComm4xStringBundle.

I think we can simplify.  I'm not convinced why we need nsComm4xStringBundle.

if we need to get a string by id or name, why not just get the string bundle server
and get the string bundle (for
"chrome://messenger/locale/comm4xImportMsgs.properties")
and that's it.

in the past we've had string build helper objects to cache the string bundle
object for performance
reasons.  if performance is still an issue (I'm not sure it is in the 6 or 7
times you use the string bundle)
you can always have a member variable in the class that needs the string bundle.

	nsCOMPtr <nsIStringBundle> mBundle;


7)

+void ImportAddressImpl::ReportError( PRInt32 errorNum, nsString& name, nsString
*pStream)
+{
+
if (!pStream)
+
	return;
+
// load the error string
+
nsIStringBundle *pBundle = nsComm4xStringBundle::GetStringBundleProxy();
+
PRUnichar *pFmt = nsComm4xStringBundle::GetStringByID( errorNum, pBundle);
+
PRUnichar *pComm4x = nsTextFormatter::smprintf( pFmt, name.GetUnicode());
+
pStream->Append( pComm4x);
+
nsTextFormatter::smprintf_free( pComm4x);
+
nsComm4xStringBundle::FreeString( pFmt);
+
pStream->AppendWithConversion( char(nsCRT::LF));
+
NS_IF_RELEASE( pBundle);
+}

bad string usage.  use nsXPILDString

take a look at
http://lxr.mozilla.org/seamonkey/source/mailnews/local/src/nsPop3Protocol.cpp#1370

I've stopped reviewing at this point.  I'll review again when the patch makes it
back for super review.

Comment 66

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

I know the l10n deadline is coming.  nothing stops us from landing *approved*,
finalized strings from jatin before the 6/26 deadline (so the localizers can
localize) and then landing the code that uses those strings later, when it is ready.

Comment 67

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

I'm against having the version.  let's use the brandShortName, like the rest of
the product.

"Netscape Communicator 4.x has one mailing address for each card, while Netscape
6 has two (Home and Work)."

do an LXR search for brandShortName (defined in the ns tree as "Netscape 6")
http://beckett.mcom.com/commercial/source/xpfe/global/resources/locale/en-US/brand.properties

do it like this:

<!--LOCALIZATION NOTE Don't translate "&brandShortName;" --!>
<!ENTITY foobar "blah blah &brandShortName; blah blah">

Comment 68

[lrg]

I have found a bug which occurs when trying to import empty address books or 
address books with blank entries (cards).  This will cause the application to 
crash on linux.  On MacOS the crash only occurs with empty address books or with 
address books with nothing but blank entries (as opposed to on linux).  This 
doesn't seem to occur on windows.

Comment 69

[lrg]

Attachment: Logfile of Blank address book crash on MacOS

Comment 70

[Rajiv Dayal]

Attachment: import comm4x address book strings for localization checkin in mozilla

Comment 71

[Rajiv Dayal]

Attachment: import comm4x address book strings for localization checkin in ns

Comment 72

[chuang]

r=chuang for localization checkin.

Comment 73

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

rs=sspitzer  (rs=rubber stamp)

note, I think you need localization notes around "*.na2" and "pab.na2".

those strings should not be localized.

Comment 74

[Rajiv Dayal]

Attachment: patch for mozilla with additional localization note for "pab.na2" / ".na2"

Comment 75

[Rajiv Dayal]

Attachment: patch for ns with additional localization note for "pab.na2" / ".na2"

Comment 76

[Rajiv Dayal]

Tracked down the issue of 2 address book being created when reusing address book 
code for LDIF to MAB conversion. This was due to a problem in the the address 
book code, the 'AddressBookParser::ParseFile' function used the variable 
"mStoreLocAsHome" to determine if it is called to convert 4.x to 6.x (or 
for migration). However, this variable represents the bool value for whether the 
user selected Work or Home as the category to store the mailing address. Thus 
when "mStoreLocAsHome" is false (Work category selected by user) the condition 
fails and it goes ahead to create another address book. 
Fixed this to use another variable to determine whether the function is called 
for Import comm4x.

Comment 77

[chuang]

Good catch, Rajiv.  For current case, checking mDatabase should work right since
migration will have a null mDatabase.  But to prevent future confusion, passing
another variable should be a better fix.

Comment 78

[Rajiv Dayal]

Also Candice it worked for u because when you called it from the import comm4x 
you always passed PR_TRUE for the "bStoreLocAsHome" parameter of the address 
book Convert function and thus the condition always succeeded and did not create 
another address book.

I have defined another varible for the check and am not using the "mDatabase" 
variable. There also exists another variable "bMigrating" to determine if it is 
called from migration or any other place but i am not using that too to avoid 
future problems if in case the function is used by any third module later.

Comment 79

[olga charapaev]

Tested on linux. Looks OK.

Have a suggestion though:
in the Import Wizard window which allows to select Home or Work mailing address
category rename the "Next" button to  "Import". It'll make it less confusing
since importing starts right after user hits this button.

Comment 80

[Tiantian Kong]

Copied this email for future reference.

*****************************
Hi, Olga:

Thx for the link.

Please include testing of the special situations below across three platforms
for NS4.x address book import.

   1. importing empty address book file.
   2. importing address book which has blank card.
   3. importing address book where the file path is very long. 

If you can think of any other special situations, please test as well.

In addition, please perform some integration testing, specifically, using email
address in the imported address book in each application (outlook, outlook
express, Eudora, text, NS4.x),

   1. send email
   2. forward email
   3. send email to a group


Also it will be very helpful if you can talk to Esther to see if she has some
ready made test data. Thx.

Tiantian




Olga Charapaev wrote:

http://www.mozilla.org/quality/mailnews/tests/sea_mn_addrB_import.html

Comment 81

[Tiantian Kong]

Changed QA contact to [e-mail], since Luke has handed over the testing
to Olga.

Comment 82

[Dean Pinckert]

Attachment: Macintosh project file 7/9/01

Comment 83

[Rajiv Dayal]

Attachment: patch file with latest Comm4x AB import for mozilla

Comment 84

[Rajiv Dayal]

Attachment: patch file with latest import comm4x AB code for ns

Comment 85

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

overall, it is looking great.

you still need chuang to review it.

while you wait for her to review, here are some things I found:

1)  should this be removed or uncommented?

+
	  /* commented to make it work right - local testing
+      if (mStoreLocAsHome )
+        mDatabase->AddHomePhone(newRow, column);
+      else
+
	  */

2)

your last patch included

nsComm4xStringBundle.h

that's no longer used, right?

3)

#define kWhitespace     " \t\b\r\n"

unused.  if it was used, you'd use nsCRT::IsAsciiSpace().

4) use do_GetService() instead of NS_WITH_SERVICE()

NS_WITH_SERVICE( nsIImportService, impSvc, kImportServiceCID, &rv);
NS_WITH_SERVICE( nsIImportService, impSvc, kImportServiceCID, &rv);

5) use do_GetService() instead of NS_WITH_SERVICE()

NS_WITH_SERVICE( nsIProxyObjectManager, proxyMgr, kProxyObjectManagerCID, &rv);
NS_WITH_SERVICE( nsIProxyObjectManager, proxyMgr, kProxyObjectManagerCID, &rv);

6)

nsISupports *pInterface;

that can be a nsCOMPtr to make your code cleaner.

7)

I'd make m_fileLoc an nsCOMPtr to clean up your code.

8)

(const char*) pName

do pName.get() instead

when ever you have a nsXPIDLCString or nsCAutoString, and you want a const char
*, do .get()

same goes for nsXPIDLString / nsAutoString and const PRUnichar *.  use .get()

9)

IsLDIFFile() and CountFields() are copied from nsTextAddress.cpp

let's figure out a way to use the existing code instead of copying it.

10)

I think you can use NS_XPCOMPROXY_CONTRACTID instead of kProxyObjectManagerCID

You can use contract ids instead of CIDS for these, also:

+static NS_DEFINE_CID(kImportServiceCID,		NS_IMPORTSERVICE_CID);

you could use "@mozilla.org/import/import-service;1" or even better, add 

#define NS_IMPORTSERVICE_CONTRACTID "@mozilla.org/import/import-service;1"

to nsIImportService.idl and then fix nsImportFactory.cpp and your code to use
NS_IMPORTSERVICE_CONTRACTID


+static NS_DEFINE_CID(kABUpgraderCID,            NS_AB4xUPGRADER_CID);

use NS_AB4xUPGRADER_CONTRACTID 

11)

+  if (mDatabase)
+    mDeleteDB = PR_FALSE;
+  else
+    mDeleteDB = PR_TRUE;

why not just do:

mDeleteDB = mDatabase ? PR_FALSE : PR_TRUE;

Comment 86

[chuang]

I've given my r=chuang this morning per email.

Comment 87

[Håkan Waara]

Let me sneak in a review too. :)

1. Please don't insert new tabs in our tree. The standard is to use spaces. In
MSVC6, there's a function "Untabify"... Would be great if you could use spaces
in your initial version.

2.
+
NS_IMETHOD GetSampleData( PRInt32 index, PRBool *pFound, PRUnichar **pStr)
+
{ return( NS_ERROR_FAILURE);}
+
+
NS_IMETHOD SetSampleLocation( nsIFileSpec *)
+
{ return( NS_OK); }

Are those functions supposed to be used? Or are they just not implemented?

Consider returning NS_ERROR_UNIMPLEMENTED .

3. +private:
[...]
+private:

Any reason you specify private twice in the beginning of nsComm4xImport.cpp?

4.
+
return( rv);

Optional: return rv; ?

5. You are recreating a COMM4X_MSGS_URL stringbundle in ::Initialize() when you
did this in your constructor (and fed it to m_pBundle) already? I don't think
you need pBundle in ::Initialize at all.

6. I don't know about this, but should the IMPORT_LOGO() calls go into the CVS?
Are they printing to the console?

Other than that, it looks very good! r=hwaara

Comment 88

[Rajiv Dayal]

for Hakan Waara comments :

1. i will replace tabs by spaces, this is my first code check in to the tree,
didnt know about it
2. These functions could be called from the base Import class so rather then
defining them as unimplemented i followed the convention in other import
sub-modules to either return error or success.
3 and 4 - i will change (is this mozilla style thing - return rv, not return
(rv), if so i will take care in future).
5. The m_pBundle is in class 'nsComm4xImport' whereas Initialize() is member of
'ImportComm4xAddressImpl' class and it doesnot contain the nsComm4xImport class,
hence it is required to create the pBundle again in Initialize.
6. yes IMPORT _LOG0 prints on console it is enabled only in debug mode, NS_ERROR
throws dlg box on win which gets annoying for QA so i use it to print
informational msgs during debug. it is used by other import sub-modules for
printing err msgs too but for error msgs i am using NS_ERROR. 

Comment 89

[Rajiv Dayal]

for Seth's comments :

1. will remove it. i guess candice put it for some testing purpose.
2. nsComm4xStringBundle.h still needs to be used, it has macros defined for
string IDs.
3.good to know about nsCRT::isAsciiSpace(), thanks
4. and 5. NS_WITH_SERVICE is defined as do_GetService so i did not change, also
i saw NS_WITH_SERVICE being a standard usage in other mailnews code.
6 and 7. will change code
8.good to know more about the string classes
9. this will require isLDIFFile to be exported from the address book class, in
fact the best solution is that the ConvertLDIFtoMAB does this check.
10.will define contract id for ImportService which is the best solution, i am
using NS_WIH_PROXY_SERVICE for nsAB4xUpgrader which does not take contract id,
replacing it with code will mean adding several more lines, so did not change
it.

Comment 90

[Håkan Waara]

Regarding nsComm4xStringBundle.h, it's good if you can use as few customized 
stringbundle as possible and use nsStringBundle.h (nsIStringBundle.idl)

Comment 91

[Rajiv Dayal]

i was using nsIStringBundle.h. I apologize for the confusion, i have now removed 
the nsComm4xStringBundle class declaration, nsComm4xStringBundle.h now only has 
the macros for string ids.

Comment 92

[olga charapaev]

Tested provided 07.11 build on WinNT 4.0

Feature works as expected, 
mentioned problem with long path name is fixed.

No regressions found in importing Outlook, Outlook Express,
text, Eudora

Comment 93

[Brian Nesse (gone)]

Attachment: .sit archive with latest mac project file and BuildList.pm diff.

Comment 94

[Rajiv Dayal]

Attachment: mozilla patch for checkin with all comments incorporated

Comment 95

[Rajiv Dayal]

Attachment: ns patch for checkin with all comments incorporated

Comment 96

[chuang]

r=chuang for 7/18's patch

Comment 97

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

sr=sspitzer on the last two patches (assuming those are the patches I reviewed
over email)

I think #85489 is now invalid.

Comment 98

[Rajiv Dayal]

Attachment: patch with only packager file diffs for new comm4x binary

Comment 99

[Blake Ross]

Missed the 0.9.3 train.

Comment 100

[Brian Nesse (gone)]

Actually this code was checked in last week.

Resolving as FIXED to trigger a QA cycle.

Comment 101

[Rajiv Dayal]

Also Luke from eClient QA did the testing with the last Friday builds. Luke can
you please put in ur comments.

Comment 102

[lrg]

All of the problem that i found while this bug was in development have been
fixed, I think that anything new probably will be filed into seperate bugs. 
This feature seems to be working well on all three major platforms as of the
friday build.

Comment 103

[olga charapaev]

Rajiv, 
I see your checkins only in commercial builds but not in mozilla.
Testing commercial.

Comment 104

[Rajiv Dayal]

This feature is available only in Netscape and not mozilla.

Comment 105

[Håkan Waara]

Why can't this go into the mozilla tree too? Why was it filed as a bugzilla bug
in the first place?

Comment 106

[Hong Kwon]

I believe this was originally intended to be a mozilla bug but because it had
dependencies on commercial code it was made into a commercial only bug.  Note
the comment by sspitzer:

>------- Additional Comments From Seth Spitzer 2001-06-21 22:45 ------
>
>this bug needs to move to bugscape.  
>
>this will not be a mozilla feature, since it depends on the 4.x addressbook code
>which only ships with the Netscape commercial build.
>
>asa, can you help me to move it?

Are there any objections to moving this to Bugscape?

Comment 107

[Håkan Waara]

Dangit, that's sad. I, for one was awaiting this, and I'm sure a lot of the
non-netscape people on the CC did too. :(

Comment 108

[Hong Kwon]

>------- Additional Comments From Seth Spitzer 2001-06-15 20:33 -------
>
>[e-mail]:
>
>>4.x -> 0.9.1 will not migrate your 4.x address book.  even after this fix, you
>>won't be able to do it by hand.
>>
>the reason is the 4.x addressbook migration code is based on some non-public
>code (that was originally part of 4.x) that we don't own.
>
>you'd have to use Netscape 6.x to get that feature.  you can do it by hand by
>exporting your ab in 4.x to ldif, and then importing the ldif back into to
>mozilla.

Comment 109

[olga charapaev]

Feature is working well.
Problems found will be filed into seperate bugs in Bugscape.
Marking as VERIFIED