Contact import mapping problem

Created attachment 47323
CSV testcase

Mapping used in testcase:
    Work Phone -> Work
    Last Name -> Last Name
    First Name -> First Name
    Custom 1 -> VM Box
    Home Phone -> Home
    Pager Number -> Pager
    Fax Number -> Fax
    Cellular Number-> Cell Phone
    Primary Email -> E-mail
    Notes -> Other

All other AB fields were deselected.

Marking these all WORKSFORME sorry about lack of response but were very
overloaded here. Only reopen the bug if you can reproduce with the following steps:

1) Download the latest nightly (or 0.9.6 which should be out RSN)
2) Create a new profile
3) test the bug again

If it still occurs go ahead and reopen the bug. Again sorry about no response
were quite overloaded here and understaffed.

reopening.

bug 101145 had a similar problem with blank Display Name, later fixed in bug
99391.

mythdraug: are you still seeing this bug?

Yes. Just verified that with 2002020703, the testcase above does
not generate a displayname when imported.

taking all of chuang's bugs. she doesn't work on mozilla anymore.

over to cavin

Created attachment 164315
Patch to 97294

This patch will patch nsTextAddress.cpp/ProcessLine to add the generation of
the "Display Name" field should none be provided by a text-format import. This
should resolve this case.

I would note that other importing modules are not effected by this patch and I
am unaware of whether they suffer from the same lack of functionality. It would
be much more elegant to solve this within AddCardRowToDB but I'm not sure that
is entirely possible without signifigant revision or even needed.

Marked this for review. I'm not sure who is doing the reviewing for this, so I
ended up marking it for two different people; my apologies for the annoyance.

Comment on attachment 164315
Patch to 97294

Thanks for the nice patch, sorry its taken so long for someone to look at it.
Works well, but could do with a couple of tweaks:

+ switch( fieldNum) {
...
+ }

Please add in a default: break; option to the switch. Although not strictly
required, I believe its better practice to have one.

+ displayName.Append(PRUnichar(' '));

It should be:

displayName.Append(NS_LITERAL_STRING(" ");

this aviods run-time conversions.

+ if (!firstName.IsEmpty()) {
+ displayName = firstName;
+ }
+ if (!firstName.IsEmpty() && !lastName.IsEmpty()) {
+ displayName.Append(PRUnichar(' '));
+ }

Move the second if inside the first if statement, and that way you can drop the
second check for !firstName.IsEmpty().

If you could update the patch with those changes, and re-request review from
myself, that would be good (hence I assigned the bug to you). If you can't do
that, reassign the bug to the default owner, and let us know.

(In reply to comment #11)
> (From update of attachment 164315 [edit])
> Works well, but could do with a couple of tweaks:

Ok, I've just been reminded that there are localisation issues here. Some
locales require the lastname before the first name. This is currently set via
the preference:

mail.addr_book.displayName.lastnamefirst

See
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abCardOverlay.js#502
and
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abCardOverlay.js#307

for how the new card dialog actually does it.

Also, should we have an option to turn this on and off? See bug 180057 for one
example where I added an option to skip the first line of imported files.

currently, in thunderbird 1.5.0.10, our users are really upset about the lack of such feature. May I ask what's the status of this ? Are we going to see this in tb-2 ?

Created attachment 363018
Adds DisplayName generation to text-based import.

I've updated the code to generate names in the same manner that Card's do (respecting the first/last order and format strings). I am using the displayName.autoGeneration preference to decide whether to do it or not, but a checkbox may still be desirable.

Scott: please set the review flag of the patch to the email address of a suitable reviewer - like standard8.
(See http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree)

Comment on attachment 363018
Adds DisplayName generation to text-based import.

Updated review flag to Mark Banner.

Comment on attachment 363018
Adds DisplayName generation to text-based import.

>+#define PREF_MAIL_ADDR_BOOK_DISPLAYNAME_AUTOGENERATION "mail.addr_book.displayName.autoGeneration"

I think we should expose this in the import UI. Some people may not have display names, and prefer not to generate them, I've just cc'ed our user-experience guy in case he disagrees.

>+ case 2:
>+ hasDisplayName = PR_TRUE;
>+ break;

What about the case where the user has chosen to import an existing address book, but hasn't got a display name in some/all rows?

>+ } else if (!primaryEmail.IsEmpty()) {
>+ // see bug #211078
>+ // if there is no generated name at this point
>+ // use the userid from the email address
>+ // it is better than nothing.
>+ displayName = primaryEmail;
>+ PRInt32 index = displayName.FindChar('@');
>+ if (index != -1)
>+ displayName.SetLength(index);

I'm not sure we want to copy this clause. If we haven't got a first/last name then I think generating one from the email in this case isn't such a good idea, and we should just give up.

(In reply to comment #17)
> I think we should expose this in the import UI. Some people may not have
> display names, and prefer not to generate them, I've just cc'ed our
> user-experience guy in case he disagrees.
>
> What about the case where the user has chosen to import an existing address
> book, but hasn't got a display name in some/all rows?

With that in mind, I would agree that having a check box (defaulting to the state of the preference in mail.addr_book.displayName.autoGeneration) would be best. In that case, the user can make the decision if only some rows have display names. In the case that the user would like to replace all of the display names, they could simply uncheck importing that field while auto-generate was enabled.

> >+ } else if (!primaryEmail.IsEmpty()) {
> >+ // see bug #211078
> >+ // if there is no generated name at this point
> >+ // use the userid from the email address
> >+ // it is better than nothing.
> >+ displayName = primaryEmail;
> >+ PRInt32 index = displayName.FindChar('@');
> >+ if (index != -1)
> >+ displayName.SetLength(index);
>
> I'm not sure we want to copy this clause. If we haven't got a first/last name
> then I think generating one from the email in this case isn't such a good idea,
> and we should just give up.

Having now read bug #211078, I would agree it was an ill-advised copy.

I will wait to hear the opinion of the user-experience guy before revising further.

Comment on attachment 363018
Adds DisplayName generation to text-based import.

Removed review flag pending suggested UI changes.

User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20101206 Ubuntu/10.04 (lucid) Firefox/3.6.13
Build Identifier: 3.1.7+build3+nobinonly-0ubuntu0.10.04.1

- When importing, Display Name is not automatically filled, causing the user portion of the email address to be used as the name.

- When importing a csv file, the file manager dialog opens a folder instead of a file, pressing the down arrow makes the selection go up (looks and acts very buggy).

- When importing csv contacts into TB, remapping using up/down is extremely complicated and shuffles the other ones around.

Reproducible: Always

Steps to Reproduce:
1. Tools > Import
2. Select CSV File
3. Mapping tool shows up, select/align fields
4. Import
Actual Results:
All contact names are the user portion of the email address. Example for <email address hidden>, bsmith is the display name used, even though "Bob" is the first name and "Smith" is the last name.

Expected Results:
If the import tool sees a First Name and Last Name and no display name, it should offer to fill in the Display Name automatically.

Original Bug:
Binary package hint: thunderbird

When importing contacts into thunderbird's address book, if the record has a First Name and a Last Name, these do net automatically get concatenated in Display Name (something which thunderbird usually does).

Steps to reproduce:
1) Tools > Import
2) Select CSV File
3) Mapping tool shows up, select/align fields
4) Import

What happens:
All contact names are the user portion of the email address. Example for <email address hidden>, bsmith is the display name used, even though "Bob" is the first name and "Smith" is the last name.

Expected:
If the import tool sees a First Name and Last Name and no display name, it should offer to fill in the Display Name automatically.

ProblemType: Bug
DistroRelease: Ubuntu 10.04
Package: thunderbird 3.0.4+nobinonly-0ubuntu4
ProcVersionSignature: Ubuntu 2.6.32-22.36-generic 2.6.32.11+drm33.2
Uname: Linux 2.6.32-22-generic i686
Architecture: i386
Date: Thu Jul 1 11:13:37 2010
InstallationMedia: Ubuntu 10.04 LTS "Lucid Lynx" - Release i386 (20100429)
ProcEnviron:
 LANG=en_US.utf8
 SHELL=/bin/bash
SourcePackage: thunderbird

Only one issue per bug please, and please be careful to search for duplicates first (across both Thunderbird and Mailnews Core products)

(In reply to comment #0)
> - When importing, Display Name is not automatically filled, causing the user
> portion of the email address to be used as the name.

Bug 97294

> - When importing a csv file, the file manager dialog opens a folder instead of
> a file, pressing the down arrow makes the selection go up (looks and acts very
> buggy).

I don't understand this. It sounds like either a linux specific bug or a ubuntu specific bug. After testing with a version of Thunderbird supplied by mozilla if the issue is still there, please file a new bug with clear steps to repeat, and possibly a screenshot or two.

> - When importing csv contacts into TB, remapping using up/down is extremely
> complicated and shuffles the other ones around.

Covered by Bug 95087 and others.

Resolving as invalid as we only generally deal with one issue per bug, and the ones raised are either dupes or need a clear separate issue raised.

Scott, will you be able to update the patch?

reported in http://getsatisfaction.com/mozilla_messaging/topics/address_book_not_working