Validate emails before adding MailJet contact #57714
Conversation
bethanyaconnor
force-pushed
the
bethany/lifecycle-marketing/email-validation
branch
from
6f471db
to
4938891
Compare
bethanyaconnor
force-pushed
the
bethany/lifecycle-marketing/email-validation
branch
from
4938891
to
ba7c04e
Compare
bethanyaconnor
changed the title
bethanyaconnor
changed the base branch from
staging
to
bethany/lifecycle-marketing/create-mailgun-secrets
| @@ -16,7 +18,8 @@ def self.enabled? | |||
| DCDO.get('use_mailjet', false) && | |||
| !Rails.env.test? && | |||
| API_KEY.present? && | |||
| SECRET_KEY.present? | |||
| SECRET_KEY.present? && | |||
| MAILGUN_API_KEY.present? | |||
There was a problem hiding this comment.
Now when I see api keys I'm called to ask: have we tested that the key is correctly configured? I'm sorry if this has already been tested elsewhere and I'm late to the game!
There was a problem hiding this comment.
no it's a good question! I've actually been checking in Secrets Manager for each key every time I update one, particularly for the exact issue that hit us with Statsig 😅 Obviously, testing that the key is definitely correct (ie I didn't copy all but the last character or something) is harder to test before this hits products, but, as this is behind a DCDO flag, hopefully it would be a short outage if there is one.
lib/cdo/mailjet.rb
Outdated
| } | ||
| ] | ||
| ) | ||
| end | ||
|
|
||
| def self.send_template_email(to_email, to_name, email_config) | ||
| def self.send_template_email(contactdata, email_config) |
There was a problem hiding this comment.
Is there any world in which this returns an error?
There was a problem hiding this comment.
I'm trying to remember -- I actually had more issues with this failing silently instead of throwing errors. Do you think this should be wrapped in exception handling in some way? (Might also be feedback for me to incorporate into #58099)
lib/test/cdo/test_mailjet.rb
Outdated
| def test_create_contact | ||
| email = 'fake.email@email.com' | ||
| def test_create_contactdata | ||
| email = 'fake.email@test.xx' |
| refute MailJet.valid_email?('test@email.com') | ||
| end | ||
|
|
||
| def test_valid_email_do_not_send |
There was a problem hiding this comment.
Thanks for all these tests!
…marketing/email-validation
|
@hannahbergam I found a bug when testing on the adhoc and updated this PR with that fix in 69c2e61 |
There was a problem hiding this comment.
Good find! So I'm following- it sounds like MailJet has two different concepts for a 'Contact' and that contact's 'ContactData' (and prior to this commit, those were combined in ContactData). Now I'm curious about all the data that could live in 'data' that wouldn't be represented in 'Contact', but that's an aside! LGTM
|
Yeahhh, it seems like |
MailGun, a related product to MailJet, offers an API to validate email addresses and out technical account manager suggested we use this functionality. This checks to make sure the emails we add to MailJet are at least real addresses and that they're not known spammy addresses.
This PR adds the MailGun gem and utilizes this API. More info on this API is here.
This also turned into a bit of a refactor to ensure the email validation is only checked once; when I first wrote it, I was checking email validation on both contact creation and email send. I decided to do this by making it so emails must be sent to MailJet contacts, even though that's not strictly necessary. Definitely open to feedback here!