Update outstanding_balance to use reimbursements #2002

jhawthorn · 2017-06-07T23:59:50Z

This updates outstanding_balance to use the value of "reimbursements" when determining the payment total.

Previously the value of an order's refunds was used both for representing the money returned to the customer as well as the reduced "adjusted total" of the order (which we expected to be paid). This was
wrong in a number of cases.

This updates outstanding_balance to use the value of reimbursements for calculating the "adjusted total" of the order. This allows all cases to (at least) be represented properly. This better
matches the best practice double-entry bookkeeping style.

Reimbursements are created when going through the refund system so this should continue to work for cases where refunds where made through the returns system.

This PR includes a test from @ericsaupe (showing breakage of the existing outstanding_balance as well as my #1578 integration tests (with all :pending specs now passing!) and #1926 regression test (already merged in master).

Thanks to everyone who had continued to report, prod and provide tests for this issue (too many to list but I have CC'd some of you fine folks). This has been a doozy, and I hope this is a viable solution.

CC @ericsaupe @skukx @DanielePalombo @jasonfb @bbuchalter @jordan-brough @mamhoff @stewart @luukveenis @Senjai

If any of you can verify that this meets your needs, or observe it's effects on production-ish data that would be amazing.

Not addressed here is a data migration to update the payment_state of existing orders. This will require a detailed CHANGELOG entry.

tvdeyen

Makes lots of sense to me. Thanks to everybody involved in fixing this! 👏

ericsaupe

Works for me. Glad we got this figured out!

mamhoff

This puts an end to a bug that's long been bothering us. 👍!

jordan-brough

Thank you @jhawthorn! This matches the "Potential solution" from #1254 and covers all the scenarios there as far as I can tell, while also adding the coverage of reimbursements. Very nice.

I left a few questions in the specs.

I also opened a PR against your branch with a couple extra specs that I tried out while reviewing #1254, if you'd like: jhawthorn#3

The changelog update is still on the TODO list, right? I imagine one important item would be to let people know that if they want to "just give money back" to a customer (e.g. give them $5 back because they were unhappy), they now need to create an adjustment (along with the refund), where previously they would just create the Refund. Right?

jordan-brough · 2017-06-19T19:45:26Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+        expect(order.payment_total).to eq(16)
+        expect(order.outstanding_balance).to eq(0)
+
+        # Less sure about this one....


Did you intend to leave this comment in? 19 seems OK to me (slightly confusing but does make sense from a certain POV)

jordan-brough · 2017-06-19T19:46:40Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+      end
+
+      context 'and there is a full refund' do
+        it 'has the correct amount'


Did you intend to implement this spec?

jordan-brough · 2017-06-19T19:54:41Z

Linking to #1254 (since this fixes it)

This updates outstanding_balance to use the value of "reimbursements" when determining the payment total. Previously the value of an order's refunds was used both for representing the money returned to the customer as well as the reduced "adjusted total" of the order (which we expected to be paid). This was wrong in a number of cases. This commit updates outstanding_balance to use the value of reimbursements for calculating the "adjusted total" of the order. This allows all cases to (at least) be represented properly. This better matches the best practice double-entry bookkeeping style. Reimbursements are created when going through the refund system so this should continue to work for cases where refunds where made through the returns system.

houndci-bot · 2017-06-22T22:33:32Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+      context 'and there is a full refund' do
+        let!(:refund) { create(:refund, payment: payment, amount: payment.amount) }
+        it { should == 0 }


Use should.zero? instead of should == 0.

houndci-bot · 2017-06-22T22:33:32Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+      context 'and the payment is voided' do
+        before { payment.update_attributes!(state: "void") }
+        it { should == 0 }


Use should.zero? instead of should == 0.

houndci-bot · 2017-06-22T22:33:32Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+      context 'and there is a full refund' do
+        let!(:refund) { create(:refund, payment: payment, amount: payment.amount) }
+        it { should == 0 }


Use should.zero? instead of should == 0.

houndci-bot · 2017-06-22T22:33:33Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+      context 'and the payment is voided' do
+        before { payment.update_attributes!(state: "void") }
+        it { should == 0 }


Use should.zero? instead of should == 0.

houndci-bot · 2017-06-22T22:33:33Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+  context 'when the order is fully paid' do
+    let!(:payment) { create(:payment, :completed, order: order, amount: order.total) }
+    it { should == 0 }


Use should.zero? instead of should == 0.

houndci-bot · 2017-06-22T22:33:33Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+    context 'when the order is cancelled' do
+      before { order.cancel! }
+      it { should == 0 }


Use should.zero? instead of should == 0.

houndci-bot · 2017-06-22T22:38:37Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+      context 'and there is a partial refund' do
+        let!(:refund) { create(:refund, payment: payment, amount: 6) }
+        it { should eq -4 }


Ambiguous negative number operator. Parenthesize the method arguments if it's surely a negative number operator, or add a whitespace to the right of the - if it should be a subtraction.

houndci-bot · 2017-06-22T22:38:38Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+    context 'when the order is cancelled' do
+      before { order.update_attributes!(state: "canceled") }
+      it { should eq -10 }


Ambiguous negative number operator. Parenthesize the method arguments if it's surely a negative number operator, or add a whitespace to the right of the - if it should be a subtraction.

houndci-bot · 2017-06-22T22:38:38Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+      context 'and there is a partial refund' do
+        let!(:refund) { create(:refund, payment: payment, amount: 6) }
+        it { should eq -13 }


Ambiguous negative number operator. Parenthesize the method arguments if it's surely a negative number operator, or add a whitespace to the right of the - if it should be a subtraction.

houndci-bot · 2017-06-22T22:38:38Z

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

+
+    context 'when the order is cancelled' do
+      before { order.update_attributes!(state: "canceled") }
+      it { should eq -19 }


Ambiguous negative number operator. Parenthesize the method arguments if it's surely a negative number operator, or add a whitespace to the right of the - if it should be a subtraction.

Created a spec to simulate the behavior listed in solidusio#1475 in order to properly fix the issue.

Update this spec to match the values it currently returns. I changed the values in this spec into what values we got from the system, which isn't ideal, but I think these values make sense. We have a payment for an amount and a refund for the same amount, so payment total should be zero. Because the order is cancelled, this is the state we want, and the outstanding balance should be zero.

jhawthorn added type:bug Error, flaw or fault Code review needed labels Jun 7, 2017

tvdeyen approved these changes Jun 8, 2017

View reviewed changes

ericsaupe approved these changes Jun 8, 2017

View reviewed changes

mamhoff approved these changes Jun 13, 2017

View reviewed changes

jhawthorn requested a review from jordan-brough June 14, 2017 21:59

jordan-brough reviewed Jun 19, 2017

View reviewed changes

jhawthorn mentioned this pull request Jun 21, 2017

Fix outstanding balance calculation with refunds #1835

Closed

jhawthorn force-pushed the outstanding_balance_omnibus branch from 3d4959b to 915097f Compare June 22, 2017 22:32

houndci-bot reviewed Jun 22, 2017

View reviewed changes

jhawthorn force-pushed the outstanding_balance_omnibus branch from 915097f to b0d21bc Compare June 22, 2017 22:38

houndci-bot reviewed Jun 22, 2017

View reviewed changes

jhawthorn and others added 5 commits June 22, 2017 15:40

Create integration spec for outstanding_balance

a9efc72

Added spec for refunds not updating balance due

96037f1

Created a spec to simulate the behavior listed in solidusio#1475 in order to properly fix the issue.

Reload order to get current totals

3f37aa1

Add CHANGELOG entry for outstanding_balance change

6a031da

jhawthorn force-pushed the outstanding_balance_omnibus branch from b0d21bc to 6a031da Compare June 22, 2017 22:40

jhawthorn merged commit 43ff4a4 into solidusio:master Jun 22, 2017

This was referenced Jun 22, 2017

Order#outstanding_balance implementation limits Refund usage #1254

Closed

Refunding Doesn't Update Credit Owed #1475

Closed

Fix how orders payment total is calculated #1536

Closed

jordan-brough mentioned this pull request Jun 28, 2017

Add a few more specs to outstanding_balance_integration_spec #2051

Merged

mamhoff removed the Code review needed label Jul 14, 2017

tvdeyen mentioned this pull request Oct 4, 2017

Add integration spec to cover Order#outstanding_balance #1578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update outstanding_balance to use reimbursements #2002

Update outstanding_balance to use reimbursements #2002

jhawthorn commented Jun 7, 2017

tvdeyen left a comment

ericsaupe left a comment

mamhoff left a comment

jordan-brough left a comment

jordan-brough Jun 19, 2017

jordan-brough Jun 19, 2017

jordan-brough commented Jun 19, 2017 •

edited

Loading

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

houndci-bot Jun 22, 2017

Update outstanding_balance to use reimbursements #2002

Update outstanding_balance to use reimbursements #2002

Conversation

jhawthorn commented Jun 7, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

ericsaupe left a comment

Choose a reason for hiding this comment

mamhoff left a comment

Choose a reason for hiding this comment

jordan-brough left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordan-brough commented Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordan-brough commented Jun 19, 2017 •

edited

Loading