Skip to content

Conversation

@gavine99
Copy link

@gavine99 gavine99 commented Mar 30, 2025

Summary

changes to ensure all notification deletions are routed through the core notification manager's markProcessed(). that function then calls this app's markProcessed() function which actually performs the delete(s) from the db.

reason for change

routing all deletions through the core notification manager gives other registered notification apps the ability to be aware that the notification has been or will be deleted and allows them to perform relevant actions.

no new tests required. 4 tests no longer required and removed.
no front-end changes.
no documentation updates required.
no backporting required.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

routing all deletions through the core notification manager gives other registered notification apps the ability to be aware that the notification has been or will be deleted and allows them to perform relevant actions.

You should already be able to do exactly this, by implementing an IDismissableNotifier

Interface INotifier classes should implement if they want to process notifications that are dismissed by the user.

E.g. the notify_push app that is used by the desktop client and web is using that to refresh it's list after the user purged notifications or deleted a single one:
https://github.com/nextcloud/notify_push/blob/634ce61a67f29c443d8b1df9b530758130205b2e/lib/Listener.php#L108-L112

I guess that is exactly what you also want to do?


I'm also a bit hesitant to do this, as the code is highly optimized for extreme cases atm, e.g. a call with notification sending notifications to hundreds of users in the same request, and so on. That has to delete old call notifications before, and I'm not sure that would scale well if it takes yet another loop through all the layers.

At least the current removal of the dismissNotification call would result in breaking/changed behaviour of notify_push, so this needs more thought's and can't go in easily atm.

@gavine99
Copy link
Author

gavine99 commented Apr 7, 2025

thanks @nickvergessen . i'll look at using the IDismissableNotifier to achieve the desired outcome

@gavine99
Copy link
Author

so, i've modified my app to use inotifier. for me, it's the very most ideal because it sets my app as a notifier, which it is not but, as you say, it enables access to the dimissnotification() method.
that makes this pr not required so i have closed it.

@gavine99 gavine99 closed this Apr 19, 2025
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.

2 participants