Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework of LDAP user import UI #17214

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

cconard96
Copy link
Contributor

@cconard96 cconard96 commented May 31, 2024

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -

Based on #16951. Clean up/twig migration for the LDAP user import UI.

Before:
Selection_334

After:
Selection_336

Goal is to remove the use of temporary data in the session if possible and use less page reloads. Currently with this PR, switching between "expert" and "simple" views is seamless now.

@cconard96 cconard96 marked this pull request as ready for review June 2, 2024 21:41
@cconard96 cconard96 changed the title [WIP] Rework of LDAP user import UI Rework of LDAP user import UI Jun 2, 2024
@cconard96 cconard96 removed the wip label Jun 2, 2024
@cconard96
Copy link
Contributor Author

Sorting of results is not present in the new UI. The old behavior was completely broken and would clear the results (#14865). The implementation also only would have changed the order of the results returned by the LDAP server. It didn't actually sort by any field client-side or server-side. When researching server-side sorting, I found that it is an optional implementation and Microsoft AD doesn't seem to support it. For client-side sorting, it would still only sort the results returned which may only be a partial result depending on LDAP settings. So, there doesn't seem to be a good way to support sorting here.

Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LDAP tests are failing.

@cconard96
Copy link
Contributor Author

LDAP tests are failing.

Fixed

Copy link
Contributor

@trasher trasher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I've not any local setup to test, and tha'ts not a part I know well; I do not know if changes are OK.
I'm "afraid" changes from $_SESSSION to $_REQUEST (which we should avoid, but that's not the point) would really break things - for example.

This seems a bit huge change for a "UI rework".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants