V13: Introduce publishNotifications method on IMembershipMemberService

[Zeegaan]

Fixes #12673

Notes

• MemberSavedNotification would fire twice when creating a new member, why is explained in the original issue here: MemberSavedNotification fires twice when creating a new member #12673 (comment)
• This PR remedies that by introducing a publishNotifications in the Save method, so you can pass in whether or not you'd like to fire your notifications when using the Save.
• Note that ´scope.Notifications.Supress()´ does not work in this scenario, as it would suppress all the notifications, even from the parent scope..

How to test

• Follow steps in original issue, here is a notification handler you can use:

using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Notifications;

public class MyNotificationHandler : INotificationAsyncHandler<MemberSavedNotification>
{
    public async Task HandleAsync(MemberSavedNotification notification, CancellationToken cancellationToken)
    {
        IMember? member = notification.SavedEntities.FirstOrDefault();
        if (member is null)
        {
            return;
        }
    }
}


public class MyComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder) => builder.AddNotificationAsyncHandler<MemberSavedNotification, MyNotificationHandler>();
}

[AndyButland]

I haven't tested this out yet, but can see that this should solve the problem of firing multiple member save notifications. However, does it help with the original issue of wanting to respond to created members?

In other words, in the remaining notification that will include all the member properties, is there a way to tell that this is a newly created member? I'm concerned it'll look the same as an update (as, under the hood, that's what it is).

[Zeegaan]

@AndyButland Good point, I will look into it

[Zeegaan]

Looking further into it, you can indeed check on member.CreatedDate & member.UpdateDate, to distinguish a create / update, as these values will be the same on create, but will be different on update 🙌

[AndyButland]

Nice, yes, that works - I've verified the changes and confirmed that you can meet the requirement with this notification handler:

using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Notifications;

namespace Umbraco.Cms.Web.UI.Custom;

public class MemberSavedNotificationHandler : INotificationHandler<MemberSavedNotification>
{
    private readonly ILogger<MemberSavedNotificationHandler> _logger;

    public MemberSavedNotificationHandler(ILogger<MemberSavedNotificationHandler> logger) => _logger = logger;

    public void Handle(MemberSavedNotification notification)
    {
        foreach (IMember member in notification.SavedEntities)
        {
            var isNew = member.CreateDate == member.UpdateDate;
            _logger.LogInformation("Member {MemberId} {MemberName} was {Action}.", member.Id, member.Name, isNew ? "created" : "updated");
        }
    }
}

From that I see the expected single "created" action, and then if I re-save the member I see an "update".

I've left a comment about the method signature though... not sure what's best but wanted you to have a think about it before merging in.

[AndyButland]

Wondering if the boolean parameter is quite right here. Reason is that even if we set publishNotifications to false, we still publish the "Saving" notification. I know why we do and we should do in this scenario - so the operation can be cancelled - but it's a bit confusing as the method signature would suggest we aren't publishing any notifications.

So perhaps it would be clearer if this was an enum? Could be:

enum PublishNotificationSaveOptions
{
    None,
    Saving,
    Saved,
    All
}

Or a "Flags" enum that handles all CUD operations... something like:

[Flags]
enum PublishNotificationOptions
{
    None = 0,
    Saving = 1,
    Saved = 2,
    Deleting = 4,
    Deleted = 8,
    AllSaved = Saving | Saved,
    AllDeleted= Deleting | Deleted,
    All = AllSaved | AllDeleted
}

Or maybe you could combine all the "ing" and "eds", something like:

enum PublishNotificationOptions
{
    None,
    PreOperation,
    PostOperation,
    All
}

With one of these you can then be explicit about which notification you are publishing and which you are suppressing. In all cases the default would be the appropriate "All" option.

[Zeegaan]

Hmm, I think the first option is best:

enum PublishNotificationSaveOptions
{
    None,
    Saving,
    Saved,
    All
}

The only thing I'm scared about, is someone thinking this will all of a sudden go in all services (I mean it could technically, but I don't see the use case). 🤔

[AndyButland]

Agree with the first option looking back over it. The others seem a bit "forced".

Also fair point about leading to an expectation on other services, but you could argue that also just for the bool parameter.

[Zeegaan]

@AndyButland Took a stab at implementing the Enum, please have a look 😁

[AndyButland]

Looks a bit clearer with the enum I think, at least more explicit for what the calling code can expect in terms of notifications to be fired.

I left a few inline comments, mostly minor, the main one being this question about whether you should be able to cancel the create of a member via the first .Saving notification.

[AndyButland]

Couple of last suggestions but looks good. I'll pull it down for one last test again this afternoon.

[AndyButland]

All looks good to me - code wise and have tested as I did earlier, and can confirm I see just one notification on create with the information available to be able to distinguish between new and updated members.