Skip to content

Added user autocomplete from ldap in create view and a parameter to enable it#543

Open
AndreScara11 wants to merge 5 commits into
2amigos:masterfrom
AndreScara11:create-new-user-autocomlete-from-ldap
Open

Added user autocomplete from ldap in create view and a parameter to enable it#543
AndreScara11 wants to merge 5 commits into
2amigos:masterfrom
AndreScara11:create-new-user-autocomlete-from-ldap

Conversation

@AndreScara11

Copy link
Copy Markdown
Contributor

Implemented the possibility to autocomplete email and username from ldap when create a new user

@maxxer maxxer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for your contribution. Please review the suggested changes, don't forget to extract messages and run php-cs-fixer.

Comment thread src/User/resources/views/admin/_user.php Outdated
Comment thread src/User/Module.php Outdated
Comment thread src/User/Dictionary/UserSourceType.php
Comment thread src/User/resources/views/admin/_user.php Outdated
Comment thread src/User/resources/views/admin/_user.php Outdated
Comment thread src/User/resources/views/admin/_user.php Outdated
Comment thread src/User/resources/views/admin/_user.php Outdated
@maxxer

maxxer commented Mar 8, 2024

Copy link
Copy Markdown
Collaborator

@schmunk42 @eluhr since you use LDAP extension, do you mind having a look as well? thank you

@AndreScara11

Copy link
Copy Markdown
Contributor Author

In the last commit I adjusted the following things:

  • Added parameters with explanations in the docs folder
  • Added how to install typeahead if you want to use it, in the docs folder
  • Fixed translations
  • Run php-cs-fixer

@TonisOrmisson

TonisOrmisson commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

First, thank you for contributing!

I am not a fan of how the Typeahead is being used here.

The typeahead original repo seems to be dead?

Also, the similar functionality here is used via 2amigos/yii2-selectize-widget. The latter is also abandoned, but using the already used dependency we will not be introducing a new "old/outdated" dependency here. In v2 branch bootstrap5 version select2 replaces yii2-selectize-widget.

We are generally dealing in v2 with the issue of how to get past old dependencies (bootstrap3), in this light adding something "old" on top of existing seems like going the wrong way.

I would suggest either use something alive and up to date, or go with using the already used selectize-widget here.

edit: or use kartik/select2 for this (replaces selectize-widget in v2). both select2 and selectize seem to have ajax quering options

Comment thread docs/install/configuration-options.md Outdated
Comment thread docs/install/configuration-options.md Outdated
Comment thread src/User/Bootstrap.php Outdated
@AndreScara11

Copy link
Copy Markdown
Contributor Author

Last changes:

  • Used Selectizetextinput instead of Typeahead
  • Added property ldapUid to User model to handle the ldap users without the username
  • Changed ldap params names
  • Updated docs

Comment thread docs/install/configuration-options.md Outdated
Comment thread src/User/Bootstrap.php Outdated
Comment thread src/User/Model/User.php
Comment thread docs/install/configuration-options.md Outdated
@AndreScara11

Copy link
Copy Markdown
Contributor Author

Done, sorry for the misunderstanding.

@AndreScara11

Copy link
Copy Markdown
Contributor Author

Have you had a chance to review?

@eluhr

eluhr commented Apr 17, 2024

Copy link
Copy Markdown
Contributor

@schmunk42 @eluhr since you use LDAP extension, do you mind having a look as well? thank you

@maxxer since you asked for my opinion on this, here is my assessment. I find it somewhat critical to wire the LDAP so closely to the package. I think the whole thing would be better off in a separate package, which overwrites the required views and controller actions or works with the existing events at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants