124217 - Addressbook export tool does not export correctly

Status: VERIFIED FIXED

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

Description

[Shay]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020406

This is similar to bug: 120259
ie. 
Exported address books are not in the format selected.
Plus Saving of CSV file is incorrect.


Reproducible: Always
Steps to Reproduce:
In address book
1) Choose File->export
2) In save as type: select csv (or tab or txt)
3) type in File name without ending tag (eg .csv)

Actual Results:  
1) Output file should be appended with the correct extension
2) Regardless of the selected extension, the output format is always LDIF


Expected Results:  
1) Output file  is not appended with the extensions as expected
2) Output file should be in the format selected in the Save as Type field.

Furthermore, when saving in CSV format, each field should be enclosed in quotes,
and if quotes are used in the field it should be escaped with \".
The problem of not doing this is apparent when a comma is used in say an address
field. eg:
(1)
firstname,lastname,15 Philmore Drive, Cresent apartments

(2)
"firstname","lastname","15 Philmore Drive, Cresent apartments"

There is no way of deciding if (1) or (2) is the correct way of interpreting the
data. ie. 4 fields in (1) and 3 fields in (2)

Comment 1

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

I bet it works if you say the filename is

"foo.csv"

if I remember the code, we probably use the filename to decide what the type 
should be.  (which is wrong.)

and if we don't know, we default to ldif.

Comment 2

[racham]

Seth is right. If you append the extension (which is very unintuitive), it works
fine. This behavior can just be treated as a work around. We need to append the
right suffix and do the right thing to avoid any confusion.

Comment 3

[racham]

s/unintuitive/counter-intuitive

Comment 4

[Shay]

Yes, you're right, it does work if you explicitly give it an extension. I'm
sorry, perhaps I should have explained more clearly in the first post. Both CSV
and TAB work.

That's how I realised that in CSV format, each field is not delimited by quotes,
making paring of the csv file ambiguious.

Cheers,
shay

Comment 5

[Steve Meredith (gone)]

Attachment: Patch to fix export type, quotes, and commas in data.

This patch does the following to address the issues listed in this bug:

Use the dropdown in the file picker to dictate what type of export this is, not
the file extension.

If the extension is missing or incorrect for the selected export type, add an
appropriate extension.

For .csv exports, surround each data value in quotes. This allows commas to
appear in the data without confusing the fields

For .csv exports, remove existing double quotes from data and replace them with
single quotes. This prevents double quotes from confusing the fields. I thought
about escaping double quotes, but there doesn't seem to be a real standard for
handling them across apps, so I thought replacing them with single quotes is a
good alternative.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 106536 [details] [diff] [review]
Patch to fix export type, quotes, and commas in data.

Just a few minor nits:

>+  nsCOMPtr<nsILocalFile> localFileRef;

Why is the variable necessary -- wouldn't footprint be smaller if you just used
localFile directly?

>+                nsAutoString quotedStr;
>+
>+                // Surround data with quotes.
>+                quotedStr.Append(NS_LITERAL_STRING("\"").get());
>+                quotedStr.Append(tempStr);
>+                quotedStr.Append(NS_LITERAL_STRING("\"").get());
>+
>+                tempStr = quotedStr;
>+
or

If you use operator+, you can avoid unnecessary allocations.  Give this a shot:

tempStr = PRUnichar('"') + tempStr + PRUnichar('"');

If the compiler barfs on that, you may need to fall back to:

tempStr = NS_LITERAL_STRING("\"") + tempStr + NS_LITERAL_STRING("\"");

>+              } 
>+              else {
>+
>+                tempStr = value.get();
>+              }

I don't _think_ a .get() is necessary here, but I could be wrong.

Comment 7

[Steve Meredith (gone)]

Attachment: New patch with Dan's suggestions.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 106762 [details] [diff] [review]
New patch with Dan's suggestions.

r=dmose

Comment 9

[racham]

Reassigning to Steve.

Comment 10

[Ninoschka Baca]

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

Comment 11

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

Comment on attachment 106762 [details] [diff] [review]
New patch with Dan's suggestions.

1)

I don't think you are following the "standard" way.  The reason I quote it, is
there doesn't seem to
be an official standard, but I think we should follow microsoft excel.

I also tried out msft excel, and it seemed to follow the rules specified here:
http://www.creativyst.com/Doc/Articles/CSV/CSV01.htm#FileFormat

we want to play nice with the 800 lb csv / tab gorilla (msft excel, other msft
apps), and
make sure that if we export from mozilla -> import to mozilla (say on another
machine) we
do the right thing.  (I'm less worried with breaking 4.x)

2)

csv:

+ rv = ExportDirectoryToDelimitedText(aDirectory, CSV_DELIM, CSV_DELIM_LEN,
localFile, PR_TRUE);

tab / txt:

+ rv = ExportDirectoryToDelimitedText(aDirectory, TAB_DELIM, TAB_DELIM_LEN,
localFile, PR_FALSE);

I think you should remove the new argument, and both should act the same.

for msft excel, both tab and cvs appear the same rules (just the delimiter is
different)

3)

+		 // Remove any double quotes from the data and replace with
singe quotes

