Umbraco-CMS icon indicating copy to clipboard operation
Umbraco-CMS copied to clipboard

Property is always dirty when empty

Open bjarnef opened this issue 3 years ago • 5 comments

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

8.13.1

Bug summary

In ContentService.Saving event I have some logic to check if specific properties are empty.

var dirty = (ICanBeDirty)entity;
var test1 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.ExternalName).Alias); // Textbox
var test2 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.Measures).Alias); // Contentment DataList picker
var test3 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.Location).Alias);  // Contentment DataList picker
var test4 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.Prepaid).Alias); // Decimal

The first three properties have values and return false - however the 4th property (decimal) returns true first time (empty), but also true second time even though no change has been made.

However if I set a value, e.g. 1000 it returns true on first save (expected) and false second, because no change has been made.

Specifics

No response

Steps to reproduce

Add a few property to a document type. Hook into ContentService.Saving event and watch dirty state of each of these properties, e.g. textstring and decimal properties.

Leave fields empty and save document type. First time it will returns true, but also returns true on second save although no changes has been made.

Set a value in the properties. First time it will returns true, but returns false on second save, which is expected.

using Newtonsoft.Json;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Umbraco.Core;
using Umbraco.Core.Composing;
using Umbraco.Core.Events;
using Umbraco.Core.Logging;
using Umbraco.Core.Models;
using Umbraco.Core.Models.Entities;
using Umbraco.Core.Services;
using Umbraco.Core.Services.Implement;
using ContentModels = Umbraco.Web.PublishedModels;

namespace MyProject.Core.Composing
{
    [RuntimeLevel(MinLevel = RuntimeLevel.Run)]
    public class SubscribeToSavingEventComposer : ComponentComposer<SubscribeToSavingEventComponent>
    { }

    public class SubscribeToSavingEventComponent : IComponent
    {
        private readonly ILogger _logger;

        public SubscribeToSavingEventComponent(
            ILogger logger)
        {
            _logger = logger;
        }

        public void Initialize()
        {
            ContentService.Saving += ContentService_Saving;
        }

        public void Terminate()
        {
            //unsubscribe during shutdown
            ContentService.Saving -= ContentService_Saving;
        }

        private void ContentService_Saving(IContentService sender, ContentSavingEventArgs e)
        {
            foreach (IContent entity in e.SavedEntities)
            {
                if (entity.ContentType.Alias == ContentModels.Brand.ModelTypeAlias)
                {
                    bool isNew = entity.HasIdentity == false;

                    if (isNew)
                    {
                        // Do something
                    }
                    else
                    {
                        var dirty = (ICanBeDirty)entity;

                        var test1 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.Phone).Alias);
                        var test2 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.ExternalName).Alias);
                        var test3 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.Measures).Alias);
                        var test4 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.Location).Alias);
                        var test5 = dirty.IsPropertyDirty(ContentModels.Brand.GetModelPropertyType(x => x.Prepaid).Alias);

                        // This always returns true if any of the properties above are empty.
                        bool isDirty = test1 || test2 || test3 || test4 || test5;

                        // No change then skip
                        if (isDirty == false)
                            return;

                        // Do something
                    }
                }
            }
        }
    }
}

Expected result / actual result

I would expect IsPropertyDirty() only the return true first time or if value has changed.


This item has been added to our backlog AB#35177

bjarnef avatar Nov 23 '21 11:11 bjarnef

Hi @bjarnef ,

The operation of diry properties does not work properly at all. Sometimes properties that a user can't even access (removed via EditorModelEventManager.SendingContentModel) are marked as dirty while the user can't edit these (in our case a checkbox).

inetzo avatar Dec 01 '21 12:12 inetzo

hey @bjarnef, sorry for the late reply

Leave fields empty and save document type. First time it will returns true, but also returns true on second save although no changes has been made.

Do you mean save the document? Or is the issue on saving the document type?

Migaroez avatar Nov 20 '23 11:11 Migaroez

@Migaroez yes, document (not document type) 😊

bjarnef avatar Nov 20 '23 11:11 bjarnef

  • [x] verified on v10 with the following code
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Notifications;

namespace Umbraco.Cms.Web.UI.Code;

public class Composer11683 : IComposer
{
    public void Compose(IUmbracoBuilder builder) =>
        builder.AddNotificationHandler<ContentSavingNotification, SavingNotificationHandler11683>();
}

public class SavingNotificationHandler11683 : INotificationHandler<ContentSavingNotification>
{
    private readonly ILogger _logger;

    public SavingNotificationHandler11683(ILogger<Composer11683> logger)
    {
        _logger = logger;
    }

    public void Handle(ContentSavingNotification notification)
    {
        foreach (IContent entity in notification.SavedEntities)
        {
            _logger.LogInformation("Checking dirty properties for {entityId}", entity.Id);
            foreach (var property in entity.Properties)
            {
                if (entity.IsPropertyDirty(property.Alias))
                {
                    _logger.LogInformation("[{entityId}] dirty property detected: {propertyAlias} - {values}",
                        entity.Id, property.Alias, property.Values);
                }
            }
        }
    }
}

This seems to have todo with how we construct the properties when the document is new vs when it comes from the database and the use of Property.GetPValue in combination with Property.SetValue where the GetPValue create parameter is always true

Migaroez avatar Nov 20 '23 12:11 Migaroez

This also affects MemberSavingNotification and MemberSavedNotification in Umbraco 10.8.2.

creativesuspects avatar Feb 09 '24 18:02 creativesuspects