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 Spree::Shipment#item_cost for split shipments #18

Open
wants to merge 4 commits 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
2 changes: 1 addition & 1 deletion api/app/views/spree/api/shipments/_small.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ json.cache! [I18n.locale, shipment] do
json.(shipping_category, :id, :name)
end
end
json.manifest(shipment.manifest) do |manifest_item|
json.manifest(shipment.shipping_manifest_items) do |manifest_item|
json.variant_id manifest_item.variant.id
json.quantity(manifest_item.quantity)
json.states(manifest_item.states)
Expand Down
2 changes: 1 addition & 1 deletion api/app/views/spree/api/shipments/show.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ json.cache! [I18n.locale, @shipment] do
json.(shipping_category, :id, :name)
end
end
json.manifest(@shipment.manifest) do |manifest_item|
json.manifest(@shipment.shipping_manifest_items) do |manifest_item|
json.variant do
json.partial!("spree/api/variants/small", variant: manifest_item.variant)
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<% shipment.manifest.each do |item| %>
<% shipment.shipping_manifest_items.each do |item| %>
<tr class="stock-item" data-item-quantity="<%= item.quantity %>">
<td class="item-image">
<%= render 'spree/admin/shared/image',
Expand Down
2 changes: 1 addition & 1 deletion backend/spec/features/admin/orders/order_details_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@
visit spree.edit_admin_order_path(order)

expect(order.shipments.count).to eq(1)
expect(order.shipments.first.manifest.count).to eq(2)
expect(order.shipments.first.shipping_manifest_items.count).to eq(2)

within('tr', text: line_item.sku) { click_icon 'arrows-h' }
complete_split_to(stock_location2)
Expand Down
2 changes: 1 addition & 1 deletion core/app/mailers/spree/carton_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class CartonMailer < BaseMailer
def shipped_email(options, _deprecated_options = {})
@order = options.fetch(:order)
@carton = options.fetch(:carton)
@manifest = @carton.manifest_for_order(@order)
@shipping_manifest_for_order = @carton.manifest_for_order(@order)
options = { resend: false }.merge(options)
@store = @order.store
subject = (options[:resend] ? "[#{t('spree.resend').upcase}] " : '')
Expand Down
20 changes: 16 additions & 4 deletions core/app/models/spree/carton.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,27 @@ def display_shipped_at
shipped_at.to_s(:rfc822)
end

def manifest
@manifest ||= Spree::ShippingManifest.new(inventory_units: inventory_units).items
def shipping_manifest_items
@shipping_manifest_items ||= shipping_manifest.items
end

def manifest_for_order(order)
Spree::ShippingManifest.new(inventory_units: inventory_units).for_order(order).items
alias manifest shipping_manifest_items
deprecate manifest: :shipping_manifest_items, deprecator: Spree::Deprecation

def shipping_manifest_for_order(order)
shipping_manifest.for_order(order).items
end

alias manifest_for_order shipping_manifest_for_order
deprecate manifest_for_order: :shipping_manifest_for_order, deprecator: Spree::Deprecation

def any_exchanges?
inventory_units.any?(&:original_return_item)
end

private

def shipping_manifest
@shipping_manifest ||= Spree::ShippingManifest.new(inventory_units: inventory_units)
end
end
17 changes: 12 additions & 5 deletions core/app/models/spree/shipment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,11 @@ def add_shipping_method(shipping_method, selected = false)
deprecate :add_shipping_method, deprecator: Spree::Deprecation

def after_cancel
manifest.each { |item| manifest_restock(item) }
shipping_manifest_items.each { |item| manifest_restock(item) }
end

def after_resume
manifest.each { |item| manifest_unstock(item) }
shipping_manifest_items.each { |item| manifest_unstock(item) }
end

def backordered?
Expand Down Expand Up @@ -184,7 +184,7 @@ def inventory_units_for_item(line_item, variant = nil)
end

def item_cost
line_items.map(&:total).sum
manifest.sum(&:item_cost)
end

def ready_or_pending?
Expand Down Expand Up @@ -228,10 +228,13 @@ def selected_shipping_rate
shipping_rates.detect(&:selected?)
end

def manifest
@manifest ||= Spree::ShippingManifest.new(inventory_units: inventory_units).items
def shipping_manifest_items
@shipping_manifest_items ||= shipping_manifest.items
end

alias manifest shipping_manifest_items
deprecate manifest: :shipping_manifest_items, deprecator: Spree::Deprecation

def selected_shipping_rate_id
selected_shipping_rate.try(:id)
end
Expand Down Expand Up @@ -398,6 +401,10 @@ def finalize_pending_inventory_units
Spree::Stock::InventoryUnitsFinalizer.new(pending_units).run!
end

def shipping_manifest
@shipping_manifest ||= Spree::ShippingManifest.new(inventory_units: inventory_units)
end

