owncloud / activity

:zap: Activity app for ownCloud
33 stars 40 forks source link

[QA] notification bell does not appear for pending group shares #1192

Closed jnweiger closed 6 months ago

jnweiger commented 1 year ago

Seen with core 10.13.0-beta1 and activity 2.7.2and both core 10.13.0-beta1 and 10.12.2 (not a regression)

Expected behaviour: Both user and group shares behave the same.

pako81 commented 1 year ago

Not reproducible on 10.13.0-beta1 with both activity 2.7.1 and 2.7.2:

image
pako81 commented 1 year ago

Reproducible when the option Automatically accept new incoming local user shares is opted out by the single user rather than by the admin itself for all the users.

pako81 commented 1 year ago

Problem is the following:

This is done by the following code https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/Controller/Share20OcsController.php#L590-L600, which checks if the share is a group share and if $globalAutoAccept is set, and for every user having !$userAutoAccept it creates a subshare with status pending.

Problem is that there the parent group share is not set to status pending as well, so we end up in https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/Service/NotificationPublisher.php#L79-L81 and no notification will be created.

Even if we set the status to pending for the parent group share in Share20OcsController.php, https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/Service/NotificationPublisher.php#L52-L61 will create a notification for all group members, even those who chose to automatically accept shares...

Not sure this can be fixed. Maybe @jvillafanez @phil-davis @IljaN you see a solution here.

jvillafanez commented 1 year ago

I don't know if it's possible, but maybe the state of the group share should be dynamically evaluated based on the subshares: if there is at least a pending subshare, the group share should also be pending. The additional problem is that the notification is sent based on the share not on the the subshares.

My main worry is that the change could break more things, and I'm not sure how much we need to test