From df95ee034f379c23abaf5de4db123fab776bf99e Mon Sep 17 00:00:00 2001
From: Elia Schito <elia@schito.me>
Date: Mon, 3 Feb 2020 15:24:07 +0100
Subject: [PATCH 1/7] Move a loose OrdersController spec to its spec file

---
 .../spree/current_order_tracking_spec.rb           | 14 --------------
 .../controllers/spree/orders_controller_spec.rb    |  8 ++++++++
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/frontend/spec/controllers/spree/current_order_tracking_spec.rb b/frontend/spec/controllers/spree/current_order_tracking_spec.rb
index 30c4e33cff3..5aa88698bcc 100644
--- a/frontend/spec/controllers/spree/current_order_tracking_spec.rb
+++ b/frontend/spec/controllers/spree/current_order_tracking_spec.rb
@@ -31,17 +31,3 @@ def index
     end
   end
 end
-
-describe Spree::OrdersController, type: :controller do
-  let(:user) { create(:user) }
-
-  before { allow(controller).to receive_messages(try_spree_current_user: user) }
-
-  describe Spree::OrdersController do
-    it "doesn't create a new order out of the blue" do
-      expect {
-        get :edit
-      }.not_to change { Spree::Order.count }
-    end
-  end
-end
diff --git a/frontend/spec/controllers/spree/orders_controller_spec.rb b/frontend/spec/controllers/spree/orders_controller_spec.rb
index 00132403322..3833471aa42 100644
--- a/frontend/spec/controllers/spree/orders_controller_spec.rb
+++ b/frontend/spec/controllers/spree/orders_controller_spec.rb
@@ -251,4 +251,12 @@
       expect(order.reload.line_items.count).to eq 0
     end
   end
+
+  describe '#edit' do
+    it "doesn't create a new order" do
+      allow(controller).to receive_messages(try_spree_current_user: user)
+
+      expect { get :edit }.not_to change { Spree::Order.count }
+    end
+  end
 end

From 4bfe9a7658fdc07e793d342032f3204995ea6655 Mon Sep 17 00:00:00 2001
From: Elia Schito <elia@schito.me>
Date: Mon, 3 Feb 2020 15:26:11 +0100
Subject: [PATCH 2/7] Only pass the `lock` option to
 #find_order_by_token_or_user

Restrict the number of options forwarded to
#find_order_by_token_or_user since the only one it uses is :lock.
---
 core/lib/spree/core/controller_helpers/order.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/core/lib/spree/core/controller_helpers/order.rb b/core/lib/spree/core/controller_helpers/order.rb
index 9742149115c..0719fab4707 100644
--- a/core/lib/spree/core/controller_helpers/order.rb
+++ b/core/lib/spree/core/controller_helpers/order.rb
@@ -34,7 +34,7 @@ def current_order(options = {})
 
           return @current_order if @current_order
 
-          @current_order = find_order_by_token_or_user(options)
+          @current_order = find_order_by_token_or_user(lock: options[:lock])
 
           if options[:create_order_if_necessary] && (@current_order.nil? || @current_order.completed?)
             @current_order = Spree::Order.new(new_order_params)

From aa5eaab08776ec164414ca4e17421b16f42f9ce5 Mon Sep 17 00:00:00 2001
From: Elia Schito <elia@schito.me>
Date: Mon, 3 Feb 2020 15:28:26 +0100
Subject: [PATCH 3/7] Avoid mutating the options hash passed order helpers

The mutation had no practical purpose and by avoiding it we also save
some cpu cycles.
---
 core/lib/spree/core/controller_helpers/order.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/core/lib/spree/core/controller_helpers/order.rb b/core/lib/spree/core/controller_helpers/order.rb
index 0719fab4707..83bc7e4e065 100644
--- a/core/lib/spree/core/controller_helpers/order.rb
+++ b/core/lib/spree/core/controller_helpers/order.rb
@@ -30,13 +30,13 @@ def simple_current_order
 
         # The current incomplete order from the guest_token for use in cart and during checkout
         def current_order(options = {})
