From ad935e8d660c7bc352eb3a67b646ad5b3d424170 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Wed, 13 Mar 2019 18:55:45 +1100 Subject: [PATCH 1/8] Require spec_helper.rb for OrderSyncer specs --- spec/services/order_syncer_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index ee54e02125..77bee4f264 100644 --- a/spec/services/order_syncer_spec.rb +++ b/spec/services/order_syncer_spec.rb @@ -1,3 +1,5 @@ +require "spec_helper" + describe OrderSyncer do describe "updating the shipping method" do let(:subscription) { create(:subscription, with_items: true, with_proxy_orders: true) } From 7200e2c2c22a95f958b32acdd0846bf85e516af1 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 15 Mar 2019 04:54:12 +1100 Subject: [PATCH 2/8] Specify hub address in some OrderSyncer specs This fixes some assertions from using the same subscription shipping and billing addresses, and distributor address. One of the specs also pass because the subscription shipping address matches the distributor address. This commit makes that scenario clearer. --- spec/services/order_syncer_spec.rb | 49 +++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index 77bee4f264..9a62488076 100644 --- a/spec/services/order_syncer_spec.rb +++ b/spec/services/order_syncer_spec.rb @@ -126,7 +126,15 @@ describe OrderSyncer do end describe "changing the billing address" do - let(:subscription) { create(:subscription, with_items: true, with_proxy_orders: true) } + let!(:distributor_address) { create(:address, address1: "Distributor Address") } + let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } + let!(:original_ship_address) { create(:address) } + + let(:subscription) do + create(:subscription, shop: distributor, ship_address: original_ship_address, + with_items: true, with_proxy_orders: true) + end + let(:shipping_method) { subscription.shipping_method } let!(:order) { subscription.proxy_orders.first.initialise_order! } let!(:bill_address_attrs) { subscription.bill_address.attributes } @@ -149,7 +157,7 @@ describe OrderSyncer do expect(order.bill_address.phone).to eq "1123581321" expect(order.ship_address.firstname).to eq "Bill" 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.address1).to eq distributor_address.address1 expect(order.ship_address.phone).to eq "1123581321" end end @@ -167,7 +175,7 @@ describe OrderSyncer do expect(order.bill_address.phone).to eq bill_address_attrs["phone"] expect(order.ship_address.firstname).to eq "Jane" 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.address1).to eq distributor_address.address1 expect(order.ship_address.phone).to eq ship_address_attrs["phone"] end end @@ -215,7 +223,15 @@ describe OrderSyncer do end describe "changing the ship address" do - let(:subscription) { create(:subscription, with_items: true, with_proxy_orders: true) } + let!(:distributor_address) { create(:address, address1: "Distributor Address") } + let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } + let!(:original_ship_address) { create(:address) } + + let(:subscription) do + create(:subscription, shop: distributor, ship_address: original_ship_address, + with_items: true, with_proxy_orders: true) + end + let(:shipping_method) { subscription.shipping_method } let!(:order) { subscription.proxy_orders.first.initialise_order! } let!(:bill_address_attrs) { subscription.bill_address.attributes } @@ -233,23 +249,28 @@ describe OrderSyncer do 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.address1).to eq distributor_address.address1 expect(order.ship_address.phone).to eq ship_address_attrs["phone"] 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) } + before { params.merge!(shipping_method_id: new_shipping_method.id) } - it "updates ship_address attrs" do - subscription.assign_attributes(params) - expect(syncer.sync!).to be true - expect(syncer.order_update_issues.keys).to_not include order.id - order.reload; - expect(order.ship_address.firstname).to eq "Ship" - expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"] - expect(order.ship_address.address1).to eq "123 abc st" - expect(order.ship_address.phone).to eq "1123581321" + context "when the original ship address is the same as the distributor address" do + let!(:original_ship_address) { create(:address, address1: distributor_address.address1) } + + it "updates ship_address attrs" do + subscription.assign_attributes(params) + expect(syncer.sync!).to be true + expect(syncer.order_update_issues.keys).to_not include order.id + order.reload; + expect(order.ship_address.firstname).to eq "Ship" + expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"] + expect(order.ship_address.address1).to eq "123 abc st" + expect(order.ship_address.phone).to eq "1123581321" + end end end end From ba100a6522fa87a856bdb5a9f4bc4678fafd903a Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 15 Mar 2019 02:09:09 +0800 Subject: [PATCH 3/8] Specify order addresses in some OrderSyncer specs This is to check that we are using the correct address data in assertions. This also clarifies the scenario for one of the specs. --- spec/services/order_syncer_spec.rb | 40 +++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index 9a62488076..f247120516 100644 --- a/spec/services/order_syncer_spec.rb +++ b/spec/services/order_syncer_spec.rb @@ -128,11 +128,17 @@ describe OrderSyncer do describe "changing the billing address" do let!(:distributor_address) { create(:address, address1: "Distributor Address") } let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } - let!(:original_ship_address) { create(:address) } + let!(:original_bill_address) do + create(:address, firstname: "Walter", lastname: "Wolf", address1: "White") + end + let!(:original_ship_address) do + create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") + end let(:subscription) do - create(:subscription, shop: distributor, ship_address: original_ship_address, - with_items: true, with_proxy_orders: true) + create(:subscription, shop: distributor, bill_address: original_bill_address, + ship_address: original_ship_address, with_items: true, + with_proxy_orders: true) end let(:shipping_method) { subscription.shipping_method } @@ -156,7 +162,7 @@ describe OrderSyncer do expect(order.bill_address.address1).to eq "123 abc st" expect(order.bill_address.phone).to eq "1123581321" expect(order.ship_address.firstname).to eq "Bill" - expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"] + expect(order.ship_address.lastname).to eq bill_address_attrs["lastname"] expect(order.ship_address.address1).to eq distributor_address.address1 expect(order.ship_address.phone).to eq "1123581321" end @@ -174,7 +180,7 @@ describe OrderSyncer do expect(order.bill_address.address1).to eq bill_address_attrs["address1"] expect(order.bill_address.phone).to eq bill_address_attrs["phone"] expect(order.ship_address.firstname).to eq "Jane" - expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"] + expect(order.ship_address.lastname).to eq bill_address_attrs["lastname"] expect(order.ship_address.address1).to eq distributor_address.address1 expect(order.ship_address.phone).to eq ship_address_attrs["phone"] end @@ -225,11 +231,17 @@ describe OrderSyncer do describe "changing the ship address" do let!(:distributor_address) { create(:address, address1: "Distributor Address") } let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } - let!(:original_ship_address) { create(:address) } + let!(:original_bill_address) do + create(:address, firstname: "Walter", lastname: "Wolf", address1: "White") + end + let!(:original_ship_address) do + create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") + end let(:subscription) do - create(:subscription, shop: distributor, ship_address: original_ship_address, - with_items: true, with_proxy_orders: true) + create(:subscription, shop: distributor, bill_address: original_bill_address, + ship_address: original_ship_address, with_items: true, + with_proxy_orders: true) end let(:shipping_method) { subscription.shipping_method } @@ -247,8 +259,8 @@ describe OrderSyncer do expect(syncer.sync!).to be true expect(syncer.order_update_issues.keys).to_not 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.firstname).to eq bill_address_attrs["firstname"] + expect(order.ship_address.lastname).to eq bill_address_attrs["lastname"] expect(order.ship_address.address1).to eq distributor_address.address1 expect(order.ship_address.phone).to eq ship_address_attrs["phone"] end @@ -258,8 +270,12 @@ describe OrderSyncer do before { params.merge!(shipping_method_id: new_shipping_method.id) } - context "when the original ship address is the same as the distributor address" do - let!(:original_ship_address) { create(:address, address1: distributor_address.address1) } + context "when the original ship address is the bill contact using distributor address" do + let!(:original_ship_address) do + create(:address, firstname: original_bill_address.firstname, + lastname: original_bill_address.lastname, + address1: distributor_address.address1) + end it "updates ship_address attrs" do subscription.assign_attributes(params) From 3ab00d862e5e3f5c2aca5933f11d6846e49afe5a Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 15 Mar 2019 02:58:00 +0800 Subject: [PATCH 4/8] Create subscriptions with set up shipping methods The shipping methods are updated to their target settings after the subscription order has been created, so the order is created with the "require_ship_address" factory default which is "true". The shipping method setting at time of order creation has implications on whether a shipment is set up for the order or not. --- spec/services/order_syncer_spec.rb | 58 ++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index f247120516..4cbde81fee 100644 --- a/spec/services/order_syncer_spec.rb +++ b/spec/services/order_syncer_spec.rb @@ -134,14 +134,14 @@ describe OrderSyncer do let!(:original_ship_address) do create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") end + let!(:shipping_method) { create(:shipping_method, distributors: [distributor]) } let(:subscription) do create(:subscription, shop: distributor, bill_address: original_bill_address, - ship_address: original_ship_address, with_items: true, - with_proxy_orders: true) + ship_address: original_ship_address, shipping_method: shipping_method, + with_items: true, with_proxy_orders: true) end - let(:shipping_method) { subscription.shipping_method } let!(:order) { subscription.proxy_orders.first.initialise_order! } let!(:bill_address_attrs) { subscription.bill_address.attributes } let!(:ship_address_attrs) { subscription.ship_address.attributes } @@ -149,7 +149,9 @@ describe OrderSyncer do let(:syncer) { OrderSyncer.new(subscription) } context "when a ship address is not required" do - before { shipping_method.update_attributes(require_ship_address: false) } + let!(:shipping_method) do + create(:shipping_method, distributors: [distributor], require_ship_address: false) + end context "when the bill_address on the order matches that on the subscription" do it "updates all bill_address attrs and ship_address names + phone" do @@ -188,7 +190,9 @@ describe OrderSyncer do end context "when a ship address is required" do - before { shipping_method.update_attributes(require_ship_address: true) } + let!(:shipping_method) do + create(:shipping_method, distributors: [distributor], require_ship_address: true) + end context "when the bill_address on the order matches that on the subscription" do it "only updates bill_address attrs" do @@ -237,11 +241,14 @@ describe OrderSyncer do let!(:original_ship_address) do create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") end + let!(:shipping_method) do + create(:shipping_method, distributors: [distributor], require_ship_address: true) + end let(:subscription) do create(:subscription, shop: distributor, bill_address: original_bill_address, - ship_address: original_ship_address, with_items: true, - with_proxy_orders: true) + ship_address: original_ship_address, shipping_method: shipping_method, + with_items: true, with_proxy_orders: true) end let(:shipping_method) { subscription.shipping_method } @@ -252,7 +259,9 @@ describe OrderSyncer do let(:syncer) { OrderSyncer.new(subscription) } context "when a ship address is not required" do - before { shipping_method.update_attributes(require_ship_address: false) } + let!(:shipping_method) do + create(:shipping_method, distributors: [distributor], require_ship_address: false) + end it "does not change the ship address" do subscription.assign_attributes(params) @@ -277,22 +286,41 @@ describe OrderSyncer do address1: distributor_address.address1) end - it "updates ship_address attrs" do + before do subscription.assign_attributes(params) + end + + it "updates ship_address attrs" do expect(syncer.sync!).to be true - expect(syncer.order_update_issues.keys).to_not include order.id - order.reload; - expect(order.ship_address.firstname).to eq "Ship" + 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 "123 abc st" - expect(order.ship_address.phone).to eq "1123581321" + expect(order.ship_address.address1).to eq ship_address_attrs["address1"] + expect(order.ship_address.phone).to eq ship_address_attrs["phone"] + 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) } + + it "updates ship_address attrs" do + expect(syncer.sync!).to be true + expect(syncer.order_update_issues.keys).not_to include order.id + order.reload + expect(order.ship_address.firstname).to eq "Ship" + expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"] + expect(order.ship_address.address1).to eq "123 abc st" + expect(order.ship_address.phone).to eq "1123581321" + end end end end end context "when a ship address is required" do - before { shipping_method.update_attributes(require_ship_address: true) } + let!(:shipping_method) do + create(:shipping_method, distributors: [distributor], require_ship_address: true) + end context "when the ship address on the order matches that on the subscription" do it "updates ship_address attrs" do From d047e04e33b970c94c544cfb37a574f8ca297b57 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 22 Mar 2019 01:31:20 +0800 Subject: [PATCH 5/8] Customize addresses for subscriptions in specs This forces us to write correct assertions when adding specs affected by subscription addresses. --- spec/factories.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/factories.rb b/spec/factories.rb index 9bdd09933b..6bd576e28c 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -142,8 +142,8 @@ FactoryBot.define do shop { create :enterprise } schedule { create(:schedule, order_cycles: [create(:simple_order_cycle, coordinator: shop)]) } customer { create(:customer, enterprise: shop) } - bill_address { create(:address) } - ship_address { create(:address) } + bill_address { create(:address, firstname: "Walter", lastname: "Wolf", address1: "White") } + ship_address { create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") } payment_method { create(:payment_method, distributors: [shop]) } shipping_method { create(:shipping_method, distributors: [shop]) } begins_at { 1.month.ago } From 7be669e1dd8d6f8ed900b8a2ac4e650160ca33f4 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 22 Mar 2019 02:45:45 +0800 Subject: [PATCH 6/8] Add address factory with randomized attributes Randomizing all addresses generated will be explored separately. --- spec/factories/address_factory.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 spec/factories/address_factory.rb diff --git a/spec/factories/address_factory.rb b/spec/factories/address_factory.rb new file mode 100644 index 0000000000..ec5f149d9c --- /dev/null +++ b/spec/factories/address_factory.rb @@ -0,0 +1,11 @@ +FactoryBot.modify do + factory :address do + trait :randomized do + firstname { Faker::Name.first_name } + lastname { Faker::Name.last_name } + address1 { Faker::Address.street_address } + address2 nil + phone { Faker::PhoneNumber.phone_number } + end + end +end From 0c2a75a22745ae24ef0e92ad3ea6010a4aafd2de Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 22 Mar 2019 02:47:12 +0800 Subject: [PATCH 7/8] Randomize bill and ship address of sample subscriptions This also fixes some phone references in OrderSyncer specs. --- spec/factories.rb | 4 +-- spec/services/order_syncer_spec.rb | 39 ++++++++++++------------------ 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/spec/factories.rb b/spec/factories.rb index 6bd576e28c..bc140ce537 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -142,8 +142,8 @@ FactoryBot.define do shop { create :enterprise } schedule { create(:schedule, order_cycles: [create(:simple_order_cycle, coordinator: shop)]) } customer { create(:customer, enterprise: shop) } - bill_address { create(:address, firstname: "Walter", lastname: "Wolf", address1: "White") } - ship_address { create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") } + bill_address { create(:address, :randomized) } + ship_address { create(:address, :randomized) } payment_method { create(:payment_method, distributors: [shop]) } shipping_method { create(:shipping_method, distributors: [shop]) } begins_at { 1.month.ago } diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index 4cbde81fee..9219c078fa 100644 --- a/spec/services/order_syncer_spec.rb +++ b/spec/services/order_syncer_spec.rb @@ -128,23 +128,15 @@ describe OrderSyncer do describe "changing the billing address" do let!(:distributor_address) { create(:address, address1: "Distributor Address") } let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } - let!(:original_bill_address) do - create(:address, firstname: "Walter", lastname: "Wolf", address1: "White") - end - let!(:original_ship_address) do - create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") - end let!(:shipping_method) { create(:shipping_method, distributors: [distributor]) } - let(:subscription) do - create(:subscription, shop: distributor, bill_address: original_bill_address, - ship_address: original_ship_address, shipping_method: shipping_method, - with_items: true, with_proxy_orders: true) + create(:subscription, shop: distributor, shipping_method: shipping_method, with_items: true, + with_proxy_orders: true) end - let!(:order) { subscription.proxy_orders.first.initialise_order! } let!(:bill_address_attrs) { subscription.bill_address.attributes } let!(:ship_address_attrs) { subscription.ship_address.attributes } + let(:params) { { bill_address_attributes: { id: bill_address_attrs["id"], firstname: "Bill", address1: "123 abc st", phone: "1123581321" } } } let(:syncer) { OrderSyncer.new(subscription) } @@ -184,7 +176,7 @@ describe OrderSyncer do expect(order.ship_address.firstname).to eq "Jane" expect(order.ship_address.lastname).to eq bill_address_attrs["lastname"] expect(order.ship_address.address1).to eq distributor_address.address1 - expect(order.ship_address.phone).to eq ship_address_attrs["phone"] + expect(order.ship_address.phone).to eq bill_address_attrs["phone"] end end end @@ -235,20 +227,13 @@ describe OrderSyncer do describe "changing the ship address" do let!(:distributor_address) { create(:address, address1: "Distributor Address") } let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } - let!(:original_bill_address) do - create(:address, firstname: "Walter", lastname: "Wolf", address1: "White") - end - let!(:original_ship_address) do - create(:address, firstname: "Melanie", lastname: "Miller", address1: "Magenta") - end let!(:shipping_method) do create(:shipping_method, distributors: [distributor], require_ship_address: true) end let(:subscription) do - create(:subscription, shop: distributor, bill_address: original_bill_address, - ship_address: original_ship_address, shipping_method: shipping_method, - with_items: true, with_proxy_orders: true) + create(:subscription, shop: distributor, shipping_method: shipping_method, with_items: true, + with_proxy_orders: true) end let(:shipping_method) { subscription.shipping_method } @@ -271,7 +256,7 @@ describe OrderSyncer do expect(order.ship_address.firstname).to eq bill_address_attrs["firstname"] expect(order.ship_address.lastname).to eq bill_address_attrs["lastname"] expect(order.ship_address.address1).to eq distributor_address.address1 - expect(order.ship_address.phone).to eq ship_address_attrs["phone"] + expect(order.ship_address.phone).to eq bill_address_attrs["phone"] end context "but the shipping method is being changed to one that requires a ship_address" do @@ -280,10 +265,18 @@ describe OrderSyncer do before { params.merge!(shipping_method_id: new_shipping_method.id) } context "when the original ship address is the bill contact using distributor address" do + let!(:original_bill_address) { create(:address, :randomized) } let!(:original_ship_address) do create(:address, firstname: original_bill_address.firstname, lastname: original_bill_address.lastname, - address1: distributor_address.address1) + address1: distributor_address.address1, + phone: original_bill_address.phone) + end + let(:subscription) do + create(:subscription, shop: distributor, bill_address: original_bill_address, + ship_address: original_ship_address, + shipping_method: shipping_method, with_items: true, + with_proxy_orders: true) end before do From c222971e26fe3be7b1fcdedc2d3c2662b964c65b Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 22 Mar 2019 03:30:32 +0800 Subject: [PATCH 8/8] Clean up setup data for some OrderSyncer specs --- spec/services/order_syncer_spec.rb | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index 9219c078fa..74ba7f4d50 100644 --- a/spec/services/order_syncer_spec.rb +++ b/spec/services/order_syncer_spec.rb @@ -126,9 +126,8 @@ describe OrderSyncer do end describe "changing the billing address" do - let!(:distributor_address) { create(:address, address1: "Distributor Address") } + let!(:distributor_address) { create(:address, :randomized) } let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } - let!(:shipping_method) { create(:shipping_method, distributors: [distributor]) } let(:subscription) do create(:subscription, shop: distributor, shipping_method: shipping_method, with_items: true, with_proxy_orders: true) @@ -225,21 +224,16 @@ describe OrderSyncer do end describe "changing the ship address" do - let!(:distributor_address) { create(:address, address1: "Distributor Address") } + let!(:distributor_address) { create(:address, :randomized) } let!(:distributor) { create(:distributor_enterprise, address: distributor_address) } - let!(:shipping_method) do - create(:shipping_method, distributors: [distributor], require_ship_address: true) - end - - let(:subscription) do + let!(:subscription) do create(:subscription, shop: distributor, shipping_method: shipping_method, with_items: true, with_proxy_orders: true) end - - let(:shipping_method) { subscription.shipping_method } let!(:order) { subscription.proxy_orders.first.initialise_order! } let!(:bill_address_attrs) { subscription.bill_address.attributes } let!(:ship_address_attrs) { subscription.ship_address.attributes } + let(:params) { { ship_address_attributes: { id: ship_address_attrs["id"], firstname: "Ship", address1: "123 abc st", phone: "1123581321" } } } let(:syncer) { OrderSyncer.new(subscription) }