def after_ship
order.shipping.ship_shipment(self, suppress_mailer: suppress_mailer)
end
Expand Down
6 changes: 5 additions & 1 deletion core/app/models/spree/shipping_manifest.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
# frozen_string_literal: true

class Spree::ShippingManifest
ManifestItem = Struct.new(:line_item, :variant, :quantity, :states)
ManifestItem = Struct.new(:line_item, :variant, :quantity, :states) do
def item_cost
(line_item.price + (line_item.adjustment_total / line_item.quantity)) * quantity
end
end

def initialize(inventory_units:)
@inventory_units = inventory_units.to_a
Expand Down
2 changes: 1 addition & 1 deletion core/app/views/spree/carton_mailer/shipped_email.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<%= t('spree.shipment_mailer.shipped_email.shipment_summary') %>
</p>
<table>
<% @manifest.each do |item| %>
<% @shipping_manifest_for_order.each do |item| %>
<tr>
<td><%= item.variant.sku %></td>
<td><%= item.variant.product.name %></td>
Expand Down
2 changes: 1 addition & 1 deletion core/app/views/spree/carton_mailer/shipped_email.text.erb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
============================================================
<%= t('spree.shipment_mailer.shipped_email.shipment_summary') %>
============================================================
<% @manifest.each do |item| %>
<% @shipping_manifest_for_order.each do |item| %>
<%= item.variant.sku %> <%= item.variant.product.name %> <%= item.variant.options_text %>
<% end %>
============================================================
Expand Down
20 changes: 19 additions & 1 deletion core/spec/models/spree/carton_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,16 @@
end

describe "#manifest" do
subject { carton.manifest }
it "is deprecated" do
Spree::Deprecation.silence do
expect(Spree::Deprecation).to(receive(:warn))
Spree::Shipment.new.manifest
end
end
end

describe "#shipping_manifest_items" do
subject { carton.shipping_manifest_items }

let(:carton) { create(:carton, inventory_units: [first_order.inventory_units, second_order.inventory_units].flatten) }
let(:first_order) { create(:order_ready_to_ship, line_items_count: 1) }
Expand All @@ -97,6 +106,15 @@
end

describe "#manifest_for_order" do
it "is deprecated" do
Spree::Deprecation.silence do
expect(Spree::Deprecation).to(receive(:warn))
Spree::Carton.new.manifest_for_order(create(:order))
end
end
end

describe "#shipping_manifest_for_order" do
subject { carton.manifest_for_order(first_order) }

let(:carton) { create(:carton, inventory_units: [first_order.inventory_units, second_order.inventory_units].flatten) }
Expand Down
35 changes: 32 additions & 3 deletions core/spec/models/spree/shipment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,26 @@
it 'should equal line items final amount with tax' do
expect(shipment.item_cost).to eql(11.0)
end

context 'with more shipments for the same line_item' do
let(:order) do
create(
:order_with_line_items,
line_items_attributes: [{ price: 10, variant: variant, quantity: 2 }],
ship_address: ship_address,
)
end
let(:other_shipment) { order.shipments.create(stock_location_id: shipment.stock_location) }

it 'returns shipment line items amount with tax' do
expect(order.shipments.first.item_cost).to eql(22.0)
expect {
order.inventory_units.last.update(shipment_id: other_shipment.id)
}.to change { order.shipments.reload.first.item_cost }.from(22.0).to(11.0)

expect(order.shipments.second.item_cost).to eql(11.0)
end
end
end

it "#discounted_cost" do
Expand Down Expand Up @@ -187,21 +207,30 @@
expect(shipment.total).to eq(8)
end

context "manifest" do
describe "#manifest_items" do
let(:order) { create(:order) }
let(:variant) { create(:variant) }
let!(:line_item) { order.contents.add variant }
let!(:shipment) { order.create_proposed_shipments.first }

it "returns variant expected" do
expect(shipment.manifest.first.variant).to eq variant
expect(shipment.shipping_manifest_items.first.variant).to eq variant
end

context "variant was removed" do
before { variant.discard }

it "still returns variant expected" do
expect(shipment.manifest.first.variant).to eq variant
expect(shipment.shipping_manifest_items.first.variant).to eq variant
end
end
end

context "manifest" do
it "is deprecated" do
Spree::Deprecation.silence do
expect(Spree::Deprecation).to(receive(:warn))
Spree::Shipment.new.manifest
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion frontend/app/views/spree/checkout/_delivery.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<th><%= t('spree.price') %></th>
</thead>
<tbody>
<% ship_form.object.manifest.each do |item| %>
<% ship_form.object.shipping_manifest_items.each do |item| %>
<tr class="stock-item">
<td class="item-image">
<%= render 'spree/shared/image',
Expand Down