raggedstaff/openfoodnetwork
1
0
Fork 0
mirror of https://github.com/openfoodfoundation/openfoodnetwork synced 2026-02-27 01:43:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #3589 from kristinalim/fix/2788-ship_address_changed

Browse Source 
2788 [Subscriptions] Fix address issues when syncing subscriptions and orders
This commit is contained in:
Luis Ramos
2019-05-03 23:48:32 +01:00
committed by GitHub
parent 4f0930b77c 4a9570b1a1
commit 4afa0cdca1
6 changed files with 77 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
app/models/order_updater.rb
View File

@@ -21,6 +21,15 @@ class OrderUpdater < SimpleDelegator
shipping_address_from_distributor
end
# Sets the distributor's address as shipping address of the order for those
# shipments using a shipping method that doesn't require address, such us
# a pickup.
def shipping_address_from_distributor
return if order.shipping_method.blank? || order.shipping_method.require_ship_address
order.ship_address = order.address_from_distributor
end
private
def infer_payment_state
@@ -79,16 +88,4 @@ class OrderUpdater < SimpleDelegator
def failed_payments?
payments.present? && payments.valid.empty?
end
# Sets the distributor's address as shipping address of the order for those
# shipments using a shipping method that doesn't require address, such us
# a pickup.
def shipping_address_from_distributor
shipments.each do |shipment|
shipping_method = shipment.shipping_method
next if shipping_method.require_ship_address
order.ship_address = order.distributor.address
end
end
end

12
app/models/spree/order_decorator.rb
View File

@@ -325,12 +325,6 @@ Spree::Order.class_eval do
total.to_f > 0.0 && !skip_payment_for_subscription?
end
private
def skip_payment_for_subscription?
subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now
end
def address_from_distributor
address = distributor.address.clone
if bill_address
@@ -341,6 +335,12 @@ Spree::Order.class_eval do
address
end
private
def skip_payment_for_subscription?
subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now
end
def provided_by_order_cycle?(line_item)
order_cycle_variants = order_cycle.andand.variants || []
order_cycle_variants.include? line_item.variant

27
app/services/order_syncer.rb
View File

@@ -31,8 +31,8 @@ class OrderSyncer
def update_associations_for(order)
update_bill_address_for(order) if (bill_address.changes.keys & relevant_address_attrs).any?
update_ship_address_for(order) if (ship_address.changes.keys & relevant_address_attrs).any?
update_shipment_for(order) if shipping_method_id_changed?
update_ship_address_for(order)
update_payment_for(order) if payment_method_id_changed?
end
@@ -48,11 +48,6 @@ class OrderSyncer
order.bill_address.update_attributes(bill_address.attributes.slice(*relevant_address_attrs))
end
def update_ship_address_for(order)
return unless ship_address_updatable?(order)
order.ship_address.update_attributes(ship_address.attributes.slice(*relevant_address_attrs))
end
def update_payment_for(order)
payment = order.payments.with_state('checkout').where(payment_method_id: payment_method_id_was).last
if payment
@@ -75,6 +70,19 @@ class OrderSyncer
end
end
def update_ship_address_for(order)
# The conditions here are to achieve the same behaviour in earlier versions of Spree, where
# switching from pick-up to delivery affects whether simultaneous changes to shipping address
# are ignored or not.
pickup_to_delivery = force_ship_address_required?(order)
if !pickup_to_delivery || order.shipment.present?
save_ship_address_in_order(order) if (ship_address.changes.keys & relevant_address_attrs).any?
end
if !pickup_to_delivery || order.shipment.blank?
order.updater.shipping_address_from_distributor
end
end
def relevant_address_attrs
["firstname", "lastname", "address1", "zipcode", "city", "state_id", "country_id", "phone"]
end
@@ -99,12 +107,17 @@ class OrderSyncer
# address on the order matches the shop's address
def force_ship_address_required?(order)
return false unless shipping_method.require_ship_address?
distributor_address = order.__send__(:address_from_distributor)
distributor_address = order.address_from_distributor
relevant_address_attrs.all? do |attr|
order.ship_address[attr] == distributor_address[attr]
end
end
def save_ship_address_in_order(order)
return unless ship_address_updatable?(order)
order.ship_address.update_attributes(ship_address.attributes.slice(*relevant_address_attrs))
end
def pending_shipment_with?(order, shipping_method_id)
return false unless order.shipment.present? && order.shipment.state == "pending"
order.shipping_method.id == shipping_method_id

12
spec/jobs/subscription_placement_job_spec.rb
View File

