Merge pull request #3494 from nebulab/elia/frontend-orders-controller…

…-edit-current-order

Use create_if_necessary instead of a simple find_or_initialize

core/app/models/spree/order.rb

@@ -790,7 +790,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

core/lib/spree/core/controller_helpers/order.rb

@@ -30,18 +30,19 @@ 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
+          should_build = options[:build_order_if_necessary] || should_create
 
           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?)
+          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
@@ -84,14 +85,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

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/)

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) }
@@ -74,6 +75,23 @@ class FakesController < ApplicationController
         }.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

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)

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

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 }

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

core/spec/models/spree/order_spec.rb

@@ -1538,6 +1538,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

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]

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

frontend/spec/controllers/spree/orders_controller_spec.rb

@@ -251,4 +251,25 @@
       expect(order.reload.line_items.count).to eq 0
     end
   end
+
+  describe '#edit' 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)
+
+      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