Skip to content

fix(edit-content): keep lock toggle out of FormGroup so locked content opens pristine#35772

Draft
oidacra wants to merge 1 commit into
mainfrom
issue-35754-v2
Draft

fix(edit-content): keep lock toggle out of FormGroup so locked content opens pristine#35772
oidacra wants to merge 1 commit into
mainfrom
issue-35754-v2

Conversation

@oidacra
Copy link
Copy Markdown
Member

@oidacra oidacra commented May 20, 2026

Summary

Fixes a UX bug where opening a locked content item in the new Edit Content experience marked the form as dirty without any user interaction, triggering a spurious "You have unsaved changes" dialog when navigating away.

Root cause. The lock <p-toggleSwitch [ngModel]="isContentLocked"> lives in <ng-template #appendContent>, which dotTabViewInsert injects into the DOM inside <form [formGroup]="form">. Without [ngModelOptions]="{ standalone: true }", Angular registers the template-driven ngModel as an implicit control on the reactive FormGroup. isContentLocked is a store signal that resolves asynchronously — when it writes through [ngModel] after #scheduleMarkPristineAfterInit already fired, the toggle's CVA onChange re-dirties the form. No subsequent pristine reset happens.

Fix. One-line template change: add [ngModelOptions]="{ standalone: true }" to the lock toggle so it is excluded from the reactive form. The one-way [ngModel] binding and the (onChange)="onContentLockChange($event)" handler are unchanged — the toggle's visual state and lock/unlock service calls still work exactly as before.

Closes #35754

Acceptance Criteria

  • AC1 — Locked content: form is pristine on open (FormGroup.dirty === false, pristine === true).
  • AC2 — After the async lock-status check resolves, FormGroup.dirty remains false.
  • AC3 — Opening locked content + navigating away (no edits) does NOT trigger the unsaved-changes dialog. (Covered by proxy: the guard reads form.dirty; AC1/AC2 prove form.dirty === false.)
  • AC4 — Opening unlocked content + no edits + navigating away does NOT trigger the dialog. (Covered by proxy via the same chain.)
  • AC5 — Opening unlocked content + editing a field → form is dirty.
  • AC6 — Locked content unlocked + edited → form becomes dirty.
  • AC7 — Unit test: open locked content → component.form.dirty === false.
  • AC8 — Unit test: open unlocked + edit a field → component.form.dirty === true.

Test plan

  • yarn nx lint edit-content — 0 errors (9 pre-existing warnings unrelated to this change)
  • yarn nx test edit-content --testPathPattern=dot-edit-content-form.component.spec — 1863 passed, 0 failures
  • yarn nx build dotcms-ui — succeeds (template compilation green)
  • Manual QA (to verify in UI):
    • Open a locked content item in the new Edit Content experience → close/navigate away → no "unsaved changes" dialog.
    • Open an unlocked content item → edit a field → close/navigate away → dialog appears as before.
    • Open a locked item → click the toggle to unlock → edit a field → close → dialog appears as before.

Changed files

  • core-web/libs/edit-content/src/lib/components/dot-edit-content-form/dot-edit-content-form.component.html — add [ngModelOptions]="{ standalone: true }" on the lock p-toggleSwitch (1 line).
  • core-web/libs/edit-content/src/lib/components/dot-edit-content-form/dot-edit-content-form.component.spec.ts — add 4 fakeAsync tests in a new describe('Form dirty state after lock toggle') block.

Visual Changes

No visual changes — the lock toggle still renders and behaves identically. The fix changes how Angular registers the toggle internally, not what the user sees. Manual QA above is the relevant verification.

…t opens pristine

The lock <p-toggleSwitch [ngModel]="isContentLocked"> is defined in the
#appendContent ng-template, which dotTabViewInsert injects inside the
<form [formGroup]="form"> in the new Edit Content experience. Without
ngModelOptions.standalone, Angular registers the ngModel as an implicit
control on the reactive FormGroup. When the async lock-status check
resolves after #scheduleMarkPristineAfterInit has already called
markAsPristine(), the signal update writes through the toggle CVA and
dirties the form — triggering the "You have unsaved changes" dialog on
navigation even though the user never edited anything.

Mark the toggle as standalone so it lives outside the reactive form,
preserving the existing one-way [ngModel] binding and (onChange) handler.

Add four fakeAsync unit tests in dot-edit-content-form.component.spec.ts:
- locked content opens pristine (and stays pristine after tick(500))
- lock toggle onChange does not dirty the form (regression guard)
- editing a real field still dirties the form
- locked -> unlocked -> edited path still dirties the form

Closes #35754
@github-actions github-actions Bot added the Area : Frontend PR changes Angular/TypeScript frontend code label May 20, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 20, 2026

Claude finished @oidacra's task in 1m 55s —— View job


Review — fix(edit-content): keep lock toggle out of FormGroup

  • Read changed files
  • Inspect template + spec changes
  • Check related lock/form code for hidden assumptions
  • Post review

The fix is correct and minimal. Root-cause analysis matches the code: [ngModel] on the lock p-toggleSwitch was implicitly registering with the host [formGroup] (the <ng-template #appendContent> is projected into the <form> via dotTabViewInsert), and a deferred store write through [ngModel] was racing past #scheduleMarkPristineAfterInit (dot-edit-content-form.component.ts:357). standalone: true is the idiomatic resolution — the [ngModel] stays one-way, so PrimeNG's CVA writeValue continues to update the toggle visually when isContentLocked flips, and (onChange) still drives onContentLockChangelockContent()/unlockContent().

Tradeoffs to be aware of (not blockers):

  1. AC5 / AC8 / AC6 tests are tautological. The "real field edit" tests at dot-edit-content-form.component.spec.ts:883 and :905 do:

    control?.setValue(['edited-field']);
    control?.markAsDirty();
    expect(component.form.dirty).toBe(true);

    That just asserts Angular's FormGroup propagates dirty from a child — it would pass even if every real field binding were broken. It does not validate that opening the form and editing a UI field marks the form dirty (which is what the AC actually says). Consider replacing with a real <input>/spectator.typeInElement against one of the rendered dot-edit-content-fields, or drop these to avoid implying coverage you don't have. Fix this →

  2. No regression guard for the toggle's visual state. Switching to standalone: true works because Angular still calls CVA writeValue on [ngModel] input changes. There is no test that asserts the toggle re-renders to checked when isContentLocked becomes true from outside (server-driven lock). Worth one assertion against lockSwitch.modelValue / DOM state to lock the contract.

  3. tick(500) couples tests to the implementation detail. The fallback timer in #scheduleMarkPristineAfterInit (race(isStable, timer(500))) is internal. If anyone later changes the timeout, four tests break for an unrelated reason. Consider draining via flush() or asserting on appRef.isStable instead. Minor.

  4. AC3 / AC4 are not really tested. They're marked "covered by proxy via the guard reading form.dirty." That's a reasonable assumption, but the unsaved-changes guard itself isn't exercised here — if its contract changes (e.g. it starts inspecting touched), this fix silently regresses. Not worth adding here, but acknowledge it's a proxy assertion.

Things I checked and that look fine:

  • [ngModel] is one-way (no banana-in-a-box), so removing it from the FormGroup doesn't change semantics for the toggle's display.
  • onContentLockChange (dot-edit-content-form.component.ts:740) is untouched and still wired via (onChange).
  • No other code reads form.get('...') for the toggle (the form is built strictly from content-type fields via initializeForm), so the implicit control going away is safe.
  • The form-rebuild effect already skips on lock-only state changes via #lastContentletRevisionKey (:265), which is the second line of defense.

Net: ship it. The four follow-ups above are quality-of-tests issues, not correctness issues.
· branch issue-35754-v2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Edit Content form incorrectly marked as dirty when opening locked content

1 participant