Create a wrapper around the Mailjet API #57580

bethanyaconnor · 2024-03-27T16:55:35Z

This PR creates a wrapper around the mailjet SDK. It also creates the method that will send the welcome email and create a contact.

I decided to include a constants file that lists the template ids (ids from the mailjet platform), as well as some general metadata about each email (such as who it is from). I figured this would be extensible if we ever moved workshop emails to this platform as well, which don't come from Hadi. In all likelihood, I'll update these constants again before full launch.

See the tech plan for details.

…marketing/mailjet-wrapper

hannahbergam

I left a few questions- thanks for adding the tests!

hannahbergam · 2024-04-08T17:53:27Z

lib/cdo/mailjet.rb

+  SECRET_KEY = CDO.try(:mailjet_secret_key).freeze
+
+  def self.enabled?
+    return false unless DCDO.get('use_mailjet', false)


I always have a hard time reading 'unless', but I'm surprised ust_mailjet would be false here

This feels super unintuitive and requires a few mental jumps. Why can't we just use the much simpler form?

Suggested change

return false unless DCDO.get('use_mailjet', false)

return DCDO.get('use_mailjet', false)

Combined this line with the one below

And used present? over negating nil?

hannahbergam · 2024-04-08T17:54:36Z

lib/cdo/mailjet.rb

+  def self.create_contact_and_send_welcome_email(user)
+    return unless enabled?
+
+    return if user&.id.nil?


Can we switch this one to an 'unless' to match the other two for readability?

lib/cdo/mailjet.rb

hannahbergam · 2024-04-08T17:55:52Z

lib/cdo/mailjet.rb

+
+    # If a contact already exists, Mailjet will raise an error.
+    # This shouldn't happen as user emails should be unique, but
+    # we don't want to block sign up if it does.


hannahbergam · 2024-04-08T17:57:58Z

lib/cdo/mailjet.rb

+    end
+
+    # Most likely, the above would fail if a contact already exists.
+    # In that case, we want to update the contact with the sign up date.


Is this in the case that (for example), someone would opt in to receive the 'host an hour of code' email but not yet have an account?

oo true, I always forget about how many ways we get user emails. That seems like a likely pathway!

cat5inthecradle

Got some nits about the implementation and always like to see more exhaustive tests around non-happy paths.

At a high level I think that the API you're exposing from Cdo::MailJet makes sense, and gives us a good place to wire in error handling, metrics, etc

cat5inthecradle · 2024-04-08T19:42:58Z

lib/cdo/mailjet.rb

+  def self.create_contact(email, name, sign_up_date)
+    return unless enabled?
+
+    Mailjet.configure do |config|
+      config.api_key = API_KEY
+      config.secret_key = SECRET_KEY
+      config.api_version = "v3"
+    end
+
+    # If a contact already exists, Mailjet will raise an error.
+    # This shouldn't happen as user emails should be unique, but
+    # we don't want to block sign up if it does.
+    begin
+      Mailjet::Contact.create(
+        is_excluded_from_campaigns: true,
+        email: email,
+        name: name
+      )
+    rescue => exception
+      Honeybadger.notify(exception)
+    end
+
+    # Most likely, the above would fail if a contact already exists.
+    # In that case, we want to update the contact with the sign up date.
+    # However, in the case of a different error, we want to notify Honeybadger,
+    # but not block sign up.
+    begin
+      contact = Mailjet::Contactdata.find(email)
+      contact.update_attributes(
+        data: [
+          {
+            name: 'sign_up_date',
+            value: sign_up_date
+          }
+        ]
+      )
+    rescue => exception
+      Honeybadger.notify(exception)
+    end
+  end


Right now this function always makes three API calls. Can we reduce that to one or two calls by rearranging it?

FIND contact by email

if found, UPDATE the sign_up_date if necessary

else CREATE with sign_up_date attributes

I think that would also make it more straightforward to add test cases for the various code paths.

Additionally, this function is squashing all errors. That seems undesireable and I can think of a few scenarios where you definitely don't want to be calling send_template_email if create_contact fails.

CREATE with sign_up_date attributes

This always requires two API calls because properties have to be updated through a separate API.

I think you've got a good point about finding the contact first then branching though -- I'll update.

Ah, that's frustrating. In that case perhaps the create doing double duty as an existence check is fine.

Refactored and it's much cleaner! I ended up removing the error squashing in general, but I'm very open to discussion there. In particular, moving the call to find the contact up means that the primary error case disappeared, so I'm less attached to squashing the errors. There's a part of me that's considering adding them back, however, in case MailJet is down or something. Thoughts?

Depends if we're going to do anything about it. For a general wrapper, I think it makes sense to raise an exception if create_contact fails to create a contact. Catching that exception is the responsibility of the caller, and it can decide whether to retry, fail, or move on. Unless there's a specific exception category that this wrapper should be responsible for resolving (for example, I've worked with API's that have rate limits that were resolved by trying again one more time right now)

