1574590 - Selected LDAP directory server preference becomes de-selected

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect, P2)

Description

[Mabry Tyson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

In TB 60.8.0, 68.0, or 69.0
go to Preferences > Composition > Addressing.
If you don't already have a directory server, create one and make sure it is selected (both checked & selected) (It probably can be a fake one as this doesn't require actual LDAP service)
Now click on Edit Directories. This brings up a window to edit properties. Make sure the window isn't blocking your view of the Preferences window that shows the selected directory server.
With or without making any changes, click OK in the edit properties pop-up.

Actual results:

When I click OK, the selected directory server is deselected and no directory server is selected.

Expected results:

The selected directory server shouldn't get deselected. If it must be (for reasons I don't understand), then please alert the user he should reselect it. Otherwise the user will likely not notice this happened and will get confused when his LDAP directory isn't working.

I'm going to call this a defect but it is a minor one. It is an annoyance and not what I expect you want.

Comment 1

[Jorg K (CEST = GMT+2)]

Confirmed. Should be easy to fix. Aceman, can you take a look.

Comment 2

[Jorg K (CEST = GMT+2)]

Actually, I can't see the menulist being lost in TB 60.

A bit of research: The pref shown in the field is this:
https://searchfox.org/comm-central/search?q=ldap_2.autoComplete.directoryServer&case=false&regexp=false&path=

"Edit directories" runs gComposePane.editDirectories(). Code here:
https://searchfox.org/comm-central/source/mailnews/addrbook/prefs/content

I think this is a de-XBL issue in
https://searchfox.org/comm-central/source/mail/components/addrbook/content/menulist-addrbooks.js

I'll blame it on the original author ;-)

Comment 3

[Jorg K (CEST = GMT+2)]

Certainly not happening in TB 60. There, if you edit a server name and that server is selected, the display changes accordingly instead of being deselected.

EDIT: And BTW, deselected/empty is not a valid value. If anything, it should be None. So I'm wondering whether it's just a display glitch.

Comment 4

[Alessandro Castellani [:aleca]]

Confirmed, it happens on 68 and 69.
Most likely my fault, I'll deal with it.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1574590-ldap-directory-regression.patch

This should fix also bug 1574712.

So, during the de-xbl bug, I overlooked the fact that the menulist value would change and not return anymore "URI" or "dirPrefId", which are the 2 directory types we need in order to properly build the list with names and correct values.

Take a look at what I did and if it makes sense as a solution.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086485 [details] [diff] [review]
1574590-ldap-directory-regression.patch

Review of attachment 9086485 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me and looks reasonable. I've kept out of de-XBL and since we're down to two bindings, I was quite successful at that, so I'm not getting involved now ;-) - I hope Magnus can give his seal of approval soon, so we can forget this issue.

::: mail/components/addrbook/content/menulist-addrbooks.js
@@ +87,5 @@
>        }, { once: true });
>      }
>  
> +    get _value() {
> +      return this.getAttribute("value") || "URI";

Why the || "URI"?

Comment 7

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=93bf219a8ac27c4bfcbb550d3be100181a9bed8b

Comment 8

[Alessandro Castellani [:aleca]]

(In reply to Jorg K (GMT+2) from comment #6)

get _value() {
  return this.getAttribute("value") || "URI";

Why the || "URI"?

Argh, that's a leftover.
Maybe at this point the entire getter method can be removed.
I'll fix it.

Comment 9

[Jorg K (CEST = GMT+2)]

Perfect green try, even bct passed.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1574590-ldap-directory-regression.patch

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086531 [details] [diff] [review]
1574590-ldap-directory-regression.patch

Review of attachment 9086531 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/menulist-addrbooks.js
@@ +87,5 @@
>        }, { once: true });
>      }
>  
> +    get _type() {
> +      return this.getAttribute("type") || "URI";

We talked about this on IRC. So URI is for local lists which don't have that attribute, to distinguish it from LDAP. Maybe add a comment, or use a different string? Or give them a type? I guess there's code that compares with "URI" somewhere outside this patch, so this is minimal change?

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9086531 [details] [diff] [review]
1574590-ldap-directory-regression.patch

Review of attachment 9086531 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/menulist-addrbooks.js
@@ +152,5 @@
>            // Skip the empty members added above.
>            continue;
>          }
>  
> +        let listItem = this.appendItem(ab.dirName, ab[type]);

While the type approach is better than it was before (which was very confusing), I think we can just make it even more explicit by remoteonly ? ab.dirPrefId : ab.URI

There is already remoteonly="true" which seems to be used for the same thing, so it would be preferable to make it conditional on that. This widget has too many options already. Maybe you can add documentation for the attributes one can set? 

CE:s should also really react to changes of any such attributes but we don't do that atm.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1574590-ldap-directory-regression.patch

Patch updated to use the remoteonly attribute instead of adding a new one.

I decided to leave the _type() getter as it makes things easier, and added a comment there.
I didn't use the built-in match() method because that's strictly used to add/remove items from the list based on the remoteonly attribute.

This is a bit messed up because address lists are not used consistently across the platform. Example:

In the Address Book or Messenger Compose window
The full list is showed with always the local "URI" value, even for LDAP directories

• moz-abmdbdirectory://abook.mab
• moz-abldapdirectory://ldap_2.servers.Adams
• etc...

In the Preferences > Composition
Only the LDAP directories are showed, using the "dirPrefId" value

• ldap_2.servers.Test
• ldap_2.servers.Adams
• etc...

So, even if the Address Book Object itself has an attribute called isRemote, that doesn't mean we can use it to always return the "dirPrefId" value, because sometimes we need the "URI" even if it's a remote LDAP directory.

I hope what I wrote makes sense.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9086729 [details] [diff] [review]
1574590-ldap-directory-regression.patch

Review of attachment 9086729 [details] [diff] [review]:
-----------------------------------------------------------------

I suppose this works though I still think the _type()  getter is superflous

Comment 15

[Jorg K (CEST = GMT+2)]

20:25:02 - jorgk: aleca: Another tweak to the LDAP thing?
20:26:28 - aleca: jorgk, we can think about that in a follow up bug

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/eb47af85ed8e
Change MozMenulistAddrbooks CE to fix LDAP auto-complete and LDAP server pref. r=mkmelin

Comment 17

[Jorg K (CEST = GMT+2)]

TB 69 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/1e92fa2aab3f6481a1e2f28a4cce11370328a2a6

Comment 18

[Jorg K (CEST = GMT+2)]

TB 68.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/779ea47d7c351fbd7255260a66fb60105d56ebf3