-          options[:create_order_if_necessary] ||= false
+          should_create = options[:create_order_if_necessary] || false
 
           return @current_order if @current_order
 
           @current_order = find_order_by_token_or_user(lock: options[:lock])
 
-          if options[:create_order_if_necessary] && (@current_order.nil? || @current_order.completed?)
+          if should_create && (@current_order.nil? || @current_order.completed?)
             @current_order = Spree::Order.new(new_order_params)
             @current_order.user ||= try_spree_current_user
             # See issue https://github.com/spree/spree/issues/3346 for reasons why this line is here
@@ -84,14 +84,14 @@ def new_order_params
         end
 
         def find_order_by_token_or_user(options = {}, with_adjustments = false)
-          options[:lock] ||= false
+          should_lock = options[:lock] || false
 
           # Find any incomplete orders for the guest_token
           if with_adjustments
             Spree::Deprecation.warn "The second argument to find_order_by_token_or_user is deprecated, and will be removed in a future version."
-            order = Spree::Order.incomplete.includes(:adjustments).lock(options[:lock]).find_by(current_order_params)
+            order = Spree::Order.incomplete.includes(:adjustments).lock(should_lock).find_by(current_order_params)
           else
-            order = Spree::Order.incomplete.lock(options[:lock]).find_by(current_order_params)
+            order = Spree::Order.incomplete.lock(should_lock).find_by(current_order_params)
           end
 
           # Find any incomplete orders for the current user

From 7c79a1ce29b94b9097cfc5cf3e2f4cab60a0ed48 Mon Sep 17 00:00:00 2001
From: Elia Schito <elia@schito.me>
Date: Mon, 3 Feb 2020 15:41:33 +0100
Subject: [PATCH 4/7] Don't define top-level classes within specs

The same result can easily be obtained with the #controller helper.

Prior to this change the specs were interdependent and would have
worked only when loaded together, hiding the dependency of one helper
module on the others.
---
 .../core/controller_helpers/auth_spec.rb      | 22 ++++++++++---------
 .../core/controller_helpers/order_spec.rb     | 11 +++++-----
 .../core/controller_helpers/pricing_spec.rb   |  9 ++++----
 .../core/controller_helpers/search_spec.rb    | 10 ++++-----
 .../core/controller_helpers/store_spec.rb     |  8 +++----
 .../strong_parameters_spec.rb                 |  8 +++----
 6 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/core/spec/lib/spree/core/controller_helpers/auth_spec.rb b/core/spec/lib/spree/core/controller_helpers/auth_spec.rb
index 577030adc52..6d0c66d1abe 100644
--- a/core/spec/lib/spree/core/controller_helpers/auth_spec.rb
+++ b/core/spec/lib/spree/core/controller_helpers/auth_spec.rb
@@ -2,13 +2,11 @@
 
 require 'rails_helper'
 
-class FakesController < ApplicationController
-  include Spree::Core::ControllerHelpers::Auth
-  def index; render plain: 'index'; end
-end
-
 RSpec.describe Spree::Core::ControllerHelpers::Auth, type: :controller do
-  controller(FakesController) {}
+  controller(ApplicationController) {
+    include Spree::Core::ControllerHelpers::Auth
+    def index; render plain: 'index'; end
+  }
 
   describe '#current_ability' do
     it 'returns Spree::Ability instance' do
@@ -17,9 +15,12 @@ def index; render plain: 'index'; end
   end
 
   describe '#redirect_back_or_default' do
-    controller(FakesController) do
-      def index; redirect_back_or_default('/'); end
+    before do
+      def controller.index
+        redirect_back_or_default('/')
+      end
     end
+
     it 'redirects to session url' do
       session[:spree_user_return_to] = '/redirect'
       get :index
@@ -32,12 +33,13 @@ def index; redirect_back_or_default('/'); end
   end
 
   describe '#set_guest_token' do
-    controller(FakesController) do
-      def index
+    before do
+      def controller.index
         set_guest_token
         render plain: 'index'
       end
     end
+
     it 'sends cookie header' do
       get :index
       expect(response.headers["Set-Cookie"]).to match(/guest_token.*HttpOnly/)
diff --git a/core/spec/lib/spree/core/controller_helpers/order_spec.rb b/core/spec/lib/spree/core/controller_helpers/order_spec.rb
index 6a20886a4ca..69992bd2dc0 100644
--- a/core/spec/lib/spree/core/controller_helpers/order_spec.rb
+++ b/core/spec/lib/spree/core/controller_helpers/order_spec.rb
@@ -2,12 +2,13 @@
 
 require 'rails_helper'
 
-class FakesController < ApplicationController
-  include Spree::Core::ControllerHelpers::Order
-end
-
 RSpec.describe Spree::Core::ControllerHelpers::Order, type: :controller do
-  controller(FakesController) {}
+  controller(ApplicationController) {
+    include Spree::Core::ControllerHelpers::Store
+    include Spree::Core::ControllerHelpers::Pricing
+    include Spree::Core::ControllerHelpers::Auth
+    include Spree::Core::ControllerHelpers::Order
+  }
 
   let(:user) { create(:user) }
   let(:order) { create(:order, user: user, store: store) }
diff --git a/core/spec/lib/spree/core/controller_helpers/pricing_spec.rb b/core/spec/lib/spree/core/controller_helpers/pricing_spec.rb
index 8b7ecee7613..7309f05d180 100644
--- a/core/spec/lib/spree/core/controller_helpers/pricing_spec.rb
+++ b/core/spec/lib/spree/core/controller_helpers/pricing_spec.rb
@@ -2,12 +2,11 @@
 
 require 'rails_helper'
 
-class FakesController < ApplicationController
-  include Spree::Core::ControllerHelpers::Pricing
-end
-
 RSpec.describe Spree::Core::ControllerHelpers::Pricing, type: :controller do
-  controller(FakesController) {}
+  controller(ApplicationController) {
+    include Spree::Core::ControllerHelpers::Store
+    include Spree::Core::ControllerHelpers::Pricing
+  }
 
   before do
     allow(controller).to receive(:current_store).and_return(store)
diff --git a/core/spec/lib/spree/core/controller_helpers/search_spec.rb b/core/spec/lib/spree/core/controller_helpers/search_spec.rb
index 180062f5b18..f54435a5794 100644
--- a/core/spec/lib/spree/core/controller_helpers/search_spec.rb
+++ b/core/spec/lib/spree/core/controller_helpers/search_spec.rb
@@ -2,12 +2,12 @@
 
 require 'rails_helper'
 
-class FakesController < ApplicationController
-  include Spree::Core::ControllerHelpers::Search
-end
-
 RSpec.describe Spree::Core::ControllerHelpers::Search, type: :controller do
-  controller(FakesController) {}
+  controller(ApplicationController) {
+    include Spree::Core::ControllerHelpers::Auth
+    include Spree::Core::ControllerHelpers::Pricing
+    include Spree::Core::ControllerHelpers::Search
+  }
 
   describe '#build_searcher' do
     it 'returns Spree::Core::Search::Base instance' do
diff --git a/core/spec/lib/spree/core/controller_helpers/store_spec.rb b/core/spec/lib/spree/core/controller_helpers/store_spec.rb
index 8258301cc3f..8806f557953 100644
--- a/core/spec/lib/spree/core/controller_helpers/store_spec.rb
+++ b/core/spec/lib/spree/core/controller_helpers/store_spec.rb
@@ -2,12 +2,10 @@
 
 require 'rails_helper'
 
-class FakesController < ApplicationController
-  include Spree::Core::ControllerHelpers::Store
-end
-
 RSpec.describe Spree::Core::ControllerHelpers::Store, type: :controller do
-  controller(FakesController) {}
+  controller(ApplicationController) {
+    include Spree::Core::ControllerHelpers::Store
+  }
 
   describe '#current_store' do
     let!(:store) { create :store, default: true }
diff --git a/core/spec/lib/spree/core/controller_helpers/strong_parameters_spec.rb b/core/spec/lib/spree/core/controller_helpers/strong_parameters_spec.rb
index 6beafd04878..aff6092626c 100644
--- a/core/spec/lib/spree/core/controller_helpers/strong_parameters_spec.rb
+++ b/core/spec/lib/spree/core/controller_helpers/strong_parameters_spec.rb
@@ -2,12 +2,10 @@
 
 require 'rails_helper'
 
-class FakesController < ApplicationController
-  include Spree::Core::ControllerHelpers::StrongParameters
-end
-
 RSpec.describe Spree::Core::ControllerHelpers::StrongParameters, type: :controller do
-  controller(FakesController) {}
+  controller(ApplicationController) {
+    include Spree::Core::ControllerHelpers::StrongParameters
+  }
 
   describe '#permitted_attributes' do
     it 'returns Spree::PermittedAttributes module' do

From f40f6274f342cc25860b4d530937d52f7bfe3bd7 Mon Sep 17 00:00:00 2001
From: Elia Schito <elia@schito.me>
Date: Mon, 3 Feb 2020 16:02:46 +0100
Subject: [PATCH 5/7] Let Order#record_ip_address work with new records

If the order is being built it should keep setting the last_ip_address
on the instance.
---
 core/app/models/spree/order.rb       | 4 +++-
 core/spec/models/spree/order_spec.rb | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 020e98ba18d..32378adcf97 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -778,7 +778,9 @@ def add_default_payment_from_wallet
     deprecate assign_default_credit_card: :add_default_payment_from_wallet, deprecator: Spree::Deprecation
 
     def record_ip_address(ip_address)
-      if last_ip_address != ip_address
+      if new_record?
+        self.last_ip_address = ip_address
+      elsif last_ip_address != ip_address
         update_column(:last_ip_address, ip_address)
       end
     end
diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb
index cc02cd7348b..74c555cb02f 100644
--- a/core/spec/models/spree/order_spec.rb
+++ b/core/spec/models/spree/order_spec.rb
@@ -1526,6 +1526,14 @@ def generate
           expect(subject).to change(order, :last_ip_address).to(ip_address)
         end
       end
+
+      context "with a new order" do
+        let(:order) { build(:order) }
+
+        it "updates the IP address" do
+          expect(subject).to change(order, :last_ip_address).to(ip_address)
+        end
+      end
     end
 
     describe "#display_order_total_after_store_credit" do

From d90be784a65f1bb559f4458f4acf3557efbe9251 Mon Sep 17 00:00:00 2001
From: Elia Schito <elia@schito.me>
Date: Mon, 3 Feb 2020 16:06:26 +0100
Subject: [PATCH 6/7] Let current_order accept a build_order_if_necessary
 option

This allows to easily build valid orders with all the expected
metadata.
---
 core/lib/spree/core/controller_helpers/order.rb |  5 +++--
 .../spree/core/controller_helpers/order_spec.rb | 17 +++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/core/lib/spree/core/controller_helpers/order.rb b/core/lib/spree/core/controller_helpers/order.rb
index 83bc7e4e065..ec24368fa57 100644
--- a/core/lib/spree/core/controller_helpers/order.rb
+++ b/core/lib/spree/core/controller_helpers/order.rb
@@ -31,17 +31,18 @@ def simple_current_order
         # The current incomplete order from the guest_token for use in cart and during checkout
         def current_order(options = {})
           should_create = options[:create_order_if_necessary] || false
+          should_build = options[:build_order_if_necessary] || should_create
 
           return @current_order if @current_order
 
           @current_order = find_order_by_token_or_user(lock: options[:lock])
 
