227210 - Adding entries to a mailing list does not use LDAP directory

Status: RESOLVED WORKSFORME

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

Description

[Scott MacGregor]

If you create a new mailing list and start using type ahead to add entries to
the list, we don't use the LDAP directory if one has been configured for type
ahead here. Apparently 4.x did that.

I guess we would also have to create a new card for each LDAP entry that got
added to the list?

Comment 1

[David :Bienvenu]

taking - yes, we would have to add a card in the main ab if an ldap entry was
accepted in the list.

Comment 2

[David :Bienvenu]

Attachment: proposed fix

this uses the default ldap auto complete server for autocomplete in the mailing
list UI.

Comment 3

[David :Bienvenu]

Comment on attachment 137516 [details] [diff] [review]
proposed fix

One thing I'm not happy about is this pretty much clones the
setupLdapAutocompleteSession() routine from msgComposeCommands.js - if I wanted
to try to share this code, where would I put it? It's slightly different, but I
think the differences could be reconciled.

Comment 4

[Scott MacGregor]

may not be ideal but in tbird we have a file called mailCore.js which contains
JS shared by all 3 components (ab,compose,3-pane).

Comment 5

[Larry Fields]

Has the proposed fix fallen on the trunk?  It's an issue for us here at
Georgetown U., where Mozilla replaced NS Communicator 4.x as our recommended
e-mail client, and users need addresses from LDAP for their mailing lists.

Comment 6

[David :Bienvenu]

No, it has not landed on the trunk. It will land when the tree opens after 1.7a.
For the trunk, I'd like to share the code instead of duping it.

Comment 7

[Larry Fields]

I see this is still in the 'proposed fix' stage, but I thought it would be in
1.7.  Will it make 1.8?

Comment 8

[Wayne Mery (:wsmwk)]

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

Comment 9

[Wayne Mery (:wsmwk)]

larry wrote "Will it make 1.8?" 
:) apparently not

Comment 10

[Larry Fields]

What is up with this bug?  This proposed patch has been in the queue for almost
a year and a half, and my users are complaining about this.  Does Mozilla intend
to penetrate the enterprise space or not?  If so, this is an important feature!

Comment 11

[Larry Fields]

I also forgot to mention that Thunderbird 1.x also exhibits the same behavior.

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to comment #11)
> I also forgot to mention that Thunderbird 1.x also exhibits the same behavior.

TB equivalent is Bug 282040 which has a proposed patch that hasn't yet landed. 
The last bug entry states "... possibly - I'll land it when the tree opens post
1.1a in a week or so."

Comment 13

[Mark Banner (:standard8)]

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

Comment 14

[Mark Banner (:standard8)]

David, is the patch on this bug still valid? The checkin for bug 282040
(thunderbird) is slightly different. We'd like to get the fix for this into the
suite, I'm happy to port the patch from 282040 if you are too busy.

Comment 15

[David :Bienvenu]

Mark, I don't know if it's still valid for the trunk, but either it, or the one
I checked into thunderbird should work. Neil was looking at the patch I checked
in, with an eye towards figuring it out for Seamonkey, including avoiding the
duplicated code, so he might have an idea.

Comment 16

[Mark Banner (:standard8)]

Attachment: Suite version of 282040 patch

This is the suite version of the tb patch from bug 282040. It works on my
build, but I haven't got an LDAP server, so if someone could test it, I'd be
grateful.

Comment 17

[Ian Neal]

Yes it works on a LDAP server too, but it's hard to tell where it is picking the
addresses up from because there is no icon to indicate if each address is from
LDAP or other addressbook.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 137516 [details] [diff] [review]
proposed fix

Cancelling prior review requests.

David - if it's ok with you we'll take the new patch that I posted that is a
variant of the thunderbird one. It should be more consistent that way.

Comment 19

[Mark Banner (:standard8)]

Attachment: Suite patch v2

This revised patch adds some changes to the themes so that we get the icons in
the mailing list autocomplete that indicate where the address comes from. I've
looked through the new autocomplete code and not found any changes that need
making.

