PR 19795 review

rancher/dashboard PR 19795 (codyrancher), stop a rejected user retention save from persisting values, at 408332f, reviewed 2026-11-05.

Preview: rancherpr19795r.mf-created-dashboard.ui.rancher.space, built from 408332f, backend mf Rancher (v2.16.0-7758e3e-head). Baseline: the shared dashboard-master preview, rebuilt today from 261b8932f, the current tip. The 4 commits between the PR's base and that tip don't touch this code.

Verdict: approve, nothing to change

The bug is fixed, against the real webhook. Master comes back with 400hours ticked and stores the schedule. The PR comes back empty and leaves the server unchanged.
The issue's second criterion works too. The periods save first and the rest only if both succeed. Every case in the description matches.
No regression found. A valid save sends the same five PUTs as master, in the same order but no longer all at once.
The simple design is right. Cloning at save time fixes the cause, and the diff is small. It also fixes two things the description doesn't mention: a rejected last login coming back as NaN, and re-ticking a box bringing back a rejected value.
Merging: Branch Milestone stays red until master maps to v2.17.0. That's the known gate, so leave it.
Nothing is posted, and suggested text is at the bottom. Cards 1 and 2 are for you and aren't in it.

Test it yourself

These previews talk to the real mf Rancher, which has 87 users and retention off. Stick to the rejected walk below. A valid save with a period and a schedule arms retention for real, so don't do that one here.

The login page defaults to OpenLDAP, so click Log in as a local user first, then use admin and RANCHER_PASSWORD.

Open PR: User retention. Tick Delete user accounts, enter 400hours, and set the schedule to 0 0 1 1 *. Save: a red banner shows the webhook's message.
Click Users in the side nav, then User retention settings. Delete is unticked and empty.
Do the same on master: User retention: 400hours comes back ticked. Master really stores the schedule and dry run, so clear them afterwards:

export KUBECONFIG=kubeconfig/kube_config_server_mf.yaml
for s in user-retention-cron user-retention-dry-run; do kubectl patch settings.management.cattle.io "$s" --type=merge -p '{"value":""}'; done

Back on the page after the rejected save

Master after returning to the page: Delete ticked with 400hours and the schedule filled in — **master:** the rejected `400hours` is back, ticked, and the schedule was stored.

PR after returning to the page: both checkboxes unticked and both inputs empty — **PR:** the stored state, nothing ticked.

Master: stored 500h edited to 600hours, rejected, shows 600hours on return — **master:** stored `500h`, edited to `600hours`, rejected. It shows `600hours`.

PR: stored 500h edited to 600hours, rejected, shows 500h on return — **PR:** the same walk shows the stored `500h`.

How it was and how it is now

Case	master	PR
The description's steps. Delete `400hours`, schedule `0 0 1 1 *`, Save, Users, back.real writes	All five PUTs go out and Delete gets a 400. Back on the page, Delete is ticked with `400hours`. On the server, the schedule and dry run were stored.	Disable 200, Delete 400, then it stops. Back on the page, Delete is unticked and empty. On the server, all five are unchanged.
Valid Disable, rejected Delete. `400h` and `400hours`.server-side dry run	All five PUTs. The schedule, dry run and last login save alongside the rejection.	Disable 200, Delete 400, then it stops. Only Disable is saved, as the description says.
Then correct Delete to `500h` and save again.server-side dry run	All five 200, back to Users, values shown on return.	Same. The second Disable PUT carried the version the first one returned (1268, then 1269), so no conflict.
Stored `500h` edited to `600hours`, Save, Users, back.server-side dry run	Comes back as `600hours`.	Comes back as `500h`.
Rejected Disable, valid Delete. `400hours` and `500h`.server-side dry run	All five PUTs. Back on the page, Disable is ticked with `400hours`.	Delete 200, Disable 400, then it stops. Back on the page, Disable is empty and Delete is `500h`. The schedule is empty because it never saved, so Save stays disabled until you fill it in. See card 1.
Both rejected.server-side dry run	The other three still save, and both bad values come back.	Nothing else saves, and both come back unticked and empty.
Rejected default last login. `abc` goes out as `Invalid Date`, and the webhook refuses it. The form has no check on this field.server-side dry run	Back on the page, the field shows `NaN`.	The periods, schedule and dry run save, last login gets a 400. Back on the page, the field is empty.
Stay on the page after a rejection: untick and re-tick Delete.server-side dry run	`400hours` comes back.	The stored value comes back, empty here. The schedule goes back to stored too, so you retype it.
Full valid save, browser in São Paulo.server-side dry run	All five 200. The date goes out as `2023-12-31T21:00:00-03:00` and reopens as `1704067200000`.	Same. The PUT bodies match master except the preview hostname in `links`, and no `__clone` goes out.
Untick both and save, with values stored. Also re-tick Disable first, for the moved watcher filter.server-side dry run	All five go out null and it reopens empty. Re-ticking restores Disable and the schedule.	Same.

