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.

Sign up for GitHub

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

Jump to bottom

cleanup LDAP's UpdateGroups #33034

Merged
merged 4 commits into from
Jun 28, 2022
Merged

cleanup LDAP's UpdateGroups #33034

merged 4 commits into from
Jun 28, 2022

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jun 27, 2022 •
edited
Loading

  • TimedJob from API
  • DI of IConfig
  • property types
  • throws hints in phpdoc
  • argument and return types
  • a missing return statement

@blizzz blizzz added this to the Nextcloud 25 milestone Jun 27, 2022
@blizzz blizzz requested review from CarlSchwan, come-nc, a team and skjnldsv and removed request for a team June 27, 2022 18:33
CarlSchwan
CarlSchwan approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Nice one, I added some (optional) suggestions to improve this a bit further ;)

apps/user_ldap/lib/Jobs/UpdateGroups.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Jobs/UpdateGroups.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Jobs/UpdateGroups.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Jobs/UpdateGroups.php
*/
private function handleRemovedGroups($removedGroups) {
private function handleRemovedGroups(array $removedGroups): void {
$this->logger->debug(
'bgJ "updateGroups" – dealing with removed groups.',
['app' => 'user_ldap']

This comment was marked as resolved.

Sign in to view

apps/user_ldap/lib/Jobs/UpdateGroups.php Show resolved Hide resolved
apps/user_ldap/lib/Jobs/UpdateGroups.php Outdated Show resolved Hide resolved
come-nc
come-nc reviewed Jun 28, 2022
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I agree with Carl’s remarks, plus the calls to execute should be updated to executeQuery or executeStatement.

apps/user_ldap/lib/Jobs/UpdateGroups.php Show resolved Hide resolved
blizzz added 2 commits June 28, 2022 14:34
@blizzz
cleanup LDAP's UpdateGroups
632f2d7 
- TimedJob from API
- DI of config
- property types
- throws hints in phpdoc
- argument and return types
- replace depracet execute() with executeStatement or -Query
- a missing return statement

Co-authored-by: Carl Schwan <carl@carlschwan.eu>

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
delete groups in chunks instead of one by one
d97f32d 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the td/noid/ldap_group_updater branch from 96cf2c7 to d97f32d Compare June 28, 2022 12:35
@blizzz
Copy link
Member Author

blizzz commented Jun 28, 2022

Took also the optional suggestions into account 🚀

@blizzz @come-nc
simplify
52c957e 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 28, 2022
@blizzz blizzz force-pushed the td/noid/ldap_group_updater branch from fb0b9a0 to f364616 Compare June 28, 2022 20:29
@blizzz
adjust tests
8185a3d 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the td/noid/ldap_group_updater branch from f364616 to 8185a3d Compare June 28, 2022 20:47
@blizzz blizzz merged commit cb7853c into master Jun 28, 2022
@blizzz blizzz deleted the td/noid/ldap_group_updater branch June 28, 2022 21:48
@joshtrichards joshtrichards mentioned this pull request Sep 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: ldap technical debt
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
3 participants
@blizzz @CarlSchwan @come-nc