nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
26.91k stars 4.01k forks source link

Fix or retire legacy notifications in the Updater to avoid constant updater notifications if the Notifications app is disabled #43645

Open benapetr opened 7 months ago

benapetr commented 7 months ago

How to use GitHub

Is your feature request related to a problem? Please describe. Yes, when there is a brand new update that I can't update to, because some of my extensions aren't yet compatible with it, I keep getting annoying popups that are telling me I should update to new version every single time I load any page, or refresh it. I need to disable this.

Describe the solution you'd like I want to be able to disable all those annoying update notifications.

Describe alternatives you've considered I was looking through the admin configuration, couldn't see any option to disable it.

Additional context

image
solracsf commented 7 months ago

Did you try disabling the "Update notification" / "Nextcloud announcements" apps?

benapetr commented 7 months ago

I don't see any such app, but I have disabled "Notifications" which seem to be providing this kind of notification and I am still receiving them:

image
benapetr commented 7 months ago

OK, I fond 2 other apps that provide "update notifications", once I disabled them all it stopped appearing.

joshtrichards commented 7 months ago

Yes, when there is a brand new update that I can't update to, because some of my extensions aren't yet compatible with it, I keep getting annoying popups that are telling me I should update to new version every single time I load any page, or refresh it. I need to disable this.

I think this is the legacy update notifier you're seeing (which I hadn't realized even existed until now) not the regular one. This explains some weird reports I've seen that never made sense to me. It looks like you already had the "Notification" app (notifications) disabled when you reported this (based on the lack of a bell icon in your screenshot). So you aren't getting the standard push notifications about updates (which are better handled in that code path - more on that below).

I think you'll get the behavior you want if you enable both apps (Notifications and Update notification) then just dismiss the next update notification (once). That notification shouldn't appear inline nor repeat itself (i.e. on each page reload like you were seeing when you reported this). Instead, a single update notification will some as one-time push notifications under the bell icon (which will return after you re-enable the standard Notification app). And you can leave it or dismiss it at your discretion, but it won't repeat nor be visible on every page reload. You'll only get one for each new major version when made available, instead of constantly seeing those inline notifications.

In short: The update notifications appearing inline on every page should only occur if the Notifications app is disabled (since a legacy update notification code path gets triggered to make sure admins still at least get told about updates being available).

Background (mostly notes for myself or whoever gets around to cleaning up the current behavior)

In a standard installation, the update check runs once a day and the notification of a new update being available should not return once the initial notification is dismissed (even the next day after the next check) as long as the version being offered hasn't changed:

https://github.com/nextcloud/server/blob/1731d3154a8e9c8d3692c6c73f5a6b91b9bc17a5/apps/updatenotification/lib/Notification/BackgroundJob.php#L166-L168

But things are different if the legacy update notification handler is triggered. The legacy one sends a notification constantly (upon each DOM load!).

I think we have the following three scenarios occurring:

(1) Standard installation:

(2) If the Notifications app gets manually disabled by the user:

(3) Both the Notifications and Update notification apps disabled:

Both (1) and (3) seem reasonable (though I suspect some people have utilized the third approach - disabling all push notifications - just to try to get the DOM loading notifications to go away - which technically disabling outright does solve but has obvious trade-offs of disabling all push notifications). I think (2) with the legacy handler should probably get refactored. It's tempted to pull it out entirely, but I presume it's in-place to attempt to make sure that admins that have push notifications disabled still get some reminders about new update availability.

So there may be some room here for clean-up - e.g.: