sabre-io / Baikal

Baïkal is a Calendar+Contacts server
https://sabre.io/baikal/
GNU General Public License v3.0
2.5k stars 289 forks source link

add SMTP email sending, LDAP authentication with auto user creation #1119

Closed epsilon-0 closed 2 years ago

epsilon-0 commented 2 years ago

Signed-off-by: Aisha Tammy floss@bsd.ac

Closes #1014 Closes #937 Closes #865 Closes #1114

epsilon-0 commented 2 years ago

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.

epsilon-0 commented 2 years ago

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.

Would really like to get these features in the main repo, I think they are very useful for a lot of people.

epsilon-0 commented 2 years ago

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

epsilon-0 commented 2 years ago

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.

ByteHamster commented 2 years ago

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?

El-Virus commented 2 years ago

@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.

epsilon-0 commented 2 years ago

: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.

El-Virus commented 2 years ago

@epsilon-0, Great it's good to check on multiple instances. If you find anything, feel free let me know.

El-Virus commented 2 years ago

@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.

ByteHamster commented 2 years ago

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/

El-Virus commented 2 years ago

@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.

ByteHamster commented 2 years ago

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.

El-Virus commented 2 years ago

Ok, I'll start making smaller PRs in a few minutes.

El-Virus commented 2 years ago

@ByteHamster, @epsilon-0, I've opened #1122, #1123, #1124. Also, note that #1123 and #1124 branch off of #1122, so keep that in mind.