Allow to pass in boolean to noDirtyCheck directive #8638

bjarnef · 2020-08-17T11:53:46Z

Prerequisites

I have added steps to test this contribution in the description below

Description

At the moment we have some duplicated logic to be able to set no-dirty-check directive on e.g. <umb-checkbox> and <umb-radiobutton>.

I would however like just to pass in a boolean value whether to set this or not.

For example:

no-dirty-check without attribute value set $dirty to false.
no-dirty-check="true" with attribute value true works as before mentioned.
no-dirty-check="false" with attribute value true works as default behaviour.

The directive was originally created based on this https://stackoverflow.com/a/27430980 but note sure why we need the local variable with getter and setter. Furthermore why it change $pristine. It seems this was changed in #2476

I found some different variants setting $dirty to false or angular.noop:
https://stackoverflow.com/a/29225603

Test notes

Check that changes to input fields in content trigger the "Unsaved changes" dialog
Add a custom dashboard with use on no-dirty-check, no-dirty-check="true" and no-dirty-check="true" on <input> field works as expected. Also check with<umb-checkbox> and <umb-radiobutton> using disable-dirty-check property with true/false value.

<div>
    <label>Text 1</label>:<br>
    <input type="text" name="text1" placeholder="Enter value..." ng-model="text1" no-dirty-check />
</div>

<div>
    <label>Text 2</label>:<br>
    <input type="text" name="text2" placeholder="Enter value..." ng-model="text2" no-dirty-check="false" />
</div>

<div>
    <label>Text 3</label>:<br>
    <input type="text" name="text3" placeholder="Enter value..." ng-model="text3" no-dirty-check="true" />
</div>

<div>
    <label>Checkbox</label>:<br>
    <umb-checkbox model="check1" name="check1" disable-dirty-check="false">Test Check</umb-checkbox>
</div>

<div>
    <label>Radiobutton</label>:<br>
    <umb-radiobutton model="radio1" name="radio1" disable-dirty-check="true">Test Radio</umb-radiobutton>
</div>

MyDashboard.zip

… dirty check

bjarnef · 2020-08-17T12:40:13Z

@nul800sebastiaan @nielslyngsoe I believe this will clean up code a bit and we don't need some duplicated code.
Something similar could be done for prevent-enter-submit:
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/views/components/forms/umb-search-filter.html#L5-L26

It looks like we have something like this for preventEnterSubmit, but I can't see the enabled variable is used for anything - it simply always prevent enter submit when this directive is added 🙈
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/common/directives/components/forms/prevententersubmit.directive.js#L10-L17

bjarnef · 2020-08-17T12:47:33Z

However for preventDefault this seems to be used here:
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/common/directives/components/forms/preventdefault.directive.js#L33

bjarnef · 2020-08-17T13:59:51Z

I have submitted another PR to fix the issue with preventEnterSubmit: #8639

bjarnef · 2020-08-17T15:41:56Z

@kjac maybe you want to test this? 😎

nul800sebastiaan · 2020-08-31T18:07:42Z

This one is too intricate for me to evaluate, I will need some help from @kjac, @nathanwoulfe or if neither can help then @nielslyngsoe. Any takers? 🤞

kjac · 2020-08-31T18:22:41Z

Sure I'll give it a spin in the morning 👍

kjac · 2020-09-01T04:10:34Z

src/Umbraco.Web.UI.Client/src/common/directives/components/forms/umbcheckbox.directive.js

