The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9alpha1

Status

MailNews Core
Address Book
--
enhancement
VERIFIED FIXED
17 years ago
7 years ago

People

(Reporter: Rich Burridge, Assigned: sgautherie)

Tracking

({fixed-seamonkey1.0, fixed1.8.1})

Trunk
mozilla1.9alpha1
fixed-seamonkey1.0, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
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.
(Reporter)

Comment 2

17 years ago
Created attachment 21737 [details] [diff] [review]
Changes to abCardOverlay.js to fix this bug.
(Reporter)

Comment 3

17 years ago
Created attachment 21738 [details] [diff] [review]
Changes to addressBook.properties to fix this bug.
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

17 years ago
Changing qa assigned
QA Contact: esther → pmock

Comment 7

16 years ago
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

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

Comment 9

16 years ago
OS -> All
Platform -> All
OS: Solaris → All
Hardware: Sun → All

Comment 10

16 years ago
Qa-assigned-to myself.
QA Contact: pmock → fenella

Updated

16 years ago
QA Contact: fenella → nbaca

Comment 11

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

Comment 12

15 years ago
removing myself from cc:

Comment 13

15 years ago
removing esther from cc
taking all of chuang's bugs.  she doesn't work on mozilla anymore.
Assignee: chuang → sspitzer
Created attachment 134959 [details] [diff] [review]
Preliminary fix

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.
Created attachment 135776 [details] [diff] [review]
(Cv1) New Patch
Attachment #134959 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #135776 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 18

13 years ago
[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 19

13 years ago
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-
(Assignee)

Updated

13 years ago
Attachment #135776 - Attachment description: New Patch → (Cv1) New Patch
Attachment #135776 - Attachment is obsolete: true
(Assignee)

Comment 20

13 years ago
Created attachment 141707 [details] [diff] [review]
(Dv1) simplified patch

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

Comment 21

13 years ago
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)
(Assignee)

Updated

13 years ago
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)

Updated

13 years ago
Assignee: sspitzer → gautheri
Status: ASSIGNED → NEW
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED

Comment 22

13 years ago
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-
(Assignee)

Comment 23

13 years ago
Created attachment 143993 [details] [diff] [review]
(Dv2) simplified & extended patch

Dv1, with comment 22 suggestion(s).
Attachment #141707 - Attachment is obsolete: true
(Assignee)

Comment 24

13 years ago
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)
(Assignee)

Comment 25

13 years ago
(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

Comment 26

13 years ago
(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. ***
Blocks: 126491
(Assignee)

Comment 30

12 years ago
(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.
(Assignee)

Comment 32

12 years ago
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 ?
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-
(Assignee)

Comment 35

12 years ago
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.
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 37

12 years ago
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.
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+
(Assignee)

Comment 39

12 years ago
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 40

12 years ago
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+
Depends on: 307056
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)
(Assignee)

Comment 42

12 years ago
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.
Attachment #196845 - Flags: superreview?(bienvenu)
Attachment #196845 - Flags: review?(bugzilla)
(Assignee)

Updated

12 years ago
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+

Updated

12 years ago
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)
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 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 46

11 years ago
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 48

11 years ago
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.
Blocks: 314995
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
Keywords: fixed1.8.0.1
Attachment #196845 - Flags: approval1.8.1? → branch-1.8.1?(mscott)

Updated

11 years ago
Attachment #196845 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
Gv1 port checked into branch.
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
(Assignee)

Updated

11 years ago
Attachment #196845 - Attachment description: (Gv1) port to ThunderBird (checked in) → (Gv1) port to ThunderBird [Checked in: Comment 44]
Attachment #196845 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
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.