Merge pull request #2 from Matt-Yorkley/fix-order-progressing-automatically-on-BOO

Fix pre-existing issues with admin order controller

app/controllers/api/v0/shipments_controller.rb

@@ -21,7 +21,9 @@ module Api
         @shipment.refresh_rates
         @shipment.save!
 
-        render json: @shipment.reload, serializer: Api::ShipmentSerializer, status: :ok
+        OrderWorkflow.new(@order).advance_to_payment if @order.line_items.any?
+
+        render json: @shipment, serializer: Api::ShipmentSerializer, status: :ok
       end
 
       def update

app/controllers/spree/admin/orders_controller.rb

@@ -8,9 +8,8 @@ module Spree
       include OpenFoodNetwork::SpreeApiKeyLoader
       helper CheckoutHelper
 
-      before_action :load_order, only: [:edit, :update, :fire, :resend,
-                                        :invoice, :print, :distribution]
-      before_action :load_distribution_choices, only: [:new, :edit, :update, :distribution]
+      before_action :load_order, only: [:edit, :update, :fire, :resend, :invoice, :print]
+      before_action :load_distribution_choices, only: [:new, :create, :edit, :update]
       before_action :require_distributor_abn, only: :invoice
       before_action :restore_saved_query!, only: :index
 
@@ -24,47 +23,35 @@ module Spree
       end
 
       def new
-        @order = Order.create
-        @order.created_by = spree_current_user
-        @order.generate_order_number
-        @order.save
-        redirect_to spree.distribution_admin_order_path(@order)
-      end
-
-      def distribution
-        return if order_params.blank?
-
-        on_update
-
-        @order.assign_attributes(order_params)
-        return unless @order.save(context: :set_distribution_step)
-
-        redirect_to spree.admin_order_customer_path(@order)
+        @order = Spree::Order.new
       end
 
       def edit
         @order.shipments.map(&:refresh_rates)
-        @order.errors.clear
+      end
+
+      def create
+        @order = Spree::Order.new(order_params.merge(created_by: spree_current_user))
+
+        if @order.save(context: :require_distribution)
+          redirect_to spree.admin_order_customer_path(@order)
+        else
+          render :new
+        end
       end
 
       def update
         on_update
 
-        if params[:set_distribution_step] && @order.update(order_params)
-          OrderWorkflow.new(@order).advance_to_payment if @order.state.in? ["cart", "address", "delivery"]
-          return redirect_to spree.admin_order_customer_path(@order)
-        end
-
-        unless order_params.present? && @order.update(order_params) && @order.line_items.present?
-          if @order.line_items.empty? && !params[:suppress_error_msg]
-            @order.errors.add(:line_items, Spree.t('errors.messages.blank'))
-          end
+        order_updated = order_params.present? && @order.update(order_params)
 
+        unless order_updated && line_items_present?
           flash[:error] = @order.errors.full_messages.join(', ') if @order.errors.present?
           return redirect_to spree.edit_admin_order_path(@order)
         end
 
-        OrderWorkflow.new(@order).advance_to_payment if @order.state.in? ["cart", "address", "delivery"]
+        OrderWorkflow.new(@order).advance_to_payment
+
         if @order.complete?
           redirect_to spree.edit_admin_order_path(@order)
         else
@@ -117,6 +104,13 @@ module Spree
 
       private
 
+      def line_items_present?
+        return true if @order.line_items.any?
+
+        @order.errors.add(:line_items, Spree.t('errors.messages.blank'))
+        false
+      end
+
       def update_search_results
         session[:admin_orders_search] = search_params
 

app/models/spree/order.rb

@@ -89,7 +89,7 @@ module Spree
 
     # Needs to happen before save_permalink is called
     before_validation :set_currency
-    before_validation :generate_order_number, on: :create
+    before_validation :generate_order_number, if: :new_record?
     before_validation :clone_billing_address, if: :use_billing?
     before_validation :ensure_customer
 
