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: automate critical bounce handling #318

Merged
merged 68 commits into from
Sep 22, 2020
Merged

Conversation

mantariksh
Copy link
Contributor

@mantariksh mantariksh commented Sep 14, 2020

Problem

Our work to handle critical bounces is generally repetitive. We usually have to send the same email to admins repeatedly, filling in the blanks for form titles and URLs ourselves. Moreover, when all bounces are permanent, the form needs to be deactivated. This work should be automated.

Solution

Automate sending of critical bounce emails as well as form deactivation.

The changes can be reviewed in commit order, but here is a list of changes and refactors:

  1. Types from mail.service.ts and mail.util.ts were moved into src/types/mail.ts.
  2. The sendBounceNotification function was implemented in mail.service.ts, and tests were implemented in mail.service.spec.ts.
  3. Static method deactivateById was added to the Form schema for form deactivation, and tests were implemented in form.server.model.spec.ts.
  4. The key hasAlarmed in the Bounce schema was changed to hasAutoEmailed, to reflect whether an automated email has been sent for this form for this critical bounce. Bounce schema was updated to store a bounceType for every email address. This is important in order to be sure that all emails have permanently bounced before deactivating the form.
  5. Bounce schema instance method merge was changed to updateBounceInfo, and now takes in only an SNS notification instead of a Bounce document as well as an SNS notification. It didn't make sense to derive a Bounce document from an SNS notification, then pass both to the merge method. Hence updateBounceInfo now updates the Bounce document purely based on the notification.
  6. Instance methods areAllPermanentBounces, getEmails and setHasAutoEmailed were implemented on the Bounce schema for convenience.
  7. Templates were added for permanent and transient bounce emails under src/app/views.
  8. bounce.service.ts was updated to compute the list of admins/collaborators who were not amongst the bounced recipients, and call mailService.sendBounceNotification to email them.
  9. The Bounce controller and service were refactored to give the controller more control over the flow of actions. All the actions used to be carried out in one function in the service; instead, the controller now decides how to handle the bounce, and delegates business logic to services.
  10. Tests were added in src/app/modules/bounce/__tests__ to reflect all of the above changes.

Screenshots

image

Tests

  • Create an email mode form where there are two email recipients, one valid and one invalid. Submit the form. The form should remain active and the valid admin should not receive any automated critical bounce emails. In the database, check that the document for this form in the Bounce collection has been updated correctly with hasBounced: true and bounceType: Permanent for the invalid email recipient.
  • Change the recipients such that all recipients are invalid. Add a collaborator with a valid email. Submit the form. The form should be deactivated, and both the admin and collaborator should receive an email following the permanent bounce template notifying them of the critical bounce.
  • In production, BOUNCE_LIFE_SPAN has been updated to 86400000 (24h in milliseconds).
  • In production CloudWatch queries and dashboard widgets, Critical bounce has been changed to Bounced submission.

@mantariksh mantariksh marked this pull request as ready for review September 15, 2020 06:04
@mantariksh mantariksh requested a review from tshuli September 15, 2020 06:04
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

looks lgtm, with some minor changes suggested. could you also update the import alias to use bounceService instead of snsService? naming is hard, but doing it consistently will help to keep it simple for everyone.

@mantariksh
Copy link
Contributor Author

@liangyuanruo @karrui I'm currently writing new tests for the refactored code, but meanwhile if you can spare some time, could you help me take a look at this commit pls? I'm not sure if I'm allocating responsibility between the controller and service correctly.

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

partial comments

src/app/modules/bounce/__tests__/bounce.controller.spec.ts Outdated Show resolved Hide resolved
src/app/modules/bounce/bounce.controller.ts Outdated Show resolved Hide resolved
src/app/modules/bounce/bounce.controller.ts Outdated Show resolved Hide resolved
@mantariksh mantariksh force-pushed the bounce-automation branch 3 times, most recently from e6d6e35 to 813ec66 Compare September 21, 2020 00:32
@mantariksh mantariksh force-pushed the bounce-automation branch 2 times, most recently from bd2490e to 264e6e9 Compare September 22, 2020 02:09
@mantariksh mantariksh merged commit cbecaa5 into develop Sep 22, 2020
@tshuli tshuli mentioned this pull request Sep 22, 2020
@mantariksh mantariksh deleted the bounce-automation branch November 10, 2020 01:52
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.

4 participants