Skip to content

Add a Reopen button for expired requests - #598

Draft
barborico wants to merge 18 commits into
mainfrom
brynna/reopen_expired_request
Draft

barborico wants to merge 18 commits into
mainfrom
brynna/reopen_expired_request

Conversation

@barborico

@barborico barborico commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a "Reopen Request" button to the view for expired access, role, and group requests. It opens the normal create dialog prefilled from the lapsed request.

Urgency: LOW - 2 weeks?
Expected review effort: MEDIUM

Stacked on #597 — please review that one first; this branch targets brynna/request_expiration.

Motivation

#597 makes role and group requests expire, which means more requests will lapse without anyone having said no. Today a user whose request expired has no path back other than rebuilding it by hand from the read view: retyping the group, the duration, and the justification. This makes the common case one click.

Reopen is deliberately an ergonomic shortcut for "create a fresh one", not a new operation. It adds no endpoint; it drives the existing create endpoints, so tag constraints and conditional-access hooks re-run exactly as they would on a hand-built request.

Description & Screenshots of Changes

Recognising an expired request. Expiration is not modelled as its own status or column, so isExpiredRequest (src/helpers.tsx) infers it from status === 'REJECTED' + a null resolver + an exact match on the sweep's reason string. A null resolver alone is not enough: user deletion, group deletion, group unmanaging, and a conditional-access plugin denial all close requests with no resolver, and a policy denial in particular must never get a one-click reopen. The check goes through resolver, not resolver_user_id, because GroupRequestDetail exposes only the former — keying off the id would silently never match on group requests.

The reason string is duplicated across the language boundary and pinned by tests/test_expired_reason_constant.py, which reads src/helpers.tsx and asserts the backend constant appears in it.

Duration prefill is an extraction, not new logic. Recovering the duration a request originally asked for already existed, duplicated, in requests/Read.tsx and role_requests/Read.tsx. It moved to reconstructRequestedUntil in src/helpers.tsx and both call sites now use it, so this leaves one copy where there were two. The option-label map is a parameter rather than an assumption, because the access and role views source it differently (accessConfig.ACCESS_TIME_LABELS vs a hardcoded map) — unifying those is out of scope.

Requests store an absolute request_ending_at, so a request that expired because its window lapsed has a stored date in the past. Diffing against created_at recovers the duration: an exact match round-trips to that option, anything else is re-offered as a custom date based from today, and a tag time limit that has since tightened clamps the result.

The button lives inside CreateRequest. It is rendered via a new reopen prop (mirroring the existing renew prop) rather than as a separate button in the read view, so it sits inside CreateRequest's own null-return guards. A hand-rolled button plus controlled open/setOpen would gate the button and the dialog on different conditions and produce a dead button — visible, clicked, opening nothing — for a requester who has since become an owner of the group.

Visibility mirrors who may create the same request:

Type Who sees reopen Why
Access request the original requester only an access request targets its own requester, so nobody else can recreate that request
Role request current owners of the role a role request must be submitted by a role owner, so any of them can
Group request any authenticated viewer anyone may request a group

Group requests carry the most prefill (type, app, name, description, ownership duration, reason, tags, plugin config) and need two client-side resolutions the API returns bare — app id → AppDetail, tag ids → TagDetail[]. Both reuse locals the read view already computes; no new fetches. Plugin config is dropped when the app's lifecycle plugin changed since the request was written, since the stored config is keyed by the plugin id configured then; the rest of the prefill still applies.

Screenshot — reopening an expired individual access request:

image

Screenshot — reopening an expired app-group request. Note the name field holds the un-prefixed suffix (Reporting) beside the static App-Zztestapp-, matching how the create form works:

image

