From 007a908e833cdd1b229f485c34c17c6510a55f9c Mon Sep 17 00:00:00 2001 From: Dries Augustyns Date: Thu, 18 Dec 2025 19:43:59 +0100 Subject: [PATCH] fix: Persistence of subscription state for existing contacts --- .../src/__tests__/integration/actions.test.ts | 300 ++++++++++++++++++ apps/api/src/controllers/Actions.ts | 4 +- .../jobs/__tests__/import-processor.test.ts | 281 ++++++++++++++++ apps/api/src/jobs/import-processor.ts | 6 +- packages/shared/src/schemas/index.ts | 4 +- 5 files changed, 589 insertions(+), 6 deletions(-) create mode 100644 apps/api/src/jobs/__tests__/import-processor.test.ts diff --git a/apps/api/src/__tests__/integration/actions.test.ts b/apps/api/src/__tests__/integration/actions.test.ts index a5ad8c9..4e75dc1 100644 --- a/apps/api/src/__tests__/integration/actions.test.ts +++ b/apps/api/src/__tests__/integration/actions.test.ts @@ -733,4 +733,304 @@ describe('Actions API Integration Tests', () => { }); }); }); + + // ======================================== + // SUBSCRIPTION STATUS PRESERVATION + // ======================================== + describe('Subscription Status Preservation', () => { + describe('/v1/send endpoint', () => { + it('should NOT change subscription status when sending to subscribed contact without subscribed field', async () => { + // Create a subscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: true, + email: 'subscribed@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(true); + + // Send transactional email without specifying subscribed field + await EmailService.sendTransactionalEmail({ + projectId, + contactId: contact.id, + subject: 'Test', + body: 'Test', + from: 'test@example.com', + }); + + // Verify subscription status unchanged + const updatedContact = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updatedContact?.subscribed).toBe(true); + }); + + it('should NOT change subscription status when sending to unsubscribed contact without subscribed field', async () => { + // Create an unsubscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'unsubscribed@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(false); + + // Send transactional email without specifying subscribed field + await EmailService.sendTransactionalEmail({ + projectId, + contactId: contact.id, + subject: 'Test', + body: 'Test', + from: 'test@example.com', + }); + + // Verify subscription status unchanged (should still be false) + const updatedContact = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updatedContact?.subscribed).toBe(false); + }); + + it('should allow explicit subscription when subscribed=true is provided', async () => { + // Create an unsubscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'resubscribe@example.com', + }); + + // This test would need to be implemented at the controller level + // since EmailService.sendTransactionalEmail doesn't accept subscribed parameter + // For now, verify the schema allows it + const result = ActionSchemas.send.safeParse({ + to: contact.email, + subject: 'Test', + body: 'Test', + from: 'test@example.com', + subscribed: true, + }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.subscribed).toBe(true); + } + }); + + it('should allow explicit unsubscription when subscribed=false is provided', async () => { + // Verify the schema allows explicit false + const result = ActionSchemas.send.safeParse({ + to: 'test@example.com', + subject: 'Test', + body: 'Test', + from: 'test@example.com', + subscribed: false, + }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.subscribed).toBe(false); + } + }); + + it('should default to undefined when subscribed field is omitted', () => { + const result = ActionSchemas.send.safeParse({ + to: 'test@example.com', + subject: 'Test', + body: 'Test', + from: 'test@example.com', + }); + + expect(result.success).toBe(true); + if (result.success) { + // Should be undefined, not false + expect(result.data.subscribed).toBeUndefined(); + } + }); + }); + + describe('/v1/track endpoint', () => { + it('should NOT change subscription status when tracking event for subscribed contact', async () => { + // Create a subscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: true, + email: 'track-subscribed@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(true); + + // Track event without specifying subscribed field + // This would be done via ContactService.upsert in the track endpoint + const {ContactService} = await import('../../services/ContactService.js'); + await ContactService.upsert(projectId, contact.email, {event: 'test'}, undefined); + + // Verify subscription status unchanged + const updatedContact = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updatedContact?.subscribed).toBe(true); + }); + + it('should NOT re-subscribe unsubscribed contact when tracking event', async () => { + // Create an unsubscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'track-unsubscribed@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(false); + + // Track event without specifying subscribed field + const {ContactService} = await import('../../services/ContactService.js'); + await ContactService.upsert(projectId, contact.email, {event: 'test'}, undefined); + + // Verify subscription status unchanged (should still be false, NOT re-subscribed) + const updatedContact = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updatedContact?.subscribed).toBe(false); + }); + + it('should create new contacts as subscribed when subscribed is undefined', async () => { + const newEmail = 'new-track-contact@example.com'; + + // Track event for new contact without specifying subscribed + const {ContactService} = await import('../../services/ContactService.js'); + const contact = await ContactService.upsert(projectId, newEmail, {event: 'test'}, undefined); + + // New contacts should default to subscribed=true + expect(contact.subscribed).toBe(true); + }); + + it('should allow explicit subscription when subscribed=true is provided', async () => { + const result = ActionSchemas.track.safeParse({ + event: 'test', + email: 'test@example.com', + subscribed: true, + }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.subscribed).toBe(true); + } + }); + + it('should allow explicit unsubscription when subscribed=false is provided', async () => { + const result = ActionSchemas.track.safeParse({ + event: 'test', + email: 'test@example.com', + subscribed: false, + }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.subscribed).toBe(false); + } + }); + + it('should default to undefined when subscribed field is omitted', () => { + const result = ActionSchemas.track.safeParse({ + event: 'test', + email: 'test@example.com', + }); + + expect(result.success).toBe(true); + if (result.success) { + // Should be undefined, not true + expect(result.data.subscribed).toBeUndefined(); + } + }); + }); + + describe('ContactService.upsert behavior', () => { + it('should preserve subscription status when undefined is passed for existing contact', async () => { + // Create a subscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: true, + email: 'upsert-test@example.com', + }); + + const {ContactService} = await import('../../services/ContactService.js'); + + // Update with undefined subscribed + await ContactService.upsert(projectId, contact.email, {firstName: 'John'}, undefined); + + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(true); + }); + + it('should preserve unsubscribed status when undefined is passed', async () => { + // Create an unsubscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'upsert-unsub@example.com', + }); + + const {ContactService} = await import('../../services/ContactService.js'); + + // Update with undefined subscribed + await ContactService.upsert(projectId, contact.email, {firstName: 'Jane'}, undefined); + + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(false); + }); + + it('should allow explicit subscription change to true', async () => { + // Create an unsubscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'explicit-sub@example.com', + }); + + const {ContactService} = await import('../../services/ContactService.js'); + + // Explicitly subscribe + await ContactService.upsert(projectId, contact.email, {}, true); + + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(true); + }); + + it('should allow explicit subscription change to false', async () => { + // Create a subscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: true, + email: 'explicit-unsub@example.com', + }); + + const {ContactService} = await import('../../services/ContactService.js'); + + // Explicitly unsubscribe + await ContactService.upsert(projectId, contact.email, {}, false); + + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(false); + }); + }); + }); }); diff --git a/apps/api/src/controllers/Actions.ts b/apps/api/src/controllers/Actions.ts index 58deac8..00314bb 100644 --- a/apps/api/src/controllers/Actions.ts +++ b/apps/api/src/controllers/Actions.ts @@ -26,7 +26,7 @@ export class Actions { * Request body: * - event: string (required) - Event name * - email: string (required) - Contact email - * - subscribed: boolean (optional, default: true) - Contact subscription status + * - subscribed: boolean (optional) - Contact subscription status (only updates if explicitly specified) * - data: object (optional) - Event and contact data * - Simple values are saved to contact (persistent) * - {value: any, persistent: false} are only available to workflows (non-persistent) @@ -111,7 +111,7 @@ export class Actions { * - Array: ["user1@example.com", {name: "Jane", email: "user2@example.com"}] * - subject: string (required) - Email subject * - body: string (required) - Email HTML body - * - subscribed: boolean (optional, default: false) - Contact subscription status + * - subscribed: boolean (optional) - Contact subscription status (only updates if explicitly specified) * - name: string (optional) - Sender name (alternative to from.name) * - from: string | object (optional) - Sender email or {name, email} object (must be from verified domain) * - reply: string (optional) - Reply-to email diff --git a/apps/api/src/jobs/__tests__/import-processor.test.ts b/apps/api/src/jobs/__tests__/import-processor.test.ts new file mode 100644 index 0000000..b246699 --- /dev/null +++ b/apps/api/src/jobs/__tests__/import-processor.test.ts @@ -0,0 +1,281 @@ +import {beforeEach, describe, expect, it} from 'vitest'; +import {factories, getPrismaClient} from '../../../../../test/helpers'; +import {ContactService} from '../../services/ContactService.js'; + +/** + * Tests for Contact Import Processor - Subscription Status Preservation + * Verifies that CSV imports preserve subscription status correctly + */ +describe('Contact Import - Subscription Status Preservation', () => { + let projectId: string; + const prisma = getPrismaClient(); + + beforeEach(async () => { + const {project} = await factories.createUserWithProject(); + projectId = project.id; + }); + + describe('Existing contacts', () => { + it('should NOT change subscription status when CSV has no subscribed column for subscribed contact', async () => { + // Create a subscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: true, + email: 'existing-subscribed@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(true); + + // Simulate import without subscribed column (undefined) + await ContactService.upsert(projectId, contact.email, {firstName: 'John'}, undefined); + + // Verify subscription status unchanged + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(true); + }); + + it('should NOT re-subscribe unsubscribed contact when CSV has no subscribed column', async () => { + // Create an unsubscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'existing-unsubscribed@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(false); + + // Simulate import without subscribed column (undefined) + await ContactService.upsert(projectId, contact.email, {firstName: 'Jane'}, undefined); + + // Verify subscription status unchanged (should NOT be re-subscribed) + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(false); + }); + + it('should update subscription status when CSV explicitly has subscribed=true', async () => { + // Create an unsubscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'import-resubscribe@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(false); + + // Simulate import with explicit subscribed=true + await ContactService.upsert(projectId, contact.email, {firstName: 'John'}, true); + + // Verify subscription status changed + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(true); + }); + + it('should update subscription status when CSV explicitly has subscribed=false', async () => { + // Create a subscribed contact + const contact = await factories.createContact({ + projectId, + subscribed: true, + email: 'import-unsubscribe@example.com', + }); + + // Verify initial state + expect(contact.subscribed).toBe(true); + + // Simulate import with explicit subscribed=false + await ContactService.upsert(projectId, contact.email, {firstName: 'Jane'}, false); + + // Verify subscription status changed + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + expect(updated?.subscribed).toBe(false); + }); + }); + + describe('New contacts', () => { + it('should create new contact as subscribed when CSV has no subscribed column', async () => { + const newEmail = 'new-import-default@example.com'; + + // Simulate import without subscribed column (undefined) + const contact = await ContactService.upsert(projectId, newEmail, {firstName: 'New'}, undefined); + + // New contacts should default to subscribed=true + expect(contact.subscribed).toBe(true); + }); + + it('should create new contact as subscribed when CSV explicitly has subscribed=true', async () => { + const newEmail = 'new-import-explicit-sub@example.com'; + + // Simulate import with explicit subscribed=true + const contact = await ContactService.upsert(projectId, newEmail, {firstName: 'New'}, true); + + expect(contact.subscribed).toBe(true); + }); + + it('should create new contact as unsubscribed when CSV explicitly has subscribed=false', async () => { + const newEmail = 'new-import-explicit-unsub@example.com'; + + // Simulate import with explicit subscribed=false + const contact = await ContactService.upsert(projectId, newEmail, {firstName: 'New'}, false); + + expect(contact.subscribed).toBe(false); + }); + }); + + describe('CSV parsing logic', () => { + it('should parse "true" string as boolean true', () => { + const subscribedValue = 'true'; + const lowerValue = subscribedValue.toLowerCase().trim(); + const subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + + expect(subscribed).toBe(true); + }); + + it('should parse "1" string as boolean true', () => { + const subscribedValue = '1'; + const lowerValue = subscribedValue.toLowerCase().trim(); + const subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + + expect(subscribed).toBe(true); + }); + + it('should parse "yes" string as boolean true', () => { + const subscribedValue = 'yes'; + const lowerValue = subscribedValue.toLowerCase().trim(); + const subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + + expect(subscribed).toBe(true); + }); + + it('should parse "false" string as boolean false', () => { + const subscribedValue = 'false'; + const lowerValue = subscribedValue.toLowerCase().trim(); + const subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + + expect(subscribed).toBe(false); + }); + + it('should parse "0" string as boolean false', () => { + const subscribedValue = '0'; + const lowerValue = subscribedValue.toLowerCase().trim(); + const subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + + expect(subscribed).toBe(false); + }); + + it('should parse "no" string as boolean false', () => { + const subscribedValue = 'no'; + const lowerValue = subscribedValue.toLowerCase().trim(); + const subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + + expect(subscribed).toBe(false); + }); + + it('should handle empty string as undefined', () => { + const subscribedValue = ''; + let subscribed: boolean | undefined; + + if (subscribedValue !== undefined && subscribedValue !== '') { + const lowerValue = subscribedValue.toLowerCase().trim(); + subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + } + + expect(subscribed).toBeUndefined(); + }); + + it('should handle undefined as undefined', () => { + const subscribedValue = undefined; + let subscribed: boolean | undefined; + + if (subscribedValue !== undefined && subscribedValue !== '') { + const lowerValue = subscribedValue.toLowerCase().trim(); + subscribed = lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes'; + } + + expect(subscribed).toBeUndefined(); + }); + }); + + describe('Data preservation', () => { + it('should preserve existing contact data while updating subscription', async () => { + // Create contact with existing data + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'preserve-data@example.com', + data: { + firstName: 'Original', + lastName: 'Name', + plan: 'pro', + }, + }); + + // Update only subscription via import + await ContactService.upsert(projectId, contact.email, {}, true); + + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + // Subscription should be updated + expect(updated?.subscribed).toBe(true); + + // Original data should be preserved + const data = updated?.data as Record; + expect(data?.firstName).toBe('Original'); + expect(data?.lastName).toBe('Name'); + expect(data?.plan).toBe('pro'); + }); + + it('should merge new data while preserving subscription', async () => { + // Create contact with existing data + const contact = await factories.createContact({ + projectId, + subscribed: false, + email: 'merge-data@example.com', + data: { + firstName: 'John', + plan: 'pro', + }, + }); + + // Import new data without changing subscription + await ContactService.upsert( + projectId, + contact.email, + { + lastName: 'Doe', + company: 'Acme Inc', + }, + undefined, + ); + + const updated = await prisma.contact.findUnique({ + where: {id: contact.id}, + }); + + // Subscription should be unchanged + expect(updated?.subscribed).toBe(false); + + // Data should be merged + const data = updated?.data as Record; + expect(data?.firstName).toBe('John'); // Preserved + expect(data?.plan).toBe('pro'); // Preserved + expect(data?.lastName).toBe('Doe'); // New + expect(data?.company).toBe('Acme Inc'); // New + }); + }); +}); diff --git a/apps/api/src/jobs/import-processor.ts b/apps/api/src/jobs/import-processor.ts index ee47c19..15aab66 100644 --- a/apps/api/src/jobs/import-processor.ts +++ b/apps/api/src/jobs/import-processor.ts @@ -128,8 +128,10 @@ export function createImportWorker() { const existingContact = await ContactService.findByEmail(projectId, email); const isUpdate = !!existingContact; - // Upsert contact with subscribed value from CSV if provided, otherwise default to true - await ContactService.upsert(projectId, email, data, subscribed ?? true); + // Upsert contact with subscribed value from CSV if provided + // For new contacts, ContactService.upsert defaults to true + // For existing contacts, only update if explicitly provided in CSV + await ContactService.upsert(projectId, email, data, subscribed); result.successCount++; if (isUpdate) { diff --git a/packages/shared/src/schemas/index.ts b/packages/shared/src/schemas/index.ts index 58f049c..d3b156c 100644 --- a/packages/shared/src/schemas/index.ts +++ b/packages/shared/src/schemas/index.ts @@ -315,7 +315,7 @@ export const ActionSchemas = { track: z.object({ event: z.string().min(1), email, - subscribed: z.boolean().optional().default(true), + subscribed: z.boolean().optional(), data: jsonSchema.optional(), }), send: z @@ -341,7 +341,7 @@ export const ActionSchemas = { subject: z.string().min(1).max(998).optional(), body: z.string().min(1).optional(), template: uuid.optional(), - subscribed: z.boolean().optional().default(false), + subscribed: z.boolean().optional(), name: z.string().optional(), from: z.union([ email, // Simple email string (backward compatible)