feat(salary): allow recalling approval on a salary run (approved → review) (#894)

* feat(salary): allow recalling approval on a salary run (approved → review) An approved run was a dead end: the only forward path was paid → booked, so a wrong salary snapshot (e.g. stale employee monthly pay) could not be fixed without paying and then storno-correcting. Approval is an internal control point — nothing legally binding happens until payment, booking, or AGI filing — so recalling it is allowed until the AGI reaches Skatteverket. - POST /api/salary/runs/[id]/unapprove: approved → review; clears approved_by/at and payment-file tracking; deletes generated-but-unfiled AGI declarations (stale XML must not stay exportable); 409 once the AGI is pending_signature/submitted/accepted — correction AGI (same specifikationsnummer) is the lawful path then. - New salary_run.approval_reverted event for the audit trail. - "Ångra godkännande" secondary action on the run page with a consequence-aware confirm (payment file possibly at the bank, sent payslips, generated AGI), sv + en. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(salary): delete stale AGI after the unapprove transition, not before Bot-review triage on #894: the declaration delete ran before the optimistic status update, so a failed transition (concurrent flip, transient error) would have destroyed the generated AGI while the run stayed approved. Flip the run first; a delete failure afterwards is harmless (agi_generated_at is already null, regeneration upserts over the orphan). Also record the deleted declaration id in the approval_reverted event payload, and warn in the confirm dialog that a manually filed AGI requires a correction declaration instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(salary): close the unapprove TOCTOU on concurrent AGI filing Superagent P2 + compliance-bot round 2 on #894: AGI submission is allowed from approved (also out-of-band via MCP/public API), so a filing could land between the route's read and its update, and the route would flip the run and delete a submitted declaration. - Re-assert agi_submitted_at IS NULL inside the optimistic update filter, not just on the stale read. - Guard the declaration delete with the same status filter so it no-ops if the declaration advanced since the read; log a miss. - Zero-row update (PGRST116) now returns 409 "status har ändrats" instead of a generic 500. - The approval_reverted event only reports deletedAgiDeclarationId when a row was actually deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-05 19:44:22 +02:00
parent 2c2743eb79
commit c43c4a076c
8 changed files with 381 additions and 0 deletions
@@ -11,3 +11,5 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. Appended by agents and
 [2026-07-03] VAT view auto-fetches on period change and drops the "Hämta" button; fetch state is derived from a key-tagged result object instead of setLoading/setError in the effect: keeps react-hooks/set-state-in-effect ratchet at baseline (repo gate is per-rule count).
 [2026-07-03] Added ReportDescriptor.standalone (only vat-declaration) to hide the report-shell back link + fiscal-year selector, instead of changing behavior for all params:'calendar' reports: periodisk-sammanstallning keeps its current shell; scoped diff.
 [2026-07-03] New user-facing strings on skattekonto follow that file's existing hardcoded-Swedish convention; the deadlines callout uses next-intl (page already translated). Year-end stays Swedish per .claude/rules/i18n.md.
+[2026-07-05] Salary run "Ångra godkännande" transitions approved → review (not straight to draft) and hard-deletes generated-but-unfiled AGI declarations — symmetric with the approve step for a clean audit trail, and stale AGI XML must not stay exportable. Blocked with 409 once the AGI is pending_signature/submitted/accepted: the lawful path is then a correction AGI with the same specifikationsnummer. Payment-file tracking is cleared; whether the file reached the bank is outside app knowledge, so the UI confirm makes the user own that check.
+[2026-07-05] PR #894 bot triage: accepted the delete-after-update reorder (destructive op last) and the manual-filing warning in confirm_unapprove_agi; declined soft-cancel status for unfiled AGI drafts and preserving approved_by on recall — a never-filed generated AGI is regenerable working data derived entirely from retained run data (not räkenskapsinformation; unapprove 409s once anything is filed), and the approval with legal weight is the one in force at booking, which unapprove can never touch (paid/booked runs are locked out).
@@ -202,6 +202,21 @@ export default function SalaryRunPage({ params }: { params: Promise<{ id: string
    setActionLoading(null)
  }

+  // Recall an approval (approved → review). Approval is only an internal
+  // control point, but side effects may already exist — a payment file that
+  // could be sitting at the bank, sent payslips, a generated AGI — so the
+  // confirm spells out exactly the ones that apply to this run. The API
+  // refuses outright once the AGI has been filed with Skatteverket.
+  function handleUnapprove() {
+    if (!run) return
+    const lines = [t('confirm_unapprove_intro')]
+    if (run.payment_file_generated_at) lines.push(t('confirm_unapprove_payment_file'))
+    if ((run.payslip_deliveries_summary?.sent ?? 0) > 0) lines.push(t('confirm_unapprove_payslips'))
+    if (run.agi_generated_at) lines.push(t('confirm_unapprove_agi'))
+    if (!confirm(lines.join('\n\n'))) return
+    handleAction('unapprove')
+  }
+
  async function handleDelete() {
    if (!run) return
    const period = periodLabelOf(run)
@@ -526,6 +541,7 @@ export default function SalaryRunPage({ params }: { params: Promise<{ id: string
        primaryAction={primaryAction}
        onPreview={handlePreview}
        onRevert={() => handleAction('revert')}
+        onUnapprove={handleUnapprove}
        onSendPayslips={handleSendPayslips}
        onDownloadPayslips={handleBulkPayslipDownload}
      />
@@ -0,0 +1,199 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import { NextResponse } from 'next/server'
+import {
+  createQueuedMockSupabase,
+  createMockRequest,
+  parseJsonResponse,
+  createMockRouteParams,
+} from '@/tests/helpers'
+
+// The route is wrapped in withRouteContext (auth via requireAuth, company via
+// getActiveCompanyId, write gate via requireWritePermission) — mock those.
+vi.mock('@/lib/init', () => ({ ensureInitialized: vi.fn() }))
+vi.mock('@/lib/auth/require-auth', () => ({ requireAuth: vi.fn() }))
+vi.mock('@/lib/company/context', () => ({
+  getActiveCompanyId: vi.fn().mockResolvedValue('company-1'),
+}))
+vi.mock('@/lib/auth/require-write', () => ({
+  requireWritePermission: vi.fn().mockResolvedValue({ ok: true }),
+}))
+vi.mock('@/lib/events', () => ({ eventBus: { emit: vi.fn().mockResolvedValue(undefined) } }))
+
+import { POST } from '../route'
+import { requireAuth } from '@/lib/auth/require-auth'
+import { eventBus } from '@/lib/events'
+
+const mockUser = { id: 'user-1', email: 'test@test.se' }
+
+function authed(supabase: unknown) {
+  vi.mocked(requireAuth).mockResolvedValue({
+    user: mockUser as never,
+    supabase: supabase as never,
+    error: null,
+  })
+}
+
+const approvedRun = {
+  id: 'run-1',
+  company_id: 'company-1',
+  status: 'approved',
+  agi_submitted_at: null,
+}
+
+describe('POST /api/salary/runs/[id]/unapprove', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it('returns 401 when not authenticated', async () => {
+    vi.mocked(requireAuth).mockResolvedValue({
+      user: null as never,
+      supabase: {} as never,
+      error: NextResponse.json({ error: 'Unauthorized' }, { status: 401 }),
+    })
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status } = await parseJsonResponse(response)
+
+    expect(status).toBe(401)
+  })
+
+  it('returns 404 when the salary run is not found', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    authed(supabase)
+
+    enqueueMany([
+      { data: null, error: { message: 'Not found' } }, // salary_runs lookup
+    ])
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status, body } = await parseJsonResponse<{ error: string }>(response)
+
+    expect(status).toBe(404)
+    expect(body.error).toContain('hittades inte')
+  })
+
+  it('returns 400 when the run is not approved (e.g. already paid)', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    authed(supabase)
+
+    enqueueMany([
+      { data: { ...approvedRun, status: 'paid' } }, // salary_runs lookup
+    ])
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status, body } = await parseJsonResponse<{ error: string }>(response)
+
+    expect(status).toBe(400)
+    expect(body.error).toContain('godkänd')
+  })
+
+  it('returns 409 when the AGI declaration has been submitted to Skatteverket', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    authed(supabase)
+
+    enqueueMany([
+      { data: approvedRun },                              // salary_runs lookup
+      { data: { id: 'agi-1', status: 'submitted' } },     // agi_declarations lookup
+    ])
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status, body } = await parseJsonResponse<{ error: string }>(response)
+
+    expect(status).toBe(409)
+    expect(body.error).toContain('Skatteverket')
+    expect(eventBus.emit).not.toHaveBeenCalled()
+  })
+
+  it('returns 409 when the run itself is stamped agi_submitted_at', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    authed(supabase)
+
+    enqueueMany([
+      { data: { ...approvedRun, agi_submitted_at: '2026-07-01T10:00:00Z' } }, // run lookup
+      { data: null, error: { message: 'No rows' } },                          // agi lookup
+    ])
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status } = await parseJsonResponse(response)
+
+    expect(status).toBe(409)
+  })
+
+  it('returns 409 when the run transitions concurrently between read and update', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    authed(supabase)
+
+    enqueueMany([
+      { data: approvedRun },                            // salary_runs lookup
+      { data: null, error: { message: 'No rows' } },    // agi_declarations lookup
+      { data: null, error: { code: 'PGRST116', message: 'no rows returned' } }, // update matched 0 rows
+    ])
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status, body } = await parseJsonResponse<{ error: string }>(response)
+
+    expect(status).toBe(409)
+    expect(body.error).toContain('ändrats')
+    expect(eventBus.emit).not.toHaveBeenCalled()
+  })
+
+  it('reverts an approved run to review and emits the event', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    authed(supabase)
+
+    enqueueMany([
+      { data: approvedRun },                          // salary_runs lookup
+      { data: null, error: { message: 'No rows' } },  // agi_declarations lookup (none)
+      { data: { id: 'run-1', status: 'review' } },    // salary_runs update
+    ])
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status, body } = await parseJsonResponse<{ data: { status: string } }>(response)
+
+    expect(status).toBe(200)
+    expect(body.data.status).toBe('review')
+    expect(eventBus.emit).toHaveBeenCalledWith({
+      type: 'salary_run.approval_reverted',
+      payload: {
+        salaryRunId: 'run-1',
+        revertedBy: 'user-1',
+        deletedAgiDeclarationId: null,
+        userId: 'user-1',
+        companyId: 'company-1',
+      },
+    })
+  })
+
+  it('deletes a generated (unfiled) AGI declaration after reverting', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    authed(supabase)
+
+    enqueueMany([
+      { data: approvedRun },                          // salary_runs lookup
+      { data: { id: 'agi-1', status: 'generated' } }, // agi_declarations lookup
+      { data: { id: 'run-1', status: 'review' } },    // salary_runs update
+      { data: [{ id: 'agi-1' }] },                    // agi_declarations delete
+    ])
+
+    const request = createMockRequest('/api/salary/runs/run-1/unapprove', { method: 'POST' })
+    const response = await POST(request, createMockRouteParams({ id: 'run-1' }))
+    const { status, body } = await parseJsonResponse<{ data: { status: string } }>(response)
+
+    expect(status).toBe(200)
+    expect(body.data.status).toBe('review')
+    expect(eventBus.emit).toHaveBeenCalledWith(
+      expect.objectContaining({
+        type: 'salary_run.approval_reverted',
+        payload: expect.objectContaining({ deletedAgiDeclarationId: 'agi-1' }),
+      }),
+    )
+  })
+})
@@ -0,0 +1,139 @@
+import { NextResponse } from 'next/server'
+import { ensureInitialized } from '@/lib/init'
+import { withRouteContext } from '@/lib/api/with-route-context'
+import { eventBus } from '@/lib/events'
+
+ensureInitialized()
+
+/**
+ * approved → review (recall approval, unlock the run for recalculation).
+ *
+ * Approval is an internal control point — nothing legally binding has happened
+ * until payment, booking, or AGI filing — so recalling it is allowed as long
+ * as the AGI has not reached Skatteverket. Once the AGI is in flight
+ * (pending_signature) or filed (submitted/accepted), the period must instead
+ * be redone via a correction AGI with the same specifikationsnummer, so this
+ * route refuses.
+ */
+export const POST = withRouteContext<{ params: Promise<{ id: string }> }>(
+  'salary.run.unapprove',
+  async (_request, { supabase, companyId, user, log }, { params }) => {
+    const { id } = await params
+
+    const { data: run, error: runError } = await supabase
+      .from('salary_runs')
+      .select('*')
+      .eq('id', id)
+      .eq('company_id', companyId)
+      .single()
+
+    if (runError || !run) {
+      return NextResponse.json({ error: 'Lönekörning hittades inte' }, { status: 404 })
+    }
+
+    if (run.status !== 'approved') {
+      return NextResponse.json(
+        { error: 'Bara en godkänd lönekörning kan låsas upp. En betald eller bokförd körning korrigeras via korrigeringsflödet.' },
+        { status: 400 },
+      )
+    }
+
+    const { data: agiDeclaration } = await supabase
+      .from('agi_declarations')
+      .select('id, status')
+      .eq('company_id', companyId)
+      .eq('salary_run_id', id)
+      .single()
+
+    if (
+      run.agi_submitted_at ||
+      ['pending_signature', 'submitted', 'accepted'].includes(agiDeclaration?.status ?? '')
+    ) {
+      return NextResponse.json(
+        { error: 'AGI har redan skickats till Skatteverket för denna period. Ändra genom att lämna in en korrigerad AGI (samma specifikationsnummer) i stället.' },
+        { status: 409 },
+      )
+    }
+
+    // Clear payment-file tracking too: a previously generated file would show
+    // as current after re-approval even though the amounts may change. Whether
+    // the file already reached the bank is outside the app's knowledge — the
+    // UI makes the user confirm that before calling this route.
+    const { data: updatedRun, error } = await supabase
+      .from('salary_runs')
+      .update({
+        status: 'review',
+        approved_by: null,
+        approved_at: null,
+        agi_generated_at: null,
+        payment_file_format: null,
+        payment_file_generated_at: null,
+      })
+      .eq('id', id)
+      .eq('company_id', companyId)
+      .eq('status', 'approved')
+      // TOCTOU guard: AGI submission is allowed from `approved` (also out of
+      // band via MCP / the public API), so a filing may have landed since the
+      // read above. Re-assert it hasn't inside the update filter.
+      .is('agi_submitted_at', null)
+      .select()
+      .single()
+
+    if (error || !updatedRun) {
+      // Zero rows matched (PGRST116): the run moved on concurrently — marked
+      // paid, or the AGI was filed — between the read and this update. Not a
+      // server fault; tell the user to reload instead of returning 500.
+      if ((error as { code?: string } | null)?.code === 'PGRST116') {
+        return NextResponse.json(
+          { error: 'Lönekörningens status har ändrats — ladda om sidan och försök igen.' },
+          { status: 409 },
+        )
+      }
+      return NextResponse.json({ error: 'Kunde inte återkalla godkännandet' }, { status: 500 })
+    }
+
+    // A generated-but-unfiled AGI now carries stale amounts — delete it so the
+    // stale XML can't be exported. Deliberately after the status flip: the
+    // reverse order could destroy the declaration and then fail the
+    // transition, leaving an approved run with its AGI gone. If this delete
+    // misses instead, agi_generated_at is already null and regeneration on the
+    // forward path upserts over the orphaned row. The status filter makes the
+    // delete a no-op if the declaration advanced (e.g. to pending_signature)
+    // since the read. A rejected declaration is kept: it documents the
+    // rejection.
+    const staleAgi =
+      agiDeclaration && ['generated', 'exported'].includes(agiDeclaration.status)
+        ? agiDeclaration
+        : null
+    let deletedAgiDeclarationId: string | null = null
+    if (staleAgi) {
+      const { data: deletedRows, error: deleteError } = await supabase
+        .from('agi_declarations')
+        .delete()
+        .eq('id', staleAgi.id)
+        .in('status', ['generated', 'exported'])
+        .select('id')
+      deletedAgiDeclarationId = deletedRows?.length ? staleAgi.id : null
+      if (deleteError) {
+        log.warn('stale AGI declaration delete failed', {
+          agiDeclarationId: staleAgi.id,
+          error: deleteError.message,
+        })
+      }
+    }
+
+    await eventBus.emit({
+      type: 'salary_run.approval_reverted',
+      payload: {
+        salaryRunId: id,
+        revertedBy: user.id,
+        deletedAgiDeclarationId,
+        userId: user.id,
+        companyId,
+      },
+    })
+
+    return NextResponse.json({ data: updatedRun })
+  },
+  { requireWrite: true },
+)
@@ -24,6 +24,7 @@ interface RunProgressBarProps {
  // obligation. (Recalculate lives with the employee rows.)
  onPreview: () => void
  onRevert: () => void
+  onUnapprove: () => void
  onSendPayslips: () => void
  onDownloadPayslips: () => void
 }
