Skip to content
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

feat: migrate transactional emails to cio #1940

Merged
merged 4 commits into from
May 28, 2024
Merged

feat: migrate transactional emails to cio #1940

merged 4 commits into from
May 28, 2024

Conversation

idoshamun
Copy link
Member

Still WIP but most of the work is done. I placed some todos that I need to fix, I'm waiting for a response from their support.

It actually required more changes than I expected as some things are not supported:

  • Sending emails in batches with personalization parameters
  • Generating batch id for the digest

This comment has been minimized.

@@ -683,64 +683,6 @@ describe('generateNotification', () => {
]);
});

it('should generate squad_post_viewed notification', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This notification is no longer used

This comment has been minimized.

@@ -1049,24 +991,6 @@ describe('storeNotificationBundle', () => {
expect(actual.attachments.length).toEqual(0);
});

it('should generate squad_access notification', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This notification is no longer used

if (!('id' in data.identifiers)) {
throw new Error('identifiers.id is required');
}
const token = await signJwt({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required for custom unsubscribe as mentioned here:
https://customer.io/docs/journeys/custom-unsubscribe-links/

Still WIP but most of the work is done. I placed some todos that I need to fix, I'm waiting for a response from their support.

It actually required more changes than I expected as some things are not supported:
* Sending emails in batches with personalization parameters
* Generating batch id for the digest
@idoshamun idoshamun marked this pull request as ready for review May 27, 2024 15:50
@idoshamun idoshamun requested review from capJavert and a team as code owners May 27, 2024 15:50
@idoshamun idoshamun changed the title feat: migrate transactional emails to cio (wip) feat: migrate transactional emails to cio May 27, 2024

This comment has been minimized.

src/routes/index.ts Fixed Show resolved Hide resolved
Copy link
Contributor

@capJavert capJavert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor stuff so far, actually surprised because I expected more major changes. Nice work 🚀

@@ -127,7 +119,6 @@ export const features = {
}),
dailyDigest: new Feature('daily_personalized_digest', {
...digestFeatureBaseConfig,
templateId: 'd-925d2759ddd641f99220b3c7c6836458',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Digests now have the same template ID by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. nimrod asked to merge the templates together

Comment on lines 141 to 142
fastify.post<{ Querystring: { token: string } }>(
'/unsubscribe',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move under /notifications, separate file or just add to path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow really not that bad of a change!

category: 'Digests',
asmGroupId: 23809,
},
templateId: '46',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to update more templateId's?
Or we'll do the main ones seperate later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

pulumi bot commented May 28, 2024

🍹 The Update (preview) for dailydotdev/api/prod was successful.

Resource Changes

    Name                                        Type                           Operation
~   vpc-native-update-tag-recommendations-cron  kubernetes:batch/v1:CronJob    update
~   vpc-native-generate-search-invites-cron     kubernetes:batch/v1:CronJob    update
+   vpc-native-api-migration-0761068c           kubernetes:batch/v1:Job        create
~   vpc-native-daily-digest-cron                kubernetes:batch/v1:CronJob    update
~   vpc-native-ws-deployment                    kubernetes:apps/v1:Deployment  update
~   vpc-native-check-analytics-report-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-update-trending-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-update-source-tag-view-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-bg-deployment                    kubernetes:apps/v1:Deployment  update
~   vpc-native-update-discussion-score-cron     kubernetes:batch/v1:CronJob    update
~   vpc-native-update-views-cron                kubernetes:batch/v1:CronJob    update
~   vpc-native-export-to-tinybird-cron          kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tags-str-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-images-cron         kubernetes:batch/v1:CronJob    update
~   vpc-native-personalized-digest-cron         kubernetes:batch/v1:CronJob    update
~   vpc-native-deployment                       kubernetes:apps/v1:Deployment  update
~   vpc-native-private-deployment               kubernetes:apps/v1:Deployment  update
~   vpc-native-personalized-digest-deployment   kubernetes:apps/v1:Deployment  update
-   vpc-native-api-migration-f868865a           kubernetes:batch/v1:Job        delete
+-  vpc-native-k8s-secret                       kubernetes:core/v1:Secret      create-replacement
~   vpc-native-clean-zombie-users-cron          kubernetes:batch/v1:CronJob    update
~   vpc-native-generic-referral-reminder-cron   kubernetes:batch/v1:CronJob    update

src/routes/notifications.ts Dismissed Show dismissed Hide dismissed
@idoshamun idoshamun merged commit eb83dae into main May 28, 2024
8 checks passed
@idoshamun idoshamun deleted the cio-email branch May 28, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants