64305 - Adding a New card in the Address book w/o giving any info will add it successfully

Status: VERIFIED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[Rich Burridge]

Tried to add a New Card on the Address Book without giving any information.

Steps..

1. launch the Address book (Task --> Address Book)
2. click on New Card
3. a new card window will come up, try not to give any details and hit on the OK
button.

it will not give you any warning message that you need to type something and it
will successfully add it in the address book.

Comment 1

[Rich Burridge]

Here are some comments from Denis Y. Antrushin <[e-mail]>

Here is fix for this bug.

It's very simple, but please note two things:

1. I decide whether new card is empty or not based solely on
'Primary Email' field. It's very easy to check other fields
as well, but who should decide what fields to check?

2. I use standard javascript's method window.confirm() instead
of writing yet another XUL, JS and DTD files for simple
confirmation dialog. I don't know is it bad or not, but
it's simple and it works (it changes look'n'feel under
different themes). And it can be l10n'ed through addressBook.properties
string bundle.

Comment 2

[Rich Burridge]

Attachment: Changes to abCardOverlay.js to fix this bug.

Comment 3

[Rich Burridge]

Attachment: Changes to addressBook.properties to fix this bug.

Comment 4

[Rich Burridge]

I've added two attachments containing fixes needed to resolve this
problem. Could somebody else please test them out. Thanks.

Comment 5

[chuang]

It looks good.  I'll try it when I get my build.

Comment 6

[pmock]

Changing qa assigned

Comment 7

[vbreddy]

Yeah I observed same behaviour as per #64305 on windows with 2001012008 build. 
It is good idea not to accept creating an address card without providing any 
information. few fileds can be made mandatory such as as email and names.

PS: While trying to choose recipents from address book without a value for Email 
field it shows an alert., This check can be done at the earlier stages i.e while 
creating address card itself.

Comment 8

[Håkan Waara]

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

Comment 9

[Håkan Waara]

OS -> All
Platform -> All

Comment 10

[fenella]

Qa-assigned-to myself.

Comment 11

[Margaret Chan]

Can we get the patches in please?  Pretty minor benign changes, in my opinion.

Comment 12

[Denis Antrushin]

removing myself from cc:

Comment 13

[esther]

removing esther from cc

Comment 14

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

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

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Preliminary fix

Rich's previous patches didn't work, as 'Bundle' was undefined.

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

I think we should check for at least:

First, Last and Email fields...

Also, I'll rework the text and the extra whitespace has already been removed.

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: (Cv1) New Patch

Comment 18

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6) Gecko/20040113] (W98SE)

Confirmed.

[Netscape® Communicator 4.8 : en-20020722] (W98SE)

Same behaviour.


Updating:
*(S) Normal -> Enhancement.

Comment 19

[[e-mail]]

Comment on attachment 135776 [details] [diff] [review]
(Cv1) New Patch

>+      var alert = promptService.alert(window, null, gAddressBookBundle.getString("newCardIsEmpty"));
>+      if ( !alert )
>+        return false;       
alert returns void.

>+newCardIsEmpty=Please enter either a first name, last name, or an email address.
Except you're actually checking for a display name or an email address.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) simplified patch

Cv1, with comment 19 suggestion(s),
and with simplified code.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 141707 [details] [diff] [review]
(Dv1) simplified patch


Should this check be done in |function EditCardOKButton()| too !!?

Comment 22

[[e-mail]]

Comment on attachment 141707 [details] [diff] [review]
(Dv1) simplified patch

>+        promptService.alert(window, null,
>+                        gAddressBookBundle.getString("newCardNeedsMoreData"));
Sorry for not spotting this before, but I think we shouldn't use the default
title for this alert.

Also, I'm glad you pointed out the edit card OK function, for consistency we
should do the same checks there. I think that collected addresses will always
have a display name, it wouldn't do to be unable to edit a collected address!

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Dv2) simplified & extended patch

Dv1, with comment 22 suggestion(s).

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 143993 [details] [diff] [review]
(Dv2) simplified & extended patch


