Last Comment Bug 64305 - Adding a New card in the Address book w/o giving any info will add it successfully
: Adding a New card in the Address book w/o giving any info will add it success...
Status: VERIFIED FIXED
: fixed-seamonkey1.0, fixed1.8.1
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9alpha1
Assigned To: Serge Gautherie (:sgautherie)
: Ninoschka Baca
Mentors:
: 66112 126498 (view as bug list)
Depends on: 307056
Blocks: 126491 314995
  Show dependency treegraph
 
Reported: 2001-01-04 09:13 PST by Rich Burridge
Modified: 2010-02-28 10:06 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Changes to abCardOverlay.js to fix this bug. (951 bytes, patch)
2001-01-04 09:17 PST, Rich Burridge
no flags Details | Diff | Splinter Review
Changes to addressBook.properties to fix this bug. (567 bytes, patch)
2001-01-04 09:18 PST, Rich Burridge
no flags Details | Diff | Splinter Review
Preliminary fix (1.94 KB, patch)
2003-11-06 19:53 PST, Stephen Donner [:stephend]
no flags Details | Diff | Splinter Review
(Cv1) New Patch (3.17 KB, patch)
2003-11-17 18:35 PST, Stephen Donner [:stephend]
neil: review-
Details | Diff | Splinter Review
(Dv1) simplified patch (2.67 KB, patch)
2004-02-18 16:25 PST, Serge Gautherie (:sgautherie)
neil: review-
Details | Diff | Splinter Review
(Dv2) simplified & extended patch (3.18 KB, patch)
2004-03-15 16:57 PST, Serge Gautherie (:sgautherie)
standard8: review-
Details | Diff | Splinter Review
(Ev1) merged ++ (18.80 KB, patch)
2005-07-18 16:34 PDT, Serge Gautherie (:sgautherie)
standard8: review-
Details | Diff | Splinter Review
(Fv1) enhanced (20.19 KB, patch)
2005-07-31 18:13 PDT, Serge Gautherie (:sgautherie)
standard8: review+
Details | Diff | Splinter Review
(Fv1a) enhanced reviewed [Checked in: Comment 41] (20.35 KB, patch)
2005-08-13 14:56 PDT, Serge Gautherie (:sgautherie)
standard8: review+
mozilla: superreview+
Details | Diff | Splinter Review
(Gv1) port to ThunderBird [Checked in: Comment 44] (21.54 KB, patch)
2005-09-20 15:37 PDT, Serge Gautherie (:sgautherie)
standard8: review+
mozilla: superreview+
mscott: approval‑branch‑1.8.1+
Details | Diff | Splinter Review

Description Rich Burridge 2001-01-04 09:13:45 PST
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 2001-01-04 09:15:36 PST
Here are some comments from Denis Y. Antrushin <adu@sparc.spb.su>

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 2001-01-04 09:17:19 PST
Created attachment 21737 [details] [diff] [review]
Changes to abCardOverlay.js to fix this bug.
Comment 3 Rich Burridge 2001-01-04 09:18:07 PST
Created attachment 21738 [details] [diff] [review]
Changes to addressBook.properties to fix this bug.
Comment 4 Rich Burridge 2001-01-04 09:19:19 PST
I've added two attachments containing fixes needed to resolve this
problem. Could somebody else please test them out. Thanks.
Comment 5 chuang 2001-01-04 10:47:00 PST
It looks good.  I'll try it when I get my build.
Comment 6 pmock 2001-01-04 15:23:35 PST
Changing qa assigned
Comment 7 vbreddy 2001-01-21 08:31:02 PST
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 2001-01-21 10:30:03 PST
*** Bug 66112 has been marked as a duplicate of this bug. ***
Comment 9 Håkan Waara 2001-01-21 10:32:41 PST
OS -> All
Platform -> All
Comment 10 fenella 2001-02-20 13:33:55 PST
Qa-assigned-to myself.
Comment 11 Margaret Chan 2002-02-08 13:49:06 PST
Can we get the patches in please?  Pretty minor benign changes, in my opinion.
Comment 12 Denis Antrushin 2002-04-25 01:57:34 PDT
removing myself from cc:
Comment 13 esther 2002-04-25 08:16:56 PDT
removing esther from cc
Comment 14 (not reading, please use seth@sspitzer.org instead) 2002-09-07 17:17:03 PDT
taking all of chuang's bugs.  she doesn't work on mozilla anymore.
Comment 15 Stephen Donner [:stephend] 2003-11-06 19:53:48 PST
Created attachment 134959 [details] [diff] [review]
Preliminary fix

