feat(delete): optionally remove resources associated with a zone or domain (PPT-1203) - #307
Open
camreeves wants to merge 5 commits into
Open
feat(delete): optionally remove resources associated with a zone or domain (PPT-1203)#307camreeves wants to merge 5 commits into
camreeves wants to merge 5 commits into
Conversation
…omain (PPT-1203) Deleting a zone has never removed the systems inside it. `Zone#destroy` cascades child zones, trigger instances, metadata, settings and group links, but `sys.zones` is a text array with no foreign key, so the zone id was simply stripped and any system left with no zones became an orphan (PROJ-845 — the dev install currently carries six such systems). The confirmation copy already claimed otherwise: "Deleting this zone will immediately remove systems without another zone". The delete confirmation now offers "Also delete associated resources", **off by default** — deleting an item without touching what hangs off it stays the default behaviour. Switching it on resolves a plan first and shows exactly what would go before anything is confirmed. Zones remove the systems whose every zone falls inside the subtree, so a system shared with a zone outside it is kept, as it is today. The backend takes their modules, triggers, metadata and settings. Domains remove their OAuth applications (`oauth_applications.owner_id` has no foreign key, so these orphan today) and their staff API tenant, matched on domain name the same way the admin screen does. Zones are only reachable from a domain through the `authority.config.org_zone` convention, which is not exclusive — three domains share one org zone on the dev install — so the zone tree is only included when no other domain references it, and the dialog names the domains that caused it to be skipped. Because the removals are destructive, the system index (Elasticsearch, which can lag) only nominates candidates; each one is re-read through `showSystem` and re-checked against the database before it makes the list. Also here: - `ConfirmModalComponent` gains optional checkboxes with lazily resolved detail, reported back on the confirmation event. Existing callers pass no options and are unaffected. - The zone mock filtered on a `parent` query param the API has never sent; it now honours `parent_id`, including comma separated lists and `root`. Mock zones gained the `parent_id` hierarchy their `zones` arrays imply. - `config/proxy.conf.js` accepts `PLACEOS_DOMAIN` so the dev server can point at a local stack. - `ZONES.DELETE_MSG` now describes what actually happens. The stale translation was dropped from the non-English locales so they fall back to the corrected source string rather than repeating the old claim. Verified against a local PlaceOS stack: an org > building > two levels tree with three systems (two enclosed, one straddling an outside zone) and their modules. With the option on, the tree, both enclosed systems and both modules were removed while the straddling system survived holding only its outside zone; with it off, the zone went and the system was left orphaned exactly as before. A domain delete removed its org zone tree, orphaned system, OAuth application and staff API tenant; a domain sharing its org zone with another left the zone, its system and the other domain untouched.
The org zone line already reads "and everything beneath it", so appending the nested zone plan's own scope line stacked two "Scope:" sentences on top of each other in the dialog.
The checkbox label and the empty box outline both rendered near-black on the dark background. The app already styles `mat-checkbox`, but those rules target `.mdc-checkbox__background` and `.mdc-label`, which never win against Angular Material's own selectors — so the colours were still coming from the prebuilt `indigo-pink` (light) palette. Nothing hit this before because no other `<mat-checkbox>` in the app has label content, and the faint outline was easy to miss. Set at the token layer instead, where Material actually reads them, using `currentColor` so they follow whichever theme is active. Measured on the delete confirmation: label outline dark 1.21:1 -> 10.84:1 (WCAG wants 3:1 for controls) label text dark 1.18:1 -> 10.84:1 (WCAG wants 4.5:1 for body text) Light theme is unchanged at 20.12:1. This affects every checkbox on the dark theme, all of which had the same invisible outline. The cascade warning line used `text-warning` — a yellow that only reads on a dark background (1.39:1 on the light theme). It now tints the block and keeps the inherited text colour, matching how warnings are done elsewhere. Secondary text nudged from 60% to 70% opacity for the same reason. Every line in the dialog now passes WCAG AA in both themes.
A toast saying "Removed 6 associated resources" is not something you can act
on afterwards. When a cascade runs, the confirmation now becomes a receipt
listing every resource that went — type, name and id — in the order they were
removed, ending with the item itself. A "Copy list" button puts it on the
clipboard as tab separated rows for pasting into a ticket.
Cascade steps carry a `CascadeResource` ({type, id, name}) instead of a
pre-rendered progress string, and `CascadeOutcome.removed` is that list rather
than a count, so the receipt reports what actually happened rather than what
was planned.
Partial failures now show the same receipt with the failures called out
separately, and the item is deliberately left in place — previously this was a
notification that named only the first failure and left the operator guessing
which of the rest had gone.
Existing delete flows are untouched: the receipt only appears when a cascade
actually ran, so every other resource type still closes the dialog and shows
its usual notification. There is an e2e test asserting exactly that.
`receiptToTsv` is a pure function so the clipboard formatting is tested
without DI. It also keeps `@angular/cdk/clipboard` out of the spec, which
upsets vitest's `vi.mock` hoisting analysis and made it warn about mocks it
had previously accepted.
Verified against the local stack: deleting a domain with two OAuth apps, a
staff tenant, an org zone tree and three enclosed systems produced an 8 row
receipt, and every id on it was confirmed absent from the API afterwards
while the four intended survivors remained.
Captured from the local stack for the pull request. 512KB total — say the word if the team would rather not carry them in tree and I will move them out.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
MrYuion
requested changes
Jul 29, 2026
Collaborator
There was a problem hiding this comment.
The cascade flow needs to fail closed before this is safe to merge.
- [P1] Domain lookup failures can delete another domain's org zone: converting
queryDomainsfailures to an empty list makes an unavailable or partially paginated ownership list look like no other domain references the zone. - [P1] Failed system deletion does not prevent deleting its zone:
runCascadecontinues after errors, while domain plans append the org-zone deletion after system removals, so a failed system can be orphaned. - [P1] Failed plan resolution silently falls back to ordinary deletion: when details resolution rejects, the selected option has no plan, the cascade branch is skipped, and
actions.removestill runs. Please make plan resolution fail closed, stop dependent deletion steps after a failure, and block confirmation unless every selected option has a valid plan.
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
2 participants
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.
Closes PPT-1203. Also addresses the behaviour reported in PROJ-845.
The problem
Deleting a zone has never removed the systems inside it.
Zone#destroycascades child zones, trigger instances, metadata, settings and group links, butsys.zonesis a text array with no foreign key — soZone#remove_array_referencesjust strips the id and any system left with no zones is orphaned.The confirmation copy already claimed otherwise:
It never did. That is exactly the complaint in PROJ-845, and
placeos-dev.aca.imcurrently carries 6 systems withzones = {}as a result, plus one pointing at azone-123that does not exist.Tearing down a domain is worse — it is the manual, one-at-a-time process described in PPT-1203, and OAuth applications orphan silently because
oauth_applications.owner_idhas no foreign key back toauthority.What this adds
An opt-in checkbox on the delete confirmation. It is off by default — deleting an item without touching what hangs off it stays the default behaviour, unchanged.
Ticking it resolves a plan first and shows exactly what would go, before anything is confirmed:
Afterwards the dialog becomes a receipt of what actually went — type, name and id, in removal order, ending with the item itself. "Copy list" puts it on the clipboard as TSV for pasting into a ticket.
The rules it applies
Zones — removes systems whose every zone falls inside the subtree, i.e. exactly those that would be left with no zone. A system shared with a zone outside it is kept, as today, and the dialog says so. The backend then takes their modules, triggers, metadata and settings; the zone delete takes the rest.
Domains — removes the domain's OAuth applications and its staff API tenant (matched on domain name, the same rule
staff-api.component.tsalready uses to list them). Zones are only reachable from a domain through theauthority.config.org_zoneconvention.That convention is not exclusive, and this is the important bit. On dev:
Three domains share one org zone and three have none at all. Cascading zones off a domain unconditionally would destroy another customer's estate, so the org zone tree is only included when no other domain references it, and the dialog names the domains that caused it to be skipped:
Drivers and repositories are deliberately out of scope — they are install-wide and shared across domains, and deleting one destroys every module using it everywhere.
Notes for review
Nothing existing changes. The receipt and the cascade only engage when the option is ticked. Users, drivers, modules, triggers, and zones/domains with the box left off all behave exactly as before — there are e2e tests asserting both (
shows no receipt for a delete without the option,leaves systems alone when the option is left off).Destructive reads go to the database, not the index.
GET /systems?zone_id=is Elasticsearch-backed and can lag. Since a stalezonesarray would mean deleting a system that still belongs somewhere, the index only nominates candidates — each is re-read throughshowSystemand re-checked before it makes the list.It is frontend-orchestrated, so a large teardown is N delete requests rather than one. That was a deliberate call: it works against every deployed backend today with no coordinated release, and gives per-item progress and precise partial-failure reporting. A rest-api
DELETE /zones/:id?recursive=truecould replace the inner loop later without changing anything the user sees — worth a follow-up ticket.Partial failures show the same receipt with the failures called out, and the item is deliberately left in place rather than deleted on top of a broken cascade.
Two drive-by fixes, both in
styles.cssand both affecting checkboxes app-wide on the dark theme: the label and the empty box outline were rendering near-black (1.18:1 and 1.21:1) because the app'smat-checkboxrules target.mdc-checkbox__background/.mdc-labeland never win against Material's own selectors, so the colours still came from the prebuilt light palette. Set at the token layer instead. Nothing hit this before because no other<mat-checkbox>in the app has label content.Also here: the zones mock filtered on a
parentquery param the API has never sent (nowparent_id, including comma lists androot), mock zones gained theparent_idhierarchy theirzonesarrays already implied, andconfig/proxy.conf.jsacceptsPLACEOS_DOMAINso the dev server can point at a local stack.Screenshots live in
docs/ppt-1203/(512KB). Happy to move them out of tree if you would rather not carry them.Testing
cascade-delete.spec.ts(24),cascade-locale.spec.ts(14, pins singular/plural against the real locale file), plus 23 on the confirm modal.e2e/src/cascade-delete.spec.ts, 9 tests. Full chromium suite 84 passing, 1 skipped.org > building > 2 levels, 3 enclosed systems + 1 straddling an outside zone, option onA seeded sandbox that reproduces all four is in
tasks/PPT-1203/demo(not part of this branch).