3rd (of 3) location:
{{
function NewCardOKButton()
{
  if (gOkCallback)
  {
    SetCardValues(editCard.card, document);
    var addressbook =
Components.classes["@mozilla.org/addressbook;1"].createInstance(Components.inte
rfaces.nsIAddressBook);
    gOkCallback(addressbook.abCardToEscapedVCard(editCard.card));
    return true;  // close the window
  }
}}
Is it correct not to check there ?
Or should I move the (3) check into |SetCardValues()| ?

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> I think that collected addresses will always
> have a display name, it wouldn't do to be unable to edit a collected address!

(They are more likely (= guaranted !?) to have a PrimaryAddress than a
DisplayName, aren't they ?)

Comment 26

[[e-mail]]

(In reply to comment #25)
>(In reply to comment #22)
>>I think that collected addresses will always
>>have a display name, it wouldn't do to be unable to edit a collected address!
>(They are more likely (= guaranted !?) to have a PrimaryAddress than a
>DisplayName, aren't they ?)
Somehow I forgot the word "don't" :-[

I'm not a big fan of alerts, perhaps we could just disable the ok button if the
primary address doesn't have an @ in it (or something like that)?

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 143993 [details] [diff] [review]
(Dv2) simplified & extended patch

The general patch looks ok to me, however I've just been discussing it on irc,
and I believe we'd prefer to ensure that just a primary email address is
supplied, we're not worried about the display name at the moment.

The main reason for this being that mailing lists really require a card to have
an email address.

Comment 28

[Mark Banner (:standard8)]

Serge, any chance you could also incorporate the tidy up suggested in the patch
(https://bugzilla.mozilla.org/attachment.cgi?id=139980) on bug 126498 into your
one? If you prefer not to, let me know and I'll raise a seperate bug.

Comment 29

[Mark Banner (:standard8)]

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

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> Serge, any chance you could also incorporate the tidy up suggested in the patch
> (https://bugzilla.mozilla.org/attachment.cgi?id=139980) on bug 126498 into your
> one?

I can;
You mean using array(s) and for loop(s) coding style, don't you !?

Comment 31

[Mark Banner (:standard8)]

(In reply to comment #30)
> I can;
> You mean using array(s) and for loop(s) coding style, don't you !?

Yes, David and I think it's neater that way. Also, just to note please use the:
for
{
}
style of brackets as opposed to the version that is in that patch.

Thanks.

Comment 32

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) merged ++

Dv2, with comment 28 suggestion(s),
and a few random additional "cleanups".

I tested it very lightly: further testing welcomed as needed.

Comment 24: remains to be answered.

(In reply to comment #26)
> I'm not a big fan of alerts, perhaps we could just disable the ok button if
the
> primary address doesn't have an @ in it (or something like that)?

I understand; yet we might want to keep the alert, unless we could add
something else to tell the user what we expect.
I guess we could check for a 'a@b.c' template to ensure the address is there
and (hopefully) valid ! Is there such a check-function already
available/callable ?

Comment 33

[Mark Banner (:standard8)]

Sorry for the delay in this. Here are some answers to your questions, I'll try
and review the patch also within the next day so that any comments can be
incorporated with the changes requested below.

(In reply to comment #32)
> Comment 24: remains to be answered.
I think it's ok to leave the checks where they are - it's easy to understand
that way.
 
> (In reply to comment #26)
> > I'm not a big fan of alerts, perhaps we could just disable the ok button if
> the
> > primary address doesn't have an @ in it (or something like that)?
> 
> I understand; yet we might want to keep the alert, unless we could add
> something else to tell the user what we expect.
> I guess we could check for a 'a@b.c' template to ensure the address is there
> and (hopefully) valid ! Is there such a check-function already
> available/callable ?
I've had a chat to Neil over IRC and we think that it's best to keep the alert
in this case, but can we also focus the primary email address field afterwards
as well to make it really clear. 

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 189717 [details] [diff] [review]
(Ev1) merged ++

+cardWantedDataMissingMessage=Please enter at least a primary Email address.
+cardWantedDataMissingTitle=Wanted data missing

The capitalisation needs to be consistent on the message. Also, I'd prefer
something like "Data Missing" or "Required Data Missing" for the dialog title.

+  var i;
+  for (i = directory.addressLists.Count(); i-- > 0; ) {
...

Please change to:
var i = directory.addressLists.Count();
while (i--) {...}
return i;

It's slightly simpler and easier to understand.

   // hide phonetic fields if indicated by the pref
   if (showPhoneticFields == "true")
-  {
-    var element = document.getElementById("PhoneticLastName");
-    element.setAttribute("hidden", "false");
-    element = document.getElementById("PhoneticLabel1");
-    element.setAttribute("hidden", "false");
-    element = document.getElementById("PhoneticSpacer1");
-    element.setAttribute("hidden", "false");
-
-    element = document.getElementById("PhoneticFirstName");
-    element.setAttribute("hidden", "false");
-    element = document.getElementById("PhoneticLabel2");
-    element.setAttribute("hidden", "false");
-    element = document.getElementById("PhoneticSpacer2");
-    element.setAttribute("hidden", "false");
-  }
+    for (var i = kPhoneticFields.length; i-- > 0; )
+      document.getElementById(kPhoneticFields[i]).hidden = false;
 }

Please keep the braces for the if statement (around the for) I think it's
clearer that way.

For a email check function, here are a couple:
http://lxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/a
m-identity-edit.js#156
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/resources/content/MsgC
omposeCommands.js#1869

Though I don't think you can re-use them as they are.

Comment 35

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) enhanced

Ev1, with comment 26 + comment 33 + comment 34 suggestion(s),
and checking required data on all 3 places per Mark's email answer,
and diffed to current Trunk.

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 191155 [details] [diff] [review]
(Fv1) enhanced

+  // Simple checks that the primary email could be in the form of |x@y.z|.
+  var primaryEmail = doc.getElementById("PrimaryEmail");
+  var primaryEmailValue = primaryEmail.value;
+  if (!((primaryEmailValue.length >= 5) &&
+	 (primaryEmailValue.lastIndexOf("@") >= 1) &&
+	 (primaryEmailValue.lastIndexOf(".") >= 3)))

I think this could break some local email systems, I'd prefer to just check for
|x@y|.

+    // Focus the dialog field, to help the user.
+    primaryEmail.focus();

We need to also focus the contact tab here, adding :
document.getElementById("abTabs").selectedIndex = 0;

before the primaryEmail.focus() will do it.

+cardRequiredDataMissingMessage=Please enter at least a primary email address.

Now I've looked at this again, I think we need some sort of indication of the
format here. How about:

"Please enter at least a primary email address of the form user@host".

r=me with those fixed.

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1a) enhanced reviewed [Checked in: Comment 41]

Fv1, with comment 36 suggestion(s),
and a few nits of my own.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

+    // Focus the dialog field, to help the user.
+    document.getElementById("abTabs").selectedIndex = 0;
+    primaryEmail.focus();
+
+    Components
+      .classes["@mozilla.org/embedcomp/prompt-service;1"]
+      .getService(Components.interfaces.nsIPromptService)
+      .alert(
+	 window,
+	 gAddressBookBundle.getString("cardRequiredDataMissingTitle"),
+	 gAddressBookBundle.getString("cardRequiredDataMissingMessage"));


Put the two focus lines after the alert and it'll work properly on linux at
least.

r=me with that changed.

Comment 39

[Serge Gautherie (:sgautherie)]

Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]


No super-review from <[e-mail]> since "2005-08-13" :-(

Comment 40

[David :Bienvenu]

Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

sr=bienvenu, but I think these messages and titles need a little work.

+cardRequiredDataMissingMessage=Please enter at least a primary email address
of the form user@host.
+cardRequiredDataMissingTitle=Required Data Missing

I'd suggest:

You must enter a primary e-mail address of the form user@host.

and Required Information Missing or Required Field Information

Comment 41

[Mark Banner (:standard8)]

Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

Checked in with review comments addressed.

/cvsroot/mozilla/mailnews/addrbook/resources/content/abCardOverlay.js,v  <-- 
abCardOverlay.js
new revision: 1.57; previous revision: 1.56
/cvsroot/mozilla/mailnews/addrbook/resources/locale/en-US/addressBook.propertie
s,v  <--  addressBook.properties
new revision: 1.34; previous revision: 1.33

Comment 42

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1) port to ThunderBird [Checked in: Comment 44]

Fv1a port (= copy) to ThunderBird,
with an unintentional SM leftover from Fv1a.

Comment 43

[Mark Banner (:standard8)]

Comment on attachment 196845 [details] [diff] [review]
(Gv1) port to ThunderBird
[Checked in: Comment 44]

r=me

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 196845 [details] [diff] [review]
(Gv1) port to ThunderBird
[Checked in: Comment 44]

This patch is now checked in:
/cvsroot/mozilla/mail/components/addrbook/content/abCardOverlay.js,v  <-- 
abCardOverlay.js
new revision: 1.10; previous revision: 1.9
done
Checking in
mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/addressbook/addressBook.pr
operties,v  <--  addressBook.properties
new revision: 1.7; previous revision: 1.6
done
Checking in mailnews/addrbook/resources/content/abCardOverlay.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCardOverlay.js,v  <-- 
abCardOverlay.js
new revision: 1.58; previous revision: 1.57

Comment 45

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2005-10-13-16 of SeaMonkey trunk and version 1.6a1
(20051018) of Thunderbird on Windows XP.

The alert dialog shows correctly and then on OK it focuses the email textfield.

Comment 46

[Ian Neal]

Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

a=me for SM1.0b, one more needed

Comment 47

[Mark Banner (:standard8)]

If a patch from this bug goes into the branch, the the one for bug 314995 should also go into the branch...

Comment 48

[[e-mail]]

Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

a=me for SeaMonkey

Comment 49

[Mark Banner (:standard8)]

SeaMonkey only parts of the patches checked into the 1.8 branch in time for SeaMonkey 1.0b.

Comment 50

[Mark Banner (:standard8)]

(In reply to comment #49)
> SeaMonkey only parts of the patches checked into the 1.8 branch in time for
> SeaMonkey 1.0b.
> 
and now the 1.8.0 branch as well.

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 196845 [details] [diff] [review]
(Gv1) port to ThunderBird
[Checked in: Comment 44]

Requesting approval for 1.8.1 branch (Thunderbird-only parts, SeaMonkey parts already in). This patch improves  that back-end code for the address book card dialog, and allows specification of a minimum entry requirement for address book cards. Bug 314995 is also required if this patch is approved for branch, as that provides a better minimum entry requirment. This patch also needs bug 307056 to be accepted for branch. We'll want this patch for trunk synchronisation for birthday & anniversary fields if we implement them for 2.0.

Comment 52

[Daniel Veditz [:dveditz]]

Adding fixed1.8.0.1 per comments. Sounds like there may need to be another checkin before adding fixed1.8.1 as well

Comment 53

[Mark Banner (:standard8)]

Gv1 port checked into branch.