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

Stop passing line_item to compute unit cancelation adjustment's amount #5278

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
28 changes: 16 additions & 12 deletions core/app/models/spree/unit_cancel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,19 @@ class Spree::UnitCancel < Spree::Base
DEFAULT_REASON = 'Cancel'

belongs_to :inventory_unit, optional: true

has_one :adjustment, as: :source, dependent: :destroy
has_one :line_item, through: :inventory_unit

validates :inventory_unit, presence: true

# Creates necessary cancel adjustments for the line item.
def adjust!
raise "Adjustment is already created" if adjustment

amount = compute_amount(inventory_unit.line_item)
amount = compute_amount

self.adjustment = inventory_unit.line_item.adjustments.create!(
self.adjustment = line_item.adjustments.create!(
source: self,
amount: amount,
order: inventory_unit.order,
Expand All @@ -27,27 +29,29 @@ def adjust!
finalized: true
)

inventory_unit.line_item.order.recalculate
line_item.order.recalculate
adjustment
end

# This method is used by Adjustment#update to recalculate the cost.
def compute_amount(line_item)
raise "Adjustable does not match line item" unless line_item == inventory_unit.line_item

-weighted_line_item_amount(line_item)
def compute_amount
Copy link
Member

Choose a reason for hiding this comment

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

compute_amount is a public method. We have two ways of handling the change of the signature of a public method:

  1. Rename the method and deprecate compute_amount, delegating its usage to the new method.
  2. Keep compute_amount with the same signature but make the argument optional and deprecate its usage.

I think it works both ways, up to you!

-weighted_amount(line_item.total_before_tax)
end

private

def weighted_line_item_amount(line_item)
quantity_of_line_item = quantity_of_line_item(line_item)
raise ZeroDivisionError, "Line Item does not have any inventory units available to cancel" if quantity_of_line_item.zero?
def weighted_amount(amount)
quantity = quantity_of_line_item

if quantity.zero?
raise ZeroDivisionError,
'Line Item does not have any inventory units available to cancel'
end

line_item.total_before_tax / quantity_of_line_item
amount / quantity
end

def quantity_of_line_item(line_item)
def quantity_of_line_item
BigDecimal(line_item.inventory_units.not_canceled.reject(&:original_return_item).size)
end
end
14 changes: 3 additions & 11 deletions core/spec/models/spree/unit_cancel_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@
end

describe '#compute_amount' do
subject { unit_cancel.compute_amount(line_item) }
subject(:amount) { unit_cancel.compute_amount }

let(:line_item) { inventory_unit.line_item }
let!(:inventory_unit2) { create(:inventory_unit, line_item: inventory_unit.line_item) }
let!(:inventory_unit2) { create :inventory_unit, line_item: inventory_unit.line_item }

context "all inventory on the line item are not canceled" do
it "divides the line item total by the inventory units size" do
expect(subject).to eq(-5.0)
expect(amount).to eq(-5.0)
end
end

Expand All @@ -55,14 +55,6 @@
end
end

context "it is called with a line item that doesnt belong to the inventory unit" do
let(:line_item) { create(:line_item) }

it "raises an error" do
expect { subject }.to raise_error RuntimeError, "Adjustable does not match line item"
end
end

context "multiple inventory units" do
let(:quantity) { 4 }
let(:order) { create(:order_with_line_items, line_items_attributes: [{ quantity: quantity }]) }
Expand Down