Validation of Changes

  • make test green: ruff, ty, 879 backend pytest, 57 frontend vitest. npm run tsc clean.
  • Unit tests on the pure helpers, which is where the decision logic lives: isExpiredRequest (the true case, a human rejection with both a populated and an empty reason, each of the four other null-resolver closure reasons, pending/approved, and missing fields); reconstructRequestedUntil (indefinite, exact-option round-trip, the 100-second rounding, the custom re-base, both clamp paths, and the no-clamp default); prefillablePluginData (matching plugin, switched plugin, no plugin, no app, and that it narrows to only the matching plugin's entry).
  • The cross-language pin was tamper-checked: rewording the frontend constant makes it fail, and it passes again on revert.
  • Browser-verified against a locally seeded database (make run-backend + make run-frontend, six seeded requests):
    • Button appears on the expired access, role, and group requests; absent on a control access request rejected by a person with reason "Not this quarter." — which is the discrimination the reason-string match buys.
    • Access request: group App-Access-Default, until reconstructed to 7776000 / "90 Days" from the original 90-day ask, reason verbatim, Member toggle set (Owner false).
    • Role request: role Role-Zz-Reopen-Demo, target group, "90 Days", reason verbatim, Member toggle. Dialog opened, so no dead button.
    • app_group: name field holds Reporting with App-Zztestapp- rendered as static text beside it — not App-Zztestapp-Reporting. This is the case only a browser catches; the pre-fix bug produced a valid name, so no test, type check, or lint would flag it. Type correctly seeded to app_group so the app selector rendered at all, app Zztestapp selected, tag chip Zz-Quarterly-Renewal resolved from its stored id, ownership "90 Days", reason verbatim.
    • role_group: Role-Zz-Analytics → name Zz-Analytics.
    • okta_group: Zzwidgets left intact — no over-stripping of a name that has no prefix.
    • No console warnings, including no MUI Autocomplete "none of the options match". All /api/ calls 200.
    • A request whose original duration was a custom date (47 days, not one of the preset options): the "Custom End Date" field renders and is pre-filled with today + 47 days. This case was broken until review caught it — the earlier pass only exercised the exact-option ("90 Days") path, which is the path that worked.
  • No page render tests: this repo has essentially no such infrastructure (two files use React Testing Library) and .claude/access-dev-testing.md says coverage is concentrated on pure helpers and form logic. That is why the browser checklist above matters rather than being belt-and-braces.
  • No migration, no Pydantic schema change, no generated-client edit, no new endpoint.

Guidance for Reviewers

Go file-by-file rather than commit-by-commit — the last commit is a fix pass that touches several of the earlier files, so the commits do not cleanly reflect final state.

Three callouts:

  1. The inferred-expiry approach (isExpiredRequest). It matches a display string, which is a bit janky. The argument for it: reopen only opens a prefilled form, and submitting re-runs constraints and conditional-access hooks, so a wrongly-shown button self-corrects rather than granting anything. If we would rather pay for robustness, I think the right shape is added nuance in the status enum, but that's a heftier change.
  2. Why the button is a reopen prop on CreateRequest rather than a button in the read view. This is load-bearing against the dead-button case described above; a future "simplification" that pulls it back out would reintroduce it.
  3. reconstructRequestedUntil's optional timeLimit. The two pre-existing read-view call sites deliberately pass none, because they apply their own clamp separately when building the approve form's defaultValues. The three reopen call sites do pass it. Passing it at the wrong site silently changes what the approve form defaults to; both the helper and each call site carry a comment saying so.

🤖 Generated with Claude Code

barborico and others added 4 commits August 23, 2026 19:18
Group requests are about to start expiring, and their approver may need to
negotiate a name and pick tags rather than just answer yes/no, so they get
their own knob. Unset falls back to MAX_ACCESS_REQUEST_AGE_SECONDS so no
operator's existing config changes meaning; an explicit 0 is honored as
expire-immediately rather than swallowed by the fallback.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One request that failed to reject aborted the whole sweep, and would do so
again on every subsequent sync, so a single poisoned row could stop
expiration indefinitely. Reject each request in its own try/except with a
rollback, via a _expire_each helper the role and group sweeps will reuse.

_expire_each reads each request's id up front, before any rollback, and
calls reject with the id rather than the ORM instance: db.session.rollback()
expires every instance in the identity map, not just the failed one, so
touching an attribute on an already-loaded-but-not-yet-processed request
after a prior rollback would force a lazy reload with no async context to
run it in (MissingGreenlet). Every reject operation already accepts a plain
id in place of the model instance, so this costs nothing and the same
pattern will hold for role and group requests.

Also hoists the reason string to EXPIRED_REQUEST_REASON, since all three
sweeps write it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Role requests never expired, so they accumulated in reviewers' queues
indefinitely and one past its request_ending_at stayed approvable into an
already-dead RoleGroupMap. Sweep them on the same sync pass as access
requests, sharing MAX_ACCESS_REQUEST_AGE_SECONDS since both are a yes/no on
granting access.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Group requests never expired, so they accumulated in approvers' queues
indefinitely. Age cutoff only: unlike an access or role request, one past
its requested_ownership_ending_at is not moot, because the resolver can edit
resolved_ownership_ending_at before approving. A negative test pins that
asymmetry.

Uses MAX_GROUP_REQUEST_AGE_SECONDS rather than the access cutoff, since a
group request's approver may need to negotiate a name and pick tags.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@barborico
barborico force-pushed the brynna/reopen_expired_request branch from ae9a01b to f15492e Compare August 24, 2026 19:48
…tion test

An independent review found the stated justification for group requests not
having a lapsed-window sweep to be false. ApproveGroupRequest falls back to
requested_ownership_ending_at when the approver supplies no resolved value, and
coalesce_ended_at passes a past timestamp through unchanged, so approving a
long-stale request does write an already-expired ownership row and leave the new
group unowned -- which then routes its future approvals to the app owner or an
Access admin. The policy of not expiring the request still stands; the claim
that no dead-on-arrival grant was possible does not. Both the sweep docstring
and the pinning test now say what the asymmetry actually is and name the gap
that remains in the approve path.

The isolation test was also not testing isolation. It raised before the reject
operation emitted any SQL, so the session was clean and the rollback in
_expire_each was a no-op; deleting that rollback left the test passing. Inject
at the operation's own commit instead, after it has assigned status and
resolved_at to the session, and assert the failed request is still PENDING --
without the rollback its dirty row is autoflushed by the next iteration's
SELECT ... FOR UPDATE, silently rejecting the request the sweep logged as
skipped. Verified the test now fails with the rollback removed.

Also from the review: type _expire_each's callable as Callable[[str], ...] so
ty enforces the ids-not-instances contract the comment asks for, give the group
sweep the same named reject as its two siblings, drop the unread module-level
MAX_GROUP_REQUEST_AGE_SECONDS export (it snapshotted a derived property, so it
diverged from settings under monkeypatch), and document what 0 means plus a
ge=0 bound so a negative value fails fast instead of expiring future rows.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both notes described the lapsed-ownership gap as open and said it was worth
fixing elsewhere. #599 closes it by refusing such an approval outright, so say
that instead of leaving a reader to wonder whether anything handles it.

No behavior change; comments and a test docstring only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@barborico
barborico force-pushed the brynna/reopen_expired_request branch from 7ba9a91 to eaa07f5 Compare August 25, 2026 00:34
barborico and others added 3 commits August 24, 2026 22:24
Both request-age cutoffs now accept the string "never" to switch the
age-based sweep off, and both carry an explicit one-week default rather than
the group one inheriting the access one. Explicit beats implicit: under
inheritance, changing the access cutoff silently moved group behavior.

"never" rather than None because Settings comes only from env vars and .env,
where every value is a string: an omitted variable is indistinguishable from
one set to nothing, and "", "None", and "null" all fail int validation, so
None is unreachable for a setting that must keep a default. A string sentinel
also explains itself in the config file, where 0 would need a lookup and the
natural wrong guess ("expire immediately") is the opposite of the truth.

A validator rejects ints below 1 and names "never" in the message, because -1
is the likely Unix-habit guess for disabling and would otherwise close every
pending request on the next sync. Field(ge=...) cannot do this: on a
Union[int, Literal["never"]] the bound applies to the union and rejects the
sentinel itself.

Each sweep reads the resolving property and skips its age query when that
cutoff is disabled. The requested-window pass is deliberately left unguarded:
a request whose own request_ending_at has passed is still closed, since
disabling a setting named for a maximum age says nothing about the window the
requester actually asked for. A test pins that, verified to fail when the
second pass is guarded too. Each skip logs at INFO so sync output
distinguishes a disabled sweep from one that found nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither MAX_ACCESS_REQUEST_AGE_SECONDS nor MAX_GROUP_REQUEST_AGE_SECONDS
appeared in the README or the env example, so an operator had no way to
discover either the cutoffs or the new "never" opt-out.

Calls out the three things most likely to surprise: the two settings are
independent, one of them governs role requests as well as access requests, and
"never" does not stop the separate requested-window check for those two types.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two docstrings claimed the group-request lapsed-ownership-window gap was
already repaired in ApproveGroupRequest; that fix is only proposed in a
separate, unmerged PR (#599), so reword both to describe the gap as open
rather than closed, so main doesn't tell future maintainers a hole is
patched when it isn't.

Move the README's "Request expiration" section out of the JSON-config
narrative (where MAX_ACCESS_REQUEST_AGE_SECONDS/MAX_GROUP_REQUEST_AGE_SECONDS
would silently no-op if set there) and into the env-var docs where they
actually take effect, and note explicitly that the JSON config file has no
effect on them. Also document that the new role-request requested-window
sweep is unguarded by either cutoff, so the first sync after upgrading can
close an entire backlog and fire a notification per closure; add the same
"governs access and role requests" and 0-is-rejected notes to
.env.production.example.

Extend the role-request "never" test to also cover a lapsed request_ending_at
alongside the merely-old one, mirroring the equivalent access-request test,
since that half of the README's "never disables the age cutoff only" claim
had no test coverage on the role-request side.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@barborico
barborico force-pushed the brynna/reopen_expired_request branch from eaa07f5 to a98d7c0 Compare August 25, 2026 05:52
barborico and others added 9 commits August 26, 2026 00:25
Per review: the README described the requested-window check as new and framed
its effect relative to upgrading, which reads as a changelog entry rather than
documentation. Anyone arriving at the file later has no idea what "new" was
measured against. State the behavior instead: closing a request notifies its
requester and every eligible approver, so a run that closes many at once sends
many notifications. The mechanism itself was already covered by the bullet on
`never` above it, so the paragraph now carries only the operational point.

Same fix in the group-sweep docstring and its pinning test, which pointed at an
open PR and said the gap would close "until it lands". That dates the comment to
a moment in review rather than stating what is true, and it stops being correct
the moment that PR merges either way. They now say the approve path falls back
to a stale ownership window and that repairing it belongs there, which is a fact
about the code a reader can check.

No behavior change; documentation, a docstring, and a test docstring only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Expiration is not modelled as its own status, so the UI infers it from a null
resolver plus the reason string. A null resolver alone is not enough: user
deletion, group deletion, unmanaging, and conditional-access denials all
close requests with no resolver, and a policy denial in particular must never
be offered a one-click reopen.

Checks resolver rather than resolver_user_id, which GroupRequestDetail does
not expose. A pytest test pins the string against the syncer's copy, since
rewording either side would silently break the button.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The same duration-recovery logic was duplicated in requests/Read.tsx and
role_requests/Read.tsx, and the reopen prefill needs a third copy. Extract it
to helpers, with the option-label map injected rather than assumed, since the
two pages source it differently.

Behavior-preserving: timeLimit is optional and both call sites pass none,
because they already apply their own clamp when building form defaultValues.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review findings on the just-extracted reconstructRequestedUntil helper:

1. The custom-rebase test compared the helper's internal dayjs() against a
   later dayjs() in the assertion via an exact day diff; dayjs's diff() floors,
   so any elapsed time between the two calls drops the result by a day and
   flakes in CI. Replaced with a seconds-based tolerance.

2. The clamp path could return until: 'custom' with no customUntil when no
   numeric option in untilLabels fit a tight timeLimit, contradicting the
   documented contract and leaving an empty date picker for the upcoming
   callers that pass timeLimit into a form's defaultValues. Replaced
   largestAllowedUntil with clampedToTimeLimit, which supplies a customUntil
   pinned to exactly the limit in that fallback case; added tests for a loose
   timeLimit (unclamped) and a limit tighter than every option (clamped
   custom date).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An expired request left the requester rebuilding it by hand from the read
view. Offer a button that opens the normal create dialog prefilled with the
group, ownership toggle, reason, and the duration originally asked for,
re-based from today since the stored end date may be in the past.

Shown only to the original requester: an access request targets its own
requester, so nobody else can recreate this request rather than a different
one for themselves. The button and dialog are both owned by CreateRequest
itself (via a new `reopen` prop, following the existing `renew` pattern)
rather than a separate button in the read view, so they share the same
group-level guards (deleted group, unmanaged group, already-a-manager) and
never go dead independently of each other.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An expired role request left owners rebuilding it by hand from the read view.
Offer a button that opens the normal create dialog prefilled with the role,
target group, ownership toggle, reason, and the duration originally asked
for, re-based from today since the stored end date may be in the past.

Mirrors the access-request reopen (previous task). Offered to current owners
of the role rather than only the original requester, because a role request
must be submitted by a role owner, so any of them can recreate the same
request.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes reopen across all three request types. The group create form took
no prefill at all, so it gains a single GroupRequestPrefill object; tags and
the app come from the resolutions the read view already performs, since the
API returns both as bare ids.

Plugin config is dropped when the app's lifecycle plugin changed since the
request was written, so the form never seeds config for a plugin the app no
longer uses; the rest of the prefill still applies.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ings

The Critical fix: reopening an expired app-group or role-group request
prefilled the create form's Name field with the full stored group name
(e.g. "App-Foo-Bar"), but that field holds only the un-prefixed suffix and
the form re-prepends the prefix on submit; this silently created the group
under a doubled name ("App-Foo-App-Foo-Bar"). Use the already-computed
strippedRequestedName instead, since it feeds the identical convention
elsewhere in the same file.

Also from the same review pass:
- Compute the reopen ownership-length prefill against the create form's own
  hardcoded option map (now exported) instead of the operator-configurable
  accessConfig map, so a customized config can't produce a prefill value the
  create form's select doesn't offer.
- Add a prefillablePluginData test with two stored plugin keys, since a
  single-key fixture couldn't catch an implementation that returns the whole
  stored object instead of narrowing to the matching plugin.
- Correct the reconstructRequestedUntil docstring, which described two
  call sites when there are now five, each with a different reason for its
  timeLimit argument.
- Add the missing rationale comment on role_requests/Read.tsx's reopenPrefill
  call, matching its twin in requests/Read.tsx.
- Make the reopen button's tooltip describe reopening instead of always
  showing the create-a-new-group tooltip text.
- Seed the app search input from the prefill so a prefilled app is present
  in the Autocomplete's option list, avoiding MUI's invalid-value warning.
- Note why GroupRequestPrefill is a single object where the sibling views
  use loose props, and cross-reference the two prefill-seeding comments.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…its picker

An independent review found the access and role reopen prefills broken for the
one case my manual pass never exercised: a request whose original duration was a
custom date. Both forms hold the selected duration twice -- react-hook-form's
`until` field and a plain `useState` -- and the custom-date picker is gated on
the state. Seeding only `defaultValues` produced a "Custom" selection with no
date field, submitting the hidden seeded date without the picker's `required`
validation ever running.

This is the same state-vs-form-init split the group form already handled; two of
three forms were missing it. Seed the state from props.until in both. Verified in
a browser against a 47-day (non-option) expired request: before the fix the
picker was absent, after it the field renders pre-filled with today + 47 days.

Also suppress reopen on an app_group request whose app no longer resolves.
strippedRequestedName falls back to the full stored name there, and the create
form re-prepends the prefix of whichever app the user then picks, yielding
App-Bar-App-Foo-Reporting -- the same silently-valid wrong name the previous fix
addressed, by a different route. Positional stripping is ambiguous when an app
name contains a hyphen, so withhold the affordance instead; creating a fresh
request by hand still works. The pre-existing approve form keeps its current
fallback behavior.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@barborico
barborico force-pushed the brynna/reopen_expired_request branch from a475ca6 to ffda6fa Compare August 26, 2026 07:26
Base automatically changed from brynna/request_expiration to main August 27, 2026 20:00
@barborico
barborico force-pushed the brynna/reopen_expired_request branch from ffda6fa to 9d3676e Compare August 27, 2026 20:00
@matthew-bass

Copy link
Copy Markdown
Contributor

this seems nice / harmless to add, though how often do people go and immediately re-file requests in access for things that get rejected?

@barborico
barborico force-pushed the brynna/reopen_expired_request branch from 9d3676e to ffda6fa Compare September 11, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants