From c65d82be01924900024ebc6a9c8ac04633ea19f7 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 16 Apr 2021 13:51:28 +0200 Subject: [PATCH 1/6] Notify Bugsnag if customer fails to be created This will hopefully let us see in Bugsnag when #6038 happens and so we'll have all the diagnostics data. The fact that the `order.distributor` is optional but mandatory to create the customer already gives us some clue. --- app/models/spree/order.rb | 18 +++++++++++----- spec/models/spree/order_spec.rb | 38 +++++++++++++++++++++++---------- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 2a919c54639..9f753c6cf83 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -756,11 +756,19 @@ def associate_customer def ensure_customer return if associate_customer - customer_name = bill_address.andand.full_name - self.customer = Customer.create(enterprise: distributor, email: email_for_customer, - user: user, name: customer_name, - bill_address: bill_address.andand.clone, - ship_address: ship_address.andand.clone) + self.customer = Customer.new( + enterprise: distributor, + email: email_for_customer, + user: user, + name: bill_address.andand.full_name, + bill_address: bill_address.andand.clone, + ship_address: ship_address.andand.clone + ) + customer.save + + if customer.invalid? + Bugsnag.notify(customer.errors.full_messages.join(", ")) + end end def update_adjustment!(adjustment) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index f9e258dc9d5..34354f43ded 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -922,7 +922,7 @@ context "and the state is not cart" do let(:state) { "complete" } - it "returns true" do + it "returns false" do expect(order.send(:require_customer?)).to eq(false) end end @@ -1048,18 +1048,34 @@ end context "and order#email_for_customer does not match any existing customers" do - before { + before do order.bill_address = create(:address) order.ship_address = create(:address) - } - it "creates a new customer with defaut name and addresses" do - expect(order.customer).to be_nil - expect{ order.send(:ensure_customer) }.to change{ Customer.count }.by 1 - expect(order.customer).to be_a Customer + end + + context "and the customer is not valid" do + before do + order.distributor = nil + order.user = nil + order.email = nil + end + + it "sends an error to Bugsnag" do + expect(Bugsnag) + .to receive(:notify).with("Email can't be blank, Enterprise can't be blank") + order.send(:ensure_customer) + end + end + + context "and the customer is valid" do + it "creates a new customer with defaut name and addresses" do + expect(order.customer).to be_nil + expect { order.send(:ensure_customer) }.to change{ Customer.count }.by 1 - expect(order.customer.name).to eq order.bill_address.full_name - expect(order.customer.bill_address.same_as?(order.bill_address)).to be true - expect(order.customer.ship_address.same_as?(order.ship_address)).to be true + expect(order.customer.name).to eq order.bill_address.full_name + expect(order.customer.bill_address.same_as?(order.bill_address)).to be true + expect(order.customer.ship_address.same_as?(order.ship_address)).to be true + end end end end @@ -1327,7 +1343,7 @@ def advance_to_delivery_state(order) describe '#set_payment_amount!' do let(:order) do shipment = build(:shipment_with, :shipping_method, shipping_method: build(:shipping_method)) - build(:order, shipments: [shipment] ) + build(:order, shipments: [shipment]) end context 'after transitioning to payment' do From 5845ace8e9a9c994abffdac0a8b4b9a080b84635 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 19 Apr 2021 12:23:30 +0200 Subject: [PATCH 2/6] Don't persist models to test serializer Serializers don't care whether or not the objects they go through are persisted in DB. They just convert them. --- .../api/current_order_serializer_spec.rb | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/spec/serializers/api/current_order_serializer_spec.rb b/spec/serializers/api/current_order_serializer_spec.rb index dd5be4c180f..6317477a0fb 100644 --- a/spec/serializers/api/current_order_serializer_spec.rb +++ b/spec/serializers/api/current_order_serializer_spec.rb @@ -3,11 +3,17 @@ require 'spec_helper' describe Api::CurrentOrderSerializer do - let(:distributor) { create(:distributor_enterprise) } - let(:order_cycle) { create(:simple_order_cycle) } - let(:line_item) { create(:line_item, variant: create(:variant)) } - let(:order) { create(:order, line_items: [line_item]) } - let(:serializer) { Api::CurrentOrderSerializer.new(order, current_distributor: distributor, current_order_cycle: order_cycle).to_json } + let(:distributor) { build(:distributor_enterprise) } + let(:order_cycle) { build(:simple_order_cycle) } + let(:line_item) { build(:line_item, variant: create(:variant)) } + let(:order) { build(:order, line_items: [line_item]) } + let(:serializer) do + Api::CurrentOrderSerializer.new( + order, + current_distributor: distributor, + current_order_cycle: order_cycle + ).to_json + end it "serializers the current order" do expect(serializer).to match order.id.to_s @@ -28,7 +34,11 @@ end context 'when there is a shipment' do - before { create(:shipment, order: order) } + let(:shipping_method) { build(:shipping_method) } + + before do + allow(order).to receive(:shipping_method).and_return(shipping_method) + end it 'includes the shipping method of the order' do expect(serializer).to match("\"shipping_method_id\":#{order.shipping_method.id}") From 679de40a413cfa90299b2cb291e265306b108e18 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 22 Apr 2021 15:34:26 +0200 Subject: [PATCH 3/6] Move specs to where they belong --- .../api/v0/orders_controller_spec.rb | 19 +-------------- .../api/admin/order_serializer_spec.rb | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/spec/controllers/api/v0/orders_controller_spec.rb b/spec/controllers/api/v0/orders_controller_spec.rb index 4a76605138f..29e2af80903 100644 --- a/spec/controllers/api/v0/orders_controller_spec.rb +++ b/spec/controllers/api/v0/orders_controller_spec.rb @@ -66,30 +66,13 @@ module Api context 'as an admin user' do before do allow(controller).to receive(:spree_current_user) { admin_user } - get :index end it "retrieves a list of orders with appropriate attributes, including line items with appropriate attributes" do + get :index returns_orders(json_response) end - - it "formats completed_at to 'yyyy-mm-dd hh:mm'" do - completed_dates = json_response['orders'].map{ |order| order['completed_at'] } - correct_formats = completed_dates.all?{ |a| a == order1.completed_at.strftime('%B %d, %Y') } - - expect(correct_formats).to be_truthy - end - - it "returns distributor object with id key" do - distributors = json_response['orders'].map{ |order| order['distributor'] } - expect(distributors.all?{ |d| d.key?('id') }).to be_truthy - end - - it "returns the order number" do - order_numbers = json_response['orders'].map{ |order| order['number'] } - expect(order_numbers.all?{ |number| number.match("^R\\d{5,10}$") }).to be_truthy - end end context 'as an enterprise user' do diff --git a/spec/serializers/api/admin/order_serializer_spec.rb b/spec/serializers/api/admin/order_serializer_spec.rb index 644ec45dd2e..29fceb517dd 100644 --- a/spec/serializers/api/admin/order_serializer_spec.rb +++ b/spec/serializers/api/admin/order_serializer_spec.rb @@ -4,6 +4,7 @@ describe Api::Admin::OrderSerializer do let(:serializer) { described_class.new order } + let(:order) { build(:order) } describe "#display_outstanding_balance" do let(:order) { create(:order) } @@ -67,4 +68,26 @@ end end end + + describe "#completed_at" do + let(:order) { build(:order, state: 'complete', completed_at: DateTime.parse("2021-04-02")) } + + it "formats the date" do + expect(serializer.completed_at).to eq("April 02, 2021") + end + end + + describe "#distributor" do + before { order.distributor = build(:distributor_enterprise) } + + it "returns distributor object with id key" do + expect(serializer.distributor.id).to eq(order.distributor.id) + end + end + + describe "#number" do + it "returns the order number" do + expect(serializer.number).to eq(order.number) + end + end end From 451e05985f7c3b9b7da23f08c392c7f3742e6de0 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 23 Apr 2021 13:25:07 +0200 Subject: [PATCH 4/6] Fix spec --- spec/models/spree/order/checkout_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/models/spree/order/checkout_spec.rb b/spec/models/spree/order/checkout_spec.rb index 4926a1900b5..19f678d3590 100644 --- a/spec/models/spree/order/checkout_spec.rb +++ b/spec/models/spree/order/checkout_spec.rb @@ -69,6 +69,7 @@ before do order.state = 'address' order.shipments << create(:shipment) + order.distributor = build(:distributor_enterprise) order.email = "user@example.com" order.save! end From 002dabb80cd548556ad4d4382b1e37bf1a69665e Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 23 Apr 2021 13:28:02 +0200 Subject: [PATCH 5/6] Improve spec --- spec/models/spree/order_spec.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 34354f43ded..eeee3baf175 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1324,13 +1324,9 @@ def advance_to_delivery_state(order) end end - context 'when the is not complete' do + context 'when the order is not complete' do let(:order) do - build( - :order, - completed_at: nil, - line_items: [build(:line_item)] - ) + build(:order, completed_at: nil, line_items: [build(:line_item)]) end it 'transitions to :cart state' do From ee01b0162c3fb9f3d4388da7b68bb86bc0c5517a Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Apr 2021 12:39:03 +0200 Subject: [PATCH 6/6] Write if as a one-liner and avoid extra queries --- app/models/spree/order.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 9f753c6cf83..b2b418d6216 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -766,9 +766,7 @@ def ensure_customer ) customer.save - if customer.invalid? - Bugsnag.notify(customer.errors.full_messages.join(", ")) - end + Bugsnag.notify(customer.errors.full_messages.join(", ")) unless customer.persisted? end def update_adjustment!(adjustment)