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

fix: switch bulk emails to individual email sends #1013

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ludtkemorgan
Copy link
Collaborator

@ludtkemorgan ludtkemorgan commented Dec 17, 2024

This PR addresses #986 #983 #985 #1008

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

The current setup for emails with multiple recipients has the following problems:

  • Unable to send to more than 50 emails at a time (limit from AWS)
  • Unable to track if a user got an email if there is more than one recipient of the email
  • Limited logging makes it hard to see if group emails were sent via the backend

This change resolves the above issues by doing the following:

  • Switch to using the v2 version of the AWS SES sdk and no longer use nodemailer
  • All emails are now individually sent and recipient is in the to rather than bcc
  • Use the SendBulkEmailCommand which can bulk send 50 emails at a time
    • All bulk emails are now chunked into groups of 50 emails and for each group one bulk send is sent
  • Add logger calls before every bulk email send with the number of emails it is sending to
  • Add additional logging for failed email sends
  • Lottery emails are also sent to an environment variable designated email address

Along with the above the following bugs/issues were addressed:

  • New listing notification email was missing several community types
  • The applicant lottery published email had the placeholder exygy.com url instead of the correct ones for the notificationsUrl and helpCenterUrl
  • All user related emails did not have an await. This was not causing an issue, but if a failure happened it would have been harder to track since the user potentially could have got the response before email actually sent
  • Update tests to make sure the correct emails are sent

How Can This Be Tested/Reviewed?

Since this change touches how all emails are sent in Doorway we will need to test all of the different emails sent out

Here is documentation for the the list of emails https://docs.google.com/document/d/1bPl-SVapMcktRt06u-vaT1Lial4OR-gpe__vU0Dgt4c/edit?tab=t.0

To test the site email is also sent during the three lottery emails you will need to add a value to the api's .env file. After adding this you should receive an email for lotteryReleased, lotteryPublishedAdmin, and lotteryPublishedApplicant email sends
Sample:
[email protected]

For the bulk emails some additional testing should be done.
Scenarios:

  • Trigger an email that will go out to more than 50 people
    An easy way test many email send you could add this code to the seed-staging.ts file before line 349 (change the email to your email)
  const bulkApplications = [];
  for (let i = 0; i <= 51; i++) {
    const user = await prismaClient.userAccounts.create({
      data: await userFactory({
        email: `morgan.ludtke+${i}@exygy.com`,
        confirmedAt: new Date(),
        jurisdictionIds: [jurisdiction.id],
        acceptedTerms: true,
        password: 'abcdef',
      }),
    });
    bulkApplications.push(
      await applicationFactory({
        applicant: { emailAddress: `morgan.ludtke+${i}@exygy.com` },
        userId: user.id,
      }),
    );
  }

Then change the applications list to that bulkApplications list for any of the listings

applications: bulkApplications

Then you should be able to do the whole lottery release flow to receive 50+ application emails

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@ludtkemorgan ludtkemorgan marked this pull request as ready for review December 17, 2024 19:51
@ludtkemorgan ludtkemorgan changed the title Bulk email fix: switch bulk emails to individual email sends Dec 17, 2024
@ColinBuyck
Copy link
Collaborator

ColinBuyck commented Dec 23, 2024

@ludtkemorgan Can you provide some further instructions on how to properly test the bulk email to 50 plus people? Did you do it manually? Could we expand our seeding to include a 'high-user' option like Emily did with yarn setup:large?

@ludtkemorgan
Copy link
Collaborator Author

@ColinBuyck Good question!
My vote would be to not add a new script that adds a bunch of users/applications. Since we use one SES account for both testing and production I think we should only do the large bulk emails when we are actively testing since it will impact our logs in AWS, even when testing locally.

However, it is fairly easy to add a bunch of applications in the seeding data
You can create a listing of applications like this in seed-staging.ts

  const bulkApplications = [];
  for (let i = 0; i <= 51; i++) {
    bulkApplications.push(await applicationFactory());
  }

And then in any of the listings change the applications to applications: bulkApplications

api/src/services/email.service.ts Outdated Show resolved Hide resolved
api/src/services/email.service.ts Outdated Show resolved Hide resolved
}

public async sendMfaCode(user: User, singleUseCode: string) {
const jurisdiction = await this.getJurisdiction(user.jurisdictions);
void (await this.loadTranslations(jurisdiction, user.language));

await this.sendSES({
await this.sendSingleSES({
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Why do some calls not use the useCase field but not others?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a mistake, I meant to go back to add useCase to all emails. That has been done!

console.log(e);
console.error('Failed to send email');
this.logger.log(e);
this.logger.error(`Failed to send ${useCase} email to ${mailOptions.to}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: useCase is an optional field that isn't always supplied, message can be less helpful if it is null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Usecase is now a required field!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants