Feature: clean model when a field is removed from view, such as when a condition is no longer satisfied. #371

jbsaff · 2015-04-28T18:22:12Z

This feature extends the schema-validate directive to allow users to configure an individual field or the entire form to one of a predefined set of behaviors when the $destroy event is broadcast. The common use case would be when a field is no longer visible due to a condition, which is evaluated by ng-if, no longer being met. Data that had previously been collected was remaining in the model, which would provide erroneous results on submission. By default, if no configuration is done, this feature would set the model value to undefined, just as if the field had never been visible in the form at all.

In order to properly handle complex nested arrays and objects, a new service (sfUnselect) was created. Based on the sfSelect service, sfUnselect will traverse a given path and attempt to set the endpoint to the provided value. If at any point an expected object or array is undefined, the service returns and does not create the missing object or array.

This addresses both #344 and #204

Also, in case it comes up in testing, this is related to #370, as the checkboxes will not work with this new feature until the schema-validate directive is correctly spelled.

Please pardon the commit log; I managed to fumble my git-fu roll when trying to create two pull requests.

davidlgj · 2015-04-29T20:05:07Z

src/directives/schema-validate.js

+
+
+        var DEFAULT_DESTROY_STRATEGY;
+        if (scope.options && scope.options.formDefaults) {


formDefaults should not be checked directly, it's already copied to the form object, see https://github.com/Textalk/angular-schema-form/blob/development/src/services/schema-form.js#L70

But in this case with null as a valid option distinct from undefined it might be better to move this to a global option. i.e. instead of checking scope.option.formDefaults.destroyStrategy just check scope.option.destroyStrategy

Looking at this again, I think I might have overlooked the implications of having the formDefaults copied to the form object. (It makes sense now, so I may have just had the wrong idea when I started trying to use it). It seemed like a convenient way to set the default for the whole form if someone wanted to use an alternative to undefined, but a standard global option accomplishes that. I'll make a change.

Awesome! :)

davidlgj · 2015-04-29T20:17:49Z

Hi @jbsaff and thank you for the PR! This is a much sought after feature and even though it changes current behavior I think that is a good call. It's probably what you want anyway and its the current behavior that surprises people. Nice work 👍

I made some comments, I think it's a good idea to add a global option for it instead of checking formDefaults explicitly. It was unfortunate that the change in indentation broke the diff :) was a bit hard to read!

jbsaff · 2015-04-29T20:32:09Z

Thanks for the feedback. I'm pretty comfortable in Java, but needed something client-side for my current project. I'll incorporate your feedback into another commit. Thanks for your work on ASF. It's going to save me a ton of time!

…destroyStrategy to be picked up directly from globalOptions, instead of looking in formDefaults. Properly handles for the difference between the destroyStrategy declared as undefined and completely undeclared, both at the globalOptions level and the form field definition level.

jbsaff · 2015-04-30T15:49:25Z

@davidlgj Fixes checked in, and I think I got my branch rebased to match Textalk/development.

Both the code and documentation should be updated to include the globalOption and form defined destroyStrategy. I had to rework my approach to account for the difference between having the destroyStrategy property declared as undefined and not having it declared at all.

Thanks!

davidlgj · 2015-04-30T19:49:46Z

👍 Awesome! I'm hoping to test and merge start of next week :)

One thing that struck me though, what happens if you remove the entire form? Like when navigating away in a single page app, or an ng-if on something around the form. Won't that trigger the $destroy?Will that then remove all the attributes from the model?

jbsaff · 2015-04-30T21:42:13Z

Well crud. Yes, if there is an ng-if wrapping the form, the entire form would be subject to that $destroy. I'll look for another solution.

jbsaff · 2015-05-01T13:22:42Z

Okay, I've given it a thought. There are actually two parts to the logic that ought to trigger data cleanup. One is removal from the DOM, which can be identified by the $destroy. The other is that the condition on the form element is not satisfied (which might trigger a $destroy due to how conditions are wired in with ng-if). I'm going to head in that direction and see how it goes.

davidlgj · 2015-05-01T14:16:35Z

I was thinking that there might be a solution if the $destroy is called on
the first on sf-schema $scope then we might be able to set a flag that
stops the destruction of the fields.

fre 1 maj 2015 15:22 James Saffell [email protected] skrev:

Okay, I've given it a thought. There are actually two parts to the logic
that ought to trigger data cleanup. One is removal from the DOM, which can
be identified by the $destroy. The other is that the condition on the form
element is not satisfied (which might trigger a $destroy due to how
conditions are wired in with ng-if). I'm going to head in that direction
and see how it goes.

—
Reply to this email directly or view it on GitHub
#371 (comment)
.

Changed some titles.

…a form field element triggers the $destroy. Uses a new service, based on Select, to traverse the model and update it to the value chosen as part of the configured destroyStrategy. This destroyStrategy can be configured at the field, or as part of the forms global options. If both are defined, the field-level strategy will override.

…destroyStrategy to be picked up directly from globalOptions, instead of looking in formDefaults. Properly handles for the difference between the destroyStrategy declared as undefined and completely undeclared, both at the globalOptions level and the form field definition level.

