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

MemberSavedNotification fires twice when creating a new member #12673

Open skttl opened 2 years ago

skttl commented 2 years ago

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

10.0.1

Bug summary

When creating a new member, the MemberSavedNotifcation fires twice. When saving an existing member it's only once, as expected.

Specifics

No response

Steps to reproduce

I've tried adding the NotificationHandler from the docs to a blank Umbraco 10 site. Every time I create a member, the log message gets logged twice. When saving an existing member it only gets logged once.

image

image

Expected result / actual result

I would expect MemberSavedNotification to be fired only once when creating a member.

skttl commented 2 years ago

I've found out why...

When you save a new member, the MemberController runs a CreateMemberAsync method, which first creates the member and saves it using MemberUserStore.CreateAsync, and then adds properties and saves it again.

I would argue, that the Save call in MemberUserStore should set raiseEvents to false. the method is only used in two places (outside of tests). The backoffice MemberController and the UmbRegisterController used in the frontend for registering members. Both places are followed with another Save call.

nul800sebastiaan commented 2 years ago

But it's "just" two save events, I would argue that you definitely want to know about all save events that happen. I believe you should be able to check for HasIdentity or something like that in the first event and ignore it, as before saving it, it shouldn't have an identity yet. Does that work for you?

skttl commented 2 years ago

HasIdentity is true in both notifications, but no property values are set in the first.

I need the first Saved event where properties are set though...

nul800sebastiaan commented 2 years ago

Ah yes, Saved, not Saving 👍

I'll send it to the team (who are mostly all on holiday) for feedback. But in essence: we can't take away the first notification now, that would be a breaking change for people relying on it. This has been working like this in v7 and v8 as well by the way, so I imagine there must be some way to detect the notification that you want to use.

nul800sebastiaan commented 2 years ago

From the team: as detection you could look at the CreatorId which would be 0 in the first notification and not 0 during the second notification - https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs#L428

You could add a state in MemberSavingNotification indicating it's a new member and use that in MemberSavedNotification.

Maybe we should formalize that workaround though and add a property that indicates it's a brand new member and clear that after the first Saved notification has been emitted.

JasonElkin commented 2 years ago

If MemberSavingNotification is the same as v8 then it has a similar problem. IIRC it fires three times on creation of a new member and the member's state is subtly different each time.

The additional MemberSavingNotifications and MemberSavedNotifcations are distinct events, and the member state is different each time, so I'd expect dedicated notifications to be fired for each one.

We could have the best of both worlds then with no breaking changes or "workarounds". i.e. Keep the generic Saved\Saving notifications that fire for each save and add extra notifications e.g. MemberCreatedNotifcation, MemberChangedForSomeReasonNotification etc.

nul800sebastiaan commented 2 years ago

So I just found that this is actually documented just fine in https://our.umbraco.com/documentation/Reference/Notifications/MemberService-Notifications/ - which links to https://our.umbraco.com/documentation/Reference/Notifications/determining-new-entity

So no hacks, this is how you're expected to work with it.

I will ask the team (after they're back from their holidays) if adding more events is wanted, but I suspect the answer to be along the lines of: you can already do this with the existing events. 😅

skttl commented 2 years ago

My problem is, that when the member is new, I have no properties to work on, because they get added in the second round. I tried logging out the status upon creating a new member

image

As you can see, first time around (the one before adding properties) it is new as expected. Second time around (when properties gets added), it's no longer new. And this time I got the third notification @JasonElkin mentions too.

Creating/Created notifications would help out here :)

zirrocu commented 11 months ago

I'm on Umbraco 10.6.0 and I have a problem with this notifications: member key in first notification does not match member key in second notification for same entity

So in MemberSavedNotification you expect all properties to be as they are persisted in database, but it is not. The key is a fake one

PS was able to solve my problem with following snippet:


var dirty = (IRememberBeingDirty)member;
var isNew = dirty.WasPropertyDirty("Key");

Explanation: `isNew` sets to true only on 2nd notification when all properties are set and member is completely saved to storage
ccasalicchio commented 3 months ago

For Umbraco version 13:

        var dirty = (IRememberBeingDirty)notification.SavedEntities.ElementAt(0);
        var isNew = dirty.GetWereDirtyProperties().Contains("Id");