70396 - Non-ascii name of migrated 4.x address book is corrupted

Status: VERIFIED FIXED

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

Description

[marina]

**** observed with 2001-02-27 build ****
Steps to reproduce:
- create an AB in 4.x with non-ascii name;
- migrate it to Nscp6;
- open Address Book;
//note: all non-ascii chars in AB names are removed
(btw: the migrated AB are all empty but this is a core problem and i'll now if
there is a bug filed on that)

Comment 1

[nhottanscp]

Add momoi to cc, we had a problem with addressbook migration before RTM, but
that was fixed, right?

Comment 2

[marina]

yes, the problem was with non-ascii data inside the migrated addressbook
(similar to this one: non-ascii chars were removed but the address book name
itself had no pproblem), now the addressbook name has a problem and there are no
contents after migratin, it's empty.
assigning myself as QA contact

Comment 3

[ji]

The one for non-ascii address book CARD (not address book name) at rtm time is
bug 57151.

Comment 4

[nhottanscp]

Reassign to [e-mail].

Comment 5

[scottputterman]

nominating for nsbeta1

Comment 6

[scottputterman]

marking nsbeta1+

Comment 7

[scottputterman]

moving to mozilla0.9.1

Comment 8

[scottputterman]

I just wanted to verify that you are saying that all cards inside these
Non-ascii address books are lost?

Comment 9

[marina]

not now, all cards insided the addressbook are displaying fine, so there is no
data loss. the only problem is the address book name : for japanese name i see "
user_directory1.ldif"

Comment 10

[scottputterman]

Thanks Marina.  Then I'm going to move this to 0.9.2.  

Comment 11

[marina]

more info : non-ascii names are corrupted ( stripped off accented chars or
showing "user_directory1" for double-byte ) only the first time you run Netscp6
after migration, the second time you open this profile it shows up correctly.

Comment 12

[scottputterman]

moving to 0.9.3

Comment 13

[Jaime Rodriguez, Jr.]

Marking as nsCatFood, because of migration (i.e. see Project Goals).

Comment 14

[marina]

just confirming my comments of 2001-05-01 14:00 -----: non-ascii AB name shows
up without accented chars for Latin-1 and as "user_directory1.ldif" for DB name
only the first time you open application after migration. Close-reopen will fix
this problem ... though all ldif directories from 4.x are gone... ( netcenter,
verisign and so on...)

Comment 15

[scottputterman]

moving to 0.9.4

Comment 16

[Cindy Roberts]

Selected as one of the 3 top intl bugs for eMojo mail

Comment 17

[Scott MacGregor]

keyword magic. Nothing to see here.

Comment 18

[chuang]

Move to 0.9.5

Comment 19

[msanz]

chuang, is there any update in this bug?

Comment 20

[Jaime Rodriguez, Jr.]

Why are we nominating this one, if there isn't a patch? Propose we minus

Comment 21

[marina]

why don't we go the other way around and work on the patch..? :-) This one is
more cosmetic because the problem goes away wjehn close/reopen AB, though the
user doesn't know it and it look really ugly after migration.

Comment 22

[chuang]

I'm working on it.  I don't see the garbage-like address book name with Marine's
file.  What I see is we are using the filename instead of the real display name.  

Comment 23

[chuang]

Attachment: Patch for the fix

Comment 24

[chuang]

This patch also include the fix for bug 41887, 93438.

Comment 25

[Jaime Rodriguez, Jr.]

Adding correctness Status Whiteboard, correct/expected behavior does not occur.

Comment 26

[selmer (gone)]

Are you pursuing the reviews for the patch?  It's pretty large.  I'm not totally
convinced this is stop-ship, but if it were reviewed and fully tested by 9/26
I'd consider it...

Comment 27

[Scott MacGregor]

The patch for this does look pretty large and we've lost our address book expert
so we don't have anyone right now who can guage the risk for this patch. I'd
like to nominate nsBranch- for this. We'll get it into the trunk and get plenty
of testing time for the next release.

Comment 28

[msanz]

Scott, feel free to change it to nsbranch-. It's too late now. Marina, make sure
this goes into the international section in the release notes.

Comment 29

[Scott MacGregor]

thanks. putting in the next milestone so it'll make the next release. 

Comment 30

[scottputterman]

reassigning to cavin

Comment 31

[scottputterman]

moving to 0.9.7

Comment 32

[marina]

this bug has a lot of keywords but is still NEW. Cavin, it is really ugly for
Intl users, are we planing to fix it in this release?

Comment 33

[scottputterman]

