Skip to content

Commit

Permalink
Fix a N+1 query problem in the orders controller
Browse files Browse the repository at this point in the history
Fixes spree/spree#9026.

Signed-off-by: Rémy Coutable <remy@rymai.me>
  • Loading branch information
rymai committed Oct 5, 2018
1 parent ed9e634 commit 1245a3a
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 19 deletions.
12 changes: 1 addition & 11 deletions backend/app/controllers/spree/admin/orders_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,7 @@ def index
end

@search = Spree::Order.accessible_by(current_ability, :index).ransack(params[:q])

# lazy loading other models here (via includes) may result in an invalid query
# e.g. SELECT DISTINCT DISTINCT "spree_orders".id, "spree_orders"."created_at" AS alias_0 FROM "spree_orders"
# see https://github.com/spree/spree/pull/3919
@orders = if query_present
@search.result(distinct: true)
else
@search.result
end

@orders = @orders.
@orders = @search.result.includes([:user]).
page(params[:page]).
per(params[:per_page] || Spree::Config[:orders_per_page])

Expand Down
28 changes: 20 additions & 8 deletions backend/spec/controllers/spree/admin/orders_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
it "can page through the orders" do
get :index, params: { page: 2, per_page: 10 }
expect(assigns[:orders].offset_value).to eq(10)
expect(assigns[:orders].current_per_page).to eq(10)
expect(assigns[:orders].limit_value).to eq(10)
end
end

Expand Down Expand Up @@ -291,15 +291,27 @@
allow(controller).to receive_messages spree_current_user: user
user.spree_roles << Spree::Role.find_or_create_by(name: 'admin')

create(:completed_order_with_totals)
expect(Spree::Order.count).to eq 1
create_list(:completed_order_with_totals, 2)
expect(Spree::Order.count).to eq 2
end

it "does not display duplicated results" do
get :index, params: { q: {
line_items_variant_id_in: Spree::Order.first.variants.map(&:id)
} }
expect(assigns[:orders].map(&:number).count).to eq 1
context 'by line_items_variant_id_in' do
it "does not display duplicated results" do
get :index, params: { q: {
line_items_variant_id_in: Spree::Order.first.variants.map(&:id)
} }
expect(assigns[:orders].size).to eq 1
end
end

context 'by email' do
it "does not display duplicated results" do
get :index, params: { q: {
email_start: Spree::Order.first.email
} }
expect(assigns[:orders].size).to eq 1
expect(assigns[:orders][0].email).to eq(Spree::Order.first.email)
end
end
end

Expand Down

0 comments on commit 1245a3a

Please sign in to comment.