Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error when incompatible controller concerns are detected #1809

Merged
merged 5 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ Unreleased
* ⚠️ [Breaking] Bumps minimum supported Ruby version to 3.0. Bumps `shopify_api` to 14.0 [1801](https://github.com/Shopify/shopify_app/pull/1801)
* ⚠️ [Breaking] Removes deprecated controller concerns that were renamed in `v21.10.0`. [1805](https://github.com/Shopify/shopify_app/pull/1805)
* ⚠️ [Breaking] Removes deprecated `ScripttagManager`. We realize there was communication error in our logging where we logged future deprecation instead of our inteded removal. Since we have been logging that for 2 years we felt we'd move forward with the removal instead pushing this off until the next major release. [1806](https://github.com/Shopify/shopify_app/pull/1806)
* ⚠️ [Breaking] Thows an error if a controller includes incompatible concerns (LoginProtection/EnsureInstalled) [1809](https://github.com/Shopify/shopify_app/pull/1809)
* ⚠️ [Breaking] No longer rescues non-shopify API errors during OAuth
callback [1807](https://github.com/Shopify/shopify_app/pull/1807)
* Make type param for webhooks route optional. This will fix a bug with CLI initiated webhooks.[1786](https://github.com/Shopify/shopify_app/pull/1786)
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/concerns/shopify_app/ensure_installed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ module EnsureInstalled
if defined?(ShopifyApp::LoginProtection) && ancestors.include?(ShopifyApp::LoginProtection)
message = <<~EOS
We detected the use of incompatible concerns (EnsureInstalled and LoginProtection) in #{name},
which may lead to unpredictable behavior. In a future release of this library this will raise an error.
which leads to unpredictable behavior. You cannot include both concerns in the same controller.
EOS

ShopifyApp::Logger.deprecated(message, "22.0.0")
raise message
end

before_action :check_shop_domain
Expand Down
11 changes: 5 additions & 6 deletions lib/shopify_app/controller_concerns/login_protection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@ module LoginProtection
include ShopifyApp::SanitizedParams

included do
if defined?(ShopifyApp::RequireKnownShop) &&
defined?(ShopifyApp::EnsureInstalled) &&
ancestors.include?(ShopifyApp::RequireKnownShop || ShopifyApp::EnsureInstalled)
if defined?(ShopifyApp::EnsureInstalled) &&
ancestors.include?(ShopifyApp::EnsureInstalled)
message = <<~EOS
We detected the use of incompatible concerns (RequireKnownShop/EnsureInstalled and LoginProtection) in #{name},
which may lead to unpredictable behavior. In a future release of this library this will raise an error.
We detected the use of incompatible concerns (EnsureInstalled and LoginProtection) in #{name},
which leads to unpredictable behavior. You cannot include both concerns in the same controller.
EOS
ShopifyApp::Logger.deprecated(message, "22.0.0")
raise message
end

rescue_from ShopifyAPI::Errors::HttpResponseError, with: :handle_http_error
Expand Down
31 changes: 12 additions & 19 deletions test/controllers/concerns/ensure_installed_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,30 +139,23 @@ def index
get :index, params: { shop: "shop1.myshopify.com" }
end

test "detects incompatible controller concerns" do
version = "22.0.0"
ShopifyApp::Logger.expects(:deprecated).with(regexp_matches(/incompatible concerns/), version)
ShopifyApp::Logger.stubs(:deprecated).with("Itp will be removed in an upcoming version", "22.0.0")

Class.new(ApplicationController) do
include ShopifyApp::LoginProtection
include ShopifyApp::EnsureInstalled
test "detects incompatible controller concerns and raises an error" do
assert_raise do
Class.new(ApplicationController) do
include ShopifyApp::LoginProtection
include ShopifyApp::EnsureInstalled
end
end

ShopifyApp::Logger.expects(:deprecated).with(regexp_matches(/incompatible concerns/), version)
Class.new(ApplicationController) do
include ShopifyApp::EnsureHasSession # since this indirectly includes LoginProtection
include ShopifyApp::EnsureInstalled
assert_raise do
Class.new(ApplicationController) do
include ShopifyApp::EnsureHasSession # since this indirectly includes LoginProtection
include ShopifyApp::EnsureInstalled
end
end

ShopifyApp::Logger.expects(:deprecated).with(regexp_matches(/incompatible concerns/), version)
authenticated_controller = Class.new(ApplicationController) do
include ShopifyApp::EnsureHasSession
end
Class.new(authenticated_controller) do
Class.new(ApplicationController) do
include ShopifyApp::EnsureInstalled
end

assert_within_deprecation_schedule(version)
end
end
15 changes: 7 additions & 8 deletions test/shopify_app/controller_concerns/login_protection_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -555,18 +555,17 @@ class LoginProtectionControllerTest < ActionController::TestCase
end
end

test "detects incompatible controller concerns" do
version = "22.0.0"

ShopifyApp::Logger.expects(:deprecated).with(regexp_matches(/incompatible concerns/), version)
ShopifyApp::Logger.stubs(:deprecated).with("Itp will be removed in an upcoming version", version)
test "detects incompatible controller concerns and raises an error" do
assert_raise do
Class.new(ApplicationController) do
include ShopifyApp::EnsureInstalled
include ShopifyApp::LoginProtection
end
end

Class.new(ApplicationController) do
include ShopifyApp::LoginProtection
include ShopifyApp::EnsureInstalled
end

assert_within_deprecation_schedule(version)
end

private
Expand Down
Loading