From a41accfeebc8cf9ac66d9bb52998bb637e1f03ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Janou=C5=A1ek?= Date: Fri, 25 Sep 2026 00:34:47 +0200 Subject: [PATCH] Allow administrators to reconsider resolved registrations Match the legacy request Resolve contract: offer decisions from known registration states except a transition to the current state. Recheck the state captured when the decision opened before posting. Keep bulk review and queue traversal awaiting-only, with owner and role guards intact. --- .../admin/requests_operations_smoke.spec.ts | 74 +++++++++++++++++++ .../app/admin/RequestResolveMutation.test.ts | 12 +++ src/pages/app/admin/RequestResolveMutation.ts | 11 ++- .../RequestReviewActions.behavior.test.tsx | 8 +- .../app/admin/RequestReviewActions.test.ts | 18 +++-- src/pages/app/admin/RequestReviewActions.tsx | 8 +- src/pages/app/admin/RequestReviewModel.ts | 10 ++- 7 files changed, 123 insertions(+), 18 deletions(-) diff --git a/e2e/specs/admin/requests_operations_smoke.spec.ts b/e2e/specs/admin/requests_operations_smoke.spec.ts index 77be8702..e47ab855 100644 --- a/e2e/specs/admin/requests_operations_smoke.spec.ts +++ b/e2e/specs/admin/requests_operations_smoke.spec.ts @@ -1548,3 +1548,77 @@ test('@workflow-matrix @smoke admin requests: filter segments stay contained at } expect(await page.evaluate(() => document.documentElement.scrollWidth)).toBeLessThanOrEqual(320); }); + +for (const [state, action, target] of [ + ['ignored', 'approve', 'approved'], + ['denied', 'ignore', 'ignored'], + ['pending_correction', 'approve', 'approved'], + ['approved', 'deny', 'denied'], +] as const) { + test(`@pr-smoke @pr-smoke-mobile admin requests: reconsider ${state} registration as ${target}`, async ({ page }) => { + await bootstrapVpsAdminWindow(page); + await installOsmMapMock(page); + let current = registration(950, state); + let posted: unknown; + await installHaveApiMock(page, { + user: { id: 1, login: 'admin', level: 100 }, + handlers: { + 'GET user_request/registrations/950': () => ({ registration: current }), + 'GET user_request/registrations': () => ({ registrations: [current] }), + 'GET user_request/changes': () => ({ changes: [] }), + 'GET nodes': () => ({ nodes: [] }), + 'GET locations': () => ({ locations: [{ id: 7, label: 'Prague' }] }), + 'GET os_templates': () => ({ os_templates: [{ id: 5, label: 'Debian 13', cgroup_version: 'cgroup_v2' }] }), + 'POST user_request/registrations/950/resolve': ({ reqJson }) => { + posted = reqJson; + current = { ...current, state: target }; + return { registration: current }; + }, + }, + }); + await page.goto('/admin/requests/registration/950'); + const sameAction = { approved: 'approve', denied: 'deny', ignored: 'ignore', pending_correction: 'request_correction' }[state]; + await expect(page.getByTestId(`admin.requests.resolve.action.${sameAction}`)).toHaveCount(0); + await page.getByTestId(`admin.requests.resolve.action.${action}`).click(); + if (action === 'approve') { + await page.getByTestId('admin.requests.resolve.create_vps').uncheck(); + await page.getByTestId('admin.requests.resolve.submit').click(); + } else if (action === 'deny') { + await expect(page.getByTestId('admin.requests.resolve.submit')).toBeDisabled(); + await page.getByTestId('admin.requests.resolve.reason').fill('Reviewed again'); + await page.getByTestId('admin.requests.resolve.submit').click(); + } + await expect.poll(() => posted).toEqual({ registration: { + action, + ...(action === 'approve' ? { create_vps: false, activate: true } : {}), + ...(action === 'deny' ? { reason: 'Reviewed again' } : {}), + } }); + await page.goto('/admin/requests/registration/950'); + await expect(page.getByTestId(`admin.requests.resolve.action.${action}`)).toHaveCount(0); + await expect(page.getByTestId('admin.requests.detail.metadata').locator('details')).toHaveAttribute('open', ''); + await expect(page.getByTestId('admin.requests.detail.review').getByTestId('admin.requests.detail.risk.ip')).toHaveCount(0); + const details = await page.getByTestId('admin.requests.detail.registration.fields').boundingBox(); + const ip = await page.getByTestId('admin.requests.detail.risk.ip').boundingBox(); + const mail = await page.getByTestId('admin.requests.detail.risk.mail').boundingBox(); + expect(ip!.y).toBeGreaterThan(details!.y + details!.height); + expect(mail!.y).toBeGreaterThan(details!.y + details!.height); + }); +} + +test('@pr-smoke @pr-smoke-mobile admin requests: reconsideration stops when a resolved state changes', async ({ page }) => { + await bootstrapVpsAdminWindow(page); + await installOsmMapMock(page); + let reads = 0; + let posts = 0; + await installHaveApiMock(page, { + user: { id: 1, login: 'admin', level: 100 }, + handlers: { + 'GET user_request/registrations/951': () => ({ registration: registration(951, ++reads === 1 ? 'denied' : 'approved') }), + 'POST user_request/registrations/951/resolve': () => { posts += 1; return {}; }, + }, + }); + await page.goto('/admin/requests/registration/951'); + await page.getByTestId('admin.requests.resolve.action.ignore').click(); + await expect(page.getByRole('status')).toContainText(/changed before submission|před odesláním změnila/i); + expect(posts).toBe(0); +}); diff --git a/src/pages/app/admin/RequestResolveMutation.test.ts b/src/pages/app/admin/RequestResolveMutation.test.ts index 31f4a831..f568c9b5 100644 --- a/src/pages/app/admin/RequestResolveMutation.test.ts +++ b/src/pages/app/admin/RequestResolveMutation.test.ts @@ -5,6 +5,7 @@ import { emptyRequestOverrides } from './RequestReviewModel'; import { changeResolvePayload, fetchAwaitingReviewTarget, + fetchReviewTarget, hasInvalidNumericResolveOverride, registrationResolvePayload, } from './RequestResolveMutation'; @@ -126,4 +127,15 @@ describe('request resolve payloads', () => { } as never); await expect(fetchAwaitingReviewTarget('registration', 43)).resolves.toMatchObject({ id: 43 }); }); + it('rechecks the state actually reviewed, including resolved registrations', async () => { + vi.mocked(fetchRegistrationRequest).mockResolvedValue({ + data: { id: 43, state: 'ignored', login: 'new-user', user: null }, meta: {}, + } as never); + await expect(fetchReviewTarget('registration', 43, 'ignored')).resolves.toMatchObject({ state: 'ignored' }); + await expect(fetchReviewTarget('registration', 43, 'denied')).rejects.toMatchObject({ reason: 'state_changed' }); + await expect(fetchAwaitingReviewTarget('registration', 43)).rejects.toMatchObject({ reason: 'state_changed' }); + vi.mocked(fetchRegistrationRequest).mockRejectedValue(new Error('API unavailable')); + await expect(fetchReviewTarget('registration', 43, 'ignored')).rejects.toThrow('API unavailable'); + }); + }); diff --git a/src/pages/app/admin/RequestResolveMutation.ts b/src/pages/app/admin/RequestResolveMutation.ts index 2aaf693f..b2fe275a 100644 --- a/src/pages/app/admin/RequestResolveMutation.ts +++ b/src/pages/app/admin/RequestResolveMutation.ts @@ -22,7 +22,7 @@ export class RequestReviewPreconditionError extends Error { ? 'Request target mismatch' : reason === 'owner_missing' ? 'Request owner no longer exists' - : 'Request is no longer awaiting review'); + : 'Request state changed since review'); this.name = 'RequestReviewPreconditionError'; this.reason = reason; } @@ -146,7 +146,7 @@ export async function resolveReviewedRequest( * atomic backend precondition, but it prevents a cached detail/list from * blindly resolving a request that has already changed state. */ -export async function fetchAwaitingReviewTarget(reqType: RequestReviewType, reqId: number) { +export async function fetchReviewTarget(reqType: RequestReviewType, reqId: number, expectedState: string) { const loaded = reqType === 'registration' ? (await fetchRegistrationRequest(reqId)).data : (await fetchChangeRequest(reqId)).data; @@ -157,8 +157,13 @@ export async function fetchAwaitingReviewTarget(reqType: RequestReviewType, reqI if (requestMissingRequiredUser(reqType, loaded)) { throw new RequestReviewPreconditionError('owner_missing'); } - if (!requestMatchesReviewTarget(loaded, reqType, reqId, 'awaiting')) { + if (!requestMatchesReviewTarget(loaded, reqType, reqId, expectedState)) { throw new RequestReviewPreconditionError('state_changed'); } return loaded; } + +/** Queue and bulk review remain restricted to awaiting requests. */ +export async function fetchAwaitingReviewTarget(reqType: RequestReviewType, reqId: number) { + return fetchReviewTarget(reqType, reqId, 'awaiting'); +} diff --git a/src/pages/app/admin/RequestReviewActions.behavior.test.tsx b/src/pages/app/admin/RequestReviewActions.behavior.test.tsx index 7da99f88..78296f67 100644 --- a/src/pages/app/admin/RequestReviewActions.behavior.test.tsx +++ b/src/pages/app/admin/RequestReviewActions.behavior.test.tsx @@ -5,7 +5,7 @@ import userEvent from '@testing-library/user-event'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { RequestReviewActions } from './RequestReviewActions'; -import { fetchAwaitingReviewTarget, resolveReviewedRequest } from './RequestResolveMutation'; +import { fetchReviewTarget, resolveReviewedRequest } from './RequestResolveMutation'; const acquireLocalLock = vi.fn(); const settleLocalLock = vi.fn(); @@ -34,7 +34,7 @@ vi.mock('./RequestResolveMutation', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - fetchAwaitingReviewTarget: vi.fn(), + fetchReviewTarget: vi.fn(), resolveReviewedRequest: vi.fn(), }; }); @@ -54,7 +54,7 @@ vi.mock('./RequestResolveResources', () => ({ }), })); -const fetchTargetMock = vi.mocked(fetchAwaitingReviewTarget); +const fetchTargetMock = vi.mocked(fetchReviewTarget); const resolveMock = vi.mocked(resolveReviewedRequest); describe('RequestReviewActions behavior', () => { @@ -94,7 +94,7 @@ describe('RequestReviewActions behavior', () => { 'ignore', expect.objectContaining({ reason: undefined, approveCreateVps: false, approveActivate: false }), ); - expect(fetchTargetMock).toHaveBeenCalledWith('registration', 19327); + expect(fetchTargetMock).toHaveBeenCalledWith('registration', 19327, 'awaiting'); expect(acquireLocalLock).toHaveBeenCalledTimes(1); }); }); diff --git a/src/pages/app/admin/RequestReviewActions.test.ts b/src/pages/app/admin/RequestReviewActions.test.ts index dd78ea02..6340f8e9 100644 --- a/src/pages/app/admin/RequestReviewActions.test.ts +++ b/src/pages/app/admin/RequestReviewActions.test.ts @@ -10,12 +10,18 @@ import { } from './RequestReviewModel'; describe('requestReviewActions', () => { - it('fails closed once a request is no longer awaiting review', () => { - expect(requestReviewActions('registration', { id: 1, state: 'approved' }, true)).toEqual([]); - expect(requestReviewActions('registration', { id: 2, state: 'denied' }, true)).toEqual([]); - expect(requestReviewActions('registration', { id: 3, state: 'ignored' }, true)).toEqual([]); - expect(requestReviewActions('registration', { id: 4, state: 'pending_correction' }, true)).toEqual([]); + it.each([ + ['approved', ['deny', 'ignore', 'request_correction']], + ['denied', ['approve', 'ignore', 'request_correction']], + ['ignored', ['approve', 'deny', 'request_correction']], + ['pending_correction', ['approve', 'deny', 'ignore']], + ])('allows revisiting %s registrations without repeating the same state', (state, actions) => { + expect(requestReviewActions('registration', { id: 1, state }, true)).toEqual(actions); + }); + + it('fails closed for unknown states', () => { expect(requestReviewActions('registration', { id: 5 }, true)).toEqual([]); + expect(requestReviewActions('registration', { id: 5, state: 'unknown' }, true)).toEqual([]); }); it('does not strand change requests in a correction state applicants cannot resubmit', () => { @@ -44,7 +50,7 @@ describe('requestReviewActions', () => { expect(requestReviewActions('change', { ...orphan, user: { id: Number.MAX_SAFE_INTEGER + 1 } }, true)).toEqual([]); }); - it('offers the complete registration decision set only while awaiting', () => { + it('offers the complete registration decision set while awaiting', () => { expect(requestReviewActions('registration', { id: 6, state: 'awaiting' }, true)).toEqual([ 'approve', 'deny', diff --git a/src/pages/app/admin/RequestReviewActions.tsx b/src/pages/app/admin/RequestReviewActions.tsx index c5469d5b..36e12cc5 100644 --- a/src/pages/app/admin/RequestReviewActions.tsx +++ b/src/pages/app/admin/RequestReviewActions.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useMemo, useState } from 'react'; +import React, { useEffect, useMemo, useRef, useState } from 'react'; import { Link } from 'react-router-dom'; import { useQueryClient } from '@tanstack/react-query'; @@ -21,7 +21,7 @@ import { RequestResolveReview } from './RequestResolveReview'; import { RequestResolveOverridesForm } from './RequestResolveOverridesForm'; import { RequestMutationUncertainty } from './RequestMutationUncertainty'; import { - fetchAwaitingReviewTarget, + fetchReviewTarget, invalidNumericResolveOverrideKeys, RequestReviewPreconditionError, resolveReviewedRequest, @@ -121,6 +121,7 @@ export function RequestReviewActions(props: { ); const ownerMissing = requestMissingRequiredUser(props.reqType, props.request); const historicalUserId = safePositiveInteger(String(props.request.raw_user_id ?? '')); + const reviewedState = useRef(String(props.request.state ?? '').trim()); const [resolveOpen, setResolveOpen] = useState(false); const [overridesOpen, setOverridesOpen] = useState(false); const [resolveAction, setResolveAction] = useState('approve'); @@ -169,6 +170,7 @@ export function RequestReviewActions(props: { }, [approveNode, resources.nodes]); function openAction(action: ResolveUserRequestAction) { + reviewedState.current = String(props.request.state ?? '').trim(); const nextOverrides = action === 'approve' || action === 'request_correction' ? requestOverrides(props.reqType, props.request) @@ -215,7 +217,7 @@ export function RequestReviewActions(props: { let settleError: unknown; try { - await fetchAwaitingReviewTarget(props.reqType, props.reqId); + await fetchReviewTarget(props.reqType, props.reqId, reviewedState.current); mutationGeneration = await chrome.acquireLocalLock(requestRef, { durable: true }); mutationStarted = true; const res = await resolveReviewedRequest(props.reqType, props.reqId, action, options); diff --git a/src/pages/app/admin/RequestReviewModel.ts b/src/pages/app/admin/RequestReviewModel.ts index fe02616d..5c02706d 100644 --- a/src/pages/app/admin/RequestReviewModel.ts +++ b/src/pages/app/admin/RequestReviewModel.ts @@ -92,13 +92,18 @@ export function requestReviewActions( if (!isAdmin || !request) return []; if (requestMissingRequiredUser(reqType, request)) return []; const state = String(request.state ?? '').trim(); - if (state !== 'awaiting') return []; + const knownStates = ['awaiting', 'approved', 'denied', 'ignored', 'pending_correction']; + if (!knownStates.includes(state)) return []; + if (reqType === 'change' && state !== 'awaiting') return []; const actions: ResolveUserRequestAction[] = ['approve', 'deny', 'ignore']; if (reqType === 'registration') { actions.push('request_correction'); } - return actions; + const targetStates: Record = { + approve: 'approved', deny: 'denied', ignore: 'ignored', request_correction: 'pending_correction', + }; + return actions.filter((action) => targetStates[action] !== state); } /** Bulk review is intentionally narrower: registrations still require detail review before approval. */ @@ -107,6 +112,7 @@ export function requestBulkReviewActions( request: ReviewableRequest | undefined, isAdmin: boolean, ): ResolveUserRequestAction[] { + if (request?.state !== 'awaiting') return []; return requestReviewActions(reqType, request, isAdmin).filter( (action) => !(reqType === 'registration' && action === 'approve'), );