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

Update CanCan permissions on adjustments #7452

Merged

Conversation

Matt-Yorkley
Copy link
Contributor

What? Why?

Closes #7341

The previous permissions assumed that an adjustment's "adjustable" could only be only line items or orders, and that's no longer true. It's now commonly a shipment or a payment as well.

What should we test?

Shipping and Payment fee adjustments can be deleted by non-superadmin enterprise users.

Release notes

Fixed a permissions issue when deleting shipping or payment fee adjustments

Changelog Category: Technical changes

The previous permissions assumed that an adjustment's "adjustable" could only be only line items or orders, and that's no longer true. It's now commonly a shipment or a payment as well.
@Matt-Yorkley Matt-Yorkley self-assigned this Apr 21, 2021
@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #7452 (ecd433b) into master (7da0dc0) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7452      +/-   ##
==========================================
+ Coverage   93.08%   93.15%   +0.06%     
==========================================
  Files         631      630       -1     
  Lines       18097    18088       -9     
==========================================
+ Hits        16846    16849       +3     
+ Misses       1251     1239      -12     
Impacted Files Coverage Δ
app/models/spree/ability.rb 96.29% <100.00%> (+3.55%) ⬆️
app/controllers/spree/checkout_controller.rb
app/serializers/api/variant_serializer.rb 100.00% <0.00%> (+4.00%) ⬆️
app/controllers/spree/paypal_controller.rb 95.83% <0.00%> (+5.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7da0dc0...ecd433b. Read the comment docs.

Copy link
Contributor

@andrewpbrett andrewpbrett left a comment

Choose a reason for hiding this comment

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

Nice simplification to boot 👍

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

@filipefurtad0
Copy link
Contributor

Awesome @Matt-Yorkley ,

This works now (sorry for the hectic cursor :-D):
Peek 2021-04-22 16-09

Good to go!

PS: While investigating the issue I was working on a feature spec (I wanted to make it fail) to delete adjustments from shipping and transaction fees. Between these types of fees the only difference I spotted in the DB was adjustable_type which is "Spree::Shipment", "Spree::Payment" and "Spree::Order", for shipping, transaction or enterprise fees, respectively. Do you think it would bring value to add a spec deleting such fees, on the feature spec level?
I see you already have a spec on controllers, so maybe we don't need this. What do you think?

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Apr 22, 2021

adjustable_type which is "Spree::Shipment", "Spree::Payment" and "Spree::Order", for shipping, transaction or enterprise fees, respectively. Do you think it would bring value to add a spec deleting such fees, on the feature spec level?

Hmm... I'm not sure extra feature specs for each case would have much value here. The change means we don't check the adjustable_type, so any adjustment can be edited if a user has permission to edit the order it's attached to, which is a lot simpler (and also correct 😅).

@Matt-Yorkley Matt-Yorkley merged commit bef3f8c into openfoodfoundation:master Apr 22, 2021
@filipefurtad0
Copy link
Contributor

Ah cool, that makes sense. Thanks for feedback 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't delete (some) adjustments - Error 401 Unauthorized
4 participants