Break ItemAdjustments apart and put the logic in OrderUpdater #1466
Conversation
| item.additional_tax_total + | ||
| item_cancellation_total | ||
|
|
||
| if item.changed? |
There was a problem hiding this comment.
I think I disagree with our rubocop rules here.
There was a problem hiding this comment.
👍 Updated in #1467
| end | ||
|
|
||
| promo_sequences.each do |promo_sequence| | ||
| context 'with promo sequence #{promo_sequence}' do |
There was a problem hiding this comment.
Single quotes prevents interpolation here
There was a problem hiding this comment.
Thanks! Fixed by the update that groups this loop with the previous one.
| end | ||
| end | ||
|
|
||
| promo_sequences.each do |promo_sequence| |
There was a problem hiding this comment.
It might make sense to group this loop with the previous identical one.
There was a problem hiding this comment.
Great idea. Done.
| item.additional_tax_total + | ||
| item_cancellation_total | ||
|
|
||
| if item.changed? |
There was a problem hiding this comment.
I think I disagree with our rubocop rules here.
|
|
||
| def update_item_totals | ||
| [*line_items, *shipments].each do |item| | ||
| item_cancellation_total = item.adjustments.select(&:cancellation?).sum(&:amount) |
There was a problem hiding this comment.
It feels weird that promo_total, included_tax_total, and additional_tax_total are summed in their respective methods, but cancellations are not.
There was a problem hiding this comment.
Agreed. I've added a comment to that effect -- cancellation_total isn't persisted anywhere so we have to extract it here. (The others need to be done in their respective methods so that, e.g., tax calculations have the updated promo totals).
jordan-brough
force-pushed
the
split-up-item-adjustments
branch
from
a2d8000 to
9155585
Compare
|
@jhawthorn thanks for the feedback! I've added commits for all the comments (including bringing in most of the code comments from ItemAdjustments). I also rebased on latest master to get the updated rubocop rules. |
With ItemAdjustments we updated items in this order:
- item 1
- update promotions
- update taxes
- update cancellations
- update totals
- item 2
- update promotions
- update taxes
- update cancellations
- update totals
However, we need to invoke tax calculation on the order as a whole
(even though the results will be stored at the line/shipment level) in
order to provide a good extension point for external tax services.
That means we need to do this:
- update promotions
- item 1
- item 2
- update taxes
- item 1
- item 2
- update cancellations
- item 1
- item 2
- update totals
- item 1
- item 2
I couldn't think of a reasonable way to refactor ItemAdjustments to be
able to do the above, without breaking backward compatibility in a
significant way. And if we're going to break backward compatibility
then moving this logic into OrderUpdater seemed like a better change
to me as a first step.
Once we have this in place we can start working on providing an
extension point in the "update taxes" step where external tax services
can be plugged in.
Shipments can't have item cancellations.
Bring in a bunch of helpful comments that were in the ItemAdjustments class. And split apart update_item_promotions and update_order_promotions to make the code clearer since they are handled slightly differently.
Also fixes single quotes that were prevent interpolation.
jordan-brough
force-pushed
the
split-up-item-adjustments
branch
from
9155585 to
8d99136
Compare
|
|
||
| # Update and select the best promotion adjustment for the order. | ||
| # We don't update the order.promo_total yet. Order totals are updated later | ||
| # in #update_adjustment_total since they include the totals from the ordre's |
There was a problem hiding this comment.
Fixed. Thanks!
This sets up tax updates in
OrderUpdaterso that all tax updates for an orderhappen at the same time.
With
ItemAdjustmentswe updated items in this order:However, we need to invoke tax calculation on the order as a whole
(even though the results will be calculated & stored at the line/shipment level) in
order to provide a good extension point for external tax services. See #1252.
That means we need to do this:
I couldn't think of a reasonable way to refactor ItemAdjustments to be
able to do the above, without breaking backward compatibility in a
significant way. And if we're going to break backward compatibility
then moving this logic into OrderUpdater seemed like a better change
to me as a first step.
Once we have this in place we can start working on providing an
extension point in the "update taxes" step where external tax services
can be plugged in.