Closed Bug 64305 Opened 20 years ago Closed 15 years ago

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

Categories

(MailNews Core :: Address Book, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: rich.burridge, Assigned: sgautherie)

References

Details

(Keywords: fixed-seamonkey1.0, fixed1.8.1)

Attachments

(10 obsolete files)

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.
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.
I've added two attachments containing fixes needed to resolve this
problem. Could somebody else please test them out. Thanks.
It looks good.  I'll try it when I get my build.
Changing qa assigned
QA Contact: esther → pmock
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.
*** Bug 66112 has been marked as a duplicate of this bug. ***
OS -> All
Platform -> All
OS: Solaris → All
Hardware: Sun → All
Qa-assigned-to myself.
QA Contact: pmock → fenella
QA Contact: fenella → nbaca
Can we get the patches in please?  Pretty minor benign changes, in my opinion.
removing myself from cc:
removing esther from cc
taking all of chuang's bugs.  she doesn't work on mozilla anymore.
Assignee: chuang → sspitzer
Attached patch Preliminary fix (obsolete) — Splinter Review
Rich's previous patches didn't work, as 'Bundle' was undefined.
Attachment #21737 - Attachment is obsolete: true
Attachment #21738 - Attachment is obsolete: true
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.
Attached patch (Cv1) New Patch (obsolete) — Splinter Review
Attachment #134959 - Attachment is obsolete: true
Attachment #135776 - Flags: review?(neil.parkwaycc.co.uk)
[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.
Severity: normal → enhancement
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.
Attachment #135776 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attachment #135776 - Attachment description: New Patch → (Cv1) New Patch
Attachment #135776 - Attachment is obsolete: true
Attached patch (Dv1) simplified patch (obsolete) — Splinter Review
Cv1, with comment 19 suggestion(s),
and with simplified code.
Comment on attachment 141707 [details] [diff] [review]
(Dv1) simplified patch


Should this check be done in |function EditCardOKButton()| too !!?
Attachment #141707 - Flags: review?(neil.parkwaycc.co.uk)
Status: NEW → ASSIGNED
Summary: Adding a New card in the Address book w/o giving any info will add it sucessful → Adding a New card in the Address book w/o giving any info will add it successfully
Assignee: sspitzer → gautheri
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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!
Attachment #141707 - Flags: review?(neil.parkwaycc.co.uk) → review-
Dv1, with comment 22 suggestion(s).
Attachment #141707 - Attachment is obsolete: true
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()| ?
Attachment #143993 - Flags: review?(neil.parkwaycc.co.uk)
(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 ?)
Target Milestone: --- → mozilla1.8alpha
(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)?
Product: Browser → Seamonkey
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.
Attachment #143993 - Flags: review?(neil.parkwaycc.co.uk) → review-
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.
*** Bug 126498 has been marked as a duplicate of this bug. ***
(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 !?
(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.
Attached patch (Ev1) merged ++ (obsolete) — Splinter Review
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 ?
Attachment #143993 - Attachment is obsolete: true
Attachment #189717 - Flags: review?(bugzilla)
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 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.
Attachment #189717 - Flags: review?(bugzilla) → review-
Attached patch (Fv1) enhanced (obsolete) — Splinter Review
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.
Attachment #189717 - Attachment is obsolete: true
Attachment #191155 - Flags: review?(bugzilla)
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.
Attachment #191155 - Flags: review?(bugzilla) → review+
Fv1, with comment 36 suggestion(s),
and a few nits of my own.
Attachment #191155 - Attachment is obsolete: true
Attachment #192629 - Flags: superreview?(mscott)
Attachment #192629 - Flags: review?(bugzilla)
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.
Attachment #192629 - Flags: review?(bugzilla) → review+
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" :-(
Attachment #192629 - Flags: superreview?(mscott) → superreview?(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
Attachment #192629 - Flags: superreview?(bienvenu) → superreview+
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
Attachment #192629 - Attachment description: (Fv1a) enhanced reviewed → (Fv1a) enhanced reviewed (checked in)
Fv1a port (= copy) to ThunderBird,
with an unintentional SM leftover from Fv1a.
Attachment #196845 - Flags: superreview?(bienvenu)
Attachment #196845 - Flags: review?(bugzilla)
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
Target Milestone: mozilla1.8alpha1 → mozilla1.9alpha
Comment on attachment 196845 [details] [diff] [review]
(Gv1) port to ThunderBird
[Checked in: Comment 44]

r=me
Attachment #196845 - Flags: review?(bugzilla) → review+
Attachment #196845 - Flags: superreview?(bienvenu) → superreview+
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
Attachment #196845 - Attachment description: (Gv1) port to ThunderBird → (Gv1) port to ThunderBird (checked in)
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

a=me for SM1.0b, one more needed
If a patch from this bug goes into the branch, the the one for bug 314995 should also go into the branch...
Comment on attachment 192629 [details] [diff] [review]
(Fv1a) enhanced reviewed
[Checked in: Comment 41]

a=me for SeaMonkey
SeaMonkey only parts of the patches checked into the 1.8 branch in time for SeaMonkey 1.0b.
Whiteboard: fixed-seamonkey1.0
(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 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.
Attachment #196845 - Flags: approval1.8.1?
Adding fixed1.8.0.1 per comments. Sounds like there may need to be another checkin before adding fixed1.8.1 as well
Keywords: fixed1.8.0.1
Attachment #196845 - Flags: approval1.8.1? → branch-1.8.1?(mscott)
Attachment #196845 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
Gv1 port checked into branch.
Keywords: fixed1.8.1
Whiteboard: fixed-seamonkey1.0
Attachment #196845 - Attachment description: (Gv1) port to ThunderBird (checked in) → (Gv1) port to ThunderBird [Checked in: Comment 44]
Attachment #196845 - Attachment is obsolete: true
Attachment #192629 - Attachment description: (Fv1a) enhanced reviewed (checked in) → (Fv1a) enhanced reviewed [Checked in: Comment 41]
Attachment #192629 - Attachment is obsolete: true
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.