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

Correctly listen to group change events #1601

Merged
merged 7 commits into from
Jan 25, 2023
Merged

Conversation

simonspa
Copy link

In order for group sharing to properly work, we also need to listen to changes in the group:

  • A user is removed from the group (Group\UserRemovedEvent): We need to remove all pictures from this user from all albums unless they are shared also directly with the user
  • A group is dropped completely (Group\GroupDeletedEvent): We need to remove the group as collaborator from all albums and remove all files added by users in this group unless they also have a direct share of the album

This MR builds on top of my other MRs and I'd be happy to rebase this branch once the others are merged.

@simonspa simonspa added bug Something isn't working 1. to develop Accepted and waiting to be taken care of labels Jan 22, 2023
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 22, 2023
@szaimen szaimen added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Jan 22, 2023
@artonge
Copy link
Collaborator

artonge commented Jan 23, 2023

  • A user is removed from the group (Group\UserRemovedEvent): We need to remove all pictures from this user from all albums unless they are shared also directly with the user

  • A group is dropped completely (Group\GroupDeletedEvent): We need to remove the group as collaborator from all albums and remove all files added by users in this group unless they also have a direct share of the album

I think that we should delete the photo even if the album owner has access to the photo through another mean. It makes things simpler and is probably what a user would expect.

@simonspa
Copy link
Author

I think that we should delete the photo even if the album owner has access to the photo through another mean. It makes things simpler and is probably what a user would expect.

I would expect the opposite actually. Imagine:

I create an album "NC Photos Hackweek" and share it with you. You add your pictures to it. Then we decide to also share it with the rest of the NC Photos team as group share. Later we realize that that's not a good idea and remove the group share again. I would expect your pictures to stay in because you are still a shareholder (sorry, collaborator) of the album. Your pictures would have been removed only because you also happened to be in the NC Photos team.

@simonspa
Copy link
Author

...to add: the issue is that we cannot determine by which means (group or direct share) a photo has been added. (and it's good because knowing this would make things even more complicated... 😄 )

@artonge
Copy link
Collaborator

artonge commented Jan 23, 2023

I misunderstood your initial comment, I thought "unless they are shared also directly with the user" was referring to a classic file share.

Other PR seems bundled inside this one. I'll review the code of this one once the other are merged :).

@simonspa
Copy link
Author

Ah, sorry, no - I stayed completely within the Photos realm. :) I agree that folding in other sharing types into this is neither sensible nor easily feasible.

I'll rebase this once the others are merged.

Simon Spannagel added 4 commits January 24, 2023 13:57
Signed-off-by: Simon Spannagel <simonspa@kth.se>
Signed-off-by: Simon Spannagel <simonspa@kth.se>
Signed-off-by: Simon Spannagel <simonspa@kth.se>
Signed-off-by: Simon Spannagel <simonspa@kth.se>
@simonspa simonspa marked this pull request as ready for review January 24, 2023 12:59
@simonspa
Copy link
Author

@artonge rebased and ready for review! 🚀

lib/Listener/GroupDeletedListener.php Outdated Show resolved Hide resolved
lib/Listener/GroupDeletedListener.php Outdated Show resolved Hide resolved
lib/Listener/GroupDeletedListener.php Outdated Show resolved Hide resolved
lib/Listener/GroupUserRemovedListener.php Outdated Show resolved Hide resolved
Simon Spannagel added 3 commits January 24, 2023 21:46
Signed-off-by: Simon Spannagel <simonspa@kth.se>
Signed-off-by: Simon Spannagel <simonspa@kth.se>
Signed-off-by: Simon Spannagel <simonspa@kth.se>
@artonge artonge merged commit c0089cb into nextcloud:master Jan 25, 2023
@simonspa
Copy link
Author

/backport to stable25

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label Jan 25, 2023
@backportbot-nextcloud backportbot-nextcloud bot removed the backport-request Pending backport by the backport-bot label Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants