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

Remove default address dependency #2686

Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class ReimbursementsController < ResourceController
rescue_from Spree::Core::GatewayError, with: :spree_core_gateway_error

def perform
@reimbursement.perform!
@reimbursement.perform!(try_spree_current_user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A named argument would explain the purpose of this argument:

@reimbursement.perform!(creator: try_spree_current_user)

redirect_to location_after_save
end

Expand Down Expand Up @@ -57,7 +57,7 @@ def load_stock_locations
end

def load_simulated_refunds
@reimbursement_objects = @reimbursement.simulate
@reimbursement_objects = @reimbursement.simulate(try_spree_current_user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well

@reimbursement.simulate(creator: try_spree_current_user)

end

def spree_core_gateway_error(error)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@
Spree::RefundReason.find_or_create_by!(name: Spree::RefundReason::RETURN_PROCESSING_REASON, mutable: false)
end

let(:user) { stub_model(Spree::LegacyUser, has_spree_role?: true, id: 1) }

before do
allow_any_instance_of(described_class).to receive(:try_spree_current_user).
and_return(user)
end

describe '#edit' do
let(:reimbursement) { create(:reimbursement) }
let(:order) { reimbursement.order }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@

before do
Spree::RefundReason.create!(name: Spree::RefundReason::RETURN_PROCESSING_REASON, mutable: false)
allow_any_instance_of(Spree::Admin::ReimbursementsController).to receive(:try_spree_current_user).
and_return(user)
end

let!(:order) { create(:shipped_order) }
let(:user) { create(:admin_user) }

# Regression test for https://github.com/spree/spree/issues/6229
it "refunds and has outstanding_balance of zero", js: true do
Expand Down
8 changes: 6 additions & 2 deletions core/app/models/spree/order_cancellations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,17 @@ def cancel_unit(inventory_unit, reason: Spree::UnitCancel::DEFAULT_REASON, whodu
# @api public
# @param [Array<InventoryUnit>] inventory_units the inventory units to be reimbursed
# @return [Reimbursement] the reimbursement for inventory being canceled
def reimburse_units(inventory_units)
def reimburse_units(inventory_units, creator = nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use a named argument here?

def reimburse_units(inventory_units, creator: nil)

unless creator
creator = Spree.user_class.where(email: 'spree@example.com').first
Spree::Deprecation.warn("Calling #reimburse_units on #{self} without creator is deprecated")
end
reimbursement = nil

Spree::OrderMutex.with_lock!(@order) do
return_items = inventory_units.map(&:current_or_new_return_item)
reimbursement = Spree::Reimbursement.new(order: @order, return_items: return_items)
reimbursement.return_all
reimbursement.return_all(creator)
end

reimbursement
Expand Down
24 changes: 18 additions & 6 deletions core/app/models/spree/reimbursement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,16 @@ def unpaid_amount
total - paid_amount
end

def perform!
def perform!(creator = nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A named parameter would help the self-documentation of this method.

unless creator
creator = Spree.user_class.where(email: 'spree@example.com').first
Spree::Deprecation.warn("Calling #perform on #{self} without creator is deprecated")
end
reimbursement_tax_calculator.call(self)
reload
update!(total: calculated_total)

reimbursement_performer.perform(self)
reimbursement_performer.perform(self, creator)

if unpaid_amount_within_tolerance?
reimbursed!
Expand All @@ -116,12 +120,16 @@ def perform!
end
end

def simulate
def simulate(creator = nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: Named argument

unless creator
creator = Spree.user_class.where(email: 'spree@example.com').first
Spree::Deprecation.warn("Calling #simulate on #{self} without creator is deprecated")
end
reimbursement_simulator_tax_calculator.call(self)
reload
update!(total: calculated_total)

reimbursement_performer.simulate(self)
reimbursement_performer.simulate(self, creator)
end

def return_items_requiring_exchange
Expand All @@ -140,10 +148,14 @@ def all_exchanges?
#
# @api public
# @return [void]
def return_all
def return_all(creator = nil)
unless creator
creator = Spree.user_class.where(email: 'spree@example.com').first
Spree::Deprecation.warn("Calling #return_all on #{self} without creator is deprecated")
end
return_items.each(&:accept!)
save!
perform!
perform!(creator)
end

private
Expand Down
20 changes: 14 additions & 6 deletions core/app/models/spree/reimbursement_performer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,30 @@ class << self
# - #description
# - #display_amount
# so they can be displayed in the Admin UI appropriately.
def simulate(reimbursement)
execute(reimbursement, true)
def simulate(reimbursement, creator = nil)
unless creator
creator = Spree.user_class.where(email: 'spree@example.com').first
Spree::Deprecation.warn("Calling #simulate on #{self} without creator is deprecated")
end
execute(reimbursement, true, creator)
end

# Actually perform the reimbursement
def perform(reimbursement)
execute(reimbursement, false)
def perform(reimbursement, creator = nil)
unless creator
creator = Spree.user_class.find_by(email: 'spree@example.com')
Spree::Deprecation.warn("Calling #perform on #{self} without creator is deprecated")
end
execute(reimbursement, false, creator)
end

private

def execute(reimbursement, simulate)
def execute(reimbursement, simulate, creator)
reimbursement_type_hash = calculate_reimbursement_types(reimbursement)

reimbursement_type_hash.flat_map do |reimbursement_type, return_items|
reimbursement_type.reimburse(reimbursement, return_items, simulate)
reimbursement_type.reimburse(reimbursement, return_items, simulate, creator)
end
end

Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/reimbursement_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class ReimbursementType < Spree::Base
# This method will reimburse the return items based on however it child implements it
# By default it takes a reimbursement, the return items it needs to reimburse, and if
# it is a simulation or a real reimbursement. This should return an array
def self.reimburse(_reimbursement, _return_items, _simulate)
def self.reimburse(_reimbursement, _return_items, _simulate, _creator)
raise "Implement me"
end
end
Expand Down
8 changes: 6 additions & 2 deletions core/app/models/spree/reimbursement_type/credit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@ class ReimbursementType::Credit < Spree::ReimbursementType
extend Spree::ReimbursementType::ReimbursementHelpers

class << self
def reimburse(reimbursement, return_items, simulate)
def reimburse(reimbursement, return_items, simulate, creator = nil)
unless creator
creator = Spree.user_class.where(email: 'spree@example.com').first
Spree::Deprecation.warn("Calling #reimburse on #{self} without creator is deprecated")
end
unpaid_amount = return_items.sum(&:total).round(2, :down)
reimbursement_list, _unpaid_amount = create_credits(reimbursement, unpaid_amount, simulate)
reimbursement_list, _unpaid_amount = create_credits(reimbursement, unpaid_amount, simulate, creator)
reimbursement_list
end
end
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/reimbursement_type/exchange.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

class Spree::ReimbursementType::Exchange < Spree::ReimbursementType
def self.reimburse(reimbursement, return_items, simulate)
def self.reimburse(reimbursement, return_items, simulate, _creator)
return [] unless return_items.present?

exchange = Spree::Exchange.new(reimbursement.order, return_items)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Spree::ReimbursementType::OriginalPayment < Spree::ReimbursementType
extend Spree::ReimbursementType::ReimbursementHelpers

class << self
def reimburse(reimbursement, return_items, simulate)
def reimburse(reimbursement, return_items, simulate, _creator)
unpaid_amount = return_items.sum(&:total).round(2, :down)
payments = reimbursement.order.payments.completed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ def create_refunds(reimbursement, payments, unpaid_amount, simulate, reimburseme
[reimbursement_list, unpaid_amount]
end

def create_credits(reimbursement, unpaid_amount, simulate, reimbursement_list = [])
credits = [create_credit(reimbursement, unpaid_amount, simulate)]
def create_credits(reimbursement, unpaid_amount, simulate, creator, reimbursement_list = [])
credits = [create_credit(reimbursement, unpaid_amount, simulate, creator)]
unpaid_amount -= credits.sum(&:amount)
reimbursement_list += credits

Expand All @@ -43,19 +43,19 @@ def create_refund(reimbursement, payment, amount, simulate)

# If you have multiple methods of crediting a customer, overwrite this method
# Must return an array of objects the respond to #description, #display_amount
def create_credit(reimbursement, unpaid_amount, simulate)
creditable = create_creditable(reimbursement, unpaid_amount)
def create_credit(reimbursement, unpaid_amount, simulate, creator)
creditable = create_creditable(reimbursement, unpaid_amount, creator)
credit = reimbursement.credits.build(creditable: creditable, amount: unpaid_amount)
simulate ? credit.readonly! : credit.save!
credit
end

def create_creditable(reimbursement, unpaid_amount)
def create_creditable(reimbursement, unpaid_amount, creator)
Spree::Reimbursement::Credit.default_creditable_class.new(
user: reimbursement.order.user,
amount: unpaid_amount,
category: Spree::StoreCreditCategory.reimbursement_category(reimbursement),
created_by: Spree::StoreCredit.default_created_by,
created_by: creator,
memo: "Refund for uncreditable payments on order #{reimbursement.order.number}",
currency: reimbursement.order.currency
)
Expand Down
22 changes: 19 additions & 3 deletions core/app/models/spree/reimbursement_type/store_credit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,33 @@ class Spree::ReimbursementType::StoreCredit < Spree::ReimbursementType
extend Spree::ReimbursementType::ReimbursementHelpers

class << self
def reimburse(reimbursement, return_items, simulate)
def reimburse(reimbursement, return_items, simulate, creator = nil)
unless creator
creator = Spree.user_class.where(email: 'spree@example.com').first
Spree::Deprecation.warn("Calling #reimburse on #{self} without creator is deprecated")
end
unpaid_amount = return_items.sum(&:total).to_d.round(2, :down)
payments = store_credit_payments(reimbursement)
reimbursement_list = []

# Credit each store credit that was used on the order
reimbursement_list, unpaid_amount = create_refunds(reimbursement, payments, unpaid_amount, simulate, reimbursement_list)
reimbursement_list, unpaid_amount = create_refunds(
reimbursement,
payments,
unpaid_amount,
simulate,
reimbursement_list
)

# If there is any amount left to pay out to the customer, then create credit with that amount
if unpaid_amount > 0.0
reimbursement_list, _unpaid_amount = create_credits(reimbursement, unpaid_amount, simulate, reimbursement_list)
reimbursement_list, _unpaid_amount = create_credits(
reimbursement,
unpaid_amount,
simulate,
creator,
reimbursement_list
)
end

reimbursement_list
Expand Down
8 changes: 0 additions & 8 deletions core/app/models/spree/store_credit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ class Spree::StoreCredit < Spree::PaymentSource
ADJUSTMENT_ACTION = 'adjustment'
INVALIDATE_ACTION = 'invalidate'

DEFAULT_CREATED_BY_EMAIL = "spree@example.com"

belongs_to :user, class_name: Spree::UserClassHandle.new
belongs_to :created_by, class_name: Spree::UserClassHandle.new
belongs_to :category, class_name: "Spree::StoreCreditCategory"
Expand Down Expand Up @@ -196,12 +194,6 @@ def invalidate(reason, user_performing_invalidation)
end
end

class << self
def default_created_by
Spree.user_class.find_by(email: DEFAULT_CREATED_BY_EMAIL)
end
end

private

def create_credit_record(amount, action_attributes = {})
Expand Down
3 changes: 2 additions & 1 deletion core/spec/models/spree/customer_return_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,8 @@
end

context 'when all reimbursements are reimbursed' do
before { reimbursement.perform! }
let(:created_by_user) { create(:user, email: 'user@email.com') }
before { reimbursement.perform!(created_by_user) }

it { is_expected.to be true }
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,14 @@
context 'with a cancelled item' do
let(:cancelations) { Spree::OrderCancellations.new(order) }
let(:cancelled_item) { item_1 }
let(:created_by_user) { create(:user, email: 'user@email.com') }

before do
# Required to refund
Spree::RefundReason.create!(name: Spree::RefundReason::RETURN_PROCESSING_REASON, mutable: false)

cancelations.cancel_unit(cancelled_item.inventory_units.first)
cancelations.reimburse_units(cancelled_item.inventory_units)
cancelations.reimburse_units(cancelled_item.inventory_units, created_by_user)

order.reload
end
Expand Down
3 changes: 2 additions & 1 deletion core/spec/models/spree/order_cancellations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,11 @@
end

describe "#reimburse_units" do
subject { Spree::OrderCancellations.new(order).reimburse_units(inventory_units) }
subject { Spree::OrderCancellations.new(order).reimburse_units(inventory_units, created_by_user) }
let(:order) { create(:shipped_order, line_items_count: 2) }
let(:inventory_units) { order.inventory_units }
let!(:default_refund_reason) { Spree::RefundReason.find_or_create_by!(name: Spree::RefundReason::RETURN_PROCESSING_REASON, mutable: false) }
let(:created_by_user) { create(:user, email: 'user@email.com') }

it "creates and performs a reimbursement" do
expect { subject }.to change { Spree::Reimbursement.count }.by(1)
Expand Down
3 changes: 2 additions & 1 deletion core/spec/models/spree/refund_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,12 @@
let(:customer_return) { reimbursement.customer_return }
let(:reimbursement) { create(:reimbursement) }
let!(:default_refund_reason) { Spree::RefundReason.find_or_create_by!(name: Spree::RefundReason::RETURN_PROCESSING_REASON, mutable: false) }
let(:created_by_user) { create(:user, email: 'user@email.com') }

subject { Spree::Refund.total_amount_reimbursed_for(reimbursement) }

context 'with reimbursements performed' do
before { reimbursement.perform! }
before { reimbursement.perform!(created_by_user) }

it 'returns the total amount' do
amount = Spree::Refund.total_amount_reimbursed_for(reimbursement)
Expand Down
9 changes: 5 additions & 4 deletions core/spec/models/spree/reimbursement_performer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,26 @@
let(:return_item) { reimbursement.return_items.first }
let(:reimbursement_type) { double("ReimbursementType") }
let(:reimbursement_type_hash) { { reimbursement_type => [return_item] } }
let(:created_by_user) { create(:user, email: 'user@email.com') }

before do
expect(Spree::ReimbursementPerformer).to receive(:calculate_reimbursement_types).and_return(reimbursement_type_hash)
end

describe ".simulate" do
subject { Spree::ReimbursementPerformer.simulate(reimbursement) }
subject { Spree::ReimbursementPerformer.simulate(reimbursement, created_by_user) }

it "reimburses each calculated reimbursement types with the correct return items as a simulation" do
expect(reimbursement_type).to receive(:reimburse).with(reimbursement, [return_item], true)
expect(reimbursement_type).to receive(:reimburse).with(reimbursement, [return_item], true, created_by_user)
subject
end
end

describe '.perform' do
subject { Spree::ReimbursementPerformer.perform(reimbursement) }
subject { Spree::ReimbursementPerformer.perform(reimbursement, created_by_user) }

it "reimburses each calculated reimbursement types with the correct return items as a simulation" do
expect(reimbursement_type).to receive(:reimburse).with(reimbursement, [return_item], false)
expect(reimbursement_type).to receive(:reimburse).with(reimbursement, [return_item], false, created_by_user)
subject
end
end
Expand Down
Loading