fix(provisioning): a per-App settings section turned off is a change, and the plan says so - #235
Merged
Merged
Conversation
… and the plan says so
An App's Settings is the COMPLETE desired override: AppAdminService rebuilds the whole
thing from it, so a null section clears that override (inherit the realm) and an empty
subdomain drops the global host route. The planner, though, diffed it as merge-patch off
a serialization using the API's WhenWritingNull option — which erases exactly the nulls
that carry the intent. Unchecking "Dedicated subdomain for this app" therefore staged a
real clear that the draft reported as NO changes at all ("22 unchanged"), while the apply
went ahead and cleared it. A plan that under-reports is worse than no plan: the whole
confirmation UX rests on it.
SectionPolicy gains NestedReplace next to NestedPatch. A replace section is serialized a
second time with nulls kept and diffed off that, so Settings.Origin.Subdomain and a
switched-off Settings.Branding show up like any other change. Optionals keep their
absence — the resolver suppresses a None through ShouldSerialize, independently of the
ignore condition. The options instance is cached per source options (building one is
expensive and the diff runs per entity).
Not a regression of this wave: the apps section has been NestedPatch since #214 (the
always-staged admin UI), and the apply was always right — only the preview lied. No
stored draft needs fixing; the plan is recomputed on every read.
Test: Turning_off_a_per_app_settings_section_shows_in_the_plan_as_the_clear_it_is —
asserts the plan reports both clears, that the apply really drops the override and the
host route, and that re-planning the cleared state is then genuinely unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gin checkbox
"Dedicated subdomain for this app" carried no information. build() produced
`Origin: { Subdomain: override ? (value || null) : null }`, so unticking the box and
ticking it with an empty field were the same payload — a second clear button next to the
one the input already has, and two controls for one fact can contradict each other
(ticked but empty). It also greyed the input out against an "inherited" value that does
not exist: a realm has a primary domain, not a subdomain to inherit.
The value is the switch now: a subdomain routes that host to the app, an empty field
means no own subdomain (the app is reached through the realm URL), and clearing it
removes the existing route — which is what the backend already did with a null.
Deliberately NOT modelled: a disabled-but-remembered subdomain. That needs a real second
property, and it raises the question of whether a parked host keeps blocking the
cross-realm uniqueness check while it routes nowhere. Worth deciding on its own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s key The Origin override checkbox is gone, so its key is dead. The field's own label had no German entry at all (it fell back to the source string, which happened to be German) and the new hint needs one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tings
build() always emitted a ChangeFeed section from the reset defaults, because unlike every
other section it has no "is there an override" state. Per-App settings are REPLACE, so
saving any App materialized {Enabled:false, 7, 1000} on an App that had nothing — an
override that says exactly what no override already says (AppChangeFeedSubscription reads
a missing section as ApplicationChangeFeedSettings.Disabled). Every draft plan then listed
three ChangeFeed changes nobody made, which is noise in the one place that has to be
trustworthy.
No new toggle: "Enable consumer change feed" already IS the switch, the same argument that
just removed the Origin checkbox. The section is sent when the feed is on, or when this App
already has one — the second half is what keeps a configured retention window from being
dropped when an admin switches the feed off. An App that never had a feed and is not
getting one now sends nothing.
Verified in the browser against the dev realm: staging a branding edit on an App without a
feed reports only Settings.Branding.ProductName; switching the feed off on an App with
30 days / 5000 events stages Enabled=false with both numbers intact and plans exactly one
change, Settings.ChangeFeed.Enabled: true -> false.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reported from the admin UI: unticking Dedicated subdomain for this app and staging it produced a draft that showed no changes at all — while the apply happily cleared the subdomain and dropped the host route. A plan that under-reports is worse than no plan; the whole staged-save UX rests on it being true.
The bug
An App's
Settingsis the complete desired override —AppAdminServicerebuilds the whole thing from it, so anullsection clears that override and an empty subdomain drops the global host route. The planner, though, diffed it as merge-patch off a serialization using the API'sWhenWritingNulloption, which erases exactly the nulls that carry the intent. It affected every section, not just Origin: turning Branding, PageTheme or any other one off staged a real clear the plan reported as nothing.SectionPolicygainsNestedReplacenext toNestedPatch. A replace section is serialized a second time with nulls kept and diffed off that.Optional<T>keeps its absence — the resolver suppresses aNonethroughShouldSerialize, independently of the ignore condition.Not a regression of the ADR 0024 wave: the apps section has been
NestedPatchsince #214, and the apply was always right. No stored draft needs fixing — the plan is recomputed on every read.Two honesty fixes in the same screen
ChangeFeedsection from its defaults, materializing an override that says exactly what no override already says (the reader falls back to disabled either way) and adding three phantom lines to every plan. No new toggle — Enable consumer change feed already is the switch. A section the App already has is kept, so switching the feed off preserves a configured retention window instead of dropping it.Deliberately not modelled: a disabled-but-remembered subdomain. That needs a real second property and raises the question of whether a parked host keeps blocking the cross-realm uniqueness check while routing nowhere. Worth deciding on its own.
Test plan
Turning_off_a_per_app_settings_section_shows_in_the_plan_as_the_clear_it_is— red before the fix (unchangedinstead ofupdate), green after; asserts the plan reports both clears, that the apply really drops the override and the host route, and that re-planning the cleared state is then genuinely unchangedpnpm type-checkclean,de.jsonparsesSettings.Origin.Subdomainwith the live value as current and no target, applying drops the origin and leaves branding intact; an App without a change feed plans only its real change; switching a configured feed off keeps 30 days / 5000 events and plans exactly one change🤖 Generated with Claude Code