From 192bcd5d1033a067d7d14b8db6635857cbf5bc69 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 13 Jan 2025 11:56:57 +0100 Subject: [PATCH 01/11] Only update merge fields in updateContactFields --- .../src/providers/newsletter/MailchimpProvider.ts | 15 +++++++++++++++ .../core/src/providers/newsletter/NoneProvider.ts | 4 ++++ packages/core/src/services/NewsletterService.ts | 8 +------- packages/core/src/type/newsletter-provider.ts | 4 ++++ 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/packages/core/src/providers/newsletter/MailchimpProvider.ts b/packages/core/src/providers/newsletter/MailchimpProvider.ts index b0cad175c..35dc893fd 100644 --- a/packages/core/src/providers/newsletter/MailchimpProvider.ts +++ b/packages/core/src/providers/newsletter/MailchimpProvider.ts @@ -288,6 +288,21 @@ export default class MailchimpProvider implements NewsletterProvider { } } + async updateContactFields( + email: string, + fields: Record + ): Promise { + await this.dispatchOperations([ + { + method: "PATCH", + path: this.emailUrl(email), + params: { skip_merge_validation: "true" }, + body: JSON.stringify({ merge_fields: fields }), + operation_id: `update_fields_${email}` + } + ]); + } + async upsertContacts(nlContacts: UpdateNewsletterContact[]): Promise { const operations: Operation[] = nlContacts.map((contact) => { const mcMember = nlContactToMCMember(contact); diff --git a/packages/core/src/providers/newsletter/NoneProvider.ts b/packages/core/src/providers/newsletter/NoneProvider.ts index 0224103f1..e2f3d8a69 100644 --- a/packages/core/src/providers/newsletter/NoneProvider.ts +++ b/packages/core/src/providers/newsletter/NoneProvider.ts @@ -23,4 +23,8 @@ export default class NoneProvider implements NewsletterProvider { async upsertContacts(contacts: UpdateNewsletterContact[]): Promise {} async archiveContacts(emails: string[]): Promise {} async permanentlyDeleteContacts(emails: string[]): Promise {} + async updateContactFields( + email: string, + fields: Record + ): Promise {} } diff --git a/packages/core/src/services/NewsletterService.ts b/packages/core/src/services/NewsletterService.ts index 05ed94271..8e79bc586 100644 --- a/packages/core/src/services/NewsletterService.ts +++ b/packages/core/src/services/NewsletterService.ts @@ -212,13 +212,7 @@ class NewsletterService { log.info(`Update contact fields for ${contact.id}`, fields); const [, nlUpdate] = await contactToNlUpdate(contact); if (nlUpdate) { - // TODO: should be an update without status - // currently we're sending the status unnecessarily when only updating fields - await this.provider.upsertContact({ - email: nlUpdate.email, - status: nlUpdate.status, - fields - }); + await this.provider.updateContactFields(nlUpdate.email, fields); } else { log.info("Ignoring contact field update for " + contact.id); } diff --git a/packages/core/src/type/newsletter-provider.ts b/packages/core/src/type/newsletter-provider.ts index 8f1f02cdb..3fb25b34a 100644 --- a/packages/core/src/type/newsletter-provider.ts +++ b/packages/core/src/type/newsletter-provider.ts @@ -11,6 +11,10 @@ export interface NewsletterProvider { contact: UpdateNewsletterContact, oldEmail?: string ): Promise; + updateContactFields( + email: string, + fields: Record + ): Promise; upsertContacts(contacts: UpdateNewsletterContact[]): Promise; archiveContacts(emails: string[]): Promise; permanentlyDeleteContacts(emails: string[]): Promise; From 7a856ed8b3db6474ee1a7516f51419815f723d19 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 13 Jan 2025 16:04:31 +0100 Subject: [PATCH 02/11] Add some documentation to NewsletterService --- .../core/src/services/NewsletterService.ts | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/packages/core/src/services/NewsletterService.ts b/packages/core/src/services/NewsletterService.ts index 8e79bc586..47f64283c 100644 --- a/packages/core/src/services/NewsletterService.ts +++ b/packages/core/src/services/NewsletterService.ts @@ -97,6 +97,12 @@ class NewsletterService { ? new MailchimpProvider(config.newsletter.settings) : new NoneProvider(); + /** + * Add the given tag to the list of contacts + * + * @param contacts List of contacts + * @param tag The tag to add + */ async addTagToContacts(contacts: Contact[], tag: string): Promise { log.info(`Add tag ${tag} to ${contacts.length} contacts`); await this.provider.addTagToContacts( @@ -105,6 +111,13 @@ class NewsletterService { ); } + /** + * Remove the given tag from the list of contacts. Any contacts that don't + * have the tag will be ignored. + * + * @param contacts List of contacts + * @param tag The tag to remove + */ async removeTagFromContacts(contacts: Contact[], tag: string): Promise { log.info(`Remove tag ${tag} from ${contacts.length} contacts`); await this.provider.removeTagFromContacts( @@ -200,11 +213,28 @@ class NewsletterService { } } + /** + * Upserts the list of contacts to the newsletter provider. This method is + * used for bulk operations but unlike upsertContact does not give any + * guarantees about the active member tag. + * + * @deprecated Only used by legacy app newsletter sync, do not use. + * @param contacts + */ async upsertContacts(contacts: Contact[]): Promise { log.info(`Upsert ${contacts.length} contacts`); await this.provider.upsertContacts(await getValidNlUpdates(contacts)); } + /** + * Update the merge fields of a contact in the newsletter provider. This + * method merges the field updates with the contact's current fields, so it + * overwrites any existing fields with the new values, but does not remove any + * fields that are not included in the update. + * + * @param contact The contact to update + * @param fields The fields to set + */ async updateContactFields( contact: Contact, fields: Record @@ -218,6 +248,11 @@ class NewsletterService { } } + /** + * Archive a list of contacts in the newsletter provider + * + * @param contacts The contacts to archive + */ async archiveContacts(contacts: Contact[]): Promise { log.info(`Archive ${contacts.length} contacts`); await this.provider.archiveContacts( @@ -237,12 +272,23 @@ class NewsletterService { ); } + /** + * Get a single newsletter contact from the newsletter provider + * + * @param email The contact's email address + * @returns The newsletter contact + */ async getNewsletterContact( email: string ): Promise { return await this.provider.getContact(email); } + /** + * Get all contacts from the newsletter provider + * + * @returns List of newsletter contacts + */ async getNewsletterContacts(): Promise { return await this.provider.getContacts(); } From 836cd567cd5a3067f24a9ed76f215fd0253368bc Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 13 Jan 2025 16:07:47 +0100 Subject: [PATCH 03/11] Refactor Mailchimp API into lib/mailchimp for better separation from the NL provider --- packages/core/src/lib/mailchimp.ts | 253 +++++++++++++ .../providers/newsletter/MailchimpProvider.ts | 347 +++--------------- packages/core/src/type/index.ts | 6 + packages/core/src/type/mc-batch.ts | 8 + packages/core/src/type/mc-member.ts | 12 + .../core/src/type/mc-operation-response.ts | 5 + packages/core/src/type/mc-operation.ts | 17 + packages/core/src/type/mc-status.ts | 1 + .../src/type/update-mc-member-response.ts | 25 ++ 9 files changed, 373 insertions(+), 301 deletions(-) create mode 100644 packages/core/src/lib/mailchimp.ts create mode 100644 packages/core/src/type/mc-batch.ts create mode 100644 packages/core/src/type/mc-member.ts create mode 100644 packages/core/src/type/mc-operation-response.ts create mode 100644 packages/core/src/type/mc-operation.ts create mode 100644 packages/core/src/type/mc-status.ts create mode 100644 packages/core/src/type/update-mc-member-response.ts diff --git a/packages/core/src/lib/mailchimp.ts b/packages/core/src/lib/mailchimp.ts new file mode 100644 index 000000000..40b1d4a7a --- /dev/null +++ b/packages/core/src/lib/mailchimp.ts @@ -0,0 +1,253 @@ +import crypto from "crypto"; + +import { NewsletterStatus } from "@beabee/beabee-common"; +import axios from "axios"; +import JSONStream from "JSONStream"; +import gunzip from "gunzip-maybe"; +import tar from "tar-stream"; + +import { MailchimpNewsletterConfig } from "#config/config"; +import { log as mainLogger } from "#logging"; +import { + MCBatch, + MCMember, + MCOperation, + MCOperationResponse, + MCStatus, + NewsletterContact, + UpdateNewsletterContact +} from "#type/index"; +import { normalizeEmailAddress } from "#utils/index"; +import OptionsService from "#services/OptionsService"; + +const log = mainLogger.child({ app: "mailchimp" }); + +export function createInstance( + settings: MailchimpNewsletterConfig["settings"] +) { + const instance = axios.create({ + baseURL: `https://${settings.datacenter}.api.mailchimp.com/3.0/`, + auth: { + username: "user", + password: settings.apiKey + } + }); + + instance.interceptors.request.use((config) => { + log.info(`${config.method} ${config.url}`, { + params: config.params, + // Don't print all the batch operations + ...((config.url !== "/batches/" || config.method !== "post") && { + data: config.data + }) + }); + + return config; + }); + + instance.interceptors.response.use( + (response) => { + return response; + }, + (error) => { + log.error( + "MailChimp API returned with status " + error.response?.status, + { + status: error.response?.status, + data: error.response?.data + } + ); + return Promise.reject(error); + } + ); + + async function createBatch(operations: MCOperation[]): Promise { + log.info(`Creating batch with ${operations.length} operations`); + const response = await instance.post("/batches/", { operations }); + return response.data as MCBatch; + } + + async function waitForBatch(batch: MCBatch): Promise { + log.info(`Waiting for batch ${batch.id}`, { + finishedOperations: batch.finished_operations, + totalOperations: batch.total_operations, + erroredOperations: batch.errored_operations + }); + + if (batch.status === "finished") { + return batch; + } else { + await new Promise((resolve) => setTimeout(resolve, 5000)); + return await waitForBatch( + (await instance.get("/batches/" + batch.id)).data + ); + } + } + + async function getBatchResponses( + batch: MCBatch, + validateStatus?: (status: number) => boolean + ): Promise { + log.info(`Getting responses for batch ${batch.id}`, { + finishedOperations: batch.finished_operations, + totalOperations: batch.total_operations, + erroredOperations: batch.errored_operations + }); + + const batchResponses: any[] = []; + + const response = await axios({ + method: "GET", + url: batch.response_body_url, + responseType: "stream" + }); + + const extract = tar.extract(); + + extract.on("entry", (header, stream, next) => { + stream.on("end", next); + + if (header.type === "file") { + log.info(`Checking batch error file: ${header.name}`); + stream + .pipe(JSONStream.parse("*")) + .on("data", (data: MCOperationResponse) => { + if (!validateStatus || validateStatus(data.status_code)) { + batchResponses.push(JSON.parse(data.response)); + } else { + log.error( + `Unexpected error for ${data.operation_id}, got ${data.status_code}`, + data + ); + } + }); + } else { + stream.resume(); + } + }); + + return await new Promise((resolve, reject) => { + response.data + .pipe(gunzip()) + .pipe(extract) + .on("error", reject) + .on("finish", () => resolve(batchResponses)); + }); + } + + async function dispatchOperations( + operations: MCOperation[], + validateStatus?: (status: number) => boolean + ): Promise { + log.info(`Dispatching ${operations.length} operations`); + + if (operations.length > 20) { + const batch = await createBatch(operations); + const finishedBatch = await waitForBatch(batch); + await getBatchResponses(finishedBatch, validateStatus); // Just check for errors + } else { + for (const operation of operations) { + try { + await instance({ + method: operation.method, + params: operation.params, + url: operation.path, + ...(operation.body && { data: JSON.parse(operation.body) }), + validateStatus: validateStatus || null + }); + } catch (err) { + log.error( + `Error in operation ${operation.operation_id}`, + err, + operation + ); + } + } + } + } + + return { + instance, + createBatch, + waitForBatch, + getBatchResponses, + dispatchOperations + }; +} + +export function mcStatusToStatus(mcStatus: MCStatus): NewsletterStatus { + switch (mcStatus) { + case "cleaned": + return NewsletterStatus.Cleaned; + case "pending": + return NewsletterStatus.Pending; + case "subscribed": + return NewsletterStatus.Subscribed; + case "unsubscribed": + return NewsletterStatus.Unsubscribed; + } +} + +export function getMCMemberUrl(listId: string, email: string) { + const emailHash = crypto + .createHash("md5") + .update(normalizeEmailAddress(email)) + .digest("hex"); + return `lists/${listId}/members/${emailHash}`; +} + +export function nlContactToMCMember( + nlContact: UpdateNewsletterContact +): Partial { + if (nlContact.status === NewsletterStatus.None) { + throw new Error("NewsletterStatus = None for " + nlContact.email); + } + + const groups: { id: string; label: string }[] = + OptionsService.getJSON("newsletter-groups"); + + return { + email_address: nlContact.email, + status: nlContact.status, + ...((nlContact.firstname || nlContact.lastname || nlContact.fields) && { + merge_fields: { + ...(nlContact.firstname && { FNAME: nlContact.firstname }), + ...(nlContact.lastname && { LNAME: nlContact.lastname }), + ...nlContact.fields + } + }), + ...(nlContact.groups && { + interests: Object.assign( + {}, + ...groups.map((group) => ({ + [group.id]: nlContact.groups?.includes(group.id) + })) + ) + }) + }; +} + +export function mcMemberToNlContact(member: MCMember): NewsletterContact { + const { FNAME, LNAME, ...fields } = member.merge_fields; + const activeMemberTag = OptionsService.getText( + "newsletter-active-member-tag" + ); + return { + email: normalizeEmailAddress(member.email_address), + firstname: FNAME || "", + lastname: LNAME || "", + joined: new Date( + member.timestamp_opt || member.timestamp_signup || member.last_changed + ), + status: mcStatusToStatus(member.status), + groups: member.interests + ? Object.entries(member.interests) + .filter(([group, isOptedIn]) => isOptedIn) + .map(([group]) => group) + : [], + tags: member.tags.map((tag) => tag.name), + fields, + isActiveMember: + member.tags.findIndex((t) => t.name === activeMemberTag) !== -1 + }; +} diff --git a/packages/core/src/providers/newsletter/MailchimpProvider.ts b/packages/core/src/providers/newsletter/MailchimpProvider.ts index 35dc893fd..6c325d658 100644 --- a/packages/core/src/providers/newsletter/MailchimpProvider.ts +++ b/packages/core/src/providers/newsletter/MailchimpProvider.ts @@ -1,237 +1,82 @@ import { NewsletterStatus } from "@beabee/beabee-common"; -import axios from "axios"; -import crypto from "crypto"; -import gunzip from "gunzip-maybe"; -import JSONStream from "JSONStream"; -import tar from "tar-stream"; import { log as mainLogger } from "#logging"; -import OptionsService from "#services/OptionsService"; -import { normalizeEmailAddress } from "#utils/index"; - import { + createInstance, + getMCMemberUrl, + mcMemberToNlContact, + nlContactToMCMember +} from "#lib/mailchimp"; +import { + MCMember, + MCOperation, NewsletterContact, NewsletterProvider, - UpdateNewsletterContact + UpdateNewsletterContact, + UpsertMCMemberResponse } from "#type/index"; import { MailchimpNewsletterConfig } from "#config/config"; +import { CantUpdateMCMember } from "#errors/CantUpdateMCMember"; const log = mainLogger.child({ app: "mailchimp-provider" }); -interface Batch { - id: string; - status: string; - finished_operations: number; - total_operations: number; - errored_operations: number; - response_body_url: string; -} - -interface OperationNoBody { - method: "GET" | "DELETE" | "POST"; - path: string; - params?: string; - operation_id: string; - body?: undefined; -} - -interface OperationWithBody { - method: "POST" | "PATCH" | "PUT"; - path: string; - params?: Record; - body: string; - operation_id: string; -} - -type Operation = OperationNoBody | OperationWithBody; - -interface OperationResponse { - status_code: number; - response: string; - operation_id: string; -} - -type MCStatus = "subscribed" | "unsubscribed" | "pending" | "cleaned"; - -interface MCMember { - email_address: string; - status: MCStatus; - interests?: { [interest: string]: boolean }; - merge_fields: Record; - tags: { id: number; name: string }[]; - timestamp_opt?: string; - timestamp_signup?: string; - last_changed: string; -} - interface GetMembersResponse { members: MCMember[]; } -function createInstance(settings: MailchimpNewsletterConfig["settings"]) { - const instance = axios.create({ - baseURL: `https://${settings.datacenter}.api.mailchimp.com/3.0/`, - auth: { - username: "user", - password: settings.apiKey - } - }); - - instance.interceptors.request.use((config) => { - log.info(`${config.method} ${config.url}`, { - params: config.params, - // Don't print all the batch operations - ...((config.url !== "/batches/" || config.method !== "post") && { - data: config.data - }) - }); - - return config; - }); - - instance.interceptors.response.use( - (response) => { - return response; - }, - (error) => { - log.error( - "MailChimp API returned with status " + error.response?.status, - { - status: error.response?.status, - data: error.response?.data - } - ); - return Promise.reject(error); - } - ); - - return instance; -} - -function mcStatusToStatus(mcStatus: MCStatus): NewsletterStatus { - switch (mcStatus) { - case "cleaned": - return NewsletterStatus.Cleaned; - case "pending": - return NewsletterStatus.Pending; - case "subscribed": - return NewsletterStatus.Subscribed; - case "unsubscribed": - return NewsletterStatus.Unsubscribed; - } -} - -function nlContactToMCMember( - nlContact: UpdateNewsletterContact -): Partial { - if (nlContact.status === NewsletterStatus.None) { - throw new Error("NewsletterStatus = None for " + nlContact.email); - } - - const groups: { id: string; label: string }[] = - OptionsService.getJSON("newsletter-groups"); - - return { - email_address: nlContact.email, - status: nlContact.status, - ...((nlContact.firstname || nlContact.lastname || nlContact.fields) && { - merge_fields: { - ...(nlContact.firstname && { FNAME: nlContact.firstname }), - ...(nlContact.lastname && { LNAME: nlContact.lastname }), - ...nlContact.fields - } - }), - ...(nlContact.groups && { - interests: Object.assign( - {}, - ...groups.map((group) => ({ - [group.id]: nlContact.groups?.includes(group.id) - })) - ) - }) - }; -} - -function mcMemberToNlContact(member: MCMember): NewsletterContact { - const { FNAME, LNAME, ...fields } = member.merge_fields; - return { - email: normalizeEmailAddress(member.email_address), - firstname: FNAME || "", - lastname: LNAME || "", - joined: new Date( - member.timestamp_opt || member.timestamp_signup || member.last_changed - ), - status: mcStatusToStatus(member.status), - groups: member.interests - ? Object.entries(member.interests) - .filter(([group, isOptedIn]) => isOptedIn) - .map(([group]) => group) - : [], - tags: member.tags.map((tag) => tag.name), - fields - }; -} - -// Ignore 404/405s from delete operations -function validateOperationStatus(statusCode: number, operationId: string) { - return ( - statusCode < 400 || - (operationId.startsWith("delete") && - (statusCode === 404 || statusCode === 405)) - ); -} - export default class MailchimpProvider implements NewsletterProvider { - private readonly instance; + private readonly api; private readonly listId; constructor(settings: MailchimpNewsletterConfig["settings"]) { - this.instance = createInstance(settings); + this.api = createInstance(settings); this.listId = settings.listId; } async addTagToContacts(emails: string[], tag: string): Promise { - const operations: Operation[] = emails.map((email) => ({ - path: this.emailUrl(email) + "/tags", + const operations: MCOperation[] = emails.map((email) => ({ + path: getMCMemberUrl(this.listId, email) + "/tags", method: "POST", body: JSON.stringify({ tags: [{ name: tag, status: "active" }] }), operation_id: `add_tag_${email}` })); - await this.dispatchOperations(operations); + await this.api.dispatchOperations(operations); } async removeTagFromContacts(emails: string[], tag: string): Promise { - const operations: Operation[] = emails.map((email) => ({ - path: this.emailUrl(email) + "/tags", + const operations: MCOperation[] = emails.map((email) => ({ + path: getMCMemberUrl(this.listId, email) + "/tags", method: "POST", body: JSON.stringify({ tags: [{ name: tag, status: "inactive" }] }), operation_id: `remove_tag_${email}` })); - await this.dispatchOperations(operations); + await this.api.dispatchOperations(operations); } async getContact(email: string): Promise { try { - const resp = await this.instance.get(this.emailUrl(email)); + const resp = await this.api.instance.get( + getMCMemberUrl(this.listId, email) + ); return mcMemberToNlContact(resp.data); } catch (err) {} } async getContacts(): Promise { - const operation: Operation = { + const operation: MCOperation = { path: `lists/${this.listId}/members`, method: "GET", operation_id: "get" }; - const batch = await this.createBatch([operation]); - const finishedBatch = await this.waitForBatch(batch); - const responses = (await this.getBatchResponses( + const batch = await this.api.createBatch([operation]); + const finishedBatch = await this.api.waitForBatch(batch); + const responses = (await this.api.getBatchResponses( finishedBatch )) as GetMembersResponse[]; @@ -244,7 +89,7 @@ export default class MailchimpProvider implements NewsletterProvider { ): Promise { const req = { method: "PUT", - url: this.emailUrl(oldEmail), + url: getMCMemberUrl(this.listId, oldEmail), params: { skip_merge_validation: true } }; @@ -292,10 +137,10 @@ export default class MailchimpProvider implements NewsletterProvider { email: string, fields: Record ): Promise { - await this.dispatchOperations([ + await this.api.dispatchOperations([ { method: "PATCH", - path: this.emailUrl(email), + path: getMCMemberUrl(this.listId, email), params: { skip_merge_validation: "true" }, body: JSON.stringify({ merge_fields: fields }), operation_id: `update_fields_${email}` @@ -304,10 +149,10 @@ export default class MailchimpProvider implements NewsletterProvider { } async upsertContacts(nlContacts: UpdateNewsletterContact[]): Promise { - const operations: Operation[] = nlContacts.map((contact) => { + const operations: MCOperation[] = nlContacts.map((contact) => { const mcMember = nlContactToMCMember(contact); return { - path: this.emailUrl(contact.email), + path: getMCMemberUrl(this.listId, contact.email), params: { skip_merge_validation: "true" }, method: "PUT", body: JSON.stringify({ ...mcMember, status_if_new: mcMember.status }), @@ -315,132 +160,32 @@ export default class MailchimpProvider implements NewsletterProvider { }; }); - await this.dispatchOperations(operations); + await this.api.dispatchOperations(operations); } async archiveContacts(emails: string[]): Promise { - const operations: Operation[] = emails.map((email) => ({ - path: this.emailUrl(email), + const operations: MCOperation[] = emails.map((email) => ({ + path: getMCMemberUrl(this.listId, email), method: "DELETE", operation_id: `delete_${email}` })); - await this.dispatchOperations(operations); + await this.api.dispatchOperations( + operations, + // Allow 404s and 405s on delete operations + (status) => status < 400 || status === 404 || status === 405 + ); } async permanentlyDeleteContacts(emails: string[]): Promise { - const operations: Operation[] = emails.map((email) => ({ - path: this.emailUrl(email) + "/actions/permanently-delete", + const operations: MCOperation[] = emails.map((email) => ({ + path: getMCMemberUrl(this.listId, email) + "/actions/permanently-delete", method: "POST", operation_id: `delete-permanently_${email}` })); - await this.dispatchOperations(operations); - } - - private emailUrl(email: string) { - const emailHash = crypto - .createHash("md5") - .update(normalizeEmailAddress(email)) - .digest("hex"); - return `lists/${this.listId}/members/${emailHash}`; - } - - private async createBatch(operations: Operation[]): Promise { - log.info(`Creating batch with ${operations.length} operations`); - const response = await this.instance.post("/batches/", { operations }); - return response.data as Batch; - } - - private async waitForBatch(batch: Batch): Promise { - log.info(`Waiting for batch ${batch.id}`, { - finishedOperations: batch.finished_operations, - totalOperations: batch.total_operations, - erroredOperations: batch.errored_operations - }); - - if (batch.status === "finished") { - return batch; - } else { - await new Promise((resolve) => setTimeout(resolve, 5000)); - return await this.waitForBatch( - (await this.instance.get("/batches/" + batch.id)).data - ); - } - } - - private async getBatchResponses(batch: Batch): Promise { - log.info(`Getting responses for batch ${batch.id}`, { - finishedOperations: batch.finished_operations, - totalOperations: batch.total_operations, - erroredOperations: batch.errored_operations - }); - - const batchResponses: any[] = []; - - const response = await axios({ - method: "GET", - url: batch.response_body_url, - responseType: "stream" - }); - - const extract = tar.extract(); - - extract.on("entry", (header, stream, next) => { - stream.on("end", next); - - if (header.type === "file") { - log.info(`Checking batch error file: ${header.name}`); - stream - .pipe(JSONStream.parse("*")) - .on("data", (data: OperationResponse) => { - if (validateOperationStatus(data.status_code, data.operation_id)) { - batchResponses.push(JSON.parse(data.response)); - } else { - log.error( - `Unexpected error for ${data.operation_id}, got ${data.status_code}`, - data - ); - } - }); - } else { - stream.resume(); - } - }); - - return await new Promise((resolve, reject) => { - response.data - .pipe(gunzip()) - .pipe(extract) - .on("error", reject) - .on("finish", () => resolve(batchResponses)); - }); - } - - private async dispatchOperations(operations: Operation[]): Promise { - log.info(`Dispatching ${operations.length} operations`); - - if (operations.length > 20) { - const batch = await this.createBatch(operations); - const finishedBatch = await this.waitForBatch(batch); - await this.getBatchResponses(finishedBatch); // Just check for errors - } else { - for (const operation of operations) { - try { - await this.instance({ - method: operation.method, - params: operation.params, - url: operation.path, - ...(operation.body && { data: JSON.parse(operation.body) }), - validateStatus: (status: number) => - validateOperationStatus(status, operation.operation_id) - }); - } catch (err) { - log.error( - `Error in operation ${operation.operation_id}`, - err, - operation - ); - } - } - } + await this.api.dispatchOperations( + operations, + // Allow 404s and 405s on delete operations + (status) => status < 400 || status === 404 || status === 405 + ); } } diff --git a/packages/core/src/type/index.ts b/packages/core/src/type/index.ts index a4d31f9f4..e9d5b5c43 100644 --- a/packages/core/src/type/index.ts +++ b/packages/core/src/type/index.ts @@ -15,6 +15,11 @@ export * from "./email-mailing-recipient"; export * from "./export-type-id"; export * from "./filter-handlers"; export * from "./login-data"; +export * from "./mc-batch"; +export * from "./mc-member"; +export * from "./mc-operation-response"; +export * from "./mc-operation"; +export * from "./mc-status"; export * from "./network-service-map"; export * from "./network-service"; export * from "./newsletter-contact"; @@ -33,4 +38,5 @@ export * from "./select-result"; export * from "./tag-assignment"; export * from "./taggable-entity"; export * from "./update-contribution-result"; +export * from "./update-mc-member-response"; export * from "./update-newsletter-contact"; diff --git a/packages/core/src/type/mc-batch.ts b/packages/core/src/type/mc-batch.ts new file mode 100644 index 000000000..48dbf8bd8 --- /dev/null +++ b/packages/core/src/type/mc-batch.ts @@ -0,0 +1,8 @@ +export interface MCBatch { + id: string; + status: string; + finished_operations: number; + total_operations: number; + errored_operations: number; + response_body_url: string; +} diff --git a/packages/core/src/type/mc-member.ts b/packages/core/src/type/mc-member.ts new file mode 100644 index 000000000..7365eff1c --- /dev/null +++ b/packages/core/src/type/mc-member.ts @@ -0,0 +1,12 @@ +import { MCStatus } from "./mc-status"; + +export interface MCMember { + email_address: string; + status: MCStatus; + interests?: { [interest: string]: boolean }; + merge_fields: Record; + tags: { id: number; name: string }[]; + timestamp_opt?: string; + timestamp_signup?: string; + last_changed: string; +} diff --git a/packages/core/src/type/mc-operation-response.ts b/packages/core/src/type/mc-operation-response.ts new file mode 100644 index 000000000..f2e290d8f --- /dev/null +++ b/packages/core/src/type/mc-operation-response.ts @@ -0,0 +1,5 @@ +export interface MCOperationResponse { + status_code: number; + response: string; + operation_id: string; +} diff --git a/packages/core/src/type/mc-operation.ts b/packages/core/src/type/mc-operation.ts new file mode 100644 index 000000000..72848c16d --- /dev/null +++ b/packages/core/src/type/mc-operation.ts @@ -0,0 +1,17 @@ +interface MCOperationNoBody { + method: "GET" | "DELETE" | "POST"; + path: string; + params?: string; + operation_id: string; + body?: undefined; +} + +interface MCOperationWithBody { + method: "POST" | "PATCH" | "PUT"; + path: string; + params?: Record; + body: string; + operation_id: string; +} + +export type MCOperation = MCOperationNoBody | MCOperationWithBody; diff --git a/packages/core/src/type/mc-status.ts b/packages/core/src/type/mc-status.ts new file mode 100644 index 000000000..2e23081b4 --- /dev/null +++ b/packages/core/src/type/mc-status.ts @@ -0,0 +1 @@ +export type MCStatus = "subscribed" | "unsubscribed" | "pending" | "cleaned"; diff --git a/packages/core/src/type/update-mc-member-response.ts b/packages/core/src/type/update-mc-member-response.ts new file mode 100644 index 000000000..14b3f5187 --- /dev/null +++ b/packages/core/src/type/update-mc-member-response.ts @@ -0,0 +1,25 @@ +import { AxiosResponse } from "axios"; +import { MCMember } from "./mc-member"; + +interface UpsertMCMemberResponseBadRequest extends AxiosResponse { + status: 400; + data: + | { + title: string; + } + | undefined; +} + +interface UpsertMCMemberResponseSuccess extends AxiosResponse { + status: 200; + data: MCMember; +} + +interface UpsertMCMemberResponseError extends AxiosResponse { + status: 500; // This should really be all statuses except 200 and 400 +} + +export type UpsertMCMemberResponse = + | UpsertMCMemberResponseBadRequest + | UpsertMCMemberResponseSuccess + | UpsertMCMemberResponseError; From be34ea2ae77b776db135bd1b74530ee131e09a63 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 13 Jan 2025 17:10:34 +0100 Subject: [PATCH 04/11] Handle active member tag logic in NewsletterService and providers --- .../core/src/errors/CantUpdateMCMember.ts | 14 ++++ .../providers/newsletter/MailchimpProvider.ts | 64 ++++++++++++++----- packages/core/src/services/ContactsService.ts | 28 ++------ .../core/src/services/NewsletterService.ts | 5 +- .../src/type/update-newsletter-contact.ts | 9 +-- 5 files changed, 77 insertions(+), 43 deletions(-) create mode 100644 packages/core/src/errors/CantUpdateMCMember.ts diff --git a/packages/core/src/errors/CantUpdateMCMember.ts b/packages/core/src/errors/CantUpdateMCMember.ts new file mode 100644 index 000000000..3e369ea16 --- /dev/null +++ b/packages/core/src/errors/CantUpdateMCMember.ts @@ -0,0 +1,14 @@ +import { InternalServerError } from "routing-controllers"; + +export class CantUpdateMCMember extends InternalServerError { + readonly code = "cant-update-mc-member"; + + constructor( + email: string, + readonly status: number, + readonly data: any + ) { + super("Can't update Mailchimp member " + email); + Object.setPrototypeOf(this, CantUpdateMCMember.prototype); + } +} diff --git a/packages/core/src/providers/newsletter/MailchimpProvider.ts b/packages/core/src/providers/newsletter/MailchimpProvider.ts index 6c325d658..606f86d0b 100644 --- a/packages/core/src/providers/newsletter/MailchimpProvider.ts +++ b/packages/core/src/providers/newsletter/MailchimpProvider.ts @@ -7,6 +7,7 @@ import { mcMemberToNlContact, nlContactToMCMember } from "#lib/mailchimp"; +import OptionsService from "#services/OptionsService"; import { MCMember, MCOperation, @@ -83,11 +84,11 @@ export default class MailchimpProvider implements NewsletterProvider { return responses.flatMap((r) => r.members).map(mcMemberToNlContact); } - async upsertContact( + private async upsertMemberOrTryPending( member: UpdateNewsletterContact, oldEmail = member.email - ): Promise { - const req = { + ): Promise { + const baseReq = { method: "PUT", url: getMCMemberUrl(this.listId, oldEmail), params: { skip_merge_validation: true } @@ -95,8 +96,8 @@ export default class MailchimpProvider implements NewsletterProvider { const mcMember = nlContactToMCMember(member); - const resp = await this.instance.request({ - ...req, + const resp = await this.api.instance.request({ + ...baseReq, data: mcMember, // Don't error on 400s, we'll try to recover validateStatus: (status) => status <= 400 @@ -104,7 +105,7 @@ export default class MailchimpProvider implements NewsletterProvider { if (resp.status === 200) { log.info("Updated member " + member.email); - return member.status; + return mcMemberToNlContact(resp.data); // Try to put the user into pending state if they're in a compliance state // This can happen if they previously unsubscribed or were cleaned @@ -116,20 +117,51 @@ export default class MailchimpProvider implements NewsletterProvider { log.info( `Member ${member.email} had status ${resp.data.title}, trying to re-add them` ); - await this.instance.request({ - ...req, + const resp2 = await this.api.instance.request< + any, + UpsertMCMemberResponse + >({ + ...baseReq, data: { ...mcMember, status: "pending" } }); - return NewsletterStatus.Pending; - // Fail gracefully, just remove the member from the newsletter - } else { - log.error("Couldn't update member " + member.email, { - status: resp.status, - data: resp.data - }); + if (resp2.status === 200) { + return mcMemberToNlContact(resp2.data); + } + } + + throw new CantUpdateMCMember(member.email, resp.status, resp.data); + } + + async upsertContact( + member: UpdateNewsletterContact, + oldEmail = member.email + ): Promise { + try { + const updatedMember = await this.upsertMemberOrTryPending( + member, + oldEmail + ); + + if (updatedMember.isActiveMember !== member.isActiveMember) { + log.info("Updating active member tag for " + member.email); + const tagOp = member.isActiveMember + ? "addTagToContacts" + : "removeTagFromContacts"; + await this[tagOp]( + [updatedMember.email], + OptionsService.getText("newsletter-active-member-tag") + ); + } - return NewsletterStatus.None; + return updatedMember.status; + } catch (err) { + if (err instanceof CantUpdateMCMember) { + log.error("Couldn't update member " + member.email, err); + return NewsletterStatus.None; + } else { + throw err; + } } } diff --git a/packages/core/src/services/ContactsService.ts b/packages/core/src/services/ContactsService.ts index de16a9a1a..9b4c1bd82 100644 --- a/packages/core/src/services/ContactsService.ts +++ b/packages/core/src/services/ContactsService.ts @@ -23,7 +23,6 @@ import CalloutsService from "#services/CalloutsService"; import ContactMfaService from "#services/ContactMfaService"; import EmailService from "#services/EmailService"; import NewsletterService from "#services/NewsletterService"; -import OptionsService from "#services/OptionsService"; import PaymentService from "#services/PaymentService"; import ReferralsService from "#services/ReferralsService"; import ResetSecurityFlowService from "#services/ResetSecurityFlowService"; @@ -183,11 +182,7 @@ class ContactsService { Object.assign(contact, updates); if (opts.sync) { - const res = await NewsletterService.upsertContact( - contact, - updates, - oldEmail - ); + await NewsletterService.upsertContact(contact, updates, oldEmail); } await PaymentService.updateContact(contact, updates); @@ -225,16 +220,8 @@ class ContactsService { await getRepository(Contact).save(contact); - if (!wasActive && contact.membership?.isActive) { - await NewsletterService.addTagToContacts( - [contact], - NewsletterService.ACTIVE_MEMBER_TAG - ); - } else if (wasActive && !contact.membership.isActive) { - await NewsletterService.removeTagFromContacts( - [contact], - NewsletterService.ACTIVE_MEMBER_TAG - ); + if (wasActive !== contact.membership?.isActive) { + await NewsletterService.upsertContact(contact); } return role; @@ -276,17 +263,16 @@ class ContactsService { roleType: RoleType ): Promise { log.info(`Revoke role ${roleType} for ${contact.id}`); + const wasActive = contact.membership?.isActive; + contact.roles = contact.roles.filter((p) => p.type !== roleType); const ret = await getRepository(ContactRole).delete({ contactId: contact.id, type: roleType }); - if (!contact.membership?.isActive) { - await NewsletterService.removeTagFromContacts( - [contact], - NewsletterService.ACTIVE_MEMBER_TAG - ); + if (wasActive !== contact.membership?.isActive) { + await NewsletterService.upsertContact(contact); } return ret.affected !== 0; diff --git a/packages/core/src/services/NewsletterService.ts b/packages/core/src/services/NewsletterService.ts index 47f64283c..9e18c6237 100644 --- a/packages/core/src/services/NewsletterService.ts +++ b/packages/core/src/services/NewsletterService.ts @@ -46,7 +46,7 @@ async function contactToNlUpdate( }); } - const nlContact = { + const nlContact: UpdateNewsletterContact = { email: updates?.email || contact.email, status: updates?.newsletterStatus || contact.profile.newsletterStatus, groups: updates?.newsletterGroups || contact.profile.newsletterGroups, @@ -65,7 +65,8 @@ async function contactToNlUpdate( contact.contributionMonthlyAmount?.toFixed(2) || "", C_PERIOD: updates?.contributionPeriod || contact.contributionPeriod || "" - } + }, + isActiveMember: contact.membership?.isActive || false }; return nlContact.status === NewsletterStatus.None diff --git a/packages/core/src/type/update-newsletter-contact.ts b/packages/core/src/type/update-newsletter-contact.ts index 5d1904db5..8eff3fe8f 100644 --- a/packages/core/src/type/update-newsletter-contact.ts +++ b/packages/core/src/type/update-newsletter-contact.ts @@ -3,8 +3,9 @@ import { NewsletterStatus } from "@beabee/beabee-common"; export interface UpdateNewsletterContact { email: string; status: NewsletterStatus; - firstname?: string; - lastname?: string; - groups?: string[]; - fields?: Record; + firstname: string; + lastname: string; + groups: string[]; + fields: Record; + isActiveMember: boolean; } From e29d0f5bd18cc56eb11f8d4346ba718cdb1ba101 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 13 Jan 2025 17:19:16 +0100 Subject: [PATCH 05/11] Improve docs and rename vars for consistency --- .../providers/newsletter/MailchimpProvider.ts | 103 +++++++++++++++--- 1 file changed, 85 insertions(+), 18 deletions(-) diff --git a/packages/core/src/providers/newsletter/MailchimpProvider.ts b/packages/core/src/providers/newsletter/MailchimpProvider.ts index 606f86d0b..117eaee42 100644 --- a/packages/core/src/providers/newsletter/MailchimpProvider.ts +++ b/packages/core/src/providers/newsletter/MailchimpProvider.ts @@ -35,6 +35,12 @@ export default class MailchimpProvider implements NewsletterProvider { this.listId = settings.listId; } + /** + * Add the given tag to the list of email addresses + * + * @param emails List of email addresses + * @param tag The tag to add + */ async addTagToContacts(emails: string[], tag: string): Promise { const operations: MCOperation[] = emails.map((email) => ({ path: getMCMemberUrl(this.listId, email) + "/tags", @@ -47,6 +53,12 @@ export default class MailchimpProvider implements NewsletterProvider { await this.api.dispatchOperations(operations); } + /** + * Remove the given tag from the list of email addresses + * + * @param emails List of email addresses + * @param tag The tag to remove + */ async removeTagFromContacts(emails: string[], tag: string): Promise { const operations: MCOperation[] = emails.map((email) => ({ path: getMCMemberUrl(this.listId, email) + "/tags", @@ -59,6 +71,12 @@ export default class MailchimpProvider implements NewsletterProvider { await this.api.dispatchOperations(operations); } + /** + * Get the newsletter contact with the given email address + * + * @param email The email address of the contact + * @returns The contact or undefined if not found + */ async getContact(email: string): Promise { try { const resp = await this.api.instance.get( @@ -68,6 +86,11 @@ export default class MailchimpProvider implements NewsletterProvider { } catch (err) {} } + /** + * Get all the contacts in the newsletter list + * + * @returns The list of newsletter contacts + */ async getContacts(): Promise { const operation: MCOperation = { path: `lists/${this.listId}/members`, @@ -84,9 +107,18 @@ export default class MailchimpProvider implements NewsletterProvider { return responses.flatMap((r) => r.members).map(mcMemberToNlContact); } - private async upsertMemberOrTryPending( - member: UpdateNewsletterContact, - oldEmail = member.email + /** + * Upsert a newsletter contact to Mailchimp. If the contact has been unsubscribed or cleaned + * then it get be automatically re-subscribed. In this case we attempt to set the status to + * pending which will trigger a double opt-in email. + * + * @param contact The newsletter contact to upsert + * @param oldEmail The old email address of the contact, if it has changed + * @returns The updated newsletter contact + */ + private async upsertContactOrTryPending( + contact: UpdateNewsletterContact, + oldEmail = contact.email ): Promise { const baseReq = { method: "PUT", @@ -94,7 +126,7 @@ export default class MailchimpProvider implements NewsletterProvider { params: { skip_merge_validation: true } }; - const mcMember = nlContactToMCMember(member); + const mcMember = nlContactToMCMember(contact); const resp = await this.api.instance.request({ ...baseReq, @@ -104,18 +136,18 @@ export default class MailchimpProvider implements NewsletterProvider { }); if (resp.status === 200) { - log.info("Updated member " + member.email); + log.info("Updated member " + contact.email); return mcMemberToNlContact(resp.data); // Try to put the user into pending state if they're in a compliance state // This can happen if they previously unsubscribed or were cleaned } else if ( - member.status === NewsletterStatus.Subscribed && + contact.status === NewsletterStatus.Subscribed && resp.status === 400 && resp.data?.title === "Member In Compliance State" ) { log.info( - `Member ${member.email} had status ${resp.data.title}, trying to re-add them` + `Member ${contact.email} had status ${resp.data.title}, trying to re-add them` ); const resp2 = await this.api.instance.request< any, @@ -130,34 +162,44 @@ export default class MailchimpProvider implements NewsletterProvider { } } - throw new CantUpdateMCMember(member.email, resp.status, resp.data); + throw new CantUpdateMCMember(contact.email, resp.status, resp.data); } + /** + * Upsert a contact and synchronise their active member status + * + * @param contact The newsletter contact to upsert + * @param oldEmail The old email address of the contact, if it has changed + * @returns The newsletter contact's new newsletter status + */ async upsertContact( - member: UpdateNewsletterContact, - oldEmail = member.email + contact: UpdateNewsletterContact, + oldEmail = contact.email ): Promise { try { - const updatedMember = await this.upsertMemberOrTryPending( - member, + const updatedContact = await this.upsertContactOrTryPending( + contact, oldEmail ); - if (updatedMember.isActiveMember !== member.isActiveMember) { - log.info("Updating active member tag for " + member.email); - const tagOp = member.isActiveMember + // Add/remove the active member tag if the statuses don't match + if (updatedContact.isActiveMember !== contact.isActiveMember) { + log.info("Updating active member tag for " + contact.email); + const tagOp = contact.isActiveMember ? "addTagToContacts" : "removeTagFromContacts"; await this[tagOp]( - [updatedMember.email], + [updatedContact.email], OptionsService.getText("newsletter-active-member-tag") ); } - return updatedMember.status; + // TODO: Update newsletter status? + + return updatedContact.status; } catch (err) { if (err instanceof CantUpdateMCMember) { - log.error("Couldn't update member " + member.email, err); + log.error("Couldn't update newsletter contact " + contact.email, err); return NewsletterStatus.None; } else { throw err; @@ -165,6 +207,13 @@ export default class MailchimpProvider implements NewsletterProvider { } } + /** + * Update a contact with the given fields. This will overwrite any existing fields + * but will not remove any fields that are not provided. + * + * @param email The email address of the contact + * @param fields The fields to update + */ async updateContactFields( email: string, fields: Record @@ -180,6 +229,13 @@ export default class MailchimpProvider implements NewsletterProvider { ]); } + /** + * Upserts the list of contacts to the newsletter provider. This method does + * not give any guarantees about the active member tag. + * + * @deprecated Only used by legacy app newsletter sync, do not use. + * @param nlContacts + */ async upsertContacts(nlContacts: UpdateNewsletterContact[]): Promise { const operations: MCOperation[] = nlContacts.map((contact) => { const mcMember = nlContactToMCMember(contact); @@ -195,6 +251,11 @@ export default class MailchimpProvider implements NewsletterProvider { await this.api.dispatchOperations(operations); } + /** + * Archive the list of contact emails, ignoring any not found errors + * + * @param emails The list of email addresses to archive + */ async archiveContacts(emails: string[]): Promise { const operations: MCOperation[] = emails.map((email) => ({ path: getMCMemberUrl(this.listId, email), @@ -208,6 +269,12 @@ export default class MailchimpProvider implements NewsletterProvider { ); } + /** + * Permanently delete the contacts with the given email addresses, ignoring + * any not found errors + * + * @param emails The list of email addresses to delete + */ async permanentlyDeleteContacts(emails: string[]): Promise { const operations: MCOperation[] = emails.map((email) => ({ path: getMCMemberUrl(this.listId, email) + "/actions/permanently-delete", From 49066eaa0200e68d908be4c881c62d22f9f99346 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Tue, 28 Jan 2025 13:38:53 +0100 Subject: [PATCH 06/11] Handle newsletter status and group updates inside NewsletterService --- .../core/src/errors/CantUpdateMCMember.ts | 14 --- .../src/errors/CantUpdateNewsletterContact.ts | 14 +++ .../providers/newsletter/MailchimpProvider.ts | 53 ++++----- .../src/providers/newsletter/NoneProvider.ts | 5 +- packages/core/src/services/ContactsService.ts | 17 ++- .../core/src/services/NewsletterService.ts | 102 ++++++------------ packages/core/src/type/newsletter-provider.ts | 3 +- 7 files changed, 77 insertions(+), 131 deletions(-) delete mode 100644 packages/core/src/errors/CantUpdateMCMember.ts create mode 100644 packages/core/src/errors/CantUpdateNewsletterContact.ts diff --git a/packages/core/src/errors/CantUpdateMCMember.ts b/packages/core/src/errors/CantUpdateMCMember.ts deleted file mode 100644 index 3e369ea16..000000000 --- a/packages/core/src/errors/CantUpdateMCMember.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { InternalServerError } from "routing-controllers"; - -export class CantUpdateMCMember extends InternalServerError { - readonly code = "cant-update-mc-member"; - - constructor( - email: string, - readonly status: number, - readonly data: any - ) { - super("Can't update Mailchimp member " + email); - Object.setPrototypeOf(this, CantUpdateMCMember.prototype); - } -} diff --git a/packages/core/src/errors/CantUpdateNewsletterContact.ts b/packages/core/src/errors/CantUpdateNewsletterContact.ts new file mode 100644 index 000000000..1aa239c23 --- /dev/null +++ b/packages/core/src/errors/CantUpdateNewsletterContact.ts @@ -0,0 +1,14 @@ +import { InternalServerError } from "routing-controllers"; + +export class CantUpdateNewsletterContact extends InternalServerError { + readonly code = "cant-update-newsletter-contact"; + + constructor( + email: string, + readonly status: number, + readonly data: any + ) { + super("Can't update newsletter contact " + email); + Object.setPrototypeOf(this, CantUpdateNewsletterContact.prototype); + } +} diff --git a/packages/core/src/providers/newsletter/MailchimpProvider.ts b/packages/core/src/providers/newsletter/MailchimpProvider.ts index 117eaee42..5e02202ce 100644 --- a/packages/core/src/providers/newsletter/MailchimpProvider.ts +++ b/packages/core/src/providers/newsletter/MailchimpProvider.ts @@ -18,7 +18,7 @@ import { } from "#type/index"; import { MailchimpNewsletterConfig } from "#config/config"; -import { CantUpdateMCMember } from "#errors/CantUpdateMCMember"; +import { CantUpdateNewsletterContact } from "#errors/CantUpdateNewsletterContact"; const log = mainLogger.child({ app: "mailchimp-provider" }); @@ -162,7 +162,11 @@ export default class MailchimpProvider implements NewsletterProvider { } } - throw new CantUpdateMCMember(contact.email, resp.status, resp.data); + throw new CantUpdateNewsletterContact( + contact.email, + resp.status, + resp.data + ); } /** @@ -175,36 +179,25 @@ export default class MailchimpProvider implements NewsletterProvider { async upsertContact( contact: UpdateNewsletterContact, oldEmail = contact.email - ): Promise { - try { - const updatedContact = await this.upsertContactOrTryPending( - contact, - oldEmail - ); - - // Add/remove the active member tag if the statuses don't match - if (updatedContact.isActiveMember !== contact.isActiveMember) { - log.info("Updating active member tag for " + contact.email); - const tagOp = contact.isActiveMember - ? "addTagToContacts" - : "removeTagFromContacts"; - await this[tagOp]( - [updatedContact.email], - OptionsService.getText("newsletter-active-member-tag") - ); - } - - // TODO: Update newsletter status? + ): Promise { + const updatedContact = await this.upsertContactOrTryPending( + contact, + oldEmail + ); - return updatedContact.status; - } catch (err) { - if (err instanceof CantUpdateMCMember) { - log.error("Couldn't update newsletter contact " + contact.email, err); - return NewsletterStatus.None; - } else { - throw err; - } + // Add/remove the active member tag if the statuses don't match + if (updatedContact.isActiveMember !== contact.isActiveMember) { + log.info("Updating active member tag for " + contact.email); + const tagOp = contact.isActiveMember + ? "addTagToContacts" + : "removeTagFromContacts"; + await this[tagOp]( + [updatedContact.email], + OptionsService.getText("newsletter-active-member-tag") + ); } + + return updatedContact; } /** diff --git a/packages/core/src/providers/newsletter/NoneProvider.ts b/packages/core/src/providers/newsletter/NoneProvider.ts index e2f3d8a69..4bcef387a 100644 --- a/packages/core/src/providers/newsletter/NoneProvider.ts +++ b/packages/core/src/providers/newsletter/NoneProvider.ts @@ -3,7 +3,6 @@ import { NewsletterProvider, UpdateNewsletterContact } from "#type/index"; -import { NewsletterStatus } from "@beabee/beabee-common"; export default class NoneProvider implements NewsletterProvider { async addTagToContacts(emails: string[], tag: string): Promise {} @@ -17,8 +16,8 @@ export default class NoneProvider implements NewsletterProvider { async upsertContact( contact: UpdateNewsletterContact, oldEmail?: string - ): Promise { - return contact.status; + ): Promise { + return { ...contact, joined: new Date(), tags: [] }; } async upsertContacts(contacts: UpdateNewsletterContact[]): Promise {} async archiveContacts(emails: string[]): Promise {} diff --git a/packages/core/src/services/ContactsService.ts b/packages/core/src/services/ContactsService.ts index 6bd02bdc6..70fb9da5a 100644 --- a/packages/core/src/services/ContactsService.ts +++ b/packages/core/src/services/ContactsService.ts @@ -285,16 +285,13 @@ class ContactsService { ): Promise { log.info("Update contact profile for " + contact.id, { updates }); - if (opts.sync) { + const { newsletterStatus, newsletterGroups, ...profileUpdates } = updates; + if (opts.sync && (newsletterStatus || newsletterGroups)) { try { - const res = await NewsletterService.upsertContact(contact, { - newsletterStatus: updates.newsletterStatus, - newsletterGroups: updates.newsletterGroups + await NewsletterService.upsertContact(contact, { + newsletterStatus, + newsletterGroups }); - - if (res) { - updates.newsletterStatus = res.newStatus; - } } catch (err) { log.error( "Error updating contact profile on newsletter provider for " + @@ -304,9 +301,9 @@ class ContactsService { } } - await getRepository(ContactProfile).update(contact.id, updates); + await getRepository(ContactProfile).update(contact.id, profileUpdates); if (contact.profile) { - Object.assign(contact.profile, updates); + Object.assign(contact.profile, profileUpdates); } } diff --git a/packages/core/src/services/NewsletterService.ts b/packages/core/src/services/NewsletterService.ts index 382807e03..9b481fe0a 100644 --- a/packages/core/src/services/NewsletterService.ts +++ b/packages/core/src/services/NewsletterService.ts @@ -16,8 +16,7 @@ import { Contact, ContactProfile } from "#models/index"; import config from "#config/config"; import { getContributionDescription } from "#utils/contact"; - -import OptionsService from "#services/OptionsService"; +import { CantUpdateNewsletterContact } from "#errors/CantUpdateNewsletterContact"; const log = mainLogger.child({ app: "newsletter-service" }); @@ -88,11 +87,6 @@ async function getValidNlUpdates( } class NewsletterService { - /** Tag used to identify active members in the newsletter system */ - get ACTIVE_MEMBER_TAG(): string { - return OptionsService.getText("newsletter-active-member-tag"); - } - private readonly provider: NewsletterProvider = config.newsletter.provider === "mailchimp" ? new MailchimpProvider(config.newsletter.settings) @@ -127,74 +121,18 @@ class NewsletterService { ); } - /** - * Handles status changes in newsletter subscriptions and manages member tags - * - * @param contact - The contact whose newsletter status is changing - * @param oldStatus - Previous newsletter status of the contact - * @param newStatus - New newsletter status being set for the contact - * - * @remarks - * This method: - * 1. Updates the newsletter status in the database if changed - * 2. Adds the active member tag if this is the first newsletter signup - */ - private async handleNewsletterStatusChange( - contact: Contact, - oldStatus: NewsletterStatus, - newStatus: NewsletterStatus - ): Promise { - // Update newsletter status in database if changed - if (newStatus !== oldStatus) { - if (!contact.profile) { - contact.profile = await getRepository(ContactProfile).findOneByOrFail({ - contactId: contact.id - }); - } - contact.profile.newsletterStatus = newStatus; - await getRepository(ContactProfile).save(contact.profile); - } - - // Only add tag if moving from no newsletter to having newsletter - if ( - oldStatus === NewsletterStatus.None && - newStatus !== NewsletterStatus.None && - contact.membership?.isActive - ) { - log.info("First newsletter signup for " + contact.id); - await this.addTagToContacts([contact], this.ACTIVE_MEMBER_TAG); - } - } - /** * Updates or inserts a contact in the newsletter provider and handles status changes * - * @param contact - The contact to update or insert - * @param updates - Optional updates to apply to the contact before syncing - * @param oldEmail - Previous email address if the email is being updated - * - * @returns Object containing old and new newsletter status if contact was updated, - * undefined if no update was needed - * - * @remarks - * This method will: - * 1. Check if updates are needed based on the provided changes - * 2. Convert the contact to newsletter format - * 3. Sync with the newsletter provider - * 4. Handle any status changes (e.g., adding active member tag) - * 5. Update the contact's newsletter status in the database if changed + * @param contact The contact to update or insert + * @param updates Optional updates to apply to the contact before syncing + * @param oldEmail Previous email address if the email is being updated */ async upsertContact( contact: Contact, updates?: ContactNewsletterUpdates, oldEmail?: string - ): Promise< - | { - oldStatus: NewsletterStatus; - newStatus: NewsletterStatus; - } - | undefined - > { + ): Promise { const willUpdate = !updates || shouldUpdate(updates); if (!willUpdate) { return; @@ -202,13 +140,33 @@ class NewsletterService { const [oldStatus, nlUpdate] = await contactToNlUpdate(contact, updates); if (nlUpdate) { - log.info("Upsert contact " + contact.id); - const newStatus = await this.provider.upsertContact(nlUpdate, oldEmail); + try { + log.info("Upsert contact " + contact.id); + const nlContact = await this.provider.upsertContact(nlUpdate, oldEmail); - // Handle newsletter status changes - await this.handleNewsletterStatusChange(contact, oldStatus, newStatus); + log.info( + `Updating newsletter status from ${oldStatus} to ${nlContact.status} for contact ${contact.id}` + ); - return { oldStatus, newStatus }; + // TODO: remove dependency on ContactProfile + await getRepository(ContactProfile).update(contact.id, { + newsletterStatus: nlContact.status, + newsletterGroups: nlContact.groups + }); + contact.profile.newsletterStatus = nlContact.status; + contact.profile.newsletterGroups = nlContact.groups; + } catch (err) { + // The newsletter provider rejected the update, set this contact's + // newsletter status to None to prevent further updates + if (err instanceof CantUpdateNewsletterContact) { + await getRepository(ContactProfile).update(contact.id, { + newsletterStatus: NewsletterStatus.None + }); + contact.profile.newsletterStatus = NewsletterStatus.None; + } + + throw err; + } } else { log.info("Ignoring contact update for " + contact.id); } diff --git a/packages/core/src/type/newsletter-provider.ts b/packages/core/src/type/newsletter-provider.ts index 3fb25b34a..7d31cccce 100644 --- a/packages/core/src/type/newsletter-provider.ts +++ b/packages/core/src/type/newsletter-provider.ts @@ -1,4 +1,3 @@ -import { NewsletterStatus } from "@beabee/beabee-common"; import { NewsletterContact } from "./newsletter-contact"; import { UpdateNewsletterContact } from "./update-newsletter-contact"; @@ -10,7 +9,7 @@ export interface NewsletterProvider { upsertContact( contact: UpdateNewsletterContact, oldEmail?: string - ): Promise; + ): Promise; updateContactFields( email: string, fields: Record From e654d7351357306dff57fdfb59f8f36590360824 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Tue, 28 Jan 2025 13:39:16 +0100 Subject: [PATCH 07/11] A bit more documentation on functions --- .../core/src/services/NewsletterService.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/core/src/services/NewsletterService.ts b/packages/core/src/services/NewsletterService.ts index 9b481fe0a..fea6bbb6a 100644 --- a/packages/core/src/services/NewsletterService.ts +++ b/packages/core/src/services/NewsletterService.ts @@ -20,6 +20,13 @@ import { CantUpdateNewsletterContact } from "#errors/CantUpdateNewsletterContact const log = mainLogger.child({ app: "newsletter-service" }); +/** + * A guard to check if the given updates object contains any changes that should + * be synced to the newsletter provider + * + * @param updates The updates to check + * @returns Whether the updates contain any changes that should be synced + */ function shouldUpdate(updates: ContactNewsletterUpdates): boolean { return !!( updates.email || @@ -34,6 +41,15 @@ function shouldUpdate(updates: ContactNewsletterUpdates): boolean { ); } +/** + * Convert a contact and optional updates to a newsletter update object that can + * be sent to the newsletter provider + * + * @param contact The contact + * @param updates The updates to the contact + * @returns A tuple with the contact's current newsletter status and the update + * object + */ async function contactToNlUpdate( contact: Contact, updates?: ContactNewsletterUpdates @@ -73,6 +89,13 @@ async function contactToNlUpdate( : [contact.profile.newsletterStatus, nlContact]; } +/** + * Convert a list of contacts to a list of newsletter updates that can be sent + * to the newsletter provider, ignoring any contacts that shouldn't be synced + * + * @param contacts The list of contacts + * @returns A list of valid newsletter updates + */ async function getValidNlUpdates( contacts: Contact[] ): Promise { From 8730058e7332f088315e485d3bb9009e1afa4153 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Tue, 28 Jan 2025 13:40:02 +0100 Subject: [PATCH 08/11] Remove inherited properties from NewsletterContact --- packages/core/src/type/newsletter-contact.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/core/src/type/newsletter-contact.ts b/packages/core/src/type/newsletter-contact.ts index 49b249447..2e7a9fce3 100644 --- a/packages/core/src/type/newsletter-contact.ts +++ b/packages/core/src/type/newsletter-contact.ts @@ -1,12 +1,6 @@ -import { NewsletterStatus } from "@beabee/beabee-common"; import { UpdateNewsletterContact } from "./update-newsletter-contact"; export interface NewsletterContact extends UpdateNewsletterContact { - firstname: string; - lastname: string; joined: Date; - status: NewsletterStatus; - groups: string[]; tags: string[]; - fields: Record; } From 475ba69b6bc9086fc815b7b5017499124556023f Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 3 Feb 2025 14:09:36 +0100 Subject: [PATCH 09/11] Clearer logging and code --- .../providers/newsletter/MailchimpProvider.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/core/src/providers/newsletter/MailchimpProvider.ts b/packages/core/src/providers/newsletter/MailchimpProvider.ts index 5e02202ce..5d15af8b7 100644 --- a/packages/core/src/providers/newsletter/MailchimpProvider.ts +++ b/packages/core/src/providers/newsletter/MailchimpProvider.ts @@ -187,14 +187,14 @@ export default class MailchimpProvider implements NewsletterProvider { // Add/remove the active member tag if the statuses don't match if (updatedContact.isActiveMember !== contact.isActiveMember) { - log.info("Updating active member tag for " + contact.email); - const tagOp = contact.isActiveMember - ? "addTagToContacts" - : "removeTagFromContacts"; - await this[tagOp]( - [updatedContact.email], - OptionsService.getText("newsletter-active-member-tag") - ); + const tag = OptionsService.getText("newsletter-active-member-tag"); + if (contact.isActiveMember) { + log.info(`Adding active member tag for ${contact.email}`); + await this.addTagToContacts([updatedContact.email], tag); + } else { + log.info(`Removing active member tag for ${contact.email}`); + await this.removeTagFromContacts([updatedContact.email], tag); + } } return updatedContact; From 0549ce2c2f18ed5cd9beb9c8bb803e298a3b723c Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 3 Feb 2025 14:12:33 +0100 Subject: [PATCH 10/11] Don't rethrow CantUpdateNewsletterContact errors --- packages/core/src/services/NewsletterService.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/core/src/services/NewsletterService.ts b/packages/core/src/services/NewsletterService.ts index fea6bbb6a..88e507a36 100644 --- a/packages/core/src/services/NewsletterService.ts +++ b/packages/core/src/services/NewsletterService.ts @@ -182,13 +182,17 @@ class NewsletterService { // The newsletter provider rejected the update, set this contact's // newsletter status to None to prevent further updates if (err instanceof CantUpdateNewsletterContact) { + log.error( + `Newsletter upsert failed, setting status to none for contact ${contact.id}`, + err + ); await getRepository(ContactProfile).update(contact.id, { newsletterStatus: NewsletterStatus.None }); contact.profile.newsletterStatus = NewsletterStatus.None; + } else { + throw err; } - - throw err; } } else { log.info("Ignoring contact update for " + contact.id); From 05bf95b389abf2ebd315bcd0e961a7f0d71f3c99 Mon Sep 17 00:00:00 2001 From: Will Franklin Date: Mon, 3 Feb 2025 14:30:32 +0100 Subject: [PATCH 11/11] Fix trying to update empty profile, better logging --- packages/core/src/services/ContactsService.ts | 28 ++++++++----------- .../core/src/services/NewsletterService.ts | 3 +- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/packages/core/src/services/ContactsService.ts b/packages/core/src/services/ContactsService.ts index 70fb9da5a..8256a92e8 100644 --- a/packages/core/src/services/ContactsService.ts +++ b/packages/core/src/services/ContactsService.ts @@ -283,27 +283,21 @@ class ContactsService { updates: Partial, opts = { sync: true } ): Promise { - log.info("Update contact profile for " + contact.id, { updates }); - const { newsletterStatus, newsletterGroups, ...profileUpdates } = updates; if (opts.sync && (newsletterStatus || newsletterGroups)) { - try { - await NewsletterService.upsertContact(contact, { - newsletterStatus, - newsletterGroups - }); - } catch (err) { - log.error( - "Error updating contact profile on newsletter provider for " + - contact.id, - err - ); - } + await NewsletterService.upsertContact(contact, { + newsletterStatus, + newsletterGroups + }); } - await getRepository(ContactProfile).update(contact.id, profileUpdates); - if (contact.profile) { - Object.assign(contact.profile, profileUpdates); + if (Object.keys(profileUpdates).length > 0) { + log.info("Update contact profile for " + contact.id, { profileUpdates }); + + await getRepository(ContactProfile).update(contact.id, profileUpdates); + if (contact.profile) { + Object.assign(contact.profile, profileUpdates); + } } } diff --git a/packages/core/src/services/NewsletterService.ts b/packages/core/src/services/NewsletterService.ts index 88e507a36..d829c4a56 100644 --- a/packages/core/src/services/NewsletterService.ts +++ b/packages/core/src/services/NewsletterService.ts @@ -168,7 +168,8 @@ class NewsletterService { const nlContact = await this.provider.upsertContact(nlUpdate, oldEmail); log.info( - `Updating newsletter status from ${oldStatus} to ${nlContact.status} for contact ${contact.id}` + `Got newsletter groups and status ${oldStatus} to ${nlContact.status} for contact ${contact.id}`, + { groups: nlContact.groups } ); // TODO: remove dependency on ContactProfile