Delete content notifications not sending

[kjsguld]

The delete notification on content does not send notifications.

Reproduction

Bug summary

Notifications are not send on deletion.

Specifics

Inspecting the logs in the backoffice under /settings/logViewer/overview doesn't provide any further insights either.

Steps to reproduce

Create an environment running 8.0.1
Set up SMTP settings in web.config
Set up notification mail in umbracosettings.config
Set up notifications on a content node
Create, move, publish notifications are sent
Upon deletion of a node, no email seems to be sent.

Expected result

That a notification would be sent as on create etc.
The message you normally receive looks like this:

Actual result

No mail is sent.

[kjac]

I can reproduce this and a few other notification issues as well... will look at a fix.

[kjac]

OK. This is a bit of a pickle.

All notification subscriptions for a specific content item are deleted when the content item is deleted due to database consistency considerations. Thus the delete notification subscriptions aren't available afterwards in the event handler that is supposed to process them.

This is one for HQ to consider. So far I can see four possible solutions, all of which are less than ideal.

1: Compromise with database consistency

The cleanest fix is probably to allow potential database inconsistency by removing the foreign key between umbracoUser2NodeNotify and umbracoNode. The fix would then be:

1. Stop deleting the notification subscriptions when deleting content.
2. Let the notification service delete the notification subscriptions after processing delete notifications.

Obviously this solution has a built-in risk of umbracoUser2NodeNotify being polluted with notification subscriptions for content that no longer exists, if something fails before the subscriptions are deleted by the notification service.

2: Something less than thread safe

The notification service uses the content service Deleted event to handle delete notifications. As described above that's too late - the notification subscriptions have already been deleted at that point.

This could be solved by letting the notification service subscribe to the Deleting event and "remember" which notification subscriptions to process in the Deleted event. But it's a really ugly solution and as far as I can see it's not at all thread safe.

3: The very un-SOLID approach

The content service could take a dependency on the notification service and initiate the delete notifications itself instead of having the notification service do it in the Deleted event.

However it seems to me that this would be adding a responsibility to the content service that does not belong there.

4: The breaking change approach

The last option is even worse 😆 but here goes anyway: If the Deleted event was fired before the content was actually deleted, the whole thing would solve itself. The notifications would be sent as expected.

This would be a behavioral breaking change and also a really strange behavior in general.

/cc @Shazwazza

[Shazwazza]

Great investigation! :)

As for #1, #3 and #4 - we cannot do any of those unfortunately.

however a variation of #2 can work.

We send notifications in the NotificationsComponent by listening to events, in this case it's the ContentService.Deleted event which eventually calls into the NotificationsComponent.SendNotification and then into the _notificationService.SendNotifications which looks up notifications for the user but as you've noted these will no longer exist in the database.

What we (probably) can do is in the ContentService.DeleteLocked before deletion occurs, we can use the INotificationsRepository to lookup user notifications (potentially all of them since I'm unsure if UmbracoContext.Security.CurrentUser is the same user Id as passed into the content service), we can add the notification relations to the event args to the AdditionalData dictionary under a specific key. We can then use that information in the NotificationsComponent to pass this information downward. There's probably a bunch of plumbing that might need to be shifted for that to work but it should be do-able.

[kjac]

Cool 👍 If you're comfortable with that "shared" notification workload I'll have a look at it, hopefully today 😊

[Shazwazza]

I guess there's 2 ways to do this:

• What i mention above, which means that the ContentService would take on a dependency on the INotificationsRepository OR
• More like what you said but it is thread safe - The ContentService.Deleting event is handled by the NotificationsComponent, then it adds whatever state it wants to the event args EventState dictionary. This even state will carry through to the Deleted event for it to handle ... BUT unfortunately i can see in the ContentService that this state will not be carried through but it absolutely should. For all of the "Firing", "Fired" events we should be using the same event args instance which you can see being done in almost all places in the services, however the args is changed to Cancelable = false before raising the "Fired" event. In the ContentService delete operation this is not happening but it should and could be done by passing the initial Delete args to the DeleteLocked method (hope this is making sense). In this option the ContentService wouldn't need to take a dependency on INotificationsRepository, so maybe this is better.

[kjac]

Awesome. Sounds like a better alternative 👍 I'll poke around in it later.

[kjac]

@kjsguld @Shazwazza upon further reflection... shouldn't delete notifications really be sent when content is moved to the recycle bin instead of deleted from it? Moving an item to the recycle bin is conceptually a delete operation (it's even called "Delete" in the tree context menu).

Receiving a notification after content is deleted from the recycle bin doesn't help anyone - you can't really do anything about it, at least not from the UI. On the other hand, receiving the notification when content is moved to the recycle bin seems to be meaningful.

I realize that this is a different behavior than V7, but if we're going to change it, now would be a good time to do so.

[kjsguld]

@kjac I agree with this perception of the concept. It's it should rather be a message concerning the fact that a given content node has been sent to the recycle bin - or "staged for deletion" in other words.
Likewise the idea of a notification about a "soft" deletion rather then a possibly irreversible change would be a much better UX for an administrator in my opinion, since it could easily be acted upon.

[kjac]

There's also the fact that the deleted notification (yes I eventually managed to get it to send) contains a link to edit the deleted item. Which obviously results in an error message stating that the element does not exist. So... all the more reason to send delete notifications when an item is moved to the recycle bin.

I have an existing PR that fixes move/copy notifications (#5255). I'll amend that to include a fix for this.

[kjac]

PR #5255 is updated to handle deletion

[Shazwazza]

Yup yup and yup! of course that all makes logical sense and probably makes the coding a little less painful :)