[ENG-8172] Improve email testing #11170
Conversation
Ostap-Zherebetskyi
force-pushed
the
feature/update_mail_testing
branch
2 times, most recently
from
338ab3b to
1c7e2d2
Compare
There was a problem hiding this comment.
This is good work, but you should include tests for the user confirmation email token being received. We've had issues in the past with browsers automatically requesting email links, like the token confirmation link and we need unittests to ensure the confirmation token is received and usable. But otherwise good.
There was a problem hiding this comment.
This looks good, other then a minor issue with the Waffle Flagging, but I got several errors testting the test_mailhog.py tests locally via the command line. I tweaked the host a little and this seems to work for CI and locall:y Johnetordoff@eb00648 If you can reproduce the issue locally and fix this way (or some other way), I will approve the PR.
| @@ -119,6 +148,17 @@ def send_mail( | |||
| logger.debug('Sending email...') | |||
| logger.debug(f'To: {to_addr}\nFrom: {from_addr}\nSubject: {subject}\nMessage: {message}') | |||
|
|
|||
| if waffle.switch_is_active(features.ENABLE_MAILHOG): | |||
There was a problem hiding this comment.
This should be a flag instead of a switch, Waffle Flags can use the request/user object to determine the flag's value, that will allow us to set a group of beta testers or staff who can test using the admin app on staging environments Waffle Switches are all or nothing, so Flags are a big improvement here.
There was a problem hiding this comment.
We can't use flags here because not all emails are triggered with a request, which is a required parameter in flag_is_active
Johnetordoff
merged commit 2f102f7
into
CenterForOpenScience:refactor-notifications
Purpose
TBD
Changes
TBD
QA Notes
TBD
Documentation
TBD
Side Effects
TBD
Ticket
https://openscience.atlassian.net/browse/ENG-8172