Allow administrators to reconsider resolved registrations

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);
+});

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');
+  });
+
 });

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');
+}

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<typeof import('./RequestResolveMutation')>();
   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);
   });
 });

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',

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<ResolveUserRequestAction>('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);

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<ResolveUserRequestAction, string> = {
+    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'),
   );