@@ -104,8 +104,9 @@ module Spree
     validates :email, presence: true,
                       format: /\A([\w.%+\-']+)@([\w\-]+\.)+(\w{2,})\z/i,
                       if: :require_email
-    validates :order_cycle, presence: true, on: :set_distribution_step
-    validates :distributor, presence: true, on: :set_distribution_step
+
+    validates :order_cycle, presence: true, on: :require_distribution
+    validates :distributor, presence: true, on: :require_distribution
 
     make_permalink field: :number
 
@@ -290,8 +291,9 @@ module Spree
                                                    created_by_id: created_by_id)
     end
 
-    # FIXME refactor this method and implement validation using validates_* utilities
     def generate_order_number
+      return if number.present?
+
       record = true
       while record
         random = "R#{Array.new(9){ rand(9) }.join}"

app/services/order_workflow.rb

@@ -24,6 +24,8 @@ class OrderWorkflow
   end
 
   def advance_to_payment
+    return unless order.state.in? ["cart", "address", "delivery"]
+
     advance_to_state("payment", advance_order_options)
   end
 

app/views/spree/admin/orders/_form.html.haml

@@ -5,24 +5,26 @@
   - if !@order.completed? && @order.insufficient_stock_lines.any?
     = render 'spree/admin/orders/insufficient_stock_lines', insufficient_stock_lines: @order.insufficient_stock_lines
 
-  = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => order }
-  - if order.line_items.exists?
+  - if @order.shipments.any?
+    = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => @order }
+
+  - if @order.line_items.exists?
     = render partial: "spree/admin/orders/note", locals: { order: @order }
 
   = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.line_item_adjustments, :title => t(".line_item_adjustments")}
   = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => order_adjustments_for_display(@order), :title => t(".order_adjustments")}
 
-  - if order.line_items.exists?
+  - if @order.line_items.exists?
     %fieldset#order-total.no-border-bottom.order-details-total
       %legend{ align: 'center' }= t(".order_total")
-      %span.order-total= order.display_total
+      %span.order-total= @order.display_total
 
-  = form_for @order, url: spree.admin_order_url(@order), method: :put do |f|
-    = render partial: 'spree/admin/orders/_form/distribution_fields'
+    = form_for @order, url: spree.admin_order_url(@order), method: :put do |f|
+      = render partial: 'spree/admin/orders/_form/distribution_fields'
 
-    .filter-actions.actions{"ng-show" => "distributionChosen()"}
-      = button t(:update_and_recalculate_fees), 'icon-refresh'
-      = link_to_with_icon 'button icon-arrow-left', t(:back), spree.admin_orders_url
+      .filter-actions.actions{"ng-show" => "distributionChosen()"}
+        = button t(:update_and_recalculate_fees), 'icon-refresh'
+        = link_to_with_icon 'button icon-arrow-left', t(:back), spree.admin_orders_url
 
   = javascript_tag do
     var order_number = '#{@order.number}';

app/views/spree/admin/orders/customer_details/edit.html.haml

@@ -22,6 +22,6 @@
 
 = render :partial => 'spree/shared/error_messages', :locals => { :target => @order }
 
-= form_for @order, :url => admin_order_customer_url(@order) do |f|
+= form_for @order, :url => admin_order_customer_path(@order) do |f|
   = render 'form', :f => f
   = f.hidden_field :customer_id, value: @order.customer_id, id: "customer_id"

app/views/spree/admin/orders/distribution.html.haml

@@ -1,27 +0,0 @@
-- content_for :page_actions do
-  %li= button_link_to t(:back_to_orders_list), spree.admin_orders_path, :icon => 'icon-arrow-left'
-
-= admin_inject_shops(module: 'admin.orders')
-= admin_inject_order_cycles
-
-- content_for :page_title do
-  = t(:new_order)
-  \#
-  = @order.number
-
-= render 'spree/admin/shared/order_tabs', :current => 'Customer Details'
-
-= csrf_meta_tags
-
-%div
-  = render 'spree/shared/error_messages', :target => @order
-
-%div{"ng-app" => "admin.orders", "ng-controller" => "orderCtrl"}
-  = form_for @order, url: distribution_admin_order_path(@order), method: :put do |f|
-    = render 'spree/admin/orders/_form/distribution_fields'
-    -# This param passed to stop validation error in next page due to no line items in order yet:
-    = hidden_field_tag 'suppress_error_msg', "true"
-    = hidden_field_tag "set_distribution_step", "true"
-    = button_tag :class => 'secondary radius expand small', :id => 'update-button' do
-      %i.icon-arrow-right
-      = t(:next)

app/views/spree/admin/orders/edit.html.haml

@@ -16,10 +16,7 @@
 = render partial: "spree/admin/shared/order_tabs", locals: { current: 'Order Details' }
 
 %div