-          if should_create && (@current_order.nil? || @current_order.completed?)
+          if should_build && (@current_order.nil? || @current_order.completed?)
             @current_order = Spree::Order.new(new_order_params)
             @current_order.user ||= try_spree_current_user
             # See issue https://github.com/spree/spree/issues/3346 for reasons why this line is here
             @current_order.created_by ||= try_spree_current_user
-            @current_order.save!
+            @current_order.save! if should_create
           end
 
           if @current_order
diff --git a/core/spec/lib/spree/core/controller_helpers/order_spec.rb b/core/spec/lib/spree/core/controller_helpers/order_spec.rb
index 69992bd2dc0..61ad7a6149a 100644
--- a/core/spec/lib/spree/core/controller_helpers/order_spec.rb
+++ b/core/spec/lib/spree/core/controller_helpers/order_spec.rb
@@ -75,6 +75,23 @@
         }.from(nil).to("0.0.0.0")
       end
     end
+
+    context 'build_order_if_necessary option is true' do
+      subject { controller.current_order(build_order_if_necessary: true) }
+
+      it 'builds a new order' do
+        expect { subject }.not_to change(Spree::Order, :count).from(0)
+        expect(subject).not_to be_persisted
+      end
+
+      it 'assigns the current_store id' do
+        expect(subject.store_id).to eq store.id
+      end
+
+      it 'records last_ip_address' do
+        expect(subject.last_ip_address).to eq("0.0.0.0")
+      end
+    end
   end
 
   describe '#associate_user' do

From 030bf6cafa7be6bf688041f798d2acf53febe154 Mon Sep 17 00:00:00 2001
From: Elia Schito <elia@schito.me>
Date: Wed, 29 Jan 2020 15:03:11 +0100
Subject: [PATCH 7/7] Use current_order with build_if_necessary in cart page

Using current_order with the newly introduced `build_if_necessary`
option creates orders that are much more complete by adding info about
the ip, the creator, and other meta data. It's also more readable.
---
 .../app/controllers/spree/orders_controller.rb  |  2 +-
 .../controllers/spree/orders_controller_spec.rb | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/frontend/app/controllers/spree/orders_controller.rb b/frontend/app/controllers/spree/orders_controller.rb
index af7cbee9be7..bc5c9f46eb3 100644
--- a/frontend/app/controllers/spree/orders_controller.rb
+++ b/frontend/app/controllers/spree/orders_controller.rb
@@ -39,7 +39,7 @@ def update
 
     # Shows the current incomplete order from the session
     def edit
-      @order = current_order || Spree::Order.incomplete.find_or_initialize_by(guest_token: cookies.signed[:guest_token])
+      @order = current_order(build_order_if_necessary: true)
       authorize! :read, @order, cookies.signed[:guest_token]
       associate_user
       if params[:id] && @order.number != params[:id]
diff --git a/frontend/spec/controllers/spree/orders_controller_spec.rb b/frontend/spec/controllers/spree/orders_controller_spec.rb
index 3833471aa42..a95c655f231 100644
--- a/frontend/spec/controllers/spree/orders_controller_spec.rb
+++ b/frontend/spec/controllers/spree/orders_controller_spec.rb
@@ -253,10 +253,23 @@
   end
 
   describe '#edit' do
-    it "doesn't create a new order" do
+    subject { get :edit }
+    let(:user) { build :user }
+
+    it "builds a new valid order with complete meta-data" do
       allow(controller).to receive_messages(try_spree_current_user: user)
 
-      expect { get :edit }.not_to change { Spree::Order.count }
+      subject
+
+      order = controller.instance_variable_get(:@order)
+
+      aggregate_failures do
+        expect(order).to be_valid
+        expect(order).not_to be_persisted
+        expect(order.store).to be_present
+        expect(order.user).to eq(user)
+        expect(order.created_by).to eq(user)
+      end
     end
   end
 end