Refactor Spree::Admin::OrdersController

app/controllers/spree/admin/orders_controller_decorator.rb

@@ -20,50 +20,17 @@ Spree::Admin::OrdersController.class_eval do
 
   before_filter :require_distributor_abn, only: :invoice
 
-
   respond_to :html, :json
 
-  # Mostly the original Spree method, tweaked to allow us to ransack with completed_at in a sane way
   def index
-    params[:q] ||= {}
-    params[:q][:completed_at_not_null] ||= '1' if Spree::Config[:show_only_complete_orders_by_default]
-    @show_only_completed = params[:q][:completed_at_not_null].present?
-    params[:q][:s] ||= @show_only_completed ? 'completed_at desc' : 'created_at desc'
+    @results = SearchOrders.new(params, spree_current_user) if json_request?
 
-    # As date params are deleted if @show_only_completed, store
-    # the original date so we can restore them into the params
-    # after the search
-    created_at_gt = params[:q][:created_at_gt]
-    created_at_lt = params[:q][:created_at_lt]
-
-    params[:q].delete(:inventory_units_shipment_id_null) if params[:q][:inventory_units_shipment_id_null] == "0"
-
-    if !params[:q][:created_at_gt].blank?
-      params[:q][:created_at_gt] = Time.zone.parse(params[:q][:created_at_gt]).beginning_of_day rescue ""
-    end
-
-    if !params[:q][:created_at_lt].blank?
-      params[:q][:created_at_lt] = Time.zone.parse(params[:q][:created_at_lt]).end_of_day rescue ""
-    end
-
-    # Changed this to stop completed_at being overriden when present
-    if @show_only_completed
-      params[:q][:completed_at_gt] = params[:q].delete(:created_at_gt) unless params[:q][:completed_at_gt]
-      params[:q][:completed_at_lt] = params[:q].delete(:created_at_lt) unless params[:q][:completed_at_gt]
-    end
-
-    @orders = orders
-
-    # Restore dates
-    params[:q][:created_at_gt] = created_at_gt
-    params[:q][:created_at_lt] = created_at_lt
-
-    respond_with(@orders) do |format|
+    respond_to do |format|
       format.html
       format.json do
         render json: {
-          orders: ActiveModel::ArraySerializer.new(@orders, each_serializer: Api::Admin::OrderSerializer),
-          pagination: pagination_data
+          orders: serialized_orders,
+          pagination: @results.pagination_data
         }
       end
     end
@@ -102,42 +69,6 @@ Spree::Admin::OrdersController.class_eval do
 
   private
 
-  def orders
-    if json_request?
-      @search = OpenFoodNetwork::Permissions.new(spree_current_user).editable_orders.ransack(params[:q])
-    else
-      @search = Spree::Order.accessible_by(current_ability, :index).ransack(params[:q])
-
-      # Replaced this search to filter orders to only show those distributed by current user (or all for admin user)
-      @search.result.includes([:user, :shipments, :payments]).distributed_by_user(spree_current_user)
-    end
-
-    search_results
-  end
-
-  def search_results
-    if using_pagination?
-      @search.result.page(params[:page]).per(params[:per_page] || Spree::Config[:orders_per_page])
-    else
-      @search.result
-    end
-  end
-
-  def using_pagination?
-    params[:per_page]
-  end
-
-  def pagination_data
-    if using_pagination?
-      {
-        results: @orders.total_count,
-        pages: @orders.num_pages,
-        page: params[:page].to_i,
-        per_page: params[:per_page].to_i
-      }
-    end
-  end
-
   def require_distributor_abn
     unless @order.distributor.abn.present?
       flash[:error] = t(:must_have_valid_business_number, enterprise_name: @order.distributor.name)
@@ -166,4 +97,7 @@ Spree::Admin::OrdersController.class_eval do
     end
   end
 
+  def serialized_orders
+    ActiveModel::ArraySerializer.new(@results.orders, each_serializer: Api::Admin::OrderSerializer)
+  end
 end

app/services/search_orders.rb

@@ -0,0 +1,41 @@
+class SearchOrders
+  attr_reader :orders
+
+  def initialize(params, current_user)
+    @params = params
+    @current_user = current_user
+
+    @orders = fetch_orders
+  end
+
+  def pagination_data
+    return unless using_pagination?
+    {
+      results: @orders.total_count,
+      pages: @orders.num_pages,
+      page: params[:page].to_i,
+      per_page: params[:per_page].to_i
+    }
+  end
+
+  private
+
+  attr_reader :params, :current_user
+
+  def fetch_orders
+    @search = OpenFoodNetwork::Permissions.new(current_user).editable_orders.ransack(params[:q])
+
+    return paginated_results if using_pagination?
+    @search.result
+  end
+
+  def paginated_results
+    @search.result
+      .page(params[:page])
+      .per(params[:per_page] || Spree::Config[:orders_per_page])
+  end
+
+  def using_pagination?
+    params[:per_page]
+  end
+end

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

