U4-7757 - Notification not shown using CancelOperation on unpublishing event

Created by Dave Woestenborghs 16 Jan 2016, 13:27:11 Updated by Dave Woestenborghs 18 Jun 2018, 13:47:06

Tags: PR

Is duplicated by: U4-8739

Is duplicated by: U4-11454

Relates to: U4-10870

Subtask of: U4-9609

What did you do?

I create a event handler on the ContentService.UnPublishing event to prevent unpublishing of pages that are needed for a functioning website.

This is the code of the event handler : private void ContentServiceOnUnPublishing(IPublishingStrategy sender, PublishEventArgs publishEventArgs) { var doctypes = new[] { Common.Constants.Doctypes.Website.DocTypeAlias, Common.Constants.Doctypes.Homepage.DocTypeAlias };

        foreach (var entity in publishEventArgs.PublishedEntities)
        {
            if (doctypes.Contains(entity.ContentType.Alias))
            {                   
                publishEventArgs.CancelOperation(new EventMessage("Waarschuwing", "Deze pagina kan niet offline gehaald worden", EventMessageType.Warning));
            }
        }
    }

What did you expect to happen? The page is not unpublished and a notification is shown to the user that page can not be unpublished

What actually happened? The page is not unpublished, but there is no notification shown in the backend. For the Trashing event this works, because I use the same logic there.

Comments

Dan Booth 07 Jul 2016, 12:51:34

I've noticed this in 7.4.3 - it is very handy to be able to stop people unpublishing essential pages (like home page!) with events.


Pete Duncanson 31 Jan 2017, 15:02:04

Had this hit me today. If you select "Delete" from the context menu on the tree then it works and shows the notification as to why it got cancelled. If you select "Unpublish" from the "Save and Publish" green button however then it still works however it doesn't show the notification messages to the front end. I think the notifications just need adding for Unpublish events.

The dev tools show the PostUnpublish ajax call returning a 400 status code with "X-Status-Reason: Validation failed" but I think this is by design and is a red herring. I think the front end is just not rendering out the messages.


Pete Duncanson 31 Jan 2017, 15:04:15

Would need some help from @madsrasmussen on this one.


Lars-Erik Aabech 15 Feb 2017, 13:32:12

Confirmed. The umbRequestHelper would've popped something if the status code is within [500,600]. When 400 it only rejects the promise. The ContentEditorController doesn't have a failed handler, so it just silently represses the failure. You'd think there'd be a nicer generic failure handler in umbRequestHelper checking whether there's any notifications on the object returned from the server. (There's full content w/notification info, just a "failure" HTTP status code) The initial intent was surely to show any notification on any response disregarding the status codes.


Lars-Erik Aabech 15 Feb 2017, 14:26:31

I made a PR to make umbRequestHelper show notifications for any 400, but I'm sure there'll be duplicate notifications elsewhere if it's just pulled as is. Can't be bothered to test everything, but at least the PR shows a pretty robust way of going about it. :)

https://github.com/umbraco/Umbraco-CMS/pull/1757


Lars-Erik Aabech 15 Feb 2017, 14:53:18

I had to create the Trashed event handler as well as the Deleted one, and yes, those pop the notification. So with the PR it makes two notifications. Better than none for unpublish, I say. And I still say centralize the logic for notification so nobody can forget.


Angus MacLean 27 Mar 2017, 13:43:07

PR v. useful but when using MediaService.Trashing, data.notifications is empty in the call to showFailureNotifications, event messages don't appear to be passed across.


Tom 09 Oct 2017, 08:59:56

I'm also having this issue in 7.7, any update on a fix for this? Cheers


Anders Bjerner 20 Dec 2017, 21:03:21

Any progress for this? My own attempt was to handle this specifically for unpublishing, but Lars-Erik's to solution seems better, as it doesn't just handle a specific case.

However in addition to the notifications, the save button could also indicate that the request failed - see more here:

https://github.com/umbraco/Umbraco-CMS/pull/2358


Tom Fulton 20 Mar 2018, 01:20:20

I ran into this on 7.9.2 and sent up [this pull request|https://github.com/umbraco/Umbraco-CMS/pull/2522], which tacks on to @abjerner code and forces the notifications to be shown.

I think @lars-erik solution would be great as well, but looks like it might require a bit of testing.


Sebastiaan Janssen 13 Apr 2018, 09:53:37

I think the solution as proposed by @tomnf is good, and I tested that it works. I'm not sure we need to do a complete overhaul as proposed by @lars-erik - I've tried with Trashing and Deleting events and they all show the warning just fine so it seems like all scenarios are covered.

I'll merge Tom's PR and close Lars-Erik's for now. Make sure to create new issues for any new items that are currently unhandled.


Lars-Erik Aabech 13 Apr 2018, 10:23:11

As long as V8 has a uniform way of handling errors. ;)


Sebastiaan Janssen 13 Apr 2018, 12:06:51

I am probably missing something @lars-erik - all scenario's seem to be handled at the moment. If there's any other concerns, make sure to open a new issue and re-open the PR :-)


Lars-Erik Aabech 13 Apr 2018, 12:35:44

@Sebastiaan IIRC I searched for notification usage and found that there were 20ish+ places that shouldn't be responsible for xhr error handling. It could benefit a lot from centralization.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.3.4, 7.7.8, 7.9.2

Due in version: 7.11.0

Sprint:

Story Points: 3

Cycle: