From e1478ce2c7dfbb3b56393829bed0cb43e697833c Mon Sep 17 00:00:00 2001 From: Patrick Sinclair Date: Tue, 10 May 2016 16:28:54 -0400 Subject: [PATCH 1/5] Remove Spree::Shipment#address association. We found that if an order address was updated after checkout through the admin, the shipment address was not updated. It turns out that the Spree::Shipment#address was not actually being used for anything in particular, so it feels better to remove the association and delegate to the Spree::Order#ship_address instead. --- core/app/models/spree/order_shipping.rb | 2 +- core/app/models/spree/shipment.rb | 7 +++++-- core/app/models/spree/stock/estimator.rb | 2 +- core/app/models/spree/stock/package.rb | 1 - core/lib/spree/testing_support/factories/order_factory.rb | 4 ++-- .../migrations/copy_shipped_shipments_to_cartons.rake | 4 +++- core/spec/models/spree/order_shipping_spec.rb | 7 +++---- core/spec/models/spree/shipment_spec.rb | 4 +--- core/spec/models/spree/stock/package_spec.rb | 1 - 9 files changed, 16 insertions(+), 16 deletions(-) diff --git a/core/app/models/spree/order_shipping.rb b/core/app/models/spree/order_shipping.rb index 418af505870..e87b5509743 100644 --- a/core/app/models/spree/order_shipping.rb +++ b/core/app/models/spree/order_shipping.rb @@ -17,7 +17,7 @@ def ship_shipment(shipment, external_number: nil, tracking_number: nil, suppress ship( inventory_units: shipment.inventory_units.shippable, stock_location: shipment.stock_location, - address: shipment.address, + address: shipment.order.ship_address, shipping_method: shipment.shipping_method, shipped_at: Time.current, external_number: external_number, diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index d082b0cf471..08046a7087c 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -1,7 +1,6 @@ module Spree class Shipment < Spree::Base belongs_to :order, class_name: 'Spree::Order', touch: true, inverse_of: :shipments - belongs_to :address, class_name: 'Spree::Address' belongs_to :stock_location, class_name: 'Spree::StockLocation' has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :delete_all @@ -21,7 +20,6 @@ class Shipment < Spree::Base # from outside of the state machine and can actually pass variables through. attr_accessor :special_instructions, :suppress_mailer - accepts_nested_attributes_for :address accepts_nested_attributes_for :inventory_units make_permalink field: :number, length: 11, prefix: 'H' @@ -372,6 +370,11 @@ def requires_shipment? !stock_location || stock_location.fulfillable? end + def address + ActiveSupport::Deprecation.warn("Calling Shipment#address is deprecated. Use Order#ship_address instead", caller) + order.ship_address if order + end + private def after_ship diff --git a/core/app/models/spree/stock/estimator.rb b/core/app/models/spree/stock/estimator.rb index bb3f1be012f..cc071c4c145 100644 --- a/core/app/models/spree/stock/estimator.rb +++ b/core/app/models/spree/stock/estimator.rb @@ -45,7 +45,7 @@ def calculate_shipping_rates(package) def shipping_methods(package) package.shipping_methods - .available_for_address(package.shipment.address) + .available_for_address(package.shipment.order.ship_address) .includes(:calculator, tax_category: :tax_rates) .to_a .select do |ship_method| diff --git a/core/app/models/spree/stock/package.rb b/core/app/models/spree/stock/package.rb index 870c5ed34f5..9f730af6282 100644 --- a/core/app/models/spree/stock/package.rb +++ b/core/app/models/spree/stock/package.rb @@ -123,7 +123,6 @@ def to_shipment Spree::Shipment.new( order: order, - address: order.ship_address, stock_location: stock_location, inventory_units: contents.map(&:inventory_unit) ) diff --git a/core/lib/spree/testing_support/factories/order_factory.rb b/core/lib/spree/testing_support/factories/order_factory.rb index 5b45ee17c85..10c2f18359b 100644 --- a/core/lib/spree/testing_support/factories/order_factory.rb +++ b/core/lib/spree/testing_support/factories/order_factory.rb @@ -46,7 +46,7 @@ end order.line_items.reload - create(:shipment, order: order, cost: evaluator.shipment_cost, shipping_method: evaluator.shipping_method, address: evaluator.ship_address, stock_location: evaluator.stock_location) + create(:shipment, order: order, cost: evaluator.shipment_cost, shipping_method: evaluator.shipping_method, stock_location: evaluator.stock_location) order.shipments.reload order.update! @@ -94,7 +94,7 @@ next unless evaluator.with_cartons Spree::Carton.create!( stock_location: shipment.stock_location, - address: shipment.address, + address: order.ship_address, shipping_method: shipment.shipping_method, inventory_units: shipment.inventory_units, shipped_at: Time.current diff --git a/core/lib/tasks/migrations/copy_shipped_shipments_to_cartons.rake b/core/lib/tasks/migrations/copy_shipped_shipments_to_cartons.rake index 4c9189a4ddc..e4324758e49 100644 --- a/core/lib/tasks/migrations/copy_shipped_shipments_to_cartons.rake +++ b/core/lib/tasks/migrations/copy_shipped_shipments_to_cartons.rake @@ -49,13 +49,15 @@ namespace 'spree:migrations:copy_shipped_shipments_to_cartons' do #{db_concat("'C'", 'spree_shipments.number')}, -- number spree_shipments.id, -- imported_from_shipment_id spree_shipments.stock_location_id, - spree_shipments.address_id, + spree_orders.ship_address_id, spree_shipping_rates.shipping_method_id, spree_shipments.tracking, spree_shipments.shipped_at, '#{Time.current.to_s(:db)}', -- created_at '#{Time.current.to_s(:db)}' -- updated_at from spree_shipments + left join spree_orders + on spree_orders.id = spree_shipments.order_id left join spree_shipping_rates on spree_shipping_rates.shipment_id = spree_shipments.id and spree_shipping_rates.selected = #{Spree::Carton.connection.quoted_true} diff --git a/core/spec/models/spree/order_shipping_spec.rb b/core/spec/models/spree/order_shipping_spec.rb index f2377f6d0e0..62b7500f52f 100644 --- a/core/spec/models/spree/order_shipping_spec.rb +++ b/core/spec/models/spree/order_shipping_spec.rb @@ -57,7 +57,7 @@ def emails let(:shipment) { order.shipments.to_a.first } let(:inventory_units) { shipment.inventory_units } let(:stock_location) { shipment.stock_location } - let(:address) { shipment.address } + let(:address) { order.ship_address } let(:shipping_method) { shipment.shipping_method } it_behaves_like 'shipment shipping' @@ -190,7 +190,7 @@ def emails order.shipping.ship( inventory_units: shipped_inventory, stock_location: shipment.stock_location, - address: shipment.address, + address: order.ship_address, shipping_method: shipment.shipping_method ) end @@ -223,8 +223,7 @@ def emails let(:shipment) do FactoryGirl.create( :shipment, - order: order, - address: FactoryGirl.create(:address) + order: order ) end diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index cd3e6643089..cd46d3a4d16 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -9,7 +9,6 @@ order.shipments.create!( state: 'pending', cost: 1, - address: order.ship_address, inventory_units: order.inventory_units, shipping_rates: [ Spree::ShippingRate.new( @@ -454,7 +453,7 @@ context "when the shipment is canceled" do let(:address){ create(:address) } let(:order){ create(:order_with_line_items, ship_address: address) } - let(:shipment_with_inventory_units) { create(:shipment, order: order, address: address, state: 'canceled') } + let(:shipment_with_inventory_units) { create(:shipment, order: order, state: 'canceled') } let(:subject) { shipment_with_inventory_units.ship! } before do allow(order).to receive(:update!) @@ -682,7 +681,6 @@ let(:unshippable_shipment) do create( :shipment, - address: create(:address), stock_location: stock_location, inventory_units: [build(:inventory_unit)] ) diff --git a/core/spec/models/spree/stock/package_spec.rb b/core/spec/models/spree/stock/package_spec.rb index fbb6a9569c6..0b0ef319097 100644 --- a/core/spec/models/spree/stock/package_spec.rb +++ b/core/spec/models/spree/stock/package_spec.rb @@ -113,7 +113,6 @@ def build_inventory_unit expect(last_unit.state).to eq 'backordered' expect(shipment.shipping_method).to eq shipping_method - expect(shipment.address).to eq order.ship_address end it 'does not add an inventory unit to a package twice' do From 4f10321c2798fe68e5b0b0eb718ac69d7a1b50ec Mon Sep 17 00:00:00 2001 From: Patrick Sinclair Date: Wed, 11 May 2016 13:44:11 -0400 Subject: [PATCH 2/5] Fixing broken API specs --- api/spec/controllers/spree/api/checkouts_controller_spec.rb | 2 +- api/spec/controllers/spree/api/shipments_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/spec/controllers/spree/api/checkouts_controller_spec.rb b/api/spec/controllers/spree/api/checkouts_controller_spec.rb index 572ee48bd70..dfd9d15e563 100644 --- a/api/spec/controllers/spree/api/checkouts_controller_spec.rb +++ b/api/spec/controllers/spree/api/checkouts_controller_spec.rb @@ -136,7 +136,7 @@ module Spree it "can update shipping method and transition from delivery to payment" do order.update_column(:state, "delivery") - shipment = create(:shipment, order: order, address: order.ship_address) + shipment = create(:shipment, order: order) shipment.refresh_rates shipping_rate = shipment.shipping_rates.where(selected: false).first api_put :update, id: order.to_param, order_token: order.guest_token, diff --git a/api/spec/controllers/spree/api/shipments_controller_spec.rb b/api/spec/controllers/spree/api/shipments_controller_spec.rb index a481e2a0e61..2868302263d 100644 --- a/api/spec/controllers/spree/api/shipments_controller_spec.rb +++ b/api/spec/controllers/spree/api/shipments_controller_spec.rb @@ -2,7 +2,7 @@ describe Spree::Api::ShipmentsController, type: :controller do render_views - let!(:shipment) { create(:shipment, address: create(:address), inventory_units: [build(:inventory_unit, shipment: nil)]) } + let!(:shipment) { create(:shipment, inventory_units: [build(:inventory_unit, shipment: nil)]) } let!(:attributes) { [:id, :tracking, :tracking_url, :number, :cost, :shipped_at, :stock_location_name, :order_id, :shipping_rates, :shipping_methods] } before do From 0b55df7d89aa65ad45b4be8162b2594ec7c5f0f2 Mon Sep 17 00:00:00 2001 From: Patrick Sinclair Date: Fri, 27 May 2016 08:11:55 +0100 Subject: [PATCH 3/5] Add migration to rename spree_shipments.address_id column to spree_shipments.deprecated_address_id --- .../20160527070401_rename_shipment_address_field.rb | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 core/db/migrate/20160527070401_rename_shipment_address_field.rb diff --git a/core/db/migrate/20160527070401_rename_shipment_address_field.rb b/core/db/migrate/20160527070401_rename_shipment_address_field.rb new file mode 100644 index 00000000000..256c8867c7b --- /dev/null +++ b/core/db/migrate/20160527070401_rename_shipment_address_field.rb @@ -0,0 +1,7 @@ +class RenameShipmentAddressField < ActiveRecord::Migration + def change + change_table :spree_shipments do |t| + t.rename :address_id, :deprecated_address_id + end + end +end From 4176f10daa90864add6a695bb1d183d77df75594 Mon Sep 17 00:00:00 2001 From: Patrick Sinclair Date: Mon, 6 Jun 2016 09:33:43 +0100 Subject: [PATCH 4/5] Remove more references to Shipment#address --- .../migrations/copy_shipped_shipments_to_cartons_spec.rb | 2 +- core/spec/models/spree/stock/coordinator_spec.rb | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb b/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb index c69a131877d..80a235ab823 100644 --- a/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb +++ b/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb @@ -49,7 +49,7 @@ expect(carton.number).to eq "C#{shipped_shipment.number}" expect(carton.stock_location).to eq shipped_shipment.stock_location - expect(carton.address).to eq shipped_shipment.address + expect(carton.address).to eq shipped_shipment.order.ship_address expect(carton.shipping_method).to eq shipped_shipment.shipping_method expect(carton.tracking).to eq shipped_shipment.tracking expect(carton.shipped_at).to eq shipped_shipment.shipped_at diff --git a/core/spec/models/spree/stock/coordinator_spec.rb b/core/spec/models/spree/stock/coordinator_spec.rb index 573eb98d81e..f51692665ce 100644 --- a/core/spec/models/spree/stock/coordinator_spec.rb +++ b/core/spec/models/spree/stock/coordinator_spec.rb @@ -26,11 +26,6 @@ module Stock expect(subject.shipments.size).to eq(1) end - it "puts the order's ship address on the shipments" do - shipments = subject.shipments - expect(shipments.map(&:address)).to eq [order.ship_address] - end - it "builds a shipment for all active stock locations" do subject.shipments.count == StockLocation.count end From f62902cf40776d1389fdf610198a45bda7280ac8 Mon Sep 17 00:00:00 2001 From: Patrick Sinclair Date: Mon, 6 Jun 2016 09:40:43 +0100 Subject: [PATCH 5/5] Add CHANGELOG entry for Spree::Shipment#address deprecation --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc2155daa41..3f07e5ce120 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ ## Solidus 1.4.0 (master, unreleased) +* Deprecate `Spree::Shipment#address` (column renamed) + + `Spree::Shipment#address` was not actually being used for anything in + particular, so the association has been deprecated and delegated to + `Spree::Order#ship_address` instead. The database column has been renamed + `spree_shipments.deprecated_address_id`. + + https://github.com/solidusio/solidus/pull/1138 + * Coupon code application has been separated from the Continue button on the Payment checkout page * JavaScript for it has been moved from address.js into its own `spree/frontend/checkout/coupon-code`