@@ -178,6 +179,19 @@ export function RunProgressBar(props: RunProgressBarProps) {
          </Button>
        </>
      )
+    } else if (run.status === 'approved') {
+      // Approval is an internal control point, not a legal event — the run can
+      // be unlocked again as long as nothing has been paid, booked, or filed.
+      // The API refuses once the AGI has reached Skatteverket.
+      secondaryActions = (
+        <>
+          {payslipActions}
+          <Button variant="ghost" size="sm" onClick={props.onUnapprove} disabled={busy}>
+            <ArrowLeftCircle className="mr-2 h-4 w-4" />
+            {t('action_unapprove')}
+          </Button>
+        </>
+      )
    } else if (payslipsAvailable) {
      secondaryActions = payslipActions
    }
@@ -123,6 +123,7 @@ export type CoreEvent =
  // Salary
  | { type: 'salary_run.created'; payload: { salaryRunId: string; periodYear: number; periodMonth: number; userId: string; companyId: string } }
  | { type: 'salary_run.approved'; payload: { salaryRunId: string; approvedBy: string; userId: string; companyId: string } }
+  | { type: 'salary_run.approval_reverted'; payload: { salaryRunId: string; revertedBy: string; deletedAgiDeclarationId: string | null; userId: string; companyId: string } }
  | { type: 'salary_run.booked'; payload: { salaryRunId: string; entryIds: string[]; userId: string; companyId: string } }
  | { type: 'agi.generated'; payload: { agiId: string; periodYear: number; periodMonth: number; userId: string; companyId: string } }
  | { type: 'agi.submitted'; payload: { salaryRunId: string; periodYear: number; periodMonth: number; userId: string; companyId: string } }
