-
Notifications
You must be signed in to change notification settings - Fork 12
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
delete_data_for_users fix #31
base: master
Are you sure you want to change the base?
delete_data_for_users fix #31
Conversation
So this is certainly a bug, but your fix isn't correct. In delete_data_for_users we only want to delete data belonging to the listed users. Your change indiscriminately deletes the galleries - regardless of if they belonged to one of the users in $userlist. The original code is obviously broken too. What it needs to do instead is include |
OK, I think I get it now. Reading over the code again, though, I think there might be another bug. $itemidsql selects items with the relevant user IDs, but not items in galleries with the relevant user IDs. Is this an issue, or does cascading deletes take care of this? (I suspect not?) Either way, the line that deletes from the mediagallery_item DB table only deletes items in galleries with with the relevant user IDs, I think? [EDIT: Not items with the relevant user IDs themselves.] |
It doesn't look like there's cascading deletes to take care of things. And I think the new issue also applies to delete_data_for_user(). I've had another go at it. Not sure how to delete comments made on an image that isn't a specified user's but is in a specified user's gallery, though. If you accept it, it would probably be good to squash merge it, to avoid having the faulty fix in the commit history. |
Oh, looks like there's a function \core_comment\privacy\provider::delete_comments_for_all_users_select() that could be useful. I might look at this tomorrow. |
I think there is a small error in the function mod_mediagallery\privacy\provider::delete_data_for_users() .