Mailchimp unsubscribe webhook #5804

citizen428 · 2020-01-29T07:22:36Z

What type of PR is this? (check all applicable)

Bug Fix

Description

When user's directly unsubscribe from Mailchimp, we don't have any record of that and keep sending emails. This PR adds a webhook endpoint so we get notified of unsubscribe events (docs.

Related Tickets & Documents

https://github.com/thepracticaldev/tech-private/issues/289

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

docs.dev.to
readme
no documentation needed

citizen428 · 2020-01-30T09:43:31Z

app/controllers/webhooks/mailchimp_unsubscribes_controller.rb

+class Webhooks::MailchimpUnsubscribesController < ApplicationController
+  class InvalidListID < StandardError; end
+
+  LIST_MAPPINGS = {


Is there a better way to do this lookup/is this mapping defined somewhere already?

This is the first of it's kind 😄

Ok, good to know. Can you please sanity check the mapping?

citizen428 · 2020-01-30T09:44:19Z

app/controllers/webhooks/mailchimp_unsubscribes_controller.rb

+
+  def create
+    not_authorized unless valid_secret?
+    user = User.find_by!(email: params.dig(:data, :email))


I decided to raise in case we can't find the user, but I'm open to other approaches (log to DataDog and no-op for example).

This should be fine for the time being. It should be caught by honeybadger.

I wonder if we should not raise while still testing this out/debugging, because if my assumption about the webhook payload are wrong we'll raise for no good reason.

app/controllers/webhooks/mailchimp_unsubscribes_controller.rb

citizen428 · 2020-01-30T09:48:29Z

config/routes.rb

@@ -154,11 +154,15 @@
    resources :reads, only: [:create]
  end

+  namespace :webhooks do
+    post "/mailchimp/:secret/unsubscribe", to: "mailchimp_unsubscribes#create", as: :mailchimp_unsubscribe


We can probably change this at one point and use routing DSL trickery, but for now this seems good enough.

maestromac · 2020-02-04T13:24:17Z

I think this is a very solid first iteration

citizen428 · 2020-02-05T07:34:12Z

I think this is a very solid first iteration

In light of this I removed the WIP tag now. Just let me know what we should regarding the exception if we can't find the user (I left a comment), then we could take this for a spin.

rhymes

I'm still not 100% sure we should these things webhooks as it maybe confusing with user registered webhooks for outgoing events but I also don't have a better name 🗡

Good job @citizen428

config/routes.rb

rhymes · 2020-02-05T12:07:44Z

app/controllers/webhooks/mailchimp_unsubscribes_controller.rb

@@ -0,0 +1,30 @@
+class Webhooks::MailchimpUnsubscribesController < ApplicationController
+  class InvalidListID < StandardError; end


should all this logic (the mappings and the email_type) be in the controller?

Since this is a POC, I opted for "easy to remove again" as my optimization target. If this sticks around, we can always move it out later. But if you prefer I can also move it out now, but since this mapping isn't used elsewhere yet, I don't have a problem with it being in this controller for the time being.

no, it makes sense, let's leave it as is

citizen428 · 2020-02-06T03:36:33Z

I'm still not 100% sure we should these things webhooks as it maybe confusing with user registered webhooks

How about IncomingWebhooks or WebhookCallbacks?

rhymes · 2020-02-06T08:36:50Z

How about IncomingWebhooks

Great idea!

citizen428 · 2020-02-06T10:51:59Z

Changed back to WIP as I'll do some renaming tomorrow.

citizen428 · 2020-02-07T03:05:39Z

@rhymes @maestromac I renamed this to IncomingWebhooks, much less potential for confusion. Please review again.

rhymes

Looks good @citizen428!

What are the next steps when this is merged?

rhymes · 2020-02-10T13:40:23Z

spec/requests/incoming_webhooks/mailchimp/unsubscribe_spec.rb

+RSpec.describe "IncomingWebhooks::MailchimpUnsubscribesController", type: :request do
+  let(:user) { create(:user, email_digest_periodic: true) }
+
+  describe "POST /webhooks/mailchimp/:secret/unsubscribe" do


there's a typo here but I don't think it should hold the PR.

Do you mean that it should say incoming_webhooks? Because I don't really see a typo, but that could also be because I knew what I wanted to write 😉

vaidehijoshi · 2020-02-10T15:32:04Z

app/controllers/incoming_webhooks/mailchimp_unsubscribes_controller.rb

+    mailchimp_sustaining_members_id: :email_membership_newsletter,
+    mailchimp_tag_moderators_id: :email_tag_mod_newsletter,
+    mailchimp_community_moderators_id: :email_community_mod_newsletter


from what i can tell, we're not using most of these keys; what do we need them for? (just trying to get context on this)

This is the mapping of option names in the SiteConfig model to newsletter types. We're using it here:

list_id = params.dig(:data, :list_id) key = LIST_MAPPINGS.keys.detect { |k| SiteConfig.public_send(k) == list_id }

Once we have mapped from list id to attribute name, we use the latter in the update user.update(email_type => false).

Does that clear it up?

app/controllers/incoming_webhooks/mailchimp_unsubscribes_controller.rb

vaidehijoshi · 2020-02-10T15:35:59Z

spec/requests/incoming_webhooks/mailchimp/unsubscribe_spec.rb

+
+      expect do
+        post "/incoming_webhooks/mailchimp/#{secret}/unsubscribe", params: params
+      end.to change { user.reload.email_newsletter }.from(true).to(false)


nice, i didn't know about the from(true).to(false) syntax! that's so much nicer than asserting the value of email_newsletter on the user before making the post request, and then checking it again afterwards!! ✨

There's also by, which can specify an amount of change, i.e.

expect do User.create(...) end.to change(User, :count).by(1)

citizen428 · 2020-02-11T01:42:38Z

@rhymes The next step would be to use @maestromac's account to subscribe/unsubscribe and confirm some of my assumptions around the payload shape etc.

Prototype Mailchimp unsubscribe webhook

ca6d251

citizen428 requested a review from a team January 29, 2020 07:22

rhymes removed the request for review from a team January 29, 2020 13:43

Implement lookup by list id

158fe52

citizen428 commented Jan 30, 2020

View reviewed changes

citizen428 force-pushed the citizen428/mailchimp-webhook-tech-private-289 branch from 73b4bab to 158fe52 Compare January 30, 2020 14:07

weekly-digest bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

citizen428 changed the title ~~[WIP] Mailchimp unsubscribe webhook~~ Mailchimp unsubscribe webhook Feb 5, 2020

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Feb 5, 2020

rhymes reviewed Feb 5, 2020

View reviewed changes

Remove extra space

cc4a2cd

citizen428 changed the title ~~Mailchimp unsubscribe webhook~~ [WIP] Mailchimp unsubscribe webhook Feb 6, 2020

pr-triage bot removed the PR: unreviewed bot applied label for PR's with no review label Feb 6, 2020

citizen428 added 2 commits February 7, 2020 10:01

Rename for clarity

86e29ba

Also rename SiteConfig value

458c78c

citizen428 changed the title ~~[WIP] Mailchimp unsubscribe webhook~~ Mailchimp unsubscribe webhook Feb 7, 2020

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Feb 7, 2020

weekly-digest bot mentioned this pull request Feb 10, 2020

Weekly Digest (3 February, 2020 - 10 February, 2020) #5980

Closed

rhymes approved these changes Feb 10, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 10, 2020

rhymes requested a review from vaidehijoshi February 10, 2020 13:41

vaidehijoshi approved these changes Feb 10, 2020

View reviewed changes

maestromac approved these changes Feb 11, 2020

View reviewed changes

maestromac merged commit 050a2e2 into master Feb 11, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 11, 2020

maestromac deleted the citizen428/mailchimp-webhook-tech-private-289 branch February 11, 2020 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mailchimp unsubscribe webhook #5804

Mailchimp unsubscribe webhook #5804

citizen428 commented Jan 29, 2020 •

edited

citizen428 Jan 30, 2020

maestromac Feb 4, 2020

citizen428 Feb 5, 2020

citizen428 Jan 30, 2020

maestromac Feb 4, 2020

citizen428 Feb 5, 2020

citizen428 Jan 30, 2020

maestromac commented Feb 4, 2020

citizen428 commented Feb 5, 2020

rhymes left a comment •

edited

rhymes Feb 5, 2020

citizen428 Feb 6, 2020

rhymes Feb 6, 2020

citizen428 commented Feb 6, 2020

rhymes commented Feb 6, 2020

citizen428 commented Feb 6, 2020

citizen428 commented Feb 7, 2020

rhymes left a comment

rhymes Feb 10, 2020

citizen428 Feb 11, 2020

vaidehijoshi Feb 10, 2020

citizen428 Feb 11, 2020 •

edited

vaidehijoshi Feb 10, 2020

citizen428 Feb 11, 2020

citizen428 commented Feb 11, 2020

		@@ -0,0 +1,30 @@
		class Webhooks::MailchimpUnsubscribesController < ApplicationController
		class InvalidListID < StandardError; end

Mailchimp unsubscribe webhook #5804

Mailchimp unsubscribe webhook #5804

Conversation

citizen428 commented Jan 29, 2020 • edited

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

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

maestromac commented Feb 4, 2020

citizen428 commented Feb 5, 2020

rhymes left a comment • edited

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

citizen428 commented Feb 6, 2020

rhymes commented Feb 6, 2020

citizen428 commented Feb 6, 2020

citizen428 commented Feb 7, 2020

rhymes 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

citizen428 Feb 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

citizen428 commented Feb 11, 2020

citizen428 commented Jan 29, 2020 •

edited

rhymes left a comment •

edited

citizen428 Feb 11, 2020 •

edited