-
-
Notifications
You must be signed in to change notification settings - Fork 317
fix: sync suppression list removal with AWS SES (closes #324) #331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { SuppressionReason, SuppressionList } from "@prisma/client"; | |||||||||||||
| import { db } from "../db"; | ||||||||||||||
| import { UnsendApiError } from "~/server/public-api/api-error"; | ||||||||||||||
| import { logger } from "../logger/log"; | ||||||||||||||
| import { deleteFromSesSuppressionList } from "../aws/ses"; | ||||||||||||||
|
|
||||||||||||||
| export type AddSuppressionParams = { | ||||||||||||||
| email: string; | ||||||||||||||
|
|
@@ -120,22 +121,69 @@ export class SuppressionService { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Remove email from suppression list | ||||||||||||||
| * Remove email from suppression list (both local DB and AWS SES) | ||||||||||||||
| */ | ||||||||||||||
| static async removeSuppression(email: string, teamId: number): Promise<void> { | ||||||||||||||
| const normalizedEmail = email.toLowerCase().trim(); | ||||||||||||||
|
|
||||||||||||||
| // Get all unique regions from team's domains for AWS SES cleanup | ||||||||||||||
| try { | ||||||||||||||
| const teamDomains = await db.domain.findMany({ | ||||||||||||||
| where: { teamId }, | ||||||||||||||
| select: { region: true }, | ||||||||||||||
| }); | ||||||||||||||
| const uniqueRegions = [...new Set(teamDomains.map((d) => d.region))]; | ||||||||||||||
|
|
||||||||||||||
| // Attempt to remove from AWS SES in all regions (best effort, don't throw) | ||||||||||||||
| if (uniqueRegions.length > 0) { | ||||||||||||||
| const results = await Promise.allSettled( | ||||||||||||||
| uniqueRegions.map((region) => | ||||||||||||||
| deleteFromSesSuppressionList(normalizedEmail, region) | ||||||||||||||
| ) | ||||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| // Check for failures - deleteFromSesSuppressionList returns false on error | ||||||||||||||
| const failures = results.filter( | ||||||||||||||
| (r) => r.status === "fulfilled" && r.value === false | ||||||||||||||
| ); | ||||||||||||||
|
Comment on lines
146
to
150
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This filter only catches fulfilled promises returning Prompt for AI agents
Suggested change
✅ Addressed in |
||||||||||||||
| if (failures.length > 0) { | ||||||||||||||
| logger.warn( | ||||||||||||||
| { | ||||||||||||||
| email: normalizedEmail, | ||||||||||||||
| teamId, | ||||||||||||||
| failedRegions: failures.length, | ||||||||||||||
| totalRegions: uniqueRegions.length, | ||||||||||||||
| }, | ||||||||||||||
| "Some AWS SES regions failed during suppression removal" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } catch (error) { | ||||||||||||||
| // AWS SES cleanup failure should not block local DB deletion | ||||||||||||||
| logger.error( | ||||||||||||||
| { | ||||||||||||||
| email: normalizedEmail, | ||||||||||||||
| teamId, | ||||||||||||||
| error: error instanceof Error ? error.message : "Unknown error", | ||||||||||||||
| }, | ||||||||||||||
| "Failed to cleanup AWS SES suppression (continuing with local deletion)" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Delete from local database | ||||||||||||||
| try { | ||||||||||||||
| const deleted = await db.suppressionList.delete({ | ||||||||||||||
| where: { | ||||||||||||||
| teamId_email: { | ||||||||||||||
| teamId, | ||||||||||||||
| email: email.toLowerCase().trim(), | ||||||||||||||
| email: normalizedEmail, | ||||||||||||||
| }, | ||||||||||||||
| }, | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| logger.info( | ||||||||||||||
| { | ||||||||||||||
| email, | ||||||||||||||
| email: normalizedEmail, | ||||||||||||||
| teamId, | ||||||||||||||
| suppressionId: deleted.id, | ||||||||||||||
| }, | ||||||||||||||
|
|
@@ -149,7 +197,7 @@ export class SuppressionService { | |||||||||||||
| ) { | ||||||||||||||
| logger.debug( | ||||||||||||||
| { | ||||||||||||||
| email, | ||||||||||||||
| email: normalizedEmail, | ||||||||||||||
| teamId, | ||||||||||||||
| }, | ||||||||||||||
| "Attempted to remove non-existent suppression - already not suppressed" | ||||||||||||||
|
|
@@ -159,7 +207,7 @@ export class SuppressionService { | |||||||||||||
|
|
||||||||||||||
| logger.error( | ||||||||||||||
| { | ||||||||||||||
| email, | ||||||||||||||
| email: normalizedEmail, | ||||||||||||||
| teamId, | ||||||||||||||
| error: error instanceof Error ? error.message : "Unknown error", | ||||||||||||||
| }, | ||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Logging only
error.messageloses the stack trace and other error details. The existing pattern in this file logs the full error object as{ err: error }for better debugging.Prompt for AI agents