-
Notifications
You must be signed in to change notification settings - Fork 216
docs (email): update e2e email local testing docs #5760
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested some blocking changes to reduce the code churn in our diffs.
docs/end-to-end-local-email-dev.md
Outdated
* Your own domain and the ability to publish MX and CNAME records to it | ||
* AWS account | ||
* (Suggested) [ngrok.io][ngrok] account | ||
* Enable Mozilla Accounts authentication (see README) | ||
- Your own domain and the ability to publish MX and CNAME records to it | ||
- AWS account | ||
- (Suggested) [ngrok.io][ngrok] account | ||
- Enable Mozilla Accounts authentication (see README) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (blocking): Since markdown supports asterisks, pluses, and hyphens interchangeably for bulleted lists, let's leave the characters un-changed as much as we can to reduce code churn in our PRs.
#### Verify emails | ||
|
||
If you are using the AWS free trial, you will need to verify any emails you are using. SES in the AWS free tier will only send to verified emails, if you "Request production access" in the dashboard you don't need to do this. Otherwise, verifying the following emails for local testing: | ||
|
||
- `[email protected]` | ||
- `[email protected]` | ||
- `[email protected]` (only if you need to do testing with replies) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: nice!
One challenge with making both of these domains the same is that the reply logic assumes there are two different domains. In order to test replies in a local environment, you have two options: | ||
|
||
1. Set up an additional domain for the `replies` email address that is configured for SES reply handling (follow same steps as above) | ||
2. Alter the following line in `emails/views.py`: | ||
|
||
`domain_numerical = get_domain_numerical(domain)` can be replaced with: | ||
|
||
`domain_numerical = get_domain_numerical(domain) if local_address != 'example_mask_address' else 2`. | ||
|
||
This will allow replies that are sent to `[email protected]` to be properly forwarded to the user that owns the mask. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: thanks for adding this!
suggestion (non-blocking): File a tech debt ticket to refactor the reply logic to no longer assume there are two different domains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/end-to-end-local-email-dev.md
Outdated
allows SES to access the key. Add it to key policy with the other statements: | ||
|
||
```json | ||
{ | ||
"Sid": "AllowSESToEncryptMessagesBelongingToThisAccount", | ||
"Effect": "Allow", | ||
"Principal": { | ||
"Service":"ses.amazonaws.com" | ||
"Service": "ses.amazonaws.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Could reduce code churn here too by NOT re-formatting the JSON inside the json blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I'll run without the pre-commit hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh was there a pre-commit hook from the repo? Or is that a personal hook you have? If we have a pre-commit hook enforcing a certain style, we should give ourselves a ticket to update ALL the markdown files in a single PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep- there's a precommit hook enforcing it. I'll disable it for this (we can merge, etc) and file a ticket for updating all of the docs as suggested + add that to my sprint.
bfead25
to
f91877e
Compare
This PR adds additional documentation for E2E local setup docs.
Note: The whitespace, newline and bullet point changes are automatically made by Husky linting.
How to test:
l10n changes have been submitted to the l10n repository, if any.I've added a unit test to test for potential regressions of this bug.All UI revisions follow the coding standards, and use Protocol / Nebula colors where applicable (see/frontend/src/styles/colors.scss
).Commits in this PR are minimal and have descriptive commit messages.