Baikal
Baikal copied to clipboard
add SMTP email sending, LDAP authentication with auto user creation
Initial setup for working towards issue https://github.com/sabre-io/Baikal/issues/1097.
This PR adds javascript functions to dynamically hide elements based on selected values in a dropdown menu or if a checkbox is marked.
This has no effect as of now but I will make a PR to add the new elements for LDAP authentication which will be hidden by default, unless 'LDAP' is selected in the dav_auth_type options.
OK, I've added a whole lot of functionality. The code is a bit rough but that can be worked on given everyone is happy with the features provided.
- Added SMTP email scheduling: this one is something a lot of people have asked for on many many channels, so this feels like something that will be very useful.
- Added LDAP authentication with auto-creation of users: I have been unable to find a way to do this in the sabre-io/dav repository plus my PR for LDAP there has been pending for a while, so I think adding it here is a better idea.
- Hiding of options that are not relevant: LDAP options are only going to be visible when it is selected as the auth type, SMTP options are only going to be visible when it is checked in the box.
Would really like to get these features in the main repo, I think they are very useful for a lot of people.
Will close issues - https://github.com/sabre-io/Baikal/issues/1014, https://github.com/sabre-io/Baikal/issues/937, https://github.com/sabre-io/Baikal/issues/865, https://github.com/sabre-io/Baikal/issues/1114
This will be easier if the LDAP PR in sabre-io/dav gets merged first, that way the functionality is present in the library and more people can use that.
Thanks for working on this!
Baikal already has dynamic element hiding functionality (see for example the database settings page). Could you please use the existing functionality instead of creating a new one?
@epsilon-0 @ByteHamster, I've created a commit in epsilon-0's repository that replaces the dynamic element hiding functionality for the original one. It also adds support for LDAP filters, groups and attribute searching.
:O @El-Virus that morphology seems quite complex, kudos for getting that working, I'll try this out for a while on my server and see how it works.
@epsilon-0, Great it's good to check on multiple instances. If you find anything, feel free let me know.
@ByteHamster, About the CI checks, and the File reversion, I'll submit a pr to @epsilon-0's repository in a couple of hours. And about the whole 3 commits in one, Morphology has to be implemented in both, so it's basically 2 PRs and being honest, they're quite small, so I personally don't think it's worth the trouble of reverting, and restaging all the changes to make them separate PRs.
And about the whole 3 commits in one, Morphology has to be implemented in both, so it's basically 2 PRs and being honest, they're quite small, so I personally don't think it's worth the trouble of reverting
This is mainly about reviewing. The morphology change itself can directly be merged with about 5 minutes of review. Both other functions need significantly more review. I had to review many of this kind of PRs in the past and splitting the PR up will definitley help getting this merged more quickly. Over the last month, I have looked at this PR multiple times and then closed it again because my spare time would have been too short for testing both LDAP and SMTP. If it was only one of them, the probability of me actually reviewing would have been a lot higher :)
See also https://www.swarmia.com/blog/why-small-pull-requests-are-better/
@ByteHamster, I'll see what I'm able to do to split the PRs, but about the Morphology commit, its purpose is to hide the SMTP/LDAP fields so it needs to be implemented in both.
The change to Listbox.php
is independent of the other changes. It can be merged right away if it is its own PR. Then the others can build on that.
Ok, I'll start making smaller PRs in a few minutes.
@ByteHamster, @epsilon-0, I've opened #1122, #1123, #1124. Also, note that #1123 and #1124 branch off of #1122, so keep that in mind.