Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix how orders payment total is calculated #1536

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/app/helpers/spree/api/api_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def required_fields_for(model)
:user_id, :created_at, :updated_at, :completed_at, :payment_total,
:shipment_state, :payment_state, :email, :special_instructions, :channel,
:included_tax_total, :additional_tax_total, :display_included_tax_total,
:display_additional_tax_total, :tax_total, :currency,
:display_additional_tax_total, :tax_total, :currency, :incoming_payment,
:covered_by_store_credit, :display_total_applicable_store_credit,
:order_total_after_store_credit, :display_order_total_after_store_credit,
:total_applicable_store_credit, :display_total_available_store_credit,
Expand Down
4 changes: 2 additions & 2 deletions api/spec/controllers/spree/api/orders_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ module Spree
let(:attributes) {
[:number, :item_total, :display_total, :total,
:state, :adjustment_total,
:user_id, :created_at, :updated_at,
:completed_at, :payment_total, :shipment_state,
:user_id, :created_at, :updated_at, :completed_at,
:payment_total, :incoming_payment, :shipment_state,
:payment_state, :email, :special_instructions,
:total_quantity, :display_item_total, :currency]
}
Expand Down
12 changes: 6 additions & 6 deletions core/app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -353,21 +353,21 @@ def create_tax_charge!
end

def outstanding_balance
# If reimbursement has happened add it back to total to prevent balance_due payment state
# See: https://github.com/spree/spree/issues/6229
adjusted_payment_total = payment_total + refund_total

if state == 'canceled'
-1 * adjusted_payment_total
-1 * payment_total
else
total - adjusted_payment_total
total - payment_total
end
end

def outstanding_balance?
outstanding_balance != 0
end

def payment_total
incoming_payment - refund_total
end

def refund_total
payments.flat_map(&:refunds).sum(&:amount)
end
Expand Down
8 changes: 6 additions & 2 deletions core/app/models/spree/order_updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def recalculate_adjustments

# Updates the following Order total values:
#
# +payment_total+ The total value of all finalized Payments (NOTE: non-finalized Payments are excluded)
# +incoming_total+ The total value of all finalized Payments (NOTE: non-finalized Payments are excluded)
# +item_total+ The total value of all LineItems
# +adjustment_total+ The total value of all adjustments (promotions, credits, etc.)
# +promo_total+ The total value of all promotion adjustments
Expand All @@ -73,7 +73,11 @@ def update_shipments
end

def update_payment_total
order.payment_total = payments.completed.includes(:refunds).map { |payment| payment.amount - payment.refunds.sum(:amount) }.sum
update_incoming_payment
end

def update_incoming_payment
order.incoming_payment = payments.completed.sum(&:amount)
end

def update_shipment_total
Expand Down
18 changes: 18 additions & 0 deletions core/db/migrate/20161028093407_add_incoming_payment_to_order.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class AddIncomingPaymentToOrder < ActiveRecord::Migration[5.0]
def change
add_column :spree_orders, :incoming_payment, :decimal, precision: 10, scale: 2, default: 0.0
remove_column :spree_orders, :payment_total, :decimal, precision: 10, scale: 2, default: 0.0

reversible do |direction|
direction.up do
Spree::Order.all.each {|o| o.update_incoming_payment }
end

direction.down do
Spree::Order.all.each do |o|
o.payment_total = o.payments.completed.includes(:refunds).map { |payment| payment.amount - payment.refunds.sum(:amount) }.sum
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@
expect(order).to have_attributes(
payment_state: 'balance_due',
total: 110,
payment_total: 0 # payment is still pending
incoming_payment: 0 # payment is still pending
)

expect(order.payments.count).to eq 1
Expand All @@ -145,7 +145,7 @@
expect(order).to be_completed
expect(order).to have_attributes(
total: 110,
payment_total: 110,
incoming_payment: 110,
payment_state: "paid",
shipment_state: "ready"
)
Expand Down Expand Up @@ -184,7 +184,7 @@
expect(order).to be_completed
expect(order).to have_attributes(
total: 110,
payment_total: 110,
incoming_payment: 110,
payment_state: "paid"
)