It's an accepted bug (the nsbeta1+) but there's a very high chance that because
it's a P3 that it won't be.  Changing the milestone to reflect the fact that it
hasn't been targetted yet.

Comment 34

[marina]

Scott, i am confused.. Do we plan to fix it in this release, do i re-nominate?
Thanks.

Comment 35

[scottputterman]

No, we don't plan to fix it in this release.  This was a bug that we'd like to
fix, but we don't have enough time. If you want to renominate, you should change
the nsbeta1- to an nsbeta1.

Comment 36

[Michael Buckland]

Discussed in mail news bug meeting.  Decided to minus this bug.

Comment 37

[Frank Tang]

move nsbeta1- to nsbeta1 . please consider this as nsbeta1+ for m1.2final

Comment 38

[Simon Montagu :smontagu]

Gregg, can you escalate this bug to the mail team? it seems to be a nasty issue.
(this is dassi btw, not simon)

Comment 39

[Gregg Landskov (gone)]

This one has been around for quite a while...it seems we have a patch.  Why
don't we try to get it on the trunk, get some testing on it, and get it for Buffy?

Comment 40

[Samir Gehani]

i18n triage team: nsbeta1+/adt2

Comment 41

[Cavin Song]

Attachment: Proposed patch, v1

In ParseFile(), try to use 'description' pref value as the dir/addrbook name if
it exists, otherwise use the leafname of the addrbook filename. Currently we
only do this for persoanl addrbook.

The problem with missing cards in the migrated addrbooks is caused by the fact
that the na2 files were not copied over to the new profile directory so when we
tried to convert/import the 4.x addrbooks no cards were ever migrated. So
CopyFilesByExtension() is created to do that during migration and the na2 files
are removed once they are migrated.

Comment 42

[Cavin Song]

> The problem with missing cards in the migrated addrbooks is caused by the fact
> that the na2 files were not copied over to the new profile directory
>
This is a regression from July 4, 2002 for Bug 15424. The original code was:

  rv = DoTheCopy(oldProfilePath, newProfilePath, PR_FALSE);                     

'oldProfilePath' points to the root of the 4.x dir so all files were copied
(including .na2 files). The change was to replace the above with:

  // just copy what we need                                                    
                                                                             
  rv = DoTheCopy(oldProfilePath, newProfilePath, COOKIES_FILE_NAME_IN_4x);
  . . .

So we do selective copies now and so we don't copy .na2 files any more.

Comment 43

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

let's start with the change to nsPrefMigration.cpp / nsPrefMigration.h  as that 
will fix http://bugscape/show_bug.cgi?id=21642

1)

for this code:
+    nsFileSpec fileOrDirName = dir.Spec();    //set first file or dir to a 
nsFileSpec
+    fileOrDirNameStr.Assign(fileOrDirName.GetLeafName());
+
+    if (fileOrDirName.IsDirectory() || !nsCStringEndsWith(fileOrDirNameStr, 
fileExtension))
+      continue;

do this instead:

+    nsFileSpec fileOrDirName = dir.Spec();    //set first file or dir to a 
nsFileSpec
+    if (fileOrDirName.IsDirectory())
+      continue;
+
+    nsCAutoString fileOrDirNameStr.Assign(fileOrDirName.GetLeafName());
+    if (!nsCStringEndsWith(fileOrDirNameStr, fileExtension))
+      continue;

that saves us from getting the leaf name for directories.

2)

for this:

+    rv = fileOrDirName.CopyToDir(newPath);
+    NS_ASSERTION(NS_SUCCEEDED(rv),"failed to copy file");
+    NS_ENSURE_SUCCESS(rv,rv);

just do

+    rv = fileOrDirName.CopyToDir(newPath);
+    NS_ENSURE_SUCCESS(rv,rv);

you don't need that assertion, as NS_ENSURE_SUCCESS(rv,rv) will assert.   
(it won't have as useful of an error message, but we will still know where it 
is asserting)

3)

+  nsresult rv;
+  nsFileSpec oldPath;
+  nsFileSpec newPath;
+
+  rv = oldPathSpec->GetFileSpec(&oldPath);

instead:

+  nsFileSpec oldPath;
+  nsFileSpec newPath;
+  nsresult rv = oldPathSpec->GetFileSpec(&oldPath);

just a style nit.


4)

before you call CopyFilesByExtension(), do this:

#include "nsIAbUpgrader.h"

...

