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.

Sign up for GitHub

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

Merge alias actions when merging abilities #538

Merged
merged 3 commits into from
Jan 26, 2019
Merged

Merge alias actions when merging abilities #538

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b41eb95
Merge alias actions when merging abilities
Oct 22, 2018
5271c27
Merge branch 'develop' into merge_alias_actions_when_merging_abilities
Jcambass Oct 24, 2018
de9bfe6
Merge branch 'develop' into merge_alias_actions_when_merging_abilities
coorasse Jan 26, 2019
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
28 changes: 27 additions & 1 deletion lib/cancan/ability.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,13 @@ def has_raw_sql?(action, subject)
relevant_rules(action, subject).any?(&:only_raw_sql?)
end

# Copies all rules of the given +CanCan::Ability+ and adds them to +self+.
# Copies all rules and aliased actions of the given +CanCan::Ability+ and adds them to +self+.
# class ReadAbility
# include CanCan::Ability
#
# def initialize
# can :read, User
# alias_action :show, :index, to: :see
# end
# end
#
Expand All @@ -216,18 +217,43 @@ def has_raw_sql?(action, subject)
#
# def initialize
# can :edit, User
# alias_action :create, :update, to: :modify
# end
# end
#
# read_ability = ReadAbility.new
# read_ability.can? :edit, User.new #=> false
# read_ability.merge(WritingAbility.new)
# read_ability.can? :edit, User.new #=> true
# read_ability.aliased_actions #=> [:see => [:show, :index], :modify => [:create, :update]]
#
# If there are collisions when merging the +aliased_actions+, the actions on +self+ will be
# overwritten.
#
# class ReadAbility
# include CanCan::Ability
#
# def initialize
# alias_action :show, :index, to: :see
# end
# end
#
# class ShowAbility
# include CanCan::Ability
#
# def initialize
# alias_action :show, to: :see
# end
# end
#
# read_ability = ReadAbility.new
# read_ability.merge(ShowAbility)
# read_ability.aliased_actions #=> [:see => [:show]]
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I'd have expected them to be see: [:show, :index] since they are merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coorasse You're right merge would kinda indicate that, but would that make sense?

If we merge two abilities we expect to gain the rules from the ability which was passed. But it wouldn't really make sense to combine the actions defined for an alias since it could change the "meaning" of that alias.

Even tho it seems like the more intuitive approach, I think it would lead to some surprises. I choose this approach because it's "safer" and doesn't change the rules defined for an alias.

def merge(ability)
ability.rules.each do |rule|
add_rule(rule.dup)
end
@aliased_actions = aliased_actions.merge(ability.aliased_actions)
self
end

Expand Down
29 changes: 29 additions & 0 deletions spec/cancan/ability_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,35 @@ class Container < Hash
expect(@ability.send(:rules).size).to eq(2)
end

it 'adds the aliased actions from the given ability' do
@ability.alias_action :show, to: :see
(another_ability = double).extend(CanCan::Ability)
another_ability.alias_action :create, :update, to: :manage

@ability.merge(another_ability)
expect(@ability.aliased_actions).to eq(
read: %i[index show],
create: %i[new],
update: %i[edit],
manage: %i[create update],
see: %i[show]
)
end

it 'overwrittes the aliased actions with the value from the given ability' do
@ability.alias_action :show, :index, to: :see
(another_ability = double).extend(CanCan::Ability)
another_ability.alias_action :show, to: :see

@ability.merge(another_ability)
expect(@ability.aliased_actions).to eq(
read: %i[index show],
create: %i[new],
update: %i[edit],
see: %i[show]
)
end

it 'can add an empty ability' do
(another_ability = double).extend(CanCan::Ability)

Expand Down