-  -# Suppress errors when manually creating a new order - needs to proceed to edit page
-  -# without having line items (which otherwise gives a validation error)
-  - unless params["suppress_error_msg"]
-    = render partial: "spree/shared/error_messages", :locals => { :target => @order }
+  = render partial: "spree/shared/error_messages", locals: { target: @order }
 
   = admin_inject_shops(module: 'admin.orders')
   = admin_inject_order_cycles
@@ -32,4 +29,4 @@
         = Spree.t(:your_order_is_empty_add_product)
 
     %div.admin-order-edit-form
-      = render :partial => 'form', :locals => { :order => @order }
+      = render partial: 'form'

app/views/spree/admin/orders/new.html.haml

@@ -1,7 +1,5 @@
 - content_for :page_title do
   = t(:new_order)
-  \#
-  = @order.number
 
 - content_for :page_actions do
   %li= button_link_to t(:back_to_orders_list), spree.admin_orders_path, :icon => 'icon-arrow-left'
@@ -9,17 +7,14 @@
 = admin_inject_shops(module: 'admin.orders')
 = admin_inject_order_cycles
 
-= render 'spree/admin/shared/order_tabs', :current => 'Order Details'
-
-= csrf_meta_tags
+= render 'spree/admin/shared/order_tabs', current: 'Order Details'
 
 %div
-  = render 'spree/shared/error_messages', :target => @order
+  = render 'spree/shared/error_messages', target: @order
 
 %div{"ng-app" => "admin.orders", "ng-controller" => "orderCtrl"}
-  %div{"ng-show" => "distributionChosen()"}
-    = render 'add_product'
-
-  - unless @order.line_items.any?
-    %div
-      = render 'form'
+  = form_for @order, url: spree.admin_orders_path, method: :post do
+    = render 'spree/admin/orders/_form/distribution_fields'
+    = button_tag class: 'secondary radius expand small', id: 'update-button' do
+      %i.icon-arrow-right
+      = t(:next)

app/views/spree/admin/shared/_routes.html.erb

@@ -10,6 +10,6 @@
     :taxons_search         => main_app.api_v0_taxons_url(:format => 'json'),
     :orders_api            => main_app.api_v0_orders_url,
     :states_search         => main_app.api_v0_states_url(:format => 'json'),
-    :cancel_order          => spree.fire_admin_order_url(id: @order ? @order.number : "", e: 'cancel')
+    :cancel_order          => spree.fire_admin_order_url(id: @order&.number ? @order.number : "", e: 'cancel')
   }.to_json %>;
 </script>

config/routes/spree.rb

@@ -89,8 +89,6 @@ Spree::Core::Engine.routes.draw do
         get :resend
         get :invoice
         get :print
-        get :distribution
-        put :distribution
       end
 
       collection do

spec/models/spree/order_spec.rb

@@ -40,6 +40,7 @@ describe Spree::Order do
   context "#generate_order_number" do
     it "should generate a random string" do
       expect(order.generate_order_number.is_a?(String)).to be_truthy
+      order.number = nil
       expect(!order.generate_order_number.to_s.empty?).to be_truthy
     end
   end

spec/system/admin/order_spec.rb

@@ -66,17 +66,13 @@ describe '
     expect(page).not_to have_content "Line items can't be blank"
 
     expect(page).to have_selector 'h1', text: 'Customer Details'
-    o = Spree::Order.last
-    expect(o.distributor).to eq(distributor)
-    expect(o.order_cycle).to eq(order_cycle)
+
+    order = Spree::Order.last
+    expect(order.distributor).to eq(distributor)
+    expect(order.order_cycle).to eq(order_cycle)
+    expect(order.line_items.count).to be_zero
 
     click_link "Order Details"
-    click_button "Update And Recalculate Fees"
-    expect(page).to have_selector '.flash.error'
-    expect(page).to have_content "Line items can't be blank"
-
-    # it suppresses validation errors when setting distribution
-    expect(page).not_to have_selector '#errorExplanation'
     expect(page).to have_content 'ADD PRODUCT'
     select2_select product.name, from: 'add_variant_id', search: true
     find('button.add_variant').click
@@ -84,7 +80,7 @@ describe '
     page.has_selector?("table.index tbody tr")
     expect(page).to have_selector 'td', text: product.name
 
-    click_button 'Update'
+    expect(order.reload.line_items.count).to be_positive
   end
 
   context "can't create an order without selecting a distributor nor an order cycle" do