@@ -4478,6 +4478,11 @@
    "approve_override_cancel": "Cancel",
    "approve_override_confirm": "Approve anyway",
    "action_revert": "Back to draft",
+    "action_unapprove": "Recall approval",
+    "confirm_unapprove_intro": "The approval is recalled and the run goes back to review.",
+    "confirm_unapprove_payment_file": "A payment file has been generated. If it has already been uploaded to the bank, the payment must be cancelled there — otherwise the old amounts will be paid out.",
+    "confirm_unapprove_payslips": "Payslips have already been sent and need to be re-sent after recalculation.",
+    "confirm_unapprove_agi": "The generated AGI file is deleted and needs to be regenerated. If you have already filed it manually with Skatteverket, file a corrected employer declaration instead.",
    "action_mark_paid": "Mark as paid",
    "action_continue": "Continue",
    "action_book": "Post",
@@ -4478,6 +4478,11 @@
    "approve_override_cancel": "Avbryt",
    "approve_override_confirm": "Godkänn ändå",
    "action_revert": "Tillbaka till utkast",
+    "action_unapprove": "Ångra godkännande",
+    "confirm_unapprove_intro": "Godkännandet återkallas och körningen går tillbaka till granskning.",
+    "confirm_unapprove_payment_file": "En betalfil har genererats. Om den redan laddats upp till banken måste betalningen makuleras där — annars betalas de gamla beloppen ut.",
+    "confirm_unapprove_payslips": "Lönebesked har redan skickats och behöver skickas om efter ny beräkning.",
+    "confirm_unapprove_agi": "Den genererade AGI-filen raderas och behöver genereras om. Har du redan lämnat in den manuellt hos Skatteverket ska du i stället lämna en korrigerad arbetsgivardeklaration.",
    "action_mark_paid": "Markera som betald",
    "action_continue": "Fortsätt",
    "action_book": "Bokför",