Skip to content

Commit ba0054d

Browse files
committed
fix(org): keep retention's list on the server's document after a save
The form re-based its never-edited draft onto what it had sent, which left it dirty for the mount: later server values were held back in a conflict nothing showed, and a policy whose baseline no longer matched the server stopped saving with no message. The form now reads the query cache, which a save fills from the route's answer. A changed policy says so at save, and a blank or invalid custom pattern blocks the save instead of being stripped on the way out.
1 parent 2733686 commit ba0054d

6 files changed

Lines changed: 340 additions & 29 deletions

File tree

‎apps/sim/ee/data-retention/hooks/data-retention.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ export function useUpdateOrganizationRetention() {
5454
params: { id: orgId },
5555
body: settings,
5656
}),
57+
onSuccess: ({ data }, { orgId }) => {
58+
queryClient.setQueryData(dataRetentionKeys.settings(orgId), data)
59+
},
5760
onSettled: (_data, _error, { orgId }) => {
5861
queryClient.invalidateQueries({ queryKey: dataRetentionKeys.settings(orgId) })
5962
},

‎apps/sim/lib/guardrails/custom-patterns.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ export const MAX_CUSTOM_PII_PATTERNS = 20
99
* removed rather than kept.
1010
*/
1111
export function customPatternError(regex: string): string | undefined {
12-
const validation = regex.length > 0 ? validateRegexPattern(regex) : null
13-
return validation && !validation.valid ? validation.error : undefined
12+
if (regex.length === 0) return 'Enter a regular expression, or remove this pattern'
13+
const validation = validateRegexPattern(regex)
14+
return validation.valid ? undefined : validation.error
1415
}
Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
/** @vitest-environment jsdom */
2+
3+
import { act, type ReactNode } from 'react'
4+
import { flushMacrotask } from '@sim/testing/helpers/async'
5+
import {
6+
apiClientRequestMock,
7+
apiClientRequestMockFns,
8+
} from '@sim/testing/mocks/api-client-request.mock'
9+
import { stubResizeObserver } from '@sim/testing/mocks/browser-apis.mock'
10+
import { emcnMock, emcnMockFns } from '@sim/testing/mocks/emcn.mock'
11+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
12+
import { createRoot, type Root } from 'react-dom/client'
13+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
14+
import {
15+
getOrganizationDataRetentionContract,
16+
type UpdateOrganizationDataRetentionBody,
17+
} from '@/lib/api/contracts/organization'
18+
import type { PolicyDraft } from '@/ee/data-retention/lib/policy-draft'
19+
20+
vi.mock('@sim/emcn', () => emcnMock)
21+
vi.mock('@/lib/api/client/request', () => apiClientRequestMock)
22+
vi.mock('@/hooks/queries/workspace', () => ({
23+
useWorkspacesQuery: () => ({ data: [], isPending: false }),
24+
}))
25+
vi.mock(
26+
'@/platform/app/(shell)/settings/_components/sections/data-retention/policy-detail',
27+
() => ({
28+
PolicyDetail: ({
29+
draft,
30+
changed,
31+
onChange,
32+
onSave,
33+
}: {
34+
draft: PolicyDraft
35+
changed: boolean
36+
onChange: (draft: PolicyDraft) => void
37+
onSave: () => void
38+
}): ReactNode => (
39+
<div>
40+
<output>{draft.logDays}</output>
41+
<button type='button' onClick={() => onChange({ ...draft, logDays: '7' })}>
42+
keep logs 7 days
43+
</button>
44+
<button type='button' onClick={() => onChange({ ...draft, logDays: '14' })}>
45+
keep logs 14 days
46+
</button>
47+
<button type='button' disabled={!changed} onClick={onSave}>
48+
Save
49+
</button>
50+
</div>
51+
),
52+
})
53+
)
54+
55+
import type { SettingsSectionProps } from '@/platform/app/(shell)/settings/_components/section-registry'
56+
import DataRetentionSection from '@/platform/app/(shell)/settings/_components/sections/data-retention/data-retention-section'
57+
58+
const days = (count: number) => count * 24
59+
60+
function retentionDocument(logDays: number) {
61+
const hours = {
62+
logRetentionHours: days(logDays),
63+
softDeleteRetentionHours: days(30),
64+
taskCleanupHours: days(30),
65+
}
66+
return {
67+
isEnterprise: true,
68+
configured: { ...hours, retentionOverrides: [], piiRedaction: { rules: [] } },
69+
effective: { ...hours, retentionOverrides: [], piiRedaction: { rules: [] } },
70+
}
71+
}
72+
73+
const ORGANIZATION = {
74+
organization: { id: 'org-1' },
75+
deployment: { billingEnabled: false },
76+
} as unknown as SettingsSectionProps['organization']
77+
78+
/** The saved document on the far side of the real query and mutation hooks. */
79+
const server = {
80+
document: retentionDocument(30),
81+
writes: [] as UpdateOrganizationDataRetentionBody[],
82+
/** What the route makes of a write; by default it stores what it was sent. */
83+
answer: (body: UpdateOrganizationDataRetentionBody) =>
84+
retentionDocument((body.logRetentionHours ?? 0) / 24),
85+
read: async (): Promise<unknown> => ({ success: true, data: server.document }),
86+
}
87+
88+
describe('DataRetentionSection across saves', () => {
89+
let container: HTMLDivElement
90+
let root: Root
91+
let queryClient: QueryClient
92+
93+
const click = (name: string) =>
94+
act(async () => {
95+
const target = Array.from(container.querySelectorAll('button')).find((node) =>
96+
node.textContent?.includes(name)
97+
)
98+
if (!target) throw new Error(`No button named ${name}: ${container.textContent}`)
99+
target.click()
100+
await flushMacrotask()
101+
})
102+
103+
/** Query results reach React on a timer, after the read itself has finished. */
104+
async function settle() {
105+
await act(async () => {
106+
await vi.waitFor(() => expect(queryClient.isFetching()).toBe(0))
107+
await flushMacrotask()
108+
})
109+
}
110+
111+
async function saveLogs(logDays: 7 | 14) {
112+
await click('Organization')
113+
await click(`keep logs ${logDays} days`)
114+
await click('Save')
115+
await settle()
116+
}
117+
118+
beforeEach(async () => {
119+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
120+
stubResizeObserver()
121+
server.document = retentionDocument(30)
122+
server.writes = []
123+
server.answer = (body) => retentionDocument((body.logRetentionHours ?? 0) / 24)
124+
server.read = async () => ({ success: true, data: server.document })
125+
apiClientRequestMockFns.mockRequestJson
126+
.mockReset()
127+
.mockImplementation(
128+
async (contract: unknown, request: { body?: UpdateOrganizationDataRetentionBody }) => {
129+
if (contract === getOrganizationDataRetentionContract) return server.read()
130+
const body = request.body as UpdateOrganizationDataRetentionBody
131+
server.writes.push(body)
132+
server.document = server.answer(body)
133+
return { success: true, data: server.document }
134+
}
135+
)
136+
queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } })
137+
container = window.document.createElement('div')
138+
window.document.body.appendChild(container)
139+
root = createRoot(container)
140+
await act(async () => {
141+
root.render(
142+
<QueryClientProvider client={queryClient}>
143+
<DataRetentionSection organization={ORGANIZATION} subpath={[]} query={{}} />
144+
</QueryClientProvider>
145+
)
146+
})
147+
await settle()
148+
})
149+
150+
afterEach(() => {
151+
act(() => root.unmount())
152+
container.remove()
153+
queryClient.clear()
154+
})
155+
156+
it('adopts what another admin saved after this admin has saved', async () => {
157+
await saveLogs(7)
158+
expect(container.textContent).toContain('Log 7d')
159+
160+
server.document = retentionDocument(60)
161+
await act(async () => {
162+
await queryClient.invalidateQueries()
163+
})
164+
await settle()
165+
166+
expect(container.textContent).toContain('Log 60d')
167+
})
168+
169+
it('says so and writes nothing when a later save finds the policy changed, then saves once chosen', async () => {
170+
await saveLogs(7)
171+
server.document = retentionDocument(60)
172+
173+
await saveLogs(14)
174+
175+
expect(server.writes).toHaveLength(1)
176+
expect(emcnMockFns.mockToast.error).toHaveBeenCalledWith(
177+
'This policy changed while you were editing. Nothing was saved.'
178+
)
179+
expect(container.textContent).toContain('changed while you were editing')
180+
181+
await click('Keep my edits')
182+
await click('Save')
183+
await settle()
184+
185+
expect(server.writes.map((write) => write.logRetentionHours)).toEqual([days(7), days(14)])
186+
})
187+
188+
it('keeps saving a policy whose stored form differs from what was sent', async () => {
189+
server.answer = () => retentionDocument(90)
190+
191+
await saveLogs(7)
192+
expect(container.textContent).toContain('Log 90d')
193+
194+
server.answer = (body) => retentionDocument((body.logRetentionHours ?? 0) / 24)
195+
await saveLogs(14)
196+
197+
expect(server.writes.map((write) => write.logRetentionHours)).toEqual([days(7), days(14)])
198+
expect(emcnMockFns.mockToast.error).not.toHaveBeenCalled()
199+
})
200+
201+
it('shows the route answer without waiting on the read that follows the save', async () => {
202+
server.answer = () => retentionDocument(90)
203+
await click('Organization')
204+
await click('keep logs 7 days')
205+
const pendingRead = new Promise(() => {})
206+
const firstRead = server.read
207+
let reads = 0
208+
server.read = () => (++reads === 1 ? firstRead() : pendingRead)
209+
210+
await click('Save')
211+
212+
expect(container.textContent).toContain('Log 90d')
213+
})
214+
})

‎apps/sim/platform/app/(shell)/settings/_components/sections/data-retention/data-retention-section.tsx‎

Lines changed: 10 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
import { useState } from 'react'
44
import { toast } from '@sim/emcn'
55
import { toError } from '@sim/utils/errors'
6-
import { compareStrings } from '@sim/utils/string'
76
import { useSettingsUnsavedGuard } from '@/components/settings/use-settings-unsaved-guard'
87
import { normalizeRuleStages } from '@/lib/guardrails/pii-entities'
98
import {
@@ -82,23 +81,6 @@ function policyDraftOf(snapshot: RetentionSnapshot, selection: PolicySelection):
8281
const samePolicy = (left: PolicyDraft, right: PolicyDraft) =>
8382
normalizePolicyDraft(left) === normalizePolicyDraft(right)
8483

85-
const sameSnapshot = (left: RetentionSnapshot, right: RetentionSnapshot) =>
86-
JSON.stringify(buildComparable(left)) === JSON.stringify(buildComparable(right))
87-
88-
/** Rule ids and array order carry no meaning here, and the order differs across a save. */
89-
function buildComparable(snapshot: RetentionSnapshot) {
90-
const byWorkspace = (left: { workspaceId: string }, right: { workspaceId: string }) =>
91-
compareStrings(left.workspaceId, right.workspaceId)
92-
return {
93-
...snapshot,
94-
defaultPii: snapshot.defaultPii?.stages ?? null,
95-
piiOverrides: [...snapshot.piiOverrides]
96-
.sort(byWorkspace)
97-
.map(({ workspaceId, stages }) => ({ workspaceId, stages })),
98-
overrides: [...snapshot.overrides].sort(byWorkspace),
99-
}
100-
}
101-
10284
interface PolicyEditorProps {
10385
selection: PolicySelection
10486
snapshot: RetentionSnapshot
@@ -147,7 +129,11 @@ function PolicyEditor({
147129
toast.error('Could not confirm the latest retention settings. Nothing was saved.')
148130
return null
149131
}
150-
return samePolicy(policyDraftOf(latest, selection), policy.baseline) ? latest : null
132+
if (!samePolicy(policyDraftOf(latest, selection), policy.baseline)) {
133+
toast.error('This policy changed while you were editing. Nothing was saved.')
134+
return null
135+
}
136+
return latest
151137
} finally {
152138
setChecking(false)
153139
}
@@ -216,20 +202,18 @@ interface RetentionFormProps {
216202
refetch: () => Promise<{ data?: DataRetentionResponse }>
217203
}
218204

219-
/** The whole document is one save: the route replaces each key, so all of it is always sent. */
205+
/**
206+
* The whole document is one save: the route replaces each key, so all of it is always sent. It holds
207+
* no copy of the document: the query cache does, and a save writes the route's answer there.
208+
*/
220209
function RetentionForm({
221210
initialData,
222211
organizationId,
223212
workspaceOptions,
224213
refetch,
225214
}: RetentionFormProps) {
226215
const update = useUpdateOrganizationRetention()
227-
const retention = useServerDraft({
228-
server: snapshotFrom(initialData),
229-
isEqual: sameSnapshot,
230-
saving: update.isPending,
231-
})
232-
const snapshot = retention.baseline
216+
const snapshot = snapshotFrom(initialData)
233217
const [selection, setSelection] = useState<PolicySelection | null>(null)
234218
const workspaceName = (id: string) =>
235219
workspaceOptions.find((option) => option.value === id)?.label ?? 'Unknown workspace'
@@ -241,7 +225,6 @@ function RetentionForm({
241225

242226
async function persist(next: RetentionSnapshot) {
243227
await update.mutateAsync({ orgId: organizationId, settings: buildRetentionUpdate(next) })
244-
retention.markSaved(next, next)
245228
}
246229

247230
if (selection) {

0 commit comments

Comments
 (0)