umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.4k stars 2.66k forks source link

Messages added through Content/Media Moving Notifications are not displayed in the backoffice #13922

Open wtct opened 1 year ago

wtct commented 1 year ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

9.5.4, 10.4.0, 11.2.0

Bug summary

Hi!

We have just noticed that messages added through any Content or Media Moving Notifications are not displayed in the backoffice.

Best!

cc: @piotrbach

Specifics

No response

Steps to reproduce

  1. Try to create notification handlers as below:
public class TestMovingNotificationHandlers : 
    INotificationHandler<ContentMovingNotification>, 
    INotificationHandler<ContentMovingToRecycleBinNotification>,
    INotificationHandler<MediaMovingNotification>,
    INotificationHandler<MediaMovingToRecycleBinNotification>
{
    public void Handle(ContentMovingNotification notification)
    {
        notification.Messages.Add(new EventMessage("Test", "Not displayed notification"));
    }

    public void Handle(ContentMovingToRecycleBinNotification notification)
    {
        notification.Messages.Add(new EventMessage("Test", "Not displayed notification"));
    }

    public void Handle(MediaMovingNotification notification)
    {
        notification.Messages.Add(new EventMessage("Test", "Not displayed notification"));
    }

    public void Handle(MediaMovingToRecycleBinNotification notification)
    {
        notification.Messages.Add(new EventMessage("Test", "Not displayed notification"));
    }
}
  1. Then register the created notification handlers in any composer as below:
public class TestComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.AddNotificationHandler<ContentMovingNotification, TestMovingNotificationHandlers>();
        builder.AddNotificationHandler<ContentMovingToRecycleBinNotification, TestMovingNotificationHandlers>();

        builder.AddNotificationHandler<MediaMovingNotification, TestMovingNotificationHandlers>();
        builder.AddNotificationHandler<MediaMovingToRecycleBinNotification, TestMovingNotificationHandlers>();
    }
}
  1. Finally, try to move or delete any node and notifications will not be displayed.

Expected result / actual result

Messages should be displayed in the same way as added e.g. in the ContentSavingNotification.

github-actions[bot] commented 1 year ago

Hi there @wtct!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

kjac commented 1 year ago

Hi @wtct 👋 I can reproduce this... and this is going to be a repeat comment from your other notification related issue 😄

While notifications execute reliably on the server, the messaging system to the clientside is somewhat shaky. There are a few other issues concerning notification messages - https://github.com/umbraco/Umbraco-CMS/issues/12636 for example.

We're currently not prioritising fixing the messaging system because we have our hands rather full with the headless features for V12 and the whole backoffice rewrite for V13. Obviously we'll have to make sure to not repeat the shaky implementation for V13.

That being said, if anyone wants to have a look at this, we'd be more than happy to see a fix for some or all of these cases. I'm putting this up for grabs 🙏

github-actions[bot] commented 1 year ago

Hi @wtct,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

miguelcrpinto commented 10 months ago

@kjac while it seems to be possible to fix issue 13923 without changing the return types of the PostMove methods from the ContentController and MediaController, in this case to make these messages work, we would have to change the return types.

I believe this would be considered a breaking change and therefore not an option, right?

kjac commented 10 months ago

@miguelcrpinto perhaps? Can you be a little more specific?

miguelcrpinto commented 10 months ago

@kjac currently both the PostMove method from the ContentController and from the MediaController return the moved node path as a string. In order to provide the FE with the notification messages (in case of successful move) we need to return an object that implements the INotificationModel instead of the string that is being returned now.

kjac commented 10 months ago

@miguelcrpinto well.. the other operations (PostCopy, PostPublishById, ...) do not return any notifications directly in the return value either. I wonder if custom notification messages are being shown for those operations? If not then things are less functional than they should be 😛 but if they are shown, then there is a less "output model bound" way to pass notifications to the client.

miguelcrpinto commented 10 months ago

@kjac I tried the PostCopy and the PostPublishByIdAndCulture (coud not find how/where to trigger the PostPublishById) and none of them returns the Notification messages. The PostSave does return the notifications in the response model (the response type implements the INotificationModel)

kjac commented 10 months ago

Ah ok @miguelcrpinto ... well, implementing INotificationModel on the response model should be fine. If it flags as a breaking change by the build, we'll just have to do something about that.