From 77251848eed2e504b282ff8bf5e7862ba6be9fc5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 22 Oct 2018 22:09:24 +0100 Subject: [PATCH] Refactor Spree::Admin::OrdersController --- .../admin/orders_controller_decorator.rb | 80 ++----------------- app/services/search_orders.rb | 41 ++++++++++ .../spree/admin/orders/_filters.html.haml | 22 ++--- .../spree/admin/orders_controller_spec.rb | 7 +- 4 files changed, 63 insertions(+), 87 deletions(-) create mode 100644 app/services/search_orders.rb diff --git a/app/controllers/spree/admin/orders_controller_decorator.rb b/app/controllers/spree/admin/orders_controller_decorator.rb index fe8dc46445..4344ce98f7 100644 --- a/app/controllers/spree/admin/orders_controller_decorator.rb +++ b/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 diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb new file mode 100644 index 0000000000..b4ae35e2e4 --- /dev/null +++ b/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 diff --git a/app/views/spree/admin/orders/_filters.html.haml b/app/views/spree/admin/orders/_filters.html.haml index 5ebea2a4cd..29b16b19a0 100644 --- a/app/views/spree/admin/orders/_filters.html.haml +++ b/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) diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index 8d76ea3a8d..1685b9506a 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/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