Makes sense! And thinking through it, I don't want to add any exception handling here. When I hook this up into the actual user sign up flow, I might, but that is coming in a future PR.

cat5inthecradle · 2024-04-08T19:45:10Z

lib/cdo/mailjet.rb

+            Name: to_name
+          }
+        ],
+        TemplateID: template_id,


Would there be value in enforcing that the template_id be found in MailJetConstants?

Do you mean to push the fetching of the template ID down to this function?

I was just noticing that there's nothing to force future developers to use to use the template listing instead of hardcoding template ID's elsewhere in the codebase. Once we have examples of using the wrapper, the convention might be more clear though.

cat5inthecradle · 2024-04-09T14:22:42Z

lib/cdo/mailjet.rb

+  def self.subaccount
+    case Rails.env
+    when 'production'
+      'production'
+    when 'staging'
+      'staging'
+    else
+      'development'
+    end
+  end


I was re-reading the tech plan and was reminded that we're explicitly planning on NOT connecting to mailjet for Test and Drone. Is there a good place to make that intention clear in the code?

Yeah -- I could add a check in enabled?. Let me do that.

oh, sorry, I meant clear to future developers wondering why production and staging are getting values but not anything else (because we don't have a standard convention or clear guidance about the different environments)

oh gotcha! Yeah I can add something here as well

…e_email

bethanyaconnor added 4 commits March 20, 2024 11:36

Add test and constants file

9ba3d34

Update constants

34d9b17

Wrap Mailjet calls with rescue and Honeybadger notifications

8c8820b

Update contants for staging and to be from Hadi

540cb59

bethanyaconnor marked this pull request as ready for review March 27, 2024 20:01

bethanyaconnor requested review from cat5inthecradle and a team March 27, 2024 20:02

bethanyaconnor added 2 commits April 2, 2024 09:43

Merge remote-tracking branch 'origin/staging' into bethany/lifecycle-…

d5ae00d

…marketing/mailjet-wrapper

Merge remote-tracking branch 'origin/staging' into bethany/lifecycle-…

85a4fdc

…marketing/mailjet-wrapper

hannahbergam reviewed Apr 8, 2024

View reviewed changes

cat5inthecradle requested changes Apr 8, 2024

View reviewed changes

bethanyaconnor added 2 commits April 9, 2024 10:10

PR feedback: syntax

32125a0

Push date format conversion into create_contact method

ade5644

cat5inthecradle reviewed Apr 9, 2024

View reviewed changes

bethanyaconnor added 2 commits April 9, 2024 10:31

Refactor create_contact

ebad19e

Restrict sending in test and move fetching contacts into send_templat…

fe6573c

…e_email

bethanyaconnor requested review from cat5inthecradle and hannahbergam April 9, 2024 14:44

cat5inthecradle approved these changes Apr 9, 2024

View reviewed changes

Clarify that Mailjet shouldn't be used in test

e26e532

bethanyaconnor merged commit c0a6861 into staging Apr 11, 2024
2 checks passed

bethanyaconnor deleted the bethany/lifecycle-marketing/mailjet-wrapper branch April 11, 2024 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a wrapper around the Mailjet API #57580

Create a wrapper around the Mailjet API #57580

bethanyaconnor commented Mar 27, 2024 •

edited

hannahbergam left a comment

hannahbergam Apr 8, 2024

cat5inthecradle Apr 8, 2024

bethanyaconnor Apr 9, 2024

bethanyaconnor Apr 9, 2024

hannahbergam Apr 8, 2024

hannahbergam Apr 8, 2024

hannahbergam Apr 8, 2024

bethanyaconnor Apr 9, 2024

cat5inthecradle left a comment

cat5inthecradle Apr 8, 2024

cat5inthecradle Apr 8, 2024

bethanyaconnor Apr 9, 2024

cat5inthecradle Apr 9, 2024

bethanyaconnor Apr 9, 2024

cat5inthecradle Apr 9, 2024

bethanyaconnor Apr 11, 2024

cat5inthecradle Apr 8, 2024

bethanyaconnor Apr 9, 2024

cat5inthecradle Apr 9, 2024

cat5inthecradle Apr 9, 2024

bethanyaconnor Apr 9, 2024

cat5inthecradle Apr 9, 2024

bethanyaconnor Apr 11, 2024

	return false unless DCDO.get('use_mailjet', false)
	return DCDO.get('use_mailjet', false)

Create a wrapper around the Mailjet API #57580

Create a wrapper around the Mailjet API #57580

Conversation

bethanyaconnor commented Mar 27, 2024 • edited

hannahbergam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cat5inthecradle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethanyaconnor commented Mar 27, 2024 •

edited