From 5e797d864132e0b3b104137525ed87abe4732428 Mon Sep 17 00:00:00 2001 From: Francisco Solla Date: Wed, 12 Aug 2026 00:58:32 -0300 Subject: [PATCH] =?UTF-8?q?refactor(C128):=20fixes=20do=20/simplify=20?= =?UTF-8?q?=E2=80=94=20locks=20por=20pessoa=20no=20exit=20destrutivo,=20un?= =?UTF-8?q?do-toast=20suprimido,=20guard=20anti-race?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Assessora/StateDeputy: advisory lock por pessoa (person-assessora / person-state-deputy) serializa enumerate→decide→write do exit destrutivo (dois removes concorrentes de chips distintos deixavam zumbi com carteira vazia — P1 do reviewer A); enumeração da carteira usa getAdvisorMunicipalityIds canônico (F1). - RelationChipCell: commitGuard retorna boolean | 'destructive'; o veredicto destrutivo suprime o toast 'Desfazer' do lote (re-add só re-criaria a entidade vazia — votos/conta já foram) — P2 A+B. - PeopleMunicipalityCell: sentinela manifestPending (dois guards concorrentes sobrescreviam o resolver e a 1ª remoção morria em silêncio — P2-1) + cleanup no unmount resolve false (P2-2) + extraFormFields memoizado; minItems órfã removida. - personDelete: bloco batch redundante de autorias removido (o loop com deleteCampaignUserAccount já cobria — 4N DELETEs no-op — F4). - getPersonCapacityExitManifestAction: actor reloaded (papel rebaixado não lê manifest que o write recusaria — P2-A). - Revalidação: setPersonStateDeputyMunicipalities ganha revalidateMunicipalityListPaths (paridade com o twin B37 — F2). - Testes: int +2 (cascata de autorias no exit da Assessora; undo re-cria a liderança) --- src/app/(campaign)/campanha/actions/person.ts | 44 +++++---- .../people/PeopleMunicipalityCell.tsx | 49 +++++++--- .../shared/MunicipalityPortfolioCell.tsx | 2 +- .../campaign/shared/RelationChipCell.tsx | 35 +++++--- src/utilities/people/personDelete.ts | 48 ++-------- .../int/campaignPersonCellEditing.int.spec.ts | 90 +++++++++++++++++++ 6 files changed, 187 insertions(+), 81 deletions(-) diff --git a/src/app/(campaign)/campanha/actions/person.ts b/src/app/(campaign)/campanha/actions/person.ts index 375436e2..79765496 100644 --- a/src/app/(campaign)/campanha/actions/person.ts +++ b/src/app/(campaign)/campanha/actions/person.ts @@ -39,7 +39,7 @@ import { } from '@/lib/schemas/personDelete' import { nextStateDeputyAdvisorIdsAfterMembership } from '@/lib/stateDeputyAdvisorMembership' import type { CampaignUser, Contact, Leadership, StateDeputy } from '@/payload-types' -import { isCampaignUnrestricted } from '@/utilities/campaignAccess' +import { getAdvisorMunicipalityIds, isCampaignUnrestricted } from '@/utilities/campaignAccess' import { getCampaignActionContext, reloadStaffActor, @@ -255,6 +255,14 @@ export const setPersonAssessoraMembershipRecord = async ( async ({ req }) => { await reloadUnrestrictedActor(payload, actor, PERSON_ASSESSORA_UNRESTRICTED_MESSAGE, req) + // Serializes every Assessora write of the SAME person: the destructive + // exit (last municipality) decides "did the carteira empty?" from an + // enumeration, and two concurrent removals of distinct chips would each + // see a non-empty remainder and leave a zombie account with an empty + // carteira — invisible to the lifecycle. The lock makes the + // enumerate→decide→toggle/delete sequence atomic per person. + await acquireTextAdvisoryLocks(payload, req, [`person-assessora:${contactId}`]) + const accounts = await payload.find({ collection: 'campaignUser', where: { contact: { equals: contactId } }, @@ -319,19 +327,9 @@ export const setPersonAssessoraMembershipRecord = async ( // empty the carteira deletes the account (with its authored rows and // assessorado links) — the confirmation dialog listed the manifest. if (!assigned) { - const current = await payload.find({ - collection: 'municipality', - where: { advisors: { contains: accountId } }, - depth: 0, - limit: 0, - pagination: false, - // Intentional bypass: the unrestricted gate above is the - // authorization; this read only enumerates the current carteira - // (docs always carry their id). - overrideAccess: true, - req, - }) - const currentMunicipalityIDs = current.docs.map((doc) => doc.id) + // Canonical "advisors contains account" lookup, same helper the + // advisor scope uses everywhere else. + const currentMunicipalityIDs = await getAdvisorMunicipalityIds(payload, accountId, req) const next = currentMunicipalityIDs.filter((id) => !municipalityIds.includes(id)) if (next.length === 0) { const slugs = await municipalitySlugsOf(payload, req, currentMunicipalityIDs) @@ -524,6 +522,11 @@ export const setPersonStateDeputyMunicipalitiesRecord = async ( async ({ req }) => { const currentActor = await reloadStaffActor(payload, actor, PERSON_CELL_STAFF_MESSAGE, req) + // Same per-person serialization as the Assessora lifecycle: the + // destructive exit (last municipality) decides from an enumeration, and + // concurrent removals of distinct chips would leave a zombie dobradinha. + await acquireTextAdvisoryLocks(payload, req, [`person-state-deputy:${contactId}`]) + const deputies = await payload.find({ collection: 'stateDeputy', where: { contact: { equals: contactId } }, @@ -629,18 +632,27 @@ export const setPersonStateDeputyMunicipalities = async ( revalidatePath('/campanha/dobradinhas', 'page') if (stateDeputyID !== null) revalidatePath(`/campanha/dobradinhas/${stateDeputyID}`, 'page') for (const slug of slugs) revalidatePath(`/campanha/municipios/${slug}`, 'page') + // Same surface as the B37 twin: the list and the B147 v2 render the + // dobradinha chips per município. + revalidateMunicipalityListPaths({ scope: 'list' }) } /** * C128 — read-only preview for the destructive-exit confirmation dialog * (`PeopleCapacityExitDialog`): what the exit removes, verbatim. The same * scope guards the exit write runs inside its transaction — the dialog can - * never describe a destruction the write would refuse. + * never describe a destruction the write would refuse. The actor is reloaded + * fresh (same discipline as every record function): a downgraded or removed + * role must not read the manifest the write would refuse. */ export const getPersonCapacityExitManifestAction = async (input: unknown) => { const data = personCapacityExitSchema.parse(input) const { payload, actor } = await getCampaignActionContext() - return loadPersonCapacityExitManifest(payload, actor, data) + const currentActor = + data.capacity === 'account' + ? await reloadUnrestrictedActor(payload, actor, PERSON_ASSESSORA_UNRESTRICTED_MESSAGE) + : await reloadStaffActor(payload, actor, PERSON_CELL_STAFF_MESSAGE) + return loadPersonCapacityExitManifest(payload, currentActor, data) } /** diff --git a/src/components/campaign/people/PeopleMunicipalityCell.tsx b/src/components/campaign/people/PeopleMunicipalityCell.tsx index 0f3df223..1f9279d1 100644 --- a/src/components/campaign/people/PeopleMunicipalityCell.tsx +++ b/src/components/campaign/people/PeopleMunicipalityCell.tsx @@ -1,6 +1,6 @@ 'use client' -import { useCallback, useRef, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { toast } from 'sonner' import { getPersonCapacityExitManifestAction } from '@/app/(campaign)/campanha/actions/person' @@ -32,8 +32,6 @@ type PeopleMunicipalityCellProps = { municipalityIndex: readonly MunicipalityPortfolioIndexEntry[] /** Ids the actor may ADD (administered carteira for an advisor). */ addableIds?: ReadonlySet - /** Floor the relation enforces server-side — 1 for `leadership.municipalities`. */ - minItems?: number /** Read-only chips (actor may see the relation but not edit it). */ readOnly?: boolean commitAction: ( @@ -72,7 +70,6 @@ export const PeopleMunicipalityCell = ({ municipalityIds, municipalityIndex, addableIds, - minItems, readOnly = false, commitAction, drawerTitle, @@ -85,6 +82,23 @@ export const PeopleMunicipalityCell = ({ /** The pending destructive exit: the dialog is up, the commit waits for it. */ const [exitRequest, setExitRequest] = useState(null) const exitResolverRef = useRef<((confirmed: boolean) => void) | null>(null) + /** A manifest round-trip in flight — a second guard must not orphan the first. */ + const manifestPendingRef = useRef(false) + + /** Stable identity for the FormData extra-field map (memoized per contact). */ + const extraFormFields = useMemo(() => ({ contactId }), [contactId]) + + /** + * Unmounting with the dialog open must settle the pending guard (resolve + * `false` — nothing was confirmed, nothing commits) instead of leaving the + * promise orphaned and the gesture silently lost. + */ + useEffect(() => { + return () => { + exitResolverRef.current?.(false) + exitResolverRef.current = null + } + }, []) const toggleExpand = useCallback((chipKey: string) => { setExpandedKeys((current) => { @@ -126,10 +140,16 @@ export const PeopleMunicipalityCell = ({ * the optimistic apply: a removal that empties the relation pauses the * commit and (for account/leadership) asks the manifest-backed dialog. * Fail-closed: a manifest error aborts the commit — nothing is ever deleted - * without an explicit confirmation. + * without an explicit confirmation. Returns `'destructive'` when the dialog + * confirmed the exit, so the shared cell suppresses the batch-removal undo + * toast — its "Desfazer" would only re-create the destroyed entity empty. */ const commitGuard = useCallback( - async (delta: { changedIds: number[]; assigned: boolean; currentIds: number[] }) => { + async (delta: { + changedIds: number[] + assigned: boolean + currentIds: number[] + }): Promise => { if (!exitMode || delta.assigned) return true const remaining = delta.currentIds.filter((id) => !delta.changedIds.includes(id)) if (remaining.length > 0) return true @@ -138,9 +158,12 @@ export const PeopleMunicipalityCell = ({ // the cleanup is automatic (intention rabbit hole), no dialog. if (exitMode === 'stateDeputy') return true - // A second guard while one dialog is already pending is refused: the - // pending commit must resolve first. - if (exitResolverRef.current) return false + // A second guard while a dialog is pending OR a manifest round-trip is + // in flight is refused: the pending commit must resolve first (two + // concurrent guards would overwrite each other's resolver and orphan + // the first commit silently). + if (exitResolverRef.current || manifestPendingRef.current) return false + manifestPendingRef.current = true let manifest: PersonCapacityExitManifest | null try { @@ -151,6 +174,8 @@ export const PeopleMunicipalityCell = ({ } catch (error) { toast.error(mapManifestError(error)) return false + } finally { + manifestPendingRef.current = false } // Nothing to destroy (no entity) — the server no-ops the removal. if (manifest === null) return true @@ -161,10 +186,11 @@ export const PeopleMunicipalityCell = ({ if (manifest.declaredVoteCount === 0 && manifest.inviteCount === 0) return true } - return new Promise((resolve) => { + const confirmed = await new Promise((resolve) => { exitResolverRef.current = resolve setExitRequest(manifest) }) + return confirmed ? 'destructive' : false }, [exitMode, contactId], ) @@ -183,7 +209,6 @@ export const PeopleMunicipalityCell = ({ municipalityIds={municipalityIds} municipalityIndex={municipalityIndex} addableIds={addableIds} - minItems={minItems} commitAction={commitAction} drawerTitle={drawerTitle} updateErrorMessage={updateErrorMessage} @@ -192,7 +217,7 @@ export const PeopleMunicipalityCell = ({ onChipClick={toggleExpand} overflowToggleLabel={(count) => `+${count}`} readOnly={readOnly} - extraFormFields={{ contactId }} + extraFormFields={extraFormFields} commitWithNullOwner commitGuard={exitMode ? commitGuard : undefined} /> diff --git a/src/components/campaign/shared/MunicipalityPortfolioCell.tsx b/src/components/campaign/shared/MunicipalityPortfolioCell.tsx index 68e3eb40..4e0a66e3 100644 --- a/src/components/campaign/shared/MunicipalityPortfolioCell.tsx +++ b/src/components/campaign/shared/MunicipalityPortfolioCell.tsx @@ -107,7 +107,7 @@ type MunicipalityPortfolioCellProps = { changedIds: number[] assigned: boolean currentIds: number[] - }) => Promise + }) => Promise } /** diff --git a/src/components/campaign/shared/RelationChipCell.tsx b/src/components/campaign/shared/RelationChipCell.tsx index 25f87614..3578b09b 100644 --- a/src/components/campaign/shared/RelationChipCell.tsx +++ b/src/components/campaign/shared/RelationChipCell.tsx @@ -200,14 +200,18 @@ type RelationChipCellProps = { * C128 — destructive-exit guard: called on removals (and additions) BEFORE * the optimistic apply, same point as the floor/cap refusals. Return `false` * to abort the commit without touching the chip state (the wrapper opens its - * confirmation dialog). `currentIds` is the optimistic-aware effective set, - * so the wrapper can decide "does this removal empty the relation?". + * confirmation dialog), `true` to proceed, or `'destructive'` when the + * wrapper confirmed a destructive exit — the caller then suppresses the + * batch-removal undo toast, whose "Desfazer" would only re-create the + * destroyed entity empty (votes/account are gone). `currentIds` is the + * optimistic-aware effective set, so the wrapper can decide "does this + * removal empty the relation?". */ commitGuard?: (delta: { changedIds: number[] assigned: boolean currentIds: number[] - }) => Promise + }) => Promise } /** @@ -520,7 +524,7 @@ export const RelationChipCell = ({ * the same burst — or an "Desfazer" pressed after a later add — would otherwise * each compute from the set their own render captured and drop the other's work. */ - const commit = (changedIds: number[], assigned: boolean) => { + const commit = (changedIds: number[], assigned: boolean, suppressUndoToast = false) => { if (changedIds.length === 0) return if (draft) { @@ -548,8 +552,11 @@ export const RelationChipCell = ({ setFeedback({ kind: 'saved' }) // A batch removal drops several links in one tap, and the row it left // behind carries no trace of what was there. Single removals are one - // search away from being restored and get no toast. - if (!assigned && changedIds.length > 1) { + // search away from being restored and get no toast. C128: a + // destructive exit (the wrapper's guard confirmed the entity dies) + // suppresses the toast — "Desfazer" would only re-create the destroyed + // entity empty. + if (!assigned && changedIds.length > 1 && !suppressUndoToast) { toast.success(copy.removedMessage(changedIds.length), { action: { label: 'Desfazer', onClick: () => commit(changedIds, true) }, }) @@ -568,15 +575,19 @@ export const RelationChipCell = ({ /** * C128 — the destructive-exit guard runs here, BEFORE the optimistic apply * (same point as the floor/cap refusals): a `false` aborts with no state - * touched — the wrapper's confirmation dialog decides. The undo-toast path - * calls `commit` directly and never passes the guard (re-adding can never - * empty a relation). + * touched — the wrapper's confirmation dialog decides. A `'destructive'` + * verdict suppresses the batch-removal undo toast below (re-adding would + * only re-create the destroyed entity empty — votes/account are gone). The + * undo-toast path calls `commit` directly and never passes the guard + * (re-adding can never empty a relation). */ const guardedCommit = async (changedIds: number[], assigned: boolean) => { - if (commitGuard && !(await commitGuard({ changedIds, assigned, currentIds: effectiveIds }))) { - return + let verdict: boolean | 'destructive' = true + if (commitGuard) { + verdict = await commitGuard({ changedIds, assigned, currentIds: effectiveIds }) + if (verdict === false) return } - commit(changedIds, assigned) + commit(changedIds, assigned, verdict === 'destructive') } /** The relation's floor, refused here so the server never has to say no. */ diff --git a/src/utilities/people/personDelete.ts b/src/utilities/people/personDelete.ts index 93a48291..7c445a0b 100644 --- a/src/utilities/people/personDelete.ts +++ b/src/utilities/people/personDelete.ts @@ -448,40 +448,14 @@ export const deletePersonRecord = async ( }) } if (accountIDs.length > 0) { - // Invites the deleted accounts CREATED for other leaderships, authored - // municipality updates and calendar feeds all carry NOT NULL FKs with - // `ON DELETE set null` — they must go before the accounts themselves. - // `deleteCampaignUserAccount` runs the same clean-up per account (the - // C128 "fim de assessoria" sibling); batch-then-loop here kept both - // paths honest by NOT hand-spelling the order twice. - await payload.delete({ - collection: 'campaignInvite', - where: { createdBy: { in: accountIDs } }, - depth: 0, - overrideAccess: true, - req, - }) - await payload.delete({ - collection: 'municipalityUpdate', - where: { author: { in: accountIDs } }, - depth: 0, - overrideAccess: true, - req, - }) - await payload.delete({ - collection: 'calendarFeed', - where: { createdBy: { in: accountIDs } }, - depth: 0, - overrideAccess: true, - req, - }) - await payload.delete({ - collection: 'supporterImportBatch', - where: { actor: { in: accountIDs } }, - depth: 0, - overrideAccess: true, - req, - }) + // Per-id so `CampaignUser.beforeDelete` (passkeys + notifications) runs + // for every account — bulk `where` deletes would skip the hooks. The + // helper also removes the NOT NULL authored rows (invites created by the + // account, municipality updates, calendar feeds, import batches) before + // the account row itself. + for (const accountID of accountIDs) { + await deleteCampaignUserAccount(payload, req, accountID) + } } if (supporterCount > 0) { await payload.delete({ @@ -522,12 +496,6 @@ export const deletePersonRecord = async ( } } - // Per-id so `CampaignUser.beforeDelete` (passkeys + notifications) runs for - // every account — bulk `where` deletes would skip the hooks. - for (const accountID of accountIDs) { - await deleteCampaignUserAccount(payload, req, accountID) - } - let contactDeleted = false let contactAnonymized = false if (signatureCount > 0 || subscriptionCount > 0) { diff --git a/tests/int/campaignPersonCellEditing.int.spec.ts b/tests/int/campaignPersonCellEditing.int.spec.ts index 4d25defe..2305665e 100644 --- a/tests/int/campaignPersonCellEditing.int.spec.ts +++ b/tests/int/campaignPersonCellEditing.int.spec.ts @@ -273,6 +273,54 @@ describe('C116 — cell edits of the people list', () => { expect(result.accountId).toBeNull() }) + it('cascades authored rows when the account dies on the last municipality', async () => { + const fixtures = campaignFixtures() + const actor = await fixtures.createCampaignUser('coordinator') + const contact = await fixtures.createContact() + const account = await fixtures.createCampaignUser('advisor', { phone: contact.phone }) + const only = await fixtures.getMunicipality() + await fixtures.assignMunicipalityAdvisors(only, [account]) + const leadership = await fixtures.createLeadership({ + contact: await fixtures.createContact(), + municipalities: [only], + }) + await fixtures.createInvite({ leadership, createdBy: account }) + await fixtures.createMunicipalityUpdate({ municipality: only, author: account }) + + await setPersonAssessoraMembershipRecord(payload, actor, { + contactId: contact.id, + municipalityIds: [only.id], + assigned: false, + }) + + const remaining = await payload.find({ + collection: 'campaignUser', + where: { id: { equals: account.id } }, + depth: 0, + pagination: false, + overrideAccess: true, + }) + expect(remaining.docs).toHaveLength(0) + // The NOT NULL authored rows went down with the account (same order as + // the person cascade). + const invites = await payload.find({ + collection: 'campaignInvite', + where: { createdBy: { equals: account.id } }, + depth: 0, + pagination: false, + overrideAccess: true, + }) + expect(invites.docs).toHaveLength(0) + const updates = await payload.find({ + collection: 'municipalityUpdate', + where: { author: { equals: account.id } }, + depth: 0, + pagination: false, + overrideAccess: true, + }) + expect(updates.docs).toHaveLength(0) + }) + it('refuses a person with more than one staff account', async () => { const fixtures = campaignFixtures() const actor = await fixtures.createCampaignUser('coordinator') @@ -474,6 +522,48 @@ describe('C116 — cell edits of the people list', () => { }) expect(result.leadershipID).toBeNull() }) + + it('re-creates the leadership when the undo toast re-adds after the exit', async () => { + const fixtures = campaignFixtures() + const actor = await fixtures.createCampaignUser('coordinator') + const contact = await fixtures.createContact() + const municipality = await fixtures.getMunicipality() + const leadership = await fixtures.createLeadership({ + contact, + municipalities: [municipality], + }) + + await setPersonLeadershipMunicipalitiesRecord(payload, actor, { + contactId: contact.id, + municipalityIds: [municipality.id], + assigned: false, + }) + const gone = await payload.find({ + collection: 'leadership', + where: { id: { equals: leadership.id } }, + depth: 0, + pagination: false, + overrideAccess: true, + }) + expect(gone.docs).toHaveLength(0) + + // The undo path re-adds (`assigned=true`) — the lifecycle must bring the + // entity back (fresh, empty). + await setPersonLeadershipMunicipalitiesRecord(payload, actor, { + contactId: contact.id, + municipalityIds: [municipality.id], + assigned: true, + }) + const reborn = await payload.find({ + collection: 'leadership', + where: { contact: { equals: contact.id } }, + depth: 0, + pagination: false, + overrideAccess: true, + }) + expect(reborn.docs).toHaveLength(1) + expect(uniqueRelationshipIds(reborn.docs[0].municipalities)).toContain(municipality.id) + }) }) describe('setPersonStateDeputyMunicipalitiesRecord (C128 lifecycle)', () => {