From 84788e7551f5079450a796343bd394a4d6e5af5c Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 21 Jun 2023 11:33:49 +0800 Subject: [PATCH 1/3] fix: mark failed to unsubscribe newsletters as unsubscribed state --- .../api/src/resolvers/subscriptions/index.ts | 4 +- packages/api/src/services/subscriptions.ts | 68 +++++++++++++------ 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/packages/api/src/resolvers/subscriptions/index.ts b/packages/api/src/resolvers/subscriptions/index.ts index 6293cf5e0..a7aa15392 100644 --- a/packages/api/src/resolvers/subscriptions/index.ts +++ b/packages/api/src/resolvers/subscriptions/index.ts @@ -111,9 +111,7 @@ export const unsubscribeResolver = authorized< } if (!subscription.unsubscribeMailTo && !subscription.unsubscribeHttpUrl) { - return { - errorCodes: [UnsubscribeErrorCode.UnsubscribeMethodNotFound], - } + log.info('No unsubscribe method found') } await unsubscribe(subscription) diff --git a/packages/api/src/services/subscriptions.ts b/packages/api/src/services/subscriptions.ts index d108db610..794dd8404 100644 --- a/packages/api/src/services/subscriptions.ts +++ b/packages/api/src/services/subscriptions.ts @@ -37,27 +37,44 @@ export const parseUnsubscribeMailTo = (unsubscribeMailTo: string) => { const sendUnsubscribeEmail = async ( unsubscribeMailTo: string, newsletterEmail: string -): Promise => { - // get subject from unsubscribe email address if exists - const parsed = parseUnsubscribeMailTo(unsubscribeMailTo) +): Promise => { + try { + // get subject from unsubscribe email address if exists + const parsed = parseUnsubscribeMailTo(unsubscribeMailTo) - const sent = await sendEmail({ - to: parsed.to, - subject: parsed.subject, - text: UNSUBSCRIBE_EMAIL_TEXT, - from: newsletterEmail, - }) + const sent = await sendEmail({ + to: parsed.to, + subject: parsed.subject, + text: UNSUBSCRIBE_EMAIL_TEXT, + from: newsletterEmail, + }) - if (!sent) { - throw new Error(`Failed to unsubscribe, email: ${unsubscribeMailTo}`) + if (!sent) { + console.log('Failed to send unsubscribe email', unsubscribeMailTo) + return false + } + + return true + } catch (error) { + console.log('Failed to send unsubscribe email', error) + return false } } -const sendUnsubscribeHttpRequest = async (url: string): Promise => { - const response = await axios.get(url) +const sendUnsubscribeHttpRequest = async (url: string): Promise => { + try { + await axios.get(url, { + timeout: 5000, // 5 seconds + }) - if (response.status !== 200) { - throw new Error(`Failed to unsubscribe, response: ${response.statusText}`) + return true + } catch (error) { + if (axios.isAxiosError(error)) { + console.log('Failed to send unsubscribe http request', error.message) + } else { + console.log('Failed to send unsubscribe http request', error) + } + return false } } @@ -85,20 +102,31 @@ export const saveSubscription = async ({ } export const unsubscribe = async (subscription: Subscription) => { + let unsubscribed = false if (subscription.unsubscribeMailTo) { // unsubscribe by sending email first - await sendUnsubscribeEmail( + unsubscribed = await sendUnsubscribeEmail( subscription.unsubscribeMailTo, subscription.newsletterEmail.address ) } else if (subscription.unsubscribeHttpUrl) { // unsubscribe by sending http request if no unsubscribeMailTo - await sendUnsubscribeHttpRequest(subscription.unsubscribeHttpUrl) + unsubscribed = await sendUnsubscribeHttpRequest( + subscription.unsubscribeHttpUrl + ) } else { - throw new Error('No unsubscribe method defined') + console.log('No unsubscribe method defined') } - // delete the subscription + if (!unsubscribed) { + // update subscription status to unsubscribed if failed to unsubscribe + console.log('Failed to unsubscribe', subscription.id) + return getRepository(Subscription).update(subscription.id, { + status: SubscriptionStatus.Unsubscribed, + }) + } + + // delete the subscription if successfully unsubscribed await getRepository(Subscription).delete(subscription.id) } @@ -114,7 +142,7 @@ export const unsubscribeAll = async ( relations: ['newsletterEmail'], }) - for (const subscription of subscriptions) { + for await (const subscription of subscriptions) { try { await unsubscribe(subscription) } catch (error) { From 4f74b32ff6d867fd774ab3361f4389c463598a4c Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 21 Jun 2023 12:02:15 +0800 Subject: [PATCH 2/3] do not subscribe a newsletter if subscription already exists and is unsubscribed --- .../api/src/resolvers/recent_emails/index.ts | 8 +++++ packages/api/src/routers/svc/newsletters.ts | 12 +++++++ .../api/src/services/save_newsletter_email.ts | 31 +++++++++---------- packages/api/src/services/subscriptions.ts | 10 ++++++ 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/packages/api/src/resolvers/recent_emails/index.ts b/packages/api/src/resolvers/recent_emails/index.ts index b3c28e8aa..ef4955ec0 100644 --- a/packages/api/src/resolvers/recent_emails/index.ts +++ b/packages/api/src/resolvers/recent_emails/index.ts @@ -115,6 +115,14 @@ export const markEmailAsItemResolver = authorized< }, newsletterEmail ) + if (!success) { + log.info('newsletter not created', recentEmail.id) + + return { + errorCodes: [MarkEmailAsItemErrorCode.BadRequest], + } + } + // update received email type await updateReceivedEmail(recentEmail.id, 'article') diff --git a/packages/api/src/routers/svc/newsletters.ts b/packages/api/src/routers/svc/newsletters.ts index 861ba3d69..537293690 100644 --- a/packages/api/src/routers/svc/newsletters.ts +++ b/packages/api/src/routers/svc/newsletters.ts @@ -3,6 +3,7 @@ import { createPubSubClient, readPushSubscription, } from '../../datalayer/pubsub' +import { SubscriptionStatus } from '../../generated/graphql' import { getNewsletterEmail, updateConfirmationCode, @@ -13,6 +14,7 @@ import { saveNewsletterEmail, } from '../../services/save_newsletter_email' import { saveUrlFromEmail } from '../../services/save_url' +import { getSubscriptionByNameAndUserId } from '../../services/subscriptions' import { isUrl } from '../../utils/helpers' interface SetConfirmationCodeMessage { @@ -128,6 +130,16 @@ export function newsletterServiceRouter() { return res.status(500).send('Error saving url from email') } } else { + // do not subscribe if subscription already exists and is unsubscribed + const existingSubscription = await getSubscriptionByNameAndUserId( + data.author, + newsletterEmail.user.id + ) + if (existingSubscription?.status === SubscriptionStatus.Unsubscribed) { + console.log('newsletter already unsubscribed:', data.author) + return res.status(200).send('newsletter already unsubscribed') + } + // save newsletter instead const result = await saveNewsletterEmail(data, newsletterEmail, saveCtx) if (!result) { diff --git a/packages/api/src/services/save_newsletter_email.ts b/packages/api/src/services/save_newsletter_email.ts index 2befa5cde..b8e6ee8bf 100644 --- a/packages/api/src/services/save_newsletter_email.ts +++ b/packages/api/src/services/save_newsletter_email.ts @@ -67,27 +67,24 @@ export const saveNewsletterEmail = async ( return false } - if (!page.siteIcon || isBase64Image(page.siteIcon)) { + let icon = page.siteIcon + if (!icon || isBase64Image(icon)) { // fetch favicon if not already set or is a base64 image - const favicon = await fetchFavicon(page.url) - if (favicon) { - page.siteIcon = favicon - await updatePage(page.id, { siteIcon: favicon }, saveCtx) + icon = await fetchFavicon(page.url) + if (icon) { + await updatePage(page.id, { siteIcon: icon }, saveCtx) } } - // creates or updates subscription only if their is a valid unsubscribe link - if (data.unsubMailTo || data.unsubHttpUrl) { - const subscriptionId = await saveSubscription({ - userId: newsletterEmail.user.id, - name: data.author, - newsletterEmail, - unsubscribeMailTo: data.unsubMailTo, - unsubscribeHttpUrl: data.unsubHttpUrl, - icon: page.siteIcon, - }) - console.log('subscription saved', subscriptionId) - } + const subscriptionId = await saveSubscription({ + userId: newsletterEmail.user.id, + name: data.author, + newsletterEmail, + unsubscribeMailTo: data.unsubMailTo, + unsubscribeHttpUrl: data.unsubHttpUrl, + icon, + }) + console.log('subscription saved', subscriptionId) // adds newsletters label to page const result = await addLabelToPage(saveCtx, page.id, { diff --git a/packages/api/src/services/subscriptions.ts b/packages/api/src/services/subscriptions.ts index 794dd8404..d00d19f6c 100644 --- a/packages/api/src/services/subscriptions.ts +++ b/packages/api/src/services/subscriptions.ts @@ -78,6 +78,16 @@ const sendUnsubscribeHttpRequest = async (url: string): Promise => { } } +export const getSubscriptionByNameAndUserId = async ( + name: string, + userId: string +): Promise => { + return getRepository(Subscription).findOneBy({ + name, + user: { id: userId }, + }) +} + export const saveSubscription = async ({ userId, name, From 826565a2c46cf79a405ae8340f1d05761c314789 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 21 Jun 2023 12:13:22 +0800 Subject: [PATCH 3/3] temporarily skip unsubscribing by url and mark them as unsubscribed automatically --- packages/api/src/services/subscriptions.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/api/src/services/subscriptions.ts b/packages/api/src/services/subscriptions.ts index d00d19f6c..59efb74ed 100644 --- a/packages/api/src/services/subscriptions.ts +++ b/packages/api/src/services/subscriptions.ts @@ -114,19 +114,14 @@ export const saveSubscription = async ({ export const unsubscribe = async (subscription: Subscription) => { let unsubscribed = false if (subscription.unsubscribeMailTo) { - // unsubscribe by sending email first + // unsubscribe by sending email unsubscribed = await sendUnsubscribeEmail( subscription.unsubscribeMailTo, subscription.newsletterEmail.address ) - } else if (subscription.unsubscribeHttpUrl) { - // unsubscribe by sending http request if no unsubscribeMailTo - unsubscribed = await sendUnsubscribeHttpRequest( - subscription.unsubscribeHttpUrl - ) - } else { - console.log('No unsubscribe method defined') } + // TODO: find a good way to unsubscribe by url if email fails or not provided + // because it often requires clicking a button on the page to unsubscribe if (!unsubscribed) { // update subscription status to unsubscribed if failed to unsubscribe