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(notifications): Remove share notification when the node is deleted #39689

Conversation

nickvergessen
Copy link
Member

Steps

  1. Optional: Set up a mobile app for better testing

  2. As UserA go to index.php/settings/user/sharing and disable the checkbox
    grafik

  3. As UserB create a folder and share it to UserA

  4. As UserA reload the page or refresh the notifications on the mobile app, so you see the share notification

  5. As UserB delete the the folder from 3.

  6. As UserA repeat 4.

Expected

No notification anymore

Actual

Notification was still visible and "Decline" yields an error

Checklist

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen added this to the Nextcloud 28 milestone Aug 3, 2023
@nickvergessen nickvergessen requested review from artonge, Antreesy and a team August 3, 2023 09:30
@nickvergessen nickvergessen self-assigned this Aug 3, 2023
@nickvergessen nickvergessen requested review from ArtificialOwl, icewind1991 and nfebe and removed request for a team August 3, 2023 09:30
@nickvergessen
Copy link
Member Author

/backport to stable27

@nickvergessen
Copy link
Member Author

/backport to stable26

@nickvergessen
Copy link
Member Author

/backport to stable25

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.

This means that deleting a file does not delete shares related to it? (since here the share exists but not the file)

@come-nc
Copy link
Contributor

come-nc commented Aug 3, 2023

This means that deleting a file does not delete shares related to it? (since here the share exists but not the file)

Corollary: What will happen to this share which was never accepted and for which notification was removed, will it live on forever in database?

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested on web client, now behavior is aligned with unsharing - notification dismisses after file is deleted

@nickvergessen
Copy link
Member Author

This means that deleting a file does not delete shares related to it? (since here the share exists but not the file)

There is a 15min background job:

class DeleteOrphanedSharesJob extends TimedJob {

And a custom command:

class DeleteOrphanShares extends Base {

But the 15min could be enough time to cause confusion.

What will happen to this share which was never accepted and for which notification was removed, will it live on forever in database?

Yes, the "pending shares" view still allows accepting it

@come-nc
Copy link
Contributor

come-nc commented Aug 3, 2023

This means that deleting a file does not delete shares related to it? (since here the share exists but not the file)

There is a 15min background job

Why is that not done through a listener of the deletion event instead? (or on top of)

@nickvergessen
Copy link
Member Author

Why is that not done through a listener of the deletion event instead? (or on top of)

We'd need to loop over all subfolders and files, potentially thousands to millions

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.

[Bug]: Notifications with shares are not cleared if share has been revoked
3 participants