Adding a route for Mandrill's url check. #38794
Conversation
cromulus
force-pushed
the
mandrill-ingress-tokenfix
branch
from
2a821d7 to
cfc3a07
Compare
| @@ -10,6 +10,13 @@ class ActionMailbox::Ingresses::Mandrill::InboundEmailsControllerTest < ActionDi | |||
| @events = JSON.generate([{ event: "inbound", msg: { raw_msg: file_fixture("../files/welcome.eml").read } }]) | |||
| end | |||
|
|
|||
| test "verifying existence of mandrill inbound route" do | |||
| # the first request has a default INGRESS_API_TOKEN of "test-webhook" | |||
| get rails_mandrill_inbound_emails_url, | |||
There was a problem hiding this comment.
This should be head, right?
My reading of the documentation is that the HEAD request won't be signed at all - it's only the fallback POST that uses the "test-webhook" key.
Would you be able to verify this behaviour with a real Mandrill account?
There was a problem hiding this comment.
Indeed, looks like it. I'll make the change.
It also appears as if some of the tests in railties break as a result of this change:
railties/test/commands/routes_test.rb:166 railties/test/commands/routes_test.rb:202 activemodel/test/application/rake/routes_test.rb:20
Should I update those tests so that they pass?
There are other tests that fail and I can't quite figure out why...
Should I continue to attempt to debug them?
There was a problem hiding this comment.
Yes, please do update those tests to reflect the new route added here. Ideally those tests would be more resilient to incidental changes... but the easiest path here is to play along 🙂
The other failures are against the latest nightly snapshot of Ruby, and won't cause the build to fail on their own - you can see them also failing on the latest (passing) master build here:
There was a problem hiding this comment.
Updated the tests. I think all of the tests that this change broke are now passing. Lets see what the buildkite comes back with. Regarding the conflicts below, I assume the maintainers can fix those problems?
There was a problem hiding this comment.
Looks like the tests are indeed passing now! ⭐️
We can fix the conflicts when merging if necessary, but if you're able to, rebasing your branch and fixing them yourself would help us out:
https://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#update-your-branch
cromulus
force-pushed
the
mandrill-ingress-tokenfix
branch
5 times, most recently
from
8b1b460 to
7518f04
Compare
actionmailbox/test/controllers/ingresses/mandrill/inbound_emails_controller_test.rb
Outdated
Show resolved
Hide resolved
cromulus
force-pushed
the
mandrill-ingress-tokenfix
branch
4 times, most recently
from
8c9032b to
3d4e0e4
Compare
|
@eugeneius |
|
I forgot to ask earlier: could you add an entry to the changelog? |
|
Changelog updated. Thank you for all of your help with this! |
Mandrill's Inbound API checks to see if a URL exists before it creates the webhook. It sends a HEAD request, to which we now return a 200 OK response to indicate that the route exists. Now we can generate inbound API calls with ease on Mandrill, without having to shuffle around tokens in production. Fixes rails#37609.
eugeneius
force-pushed
the
mandrill-ingress-tokenfix
branch
from
b82c6f4 to
fce29be
Compare
|
I tweaked a few things on your branch and squashed to one commit again. This now looks good to me, but I'd like to see if George has any feedback before merging. |
|
Thanks! |
Adding a route for Mandrill's url check.
|
Thanks for helping out! I'm thrilled to be able to contribute even in this small way. |
Summary
Mandrill's Inbound API checks to see if a URL exists before it creates the web hook. It sends a HEAD request to "/mandrill/inbound_emails", which rails converts into a GET request. This simply returns 200, and the word 'OK'.
Now we can generate inbound API calls with ease on Mandrill, without having to shuffle around tokens.
Fixes #37609
Other Information
CHANGELOG
Simplifies the creation of Mandrill Inbound API urls, no longer have to set a temporary "test-webhook" token.