typo in the comment "singe" -> "single"

4)

msft doesn't add quotes to all fields (but I think the import code should
handle it.)
msft doesn't replace CR / LF, it quotes them
msft doesn't replace " with ', it does ""

5)

I think we have to worry about spurious tabs in .tab or .txt format

I think I can type "foo<tab>bar" in notepad, copy to notes (or some other
field) and paste.

then when we export, we export the tab.  I'm not sure if we should replace it
with ' ' or wrap it with quotes.
(I couldn't make microsoft excel do it.)

6)

have you fixed the import code to handle how you are exporting?  we should do
that.

Comment 12

[Hostalet]

Names with written accent are not not exported correctly.
For example, my name is "José Manuel Hostalet Wandosell"
The exported file says:

dn::
Y249Sm9zw6kgTWFudWVsIEhvc3RhbGV0IFdhbmRvc2VsbCxtYWlsPWpvc2UuaG9zdGFsZXRAaXBhLmZoZy5kZQ==
objectclass: top
objectclass: person
objectclass: organizationalPerson
objectclass: inetOrgPerson
objectclass: mozillaAbPersonObsolete
givenName:: Sm9zw6kgTWFudWVs
sn: Hostalet Wandosell
cn:: Sm9zw6kgTWFudWVsIEhvc3RhbGV0IFdhbmRvc2VsbA==
mail: [e-mail]
modifytimestamp: 0Z

sometimes the field dn:: is too long and generates very big files
The problem is reproducible always with any name with written accent
The problem dissapear when the names are written without accent

Best regards.
José.

Comment 13

[Rafael Ebron (:rebron)]

Mail triage: nsbeta1+/adt3

Comment 14

[Cavin Song]

Steve's patch for missing file extension is still good. But I made it so that if
a string contains at least one comma, tab or double quote then we need to quote
the entire string. In aditon, if double quote is part of the string we need to
quote the double quote(s) as well. Here are some examples:

 String      String in exported file       Imported string
 ------      -----------------------       ---------------
 a b    -->  a b                      -->  a b
 a, b   -->  "a, b"                   -->  a, b
 a\tb   -->  "a\tb" ('\t' = tab)      -->  a\tb
 a"b    -->  "a""b"                   -->  a"b
 "ab"   -->  """ab"""                 -->  "ab" 

The import code also handles the above now. Not sure if I missed anything here.
A patch is coming.

Comment 15

[Cavin Song]

Attachment: Proposed patch,v1

Comment 16

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

cavin, for the first part of your patch, 
take a look at the related bug #96134
("message Save As ignores selected file type")

see what I did, and the comments I added to that code.

http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMessenger.cpp#833

I think we should do the same thing in both places, to be consistent.
(both in code, and in behaviour)
since that bug is "export message", and this bug is "export addressbook"

I think in both places, we should do what you did in this patch, and check if
the extension of the file, and if not right, append it.  (that will solve some
open issues with bug #96134)

I'll continue to review.

Comment 17

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

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

see my previous comment.

I'll review the second part of the patch tonight.

Comment 18

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

the second part of the change to addrbook/src/nsAddressBook.cpp looks good.

two minor comments:

1)

+              PRBool hasQuotes = PR_FALSE;

name that "needsQuotes".  it doesn't make sense as "hasQuotes" when you set it
to true if you find a tab or a comma.

2)

+              PRInt32 pos = newValue.FindChar('"');
+              if(pos != kNotFound)
+              {

pos is only used there, so instead do:

+              if (newValue.FindChar('"') != kNotFound)
+              {

the change to import/text/src/nsTextAddress.cpp looks reasonable, too.

but make sure that exporting / import to .tab format still works (under all
those special cases.

for references, about the csv format, see
http://www.creativyst.com/Doc/Articles/CSV/CSV01.htm#FileFormat

Comment 19

[Cavin Song]

Attachment: Proposed patch, v2

Apply the same logic for selected file type in nsMessenger::SaveAs() (ie, file
extension is based on file type selection). Also address 2nd review comment.

Comment 20

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

Comment on attachment 120245 [details] [diff] [review]
Proposed patch, v2

r/sr=sspitzer

but in all cases, let's do the same thing:

fileName.RFind(EML_FILE_EXTENSION, PR_TRUE, -1, strlen(EML_FILE_EXTENSION)) ==
kNotFound

can you fix the various places in the two files where we're not safely and
properly checking the file extension?

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

It should be possible to use sizeof instead of strlen to cause that to be
calculated at compile-time rather than run-time.  

Also, it's easier on debuggers to use "static const char[]" than #define.

Finally, is there some reason we need to use ".htm" instead of ".html"?

Comment 22

[Cavin Song]

Attachment: Addressed dmose's comments

Use sizeof() instead of strlen(), and allow either .htm or .html.

Comment 23

[Cavin Song]

Fix checked in.

Comment 24

[Ninoschka Baca]

Trunk build 2003-04-22: WinXP, Mac 10.1.5, Linux RH 8

Exported address books in txt and csv format and the extensions were added in
all cases. Importing the same file was also successful.

Thank you!