Skip to content

Commit b47e0f0

Browse files
committed
Fix shipment adjustments not persisting on order recalculate
A commit was made in solidusio#6315 (5979f25) in service of the in memory order updater solidusio#5872 that changed the behavior in the OrderTaxation class to set the adjustment value but not persist it. Instead, the persistence happens when the order is saved. If a shipment is changed, and an associated adjustment is changed, the shipment adjustment will be autosaved when the order is. However, if a shipment is *not* changed, but the adjustment is, the adjustment will *not* be autosaved when the order is. In solidusio#6315, we ensured that these changed associations cannot get left behind by setting `autosave: true`, which forces the model to check if it has any associations that need saving instead of telling it's relation "nope I haven't changed" We can do the same thing here for the shipment to ensure changing adjustments will make the shipment tell it's order "I have changed", even if none of it's direct properties have changed. You can test this change by removing the `autosave: true` option the adjustments and running 'when the address has changed to a different state' specs. They fail with the error "expected `order.shipments.first.adjustments.first.amount` to have changed from 1 to 2, but did not change", demonstrating a scenario where the shipment adjustment changes do not get persisted.
1 parent d070f94 commit b47e0f0

File tree

2 files changed

+59
-14
lines changed

2 files changed

+59
-14
lines changed

core/app/models/spree/shipment.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ class Shipment < Spree::Base
99
belongs_to :order, class_name: 'Spree::Order', touch: true, inverse_of: :shipments, optional: true
1010
belongs_to :stock_location, class_name: 'Spree::StockLocation', optional: true
1111

12-
has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :delete_all
12+
has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :delete_all, autosave: true
1313
has_many :inventory_units, dependent: :destroy, inverse_of: :shipment
1414
has_many :shipping_rates, -> { order(:cost) }, dependent: :destroy, inverse_of: :shipment
1515
has_many :shipping_methods, through: :shipping_rates

core/spec/models/spree/order_updater_spec.rb

Lines changed: 58 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -72,14 +72,14 @@ module Spree
7272
let(:tax_category) { create(:tax_category) }
7373
let(:ship_address) { create(:address, state: new_york) }
7474
let(:new_york) { create(:state, state_code: "NY") }
75-
let(:tax_zone) { create(:zone, states: [new_york]) }
75+
let(:new_york_tax_zone) { create(:zone, states: [new_york]) }
7676

77-
let!(:tax_rate) do
77+
let!(:new_york_tax_rate) do
7878
create(
7979
:tax_rate,
8080
name: "New York Sales Tax",
8181
tax_categories: [tax_category],
82-
zone: tax_zone,
82+
zone: new_york_tax_zone,
8383
included_in_price: false,
8484
amount: 0.1
8585
)
@@ -111,21 +111,66 @@ module Spree
111111
end
112112

113113
context 'when the address has changed to a different state' do
114-
let(:new_shipping_address) { create(:address) }
114+
let(:oregon) { create(:state, state_code: "OR") }
115+
let(:oregon_tax_zone) { create(:zone, states: [oregon]) }
116+
let!(:oregon_tax_rate) do
117+
create(
118+
:tax_rate,
119+
name: "Oregon Sales Tax",
120+
tax_categories: [tax_category],
121+
zone: oregon_tax_zone,
122+
included_in_price: false,
123+
amount: 0.2
124+
)
125+
end
126+
let(:new_address) { create(:address, state: oregon) }
127+
let(:shipping_method) { create(:shipping_method, tax_category:, zones: [oregon_tax_zone, new_york_tax_zone], cost: 10) }
128+
let(:shipping_rate) do
129+
create(:shipping_rate, cost: 10, shipping_method: shipping_method)
130+
end
131+
let(:shipment) { order.shipments[0] }
132+
133+
subject do
134+
order.ship_address = new_address
135+
order.bill_address = new_address
136+
137+
order.recalculate
138+
end
115139

116140
before do
117-
order.ship_address = new_shipping_address
141+
shipment.shipping_rates = [shipping_rate]
142+
shipment.selected_shipping_rate_id = shipping_rate.id
143+
order.recalculate
118144
end
119145

120-
it 'removes the old taxes' do
146+
it 'updates the taxes to reflect the new state' do
121147
expect {
122-
order.recalculate
148+
subject
123149
}.to change {
124-
order.all_adjustments.tax.count
125-
}.from(1).to(0)
150+
order.additional_tax_total
151+
}.from(2).to(4)
152+
end
126153

127-
expect(order.additional_tax_total).to eq 0
128-
expect(order.adjustment_total).to eq 0
154+
it 'updates the shipment taxes to reflect the new state' do
155+
expect {
156+
subject
157+
}.to change {
158+
order.shipments.first.additional_tax_total
159+
}.from(1).to(2)
160+
.and change {
161+
order.shipments.first.adjustments.first.amount
162+
}.from(1).to(2)
163+
end
164+
165+
it 'updates the line item taxes to reflect the new state' do
166+
expect {
167+
subject
168+
}.to change {
169+
order.line_items.first.additional_tax_total
170+
}.from(1).to(2)
171+
.and change {
172+
order.line_items.first.adjustments.first.amount
173+
}.from(1).to(2)
129174
end
130175
end
131176

@@ -179,7 +224,7 @@ module Spree
179224
order_taxes: [
180225
Spree::Tax::ItemTax.new(
181226
label: "Delivery Fee",
182-
tax_rate:,
227+
tax_rate: new_york_tax_rate,
183228
amount: 2.60,
184229
included_in_price: false
185230
)
@@ -188,7 +233,7 @@ module Spree
188233
Spree::Tax::ItemTax.new(
189234
item_id: line_item.id,
190235
label: "Item Tax",
191-
tax_rate:,
236+
tax_rate: new_york_tax_rate,
192237
amount: 1.40,
193238
included_in_price: false
194239
)

0 commit comments

Comments
 (0)