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

Use suppression of notifications if configured #611

Open
wants to merge 3 commits into
base: v13/main
Choose a base branch
from

Conversation

ljfio
Copy link

@ljfio ljfio commented Apr 10, 2024

This enables the notification suppression functionality hidden in the uSyncSettings.
The XML documentation has been updated to reflect the purpose of the flag.

Currently we have a large import (~7000) media items that is hanging on processing the MediaTreeChangedNotification, we are not concerned for these at the moment and would prefer skipping entirely.

Risks:

  • No tree changed notifications firing during the import which will mean the caches are not up-to-date.
  • Any notifications typically handled by the application will not be raised (e.g. saved / published notifications that perform actions afterwards).

@KevinJump
Copy link
Owner

KevinJump commented Apr 10, 2024

Hi,

I think we didn't use this in the end because the suppressesion all notifications, including ones that Umbraco needs to fire for things to be updates (e.g models builder and examine and the nucache all use notifications to update things when you update content types and content) and that can cause unintended issues with some core things.

I will double check this though - but i am fairy sure it messes up new syncs on clean sites, and when you change some of the fundimental stuff (like a doctype's properties.

At the moment Notifications are batched* because the scope provide has a ScopedNotificationPublisher attached (ours is in SyncScopedNotificationPublisher) to it which means you see things like the MediaTreeChangedNotification firing at the end

there is an option to let you push those notifications into a background thread (some docs). which means they still all happen but they are not in the UI thread so you can carry on with your buisness while umbraco sorts them out.

The thing we didn't leave in (and maybe we should have?). Is the 'old' behavior where the items fire right after they are imported, this is no quicker - its probably slower - but you wouldn't just see a 'processing notification' message because it would happen with each media item.


but i will double check, what the implications of this change are , i don't mind it being in but we probibly need to full understand what can break if you turn it on (only because we will get people raising issues, even if we warn them, and it would be good for us to know what they can be beforehand).

*not all notifications are batched, Umbraco will fire some as they happen regardless of what you do with a Notification Publisher

@ljfio
Copy link
Author

ljfio commented Apr 10, 2024

Hi @KevinJump,

I had a deep dive into this when the "Processing Notifications (X)" was hanging on the MediaTreeChangedNotification.

My understanding is that the ScopedNotificationPublisher by default will fire all of these at the end of the scope, only the "cancelable" ones are fired at the point they are raised unless the publishCancelableNotificationOnScopeExit was set to true when its constructed. Which I think the SyncScopedNotificationPublisher sets as false.

Based on that logic, I believe the IEventAggregator responsible for dishing out the cancellable notifications will still receive these.

With my change there will be no notifications published, both cancellable and ones fired off at the end of the scope, only if the EnableSuppressNotifications setting is set to true.

I would completely agree that using this setting should be exercised with extreme caution and that end users should understand the risks of suppressing the notifications. Developers that have experience using the ScopedNotificationPublisher.Suppress capability will hopefully understand this.

Alternatively could change this functionality to do one of the following:

  • Allow suppression of individual notification types
  • Allow suppression of only the scoped notifications (e.g. discard all and do not sent to the IEventAggregator)

@KevinJump
Copy link
Owner

KevinJump commented Apr 16, 2024

based on your note above I've added a "SuppressNamedNotifications" option here, so you can choose a notification not to process.

e.g

      "SuppressNamedNotifications": [ "ContentTreeChangeNotification" ]

I am not sure if this now makes the full suppression option less attractive / redundant, we could always add a 'All' to this option ?

intrested to know what you think ?

Worth noting that suppressing "ContentTreeChangeNotification" actually breaks the next sync, as the content hasn't been updated, so probably not the best example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants