Skip to content

Commit aa47046

Browse files
jordan-broughJeff Dutil
authored andcommitted
New paradigm for customer refunds
Initial step in a larger piece of returns & exchanges improvements. Major * new "refunded" state for return authorizations * separation of refunds from payments * add a new refund model * use "refunds" in place of negative payments * stop generating order adjustments when receiving return authorizations * we no longer need to create order adjustments in order to provide refunds * updated admin ui * added a "refund" button in the return authorization flow * list refunds along with payments * add payment identifier column to payments list Minor * extra validation on `ReturnAuthorization#force_positive_amount` * improve some existing specs by using real `ActiveMerchant::Billing::Response` objects instead of doubles Generated by @jordan-brough & @richardnuno Fixes #4883
1 parent 4c5ef67 commit aa47046

16 files changed

Lines changed: 590 additions & 116 deletions

File tree

api/spec/controllers/spree/api/payments_controller_spec.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -225,14 +225,14 @@ module Spree
225225
response.status.should == 200
226226
payment.reload.state.should == "completed"
227227

228-
# Ensure that a credit payment was created, and it has correct credit amount
229-
credit_payment = Payment.where(:source_type => 'Spree::Payment', :source_id => payment.id).last
230-
credit_payment.amount.to_f.should == -45.75
228+
# Ensure that a refund was created, and it has correct credit amount
229+
refund = payment.refunds.last
230+
refund.amount.to_f.should == 45.75
231231
end
232232

233233
context "crediting fails" do
234234
it "returns a 422 status" do
235-
fake_response = double(:success? => false, :to_s => "NO CREDIT FOR YOU")
235+
fake_response = ActiveMerchant::Billing::Response.new(false, 'NO CREDIT FOR YOU', {}, {})
236236
Spree::Gateway::Bogus.any_instance.should_receive(:credit).and_return(fake_response)
237237
api_put :credit, :id => payment.to_param
238238
response.status.should == 422

backend/app/views/spree/admin/payments/_list.html.erb

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
<table class="index" id='payments' data-order-id='<%= @order.number %>'>
22
<thead>
33
<tr data-hook="payments_header">
4+
<th><%= Spree.t(:identifier) %></th>
45
<th><%= "#{Spree.t('date')}/#{Spree.t('time')}" %></th>
56
<th><%= Spree.t(:amount) %></th>
67
<th><%= Spree.t(:payment_method) %></th>
@@ -10,8 +11,9 @@
1011
</thead>
1112
<tbody>
1213
<% payments.each do |payment| %>
13-
<tr id="<%= dom_id(payment) %>" data-hook="payments_row" class="<%= cycle('odd', 'even')%>">
14-
<td><%= link_to pretty_time(payment.created_at), spree.admin_order_payment_path(@order, payment) %></td>
14+
<tr id="<%= dom_id(payment) %>" data-hook="payments_row" class="<%= cycle('odd', 'even', name: 'payment_table_cycle')%>">
15+
<td><%= link_to payment.identifier, spree.admin_order_payment_path(@order, payment) %></td>
16+
<td><%= pretty_time(payment.created_at) %></td>
1517
<td class="align-center amount"><%= payment.display_amount.to_html %></td>
1618
<td class="align-center"><%= payment_method_name(payment) %></td>
1719
<td class="align-center"> <span class="state <%= payment.state %>"><%= Spree.t(payment.state, :scope => :payment_states, :default => payment.state.capitalize) %></span></td>
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<table class="index" id='payments' data-order-id='<%= @order.number %>'>
2+
<thead>
3+
<tr data-hook="refunds_header">
4+
<th><%= "#{Spree.t('date')}/#{Spree.t('time')}" %></th>
5+
<th><%= Spree.t(:payment_identifier) %></th>
6+
<th><%= Spree.t(:amount) %></th>
7+
<th><%= Spree.t(:payment_method) %></th>
8+
</tr>
9+
</thead>
10+
<tbody>
11+
<% refunds.each do |refund| %>
12+
<tr id="<%= dom_id(refund) %>" data-hook="refunds_row" class="<%= cycle('odd', 'even', name: 'refund_table_cycle')%>">
13+
<td><%= pretty_time(refund.created_at) %></td>
14+
<td><%= refund.payment.identifier %></td>
15+
<td class="align-center amount"><%= refund.display_amount %></td>
16+
<td class="align-center"><%= payment_method_name(refund.payment) %></td>
17+
</tr>
18+
<% end %>
19+
</tbody>
20+
</table>

backend/app/views/spree/admin/payments/index.html.erb

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,20 @@
1818
<% end %>
1919

2020
<% if @payments.any? %>
21-
<%= render :partial => 'list', :locals => { :payments => @payments } %>
21+
22+
<fieldset data-hook="payment_list" class="no-border-bottom">
23+
<legend align="center"><%= Spree.t(:payments) %></legend>
24+
<%= render :partial => 'list', :locals => { :payments => @payments } %>
25+
</fieldset>
26+
27+
<% @refunds = @payments.flat_map(&:refunds) %>
28+
<% if @refunds.any? %>
29+
<fieldset data-hook="payment_list" class="no-border-bottom">
30+
<legend align="center"><%= Spree.t(:refunds) %></legend>
31+
<%= render :partial => 'refunds', :locals => { :refunds => @refunds } %>
32+
</fieldset>
33+
<% end %>
34+
2235
<% else %>
2336
<div class="alpha twelve columns no-objects-found"><%= Spree.t(:order_has_no_payments) %></div>
2437
<% end %>

backend/app/views/spree/admin/return_authorizations/_form.html.erb

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,13 @@
1818
<td class="align-center"><%= units.select(&:shipped?).size %></td>
1919
<td class="align-center"><%= units.select(&:returned?).size %></td>
2020
<td class="return_quantity align-center">
21-
<% if @return_authorization.received? %>
22-
<%= @return_authorization.inventory_units.group_by(&:variant)[variant].try(:size) || 0 %>
23-
<% elsif units.select(&:shipped?).empty? %>
21+
<% if units.select(&:shipped?).empty? %>
2422
0
25-
<% else %>
23+
<% elsif @return_authorization.updatable? %>
2624
<%= number_field_tag "return_quantity[#{variant.id}]",
2725
@return_authorization.inventory_units.group_by(&:variant)[variant].try(:size) || 0, {:style => 'width:100px;', :min => 0} %>
26+
<% else %>
27+
<%= @return_authorization.inventory_units.group_by(&:variant)[variant].try(:size) || 0 %>
2828
<% end %>
2929
</td>
3030
</tr>
@@ -34,11 +34,11 @@
3434

3535
<%= f.field_container :amount do %>
3636
<%= f.label :amount, Spree.t(:amount) %> <span class="required">*</span><br />
37-
<% if @return_authorization.received? %>
38-
<%= @return_authorization.display_amount %>
39-
<% else %>
37+
<% if @return_authorization.updatable? %>
4038
<%= f.text_field :amount, {:style => 'width:80px;'} %> <%= Spree.t(:rma_value) %>: <span id="rma_value">0.00</span>
4139
<%= f.error_message_on :amount %>
40+
<% else %>
41+
<%= @return_authorization.display_amount %>
4242
<% end %>
4343
<% end %>
4444

backend/app/views/spree/admin/return_authorizations/edit.html.erb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@
99
<%= button_link_to Spree.t('actions.cancel'), fire_admin_order_return_authorization_url(@order, @return_authorization, :e => 'cancel'), :method => :put, :data => { :confirm => Spree.t(:are_you_sure) }, :icon => 'remove' %>
1010
<% end %>
1111
</li>
12+
<li>
13+
<% if @return_authorization.can_refund? %>
14+
<%= button_link_to Spree.t('actions.refund'), fire_admin_order_return_authorization_url(@order, @return_authorization, :e => 'refund'), :method => :put, :data => { :confirm => Spree.t(:are_you_sure) }, :icon => 'reply' %>
15+
<% end %>
16+
</li>
1217
<% end %>
1318

1419
<%= render :partial => 'spree/admin/shared/order_tabs', :locals => { :current => 'Return Authorizations' } %>

core/app/models/spree/payment.rb

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ class Payment < Spree::Base
1515
has_many :log_entries, as: :source
1616
has_many :state_changes, as: :stateful
1717
has_many :capture_events, :class_name => 'Spree::PaymentCaptureEvent'
18+
has_many :refunds, inverse_of: :payment
1819

1920
before_validation :validate_source
2021
before_create :set_unique_identifier
@@ -43,6 +44,11 @@ class Payment < Spree::Base
4344

4445
validates :amount, numericality: true
4546

47+
# transaction_id is much easier to understand
48+
def transaction_id
49+
response_code
50+
end
51+
4652
def persist_invalid
4753
return unless ['failed', 'invalid'].include?(state)
4854
state_will_change!
@@ -112,7 +118,7 @@ def offsets_total
112118
end
113119

114120
def credit_allowed
115-
amount - offsets_total.abs
121+
amount - (offsets_total.abs + refunds.sum(:amount))
116122
end
117123

118124
def can_credit?

core/app/models/spree/payment/processing.rb

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -80,35 +80,8 @@ def void_transaction!
8080
end
8181
end
8282

83-
def credit!(credit_amount=nil)
84-
protect_from_connection_error do
85-
check_environment
86-
87-
credit_amount ||= credit_allowed >= order.outstanding_balance.abs ? order.outstanding_balance.abs : credit_allowed.abs
88-
credit_amount = credit_amount.to_f
89-
credit_cents = Spree::Money.new(credit_amount, currency: currency).money.cents
90-
91-
if payment_method.payment_profiles_supported?
92-
response = payment_method.credit(credit_cents, source, response_code, gateway_options)
93-
else
94-
response = payment_method.credit(credit_cents, response_code, gateway_options)
95-
end
96-
97-
record_response(response)
98-
99-
if response.success?
100-
self.class.create!(
101-
:order => order,
102-
:source => self,
103-
:payment_method => payment_method,
104-
:amount => credit_amount.abs * -1,
105-
:response_code => response.authorization,
106-
:state => 'completed'
107-
)
108-
else
109-
gateway_error(response)
110-
end
111-
end
83+
def credit!(credit_amount=nil, return_authorization=nil)
84+
Spree::Refund.perform!(self, (credit_amount || credit_allowed).to_f, return_authorization)
11285
end
11386

11487
def cancel!

core/app/models/spree/refund.rb

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
module Spree
2+
class Refund < Spree::Base
3+
belongs_to :payment, inverse_of: :refunds
4+
belongs_to :return_authorization # optional
5+
6+
has_many :log_entries, as: :source
7+
8+
validates :payment, presence: true
9+
validates :transaction_id, presence: true
10+
validates :amount, presence: true, numericality: {greater_than: 0}
11+
12+
# attempts to perform the refund
13+
# if successful it returns the refund, otherwise it raises
14+
def self.perform!(payment, amount, return_authorization=nil)
15+
check_amount(amount)
16+
check_environment(payment)
17+
18+
credit_cents = Spree::Money.new(amount.to_f, currency: payment.currency).money.cents
19+
20+
response = process!(payment, credit_cents)
21+
22+
refund = create!({
23+
payment: payment,
24+
return_authorization: return_authorization,
25+
transaction_id: response.authorization,
26+
amount: amount,
27+
})
28+
29+
refund.log_entries.create!(details: response.to_yaml)
30+
31+
refund
32+
end
33+
34+
# return an activemerchant response object if successful or else raise an error
35+
def self.process!(payment, credit_cents)
36+
response = if payment.payment_method.payment_profiles_supported?
37+
payment.payment_method.credit(credit_cents, payment.source, payment.transaction_id, {})
38+
else
39+
payment.payment_method.credit(credit_cents, payment.transaction_id, {})
40+
end
41+
42+
if !response.success?
43+
logger.error(Spree.t(:gateway_error) + " #{response.to_yaml}")
44+
text = response.params['message'] || response.params['response_reason_text'] || response.message
45+
raise Core::GatewayError.new(text)
46+
end
47+
48+
response
49+
rescue ActiveMerchant::ConnectionError => e
50+
logger.error(Spree.t(:gateway_error) + " #{e.inspect}")
51+
raise Core::GatewayError.new(Spree.t(:unable_to_connect_to_gateway))
52+
end
53+
54+
# Saftey check to make sure we're not accidentally performing operations on a live gateway.
55+
# Ex. When testing in staging environment with a copy of production data.
56+
def self.check_environment(payment)
57+
return if payment.payment_method.environment == Rails.env
58+
message = Spree.t(:gateway_config_unavailable) + " - #{Rails.env}"
59+
raise Core::GatewayError.new(message)
60+
end
61+
62+
def self.check_amount(amount)
63+
unless amount > 0
64+
raise Core::GatewayError.new(Spree.t(:refund_amount_must_be_greater_than_zero))
65+
end
66+
end
67+
68+
def money
69+
Spree::Money.new(amount, { currency: payment.currency })
70+
end
71+
alias display_amount money
72+
73+
end
74+
end

core/app/models/spree/return_authorization.rb

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,42 @@ class ReturnAuthorization < Spree::Base
33
belongs_to :order, class_name: 'Spree::Order'
44

55
has_many :inventory_units
6+
has_many :refunds
67
belongs_to :stock_location
78
before_create :generate_number
8-
before_save :force_positive_amount
9+
before_validation :force_positive_amount
910

1011
validates :order, presence: true
11-
validates :amount, numericality: true
12+
validates :amount, numericality: { greater_than_or_equal_to: 0 }
1213
validate :must_have_shipped_units
1314

1415
state_machine initial: :authorized do
1516
after_transition to: :received, do: :process_return
17+
before_transition to: :refunded, do: :process_refund
1618

1719
event :receive do
1820
transition to: :received, from: :authorized, if: :allow_receive?
1921
end
22+
2023
event :cancel do
2124
transition to: :canceled, from: :authorized
2225
end
26+
27+
event :refund do
28+
transition to: :refunded, from: :received
29+
end
30+
31+
state all - [:received, :refunded] do
32+
def updatable?
33+
true
34+
end
35+
end
36+
37+
state :received, :refunded do
38+
def updatable?
39+
false
40+
end
41+
end
2342
end
2443

2544
def currency
@@ -67,6 +86,28 @@ def compute_amount(*args)
6786
amount.abs * -1
6887
end
6988

89+
def amount_due
90+
amount - refunds.sum(:amount)
91+
end
92+
93+
def process_refund
94+
# For now type and order of retrieved payments are not specified
95+
order.payments.completed.each do |payment|
96+
break if amount_due <= 0
97+
credit_allowed = [payment.credit_allowed, amount_due].min
98+
payment.credit!(credit_allowed, self)
99+
end
100+
101+
case amount_due
102+
when 0
103+
return true
104+
when ->(x) { x < 0 }
105+
errors.add(:base, :amount_due_less_than_zero) and return false
106+
when ->(x) { x > 0 }
107+
errors.add(:base, :amount_due_greater_than_zero) and return false
108+
end
109+
end
110+
70111
private
71112

72113
def must_have_shipped_units
@@ -91,9 +132,6 @@ def process_return
91132
end
92133
end
93134

94-
Adjustment.create(adjustable: order, amount: compute_amount, label: Spree.t(:rma_credit), source: self)
95-
order.update!
96-
97135
order.return if inventory_units.all?(&:returned?)
98136
end
99137

0 commit comments

Comments
 (0)