// if we have the upgrader, copy over the .na2 files
// so we can migrate them.
nsCOMPtr <nsIAbUpgrader> abUpgrader = do_GetService(NS_AB4xUPGRADER_CONTRACTID);
if (abUpgrader) {
  // Copy the addrbook files
  rv = CopyFilesByExtension(oldProfilePath, newProfilePath, 
ADDRBOOK_FILE_EXTENSION_IN_4X);
  NS_ENSURE_SUCCESS(rv,rv);
}

can you attach a new patch for nsPrefMigration.cpp / nsPrefMigration.h, so I 
can r/sr seperate from the i18n issue?

Comment 44

[Cavin Song]

Attachment: Fix for empty addrbook migration

Comment 45

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

Comment on attachment 117175 [details] [diff] [review]
Fix for empty addrbook migration

this looks good, but I forgot that using the nsIAbUpgrader would add a
dependency on mailnews addrbook.

(remember, you can build mozilla with mailnews disabled, and also install
mozilla as browser only.)

let me double check with ccarlen and alecf about this.

Comment 46

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

waiting for feedback from ccarlen, but it is looking like adding the dependency 
on addrbook isn't the way to go.

sorry about suggesting it.

can you attach a new patch, that always copies over the .na2 files (like you 
originally had it?)

Comment 47

[Cavin Song]

Attachment: Fix for empty addrbook migration w/out abupgrader.

Comment 48

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

Comment on attachment 117236 [details] [diff] [review]
Fix for empty addrbook migration w/out abupgrader.

sr=sspitzer

requesting review from ccarlen.

Comment 49

[Conrad Carlen (not reading bugmail)]

Comment on attachment 117236 [details] [diff] [review]
Fix for empty addrbook migration w/out abupgrader.

r=ccarlen

Comment 50

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

Comment on attachment 117040 [details] [diff] [review]
Proposed patch, v1

just a few small nits on the parts of this patch (nsPrefMigration.cpp/.h are 
part of another patch.)

1)

+    if (PL_strcmp(fileName, kPersonalAddressbook) == 0)

I know you are just moving that code, but let's do this instead:

+ if (strcmp(fileName, kPersonalAddressbook) == 0)

2)

+    // If a name is found then use it, otherwise use the filename
+    // as last resort. (ie, it's ok to ignore rv here).
+    if (!dirName.IsEmpty())

why not:

+    // If a name is found then use it, otherwise use the filename
+    // as last resort.
+    if (NS_SUCCEEDED(rv) && !dirName.IsEmpty())

if GetLocalizedUnicharPref() fails, (because there is no .description pref), we
don't need to check if dirName is empty.

3)

+
+    // If a name is found then use it, otherwise use the filename
+    // as last resort. (ie, it's ok to ignore rv here).
+    if (! dirName.IsEmpty())
+      parentDir->CreateDirectoryByURI(dirName, mDbUri, mMigrating);
     else
+    {
+      nsAutoString fileString;
+      fileString.AssignWithConversion(leafName);
	 parentDir->CreateDirectoryByURI(fileString.get(), mDbUri, mMigrating);
+    }

instead:

+    // If a name is found then use it, otherwise use the filename
+    // as last resort.
+    if (NS_SUCCEEDED(rv) && !dirName.IsEmpty())
+      dirName.AssignWithConversion(leafName);
+
+    rv = parentDir->CreateDirectoryByURI(fileString.get(), mDbUri,
mMigrating);
+    NS_ASSERTION(NS_SUCCEEDED(rv), "failed to create directory");

can you attach a new patch, excluding the nsPrefMigration code?

Comment 51

[Cavin Song]

Attachment: Fix for setting the right non-ascii addrbook names.

Comment 52

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

Comment on attachment 117290 [details] [diff] [review]
Fix for setting the right non-ascii addrbook names.

one fine string suggestion:

+      nsCAutoString prefName;
+      prefName = NS_LITERAL_CSTRING("ldap_2.servers.") +
nsDependentCString(leafName) + NS_LITERAL_CSTRING(".description");

after that, r/sr=sspitzer

don't forget to check in your change to nsMessengerMigrator.cpp, too.

nice work, cavin.

Comment 53

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

> one fine string suggestion:

I meant, one final string suggestion.

that came out as tooting my own horn.

beep beep.

Comment 54

[Cavin Song]

Fix checked in.

Comment 55

[Cavin Song]

Attachment: Remove .na2 file once it's migrated.

Comment 56

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

Comment on attachment 117523 [details] [diff] [review]
Remove .na2 file once it's migrated.

r/sr=ssputzer, I think I already reviewed this, right?

Comment 57

[Cavin Song]

Last fix checked in.

Comment 58

[marina]

it is fixed, after migration of 4.x the non-ascii AB name shows correctly