Expand Down
60 changes: 44 additions & 16 deletions core/spec/models/spree/order/payment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ module Spree
updater.update_payment_state

expect(order.payment_state).to eq('paid')
expect(order.payment_total).to eq(100)
expect(order.incoming_payment).to eq(100)

expect(payment_1).to be_completed
expect(payment_2).to be_completed
Expand All @@ -35,7 +35,7 @@ module Spree
updater.update_payment_state

expect(order.payment_state).to eq('paid')
expect(order.payment_total).to eq(100)
expect(order.incoming_payment).to eq(100)

expect(payment_1).to be_completed
expect(payment_2).to be_completed
Expand All @@ -51,7 +51,7 @@ module Spree

order.process_payments!

expect(order.payment_total).to eq(50)
expect(order.incoming_payment).to eq(50)
end
end

Expand Down Expand Up @@ -155,28 +155,54 @@ module Spree
end

context "#outstanding_balance" do
it "should return positive amount when payment_total is less than total" do
order.payment_total = 20.20
it "should return positive amount when incoming_payment is less than total" do
order.incoming_payment = 20.20
order.total = 30.30
expect(order.outstanding_balance).to eq(10.10)
end
it "should return negative amount when payment_total is greater than total" do

it "should return negative amount when incoming_payment is greater than total" do
order.total = 8.20
order.payment_total = 10.20
order.incoming_payment = 10.20
expect(order.outstanding_balance).to be_within(0.001).of(-2.00)
end

let(:order){ create(:order_with_line_items) }

before do
create(:payment, amount: 20, state: :completed, order: order)
end

context "for canceled orders" do
before { order.update_attributes(state: 'canceled') }

it "it should be a negative amount" do
# -1 * (Payment Total + Reimbursed)
# -1 * (20 - 0) = -20
expect(order.outstanding_balance).to eq(-20)
end
end

context "for non-canceled orders" do
it 'should incorporate refund reimbursements' do
# Order Total - (Payment Total + Reimbursed)
# 110 - (20 - 0) = 90
expect(order.outstanding_balance).to eq 90
end
end

context "with reimburesements on the order" do
let(:amount) { 10.0 }
let(:amount_paid) { 20.0 }
let(:amount_refunded) { 10.0 }
let(:reimbursement) { create(:reimbursement) }
let(:order) { reimbursement.order.reload }

before do
reimbursement.order = order
# Set the payment amount to actually be the order total of 110
reimbursement.order.payments.first.update_column :amount, amount
reimbursement.order.payments.first.update_column :amount, amount_paid
# Creates a refund of 110
create :refund, amount: amount,
payment: reimbursement.order.payments.first,
create :refund, amount: amount_refunded,
payment: order.payments.first,
reimbursement: reimbursement
# Update the order totals so payment_total goes to 0 reflecting the refund..
order.update!
Expand All @@ -186,14 +212,16 @@ module Spree
before { order.update_attributes(state: 'canceled') }

it "it should be a negative amount incorporating reimbursements" do
# -1 * (Payment Total + Reimbursed)
# -1 * (20 - 10) = -10
expect(order.outstanding_balance).to eq(-10)
end
end

context "for non-canceled orders" do
it 'should incorporate refund reimbursements' do
# Order Total - (Payment Total + Reimbursed)
# 110 - (0 + 10) = 100
# 110 - (20 - 10) = 100
expect(order.outstanding_balance).to eq 100
end
end
Expand All @@ -203,17 +231,17 @@ module Spree
context "#outstanding_balance?" do
it "should be true when total greater than payment_total" do
order.total = 10.10
order.payment_total = 9.50
order.incoming_payment = 9.50
expect(order.outstanding_balance?).to be true
end
it "should be true when total less than payment_total" do
order.total = 8.25
order.payment_total = 10.44
order.incoming_payment = 10.44
expect(order.outstanding_balance?).to be true
end
it "should be false when total equals payment_total" do
order.total = 10.10
order.payment_total = 10.10
order.incoming_payment = 10.10
expect(order.outstanding_balance?).to be false
end
end
Expand Down
22 changes: 11 additions & 11 deletions core/spec/models/spree/order_updater_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ module Spree
end

