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.

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

fix: implement hooks to properly cleanup users upon complete removal #626

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

pako81
Copy link
Contributor

@pako81 pako81 commented Apr 9, 2024

We are currently missing the hook for removing the user from the oc_custom_group_member table when performing the user deletion. I do not see any reason why the user should still be listed in the custom groups view (with an X avatar).

This would solve the issue for both "normal" and guest users, as currently when navigating through the group members views a new invitation mail is going to be resent to the guest user so that he/she will regain access to resources he/she should no longer have access to.

@pako81 pako81 added this to the development milestone Apr 9, 2024
@pako81 pako81 requested review from IljaN and jnweiger April 9, 2024 16:32
@pako81 pako81 self-assigned this Apr 9, 2024
Copy link
Contributor

@jnweiger jnweiger 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 to me.
My reproducer is fixed. Also I no longer see any ghost user with avatar (X).

I don't understand why CI fails, the case of removing a user from a custom group works fine for me, with or without this patch.

@pako81
Copy link
Contributor Author

pako81 commented Apr 10, 2024

I also do not understand it, but running the tests locally confirm that test is failing with this PR in place.

@pako81 pako81 marked this pull request as ready for review April 11, 2024 06:41
@pako81 pako81 changed the title fix: properly cleanup users upon removal fix: implement hooks to properly cleanup users upon complete removal Apr 11, 2024
@jnweiger jnweiger self-requested a review April 11, 2024 09:58
appinfo/app.php Outdated Show resolved Hide resolved
Copy link
Member

@IljaN IljaN left a comment

Choose a reason for hiding this comment

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

See my suggestion, other than that LGTM.

Co-authored-by: Ilja Neumann <ineumann@owncloud.com>
Copy link

sonarcloud bot commented Apr 11, 2024

Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

Hooks:register(); is nicer.
Everything works like a charm now.

@pako81 pako81 merged commit 49606db into master Apr 11, 2024
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the properly_cleanup_users branch April 11, 2024 11:23
@jnweiger jnweiger mentioned this pull request Apr 11, 2024
43 tasks
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.

3 participants