Dry run means the real webhook decided each value through a kube API server-side dry run and nothing was stored. The harness answered the page in Steve's shape and enforced resource versions, so a stale save would have got a 409. Row 1 used real writes, because nothing in it can persist a period.

For you, not in the suggested review

A rejected period now also holds back the dry run toggle

shell/pages/c/_cluster/auth/user.retention/index.vue:183-184

FYI

Say a schedule is already stored and dry run is off. An admin turns dry run on, sets Delete to 500h, and types 400hours in Disable. Delete saves, Disable is rejected, and dry run never goes out. On master, that same failed save would have stored dry run too. If they leave without fixing it, the next scheduled run deletes for real instead of only logging.

Why I wouldn't raise it: it needs a stored schedule, a typo that only gets past the form through the regex in card 2, and someone walking away from a red banner. The order is also exactly what the issue's second criterion asks for. I saw that dry run doesn't go out, and the rest comes from reading the job. I didn't let a real run happen.

The `parseDuration` hole isn't tracked anywhere I could find

shell/composables/useUserRetentionValidation.ts:39

FYI

The description calls it a separate bug, and it's on master, so it's out of scope here. In /^(\d+)h|(\d+)m|(\d+)s$/ the anchors only apply to the outer alternatives, so 400hours and 1000min pass the form and the webhook then refuses them. Along with the unchecked last login field, it's one of the few ways left to reach the path this PR fixes. My search found no issue for it, so you may want one opened.

What was checked

Unit tests	7 of 7 pass at `408332f`. With master's page and the PR's tests, the 3 tests for the new behaviour fail. `jest.config.js` pins `TZ=UTC`, so the date test isn't fragile.
Do the tests catch breakage?	10 of 12 mutations caught, including no clone, all five in parallel, the groups swapped, not waiting for the periods, and swallowing rejections. The 2 survivors are a watcher re-reading the periods and a missing error banner. Neither had a test on master, and I checked both live. Not worth asking for.
The clone path	`clone` is `classify(toJSON())`, and `Setting` adds nothing to the save. After a good save, `load` merges the response into the cached entry, and `settings[key]` is that same object, so the next clone is current. Same idiom as `useGlobalTableViews.ts`.
Real valid save	CI's `user-retention.spec.ts` passed 5 of 5 on this commit. It saves on a real server, waits for all 5 PUTs, and checks the values on return, including the date.
Backend	Read the webhook's validators and Rancher's retention job: the job only fires from the schedule. That is why valid periods went through a dry run here.
Conventions	No template, string or style changes. One new typed helper, and the existing filter moved out under a name. The new tests use `mount` like the rest of that file, and `toStrictEqual`.
CI	All green except Branch Milestone, which is the expected red: `branches-metadata` still maps master to v2.16.0 and the PR is on v2.17.0. The first attempt failed in `@charts` and passed on the bot's retry. I didn't chase it, since that job doesn't touch this page.
Not run	A real retention run. Valid periods persisted on mf (dry run instead, plus the CI spec above). A schedule the form accepts and the webhook rejects: it is in the same group as last login, which I did run. Websocket updates after dry-run saves, because nothing changes on the server. Non-admin roles: the gate isn't touched.

What I did on mf

A slip of mine: my first probe assumed Steve honours ?dryRun=All on a PUT. It doesn't, so user-retention-dry-run really went to true for a couple of minutes until I restored it. It does nothing while the periods are empty, which is why I probed on that setting first.
On purpose: the master run of row 1 stored the schedule and dry run for about 6 seconds (the Rancher log shows the daemon scheduled at 15:13:18 UTC and disabled at 15:13:24). There were no periods, and the schedule only fires on January 1.
End state, checked on the server: all five settings empty, and no retention labels on any of the 87 users. Six of those users are issue-8544*, created by another session during my run.

Suggested review (not posted)

In your voice, in the usual shape: short body, no code blocks, no preview link. Approve. I can add it as a pending review if you want.

Body

Looks good. I ran your steps on master and on this branch against a real webhook. Master comes back with 400hours ticked and stores the schedule, and this branch comes back empty and stores nothing. The other cases in the description match too: a valid Disable with a rejected Delete saves only Disable, and correcting it saves all five. A rejected edit of a stored value goes back to the stored one, and a rejected default last login no longer comes back as NaN.