… prevents the form from wiping itself when a $destroy event occurs that is not connected to the condition logic (such as if the FormController contains an element that wraps the form with an ng-if).

…rategy' into feature/cleanModelUsingDestroyStrategy Conflicts: README.md src/directives/schema-validate.js

jbsaff · 2015-05-01T21:20:23Z

Trying to understand the implications of the Angular docs for the $destroy event. It says that the listener is called just prior to the actual destruction occurring. Does that mean that it happens after all of the children have finished processing the $destroy in a depth-first fashion? If so, the $on $destroy for sfSchema would be too late to set the flag, as all of the children have already gone. I'm going to try to improve my understanding of the situation and determine what the event firing order is.

… to ignore the destroyStrategy when the $destroy event occurs on sfSchema. This will prevent the data from being cleaned up when the form is removed from the DOM.

jbsaff · 2015-05-04T20:52:43Z

I've put together an initial implementation of a flag to retain the model instead of allowing the destroyStrategy to be used when the sfSchema element is destroyed.

davidlgj · 2015-05-07T19:46:45Z

I'll merge it locally and test it out!

davidlgj · 2015-05-07T20:55:21Z

src/directives/schema-validate.js

+        scope.$on('$destroy', function() {
+
+          var form = getForm();
+          var conditionResult = $parse(form.condition);


I was looking through the code and testing some, and I'm not quite sure why we need to check on the condition here? Shouldn't we always just follow the destroy strategy if we get a destroy event? I'm thinking that condition might not be the only thing that can remove a form element from the form, any other add on might use a ng-if somewhere and since any field not in the DOM won't get any validation done on it anyways It's probably best to remove it from the model.

I had left it in as a way to help show where I was coming from with my solution, because I wasn't 100% confident that the service I put in would be correct (I'd still label myself an apprentice with Javascript). Assuming that the flag will handle for the case where the destroy is coming from outside of the form, then I agree that destroys initiated within the form should just follow the destroyStrategy. It'll be more consistent (and still configurable) and easier to explain.

davidlgj · 2015-05-07T21:00:57Z

Ok, so I didn't get through i tonight, I'll try to pick it up again soon. Keeping the flag in a service won't work since that service is shared across all instances of schema form you might have on the page, but I think I got a solution for that :) ...just got to try it out first!

jbsaff · 2015-05-07T21:12:38Z

Okay, thanks for the assist. I appreciate the help polishing it up!

davidlgj · 2015-05-08T10:24:26Z

@jbsaff I've started to merge it in, but I've also changed some things. I really hope you don't mind. You've done some solid work and I couldn't have got this done without your PR to base it on!

Initial manual testing looks good, I pushed the branch so you can see it here if you feel like checking it out: https://github.com/Textalk/angular-schema-form/tree/jbsaff-feature/cleanModelUsingDestroyStrategy

It needs some cleaning and some tests but I think it works :)

Major points of what I have changed:

I changed the option defaultStrategy value to always take a string, so its "null", "empty" (was ''), "remove" (was undfined) and "retain". This greatly simplified the logic when trying to figure out if you should inherit a global option or not.
I changed the use of a service to flag for "external destruction", instead I put it as a variable on scope. This means several instances of schema form won't clash with each other.

jbsaff · 2015-05-08T12:44:46Z

@davidlgj I'm glad to hear my work was a useful starting point. I try not to take any ego into a project; if my stuff is good, it will stay. The changes you described sound like they will make the feature far more useable within ASF, so I'm very much in favor. Thanks for the reviews and suggestions. It made my first contribution to a project a lot of fun.

jbsaff · 2015-05-08T21:41:35Z

Only thing I noticed is that when an array element (like in a tabarray) is removed, the console fills up with "TypeError: Cannot convert undefined or null to object". If I set the global option to null, I get "TypeError: Cannot set property '[object Array]' of undefined" instead. It looks like the Remove cleans up the model (as expected), then the new feature tries to do the same thing and gets an error. Do we expect that, in a case where sfSelect would return undefined, we would still want to attempt to set the value using the destroyStrategy?

davidlgj · 2015-05-10T07:08:56Z

Of course! We might not have a model if the user hasn't filled out the entire form :) Added a check!
This seems to work for me, but in certain scenarios, like when having nested objects we can end up being left with empty object {}, since there is no field with a schema-validate directive at that level... I'm gonna try and move it to the decorator directive instead.

btw I've been testing with this schema and form definition: https://gist.github.com/02ed18af7570ef149c92

davidlgj · 2015-05-10T17:52:38Z

Ok, moved it around a bit and added some tests. I tried doing removal of empty objects, but that is a bit tricky. Could easily end up mucking up arrays by removing the empty object needed for it to display an empty form. I'm not convinced it's actually needed, so I'm gonna let it be as it is. I think its ready enough for merging into the development branch :) Thanks for the PR and all the help!

Anthropic · 2015-05-11T00:40:35Z

Nice work @jbsaff & @davidlgj look forward to using it and replacing my own workarounds :)

jbsaff · 2015-05-11T13:10:34Z