@@ -50,7 +50,7 @@

        function onInit() {
            vm.inputId = vm.inputId || "umb-check_" + String.CreateGuid();
-
+            vm.disableDirtyCheck = vm.disableDirtyCheck === true;


Is this necessary? And won't it break backwards compat with people using a truthly value that's not strictly true?

kjac · 2020-09-01T04:10:47Z

src/Umbraco.Web.UI.Client/src/common/directives/components/forms/umbradiobutton.directive.js

@@ -49,7 +49,7 @@

        function onInit() {
            vm.inputId = vm.inputId || "umb-radio_" + String.CreateGuid();
-
+            vm.disableDirtyCheck = vm.disableDirtyCheck === true;


Same as above: Is this necessary? And won't it break backwards compat with people using a truthly value that's not strictly true?

Not sure if we need to support both true/false and 'true'/'false' as string values. Here is just do the comparison in the controller so the view is a bit cleaner :)

While certainly cleaner it's also more stringent. My point is that the directive (presumably) supports truthly values today, so it should keep on doing that to preserve compatibility with current usages in 3rd party extensions etc.

We can add more checks here to make it work with other truthly values. E.g.

vm.disableDirtyCheck = vm.disableDirtyCheck === true || vm.disableDirtyCheck === 'true' || vm.disableDirtyCheck === 1;

I noticed we have this isBool function in color picker:
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/views/propertyeditors/colorpicker/colorpicker.controller.js#L142-L145

Maybe we should have a global function somewhere to check for truthly values?

Come to think of it we already do - there's an Object.toBoolean(value) method (defined in Extensions.js).

kjac · 2020-09-01T04:11:35Z

src/Umbraco.Web.UI.Client/src/common/directives/validation/nodirtycheck.directive.js

-            Object.defineProperty(ctrl, '$pristine', alwaysFalse);
-            Object.defineProperty(ctrl, '$dirty', alwaysFalse);
+            // If no attribute value is "false", then skip and use default behaviour.
+            var dirtyCheck = scope.$eval(attrs.noDirtyCheck) === false;


There must be a native JS function to evaluate a boolean instead of depending on AngularJS to do it for us?

Yes, probably. However I have noticed the $eval function is used several other places to parse attribute values.

With the above changes in mind, shouldn't this be var dirtyCheck = Object.toBoolean(attrs.noDirtyCheck) === false; ?

kjac · 2020-09-01T04:14:49Z

src/Umbraco.Web.UI.Client/src/common/directives/validation/nodirtycheck.directive.js


+            ctrl.$setDirty = Utilities.noop;


While I'm sure this works, I'm a tad worried it's too good to be true. The quite intricate solution with the alwaysFalse binding to $pristine and $dirty must be there for a reason... this is just a hunch, but I'm afraid your solution over simplifies things. Wouldn't it be prudent to keep the old solution here, just after the conditional dirty check handling?

Not sure if there is a specific reason it change $pristine. In other alternatives of no-dirty-check directives I have seen in only change $dirty. Regarding the "Unsaved changes" dialog it only rely on $dirty property as far as I know.

Yep I agree with you. As I tried to explain, it should work fine, but the current solution seems quite explicitly done as-is. There can be two reasons for this: Either the current solution is over complicated and implemented as such due to lack of knowledge/understanding, or else the current solution covers edge cases we can't right now. My point is it can't hurt to retain the current solution from a better-safe-than-sorry standpoint?

Yeah, not sure if $pristine has any effect on the "Unsaved changes" dialog though. The reason we have the no-dirty-check directive is to prevent the "Unsaved changes" dialog to show in certain circumstances.

It seems it only rely on $dirty before opening the overlay here:

Umbraco-CMS/src/Umbraco.Web.UI.Client/src/common/directives/validation/valformmanager.directive.js

Lines 175 to 177 in 0140d90

if (!formCtrl.$dirty && infiniteEditors.length === 0 || isSavingNewItem && infiniteEditors.length === 0) {

return;

}

kjac

@bjarnef I see where you're going with this. Code cleanup is always nice 👍 I have left a bunch of comments for you to have a look at.

bjarnef · 2020-09-01T10:05:26Z

@kjac it should now check for truthly value 👍

kjac · 2020-09-02T05:00:07Z

Thanks @bjarnef . I left another remark about the usage of scope.$eval() for you to consider, now that we discovered Object.toBoolean() 😄

Also would you please revert the dirty handling to the previous solution? If you feel very strongly for changing it, you can do so in a new PR, but please be absolutely certain that you know exactly why the previous solution is written in such a cumbersome way before attempting to change it.

bjarnef · 2020-09-02T09:29:49Z

@kjac I have reverted the dirty handling for now and changes to Object.toBoolean() 😊

Regarding the "Unsaved changes" I don't think the $pristine has any effect on this, since it only check if the form is $dirty.

Umbraco-CMS/src/Umbraco.Web.UI.Client/src/common/directives/validation/valformmanager.directive.js

Lines 175 to 177 in 0140d90

    
           if (!formCtrl.$dirty && infiniteEditors.length === 0 || isSavingNewItem && infiniteEditors.length === 0) { 
        
               return; 
        
           }

To me it seems it was just taken from one of the first results on Google, but no comment left, why it need to change $pristine.
https://stackoverflow.com/a/27430980

kjac · 2020-10-01T07:28:01Z

@bjarnef finally got around to giving this a test run. It seems your solution didn't take into account the default (undefined) values when the directives were applied without true/false. I have pushed a fix in 0a5b442 - have a look.

Here's a test dashboard you can use with all combinations of dirty checking:

<div ng-controller="MyDashboardController as vm">
    <div>
        <input type="text" name="text1" placeholder="no-dirty-check" ng-model="text1" no-dirty-check />
    </div>

    <div>
        <input type="text" name="text2" placeholder="no-dirty-check=&quot;false&quot;" ng-model="text2" no-dirty-check="false" />
    </div>

    <div>
        <input type="text" name="text3" placeholder="no-dirty-check=&quot;true&quot;" ng-model="text3" no-dirty-check="true" />
    </div>

    <div>
        <umb-checkbox model="check0" name="check0" disable-dirty-check>disable-dirty-check</umb-checkbox>
    </div>

    <div>
        <umb-checkbox model="check1" name="check1" disable-dirty-check="false">disable-dirty-check="false"</umb-checkbox>
    </div>

    <div>
        <umb-checkbox model="check2" name="check2" disable-dirty-check="true">disable-dirty-check="true"</umb-checkbox>
    </div>

    <div>
        <umb-radiobutton model="radio0" name="radio0" disable-dirty-check>disable-dirty-check</umb-radiobutton>
    </div>

    <div>
        <umb-radiobutton model="radio1" name="radio1" disable-dirty-check="false">disable-dirty-check="false"</umb-radiobutton>
    </div>

    <div>
        <umb-radiobutton model="radio2" name="radio2" disable-dirty-check="true">disable-dirty-check="true"</umb-radiobutton>
    </div>
</div>

bjarnef · 2020-10-01T09:14:20Z

@kjac strange Object.toBoolean() should already handle undefined and null values passed in right?
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/lib/umbraco/Extensions.js#L344

Thus returning false and the evaluating the condition would be false.

kjac · 2020-10-01T09:17:16Z

@bjarnef exactly; but "no value" is supposed to mean true in this case, as adding the directive without a value should enable it 😄

bjarnef · 2020-10-01T09:26:04Z

@kjac okay, then we might need a null check as well?

kjac · 2020-10-01T09:35:42Z

@bjarnef that was my thought too, and surely it can't hurt to add them, but try as I might I could only manage to pass undefined to the directives.

bjarnef · 2021-03-29T11:56:28Z

@nathanwoulfe I wonder if you may have an idea why I am starting to see this console error? I wonder if something has changed in Angular v1.8.0?

Umbraco-CMS/src/Umbraco.Web.UI.Client/package.json

Line 17 in 97ad14c

"angular": "1.8.0",

It would be an issue if disableDirtyCheck property wasn't optional, but it is :)

nathanwoulfe · 2021-03-29T20:31:54Z

Not sure, but it could be due to using attrs rather than scope to access the value - if the value is bound in the view, and has changes, perhaps Angular is aware that it has changed, but can't propagate the change to the directive since it's not a scoped input.

That's a guess, and a big one...

bjarnef · 2021-03-29T22:09:11Z

@nathanwoulfe it doesn't seem to be related to the use of $attrs since I get the console errors without the use of this change.

If I comment out the line where vm.disableDirtyCheck is assigned in controller I don't get the console errors, but of course the logic doesn't work as expected then.

…check

mikecp · 2021-03-30T08:18:52Z

Hello @bjarnef ,

Thanks for all the investigations!

I went and tested again and I think there were some remaining issues if we wanted values undefined, null, "", "something", to behave as no-dirty-check without value as you mentioned, because they should then produce a true result while the Object.ToBoolean produces a false result.

After scratching my head for a while, I kind of "started from scratch" for the construction of the conditions. From what I understood, the behaviour we want is something like "skip the no-dirty-check logic only if the attribute is not provided (in which case you don't get into the logic in fact :-)) or if its value is explicitly set to false, go for it in all other cases".

So I rewrote the conditions in the noDirtyCheck directive to match exactly that. The Object.ToBoolean does not fit for this since it actually does the opposite: anything is false except if it's explicitly set to true.

I also rewrote the conditions in the radio/checkbox controllers so that they provide the correct true/false values to the no-dirty-check attribute.

I made tests with all the possibilities you mentioned, and it behaves as expected: only when we do not set the attribute or when we explicitly set no-dirty-check="false" (or 0), we get the "Unsaved Changed" warning.

Let me know what you think 😁

I'll try to have a look tonight for the console errors

Cheers!

bjarnef · 2021-03-30T09:02:06Z

@mikecp yeah the Object.toBoolean() extension may not fit the needs here, so it is great explicit to check the value.

Regarding noDirtyCheck I think we want e.g. this no-dirty-check="something" to behave like no-dirty-check or no-dirty-check=""

So the value need to be "0" (int or string) or "false" (bool or string) to "disable" this (like if this attribute isn't added which make the form field dirty and trigger the "Unsaved changes" dialog.

Regarding disable-dirty-check in checkbox and radiobutton components, I think we want the opposite - so it explicit need to be "1" (Int or string) or "true" (bool or string) 👍

mikecp · 2021-04-01T00:06:19Z

Hi @bjarnef,

Thanks for your feedback!
I'm probably overseeing something, but I'm not sure why we would want a different behavior in the checkbox/radiobutton components. As I understand them, no-dirty-check and disable-dirty-check are two synonyms for saying "don't check for changes", so I guess I would expect them to behave in a similar way. But maybe there is a difference I did not catch?

bjarnef · 2021-04-01T00:29:17Z

I think it comes down to that no-dirty-check directive original didn't had a attribute value, so when added it would disable the form field dirty check.
And with any invalid values it should probably just fallback to default behaviour as without attribute value.

With disable-dirty-check component property the value should be true or false, but a random string value should probably just return false which is passed into no-dirty-check="false" in the component meaning the form field would be dirty, while no-dirty-check="true" and no-dirty-check is the same.

mikecp · 2021-04-02T01:21:02Z

Hey @bjarnef ,

I start to mixup what was said in the different messages😃 . So if I try to summarize, what we want is:

no-dirty-check
- not there => display unsaved changes warning
- there without value => no display
- there with value true, 1 or '1' => no display
- there with value false, 0 or '0' => display unsaved changes warning
- there with value null, undefined or any other invalid value => no display

Regarding disable-dirty-check, the more I look at it, the more it looks like just a pass-through to the "no-dirty-check" attribute of the form control beneath it. Son I am starting to think that we might just pass its value as-is to the no-dirty-check directive, and let that one interpret it following the rules above...

Since no-dirty-check now handles all possible values sent, is that not the easiest way to keep the logic homogeneous?

Cheers!

bjarnef · 2021-04-02T06:28:18Z

@mikecp correct about the no-dirty-check attribute 👍

Yeah, er han probably just pass the value through. Not that important as it from start was supposed to use true/false values and is a property, while the other is a directive. It might could to check what happens with no disable-dirty-check, empty value, null, undefined though 😎🐥

Could you give this a last spin, then I think we are pretty close and definitely cleanup the views.

bjarnef · 2021-06-30T10:46:53Z

@mikecp did you had a change to look at this again? would be great to close this and make the code a bit cleaner 😎

mikecp · 2021-07-01T01:45:06Z

Hey @bjarnef ,
Thanks for the reminder 😉I'll have a look back at this so we can finalize it!
Cheers!

bjarnef · 2021-08-04T10:25:13Z

@mikecp did you found some time to look at this again? Let me know if you need further info or I can help with anything 😎👍

bjarnef · 2021-08-21T07:32:56Z

@mikecp anything I can help with to get this closed/merged. I would really like to cleanup some of the redundant code 🙈🤓

mikecp · 2021-08-22T01:58:59Z

Hi @bjarnef ,
My apologies for my very slow reaction, it seems I needed a longer summer break than expected, but I'll definitely look at this and finalize this in the coming days 😅
Cheers!

Changes have been integrated + adapted further

mikecp · 2021-08-30T13:57:43Z

Hi @bjarnef ,

I finally found the time to make the last checks on this PR 🎉😂

Testing on undefined, null and empty values for disable-dirty-check gave indeed some last glitches but I made some last adjustments and now everything is working as it should, meaning that disable-dirty-check gives the same behavior as no-dirty-check for the same values/bound values.

So I can finally merge this, with all my apologies for this long delay 😅

Thanks again for this PR and for your patience 👍

bjarnef · 2021-08-30T14:06:39Z

Hi @mikecp

Thanks for testing it again and reviewing the code. Great to have this redundant code cleaned up and will be useful in future development easy to enable/disable the dirty check. 👍

bjarnef added 3 commits August 17, 2020 13:34

Adjust noDirtyCheck directive to accept a parameter to enable/disable…

dd5f65b

… dirty check

Update dirty property

22d87df

Use Utilities.noop and add description

db2f095

bjarnef marked this pull request as ready for review August 17, 2020 12:36

Set default value from controller

2514d2b

nul800sebastiaan added the state/needs-investigation This requires input from HQ or community to proceed label Aug 31, 2020

kjac reviewed Sep 1, 2020

View reviewed changes

kjac previously requested changes Sep 1, 2020

View reviewed changes

Check for truthly value

0ab8651

bjarnef added 2 commits September 2, 2020 11:17

Use Object.toBoolean

0bb2d18

Revert setting $pristine and $dirty for now

eb48dbd

Kenn Jacobsen added 2 commits September 16, 2020 12:03

Merge remote-tracking branch 'upstream/v8/contrib' into pr/8638

9a4867d

Handle default values in dirty check attributes

0a5b442

update test conditions for vamue of no-dirty-check and disable-dirty-…

94e8e39

…check

bjarnef closed this Apr 1, 2021

bjarnef reopened this Apr 1, 2021

Remove whitespace

6dbc4b4

mikecp added 2 commits August 28, 2021 11:37

Merge branch 'v8/contrib' into pr/8638

f5c04ee

update check

8807bf5

mikecp approved these changes Aug 30, 2021

View reviewed changes

mikecp merged commit 7757c4d into umbraco:v8/contrib Aug 30, 2021

mikecp added community/pr release/8.17.0 type/feature and removed state/needs-investigation This requires input from HQ or community to proceed labels Aug 30, 2021

bjarnef deleted the v8/feature/adjust-no-dirty-check branch August 30, 2021 14:04

	if (!formCtrl.$dirty && infiniteEditors.length === 0 \|\| isSavingNewItem && infiniteEditors.length === 0) {
	return;
	}

Allow to pass in boolean to noDirtyCheck directive #8638

Allow to pass in boolean to noDirtyCheck directive #8638

Conversation

bjarnef commented Aug 17, 2020 • edited Loading

Prerequisites

Description

Test notes

bjarnef commented Aug 17, 2020 • edited Loading

bjarnef commented Aug 17, 2020

bjarnef commented Aug 17, 2020

bjarnef commented Aug 17, 2020

nul800sebastiaan commented Aug 31, 2020

kjac commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjac left a comment

Choose a reason for hiding this comment

bjarnef commented Sep 1, 2020

kjac commented Sep 2, 2020

bjarnef commented Sep 2, 2020

kjac commented Oct 1, 2020

bjarnef commented Oct 1, 2020

kjac commented Oct 1, 2020

bjarnef commented Oct 1, 2020

kjac commented Oct 1, 2020

bjarnef commented Mar 29, 2021 • edited Loading

nathanwoulfe commented Mar 29, 2021

bjarnef commented Mar 29, 2021

mikecp commented Mar 30, 2021

bjarnef commented Mar 30, 2021 • edited Loading

mikecp commented Apr 1, 2021

bjarnef commented Apr 1, 2021 • edited Loading

mikecp commented Apr 2, 2021

bjarnef commented Apr 2, 2021 • edited Loading

bjarnef commented Jun 30, 2021

mikecp commented Jul 1, 2021

bjarnef commented Aug 4, 2021

bjarnef commented Aug 21, 2021

mikecp commented Aug 22, 2021

mikecp commented Aug 30, 2021

bjarnef commented Aug 30, 2021

bjarnef commented Aug 17, 2020 •

edited

Loading

bjarnef commented Aug 17, 2020 •

edited

Loading

bjarnef commented Mar 29, 2021 •

edited

Loading

bjarnef commented Mar 30, 2021 •

edited

Loading

bjarnef commented Apr 1, 2021 •

edited

Loading

bjarnef commented Apr 2, 2021 •

edited

Loading