Ldap agent credentials save

[blizzz]

Fixes#4476

Some notes, first about the GUI. @nextcloud/designers !

The Save Credentials button is new, now, and always enabled (since you can allow anonymous access to LDAP). Imho it is not clear the this button must be pressed on changes to the DN or password. Also, it feels to dense. Possible enhancements:

• have a border around the DN, Password and Save button
• disable button if dn+pw were not changed, but pretty useless with browser auto-complete. Nevertheless migth still be a good idea.

What do you think? If you have other suggestions, appreciated.

Second, some technical notes:

• This also fixes a minor (i believe downstreamed) bug
• When saving LDAP config only actualy changes are written (previously everything)
• The wizard and endpoints only support saving one change a time. Which is annoying in this case, but works so far. Fixing this, requires some more changes that are out of bounds for this PR (cf. TODO in wizardTabGeneric.js)

@nextcloud/ldap

[enoch85]

Nice fix! This was somewhat confusing before. 👍

[jancborchardt]

Two pieces of feedback:

have a border around the DN, Password and Save button

I don’t know what DN is (let’s avoid technical jargon), but can we put the DN field and password field next to each other at half width, and put the save button next to it? Then it seems more like a unit.

Also, if the credentials were changed, we could give the button a class="primary" – only when changed! Additionally the Continue button should get that class too.

[blizzz]

I don’t know what DN is (let’s avoid technical jargon), but can we put the DN field and password field next to each other at half width, and put the save button next to it? Then it seems more like a unit.

The "Distinguished Name" (sort of user identifier) can be quite long. Today I saw one that already does not fit into this size. Can be done, but perhaps not that user friendly.

Also, if the credentials were changed, we could give the button a class="primary" – only when changed! Additionally the Continue button should get that class too.

That class is news to me, what does it do? Certainly possible.

[jancborchardt]

That class is news to me, what does it do?

Makes the button blue (or the color of theming). Used on primary buttons like the log in button, or »Choose file« in the file picker, Reply/Send in the Mail app etc.

[blizzz]

Makes the button blue (or the color of theming). Used on primary buttons like the log in button, or »Choose file« in the file picker, Reply/Send in the Mail app etc.

Uh, this requires additional logic for enter buttons presses, etc.? → out of scope for this PR

[codecov]

Codecov Report

Merging #5568 into master will increase coverage by <.01%.
The diff coverage is 11.11%.

@@ Coverage Diff @@## master #5568 +/- ##
============================================
+ Coverage 52.82% 52.83% +<.01% 
Complexity 22802 22802 ============================================
Files 1442 1442 Lines 88562 88567 +5 Branches 1349 1349 ============================================
+ Hits 46787 46792 +5 
Misses 41775 41775

Impacted Files	Coverage Δ	Complexity Δ
apps/user_ldap/templates/part.wizard-server.php	0% <0%> (ø)	0 <0> (ø)	⬇️
apps/user_ldap/lib/Connection.php	57.55% <0%> (ø)	119 <0> (+1)	⬆️
apps/user_ldap/lib/Configuration.php	41.91% <25%> (+0.4%)	85 <0> (-1)	⬇️
lib/private/Files/Cache/Propagator.php	96.2% <0%> (+1.26%)	16% <0%> (ø)	⬇️

[blizzz]

I don’t know what DN is (let’s avoid technical jargon), but can we put the DN field and password field next to each other at half width, and put the save button next to it? Then it seems more like a unit.

The "Distinguished Name" (sort of user identifier) can be quite long. Today I saw one that already does not fit into this size. Can be done, but perhaps not that user friendly.

I added now some whitespace instead:

[jancborchardt]

Uh, this requires additional logic for enter buttons presses, etc.? → out of scope for this PR

No, not sure what you mean. You simply add class="primary" to the button element. Try it out. :)

[blizzz]

Uh, this requires additional logic for enter buttons presses, etc.? → out of scope for this PR

No, not sure what you mean. You simply add class="primary" to the button element. Try it out. :)

I'd expect besides a nice color, there is also a shortcut connected to it (e.g. pressing Enter). Maybe my expectations are wrong.

Anyway, these buttons are styled by jQ-UI (i hear you sighing) and thus the primary class values are not respected. I have no idea how to un-jq-ui-fy them (only how to make it bad diffrenetly), other than theming it. Which could not reuse primary either.

Leaves duplicating the primary class code to the ldap css 🙊 and add important over it.

Proper solution is to get rid of the jQuery in general and transform the wizard to the tab-less approach. But this isn't done quickly and i won't have resources for it before Sep.

[jancborchardt]

I'd expect besides a nice color, there is also a shortcut connected to it (e.g. pressing Enter). Maybe my expectations are wrong.

That has to be done either by proper HTML (putting it as the main input type="submit" in the <form>, I think), or failing that, Javascript.
What I was talking about is simply to highlight the button so people know the »Continue« button floating in mid-air is the main action button.

[blizzz]

Added it the ugly way without the buttons magic.

With changed DN or Pwd:

Continue button is always primary, when disabled it looks like:

[jancborchardt]

Alright, looks good. :) But ideally there should only be one primary button at a time. That's why it's called primary. ;)

[blizzz]

Alright, looks good. :) But ideally there should only be one primary button at a time. That's why it's called primary. ;)

OK, i take the feature out of the bug fix and finish that when i am back end of Aug.

[jancborchardt]

Just make only the continue button primary.

[blizzz]

Just make only the continue button primary.

Feature nevertheless ;)

This goes to #5749

Thus, there should not be a blocker anymore. Final reviews, por favore?

[MorrisJobke]

Rebased.

[MorrisJobke]

Tested, works and looks good with the latest style updates to the LDAP wizard in master. 👍

[MorrisJobke]

@nextcloud/ldap Final review please :)