Rich's previous patches didn't work, as 'Bundle' was undefined.
Comment 16 Stephen Donner [:stephend] 2003-11-06 21:51:50 PST
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] 2003-11-17 18:35:27 PST
Created attachment 135776 [details] [diff] [review]
(Cv1) New Patch
Comment 18 Serge Gautherie (:sgautherie) 2004-02-04 01:41:18 PST
[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 neil@parkwaycc.co.uk 2004-02-06 14:39:03 PST
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) 2004-02-18 16:25:58 PST
Created attachment 141707 [details] [diff] [review]
(Dv1) simplified patch

Cv1, with comment 19 suggestion(s),
and with simplified code.
Comment 21 Serge Gautherie (:sgautherie) 2004-02-18 16:26:45 PST
Comment on attachment 141707 [details] [diff] [review]
(Dv1) simplified patch


Should this check be done in |function EditCardOKButton()| too !!?
Comment 22 neil@parkwaycc.co.uk 2004-03-14 15:51:14 PST
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) 2004-03-15 16:57:54 PST
Created attachment 143993 [details] [diff] [review]
(Dv2) simplified & extended patch

Dv1, with comment 22 suggestion(s).
Comment 24 Serge Gautherie (:sgautherie) 2004-03-15 17:03:02 PST
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) 2004-03-15 17:09:17 PST
(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 neil@parkwaycc.co.uk 2004-03-17 03:27:25 PST
(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) (afk until 26th July) 2005-07-13 12:55:19 PDT
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) (afk until 26th July) 2005-07-13 13:04:41 PDT
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) (afk until 26th July) 2005-07-13 13:06:18 PDT
*** Bug 126498 has been marked as a duplicate of this bug. ***
Comment 30 Serge Gautherie (:sgautherie) 2005-07-14 06:00:07 PDT
(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) (afk until 26th July) 2005-07-14 10:39:19 PDT
(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) 2005-07-18 16:34:15 PDT
Created attachment 189717 [details] [diff] [review]
(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) (afk until 26th July) 2005-07-23 02:45:54 PDT
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) (afk until 26th July) 2005-07-24 12:01:19 PDT
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) 2005-07-31 18:13:41 PDT
Created attachment 191155 [details] [diff] [review]
(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) (afk until 26th July) 2005-08-01 11:56:01 PDT
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) 2005-08-13 14:56:59 PDT
Created attachment 192629 [details] [diff] [review]
(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) (afk until 26th July) 2005-08-20 04:44:51 PDT
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) 2005-09-02 05:33:37 PDT
Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]


No super-review from <mscott@mozilla.org> since "2005-08-13" :-(
Comment 40 David :Bienvenu 2005-09-02 08:06:20 PDT
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) (afk until 26th July) 2005-09-19 12:57:05 PDT
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) 2005-09-20 15:37:37 PDT
Created attachment 196845 [details] [diff] [review]
(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) (afk until 26th July) 2005-09-25 05:09:15 PDT
Comment on attachment 196845 [details] [diff] [review]
(Gv1) port to ThunderBird
[Checked in: Comment 44]

r=me
Comment 44 Mark Banner (:standard8) (afk until 26th July) 2005-10-11 09:25:58 PDT
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] 2005-10-18 22:00:05 PDT
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 2005-12-14 08:51:51 PST
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) (afk until 26th July) 2005-12-14 10:05:46 PST
If a patch from this bug goes into the branch, the the one for bug 314995 should also go into the branch...
Comment 48 neil@parkwaycc.co.uk 2005-12-14 11:57:04 PST
Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

a=me for SeaMonkey
Comment 49 Mark Banner (:standard8) (afk until 26th July) 2005-12-14 14:03:08 PST
SeaMonkey only parts of the patches checked into the 1.8 branch in time for SeaMonkey 1.0b.
Comment 50 Mark Banner (:standard8) (afk until 26th July) 2005-12-22 10:25:59 PST
(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) (afk until 26th July) 2005-12-30 14:26:09 PST
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] 2006-01-20 06:47:48 PST
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) (afk until 26th July) 2006-03-24 07:02:51 PST
Gv1 port checked into branch.

Note You need to log in before you can comment on or make changes to this bug.