Update line items enterprise fee instead of deleting and recreating #13144
Conversation
Renamed to clear_order_adjustments, it doesn't clear line item adjustment
Also remove unnecessary use of `__send__`
It retrieves all the per item fees associated with an order cycle and create the appropriate Fee Applicator.
We now update or create line item fees instead of deleting them and recreating them. This is to cover the case when a product has been removed from an Order Cycle but we want to keep the fee already applied on existing order. This was an issue only if the existing order got updated after the product was removed.
Spree::Order just delegate Orders::HandleFeesService so there is no point testing fees in the order spec
rioug
force-pushed
the
13100-removed-fee-when-product-removed
branch
from
847db1a to
eb558f0
Compare
This is to be consistent with the current behavior
rioug
force-pushed
the
13100-removed-fee-when-product-removed
branch
from
eb558f0 to
8116ad9
Compare
rioug
added
the
user facing changes
There was a problem hiding this comment.
Great work, thanks for working through this bug. I found it a bit tricky to follow, but it all looks fine 👍
Due to our overly-permissive data structure, we have to add a fair bit of complexity to solve the problem which is sad.
± So to help for future reference could you please add some comments to the new code to explain what it's for?
| # does not clear line item adjustments | ||
| expect do | ||
| described_class.clear_order_adjustments order | ||
| end.to change { order.all_adjustments.count }.by(-2) |
There was a problem hiding this comment.
This doesn't check which adjustments were cleared. Can we expect(order.all_adjustments).to_not include foo1, foo2?
| def create_or_update_line_item_fees! | ||
| order.line_items.includes(:variant).each do |line_item| | ||
| # No fee associated with the line item so we just create them | ||
| next create_line_item_fees!(line_item) if line_item.enterprise_fee_adjustments.blank? |
There was a problem hiding this comment.
Interesting use of next to also execute a command here. It would be a bit clearer to break it up to a couple of lines (inside an if block).
There was a problem hiding this comment.
I guess this means the next command create_or_update_line_item_fee will only ever update, because create has already been handled.
But is this condition needed to skip update_removed_fees? I wonder if it is?
Sorry, I don't think I'm following the whole picture very well.
There was a problem hiding this comment.
next works a bit like return and can have a return value. So if line_item.enterprise_fee_adjustments.blank? is true then the execution of the block is stopped here and the return value of the block is create_line_item_fees!(line_item). The return value is actually ignored because we are in an each block. It would matter in a map block, for example. So this is confusing. I would also suggest to change this.
| next create_line_item_fees!(line_item) if line_item.enterprise_fee_adjustments.blank? | |
| if line_item.enterprise_fee_adjustments.blank? | |
| create_line_item_fees!(line_item) | |
| next | |
| end |
There was a problem hiding this comment.
Looking good!
I find it really hard to tell if this will cover all use cases correctly. We have assumptions of how the shop owner would like to adjust orders but we can't actually predict every case. This makes sense though. Just to summarise my understanding:
- Order fees are deleted and re-created.
- Line item fees are updated in place.
- If a product is removed from the order cycle, the fee is still updated.
- If a fee is deleted then it gets removed from the order.
- If a fee is added then it will be added to the order.
I find that last point questionable. I think that a lot of enterprises want to add the fee when updating an order. They wish they had added the fee to start with. And they may go through and updated existing orders after communicating with customers. But some shops may want to honour the old contract and not charge a fee that wasn't present when the item was bought.
An example I can think of:
- A shop sells milk and garlic from one farmer.
- Frank buys milk like every week.
- The shop receives more orders for milk than usual or the cows are not giving as much as usual. But they can get more supply from a different farmer, further away, with added transportation cost.
- A day later, in the same OC, Frank adds carrots to his order.
- The order adjustment suddenly made his milk more expensive even though he's getting supply from the original farmer. ❓
This scenario is out of scope here though. The old logic was re-creating fees and therefore adding new fees already. So let's keep it until someone actually encounters this scenario.
Going through this was more for my understanding. No action required. Just let me know if I got something wrong.
| @order_cycle.exchanges.supplying_to(@distributor).each do |exchange| | ||
| exchange.enterprise_fees.per_item.each do |enterprise_fee| | ||
| fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, | ||
| exchange.role) | ||
| end | ||
| end |
There was a problem hiding this comment.
There should be only one exchange per distributor, right?
| fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, | ||
| exchange.role) |
There was a problem hiding this comment.
Since we are in the same module, we should be able to omit that:
| fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, | |
| exchange.role) | |
| fees << EnterpriseFeeApplicator.new(enterprise_fee, variant, exchange.role) |
dacook
added
the
bug-s2
What? Why?
Currently when an order gets updated all enterprise fees are deleted and recreated to ensure it's up to date. This is an issue when a product has been removed from an Order Cycle, any subsequent update to the order will remove fees associated with said product. To prevent this, we now create or update per item enterprise fees, per order fee are still deleted and recreated.
An easier way to fix the issue is to assume any product in the order are from the Order Cycle linked to the order, so we could keep the delete and recreate solution. However, currently Order Cycle are not mandatory on the Order, and there is no check to ensure a product is the Order Cycle when adding/updating an order.
What should we test?
In general we want to test that enterprise fee per item are working as before
As an enterprise manager :
As a customer
1. Removing variant
As an enterprise manager:
Search for the just placed order and open the edit page
ensure both fees were applied
remove on outgoing variant
on the order edit page, Click on "update and recalculate fees!"
--> check the outgoing fee is still present
remove the incoming variant
on the order edit page, Click on "update and recalculate fees!"
--> check the incoming fee is still present
update the removed product quantity
--> check the fess are re calculated as expected
As a customer
As an enterprise manager
As a customer
--> Check the fees have not been removed to the variant removed from the Order Cycle
2. Removing fee
remove a fee from the Order Cycle,
on the order edit page, Click on "update and recalculate fees!"
--> Check the fee has been removed
Go to Configuration -> Enterprise Fees
Remove a fee associated to the Order Cycle
on the order edit page, Click on "update and recalculate fees!"
--> Check the fee has been removed
Remove a variant from the order cycle
Run the scenario above
--> Check the fee has been removed for the removed variant
3. Adding fee
add a fee to the Order Cycle
on the order edit page, Click on "update and recalculate fees!"
--> Check the fee has been added
Remove a variant from the order cycle
Run the scenario above
--> Check fee has been added to the removed variant
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.