From a8c357d71d20c1149ae9d9b1f5c38521811657d7 Mon Sep 17 00:00:00 2001 From: Aaric Pittman Date: Fri, 1 Sep 2023 16:36:02 -0500 Subject: [PATCH 1/4] fix: make regex_to_remove_controller_namespace a class method rather than an instance method --- .../bullet_train/loads_and_authorizes_resource.rb | 8 ++++---- .../bullet_train/loads_and_authorizes_resource_test.rb | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb b/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb index e4059a3b1..14e58bd4f 100644 --- a/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb +++ b/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb @@ -15,6 +15,10 @@ def model_namespace_from_controller_namespace namespace end + def regex_to_remove_controller_namespace + raise "This is a template method that needs to be implemented by controllers including LoadsAndAuthorizesResource." + end + # this is one of the few pieces of 'magical' functionality that bullet train implements # for you in your controllers beyond that is provided by the underlying gems that we've # tied together. we've taken the liberty of doing this because it's heavily based on @@ -202,10 +206,6 @@ def account_load_and_authorize_resource(model, options, old_options = {}) end end - def regex_to_remove_controller_namespace - raise "This is a template method that needs to be implemented by controllers including LoadsAndAuthorizesResource." - end - def load_team # Not all objects that need to be authorized belong to a team, # so we give @team a nil value if no association is found. diff --git a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb index 22d926acf..5c3376e0c 100644 --- a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb +++ b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb @@ -13,11 +13,11 @@ class TestClass assert TestClass.respond_to?(:account_load_and_authorize_resource) end - test "it defines #load_team" do - assert TestClass.new.respond_to?(:load_team) + test "it defines .regex_to_remove_controller_namespace" do + assert TestClass.respond_to?(:regex_to_remove_controller_namespace) end - test "it defines #regex_to_remove_controller_namespace" do - assert TestClass.new.respond_to?(:regex_to_remove_controller_namespace) + test "it defines #load_team" do + assert TestClass.new.respond_to?(:load_team) end end From 747c0a49242e58d56a22410e1d7a90ebfe50f3c7 Mon Sep 17 00:00:00 2001 From: Aaric Pittman Date: Fri, 1 Sep 2023 17:17:52 -0500 Subject: [PATCH 2/4] refactor: BulletTrain::LoadsAndAuthorizesResource.model_namespace_from_controller_namespace and add a test --- .../loads_and_authorizes_resource.rb | 15 +++++---------- .../loads_and_authorizes_resource_test.rb | 18 ++++++++++++++++-- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb b/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb index 14e58bd4f..8b9477562 100644 --- a/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb +++ b/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb @@ -2,17 +2,12 @@ module BulletTrain::LoadsAndAuthorizesResource extend ActiveSupport::Concern class_methods do + # Returns an array of module names based on the classes namespace minus regex_to_remove_controller_namespace def model_namespace_from_controller_namespace - controller_class_name = - if regex_to_remove_controller_namespace - name.gsub(regex_to_remove_controller_namespace, "") - else - name - end - namespace = controller_class_name.split("::") - # Remove "::ThingsController" - namespace.pop - namespace + name + .gsub(regex_to_remove_controller_namespace || //, "") + .split("::") + .slice(0...-1) # drops actual class name end def regex_to_remove_controller_namespace diff --git a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb index 5c3376e0c..47106c411 100644 --- a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb +++ b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb @@ -3,10 +3,24 @@ class BulletTrain::LoadsAndAuthorizeResourceTest < ActiveSupport::TestCase class TestClass include BulletTrain::LoadsAndAuthorizesResource + + def self.regex_to_remove_controller_namespace + end + end + + module Users + class TestClass + include BulletTrain::LoadsAndAuthorizesResource + + def self.regex_to_remove_controller_namespace + /^BulletTrain::LoadsAndAuthorizeResourceTest::/ + end + end end - test "it defines .model_namespace_from_controller_namespace" do - assert TestClass.respond_to?(:model_namespace_from_controller_namespace) + test "model_namespace_from_controller_namespace returns an array of modules names based on the classes namespace minus regex_to_remove_controller_namespace" do + assert_equal ["BulletTrain", "LoadsAndAuthorizeResourceTest"], TestClass.model_namespace_from_controller_namespace + assert_equal ["Users"], Users::TestClass.model_namespace_from_controller_namespace end test "it defines .account_load_and_authorize_resource" do From 2ba24d28f9d5fadf1479c9bf269175d4143e19f1 Mon Sep 17 00:00:00 2001 From: Aaric Pittman Date: Fri, 1 Sep 2023 18:22:38 -0500 Subject: [PATCH 3/4] refactor: BulletTrain::LoadsAndAuthorizesResource#load_team and add tests --- .../loads_and_authorizes_resource.rb | 20 +++-- .../loads_and_authorizes_resource_test.rb | 80 +++++++++++++++++-- .../loads_and_authorizes_resource_test.rb | 2 +- .../test/test_helper.rb | 2 + 4 files changed, 85 insertions(+), 19 deletions(-) diff --git a/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb b/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb index 8b9477562..c2d9a2d1e 100644 --- a/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb +++ b/bullet_train-super_load_and_authorize_resource/app/controllers/concerns/bullet_train/loads_and_authorizes_resource.rb @@ -7,10 +7,12 @@ def model_namespace_from_controller_namespace name .gsub(regex_to_remove_controller_namespace || //, "") .split("::") - .slice(0...-1) # drops actual class name + .tap(&:pop) # drops actual class name end def regex_to_remove_controller_namespace + return super if defined?(super) + raise "This is a template method that needs to be implemented by controllers including LoadsAndAuthorizesResource." end @@ -202,17 +204,13 @@ def account_load_and_authorize_resource(model, options, old_options = {}) end def load_team - # Not all objects that need to be authorized belong to a team, - # so we give @team a nil value if no association is found. - begin - # Sometimes `@team` has already been populated by earlier `before_action` steps. - @team ||= @child_object&.team || @parent_object&.team - rescue NoMethodError - @team = nil - end + @team ||= @child_object&.try(:team) || @parent_object&.try(:team) - # Update current attributes. - Current.team = @team + return unless @team + + if defined?(Current) && Current.respond_to?(:team=) + Current.team = @team + end # If the currently loaded team is saved to the database, make that the user's new current team. if @team.try(:persisted?) diff --git a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb index 47106c411..af5cf0945 100644 --- a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb +++ b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/bullet_train/loads_and_authorizes_resource_test.rb @@ -1,15 +1,22 @@ require "test_helper" +# Has to be required here because these tests get run in the context of another application in another repo. +require "minitest/mock" class BulletTrain::LoadsAndAuthorizeResourceTest < ActiveSupport::TestCase - class TestClass + class TestControllerClass < ActionController::Base include BulletTrain::LoadsAndAuthorizesResource + attr_accessor :child_object, :current_user, :parent_object, :team + def self.regex_to_remove_controller_namespace end + + def can?(*args) + end end module Users - class TestClass + class TestControllerClass < ActionController::Base include BulletTrain::LoadsAndAuthorizesResource def self.regex_to_remove_controller_namespace @@ -19,19 +26,78 @@ def self.regex_to_remove_controller_namespace end test "model_namespace_from_controller_namespace returns an array of modules names based on the classes namespace minus regex_to_remove_controller_namespace" do - assert_equal ["BulletTrain", "LoadsAndAuthorizeResourceTest"], TestClass.model_namespace_from_controller_namespace - assert_equal ["Users"], Users::TestClass.model_namespace_from_controller_namespace + assert_equal ["BulletTrain", "LoadsAndAuthorizeResourceTest"], TestControllerClass.model_namespace_from_controller_namespace + assert_equal ["Users"], Users::TestControllerClass.model_namespace_from_controller_namespace end test "it defines .account_load_and_authorize_resource" do - assert TestClass.respond_to?(:account_load_and_authorize_resource) + assert_respond_to TestControllerClass, :account_load_and_authorize_resource end test "it defines .regex_to_remove_controller_namespace" do - assert TestClass.respond_to?(:regex_to_remove_controller_namespace) + assert_respond_to TestControllerClass, :regex_to_remove_controller_namespace end test "it defines #load_team" do - assert TestClass.new.respond_to?(:load_team) + assert_respond_to TestControllerClass.new, :load_team + end + + test "#load_team does not set @team if child_object and parent_object are nil" do + subject = TestControllerClass.new + subject.child_object = nil + subject.parent_object = nil + + assert_nil subject.instance_variable_get(:@team) + end + + test "#load_team sets @team" do + team = OpenStruct.new + subject = TestControllerClass.new + subject.child_object = OpenStruct.new(team: team) + + subject.load_team + + assert_equal team, subject.instance_variable_get(:@team) + end + + test "#load_team sets Current attributes if defined" do + unless defined?(::Current) + temp_current_class = Class.new(ActiveSupport::CurrentAttributes) do + attribute :team + end + Object.const_set(:Current, temp_current_class) + end + + team = OpenStruct.new + subject = TestControllerClass.new + subject.child_object = OpenStruct.new(team: team) + + team.stub(:try, nil) do + subject.load_team + end + + assert_equal team, Current.team + + if temp_current_class + Object.send(:remove_const, :Current) + end + end + + test "#load_team updates current_user's current_team if persisted and can" do + current_user = Minitest::Mock.new + team = OpenStruct.new(id: 1) + subject = TestControllerClass.new + subject.child_object = OpenStruct.new(team: team) + subject.current_user = current_user + + current_user.expect(:update_column, nil, [:current_team_id, team.id]) + + team.stub(:try, true) do + subject.stub(:can?, true) do + subject.load_team + end + end + + current_user.verify end end diff --git a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/loads_and_authorizes_resource_test.rb b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/loads_and_authorizes_resource_test.rb index 8255080ae..aa7503632 100644 --- a/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/loads_and_authorizes_resource_test.rb +++ b/bullet_train-super_load_and_authorize_resource/test/controllers/concerns/loads_and_authorizes_resource_test.rb @@ -7,6 +7,6 @@ class TestClass test "includes BulletTrain::LoadsAndAuthorizesResource" do assert TestClass.ancestors.include? BulletTrain::LoadsAndAuthorizesResource - assert TestClass.respond_to?(:account_load_and_authorize_resource) + assert_respond_to TestClass, :account_load_and_authorize_resource end end diff --git a/bullet_train-super_load_and_authorize_resource/test/test_helper.rb b/bullet_train-super_load_and_authorize_resource/test/test_helper.rb index 44d56a045..1224df13c 100644 --- a/bullet_train-super_load_and_authorize_resource/test/test_helper.rb +++ b/bullet_train-super_load_and_authorize_resource/test/test_helper.rb @@ -4,6 +4,8 @@ require_relative "../test/dummy/config/environment" ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)] require "rails/test_help" +require "minitest/mock" +require "pry" # Load fixtures from the engine if ActiveSupport::TestCase.respond_to?(:fixture_path=) From 7cf6a2090aa777984238b288479a364048b9a940 Mon Sep 17 00:00:00 2001 From: Jeremy Green Date: Tue, 3 Oct 2023 10:49:08 -0500 Subject: [PATCH 4/4] Update bullet_train-super_load_and_authorize_resource/test/test_helper.rb --- .../test/test_helper.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/bullet_train-super_load_and_authorize_resource/test/test_helper.rb b/bullet_train-super_load_and_authorize_resource/test/test_helper.rb index 1224df13c..a9322fbd1 100644 --- a/bullet_train-super_load_and_authorize_resource/test/test_helper.rb +++ b/bullet_train-super_load_and_authorize_resource/test/test_helper.rb @@ -5,7 +5,6 @@ ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)] require "rails/test_help" require "minitest/mock" -require "pry" # Load fixtures from the engine if ActiveSupport::TestCase.respond_to?(:fixture_path=)