Skip to content

Conversation

@noiob
Copy link
Contributor

@noiob noiob commented Feb 15, 2021

When using LDAP, it may be necessary to check whether a group is managed by LDAP or the Database backend. I've added that info both to the group:list command when run with --info (as to not introduce breaking changes) and to the new group:info command. I've tried to keep them similar to user:list --info and user:info.

@noiob noiob force-pushed the feature/add-backend-list-groups branch 3 times, most recently from f8e8ac6 to c5b9ee4 Compare February 16, 2021 10:01
@noiob noiob marked this pull request as draft February 16, 2021 10:02
@noiob noiob force-pushed the feature/add-backend-list-groups branch from c5b9ee4 to 45c2dbc Compare February 16, 2021 10:08
@noiob noiob marked this pull request as ready for review February 16, 2021 10:10
@noiob noiob force-pushed the feature/add-backend-list-groups branch 2 times, most recently from d9aa41f to 0c8b322 Compare February 16, 2021 16:25
@kesselb
Copy link
Contributor

kesselb commented Feb 17, 2021

Thanks a lot 👍

Please migrate to CamelCase for variable names.

@noiob noiob force-pushed the feature/add-backend-list-groups branch from 0c8b322 to 5e8a029 Compare February 17, 2021 13:49
@noiob
Copy link
Contributor Author

noiob commented Feb 17, 2021

Please migrate to CamelCase for variable names.

done, hope I didn't miss any!

@noiob noiob force-pushed the feature/add-backend-list-groups branch from 5e8a029 to a3542de Compare February 17, 2021 14:38
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

That's great, thank you for the contribution!

@noiob noiob force-pushed the feature/add-backend-list-groups branch 6 times, most recently from 0e1f3da to 9b556e5 Compare February 22, 2021 18:16
@noiob
Copy link
Contributor Author

noiob commented Feb 22, 2021

@blizzz Thanks for the comprehensive review, I tried to implement all the changes you requested, it feels a good bit more elegant now 😄
I think the php-cs check isn't actually failing in my code, but in apps/sharebymail/lib/Settings/SettingsManager.php

Edit: I have opened a PR for the issue in sharebymail SettingsManager at #25778

@noiob noiob requested a review from blizzz February 22, 2021 21:48
@noiob noiob force-pushed the feature/add-backend-list-groups branch 2 times, most recently from 0a6c0b8 to dce3097 Compare March 1, 2021 13:41
Signed-off-by: Johannes Leuker <j.leuker@hosting.de>
@noiob noiob force-pushed the feature/add-backend-list-groups branch from dce3097 to 2796ef8 Compare March 1, 2021 15:02
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

looks good, tested + works, has tests, 👍

@rullzer rullzer merged commit 54cffef into nextcloud:master Mar 5, 2021
@noiob noiob deleted the feature/add-backend-list-groups branch March 8, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants