diff --git a/app/database/migrations/20260823200000_split_legacy_campaign_backfill/migration.sql b/app/database/migrations/20260823200000_split_legacy_campaign_backfill/migration.sql new file mode 100644 index 00000000..d8fc10a7 --- /dev/null +++ b/app/database/migrations/20260823200000_split_legacy_campaign_backfill/migration.sql @@ -0,0 +1,95 @@ +-- Follow-up to 20260823000000_add_publishing_campaigns: that backfill collapsed +-- every congregation's legacy `type = 'campaign'` attributions into ONE synthetic +-- ended campaign named 'Campagne'. Real history usually spans several distinct +-- drives, so split each synthetic campaign into one campaign per date cluster: +-- a gap of more than 30 days between an attribution's start and the latest end +-- seen so far opens a new cluster. Synthetic campaigns are recognized by the +-- exact stamps the backfill wrote (name 'Campagne', activatedAt = startDate, +-- endedAt = endDate) — a campaign that went through the real lifecycle never +-- has activation/end timestamps exactly equal to its day-granular bounds. +-- Single-cluster synthetics are left untouched (splitting would be a rename). +-- One data-modifying CTE chain: temp tables don't survive Prisma's statement +-- execution. + +WITH synth AS ( + SELECT id, "congregationId" + FROM "Campaign" + WHERE name = 'Campagne' + AND "activatedAt" = "startDate" + AND "endedAt" = "endDate" +), +attrs AS ( + SELECT a.id, + a."congregationId", + a."campaignId", + a."startDate", + COALESCE(a."endDate", a."startDate") AS eff_end + FROM "Attribution" a + JOIN synth s ON s.id = a."campaignId" +), +marked AS ( + SELECT attrs.*, + CASE + WHEN "startDate" > COALESCE( + MAX(eff_end) OVER ( + PARTITION BY "campaignId" + ORDER BY "startDate", id + ROWS BETWEEN UNBOUNDED PRECEDING AND 1 PRECEDING + ), + "startDate" + ) + INTERVAL '30 days' + THEN 1 ELSE 0 + END AS opens_cluster + FROM attrs +), +clustered AS ( + SELECT marked.*, + SUM(opens_cluster) OVER (PARTITION BY "campaignId" ORDER BY "startDate", id) AS cluster + FROM marked +), +-- Only campaigns that actually split into several clusters are rewritten. +bounds AS ( + SELECT "campaignId", + "congregationId", + cluster, + MIN("startDate") AS cluster_start, + MAX(eff_end) AS cluster_end + FROM clustered + WHERE "campaignId" IN ( + SELECT "campaignId" FROM clustered GROUP BY "campaignId" HAVING COUNT(DISTINCT cluster) > 1 + ) + GROUP BY "campaignId", "congregationId", cluster +), +-- One already-ended campaign per cluster, named by its period. Cluster bounds +-- within one congregation are disjoint (30-day gaps) and there is only one +-- synthetic campaign per congregation, so (congregationId, startDate, endDate) +-- uniquely identifies the created row for the repointing below. +created AS ( + INSERT INTO "Campaign" ("name", "startDate", "endDate", "activatedAt", "endedAt", "congregationId", "updatedAt") + SELECT 'Campagne ' || to_char(b.cluster_start, 'MM/YYYY'), + b.cluster_start, + b.cluster_end, + b.cluster_start, + b.cluster_end, + b."congregationId", + CURRENT_TIMESTAMP + FROM bounds b + RETURNING id, "congregationId", "startDate", "endDate" +), +repointed AS ( + UPDATE "Attribution" a + SET "campaignId" = c.id + FROM clustered lc + JOIN bounds b + ON b."campaignId" = lc."campaignId" AND b.cluster = lc.cluster + JOIN created c + ON c."congregationId" = b."congregationId" + AND c."startDate" = b.cluster_start + AND c."endDate" = b.cluster_end + WHERE a.id = lc.id + RETURNING lc."campaignId" AS old_campaign_id +) +-- The split synthetics are now empty (every attribution was repointed; the +-- backfill never paused anything) — drop them. +DELETE FROM "Campaign" +WHERE id IN (SELECT DISTINCT old_campaign_id FROM repointed); diff --git a/app/database/migrations/20260824000000_move_part_eligibility_off_presets/migration.sql b/app/database/migrations/20260824000000_move_part_eligibility_off_presets/migration.sql new file mode 100644 index 00000000..026fd29a --- /dev/null +++ b/app/database/migrations/20260824000000_move_part_eligibility_off_presets/migration.sql @@ -0,0 +1,87 @@ +-- Eligibility (which roles may fill a slot) moves off the part kind and back +-- onto the parts themselves. Trying the preset feature showed the kind is the +-- wrong granularity: two parts of the same kind — two "Sujet VCM" — can +-- legitimately belong to different populations depending on where they sit in +-- the programme. The kind keeps capability only (reader slot, labels, +-- external-speaker rule, share message). +-- +-- Step 1 makes the change neutral *for eligibility*: under the old rule a kind +-- with roles configured for a slot decided that slot and the part's own rows +-- lay dormant. Materialize that effective answer into the part rows before the +-- preset rows disappear, so no existing part's eligibility widens or narrows. +-- Step 2 is not neutral in one respect — see the allowExternalSpeaker note +-- there. + +DELETE FROM "TemplatePartAllowedRole" tpar +USING "TemplatePart" tp +WHERE tpar."partId" = tp."id" + AND EXISTS ( + SELECT 1 FROM "PartPresetAllowedRole" ppar + WHERE ppar."presetId" = tp."presetId" AND ppar."asKind" = tpar."asKind" + ); + +INSERT INTO "TemplatePartAllowedRole" ("partId", "roleId", "asKind", "congregationId") +SELECT tp."id", ppar."roleId", ppar."asKind", tp."congregationId" +FROM "TemplatePart" tp +JOIN "PartPresetAllowedRole" ppar ON ppar."presetId" = tp."presetId" +ON CONFLICT DO NOTHING; + +DELETE FROM "EventPartAllowedRole" epar +USING "EventPart" ep +WHERE epar."eventPartId" = ep."id" + AND EXISTS ( + SELECT 1 FROM "PartPresetAllowedRole" ppar + WHERE ppar."presetId" = ep."presetId" AND ppar."asKind" = epar."asKind" + ); + +INSERT INTO "EventPartAllowedRole" ("eventPartId", "roleId", "asKind", "congregationId") +SELECT ep."id", ppar."roleId", ppar."asKind", ep."congregationId" +FROM "EventPart" ep +JOIN "PartPresetAllowedRole" ppar ON ppar."presetId" = ep."presetId" +ON CONFLICT DO NOTHING; + +DROP TABLE "PartPresetAllowedRole"; + +-- Step 2: with eligibility gone, nothing distinguishes the three seeded +-- midweek talk kinds (Joyaux, Perles, Vie chrétienne) any more — merge them +-- into one "Sujet VCM" kind. Custom wording stored on the old rows (a rename +-- or an edited share message) is dropped; the merged kind starts on the +-- catalogue defaults. +-- +-- One thing does change for existing parts, and the neutrality claim above +-- does not cover it: the three kinds disagreed on allowExternalSpeaker +-- (Joyaux and Perles said no, Vie chrétienne said yes), and one merged kind +-- can only hold one answer. It keeps yes, so parts that were Joyaux or Perles +-- start offering the external-speaker option. +-- +-- Widening deliberately rather than narrowing: the flag only decides whether +-- the picker offers an external speaker, and nothing enforces it at assignment +-- time. Answering no would have withdrawn the option from parts that were Vie +-- chrétienne and hidden anyone already assigned that way. + +INSERT INTO "PartPreset" ("key", "scope", "hasReaderSlot", "allowExternalSpeaker", "isSystem", "congregationId", "updatedAt") +SELECT 'midweek-talk', 'part', false, true, true, c."id", CURRENT_TIMESTAMP +FROM "Congregation" c +ON CONFLICT ("key", "congregationId") DO NOTHING; + +UPDATE "TemplatePart" tp +SET "presetId" = np."id" +FROM "PartPreset" op, "PartPreset" np +WHERE tp."presetId" = op."id" + AND op."isSystem" = true + AND op."key" IN ('spiritual-gems', 'spiritual-pearls', 'christian-life-talk') + AND np."key" = 'midweek-talk' + AND np."congregationId" = op."congregationId"; + +UPDATE "EventPart" ep +SET "presetId" = np."id" +FROM "PartPreset" op, "PartPreset" np +WHERE ep."presetId" = op."id" + AND op."isSystem" = true + AND op."key" IN ('spiritual-gems', 'spiritual-pearls', 'christian-life-talk') + AND np."key" = 'midweek-talk' + AND np."congregationId" = op."congregationId"; + +DELETE FROM "PartPreset" +WHERE "isSystem" = true + AND "key" IN ('spiritual-gems', 'spiritual-pearls', 'christian-life-talk'); diff --git a/app/database/migrations/move-part-eligibility-off-presets.integration.test.ts b/app/database/migrations/move-part-eligibility-off-presets.integration.test.ts new file mode 100644 index 00000000..bddd9897 --- /dev/null +++ b/app/database/migrations/move-part-eligibility-off-presets.integration.test.ts @@ -0,0 +1,237 @@ +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' +import { PrismaPg } from '@prisma/adapter-pg' +import { afterAll, describe, expect, it } from 'vitest' +import { PrismaClient } from '~/database/generated/client' + +// Runs against DB_URL rather than DB_RUNTIME_URL: a migration executes as the +// schema owner, and this test recreates a dropped table, which the RLS-bound +// runtime role cannot do. +const adapter = new PrismaPg({ + connectionString: process.env.DB_URL, + max: 3, + connectionTimeoutMillis: 5000, +}) +const testDb = new PrismaClient({ adapter }) + +const MIGRATION_SQL = resolve(import.meta.dirname, '20260824000000_move_part_eligibility_off_presets', 'migration.sql') + +// The table as it stood before this migration dropped it, copied from +// 20260821000000_add_part_preset. Without it there is no "before" to migrate. +const RECREATE_DROPPED_TABLE = ` + CREATE TABLE "PartPresetAllowedRole" ( + "presetId" INTEGER NOT NULL, + "roleId" INTEGER NOT NULL, + "asKind" TEXT NOT NULL, + "congregationId" INTEGER NOT NULL, + CONSTRAINT "PartPresetAllowedRole_pkey" PRIMARY KEY ("presetId","roleId","asKind") + ) +` + +/** + * The real migration file, split into statements. + * + * Reading the shipped artifact rather than a paraphrase is the point: a test + * that re-typed the SQL would keep passing after someone edited the file. + */ +function migrationStatements(): string[] { + return readFileSync(MIGRATION_SQL, 'utf8') + .split('\n') + .filter(line => !line.trimStart().startsWith('--')) + .join('\n') + .split(';') + .map(statement => statement.trim()) + .filter(statement => statement.length > 0) +} + +/** Thrown to roll the fixture back; every assertion runs on captured values. */ +class Rollback extends Error {} + +afterAll(async () => { + await testDb.$disconnect() +}) + +interface Captured { + roleFromKindId: number + roleFromPartId: number + templateSpeaker: number[] + templateReaderUntouchedByKind: number[] + templateNoPreset: number[] + eventSpeaker: number[] + mergedKeys: string[] + customPresetSurvives: number + legacyPresetsGone: number +} + +async function runMigrationOverFixture(): Promise { + let captured: Captured | undefined + + try { + await testDb.$transaction( + async tx => { + const stamp = `mig-${process.pid}-${globalThis.performance.now().toString().replace('.', '')}` + const congregation = await tx.congregation.create({ + data: { name: stamp, slug: stamp, active: true }, + }) + const congregationId = congregation.id + + const roleFromKind = await tx.role.create({ + data: { key: `${stamp}-kind`, name: 'Depuis le type', isBuiltIn: false, congregationId }, + }) + const roleFromPart = await tx.role.create({ + data: { key: `${stamp}-part`, name: 'Depuis la partie', isBuiltIn: false, congregationId }, + }) + + const gems = await tx.partPreset.create({ + data: { key: 'spiritual-gems', scope: 'part', isSystem: true, congregationId }, + }) + const christianLife = await tx.partPreset.create({ + data: { key: 'christian-life-talk', scope: 'part', isSystem: true, congregationId }, + }) + const custom = await tx.partPreset.create({ + data: { key: 'spiritual-gems-2', name: 'Mon type', scope: 'part', isSystem: false, congregationId }, + }) + + const template = await tx.eventTemplate.create({ + data: { name: stamp, key: `${stamp}-tpl`, startTime: '19:00', endTime: '21:00', congregationId }, + }) + const author = await tx.userAccount.create({ + data: { + email: `${stamp}@test.invalid`, + password: 'hashed', + firstname: 'Mig', + lastname: 'Ration', + active: true, + congregationId, + }, + }) + const event = await tx.event.create({ + data: { + name: stamp, + startDate: new Date('2026-01-01T19:00:00Z'), + endDate: new Date('2026-01-01T21:00:00Z'), + templateId: template.id, + createdById: author.id, + congregationId, + }, + }) + + // The kind restricts this slot, so under the old rule the kind's list + // was the answer and the part's own row lay dormant. + const partKindWins = await tx.templatePart.create({ + data: { name: 'A', order: 1, templateId: template.id, presetId: gems.id, congregationId }, + }) + // The kind says nothing about the reader slot, so the part's own row + // was already the answer and has to survive untouched. + const partReaderOwn = await tx.templatePart.create({ + data: { name: 'B', order: 2, templateId: template.id, presetId: gems.id, congregationId }, + }) + const partNoPreset = await tx.templatePart.create({ + data: { name: 'C', order: 3, templateId: template.id, congregationId }, + }) + const eventPart = await tx.eventPart.create({ + data: { name: 'D', order: 1, eventId: event.id, presetId: christianLife.id, congregationId }, + }) + + await tx.templatePartAllowedRole.createMany({ + data: [ + { partId: partKindWins.id, roleId: roleFromPart.id, asKind: 'speaker', congregationId }, + { partId: partReaderOwn.id, roleId: roleFromPart.id, asKind: 'reader', congregationId }, + { partId: partNoPreset.id, roleId: roleFromPart.id, asKind: 'speaker', congregationId }, + ], + }) + await tx.eventPartAllowedRole.createMany({ + data: [{ eventPartId: eventPart.id, roleId: roleFromPart.id, asKind: 'speaker', congregationId }], + }) + + await tx.$executeRawUnsafe(RECREATE_DROPPED_TABLE) + await tx.$executeRawUnsafe( + `INSERT INTO "PartPresetAllowedRole" ("presetId", "roleId", "asKind", "congregationId") + VALUES ($1, $2, 'speaker', $3), ($4, $2, 'speaker', $3)`, + gems.id, + roleFromKind.id, + congregationId, + christianLife.id, + ) + + for (const statement of migrationStatements()) { + await tx.$executeRawUnsafe(statement) + } + + const speakerRoles = async (partId: number) => + ( + await tx.templatePartAllowedRole.findMany({ + where: { partId, asKind: 'speaker' }, + select: { roleId: true }, + }) + ).map(r => r.roleId) + + const merged = await tx.partPreset.findMany({ + where: { congregationId, id: { not: custom.id } }, + select: { key: true }, + }) + + captured = { + roleFromKindId: roleFromKind.id, + roleFromPartId: roleFromPart.id, + templateSpeaker: await speakerRoles(partKindWins.id), + templateReaderUntouchedByKind: ( + await tx.templatePartAllowedRole.findMany({ + where: { partId: partReaderOwn.id, asKind: 'reader' }, + select: { roleId: true }, + }) + ).map(r => r.roleId), + templateNoPreset: await speakerRoles(partNoPreset.id), + eventSpeaker: ( + await tx.eventPartAllowedRole.findMany({ + where: { eventPartId: eventPart.id, asKind: 'speaker' }, + select: { roleId: true }, + }) + ).map(r => r.roleId), + mergedKeys: merged.map(p => p.key).sort(), + customPresetSurvives: await tx.partPreset.count({ where: { id: custom.id } }), + legacyPresetsGone: await tx.partPreset.count({ + where: { congregationId, key: { in: ['spiritual-gems', 'spiritual-pearls', 'christian-life-talk'] } }, + }), + } + + // Everything above, including the recreated table, unwinds here. + throw new Rollback() + }, + { timeout: 25_000 }, + ) + } catch (error) { + if (!(error instanceof Rollback)) throw error + } + + if (!captured) throw new Error('fixture never ran') + return captured +} + +describe('20260824000000_move_part_eligibility_off_presets', () => { + it('materializes the eligibility each part actually resolved to, and merges the midweek kinds', async () => { + const result = await runMigrationOverFixture() + + // The kind's list decided this slot before, so it must be the part's now: + // the kind's role, and specifically NOT the dormant one the part carried. + expect(result.templateSpeaker).toEqual([result.roleFromKindId]) + + // The kind restricted nobody here, so the part's own row was already the + // answer. Losing it would widen the slot to every member. + expect(result.templateReaderUntouchedByKind).toEqual([result.roleFromPartId]) + + // No kind, nothing to resolve, nothing to change. + expect(result.templateNoPreset).toEqual([result.roleFromPartId]) + + // Event parts resolve by the same rule as template parts. + expect(result.eventSpeaker).toEqual([result.roleFromKindId]) + + // The three midweek kinds collapse to one, and the parts that used them + // are repointed rather than orphaned. + expect(result.mergedKeys).toEqual(['midweek-talk']) + expect(result.legacyPresetsGone).toBe(0) + + // A congregation's own kind is not system-seeded and must not be folded. + expect(result.customPresetSurvives).toBe(1) + }) +}) diff --git a/app/database/schema.prisma b/app/database/schema.prisma index a2f8d1d3..a31a5da0 100644 --- a/app/database/schema.prisma +++ b/app/database/schema.prisma @@ -118,7 +118,6 @@ model Congregation { templateServicePartAllowedRoles TemplateServicePartAllowedRole[] eventServicePartAllowedRoles EventServicePartAllowedRole[] partPresets PartPreset[] - partPresetAllowedRoles PartPresetAllowedRole[] externalSpeakers ExternalSpeaker[] emergencyContacts EmergencyContact[] @@ -328,7 +327,6 @@ model Role { allowedForTemplateParts TemplatePartAllowedRole[] allowedForEventParts EventPartAllowedRole[] - allowedForPartPresets PartPresetAllowedRole[] allowedForTemplateServiceParts TemplateServicePartAllowedRole[] allowedForEventServiceParts EventServicePartAllowedRole[] allowedForBoardSections BoardSectionVisibilityRole[] @@ -1058,10 +1056,19 @@ model EventServicePart { // Watchtower study. Congregation-scoped and congregation-creatable. // // Deliberately carries no defaults. The three layers are: -// PartPreset — identity + capability. Never overridable on a part. +// PartPreset — identity + capability: the reader slot, the slot labels, +// the external-speaker rule, the share message. Never +// overridable on a part. // TemplatePart — defaults (name, section, duration) so they aren't retyped. // EventPart — the instance, free to override anything the template set. // +// Eligibility — which roles may fill a slot — is deliberately NOT in that +// first layer, though it was until the 20260824 migration. Two parts of the +// same kind can be done by different roles depending on where they sit in the +// programme, so it belongs to the part. Only TemplatePart and EventPart carry +// allowedRoles; a new field goes here only if it is true of every occurrence +// of the kind. +// // That split is why `shareMessage` lives here: the wording of the message sent // to an assignee is a property of the kind of assignment, not of one evening's // occurrence of it, so it must not be editable part by part. @@ -1099,7 +1106,6 @@ model PartPreset { eventParts EventPart[] templateServiceParts TemplateServicePart[] eventServiceParts EventServicePart[] - allowedRoles PartPresetAllowedRole[] congregation Congregation @relation(fields: [congregationId], references: [id]) congregationId Int @@ -1112,23 +1118,6 @@ model PartPreset { @@index([congregationId]) } -// Which roles may be assigned to a part of this kind. Mirrors -// TemplatePartAllowedRole; `asKind` distinguishes the speaker and reader slots. -model PartPresetAllowedRole { - preset PartPreset @relation(fields: [presetId], references: [id], onDelete: Cascade) - presetId Int - role Role @relation(fields: [roleId], references: [id], onDelete: Cascade) - roleId Int - asKind String - - congregation Congregation @relation(fields: [congregationId], references: [id], onDelete: Cascade) - congregationId Int - - @@id([presetId, roleId, asKind]) - @@index([roleId]) - @@index([congregationId]) -} - model TemplatePartAllowedRole { part TemplatePart @relation(fields: [partId], references: [id], onDelete: Cascade) partId Int diff --git a/app/database/seed-marketing.ts b/app/database/seed-marketing.ts index 46f9efb2..e4536f41 100644 --- a/app/database/seed-marketing.ts +++ b/app/database/seed-marketing.ts @@ -541,7 +541,7 @@ async function cleanCongregationData(congregationId: number) { await prisma.eventTemplate.deleteMany({ where: { congregationId } }) // After the parts that point at them, before the congregation itself: the // FK to Congregation restricts, so leaving these behind makes the final - // congregation.delete fail. PartPresetAllowedRole cascades from here. + // congregation.delete fail. await prisma.partPreset.deleteMany({ where: { congregationId } }) await prisma.event.deleteMany({ where: { congregationId } }) await prisma.boardDynamicDocumentView.deleteMany({ diff --git a/app/features/events/index.ts b/app/features/events/index.ts index bb6ac412..a034b5d8 100644 --- a/app/features/events/index.ts +++ b/app/features/events/index.ts @@ -1,6 +1,10 @@ // Public client-safe surface of the events feature. -export { partAllowedRolesToWrite, resolveAllowedRoleIds } from './model/allowed-roles-resolution' +export { + MANAGED_ROLE_SLOTS_FIELD, + type PartRoleSlot, + partAllowedRolesToWrite, +} from './model/allowed-roles-write' export { dayLabel, dayLabelShort } from './model/day-label' export { EventStatus } from './model/event-status.type' export { EventTemplateKey, isSystemTemplate } from './model/event-template.type' diff --git a/app/features/events/model/allowed-roles-resolution.test.ts b/app/features/events/model/allowed-roles-resolution.test.ts deleted file mode 100644 index 1006ff9f..00000000 --- a/app/features/events/model/allowed-roles-resolution.test.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { partAllowedRolesToWrite, resolveAllowedRoleIds } from './allowed-roles-resolution' - -describe('resolveAllowedRoleIds', () => { - it('uses the preset roles when it has any', () => { - expect(resolveAllowedRoleIds({ partRoleIds: [1, 2], presetRoleIds: [7, 8] })).toEqual([7, 8]) - }) - - it('falls back to the part when the preset has none', () => { - // Empty does not mean "nobody" — resolveEligibleUserIds reads an empty list - // as "any member". Treating an unconfigured preset as authoritative would - // silently hand every part using it the widest possible audience. - expect(resolveAllowedRoleIds({ partRoleIds: [1, 2], presetRoleIds: [] })).toEqual([1, 2]) - }) - - it('leaves an unrestricted part unrestricted', () => { - expect(resolveAllowedRoleIds({ partRoleIds: [], presetRoleIds: [] })).toEqual([]) - }) - - it('lets a preset narrow a part that had no restriction', () => { - expect(resolveAllowedRoleIds({ partRoleIds: [], presetRoleIds: [7] })).toEqual([7]) - }) - - it('cannot be used to remove a restriction the part carries', () => { - // The deliberate asymmetry with allowExternalSpeaker, where the preset can - // say no. Here "no roles" is the widest setting rather than the narrowest, - // so an empty preset can only ever mean "not configured". - expect(resolveAllowedRoleIds({ partRoleIds: [1], presetRoleIds: [] })).toEqual([1]) - }) - - it('does not merge the two sides', () => { - // A union would let a part quietly widen what its kind permits. - expect(resolveAllowedRoleIds({ partRoleIds: [1, 2], presetRoleIds: [3] })).toEqual([3]) - }) -}) - -describe('partAllowedRolesToWrite', () => { - it('passes both slots through when the part has no kind', () => { - expect( - partAllowedRolesToWrite({ partPresetId: null, allowedSpeakerRoleIds: [3], allowedReaderRoleIds: [4] }), - ).toEqual({ allowedSpeakerRoleIds: [3], allowedReaderRoleIds: [4] }) - }) - - it('passes an emptied selection through, so clearing still works', () => { - expect( - partAllowedRolesToWrite({ partPresetId: null, allowedSpeakerRoleIds: [], allowedReaderRoleIds: [] }), - ).toEqual({ allowedSpeakerRoleIds: [], allowedReaderRoleIds: [] }) - }) - - it('omits both slots when a kind owns eligibility', () => { - // Not [] — the part's own rows are the kind's fallback and must survive. - expect(partAllowedRolesToWrite({ partPresetId: 55, allowedSpeakerRoleIds: [], allowedReaderRoleIds: [] })).toEqual( - {}, - ) - }) -}) diff --git a/app/features/events/model/allowed-roles-resolution.ts b/app/features/events/model/allowed-roles-resolution.ts deleted file mode 100644 index 88503688..00000000 --- a/app/features/events/model/allowed-roles-resolution.ts +++ /dev/null @@ -1,57 +0,0 @@ -/** - * Which roles may be assigned to a slot: the preset's when it has any, - * otherwise the part's own. - * - * This is deliberately NOT the rule used for allowExternalSpeaker, where the - * preset wins even when it says no. The difference is what "empty" means. - * resolveEligibleUserIds reads an empty allowed-roles list as "any member" — - * the widest setting, not the narrowest. So an unconfigured preset is - * indistinguishable from one that permits everyone, and letting it win would - * silently hand every part using that kind the largest possible audience. - * - * The consequence, worth knowing: a preset can narrow eligibility or redefine - * it, but cannot be used to lift a restriction a part already carries. Removing - * one means clearing it on the part. - * - * It also makes adopting a kind safe on the read side — until roles are set on - * it, every part resolves to exactly the eligibility it had. Keeping that true - * on the write side is a separate problem, and the reason the part editor stops - * managing these rows once a kind is chosen: see partAllowedRolesToWrite. - * - * Named rather than positional because the two lists are the same type and the - * whole rule is which one wins; swapping them would compile and invert it. - */ -export function resolveAllowedRoleIds({ - partRoleIds, - presetRoleIds, -}: { - partRoleIds: number[] - presetRoleIds: number[] -}): number[] { - return presetRoleIds.length > 0 ? presetRoleIds : partRoleIds -} - -/** - * Which role slots the part editor actually managed. - * - * The editor hides its role pickers once a kind is chosen, and an unchecked - * checkbox submits nothing — so "the user cleared the selection" and "the - * picker was never rendered" arrive as the same empty field. The kind is what - * tells them apart: with one, eligibility belongs to the kind (see - * resolveAllowedRoleIds) and the part's own rows must be left alone, because - * they are what the kind falls back to while it restricts nobody. - * - * Omitting the keys rather than sending [] is the point — the services treat - * undefined as "not managed" and [] as "managed, and empty". - */ -export function partAllowedRolesToWrite(value: { - partPresetId: number | null - allowedSpeakerRoleIds: number[] - allowedReaderRoleIds: number[] -}): { allowedSpeakerRoleIds?: number[]; allowedReaderRoleIds?: number[] } { - if (value.partPresetId != null) return {} - return { - allowedSpeakerRoleIds: value.allowedSpeakerRoleIds, - allowedReaderRoleIds: value.allowedReaderRoleIds, - } -} diff --git a/app/features/events/model/allowed-roles-write.test.ts b/app/features/events/model/allowed-roles-write.test.ts new file mode 100644 index 00000000..39d7010a --- /dev/null +++ b/app/features/events/model/allowed-roles-write.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from 'vitest' +import { MANAGED_ROLE_SLOTS_FIELD, partAllowedRolesToWrite } from './allowed-roles-write' + +describe('partAllowedRolesToWrite', () => { + it('sends both slots when the editor rendered both pickers', () => { + expect( + partAllowedRolesToWrite({ + managedSlots: ['speaker', 'reader'], + allowedSpeakerRoleIds: [4], + allowedReaderRoleIds: [9], + }), + ).toEqual({ allowedSpeakerRoleIds: [4], allowedReaderRoleIds: [9] }) + }) + + it('omits the reader slot when its picker was never rendered', () => { + // The regression this exists for: a kind with no reader slot hides the + // reader picker, an unrendered picker submits nothing, and the schema + // turns that absence into []. Writing [] deletes the restriction the part + // already carried — see the reader half of 78a9219. + expect( + partAllowedRolesToWrite({ + managedSlots: ['speaker'], + allowedSpeakerRoleIds: [4], + allowedReaderRoleIds: [], + }), + ).toEqual({ allowedSpeakerRoleIds: [4] }) + }) + + it('distinguishes a cleared picker from an absent one', () => { + // Both arrive as [], so only the declared slot list can tell them apart. + // Rendered and emptied must clear; never rendered must not. + const cleared = partAllowedRolesToWrite({ + managedSlots: ['speaker', 'reader'], + allowedSpeakerRoleIds: [], + allowedReaderRoleIds: [], + }) + const absent = partAllowedRolesToWrite({ + managedSlots: ['speaker'], + allowedSpeakerRoleIds: [], + allowedReaderRoleIds: [], + }) + + expect(cleared.allowedReaderRoleIds).toEqual([]) + expect(absent).not.toHaveProperty('allowedReaderRoleIds') + }) + + it('omits everything when the editor declared no slots at all', () => { + // A caller that manages no eligibility must leave every stored row alone, + // rather than silently clearing both slots. + expect( + partAllowedRolesToWrite({ + managedSlots: [], + allowedSpeakerRoleIds: [], + allowedReaderRoleIds: [], + }), + ).toEqual({}) + }) + + it('ignores a slot name it does not recognise', () => { + // The value crosses an HTTP boundary, so it is attacker-controlled text + // rather than a trusted enum. + expect( + partAllowedRolesToWrite({ + managedSlots: ['speaker', 'conductor'], + allowedSpeakerRoleIds: [4], + allowedReaderRoleIds: [9], + }), + ).toEqual({ allowedSpeakerRoleIds: [4] }) + }) + + it('names the form field once, so the sheet and the schema cannot drift', () => { + expect(MANAGED_ROLE_SLOTS_FIELD).toBe('managedRoleSlots') + }) +}) diff --git a/app/features/events/model/allowed-roles-write.ts b/app/features/events/model/allowed-roles-write.ts new file mode 100644 index 00000000..5c61fec7 --- /dev/null +++ b/app/features/events/model/allowed-roles-write.ts @@ -0,0 +1,48 @@ +/** The two slots a programme part can put someone in. */ +export type PartRoleSlot = 'speaker' | 'reader' + +const SLOTS: PartRoleSlot[] = ['speaker', 'reader'] + +/** + * The hidden field the part editor uses to declare which role pickers it drew. + * + * Exported so the sheet and the schema name it once. They live in different + * features, and a typo on either side would silently reintroduce the bug this + * whole mechanism exists to prevent. + */ +export const MANAGED_ROLE_SLOTS_FIELD = 'managedRoleSlots' + +/** + * Which role slots the part editor actually managed. + * + * An unchecked picker submits nothing, and so does a picker that was never + * drawn — both reach the action as an empty field. They must not mean the same + * thing: one is "the user cleared the selection" and has to delete the stored + * rows, the other is "this slot was never on screen" and has to leave them + * alone. The reader picker is hidden whenever the chosen kind has no reader + * slot, so without this distinction, giving a part such a kind deleted the + * restriction it already carried and widened the slot to every member. + * + * The form declares what it drew rather than the action re-deriving it. The + * render condition and the write decision would otherwise be two copies of one + * rule in different features, free to drift apart — which is how this bug + * arrived the first time. + * + * Omitting the keys rather than sending [] is the point: the services treat + * undefined as "not managed" and [] as "managed, and empty". + */ +export function partAllowedRolesToWrite(value: { + managedSlots: string[] + allowedSpeakerRoleIds: number[] + allowedReaderRoleIds: number[] +}): { allowedSpeakerRoleIds?: number[]; allowedReaderRoleIds?: number[] } { + // Filtered against the known slots rather than trusted: the list arrives + // from a form post, so an unrecognised name means "not managed" instead of + // reaching a lookup that would resolve to undefined. + const managed = new Set(SLOTS.filter(slot => value.managedSlots.includes(slot))) + + return { + ...(managed.has('speaker') ? { allowedSpeakerRoleIds: value.allowedSpeakerRoleIds } : {}), + ...(managed.has('reader') ? { allowedReaderRoleIds: value.allowedReaderRoleIds } : {}), + } +} diff --git a/app/features/events/model/part-preset-defaults.ts b/app/features/events/model/part-preset-defaults.ts index 62257a4c..4a83f37d 100644 --- a/app/features/events/model/part-preset-defaults.ts +++ b/app/features/events/model/part-preset-defaults.ts @@ -18,12 +18,10 @@ type MessageFn = (inputs?: Record, options?: { locale?: Locale }) const BUILT_IN_NAMES: Record = { [PartPresetKey.Prayer]: m.seed_preset_prayer, [PartPresetKey.Chairman]: m.seed_preset_chairman, - [PartPresetKey.SpiritualGems]: m.seed_preset_spiritual_gems, - [PartPresetKey.SpiritualPearls]: m.seed_preset_spiritual_pearls, + [PartPresetKey.MidweekTalk]: m.seed_preset_midweek_talk, [PartPresetKey.BibleReading]: m.seed_preset_bible_reading, [PartPresetKey.SchoolDemonstration]: m.seed_preset_school_demonstration, [PartPresetKey.SchoolTalk]: m.seed_preset_school_talk, - [PartPresetKey.ChristianLifeTalk]: m.seed_preset_christian_life_talk, [PartPresetKey.PublicTalk]: m.seed_preset_public_talk, [PartPresetKey.WatchtowerStudy]: m.seed_preset_watchtower_study, [PartPresetKey.CongregationBibleStudy]: m.seed_preset_congregation_bible_study, @@ -36,12 +34,10 @@ const BUILT_IN_NAMES: Record = { const BUILT_IN_SPEAKER_LABELS: Record = { [PartPresetKey.Prayer]: m.seed_preset_label_brother, [PartPresetKey.Chairman]: m.seed_preset_label_chairman, - [PartPresetKey.SpiritualGems]: m.seed_preset_label_speaker, - [PartPresetKey.SpiritualPearls]: m.seed_preset_label_conductor, + [PartPresetKey.MidweekTalk]: m.seed_preset_label_speaker, [PartPresetKey.BibleReading]: m.seed_preset_label_reader, [PartPresetKey.SchoolDemonstration]: m.seed_preset_label_publisher, [PartPresetKey.SchoolTalk]: m.seed_preset_label_speaker, - [PartPresetKey.ChristianLifeTalk]: m.seed_preset_label_speaker, [PartPresetKey.PublicTalk]: m.seed_preset_label_speaker, [PartPresetKey.WatchtowerStudy]: m.seed_preset_label_conductor, [PartPresetKey.CongregationBibleStudy]: m.seed_preset_label_conductor, @@ -60,18 +56,14 @@ export const SHARE_MESSAGES: Record> = { 'Bonjour {{assigneeFirstname}},\n\nTu as la prière le {{date}} à {{time}} ({{eventName}}).\n\n{{link}}', [PartPresetKey.Chairman]: 'Bonjour {{assigneeFirstname}},\n\nTu présides la réunion du {{date}} à {{time}}.\n\n{{link}}', - [PartPresetKey.SpiritualGems]: - 'Bonjour {{assigneeFirstname}},\n\nTu as un discours dans les Joyaux de la Parole de Dieu le {{date}} à {{time}}.\nSujet : {{topic}}\nDurée : {{duration}}\nNote : {{note}}\n\n{{link}}', - [PartPresetKey.SpiritualPearls]: - 'Bonjour {{assigneeFirstname}},\n\nTu conduis « Recherchons des perles spirituelles » le {{date}} à {{time}}.\nDurée : {{duration}}\nNote : {{note}}\n\n{{link}}', + [PartPresetKey.MidweekTalk]: + 'Bonjour {{assigneeFirstname}},\n\nTu as un sujet à la réunion Vie chrétienne et ministère le {{date}} à {{time}}.\nPartie : {{partName}}\nSujet : {{topic}}\nDurée : {{duration}}\nNote : {{note}}\n\n{{link}}', [PartPresetKey.BibleReading]: 'Bonjour {{assigneeFirstname}},\n\nTu as la lecture de la Bible le {{date}} à {{time}}.\nPassage : {{topic}}\nDurée : {{duration}}\nNote : {{note}}\n\n{{link}}', [PartPresetKey.SchoolDemonstration]: "Bonjour {{assigneeFirstname}},\n\nTu as un sujet de l'école le {{date}} à {{time}}.\nSujet : {{topic}}\nDurée : {{duration}}\nAvec : {{assistant}}\nNote : {{note}}\n\n{{link}}", [PartPresetKey.SchoolTalk]: "Bonjour {{assigneeFirstname}},\n\nTu as un discours de l'école le {{date}} à {{time}}.\nSujet : {{topic}}\nDurée : {{duration}}\nNote : {{note}}\n\n{{link}}", - [PartPresetKey.ChristianLifeTalk]: - 'Bonjour {{assigneeFirstname}},\n\nTu as un discours dans « Vie chrétienne » le {{date}} à {{time}}.\nSujet : {{topic}}\nDurée : {{duration}}\nNote : {{note}}\n\n{{link}}', [PartPresetKey.PublicTalk]: 'Bonjour {{assigneeFirstname}},\n\nTu donnes le discours public le {{date}} à {{time}}.\nThème : {{topic}}\nNote : {{note}}\n\n{{link}}', [PartPresetKey.WatchtowerStudy]: @@ -84,18 +76,14 @@ export const SHARE_MESSAGES: Record> = { 'Hi {{assigneeFirstname}},\n\nYou have the prayer on {{date}} at {{time}} ({{eventName}}).\n\n{{link}}', [PartPresetKey.Chairman]: 'Hi {{assigneeFirstname}},\n\nYou are chairing the meeting on {{date}} at {{time}}.\n\n{{link}}', - [PartPresetKey.SpiritualGems]: - "Hi {{assigneeFirstname}},\n\nYou have a talk in Treasures From God's Word on {{date}} at {{time}}.\nTopic: {{topic}}\nLength: {{duration}}\nNote: {{note}}\n\n{{link}}", - [PartPresetKey.SpiritualPearls]: - 'Hi {{assigneeFirstname}},\n\nYou are conducting Digging for Spiritual Gems on {{date}} at {{time}}.\nLength: {{duration}}\nNote: {{note}}\n\n{{link}}', + [PartPresetKey.MidweekTalk]: + 'Hi {{assigneeFirstname}},\n\nYou have a talk in the midweek meeting on {{date}} at {{time}}.\nPart: {{partName}}\nTopic: {{topic}}\nLength: {{duration}}\nNote: {{note}}\n\n{{link}}', [PartPresetKey.BibleReading]: 'Hi {{assigneeFirstname}},\n\nYou have the Bible reading on {{date}} at {{time}}.\nPassage: {{topic}}\nLength: {{duration}}\nNote: {{note}}\n\n{{link}}', [PartPresetKey.SchoolDemonstration]: 'Hi {{assigneeFirstname}},\n\nYou have a school demonstration on {{date}} at {{time}}.\nTopic: {{topic}}\nLength: {{duration}}\nWith: {{assistant}}\nNote: {{note}}\n\n{{link}}', [PartPresetKey.SchoolTalk]: 'Hi {{assigneeFirstname}},\n\nYou have a school talk on {{date}} at {{time}}.\nTopic: {{topic}}\nLength: {{duration}}\nNote: {{note}}\n\n{{link}}', - [PartPresetKey.ChristianLifeTalk]: - 'Hi {{assigneeFirstname}},\n\nYou have a talk in Living as Christians on {{date}} at {{time}}.\nTopic: {{topic}}\nLength: {{duration}}\nNote: {{note}}\n\n{{link}}', [PartPresetKey.PublicTalk]: 'Hi {{assigneeFirstname}},\n\nYou are giving the public talk on {{date}} at {{time}}.\nTheme: {{topic}}\nNote: {{note}}\n\n{{link}}', [PartPresetKey.WatchtowerStudy]: diff --git a/app/features/events/model/part-preset.type.ts b/app/features/events/model/part-preset.type.ts index b592eced..1cabded2 100644 --- a/app/features/events/model/part-preset.type.ts +++ b/app/features/events/model/part-preset.type.ts @@ -10,12 +10,12 @@ export enum PartPresetKey { Prayer = 'prayer', Chairman = 'chairman', - SpiritualGems = 'spiritual-gems', - SpiritualPearls = 'spiritual-pearls', + // One kind for every midweek-meeting talk (Joyaux, Perles, Vie chrétienne): + // eligibility lives on the parts, so nothing distinguished them any more. + MidweekTalk = 'midweek-talk', BibleReading = 'bible-reading', SchoolDemonstration = 'school-demonstration', SchoolTalk = 'school-talk', - ChristianLifeTalk = 'christian-life-talk', PublicTalk = 'public-talk', WatchtowerStudy = 'watchtower-study', CongregationBibleStudy = 'congregation-bible-study', diff --git a/app/features/events/routes/programs/events/_edit-event-intents.server.ts b/app/features/events/routes/programs/events/_edit-event-intents.server.ts index 29295b51..68be64c5 100644 --- a/app/features/events/routes/programs/events/_edit-event-intents.server.ts +++ b/app/features/events/routes/programs/events/_edit-event-intents.server.ts @@ -1,5 +1,5 @@ import { parseWithZod } from '@conform-to/zod' -import { partAllowedRolesToWrite } from '~/features/events' +import { partAllowedRolesToWrite } from '~/features/events/model/allowed-roles-write' import { addPartSchema, addServiceSchema, @@ -99,6 +99,7 @@ async function handleAddPart( partPresetId, allowedSpeakerRoleIds, allowedReaderRoleIds, + managedRoleSlots, } = submission.value await addPartAssignment( db, @@ -114,7 +115,7 @@ async function handleAddPart( speakerLabel: partSpeakerLabel ?? null, readerLabel: partReaderLabel ?? null, presetId: partPresetId, - ...partAllowedRolesToWrite({ partPresetId, allowedSpeakerRoleIds, allowedReaderRoleIds }), + ...partAllowedRolesToWrite({ managedSlots: managedRoleSlots, allowedSpeakerRoleIds, allowedReaderRoleIds }), congregationId, }, actorId, @@ -145,6 +146,7 @@ async function handleUpdatePart( partPresetId, allowedSpeakerRoleIds, allowedReaderRoleIds, + managedRoleSlots, } = submission.value await updatePartAssignment( db, @@ -160,7 +162,7 @@ async function handleUpdatePart( speakerLabel: partSpeakerLabel ?? null, readerLabel: partReaderLabel ?? null, presetId: partPresetId, - ...partAllowedRolesToWrite({ partPresetId, allowedSpeakerRoleIds, allowedReaderRoleIds }), + ...partAllowedRolesToWrite({ managedSlots: managedRoleSlots, allowedSpeakerRoleIds, allowedReaderRoleIds }), }, congregationId, actorId, diff --git a/app/features/events/schemas/part-preset.schema.test.ts b/app/features/events/schemas/part-preset.schema.test.ts index 21f5aade..ed1c8fac 100644 --- a/app/features/events/schemas/part-preset.schema.test.ts +++ b/app/features/events/schemas/part-preset.schema.test.ts @@ -84,25 +84,3 @@ describe('partPresetSchema', () => { expect(parsed.success && parsed.data.readerLabel).toBeNull() }) }) - -describe('partPresetSchema allowed roles', () => { - it('accepts a single role as a bare value', () => { - const parsed = partPresetSchema.safeParse({ ...base(), allowedSpeakerRoleIds: '4' }) - - expect(parsed.success && parsed.data.allowedSpeakerRoleIds).toEqual([4]) - }) - - it('accepts several roles', () => { - const parsed = partPresetSchema.safeParse({ ...base(), allowedReaderRoleIds: ['4', '9'] }) - - expect(parsed.success && parsed.data.allowedReaderRoleIds).toEqual([4, 9]) - }) - - it('treats no selection as unrestricted rather than invalid', () => { - // Empty means "any member" downstream — it is a legitimate choice, not an - // omission to reject. - const parsed = partPresetSchema.safeParse(base()) - - expect(parsed.success && parsed.data.allowedSpeakerRoleIds).toEqual([]) - }) -}) diff --git a/app/features/events/schemas/part-preset.schema.ts b/app/features/events/schemas/part-preset.schema.ts index 3075694e..bf636ea9 100644 --- a/app/features/events/schemas/part-preset.schema.ts +++ b/app/features/events/schemas/part-preset.schema.ts @@ -15,13 +15,6 @@ const checkboxField = z .optional() .transform(v => v === 'on') -// Same shape as the part form's role fields — a single value arrives as a -// string, several as an array, and nothing at all as an empty selection. -const roleIdsField = z.preprocess( - v => (Array.isArray(v) ? v : v == null || v === '' ? [] : [v]), - z.array(z.coerce.number().int().positive()), -) - export const partPresetSchema = z.object({ // Blank means "use the built-in name", which is why this is no longer // required: the placeholder in the form shows what blank will produce. @@ -53,8 +46,6 @@ export const partPresetSchema = z.object({ message: `Variable(s) inconnue(s) : ${unknown.map(name => `{{${name}}}`).join(', ')}. Disponibles : ${SHARE_VARIABLES.map(name => `{{${name}}}`).join(', ')}`, }) }), - allowedSpeakerRoleIds: roleIdsField.default([]), - allowedReaderRoleIds: roleIdsField.default([]), }) export type PartPresetFormValues = z.infer diff --git a/app/features/events/schemas/program-edit.schema.test.ts b/app/features/events/schemas/program-edit.schema.test.ts index 1a5f30f5..53f2fde7 100644 --- a/app/features/events/schemas/program-edit.schema.test.ts +++ b/app/features/events/schemas/program-edit.schema.test.ts @@ -171,3 +171,42 @@ describe('part preset selection', () => { expect(parsed.success).toBe(false) }) }) + +describe('managed role slots', () => { + // The part editor declares which role pickers it drew. Without that, a + // picker that was never rendered and one the user emptied both arrive as an + // absent field, and the action cannot tell "leave these rows alone" from + // "delete them" — the reader half of the 78a9219 regression. + function partForm(fields: Record) { + const fd = new FormData() + fd.set('intent', 'update-part') + fd.set('partAssignmentId', '9') + fd.set('partName', 'Sujet') + fd.set('partOrder', '1') + for (const [key, value] of Object.entries(fields)) { + for (const entry of Array.isArray(value) ? value : [value]) fd.append(key, entry) + } + return Object.fromEntries( + [...new Set([...fd.keys()])].map(key => [key, fd.getAll(key).length > 1 ? fd.getAll(key) : fd.get(key)]), + ) + } + + it('parses the slots the editor says it drew', () => { + const parsed = updatePartSchema.safeParse(partForm({ managedRoleSlots: ['speaker', 'reader'] })) + + expect(parsed.success && parsed.data.managedRoleSlots).toEqual(['speaker', 'reader']) + }) + + it('parses a single declared slot arriving as a bare value', () => { + const parsed = updatePartSchema.safeParse(partForm({ managedRoleSlots: 'speaker' })) + + expect(parsed.success && parsed.data.managedRoleSlots).toEqual(['speaker']) + }) + + it('treats an absent declaration as "managed nothing" rather than "managed both"', () => { + // A caller that never drew a picker must not clear the stored rows. + const parsed = updatePartSchema.safeParse(partForm({})) + + expect(parsed.success && parsed.data.managedRoleSlots).toEqual([]) + }) +}) diff --git a/app/features/events/schemas/program-edit.schema.ts b/app/features/events/schemas/program-edit.schema.ts index 3a00f7d9..baf995c3 100644 --- a/app/features/events/schemas/program-edit.schema.ts +++ b/app/features/events/schemas/program-edit.schema.ts @@ -1,10 +1,19 @@ import { z } from 'zod' +import { MANAGED_ROLE_SLOTS_FIELD } from '~/features/events/model/allowed-roles-write' const roleIdsField = z.preprocess( v => (Array.isArray(v) ? v : v == null || v === '' ? [] : [v]), z.array(z.coerce.number().int().positive()), ) +// Which role pickers the editor actually drew. Absent means it drew none, so +// every stored row is left alone — never "it drew both and they were empty". +// See partAllowedRolesToWrite for why the two must stay distinguishable. +const managedRoleSlotsField = z.preprocess( + v => (Array.isArray(v) ? v : v == null || v === '' ? [] : [v]), + z.array(z.string()), +) + // Free-text per-part display labels — capped at 50 to keep DB rows sensible // and prevent accidental novellas. Blank/empty admin input yields undefined so // the fallback default (i18n key) surfaces via the part-labels helper. @@ -55,6 +64,7 @@ export const addPartSchema = z.object({ partPresetId: partPresetField, allowedSpeakerRoleIds: roleIdsField.default([]), allowedReaderRoleIds: roleIdsField.default([]), + [MANAGED_ROLE_SLOTS_FIELD]: managedRoleSlotsField.default([]), }) export const deletePartSchema = z.object({ @@ -91,6 +101,7 @@ export const updatePartSchema = z.object({ partPresetId: partPresetField, allowedSpeakerRoleIds: roleIdsField.default([]), allowedReaderRoleIds: roleIdsField.default([]), + [MANAGED_ROLE_SLOTS_FIELD]: managedRoleSlotsField.default([]), }) export const updateServiceSchema = z.object({ diff --git a/app/features/events/server/allowed-roles.queries.test.ts b/app/features/events/server/allowed-roles.queries.test.ts index 3fdd6bad..b5030b2d 100644 --- a/app/features/events/server/allowed-roles.queries.test.ts +++ b/app/features/events/server/allowed-roles.queries.test.ts @@ -2,9 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('~/shared/infra/db.server', () => ({ unscopedDb: { - eventPart: { findMany: vi.fn() }, eventPartAllowedRole: { findMany: vi.fn() }, - partPresetAllowedRole: { findMany: vi.fn() }, eventServicePartAllowedRole: { findMany: vi.fn() }, }, })) @@ -20,47 +18,25 @@ beforeEach(() => { describe('getPartAssignmentAllowedRoleIdsForParts', () => { // The event page resolves every slot on the programme at once. Asking per - // part meant three queries per slot and two slots per part, so a twelve-part - // programme spent seventy-two round trips answering one question. - it('answers every part and slot from a fixed number of queries', async () => { + // part meant a query per slot and two slots per part, so a twelve-part + // programme spent two dozen round trips answering one question. + it('answers every part and slot from one query', async () => { vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([ { eventPartId: 1, asKind: 'speaker', roleId: 42 }, { eventPartId: 2, asKind: 'reader', roleId: 43 }, ] as never) - vi.mocked(db.eventPart.findMany).mockResolvedValue([ - { id: 1, presetId: null }, - { id: 2, presetId: 55 }, - ] as never) - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([ - { presetId: 55, asKind: 'speaker', roleId: 700 }, - ] as never) const result = await getPartAssignmentAllowedRoleIdsForParts(db, [1, 2], 1) expect(result.get(1)).toEqual({ speaker: [42], reader: [] }) - // The kind wins for the slot it configures, and the part still answers the - // slot the kind left alone. - expect(result.get(2)).toEqual({ speaker: [700], reader: [43] }) + expect(result.get(2)).toEqual({ speaker: [], reader: [43] }) expect(db.eventPartAllowedRole.findMany).toHaveBeenCalledTimes(1) - expect(db.eventPart.findMany).toHaveBeenCalledTimes(1) - expect(db.partPresetAllowedRole.findMany).toHaveBeenCalledTimes(1) - }) - - it('skips the kind lookup entirely when no part has one', async () => { - vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([] as never) - vi.mocked(db.eventPart.findMany).mockResolvedValue([{ id: 1, presetId: null }] as never) - - const result = await getPartAssignmentAllowedRoleIdsForParts(db, [1], 1) - - expect(result.get(1)).toEqual({ speaker: [], reader: [] }) - expect(db.partPresetAllowedRole.findMany).not.toHaveBeenCalled() }) it('answers for a part that no longer exists rather than omitting it', async () => { // The caller indexes by part id; a missing entry would read as undefined // and crash the picker rather than offer nobody. vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([] as never) - vi.mocked(db.eventPart.findMany).mockResolvedValue([] as never) const result = await getPartAssignmentAllowedRoleIdsForParts(db, [9], 1) @@ -72,7 +48,6 @@ describe('getPartAssignmentAllowedRoleIdsForParts', () => { expect(result.size).toBe(0) expect(db.eventPartAllowedRole.findMany).not.toHaveBeenCalled() - expect(db.eventPart.findMany).not.toHaveBeenCalled() }) }) diff --git a/app/features/events/server/allowed-roles.queries.ts b/app/features/events/server/allowed-roles.queries.ts index fade67e1..99d49d44 100644 --- a/app/features/events/server/allowed-roles.queries.ts +++ b/app/features/events/server/allowed-roles.queries.ts @@ -1,13 +1,12 @@ -import { resolveAllowedRoleIds } from '~/features/events/model/allowed-roles-resolution' import type { TransactionClient } from '~/shared/infra/db.server' import type { PartRoleKind } from './allowed-roles.server' // Read side of assignment eligibility, split from allowed-roles.server.ts so // the write side keeps its own budget. Resolving a whole programme at once is // the only reason these exist — see getPartAssignmentAllowedRoleIds for the -// single-assignment answer and the rule they both apply. +// single-assignment answer. -/** Both slots of one part, as the eligibility rule leaves them. */ +/** Both slots of one part. */ export type PartSlotRoleIds = Record const EMPTY_SLOTS = (): PartSlotRoleIds => ({ speaker: [], reader: [] }) @@ -15,9 +14,9 @@ const EMPTY_SLOTS = (): PartSlotRoleIds => ({ speaker: [], reader: [] }) /** * The same answer as getPartAssignmentAllowedRoleIds, for a whole programme. * - * Asking part by part costs three queries per slot and two slots per part, so - * a twelve-part programme spent seventy-two round trips resolving one page. - * This reads the three tables once each and applies the rule in memory. + * Asking part by part costs a query per slot and two slots per part, so a + * twelve-part programme spent two dozen round trips resolving one page. This + * reads the table once. * * Every requested id gets an entry, including one whose part has since been * deleted: the caller indexes by id, and a missing key would read as undefined @@ -32,48 +31,12 @@ export async function getPartAssignmentAllowedRoleIdsForParts( if (eventPartIds.length === 0) return resolved for (const id of eventPartIds) resolved.set(id, EMPTY_SLOTS()) - const [partRows, parts] = await Promise.all([ - db.eventPartAllowedRole.findMany({ - where: { eventPartId: { in: eventPartIds }, congregationId }, - select: { eventPartId: true, asKind: true, roleId: true }, - }), - db.eventPart.findMany({ - where: { id: { in: eventPartIds }, congregationId }, - select: { id: true, presetId: true }, - }), - ]) - - const ownRoleIds = new Map() - for (const row of partRows) { - const slots = ownRoleIds.get(row.eventPartId) ?? EMPTY_SLOTS() - slots[row.asKind as PartRoleKind].push(row.roleId) - ownRoleIds.set(row.eventPartId, slots) - } - - const presetIds = [...new Set(parts.map(part => part.presetId).filter((id): id is number => id != null))] - const presetRoleIds = new Map() - if (presetIds.length > 0) { - const presetRows = await db.partPresetAllowedRole.findMany({ - where: { presetId: { in: presetIds }, congregationId }, - select: { presetId: true, asKind: true, roleId: true }, - }) - for (const row of presetRows) { - const slots = presetRoleIds.get(row.presetId) ?? EMPTY_SLOTS() - slots[row.asKind as PartRoleKind].push(row.roleId) - presetRoleIds.set(row.presetId, slots) - } - } - - for (const part of parts) { - const own = ownRoleIds.get(part.id) ?? EMPTY_SLOTS() - const fromPreset = part.presetId != null ? presetRoleIds.get(part.presetId) : undefined - resolved.set(part.id, { - speaker: resolveAllowedRoleIds({ - partRoleIds: own.speaker, - presetRoleIds: fromPreset?.speaker ?? [], - }), - reader: resolveAllowedRoleIds({ partRoleIds: own.reader, presetRoleIds: fromPreset?.reader ?? [] }), - }) + const rows = await db.eventPartAllowedRole.findMany({ + where: { eventPartId: { in: eventPartIds }, congregationId }, + select: { eventPartId: true, asKind: true, roleId: true }, + }) + for (const row of rows) { + resolved.get(row.eventPartId)?.[row.asKind as PartRoleKind].push(row.roleId) } return resolved diff --git a/app/features/events/server/allowed-roles.server.test.ts b/app/features/events/server/allowed-roles.server.test.ts index 540c2aec..948d6e4d 100644 --- a/app/features/events/server/allowed-roles.server.test.ts +++ b/app/features/events/server/allowed-roles.server.test.ts @@ -8,7 +8,6 @@ vi.mock('~/shared/infra/db.server', () => ({ templatePartAllowedRole: { findMany: vi.fn(), createMany: vi.fn(), deleteMany: vi.fn() }, eventPartAllowedRole: { findMany: vi.fn(), createMany: vi.fn(), deleteMany: vi.fn() }, eventPart: { findFirst: vi.fn(), findMany: vi.fn() }, - partPresetAllowedRole: { findMany: vi.fn(), createMany: vi.fn(), deleteMany: vi.fn() }, templateServicePartAllowedRole: { findMany: vi.fn(), createMany: vi.fn(), deleteMany: vi.fn() }, eventServicePartAllowedRole: { findMany: vi.fn(), createMany: vi.fn(), deleteMany: vi.fn() }, }, @@ -17,7 +16,6 @@ vi.mock('~/shared/infra/db.server', () => ({ const { getPartAssignmentAllowedRoleIds, resolveEligibleUserIds, - setPartPresetAllowedRoles, setTemplatePartAllowedRoles, setPartAssignmentAllowedRoles, setTemplateServicePartAllowedRoles, @@ -150,109 +148,22 @@ describe('setServicePartAssignmentAllowedRoles', () => { }) }) -describe('getPartAssignmentAllowedRoleIds with a preset', () => { - it("prefers the preset's roles for the slot", async () => { - vi.mocked(db.eventPart.findFirst).mockResolvedValue({ presetId: 55 } as never) - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 700 }] as never) - vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }] as never) +describe('getPartAssignmentAllowedRoleIds', () => { + it("reads the part's own rows for the slot", async () => { + vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }, { roleId: 3 }] as never) - expect(await getPartAssignmentAllowedRoleIds(db, 9, 'speaker', 1)).toEqual([700]) - }) - - it('falls back to the part when the preset has none configured', async () => { - // An empty preset means "not configured", not "everyone" — reading it as - // authoritative would widen this part's audience to the whole congregation. - vi.mocked(db.eventPart.findFirst).mockResolvedValue({ presetId: 55 } as never) - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([] as never) - vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }] as never) - - expect(await getPartAssignmentAllowedRoleIds(db, 9, 'speaker', 1)).toEqual([1]) - }) - - it('uses the part alone when it has no preset', async () => { - vi.mocked(db.eventPart.findFirst).mockResolvedValue({ presetId: null } as never) - vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }] as never) - - expect(await getPartAssignmentAllowedRoleIds(db, 9, 'speaker', 1)).toEqual([1]) - expect(db.partPresetAllowedRole.findMany).not.toHaveBeenCalled() + expect(await getPartAssignmentAllowedRoleIds(db, 9, 'speaker', 1)).toEqual([1, 3]) + expect(db.eventPartAllowedRole.findMany).toHaveBeenCalledWith({ + where: { eventPartId: 9, asKind: 'speaker', congregationId: 1 }, + select: { roleId: true }, + }) }) it('keeps the two slots separate', async () => { - vi.mocked(db.eventPart.findFirst).mockResolvedValue({ presetId: 55 } as never) - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 700 }] as never) vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([] as never) await getPartAssignmentAllowedRoleIds(db, 9, 'reader', 1) - expect(vi.mocked(db.partPresetAllowedRole.findMany).mock.calls[0]?.[0]?.where?.asKind).toBe('reader') - }) -}) - -describe('setPartAssignmentAllowedRoles on a part that carries a kind', () => { - // The read side resolves the kind first, so it answers "who may fill this - // slot". The write side must not reuse that answer as its baseline: the rows - // it adds and deletes belong to the part, and diffing them against the kind's - // list makes every write wrong in a different way. - it("clears the part's own rows when the selection is emptied", async () => { - vi.mocked(db.eventPart.findFirst).mockResolvedValue({ presetId: 55 } as never) - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 700 }] as never) - vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([{ roleId: 42 }] as never) - - const diff = await setPartAssignmentAllowedRoles(db, 9, 'speaker', [], 1) - - expect(diff.removed).toEqual([42]) - expect(vi.mocked(db.eventPartAllowedRole.deleteMany).mock.calls[0]?.[0]?.where?.roleId).toEqual({ in: [42] }) - }) - - it('reports no change when the selection already matches the part', async () => { - vi.mocked(db.eventPart.findFirst).mockResolvedValue({ presetId: 55 } as never) - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 700 }] as never) - vi.mocked(db.eventPartAllowedRole.findMany).mockResolvedValue([{ roleId: 42 }] as never) - - const diff = await setPartAssignmentAllowedRoles(db, 9, 'speaker', [42], 1) - - expect(diff).toEqual({ added: [], removed: [] }) - expect(db.eventPartAllowedRole.createMany).not.toHaveBeenCalled() - expect(db.eventPartAllowedRole.deleteMany).not.toHaveBeenCalled() - }) -}) - -describe('setPartPresetAllowedRoles', () => { - it('adds only the roles that are missing', async () => { - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }] as never) - - const diff = await setPartPresetAllowedRoles(db, 55, 'speaker', [1, 2], 1) - - expect(diff.added).toEqual([2]) - expect(vi.mocked(db.partPresetAllowedRole.createMany).mock.calls[0]?.[0]?.data).toEqual([ - { presetId: 55, roleId: 2, asKind: 'speaker', congregationId: 1 }, - ]) - }) - - it('removes the roles that are no longer wanted', async () => { - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }, { roleId: 2 }] as never) - - const diff = await setPartPresetAllowedRoles(db, 55, 'speaker', [1], 1) - - expect(diff.removed).toEqual([2]) - }) - - it('writes nothing when the selection is unchanged', async () => { - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }] as never) - - await setPartPresetAllowedRoles(db, 55, 'speaker', [1], 1) - - expect(db.partPresetAllowedRole.createMany).not.toHaveBeenCalled() - expect(db.partPresetAllowedRole.deleteMany).not.toHaveBeenCalled() - }) - - it('clearing every role leaves the kind unconfigured, not forbidden', async () => { - // Empty means "any member" downstream, so this is how a kind stops - // restricting rather than how it blocks everyone. - vi.mocked(db.partPresetAllowedRole.findMany).mockResolvedValue([{ roleId: 1 }] as never) - - const diff = await setPartPresetAllowedRoles(db, 55, 'speaker', [], 1) - - expect(diff.removed).toEqual([1]) + expect(vi.mocked(db.eventPartAllowedRole.findMany).mock.calls[0]?.[0]?.where?.asKind).toBe('reader') }) }) diff --git a/app/features/events/server/allowed-roles.server.ts b/app/features/events/server/allowed-roles.server.ts index 34177375..e0fd15c3 100644 --- a/app/features/events/server/allowed-roles.server.ts +++ b/app/features/events/server/allowed-roles.server.ts @@ -1,4 +1,3 @@ -import { resolveAllowedRoleIds } from '~/features/events/model/allowed-roles-resolution' import { findMembersWithAnyRole } from '~/shared/auth/permissions.server' import type { TransactionClient } from '~/shared/infra/db.server' @@ -54,32 +53,10 @@ async function getTemplatePartAllowedRoleIds( return rows.map(r => r.roleId) } -/** The part's own rows, before the kind has any say. */ -async function getPartOwnAllowedRoleIds( - db: TransactionClient, - eventPartId: number, - asKind: PartRoleKind, - congregationId: number, -): Promise { - const rows = await db.eventPartAllowedRole.findMany({ - where: { eventPartId, asKind, congregationId }, - select: { roleId: true }, - }) - return rows.map(r => r.roleId) -} - /** - * Which roles may fill a slot on an assignment. - * - * The kind decides when it has roles configured; otherwise the part's own rows - * apply. See resolveAllowedRoleIds for why an empty preset cannot win here — - * empty means "any member", so an unconfigured kind would widen rather than - * restrict. - * - * This is the eligibility answer, not the part's stored state. Anything writing - * `EventPartAllowedRole` must read getPartOwnAllowedRoleIds instead — diffing a - * write against a list that may belong to the kind deletes rows the part never - * had and leaves the ones it does. + * Which roles may fill a slot on an assignment. The part's own rows are the + * single source of truth — the kind carries capability (reader slot, labels, + * share message), never eligibility. Empty means "any member". */ export async function getPartAssignmentAllowedRoleIds( db: TransactionClient, @@ -87,20 +64,11 @@ export async function getPartAssignmentAllowedRoleIds( asKind: PartRoleKind, congregationId: number, ): Promise { - const partRoleIds = await getPartOwnAllowedRoleIds(db, eventPartId, asKind, congregationId) - - const part = await db.eventPart.findFirst({ - where: { id: eventPartId, congregationId }, - select: { presetId: true }, - }) - if (!part?.presetId) return partRoleIds - - const presetRows = await db.partPresetAllowedRole.findMany({ - where: { presetId: part.presetId, asKind, congregationId }, + const rows = await db.eventPartAllowedRole.findMany({ + where: { eventPartId, asKind, congregationId }, select: { roleId: true }, }) - - return resolveAllowedRoleIds({ partRoleIds, presetRoleIds: presetRows.map(r => r.roleId) }) + return rows.map(r => r.roleId) } async function getTemplateServicePartAllowedRoleIds( @@ -143,45 +111,6 @@ function diffRoleIds(previous: number[], desired: number[]): DiffResult { } } -async function getPartPresetAllowedRoleIds( - db: TransactionClient, - presetId: number, - asKind: PartRoleKind, - congregationId: number, -): Promise { - const rows = await db.partPresetAllowedRole.findMany({ - where: { presetId, asKind, congregationId }, - select: { roleId: true }, - }) - return rows.map(r => r.roleId) -} - -/** Mirrors setTemplatePartAllowedRoles, for the kind rather than one part. */ -export async function setPartPresetAllowedRoles( - db: TransactionClient, - presetId: number, - asKind: PartRoleKind, - desiredRoleIds: number[], - congregationId: number, -): Promise { - const previous = await getPartPresetAllowedRoleIds(db, presetId, asKind, congregationId) - const diff = diffRoleIds(previous, desiredRoleIds) - if (diff.added.length === 0 && diff.removed.length === 0) return diff - - if (diff.removed.length > 0) { - await db.partPresetAllowedRole.deleteMany({ - where: { presetId, asKind, congregationId, roleId: { in: diff.removed } }, - }) - } - if (diff.added.length > 0) { - await db.partPresetAllowedRole.createMany({ - data: diff.added.map(roleId => ({ presetId, roleId, asKind, congregationId })), - skipDuplicates: true, - }) - } - return diff -} - export async function setTemplatePartAllowedRoles( db: TransactionClient, partId: number, @@ -214,7 +143,7 @@ export async function setPartAssignmentAllowedRoles( desiredRoleIds: number[], congregationId: number, ): Promise { - const previous = await getPartOwnAllowedRoleIds(db, eventPartId, asKind, congregationId) + const previous = await getPartAssignmentAllowedRoleIds(db, eventPartId, asKind, congregationId) const diff = diffRoleIds(previous, desiredRoleIds) if (diff.added.length === 0 && diff.removed.length === 0) return diff diff --git a/app/features/events/server/event-parts.server.ts b/app/features/events/server/event-parts.server.ts index e65fb58a..1e68352c 100644 --- a/app/features/events/server/event-parts.server.ts +++ b/app/features/events/server/event-parts.server.ts @@ -97,10 +97,8 @@ const NO_ROLE_CHANGE = { added: [] as number[], removed: [] as number[] } * Write the slots the caller actually managed. * * Undefined and [] are different: [] is "the editor offered a selection and it - * is empty", undefined is "the editor never showed one". The part form hides - * its role pickers once a kind is chosen, so it sends undefined — and the - * part's own rows must survive, because that is what the kind falls back to - * while it restricts nobody. + * is empty" and clears the slot's rows, undefined is "this caller does not + * manage eligibility" and leaves them alone. */ async function writePartAllowedRoles( db: TransactionClient, diff --git a/app/features/events/server/event-templates.server.ts b/app/features/events/server/event-templates.server.ts index 7dab12a5..dc885594 100644 --- a/app/features/events/server/event-templates.server.ts +++ b/app/features/events/server/event-templates.server.ts @@ -88,8 +88,8 @@ export async function upsertTemplatePart( // The kind this part defaults to. Null where the template genuinely cannot // know — the ministry parts change kind weekly and are set per event. presetId?: number | null - // Optional on purpose: undefined means the editor did not manage the slot, - // [] means it did and the selection is empty. See partAllowedRolesToWrite. + // Optional on purpose: undefined means the caller does not manage the + // slot, [] means it does and the selection is empty. allowedSpeakerRoleIds?: number[] allowedReaderRoleIds?: number[] }, diff --git a/app/features/events/server/part-presets.queries.ts b/app/features/events/server/part-presets.queries.ts index 72a68703..6a31f594 100644 --- a/app/features/events/server/part-presets.queries.ts +++ b/app/features/events/server/part-presets.queries.ts @@ -66,8 +66,5 @@ export function listPartPresetsForSettings(db: TransactionClient, congregationId /** Returns null when the preset does not exist in this congregation. */ export function getPartPresetById(db: TransactionClient, id: number, congregationId: number) { - return db.partPreset.findFirst({ - where: { id, congregationId }, - include: { allowedRoles: { select: { roleId: true, asKind: true } } }, - }) + return db.partPreset.findFirst({ where: { id, congregationId } }) } diff --git a/app/features/events/server/part-presets.server.test.ts b/app/features/events/server/part-presets.server.test.ts index 00b268c3..9034869b 100644 --- a/app/features/events/server/part-presets.server.test.ts +++ b/app/features/events/server/part-presets.server.test.ts @@ -1,10 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const setPartPresetAllowedRoles = vi.fn().mockResolvedValue({ added: [], removed: [] }) -vi.mock('~/features/events/server/allowed-roles.server', () => ({ - setPartPresetAllowedRoles: (...args: unknown[]) => setPartPresetAllowedRoles(...args), -})) - vi.mock('~/shared/domain/audit.server', () => ({ audit: vi.fn(), AuditAction: { @@ -36,8 +31,6 @@ function input(overrides: Record = {}) { readerLabel: null, allowExternalSpeaker: true, shareMessage: 'Bonjour {{assigneeFirstname}}, tu as {{partName}} le {{date}}.', - allowedSpeakerRoleIds: [], - allowedReaderRoleIds: [], ...overrides, } } @@ -52,7 +45,6 @@ function updatedData(db: ReturnType) { beforeEach(() => { vi.resetAllMocks() - setPartPresetAllowedRoles.mockResolvedValue({ added: [], removed: [] }) }) describe('createPartPreset', () => { @@ -283,38 +275,3 @@ describe('deletePartPreset', () => { expect(db.partPreset.delete).toHaveBeenCalled() }) }) - -describe('preset allowed roles', () => { - it('stores the roles chosen for each slot', async () => { - const db = makeDb() - - await createPartPreset( - db as never, - input({ hasReaderSlot: true, allowedSpeakerRoleIds: [4], allowedReaderRoleIds: [9] }), - 1, - 7, - ) - - expect(setPartPresetAllowedRoles).toHaveBeenCalledWith(expect.anything(), 1, 'speaker', [4], 1) - expect(setPartPresetAllowedRoles).toHaveBeenCalledWith(expect.anything(), 1, 'reader', [9], 1) - }) - - it('clears reader roles for a kind that has no reader slot', async () => { - // Otherwise the selection applies to a slot nobody is ever offered, and - // silently comes back if the slot is re-enabled later. - const db = makeDb() - - await createPartPreset(db as never, input({ hasReaderSlot: false, allowedReaderRoleIds: [9] }), 1, 7) - - expect(setPartPresetAllowedRoles).toHaveBeenCalledWith(expect.anything(), 1, 'reader', [], 1) - }) - - it('writes them on update too', async () => { - const db = makeDb() - db.partPreset.findFirst.mockResolvedValue({ id: 5, key: 'x', isSystem: false } as never) - - await updatePartPreset(db as never, 5, input({ allowedSpeakerRoleIds: [4] }), 1, 7) - - expect(setPartPresetAllowedRoles).toHaveBeenCalledWith(expect.anything(), 5, 'speaker', [4], 1) - }) -}) diff --git a/app/features/events/server/part-presets.server.ts b/app/features/events/server/part-presets.server.ts index 8fab513b..d8582262 100644 --- a/app/features/events/server/part-presets.server.ts +++ b/app/features/events/server/part-presets.server.ts @@ -1,6 +1,5 @@ import { PartPresetScope } from '~/features/events/model/part-preset.type' import { partPresetName } from '~/features/events/model/part-preset-defaults' -import { setPartPresetAllowedRoles } from '~/features/events/server/allowed-roles.server' import { AuditAction, audit } from '~/shared/domain/audit.server' import type { TransactionClient } from '~/shared/infra/db.server' @@ -15,8 +14,6 @@ export interface PartPresetInput { readerLabel: string | null allowExternalSpeaker: boolean shareMessage: string | null - allowedSpeakerRoleIds: number[] - allowedReaderRoleIds: number[] } /** @@ -67,29 +64,6 @@ function isUniqueViolation(error: unknown): boolean { return typeof error === 'object' && error !== null && (error as { code?: string }).code === 'P2002' } -/** - * Eligibility for the two slots. - * - * A kind with no reader slot cannot have reader roles — the selection would - * apply to a slot that is never offered, and would come back into effect if the - * slot were later re-enabled without anyone revisiting it. - */ -async function writeAllowedRoles( - db: TransactionClient, - presetId: number, - data: PartPresetInput, - congregationId: number, -): Promise { - await setPartPresetAllowedRoles(db, presetId, 'speaker', data.allowedSpeakerRoleIds, congregationId) - await setPartPresetAllowedRoles( - db, - presetId, - 'reader', - data.hasReaderSlot ? data.allowedReaderRoleIds : [], - congregationId, - ) -} - export async function createPartPreset( db: TransactionClient, data: PartPresetInput, @@ -120,8 +94,6 @@ export async function createPartPreset( } if (!preset) throw new Error('createPartPreset: no preset created') - await writeAllowedRoles(db, preset.id, data, congregationId) - audit({ action: AuditAction.PartPresetCreated, congregationId, @@ -156,8 +128,6 @@ export async function updatePartPreset( data: normalize(data), }) - await writeAllowedRoles(db, id, data, congregationId) - audit({ action: AuditAction.PartPresetUpdated, congregationId, diff --git a/app/features/events/server/seed-part-presets.server.test.ts b/app/features/events/server/seed-part-presets.server.test.ts index ca9bcfde..127cd028 100644 --- a/app/features/events/server/seed-part-presets.server.test.ts +++ b/app/features/events/server/seed-part-presets.server.test.ts @@ -90,9 +90,9 @@ describe('seedDefaultPartPresets', () => { expect(rowFor(db, 'watchtower-study')?.hasReaderSlot).toBe(true) }) - it('allows an external speaker on prayer but not on spiritual gems', async () => { - // Both confirmed explicitly — spiritual gems is a local assignment, whereas - // a visiting brother may offer prayer. + it('allows an external speaker on prayer but not on the Bible reading', async () => { + // Both confirmed explicitly — the Bible reading is a local assignment, + // whereas a visiting brother may offer prayer. const db = makeDb() db.partPreset.findFirst.mockResolvedValue(null as never) db.partPreset.create.mockResolvedValue({} as never) @@ -100,7 +100,7 @@ describe('seedDefaultPartPresets', () => { await seedDefaultPartPresets(db, 1, 'fr') expect(rowFor(db, 'prayer')?.allowExternalSpeaker).toBe(true) - expect(rowFor(db, 'spiritual-gems')?.allowExternalSpeaker).toBe(false) + expect(rowFor(db, 'bible-reading')?.allowExternalSpeaker).toBe(false) }) it.each(['fr', 'en'] as const)('uses only known variables in every %s message', async locale => { diff --git a/app/features/events/server/seed-part-presets.server.ts b/app/features/events/server/seed-part-presets.server.ts index ec661e3b..d9f124e5 100644 --- a/app/features/events/server/seed-part-presets.server.ts +++ b/app/features/events/server/seed-part-presets.server.ts @@ -22,14 +22,13 @@ const PRESETS: PresetCapability[] = [ // A visiting brother may offer prayer. { key: PartPresetKey.Prayer, hasReaderSlot: false, allowExternalSpeaker: true }, { key: PartPresetKey.Chairman, hasReaderSlot: false, allowExternalSpeaker: false }, - // Local assignment. - { key: PartPresetKey.SpiritualGems, hasReaderSlot: false, allowExternalSpeaker: false }, - { key: PartPresetKey.SpiritualPearls, hasReaderSlot: false, allowExternalSpeaker: false }, + // One kind for every midweek-meeting talk (Joyaux, Perles, Vie chrétienne). + // External speakers stay possible: a visiting brother may take a talk. + { key: PartPresetKey.MidweekTalk, hasReaderSlot: false, allowExternalSpeaker: true }, { key: PartPresetKey.BibleReading, hasReaderSlot: false, allowExternalSpeaker: false }, // The only school part with two people on stage. { key: PartPresetKey.SchoolDemonstration, hasReaderSlot: true, allowExternalSpeaker: false }, { key: PartPresetKey.SchoolTalk, hasReaderSlot: false, allowExternalSpeaker: false }, - { key: PartPresetKey.ChristianLifeTalk, hasReaderSlot: false, allowExternalSpeaker: true }, { key: PartPresetKey.PublicTalk, hasReaderSlot: false, allowExternalSpeaker: true }, { key: PartPresetKey.WatchtowerStudy, hasReaderSlot: true, allowExternalSpeaker: false }, { key: PartPresetKey.CongregationBibleStudy, hasReaderSlot: true, allowExternalSpeaker: false }, @@ -46,10 +45,6 @@ export const PART_PRESET_COUNT = PRESETS.length * * The locale argument is retained for call-site compatibility and is no longer * used: nothing language-specific is stored any more. - * - * Deliberately does not populate allowedRoles either. Eligibility is set on the - * kind through the preset editor, and a seeded restriction nobody asked for - * would narrow who can be assigned from the first day. */ // biome-ignore lint/suspicious/noExplicitAny: matches seedDefaultTemplates, called with a scoped client export async function seedDefaultPartPresets(db: any, congregationId: number, _locale: Locale): Promise { diff --git a/app/features/events/server/seed-templates.server.test.ts b/app/features/events/server/seed-templates.server.test.ts index 4c163290..1a02404e 100644 --- a/app/features/events/server/seed-templates.server.test.ts +++ b/app/features/events/server/seed-templates.server.test.ts @@ -6,9 +6,8 @@ const { seedDefaultTemplates } = await import('./seed-templates.server') // mistaken for a coincidental match. const SEEDED_PRESETS = [ { id: 901, key: 'prayer' }, - { id: 902, key: 'spiritual-gems' }, + { id: 902, key: 'midweek-talk' }, { id: 903, key: 'bible-reading' }, - { id: 904, key: 'christian-life-talk' }, ] function makeDb() { diff --git a/app/features/events/server/seed-templates.server.ts b/app/features/events/server/seed-templates.server.ts index ddd0a624..af3d14f6 100644 --- a/app/features/events/server/seed-templates.server.ts +++ b/app/features/events/server/seed-templates.server.ts @@ -69,7 +69,7 @@ function getTemplates(locale: Locale): TemplateDefinition[] { }, { name: m.seed_part_discourse({}, { locale }), - preset: PartPresetKey.SpiritualGems, + preset: PartPresetKey.MidweekTalk, section: m.seed_section_spiritual_gems({}, { locale }), order: 2, durationMin: 10, @@ -77,7 +77,7 @@ function getTemplates(locale: Locale): TemplateDefinition[] { }, { name: m.seed_part_search_spiritual_pearls({}, { locale }), - preset: PartPresetKey.SpiritualPearls, + preset: PartPresetKey.MidweekTalk, section: m.seed_section_spiritual_gems({}, { locale }), order: 3, durationMin: 10, @@ -115,7 +115,7 @@ function getTemplates(locale: Locale): TemplateDefinition[] { { name: m.seed_part_song({}, { locale }), section: '', order: 8, durationMin: 5, allowExternalSpeaker: false }, { name: m.seed_part_first_part({}, { locale }), - preset: PartPresetKey.ChristianLifeTalk, + preset: PartPresetKey.MidweekTalk, section: m.seed_section_christian_life({}, { locale }), order: 9, durationMin: null, @@ -123,7 +123,7 @@ function getTemplates(locale: Locale): TemplateDefinition[] { }, { name: m.seed_part_second_part({}, { locale }), - preset: PartPresetKey.ChristianLifeTalk, + preset: PartPresetKey.MidweekTalk, section: m.seed_section_christian_life({}, { locale }), order: 10, durationMin: null, diff --git a/app/features/events/ui/PartEditSheet.tsx b/app/features/events/ui/PartEditSheet.tsx index 4f9b57ef..6a83df2c 100644 --- a/app/features/events/ui/PartEditSheet.tsx +++ b/app/features/events/ui/PartEditSheet.tsx @@ -1,5 +1,6 @@ import { useEffect, useRef, useState } from 'react' import type { useFetcher } from 'react-router' +import { MANAGED_ROLE_SLOTS_FIELD } from '~/features/events/model/allowed-roles-write' import { resolvePartCapability } from '~/features/events/model/part-capability' import { partReaderLabel, partSpeakerLabel } from '~/features/events/model/part-labels' import { NO_PRESET_VALUE } from '~/features/events/schemas/program-edit.schema' @@ -223,10 +224,11 @@ export function PartEditSheet({ {partSpeakerLabel({ speakerLabel: capability.speakerLabel, readerLabel: null })}
- {/* Only offered without a kind. With one, the labels, the - external-speaker rule and the eligible roles all belong to - the preset — editing them here would suggest an override the - model does not have. The summary above shows what applies. */} + {/* Label and external-speaker rule are capability and belong to + the kind when one is chosen — editing them here would suggest + an override the model does not have. The summary above shows + what applies. Eligibility stays per part: two parts of the + same kind can be done by different roles. */} {!selectedPreset && ( <>
@@ -249,20 +251,23 @@ export function PartEditSheet({
)} - {!selectedPreset && ( -
- - -

{m.programs_edit_part_external_speaker_note()}

-
- )} +
+ {/* Declared alongside the picker, not somewhere else in the + form: an empty picker and an absent one submit the same + thing, so the action can only tell them apart if whatever + draws the picker also says it did. */} + + + +

{m.programs_edit_part_external_speaker_note()}

+
@@ -286,19 +291,21 @@ export function PartEditSheet({ /> )} - {!selectedPreset && ( -
- - -
- )} +
+ {/* This whole section disappears when the kind has no second + slot, which is exactly when the part's stored reader roles + must survive untouched. */} + + + +
)} diff --git a/app/features/events/ui/PartPresetForm.tsx b/app/features/events/ui/PartPresetForm.tsx index 29f2b656..6da84704 100644 --- a/app/features/events/ui/PartPresetForm.tsx +++ b/app/features/events/ui/PartPresetForm.tsx @@ -7,7 +7,6 @@ import { Button } from '~/shared/ui/button' import { Checkbox } from '~/shared/ui/checkbox' import { Input } from '~/shared/ui/input' import { Label } from '~/shared/ui/label' -import { type RoleOption, RolePicker } from '~/shared/ui/RolePicker' import { Textarea } from '~/shared/ui/textarea' type PartPresetFormProps = { @@ -15,7 +14,6 @@ type PartPresetFormProps = { // from what the action will accept. preset: PartPresetFormValues | null isSystem: boolean - roles: RoleOption[] /** Catalogue wording, shown when a field is blank. Leaving a field empty is how a congregation keeps the built-in text and follows its language. */ placeholders?: { name: string; speakerLabel: string; readerLabel: string; shareMessage: string } @@ -42,7 +40,7 @@ const PREVIEW_CONTEXT: ShareMessageContext = { link: 'https://unitae.app/board', } -export function PartPresetForm({ preset, isSystem, roles, placeholders, errors }: PartPresetFormProps) { +export function PartPresetForm({ preset, isSystem, placeholders, errors }: PartPresetFormProps) { const [message, setMessage] = useState(preset?.shareMessage ?? '') const [hasReaderSlot, setHasReaderSlot] = useState(preset?.hasReaderSlot ?? false) @@ -127,33 +125,6 @@ export function PartPresetForm({ preset, isSystem, roles, placeholders, errors } )} - {/* Eligibility belongs to the kind, so it is set once here rather than - repeated on every part that uses it. An empty selection means any - member — that is the widest setting, not the narrowest. */} -
- - -
- - {hasReaderSlot && ( -
- - -
- )} -