[Checkout] Shipping fees update, remove order callback #13023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`Spree::OrderContents#update_or_create` is used to update the cart when on the /shop page. If you start an order and proceed to the "Order summary" step, and then decide to update your order by using the shop link next to the cart, such update wouldn't update the shipment. This result in the order page in the backoffice displaying the wrong data, and more importantly, in the stock not being updated. So now we ensure shipment will be updated, which result in the checkout flow being restarted, thus making sure the shipment is updated.
It should be handled in the controller, it's currently handled in `Spree::OrderContents#remove`. As long as we don't manually remove line item from an order we should be good. Currently it gets trigerred each time the order is saved, which seems to happen mutiple time when we finalize an order. It's a bit useless to recalculated the fees over and over Context, it was added here : openfoodfoundation@217eda8
And update related specs
User Order::Contents#update_item to update line item on an order, it ensures the order is properly updated
rioug
added
the
user facing changes
rioug
changed the title
mkllnk
approved these changes
Dec 10, 2024
spec/system/admin/orders_spec.rb
Outdated
Comment on lines
455
to
456
| order2.contents.update_item(Spree::LineItem.where(order_id: order2.id).first, | ||
| { quantity: 5 }) |
There was a problem hiding this comment.
Suggested change
| order2.contents.update_item(Spree::LineItem.where(order_id: order2.id).first, | |
| { quantity: 5 }) | |
| order2.contents.update_item(Spree::LineItem.find_by(order: order2), { quantity: 5 }) |
dacook
approved these changes
Dec 16, 2024
Comment on lines
+1250
to
+1251
| item_num = order.line_items.sum(&:quantity) | ||
| expect(order.reload.adjustment_total).to eq(item_num * shipping_fee) |
There was a problem hiding this comment.
Because we're using test data, we should be able to specify the actual amounts here, which would be less error-prone than using a calculation. But let's not bother updating now..
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What? Why?
Now that checkout workflow is restarted when an order is updated, it fixes the issue with shipping fee calculation. That said, while investigating the issue, I realised shipment fees were recalculated multiple times when completing an order , which seemed unnecessary. I tracked down the issue to a callback ( 😮 surprise! 😮 surprise! ) :
openfoodnetwork/app/models/spree/order.rb
Line 109 in ee2a6bf
Given that we have https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/models/spree/order_contents.rb that should be used to managed adding/updating/removing an order's line item.
Spree::OrderContentsalready handle updating shipment and shipment fees, so now thebefore_savecallback becomes redundant.Drawback : shipment fees won't be updated is someone directly update a line item on an order. There isn't anyway to prevent that (as far as I know), we (as the dev team) need to enforce the use
Spree::OrderContentsfor any line item update.What should we test?
As an enterprise manager:
As a customer:
--> they should be matching.
Same scenario as above but this time once on step 3 of checkout, use "Edit" link next to "Order Details" to update your order
As an enterprise manager
As an enterprise manager, in the backoffice
--> check the shipping fee is calculated correctly
-- Add or update a line item in the order
--> check the shipping fee is updated correctly
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
#12954 needs to be merged first