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

Respect current ability in users controllers #3732

Merged
Show file tree
Hide file tree
Changes from 1 commit
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
9 changes: 8 additions & 1 deletion backend/app/controllers/spree/admin/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ def user_params
attributes += [{ spree_role_ids: [] }]
end

if can? :manage, Spree::StockLocation
attributes += [{ stock_location_ids: [] }]
end

unless can? :update_password, @user
attributes -= [:password, :password_confirmation]
end
Expand Down Expand Up @@ -178,7 +182,10 @@ def set_roles
end

def set_stock_locations
@user.stock_locations = Spree::StockLocation.where(id: (params[:user][:stock_location_ids] || []))
if user_params[:stock_location_ids]
@user.stock_locations =
Spree::StockLocation.accessible_by(current_ability).where(id: user_params[:stock_location_ids])
end
Copy link
Member

Choose a reason for hiding this comment

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

I think the behavior was slightly changed. The old code always set the user stock locations, also when the parameter was not present, in that case by stock locations to an empty array. This does not happen anymore here. Is this change wanted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question. I took this decision because with these changes admins that can't manage stock locations would remove user's stock locations since the params wouldn't be there.

end
end
end
Expand Down
92 changes: 82 additions & 10 deletions backend/spec/controllers/spree/admin/users_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,45 @@ def user
expect(user.reload.bill_address.city).to eq('New York')
end

it "can set stock locations" do
location = Spree::StockLocation.create(name: "my_location")
location_2 = Spree::StockLocation.create(name: "my_location_2")
post :create, params: { user: { stock_location_ids: [location.id, location_2.id] } }
expect(user.stock_locations).to match_array([location, location_2])
context "when the user can manage stock locations" do
stub_authorization! do |_user|
can :manage, Spree.user_class
can :manage, Spree::StockLocation
end

it "can create user with stock locations" do
location = Spree::StockLocation.create(name: "my_location")
post :create, params: { user: { stock_location_ids: [location.id] } }
expect(user.stock_locations).to eq([location])
end
end

context "when the user cannot manage stock locations" do
stub_authorization! do |_user|
can :manage, Spree.user_class
cannot :manage, Spree::StockLocation
end

it "cannot assign users stock locations" do
location = Spree::StockLocation.create(name: "my_location")
post :create, params: { user: { stock_location_ids: [location.id] } }
expect(user.stock_locations).to eq([])
end
end

context "when the user can manage only some stock locations" do
stub_authorization! do |_user|
can :manage, Spree.user_class
can :manage, Spree::StockLocation
cannot :manage, Spree::StockLocation, name: 'not_accessible_location'
end

it "can assign accessible stock locations to user" do
location1 = Spree::StockLocation.create(name: "accessible_location")
location2 = Spree::StockLocation.create(name: "not_accessible_location")
post :create, params: { user: { stock_location_ids: [location1.id, location2.id] } }
expect(user.stock_locations).to eq([location1])
end
end
end

Expand Down Expand Up @@ -341,11 +375,49 @@ def user
expect(user.reload.bill_address.city).to eq('New York')
end

it "can set stock locations" do
location = Spree::StockLocation.create(name: "my_location")
location_2 = Spree::StockLocation.create(name: "my_location_2")
post :update, params: { id: user.id, user: { stock_location_ids: [location.id, location_2.id] } }
expect(user.stock_locations).to match_array([location, location_2])
context "when the user can manage stock locations" do
stub_authorization! do |_user|
can :manage, Spree.user_class
can :manage, Spree::StockLocation
end

it "can update user stock locations" do
location1 = Spree::StockLocation.create(name: "my_location")
location2 = Spree::StockLocation.create(name: "my_location2")
user.stock_locations = [location1]
put :update, params: { id: user.id, user: { stock_location_ids: [location2.id] } }
expect(user.reload.stock_locations).to eq([location2])
end
end

context "when the user cannot manage stock locations" do
stub_authorization! do |_user|
can :manage, Spree.user_class
cannot :manage, Spree::StockLocation
end

it "cannot update users stock locations" do
location1 = Spree::StockLocation.create(name: "my_location")
location2 = Spree::StockLocation.create(name: "my_location2")
user.stock_locations = [location1]
put :update, params: { id: user.id, user: { stock_location_ids: [location2.id] } }
expect(user.reload.stock_locations).to eq([location1])
end
end

context "when the user can manage only some stock locations" do
stub_authorization! do |_user|
can :manage, Spree.user_class
can :manage, Spree::StockLocation
cannot :manage, Spree::StockLocation, name: 'not_accessible_location'
end

it "can update accessible stock locations to user" do
location1 = Spree::StockLocation.create(name: "accessible_location")
location2 = Spree::StockLocation.create(name: "not_accessible_location")
put :update, params: { id: user.id, user: { stock_location_ids: [location1.id, location2.id] } }
expect(user.reload.stock_locations).to eq([location1])
end
end
end

Expand Down