@@ -116,61 +112,50 @@ describe '
     end
   end
 
-  context "when order has a distributor and order cycle" do
-    before do
-      order.distributor = distributor
-      order.order_cycle = order_cycle
-      order.save!
-      login_as_admin
-      visit spree.distribution_admin_order_path(order)
-    end
-
-    it "can access the `/distribution` step" do
-      expect(current_path).to eq spree.distribution_admin_order_path(order)
-      expect(page).to have_content "DISTRIBUTION"
-    end
-
-    it "shows the distributor and order cycle" do
-      expect(page).to have_content distributor.name
-      expect(page).to have_content order_cycle.name
-    end
-
-    it "shows links to other steps" do
-      expect(page).to have_content "CUSTOMER DETAILS"
-      expect(page).to have_content "ORDER DETAILS"
-      expect(page).to have_content "PAYMENTS"
-      expect(page).to have_content "ADJUSTMENTS"
-    end
-  end
-
   context "when creating an order with a customer-only" do
+    let!(:order) { create(:order, distributor: distributor, order_cycle: order_cycle) }
     let(:customer2) { create(:customer, enterprise: distributor) }
     let(:customer3) { create(:customer, enterprise: distributor) }
 
     before do
       login_as_admin
-      visit spree.new_admin_order_path
-      select2_select distributor.name, from: 'order_distributor_id'
-      select2_select order_cycle.name, from: 'order_order_cycle_id'
-      click_button 'Next'
-      expect(Spree::Order.last.customer_id).to be_nil
+      visit spree.admin_order_customer_path(order)
+    end
+
+    it "sets the customer on the order" do
+      expect(order.customer_id).to be_nil
+
       tomselect_search_and_select customer2.email, from: 'customer_search_override'
       check 'order_use_billing'
       click_button 'Update'
       expect(page).to have_content 'Customer Details updated'
+
+      expect(order.reload.customer).to eq customer2
     end
 
-    it "set the right customer attached to the order" do
-      expect(Spree::Order.last.reload.customer).to eq customer2
-    end
+    context "when changing the attached customer" do
+      before do
+        order.update(
+          customer: customer2,
+          email: customer2.email,
+          ship_address: customer2.ship_address,
+          bill_address: customer2.bill_address
+        )
+        visit spree.admin_order_customer_path(order)
+      end
+
+      it "should update the order customer (not only its details)" do
+        expect(page).to have_field 'order_email', with: customer2.email
+        tomselect_search_and_select customer3.email, from: 'customer_search_override'
+        check 'order_use_billing'
 
-    it "when changing the attached customer,"\
-       "it should update the order customer (not only its details)" do
-      tomselect_search_and_select customer3.email, from: 'customer_search_override'
-      expect do
-        click_button 'Update'
         expect(page).to have_field 'order_email', with: customer3.email
-      end.to change { Spree::Order.last.reload.customer }.from(customer2).to(customer3)
+
+        expect do
+          click_button 'Update'
+          expect(page).to have_content 'Customer Details updated'
+        end.to change { order.reload.customer }.from(customer2).to(customer3)
+      end
     end
   end
 
@@ -328,7 +313,6 @@ describe '
         within(".modal") do
           expect do
             click_on("OK")
-            expect(page).not_to have_css(".modal")
           end.to change { order.reload.line_items.length }.by(-1)
         end
       end
@@ -425,14 +409,7 @@ describe '
       fill_in "order_bill_address_attributes_city", with: "Kansas"
       fill_in "order_bill_address_attributes_zipcode", with: "SP1 M11"
 
-      # The country is already selected and we avoid re-selecting it here
-      # because it would trigger an API call which we would need to wait for
-      # while we have no visual indicator for the wait.
-      #
-      # Ideally, we would implement a visual cue like disable the state field
-      # while the data is fetched from the API.
-      #
-      # select "Australia", from: "order_bill_address_attributes_country_id"
+      select "Australia", from: "order_bill_address_attributes_country_id"
       select "Victoria", from: "order_bill_address_attributes_state_id"
       fill_in "order_bill_address_attributes_phone", with: "111 1111 1111"
 
@@ -511,6 +488,7 @@ describe '
 
     # And I select that customer's email address and save the order
     tomselect_search_and_select customer.email, from: 'customer_search_override'
+    expect(page).to have_field "order_email", with: customer.email
     click_button 'Update'
 
     # Then their addresses should be associated with the order