@davidlgj Having the behavior in the decorator directive looks like it works for the case where the destroy is triggered by something outside the form. However, when a form field destroy is triggered by a condition no longer being satisfied within that form, the sf-decorator tag remains in the DOM and the cleanup of the model isn't triggered.

Would it make sense to split the behavior off into a directive that can be applied to the outermost div in each decorator template? It would require add-ons/extensions to opt-in to using the new directive, which could be a drawback. I like the way that moving the event handling up from the schema-validate level means that it will only fire once per field, instead of once for every input (long checkbox/radio lists, commonly).

davidlgj · 2015-05-11T19:39:30Z

@jbsaff You're absolutely right! A separate directive is probably the cleanest, I'll try to re-work soon.

This was referenced Apr 28, 2015

Should conditional type evaluation destroy data? #204

Closed

Config option to wipe the model when a condition is no longer fulfilled? #344

Closed

davidlgj reviewed Apr 29, 2015
View reviewed changes

Dervisevic and others added 10 commits May 1, 2015 20:05

Update README.md

3ab26b4

Changed some titles.

Update README.md

e5c6183

Additional doc example.

896ec3e

Part of another branch pull request.

e73bfe2

Removed extra console logging.

0d6f417

Only clean model if the form's condition is no longer satisfied. This…

4568337

… prevents the form from wiping itself when a $destroy event occurs that is not connected to the condition logic (such as if the FormController contains an element that wraps the form with an ng-if).

Merge remote-tracking branch 'origin/feature/cleanModelUsingDestroySt…

4d72f3d

…rategy' into feature/cleanModelUsingDestroyStrategy Conflicts: README.md src/directives/schema-validate.js

Remove extraneous console log.

d20b273

sfSchema can communicate via sfRetainModel service to force the model…

dcff62b

… to ignore the destroyStrategy when the $destroy event occurs on sfSchema. This will prevent the data from being cleaned up when the form is removed from the DOM.

davidlgj reviewed May 7, 2015
View reviewed changes

davidlgj merged commit dcff62b into json-schema-form:development May 10, 2015

ymarcon mentioned this pull request Jun 9, 2015

Model does not match condition #425

Closed

jbsaff mentioned this pull request Aug 17, 2015

destroyStrategy isn't followed when a field is removed from DOM. #508

Closed

jbsaff mentioned this pull request Aug 29, 2015

destroyStrategy works only on siblings that are array of objects. It does not work on other siblings. #529

Closed

Gbiggert mentioned this pull request May 18, 2016

DestroyStrategy does not trigger when field no longer meets condition #689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: clean model when a field is removed from view, such as when a condition is no longer satisfied. #371

Feature: clean model when a field is removed from view, such as when a condition is no longer satisfied. #371

jbsaff commented Apr 28, 2015

davidlgj Apr 29, 2015

jbsaff Apr 29, 2015

davidlgj Apr 29, 2015

davidlgj commented Apr 29, 2015

jbsaff commented Apr 29, 2015

jbsaff commented Apr 30, 2015

davidlgj commented Apr 30, 2015

jbsaff commented Apr 30, 2015

jbsaff commented May 1, 2015

davidlgj commented May 1, 2015

jbsaff commented May 1, 2015

jbsaff commented May 4, 2015

davidlgj commented May 7, 2015

davidlgj May 7, 2015

jbsaff May 7, 2015

davidlgj commented May 7, 2015

jbsaff commented May 7, 2015

davidlgj commented May 8, 2015

jbsaff commented May 8, 2015

jbsaff commented May 8, 2015

davidlgj commented May 10, 2015

davidlgj commented May 10, 2015

Anthropic commented May 11, 2015

jbsaff commented May 11, 2015

davidlgj commented May 11, 2015



		var DEFAULT_DESTROY_STRATEGY;
		if (scope.options && scope.options.formDefaults) {

Feature: clean model when a field is removed from view, such as when a condition is no longer satisfied. #371

Feature: clean model when a field is removed from view, such as when a condition is no longer satisfied. #371

Conversation

jbsaff commented Apr 28, 2015

davidlgj Apr 29, 2015

Choose a reason for hiding this comment

jbsaff Apr 29, 2015

Choose a reason for hiding this comment

davidlgj Apr 29, 2015

Choose a reason for hiding this comment

davidlgj commented Apr 29, 2015

jbsaff commented Apr 29, 2015

jbsaff commented Apr 30, 2015

davidlgj commented Apr 30, 2015

jbsaff commented Apr 30, 2015

jbsaff commented May 1, 2015

davidlgj commented May 1, 2015

jbsaff commented May 1, 2015

jbsaff commented May 4, 2015

davidlgj commented May 7, 2015

davidlgj May 7, 2015

Choose a reason for hiding this comment

jbsaff May 7, 2015

Choose a reason for hiding this comment

davidlgj commented May 7, 2015

jbsaff commented May 7, 2015

davidlgj commented May 8, 2015

jbsaff commented May 8, 2015

jbsaff commented May 8, 2015

davidlgj commented May 10, 2015

davidlgj commented May 10, 2015

Anthropic commented May 11, 2015

jbsaff commented May 11, 2015

davidlgj commented May 11, 2015