fix(forms): Don't override form group's dirty state when disabling controls #24591

martinsik · 2018-06-20T15:26:10Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

form = this.fb.group({ a: ['a'], b: ['b'] });
form.markAsDirty();
console.log(form.dirty); // true

form.controls['a'].disable();
console.log(form.dirty); // false

I can manually change dirty state of a FormGroup but when I disable a single control it changes dirty state of its parent.

Demo: https://stackblitz.com/edit/angular-8bab4v?file=src%2Fapp%2Fapp.component.ts

Issue Number: #23309

What is the new behavior?

When calling disable() on AbstractControl it now checks whether its parent is dirty and if its dirty state corresponds to the current state of its children. If not it means I had to change it myself with markAsDirty so I won't override parent's dirty state.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

This could be a BC but it's probably very unlikely that anybody was relying on this behavior since its counter-intuitive.

kara

Thanks for the bug fix! Some comments below.

packages/forms/src/model.ts

kara · 2018-12-19T00:05:09Z

packages/forms/src/model.ts

@@ -491,10 +494,13 @@ export abstract class AbstractControl {
    this._onDisabledChange.forEach((changeFn) => changeFn(false));


Seems like enable() needs to be updated with the check as well. Otherwise, you'll have the same problem of resetting dirtiness when you re-enable the control.

kara · 2018-12-19T00:05:42Z

packages/forms/test/form_control_spec.ts

+        expect(c.dirty).toBe(false);
+
+        c.disable();
+        expect(a.dirty).toBe(true);


Can we add the following to this test (as well as the other one)?

c.enable(); expect(a.dirty).toBe(true);

I think this assertion would fail as the code is currently written.

googlebot · 2019-01-09T09:41:12Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

martinsik · 2019-01-09T14:10:17Z

@kara I finally managed to run Bazel so I'll have a look at this ASAP. I've actually completely forgotten what I did here because it's been 6 months now so I'll need to recall what's going on in this PR.

kara · 2019-01-10T23:21:27Z

@martinsik No worries, sorry for the initial delay. Ping me whenever it's ready and I'll have better turnaround time this time :-)

… and then called enable() Closes angular#24591

martinsik · 2019-01-11T12:08:36Z

@kara I updated this PR. You were right, it really didn't work after calling enable() on a form control but it should be fixed now. I also added the test case you suggested.

I think I messed the history in this PR when I unintentionally referenced this PR id instead of the issue id but it's showing just 3 commits here https://github.com/angular/angular/pull/24591/commits which is correct.

martinsik · 2019-01-11T12:09:48Z

I don't know why CLA is failing. Maybe because I merged one suggestion as a comit here on GitHub.

googlebot · 2019-01-17T23:29:15Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

kara

One small nit, then should be good to go.

Can you squash your commits together into one? I think the commit in the middle is causing our linter job to fail.

packages/forms/src/model.ts

…ntrols Update packages/forms/src/model.ts Co-Authored-By: martinsik <martin.sikora.ahoj@gmail.com>

…ling controls

googlebot · 2019-01-25T00:20:29Z

CLAs look good, thanks!

kara · 2019-01-25T00:21:29Z

@martinsik FYI, I squashed the commits and fixed the nit to move the PR along. If Google internal tests pass, this should be good to go.

kara

LGTM

kara · 2019-01-25T00:25:35Z

presubmit

…ntrols (angular#24591) Update packages/forms/src/model.ts Co-Authored-By: martinsik <martin.sikora.ahoj@gmail.com> PR Close angular#24591

angular-automatic-lock-bot · 2019-09-14T15:21:35Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jun 20, 2018

mhevery requested a review from kara July 12, 2018 22:53

mhevery added the area: forms label Jul 12, 2018

kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 31, 2018

kara added the type: bug/fix label Sep 18, 2018

kara requested changes Dec 19, 2018

View reviewed changes

kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 19, 2018

ngbot bot added this to the needsTriage milestone Dec 19, 2018

kara assigned martinsik Dec 19, 2018

martinsik requested a review from a team as a code owner January 9, 2019 09:41

googlebot added cla: no and removed cla: yes labels Jan 9, 2019

martinsik added a commit to martinsik/angular that referenced this pull request Jan 11, 2019

fix(forms): Don't check parent's dirtiness when manually marked dirty…

694cd26

… and then called enable() Closes angular#24591

martinsik force-pushed the issue-23309 branch from 94823e0 to 0437853 Compare January 11, 2019 11:53

martinsik added a commit to martinsik/angular that referenced this pull request Jan 11, 2019

fix(forms): Don't check parent's dirtiness when manually marked dirty…

0437853

… and then called enable() Closes angular#24591

martinsik force-pushed the issue-23309 branch 2 times, most recently from 588243c to 7ffb45a Compare January 11, 2019 12:07

kara added cla: yes and removed cla: no labels Jan 17, 2019

kara requested changes Jan 17, 2019

View reviewed changes

packages/forms/src/model.ts Outdated Show resolved Hide resolved

martinsik and others added 2 commits January 24, 2019 16:06

fix(forms): don't override form group's dirty state when disabling co…

95ec928

…ntrols Update packages/forms/src/model.ts Co-Authored-By: martinsik <martin.sikora.ahoj@gmail.com>

fixup! fix(forms): don't override form group's dirty state when disab…

21c4999

…ling controls

kara force-pushed the issue-23309 branch from 7ffb45a to 21c4999 Compare January 25, 2019 00:20

kara approved these changes Jan 25, 2019

View reviewed changes

kara added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 25, 2019

kara unassigned martinsik Jan 25, 2019

jasonaden closed this in ef67282 Jan 25, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(forms): Don't override form group's dirty state when disabling controls #24591

fix(forms): Don't override form group's dirty state when disabling controls #24591

martinsik commented Jun 20, 2018

kara left a comment

kara Dec 19, 2018

kara Dec 19, 2018

googlebot commented Jan 9, 2019

martinsik commented Jan 9, 2019

kara commented Jan 10, 2019

martinsik commented Jan 11, 2019

martinsik commented Jan 11, 2019

googlebot commented Jan 17, 2019

kara left a comment

googlebot commented Jan 25, 2019

kara commented Jan 25, 2019

kara left a comment

kara commented Jan 25, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019

		@@ -491,10 +494,13 @@ export abstract class AbstractControl {
		this._onDisabledChange.forEach((changeFn) => changeFn(false));

fix(forms): Don't override form group's dirty state when disabling controls #24591

fix(forms): Don't override form group's dirty state when disabling controls #24591

Conversation

martinsik commented Jun 20, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

kara left a comment

Choose a reason for hiding this comment

kara Dec 19, 2018

Choose a reason for hiding this comment

kara Dec 19, 2018

Choose a reason for hiding this comment

googlebot commented Jan 9, 2019

martinsik commented Jan 9, 2019

kara commented Jan 10, 2019

martinsik commented Jan 11, 2019

martinsik commented Jan 11, 2019

googlebot commented Jan 17, 2019

kara left a comment

Choose a reason for hiding this comment

googlebot commented Jan 25, 2019

kara commented Jan 25, 2019

kara left a comment

Choose a reason for hiding this comment

kara commented Jan 25, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019