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

Some more refactoring and testing of BulletTrain::LoadsAndAuthorizesResource #516

Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@ 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("::")
.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

# this is one of the few pieces of 'magical' functionality that bullet train implements
Expand Down Expand Up @@ -202,22 +203,14 @@ 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.
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?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've inserted the return unless @team above, we can remove the try here:

Suggested change
if @team.try(:persisted?)
if @team.persisted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not calling try for null safety. I'm calling it because there is nothing in this gem to guarantee that the value of @team responds to persisted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it'd be better to raise the NoMethodError exception with our expected interface to ease debugging, but this code path was already wrapped in try so we can keep it.

Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,103 @@
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 TestControllerClass < ActionController::Base
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"], 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_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
kaspth marked this conversation as resolved.
Show resolved Hide resolved

assert_equal team, Current.team

if temp_current_class
Object.send(:remove_const, :Current)
end
end

test "it defines #regex_to_remove_controller_namespace" do
assert TestClass.new.respond_to?(:regex_to_remove_controller_namespace)
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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
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"

# Load fixtures from the engine
if ActiveSupport::TestCase.respond_to?(:fixture_path=)
Expand Down
Loading