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

[Rails 6.0] Fix order adjustments spec #7589

Merged

Conversation

andrewpbrett
Copy link
Contributor

What? Why?

Paging @Matt-Yorkley to take a look at this... I'm a bit puzzled as to what would have changed in Rails 6 to flip this spec to red. Let's see if the fix works on the current master.

What should we test?

Green build

Release notes

Updated an Order spec to prepare for Rails 6

Changelog Category: Technical changes

Dependencies

Documentation updates

@andrewpbrett andrewpbrett requested a review from Matt-Yorkley May 8, 2021 22:06
@codecov
Copy link

codecov bot commented May 8, 2021

Codecov Report

Merging #7589 (231f01d) into master (2910cbf) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7589   +/-   ##
=======================================
  Coverage   93.20%   93.20%           
=======================================
  Files         635      635           
  Lines       18136    18136           
=======================================
  Hits        16903    16903           
  Misses       1233     1233           

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 2910cbf...231f01d. Read the comment docs.

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

@Matt-Yorkley
Copy link
Contributor

What was the error? It's difficult to review without the context.

payment_fee_adjustment.finalize!
shipping_fee_adjustment.finalize!
order.all_adjustments.payment_fee.each(&:finalize!)
order.shipment_adjustments.each(&:finalize!)
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley May 9, 2021

Choose a reason for hiding this comment

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

We could maybe just do order.all_adjustments.each(&:finalize!) here for simplicity? Not a big deal though.

@andrewpbrett
Copy link
Contributor Author

andrewpbrett commented May 9, 2021

@Matt-Yorkley you can see it in this build: https://github.com/openfoodfoundation/openfoodnetwork/runs/2536112257?check_suite_focus=true

2) Spree::Order a completed order with shipping and transaction fees removing line_items when finalized fee adjustments exist on the order does not attempt to update such adjustments
     Failure/Error: expect(order.adjustment_total).to eq expected_fees
     
       expected: 16
            got: 0.13e2
     
       (compared using ==)
     # ./spec/models/spree/order_spec.rb:1158:in `block (5 levels) in <top (required)>'

From what I could tell the math it's doing is 2*5 + 1*3, when it should be doing 2*(5+3) (see line 1118 of the spec file).

@Matt-Yorkley
Copy link
Contributor

Alright, cool. Seems like it's an issue with the test setup as opposed to the code itself 👌

Copy link
Contributor

@Matt-Yorkley Matt-Yorkley left a comment

Choose a reason for hiding this comment

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

👍

@andrewpbrett andrewpbrett force-pushed the order-adjustments-spec branch from d9c9043 to 231f01d Compare May 9, 2021 17:09
@andrewpbrett andrewpbrett merged commit cbefa5f into openfoodfoundation:master May 9, 2021
@andrewpbrett andrewpbrett deleted the order-adjustments-spec branch May 10, 2021 15:44
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.

3 participants