@@ -1,38 +1,40 @@
 %div{"data-hook" => "admin_orders_index_search"}
-  = search_form_for [:admin, @search], html: { name: "orders_form", "ng-submit" => "fetchResults()"} do |f|
+  = form_tag false, {name: "orders_form", "ng-submit" => "fetchResults()"} do
     .field-block.alpha.four.columns
       .date-range-filter.field
         = label_tag nil, t(:date_range)
         .date-range-fields
-          = f.text_field :created_at_gt, class: 'datepicker', datepicker: 'q.created_at_gt', 'ng-model' => 'q.created_at_gt', :value => params[:q][:created_at_gt], :placeholder => t(:start)
+          = text_field_tag "q[created_at_gt]", nil, class: 'datepicker', datepicker: 'q.created_at_gt', 'ng-model' => 'q.created_at_gt', :placeholder => t(:start)
           %span.range-divider
             %i.icon-arrow-right
-          = f.text_field :created_at_lt, class: 'datepicker', datepicker: 'q.created_at_lt', 'ng-model' => 'q.created_at_lt', :value => params[:q][:created_at_lt], :placeholder => t(:stop)
+          = text_field_tag "q[created_at_lt]", nil, class: 'datepicker', datepicker: 'q.created_at_lt', 'ng-model' => 'q.created_at_lt', :placeholder => t(:stop)
       .field
         = label_tag nil, t(:status)
-        = f.select :state_eq, Spree::Order.state_machines[:state].states.collect {|s| [t("order_state.#{s.name}"), s.value]}, {:include_blank => true}, :class => 'select2', 'ng-model' => 'q.state_eq'
+        = select_tag("q[state_eq]",
+            options_for_select(Spree::Order.state_machines[:state].states.collect {|s| [t("order_state.#{s.name}"), s.value]}),
+            {include_blank: true, class: 'select2', 'ng-model' => 'q.state_eq'})
     .four.columns
       .field
         = label_tag nil, t(:order_number)
-        = f.text_field :number_cont, 'ng-model' => 'q.number_cont'
+        = text_field_tag "q[number_cont]", nil, 'ng-model' => 'q.number_cont'
       .field
         = label_tag nil, t(:email)
-        = f.email_field :email_cont, 'ng-model' => 'q.email_cont'
+        = email_field_tag "q[email_cont", nil, 'ng-model' => 'q.email_cont'
     .four.columns
       .field
         = label_tag nil, t(:first_name_begins_with)
-        = f.text_field :bill_address_firstname_start, :size => 25, 'ng-model' => 'q.bill_address_firstname_start'
+        = text_field_tag "q[bill_address_firstname_start]", nil, size: 25, 'ng-model' => 'q.bill_address_firstname_start'
       .field
         = label_tag nil, t(:last_name_begins_with)
-        = f.text_field :bill_address_lastname_start, :size => 25, 'ng-model' => 'q.bill_address_lastname_start'
+        = text_field_tag "q[bill_address_lastname_start]", nil, size: 25, 'ng-model' => 'q.bill_address_lastname_start'
     .omega.four.columns
       .field.checkbox
         %label
-          = f.check_box :completed_at_not_null, {:checked => @show_only_completed, 'ng-model' => 'q.completed_at_not_null'}, '1', ''
+          = check_box_tag "q[completed_at_not_null]", 1, true, {'ng-model' => 'q.completed_at_not_null'}
           = t(:show_only_complete_orders)
       .field.checkbox
         %label
-          = f.check_box :inventory_units_shipment_id_null, {'ng-model' => 'q.inventory_units_shipment_id_null'}, '1', '0'
+          = check_box_tag "q[inventory_units_shipment_id_null]", 1, false, {'ng-model' => 'q.inventory_units_shipment_id_null'}
           = t(:show_only_unfulfilled_orders)
     .field-block.alpha.eight.columns
       = label_tag nil, t(:distributors)

spec/controllers/spree/admin/orders_controller_spec.rb

@@ -70,14 +70,13 @@ describe Spree::Admin::OrdersController, type: :controller do
         order_attributes.all?{ |attr| keys.include? attr }.should == true
       end
 
-      it "sorts orders in descending id order" do
+      it "sorts orders in ascending id order" do
         ids = json_response['orders'].map{ |order| order['id'] }
-        ids[0].should > ids[1]
-        ids[1].should > ids[2]
+        expect(ids[0]).to be < ids[1]
+        expect(ids[1]).to be < ids[2]
       end
 
       it "formats completed_at to 'yyyy-mm-dd hh:mm'" do
-        pp json_response
         json_response['orders'].map{ |order| order['completed_at'] }.all?{ |a| a == order1.completed_at.strftime('%B %d, %Y') }.should == true
       end