[Spree Upgrade] Fix order_with_totals_and_distribution factory by updating shipping fees #3299
Conversation
luisramos0
changed the title
…ibution with shipping fee correctly included in its total
…ls_and_distribution by cleaning up duplicate shipping fees
|
ehehe, sorry about that! Actually, I am not sure this PR is ready. We need this last commit to fix orders_spec and it's not good looking (order.updater.update_adjustments...). |
|
The build is looking good. The lines added to the factories are not good but I believe this is related to #2685 (and PR #3250). @sauloperez what shall we do with this? |
|
Moving back to in dev because I'll take a look at it now that I got the hang of adjustments (kind of...). |
* 2-0-stable: (208 commits) Re implement orders ctrl update method so we can change redirection logic and add specs for it Improve readability in admin/orders_spec Add line items adjustments (order.price_adjustments) to the order edit page so that user is aware of adjustments included in the price like for example tax rates Adapt variants auto complete to spree v2 code with shipments Adapt customer search override to spree v2 and fix customer details spec in admin orders spec Fix distributor change spec in admin orders spec Fix admin orders spec non tax adjustments by adapting to new view Fix failing specs due to Spree 2's new order admin page Add missing form tag and OC and shops injectors on order form to make the OC field, the distributor field and the update button work Use Spree routes Fix shop accidentally becoming order coordinator Match date format in spec with import date filter Make in_stock? work for both variants and overrides by moving it from VariantStock to variant_decorator. Add unit test for VariantStock.can_supply? Remove rescue from products_reset_strategy in product import: if setting count_on_hand fails the import will raise a RuntimeError Change product import's product_reset_strategy from depending on the inexistent variant.count_on_hand DB field and instead make individual calls to variant.count_on_hand= defined in VariantStock. Also, added spec to test return value of the reset method: it should return number of updated records. Upgrade views to Spree 2 and apply overrides Updating translations for config/locales/en_US.yml Update name spaces for rake tasks to shorter 'ofn'. Refactor checking no preview image in specs Refactor checking of preview image path in specs ...
If the shipping_rate is not created at the time the `#after_save`'s `#ensure_correct_adjustment` callback is executed its call to `shipping_method.create_adjustment` won't be executed either. As a result, the OrderUpdater wasn't able to see any adjustments related to the shipment causing the order total to be outdated.
|
This is ready again. After doing quite deep spelunking I found out the actual problem was that the shipment adjustments were not created at the time they should. That is why manually triggering the logic from the factories fixed it. For future reference, this is done in https://github.com/openfoodfoundation/spree/blob/f55722b38db7e706a8521c9091a0e00119bb4d20/core/app/models/spree/shipment.rb#L280-L282 which is triggered from https://github.com/openfoodfoundation/spree/blob/f55722b38db7e706a8521c9091a0e00119bb4d20/core/app/models/spree/shipment.rb#L16. As you can see, when saving a shipment it first creates the adjustments and it updates the totals through |
sauloperez
changed the title
|
ah, great, looks good! bu there are lots of broken tests, you have fixed the shipment_with factory but I guess order_with_totals_and_distribution also needs some fix to replace update_shipping_fees |
|
|
|
Oops, I think this introduced a bunch of errors like The tests mentioned in #3113 got fixed though. |
|
@sauloperez will you take care of this? before your change in shipment_with: after your change in shipment_with: I got this in spec/models/spree/order_spec.rb:238 |
* 2-0-stable: (121 commits) Stub default value for other calls to File.exist? Add specs for error cases in shipments_controller_spec Move features/admin/reports/enterprise_fee_summaries_spec to xdescribe (will be fixed as part of spree upgrade phase 2) Add missing translation to fix shipping methods spec Test tagging polymorphism on a payment method Workaround Rails inheritance bug in Spree::Gateway Improve method names in shipments_controller_spec Move ProductImporter spec to xdescribe until all its specs are green (spree upgrade phase 2) Make bulk invoices part of spree upgrade phase 2 Make Api::ShipmentsController#create re-use order.shipment if it exists Improve code and add specs to this controller Decorate spree api shipments controller to scope variants as they are added/removed from shipments Delete all Spree::Admin::LineItemsController customizations as they are no longer used Update spec/controllers/spree/orders_controller_spec.rb Fix broken spec from refactored method Wait for button to disappear before checking flash Use flash matcher in shipping method feature specs Add RSpec matchers for flash messages Do not expect modal open when checking spinner gone Fix layout violation in Spree::Ability decorator Fix description for feature flag example groups ...
|
Among the 3 failing specs I found two in |
|
yes, the build looks good. |
|
I have reviewed now. approved! |
* 2-0-stable: Perform delivery when checking deliveries in specs
What? Why?
Closes #3113
By fixing the totals in the factory, the test sees an order with correct total of 13 (including the shipping fee of 3) and not just 10 (the line item total).
This makes the test payment amount move from 10 to 13 and, because it covers the final amount of 13, capturing the payment just works.
All credit to @mkllnk for finding out the source of the issue here.
What should we test?
spec in 3113 should be green (I have confirmed in the build).