context 'with refund' do
it "updates payment totals" do
it "updates payment incoming" do
create(:payment_with_refund, order: order, amount: 33.25, refund_amount: 3)
Spree::OrderUpdater.new(order).update_payment_total
expect(order.payment_total).to eq(30.25)
Spree::OrderUpdater.new(order).update_incoming_payment
expect(order.incoming_payment).to eq(33.25)
end
end

Expand Down Expand Up @@ -344,7 +344,7 @@ def create_adjustment(label, amount)
it "is failed" do
create(:payment, order: order, state: 'invalid')
order.total = 1
order.payment_total = 0
order.incoming_payment = 0

updater.update_payment_state
expect(order.payment_state).to eq('failed')
Expand All @@ -355,7 +355,7 @@ def create_adjustment(label, amount)
it 'is paid' do
order.payments << Spree::Payment.new(state: 'invalid')
order.total = 0
order.payment_total = 0
order.incoming_payment = 0

expect {
updater.update_payment_state
Expand All @@ -365,7 +365,7 @@ def create_adjustment(label, amount)

context "payment total is greater than order total" do
it "is credit_owed" do
order.payment_total = 2
order.incoming_payment = 2
order.total = 1

expect {
Expand All @@ -376,7 +376,7 @@ def create_adjustment(label, amount)

context "order total is greater than payment total" do
it "is balance_due" do
order.payment_total = 1
order.incoming_payment = 1
order.total = 2

expect {
Expand All @@ -387,7 +387,7 @@ def create_adjustment(label, amount)

context "order total equals payment total" do
it "is paid" do
order.payment_total = 30
order.incoming_payment = 30
order.total = 30

expect {
Expand All @@ -403,7 +403,7 @@ def create_adjustment(label, amount)

context "and is still unpaid" do
it "is void" do
order.payment_total = 0
order.incoming_payment = 0
order.total = 30
expect {
updater.update_payment_state
Expand All @@ -413,7 +413,7 @@ def create_adjustment(label, amount)

context "and is paid" do
it "is credit_owed" do
order.payment_total = 30
order.incoming_payment = 30
order.total = 30
create(:payment, order: order, state: 'completed', amount: 30)
expect {
Expand All @@ -424,7 +424,7 @@ def create_adjustment(label, amount)

context "and payment is refunded" do
it "is void" do
order.payment_total = 0
order.incoming_payment = 0
order.total = 30
expect {
updater.update_payment_state
Expand Down
16 changes: 16 additions & 0 deletions core/spec/models/spree/payment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,11 @@
payment = create(:payment, order: order, state: 'completed')
expect(order.payment_total).to eq payment.amount
end

it "update order incoming total" do
payment = create(:payment, order: order, state: 'completed')
expect(order.incoming_payment).to eq payment.amount
end
end

context "not completed payments" do
Expand All @@ -637,6 +642,12 @@
Spree::Payment.create(amount: 100, order: order)
}.not_to change { order.payment_total }
end

it "doesn't update order incoming payment" do
expect {
Spree::Payment.create(amount: 100, order: order)
}.not_to change { order.incoming_payment }
end
end

context 'when the payment was completed but now void' do
Expand All @@ -652,6 +663,11 @@
payment.void
expect(order.payment_total).to eq 0
end

it 'updates order incoming payment' do
payment.void
expect(order.incoming_payment).to eq 0
end
end

context "completed orders" do
Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/shipment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@
let(:store) { create :store }
let(:order) do
Spree::Order.create(
payment_total: 100,
incoming_payment: 100,
payment_state: 'paid',
total: 100,
item_total: 100,
Expand Down