256826 - Mozilla should not permit to import empty address book

Status: RESOLVED FIXED

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

Description

[Boying Lu]

Re-produce steps:
1. launch mozilla mail with a new profile
2. setup a mail account
3. select Window->Address Book menu item
   An address book window will popup and you will find the address book is empty
4. select Tools --> "Export..." menu item to export address book say foo.ldif
   Mozilla will create an empty file foo.ldif
5. select Tools --> "Import..." menu item to import foo.ldif

Comment 1

[Boying Lu]

Attachment: patch v0

Comment 2

[Boying Lu]

Comment on attachment 156961 [details] [diff] [review]
patch v0

Can you give r? Thanks

Comment 3

[[e-mail]]

Comment on attachment 156961 [details] [diff] [review]
patch v0

Sorry for not getting to this sooner.

>+    var errorText;
This isn't scoped properly, it should be declared where it is defined.

>+    if (file.fileSize == 0) {
>+      errorText = gImportMsgsBundle.getFormattedString('ImportEmptyAddressBook',[path]);
Nit: space after comma

>+      alert(errorText);
This should use the prompt service. It will assert in debug builds.

>+      return false;
>+    }
>+       
>     if (file == null) {
>       return( false);
>     }
Your new check should come after this important check!

Comment 4

[Boying Lu]

Attachment: patch v1

Thanks Neil, I re-made the patch based on Neil's feedback

Comment 5

[Boying Lu]

Comment on attachment 163817 [details] [diff] [review]
patch v1

Can you give r now?

Comment 6

[[e-mail]]

Comment on attachment 163817 [details] [diff] [review]
patch v1

>+      promptService.alert(window,
>+                        window.Title,
>+                      errorText);
Even if you meant window.title it would have asserted and logged a message in
the JavaScript console - and that's now that it works! I think you mean
document.title - and you might as well write this all on one line. r=me if you
fix this.

>+ImportEmptyAddressBook=Can't import empty address book %S
I looked at the other message and saw that it ended in a full stop, but then
wondered if that was such a good idea after all...

Comment 7

[Boying Lu]

Attachment: add javascript console output

Comment 8

[Boying Lu]

Comment on attachment 164064 [details] [diff] [review]
add javascript console output

Can you give r now?

Comment 9

[Boying Lu]

Attachment: add javascript consle output

Comment 10

[Boying Lu]

Comment on attachment 164067 [details] [diff] [review]
add javascript consle output

Can you give r now? Thanks

Comment 11

[[e-mail]]

Comment on attachment 164067 [details] [diff] [review]
add javascript consle output

>+      promptService.alert(window,document.title, errorText);
Nit: space after the first comma

>+      var aConsoleService = Components.classes["@mozilla.org/consoleservice;1"].
>+             getService(Components.interfaces.nsIConsoleService);
>+      aConsoleService.logStringMessage(errorText);
I didn't mean you to add this, I was trying (and failing, obviously) to explain
why window.title was wrong. r=me if you remove it again.

Comment 12

[Boying Lu]

Attachment: output to javascript console is removed

Comment 13

[Boying Lu]

Comment on attachment 164083 [details] [diff] [review]
output to javascript console is removed

Can you give r? Thanks

Comment 14

[[e-mail]]

Comment on attachment 164083 [details] [diff] [review]
output to javascript console is removed

>+      promptService.alert(window,document.title,errorText);
Now, I don't need to see another patch for a whitespace change, and I know
we've had enough misunderstandings already, but I meant to say "Add a space
after the first comma", and you took away the space after the second comma :-(
r=me if you put spaces after both the commas.

Comment 15

[Boying Lu]

Comment on attachment 164083 [details] [diff] [review]
output to javascript console is removed

Can you give sr? Thanks

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 164083 [details] [diff] [review]
output to javascript console is removed

>Index: mailnews/import/resources/content/importDialog.js
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/import/resources/content/importDialog.js,v
>retrieving revision 1.38
>diff -u -r1.38 importDialog.js
>--- mailnews/import/resources/content/importDialog.js	30 Sep 2004 14:44:44 -0000	1.38
>+++ mailnews/import/resources/content/importDialog.js	31 Oct 2004 11:21:19 -0000
>@@ -865,6 +865,13 @@
>       return( false);
>     }
> 
>+    if (file.fileSize == 0) {
>+      var errorText = gImportMsgsBundle.getFormattedString('ImportEmptyAddressBook', [path]);
>+      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
>+
>+      promptService.alert(window,document.title,errorText);
>+      return false;
>+    }
>     addInterface.SetData("addressLocation", file);
>   }
> 
>Index: mailnews/import/resources/locale/en-us/importMsgs.properties
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/import/resources/locale/en-us/importMsgs.properties,v
>retrieving revision 1.15
>diff -u -r1.15 importMsgs.properties
>--- mailnews/import/resources/locale/en-us/importMsgs.properties	14 Jun 2004 19:41:26 -0000	1.15
>+++ mailnews/import/resources/locale/en-us/importMsgs.properties	31 Oct 2004 11:21:19 -0000
>@@ -298,6 +298,7 @@
> # Error string for address import
> ImportAddressBadModule=Unable to load address book import module.
> ImportAddressNotFound=Unable to find any address books to import.  Check to make sure the selected application or format is correctly installed on this machine.
>+ImportEmptyAddressBook=Can't import empty address book %S.
> # LOCALIZATION NOTE : Do not translate the word "%S" below.
> ImportAddressFailed=An error occurred importing addresses from %S.
> # LOCALIZATION NOTE : Do not translate the word "%S" below.

Comment 17

[Kyle Yuan]

Checked in with comment 14 addressed.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 164064 [details] [diff] [review]
add javascript console output

Removing obsolete review request on bug that has been fixed for 6 months now.