Avoid overly-general rescue in callback controller #1530
Conversation
| @@ -35,13 +35,13 @@ class CallbackControllerTest < ActionController::TestCase | |||
| end | |||
|
|
|||
| test "#callback flashes error when omniauth is not present" do | |||
| get :callback, params: { shop: "shop" } | |||
There was a problem hiding this comment.
The rescue meant these tests were passing, but for the wrong reason. Without the rescue, they failed with:
Error:
ShopifyApp::CallbackControllerTest#test_#callback_flashes_error_in_Spanish:
ArgumentError: missing keywords: :code, :timestamp, :state, :host, :hmac
test/controllers/callback_controller_test.rb:44:in `block in <class:CallbackControllerTest>'
andyw8
force-pushed
the
andyw8/avoid-overly-general-rescue-in-callback-controller
branch
from
5692966 to
7b34a9a
Compare
| rescue | ||
| return respond_with_error | ||
| rescue => e | ||
| if e.class.module_parent == ShopifyAPI::Errors |
There was a problem hiding this comment.
As there is no common base for the error classes, this seemed the cleanest way. (It's from ActiveSupport).
There was a problem hiding this comment.
I like this approach!
I guess the other tricky consideration would be, what if it is an error from the gem?.
There was a problem hiding this comment.
I don't think that's possible here - the code within the rescued block only calls out to ShopifyAPI, and the ShopifyAPI gem has no knowledge of ShopifyApp.
|
I think we should get the logging in place before doing this. |
|
I plan to change this to emit a deprecation warning instead of raising. |
andyw8
force-pushed
the
andyw8/avoid-overly-general-rescue-in-callback-controller
branch
from
7b34a9a to
a332f41
Compare
andyw8
force-pushed
the
andyw8/avoid-overly-general-rescue-in-callback-controller
branch
from
7c84257 to
0157633
Compare
andyw8
force-pushed
the
andyw8/avoid-overly-general-rescue-in-callback-controller
branch
from
0157633 to
6eaaa41
Compare
|
@nelsonwittwer ready for a final review. |
|
/shipit |
andyw8
deleted the
andyw8/avoid-overly-general-rescue-in-callback-controller
branch
* Avoid overly-general rescue in callback controller * Deprecation error * Add tests * Update CHANGELOG Co-authored-by: Andy Waite <[email protected]>
Closes #1527
Still to test, just looking for feedback on the approach.