@@ -82,7 +82,7 @@ describe SubscriptionPlacementJob do
variant3.update_attribute(:on_hand, 0)
end
xit "caps quantity at the stock level for stock-limited items, and reports the change" do
it "caps quantity at the stock level for stock-limited items, and reports the change" do
changes = job.send(:cap_quantity_and_store_changes, order.reload)
expect(line_item1.reload.quantity).to be 3 # not capped
expect(line_item2.reload.quantity).to be 2 # capped
@@ -104,7 +104,7 @@ describe SubscriptionPlacementJob do
variant3.update_attribute(:on_hand, 0)
end
xit "sets quantity to 0 for unavailable items, and reports the change" do
it "sets quantity to 0 for unavailable items, and reports the change" do
changes = job.send(:cap_quantity_and_store_changes, order.reload)
expect(line_item1.reload.quantity).to be 0 # unavailable
expect(line_item2.reload.quantity).to be 2 # capped
@@ -151,7 +151,7 @@ describe SubscriptionPlacementJob do
allow(job).to receive(:unavailable_stock_lines_for) { order.line_items }
end
xit "does not place the order, clears, all adjustments, and sends an empty_order email" do
it "does not place the order, clears, all adjustments, and sends an empty_order email" do
expect{ job.send(:process, order) }.to_not change{ order.reload.completed_at }.from(nil)
expect(order.adjustments).to be_empty
expect(order.total).to eq 0
@@ -162,7 +162,7 @@ describe SubscriptionPlacementJob do
end
context "when at least one stock item is available after capping stock" do
xit "processes the order to completion, but does not process the payment" do
it "processes the order to completion, but does not process the payment" do
# If this spec starts complaining about no shipping methods being available
# on CI, there is probably another spec resetting the currency though Rails.cache.clear
expect{ job.send(:process, order) }.to change{ order.reload.completed_at }.from(nil)
@@ -170,7 +170,7 @@ describe SubscriptionPlacementJob do
expect(order.payments.first.state).to eq "checkout"
end
xit "does not enqueue confirmation emails" do
it "does not enqueue confirmation emails" do
expect{ job.send(:process, order) }.to_not enqueue_job ConfirmOrderJob
expect(job).to have_received(:send_placement_email).with(order, anything).once
end
@@ -178,7 +178,7 @@ describe SubscriptionPlacementJob do
context "when progression of the order fails" do
before { allow(order).to receive(:next) { false } }
xit "records an error and does not attempt to send an email" do
it "records an error and does not attempt to send an email" do
expect(job).to_not receive(:send_placement_email)
expect(job).to receive(:record_and_log_error).once
job.send(:process, order)

2
spec/models/order_updater_spec.rb
View File

@@ -131,7 +131,7 @@ describe OrderUpdater do
it "populates the shipping address from distributor" do
order_updater.before_save_hook
expect(order.ship_address.firstname).to eq(distributor.address.firstname)
expect(order.ship_address.address1).to eq(distributor.address.address1)
end
end

54
spec/services/order_syncer_spec.rb
View File

@@ -1,6 +1,6 @@
require "spec_helper"
xdescribe OrderSyncer do
describe OrderSyncer do
describe "updating the shipping method" do
let!(:subscription) { create(:subscription, with_items: true, with_proxy_orders: true) }
let!(:order) { subscription.proxy_orders.first.initialise_order! }
@@ -156,7 +156,7 @@ xdescribe OrderSyncer do
end
context "when the bill_address on the order matches that on the subscription" do
xit "updates all bill_address attrs and ship_address names + phone" do
it "updates all bill_address attrs and ship_address names + phone" do
subscription.assign_attributes(params)
expect(syncer.sync!).to be true
expect(syncer.order_update_issues.keys).to_not include order.id
@@ -173,8 +173,12 @@ xdescribe OrderSyncer do
end
context "when the bill_address on the order doesn't match that on the subscription" do
before { order.bill_address.update_attributes(firstname: "Jane") }
xit "does not update bill_address or ship_address on the order" do
before do
order.bill_address.update_attributes!(firstname: "Jane")
order.update!
end
it "does not update bill_address or ship_address on the order" do
subscription.assign_attributes(params)
expect(syncer.sync!).to be true
expect(syncer.order_update_issues.keys).to include order.id
@@ -214,7 +218,10 @@ xdescribe OrderSyncer do
end
context "when the bill_address on the order doesn't match that on the subscription" do
before { order.bill_address.update_attributes(firstname: "Jane") }
before do
order.bill_address.update_attributes!(firstname: "Jane")
order.update!
end
it "does not update bill_address or ship_address on the order" do
subscription.assign_attributes(params)
@@ -265,7 +272,7 @@ xdescribe OrderSyncer do
end
context "but the shipping method is being changed to one that requires a ship_address" do
let(:new_shipping_method) { create(:shipping_method, require_ship_address: true) }
let(:new_shipping_method) { create(:shipping_method, distributors: [distributor], require_ship_address: true) }
before { params.merge!(shipping_method_id: new_shipping_method.id) }
@@ -284,22 +291,27 @@ xdescribe OrderSyncer do
with_proxy_orders: true)
end
before do
subscription.assign_attributes(params)
end
context "when there is no pending shipment using the former shipping method" do
before do
order.shipment.destroy
subscription.assign_attributes(params)
end
it "updates ship_address attrs" do
expect(syncer.sync!).to be true
expect(syncer.order_update_issues.keys).to include order.id
order.reload
expect(order.ship_address.firstname).to eq ship_address_attrs["firstname"]
expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"]
expect(order.ship_address.address1).to eq ship_address_attrs["address1"]
expect(order.ship_address.phone).to eq ship_address_attrs["phone"]
it "updates ship_address attrs" do
expect(syncer.sync!).to be true
expect(syncer.order_update_issues.keys).to include order.id
order.reload
expect(order.ship_address.firstname).to eq ship_address_attrs["firstname"]
expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"]
expect(order.ship_address.address1).to eq ship_address_attrs["address1"]
expect(order.ship_address.phone).to eq ship_address_attrs["phone"]
end
end
context "when the order has a pending shipment using the former shipping method" do
let!(:shipment) { create(:shipment, order: order, shipping_method: shipping_method) }
before do
subscription.assign_attributes(params)
end
it "updates ship_address attrs" do
expect(syncer.sync!).to be true
@@ -334,7 +346,11 @@ xdescribe OrderSyncer do
end
context "when the ship address on the order doesn't match that on the subscription" do
before { order.ship_address.update_attributes(firstname: "Jane") }
before do
order.ship_address.update_attributes(firstname: "Jane")
order.update!
end
it "does not update ship_address on the order" do
subscription.assign_attributes(params)
expect(syncer.sync!).to be true