Comment 20

[Ian Neal]

Yes tested it works on a LDAP server too and icons now indicate the source of
the address.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 183846 [details] [diff] [review]
Suite patch v2

Requesting review for Mailnews parts (Neil can do the themes part if he does
sr).

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

So this patch just copy-pastes a big chunk of code from the compose-window. 
Instead of copy/pasting, the code from the compose window should be factored out
somewhere common so that it can be shared.

Comment 23

[Karsten Düsterloh]

Comment on attachment 183846 [details] [diff] [review]
Suite patch v2

Please follow dmose's advice in comment #22 (and ask him for review later - I'm
really not of much help here).

Comment 24

[Wayne Mery (:wsmwk)]

(In reply to comment #17)
> Yes it works on a LDAP server too, but it's hard to tell where it is picking the
> addresses up from because there is no icon to indicate if each address is from
> LDAP or other addressbook.

Are Bug 118451 and Bug 191355 related ?

Comment 25

[Ian Neal]

I would say not, but I could be wrong.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #3)
> (From update of attachment 137516 [details] [diff] [review] [edit])
> One thing I'm not happy about is this pretty much clones the
> setupLdapAutocompleteSession() routine from msgComposeCommands.js - if I wanted
> to try to share this code, where would I put it? It's slightly different, but I
> think the differences could be reconciled.
> 

How about in a JS component in mailnews/addrbook/src?  Conceptually, from the
point of view of mail/news, LDAP is just another addressbook, and over the long
haul, it would be nice to try and coalesce the code in that direction.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #26)
> (In reply to comment #3)
> > (From update of attachment 137516 [details] [diff] [review] [edit] [edit])
> > One thing I'm not happy about is this pretty much clones the
> > setupLdapAutocompleteSession() routine from msgComposeCommands.js - if I wanted
> > to try to share this code, where would I put it? It's slightly different, but I
> > think the differences could be reconciled.
> > 
> 

On second thought, mscott's suggestion in comment 4 sounds better.  There's no
real reason to involve extra XPCOM abstraction here.
> How about in a JS component in mailnews/addrbook/src?  Conceptually, from the
> point of view of mail/news, LDAP is just another addressbook, and over the long
> haul, it would be nice to try and coalesce the code in that direction.
> 
> 

Comment 28

[[e-mail]]

(In reply to comment #3)
>(From update of attachment 137516 [details] [diff] [review])
>One thing I'm not happy about is this pretty much clones the
>setupLdapAutocompleteSession() routine from msgComposeCommands.js
>- if I wanted to try to share this code, where would I put it?
I was thinking that it could be put in addressingWidgetOverlay.js

Comment 29

[Mark Banner (:standard8)]

(In reply to comment #28)
> I was thinking that it could be put in addressingWidgetOverlay.js

The suite patch v2 does put the function in addressingWidgetOverlay.js, however 
I *think* Dan was referering to the fact that by copying the function, tb and 
seamonkey will have seperate copies of the same function.

Dan - is that right? or did you think we would end up with two copies of it in 
mailnews or mail?

AFAIK, there's currently not really any sensible js files (relating to address 
books/mail compose areas) that both seamonkey & tb use to share this function 
between, so we would have to create a new js file and move the function from 
the one in mail to mailnews. I don't know if Scott will be happy with that.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

I'm not objecting to the fact that this function is being forked between tb & sm, just like much of the mail 
front-end has.

I'm objecting to the fact that it's already been forked inside tbird (MsgComposeCommands.js & 
abCommon.js) and that this patch proposes to do a similar fork inside seamonkey 
(MsgComposeCommands.js & addressingWidgetOverlay.js).

Comment 31

[Matt Moldvan]

This seems to work for me, but only on the very first entry ... digging through these unassigned bugs to try and find a solution.

Comment 32

[Philip Chee]

Probably fixed by Bug 452232 - Move LDAP autocomplete over to toolkit interfaces