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

PAS-452 | Design generic fallback for e-mail providers & drop PostMark #636

Merged
merged 29 commits into from
Aug 26, 2024

Conversation

jonashendrickx
Copy link
Member

@jonashendrickx jonashendrickx commented Jul 13, 2024

Ticket

Description

This pull request removes support for Postmark.

Any e-mail provider, including Postmark, can still continue to be configured using SMTP. Read more here.

Shape

Features:

  • Allow configuration of an ordered list of mail providers.
  • Can combine multiple mail providers of the same type, i.e. AWS. For example, can be used if one region is down, and fall back to using a different region. This is purely an example, as not every mail provider could support this out of the box.
  • When no mail provider is configured, use the FileMailProvider
  • Allow updating the provider configurations without having to restart the application.

In the example below, we're able to attempt to send e-mails first using AWS SES using region us-west-2, if this fails, we fall back to AWS SES using region us-east-2. If that again fails, we attempt to send e-mails using SendGrid. Please note the example is purely hypothetical and ignores any vendor specific features such as the availability zones with redundant connections with AWS SES to guarantee minimum down-time.

If all configured attempts fail, an error will be thrown by the back-end.

  "Mail": {
    "From": "johndoe@example.com",
    "Providers": [
      {
        "Name": "aws",
        "AccessKey": "aws_access_key_id",
        "SecretKey": "aws_secret_key",
        "Region": "us-west-2"
      },
      {
        "Name": "aws",
        "AccessKey": "aws_access_key_id",
        "SecretKey": "aws_secret_key",
        "Region": "us-east-1"
      },
      {
        "Name": "sendgrid",
        "ApiKey": "sendgrid_api_key"
      }
    ]
  },

Screenshots

Checklist

I did the following to ensure that my changes were tested thoroughly:

  • __

I did the following to ensure that my changes do not introduce security vulnerabilities:

  • __

@jonashendrickx jonashendrickx requested a review from a team as a code owner July 13, 2024 05:39
Copy link

codecov bot commented Jul 13, 2024

Codecov Report

Attention: Patch coverage is 34.11215% with 141 lines in your changes missing coverage. Please review.

Project coverage is 35.13%. Comparing base (4861cbe) to head (3582e58).

Files Patch % Lines
src/Common/Services/Mail/Aws/AwsMailProvider.cs 0.00% 50 Missing ⚠️
...mon/Services/Mail/SendGrid/SendGridMailProvider.cs 0.00% 18 Missing ⚠️
src/Common/Services/Mail/MailProviderFactory.cs 0.00% 16 Missing ⚠️
src/Common/Services/Mail/MailBootstrap.cs 58.33% 14 Missing and 1 partial ⚠️
src/Common/Services/Mail/MailMessage.cs 50.00% 10 Missing ⚠️
src/Common/Services/Mail/Smtp/SmtpMailProvider.cs 0.00% 10 Missing ⚠️
...mmon/Services/Mail/Smtp/SmtpMailProviderOptions.cs 0.00% 9 Missing ⚠️
...sole/Components/Pages/Organization/Verify.razor.cs 57.14% 4 Missing and 2 partials ⚠️
src/Common/HealthChecks/MailKitHealthCheck.cs 0.00% 3 Missing ⚠️
src/Common/Services/Mail/File/FileMailProvider.cs 0.00% 3 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #636      +/-   ##
==========================================
- Coverage   35.15%   35.13%   -0.02%     
==========================================
  Files         568      574       +6     
  Lines       31012    31144     +132     
  Branches      929      929              
==========================================
+ Hits        10903    10944      +41     
- Misses      19961    20055      +94     
+ Partials      148      145       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abergs
Copy link
Member

abergs commented Jul 15, 2024

@jonashendrickx we're not merging this until more work is done in PAS-452, right?

@jonashendrickx

This comment was marked as outdated.

…WS-SES-SendGrid

# Conflicts:
#	src/AdminConsole/Pages/Organization/Create.cshtml.cs
#	src/AdminConsole/Pages/Organization/Verify.cshtml.cs
#	src/Common/Common.csproj
#	src/Common/Services/Mail/MailBootstrap.cs
#	src/Common/Services/Mail/MailConfiguration.cs
src/Common/Services/Mail/Aws/AwsProvider.cs Outdated Show resolved Hide resolved
src/Common/Services/Mail/File/FileProvider.cs Outdated Show resolved Hide resolved
src/Common/Services/Mail/MailMessage.cs Outdated Show resolved Hide resolved
src/Common/Services/Mail/SendGrid/SendGridProvider.cs Outdated Show resolved Hide resolved
src/Common/Services/Mail/Smtp/SmtpProviderOptions.cs Outdated Show resolved Hide resolved
@jonashendrickx jonashendrickx requested a review from a team August 20, 2024 14:41
@jonashendrickx jonashendrickx changed the title PAS-452 | Remove Postmark PAS-452 | Design generic fallback for e-mail providers Aug 24, 2024
@jonashendrickx jonashendrickx changed the title PAS-452 | Design generic fallback for e-mail providers PAS-452 | Design generic fallback for e-mail providers & drop PostMark Aug 24, 2024
@jonashendrickx jonashendrickx enabled auto-merge (squash) August 24, 2024 08:12
Comment on lines 22 to 27
if (!section.GetChildren().Any())
{
var fileProviderOptions = new FileMailProviderOptions();
o.Providers.Add(fileProviderOptions);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a no-op mail provider in the default case. This may be useful in tests, where you don't care about emails at all and the app in the container may not have the permissions to have files.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add support for it later in the integration testing project

src/Common/Services/Mail/MailConfiguration.cs Outdated Show resolved Hide resolved
src/Common/Services/Mail/OrderedMailProvider.cs Outdated Show resolved Hide resolved
@jonashendrickx jonashendrickx merged commit 8697acd into main Aug 26, 2024
14 of 15 checks passed
@jonashendrickx jonashendrickx deleted the PAS-452-Remove-Postmark-and-use-AWS-SES-SendGrid branch August 26, 2024 19:55
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