8000 Merge pull request #5811 from irmela/make-count-query-optimisation-wo… · activeadmin/activeadmin@9a97cda · GitHub
[go: up one dir, main page]
Skip to content

Commit 9a97cda

Browse files
javierjuliojavierjulio
authored
Merge pull request #5811 from irmela/make-count-query-optimisation-work-when-using-decorate-with
Make count query optimisation work when using decorate_with
2 parents a2989f0 + 8e24320 commit 9a97cda

File tree

12 files changed

+102
-42
lines changed

12 files changed

+102
-42
lines changed

‎CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
## Unreleased
44

5+
### Enhancements
6+
7+
* Make optimization to not use expensive COUNT queries also work for decorated actions. [#5811](https://github.com/activeadmin/activeadmin/pull/5811) by [@irmela]
8+
59
## 2.3.1 [☰](https://github.com/activeadmin/activeadmin/compare/v2.3.0..v2.3.1)
610

711
### Bug Fixes
@@ -499,6 +503,7 @@ Please check [0-6-stable] for previous changes.
499503
[#5800]: https://github.com/activeadmin/activeadmin/pull/5800
500504
[#5801]: https://github.com/activeadmin/activeadmin/pull/5801
501505
[#5802]: https://github.com/activeadmin/activeadmin/pull/5802
506+
[#5811]: https://github.com/activeadmin/activeadmin/pull/5811
502507
[#5816]: https://github.com/activeadmin/activeadmin/pull/5816
503508
[#5822]: https://github.com/activeadmin/activeadmin/pull/5822
504509
[#5826]: https://github.com/activeadmin/activeadmin/pull/5826
@@ -577,3 +582,4 @@ Please check [0-6-stable] for previous changes.
577582
[@ndbroadbent]: https://github.com/ndbroadbent
578583
[@Kris-LIBIS]: https://github.com/Kris-LIBIS
579584
[@sgara]: https://github.com/sgara
585+
[@irmela]: https://github.com/irmela

‎Gemfile.common

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ gem "devise", "~> 4.7"
1414
group :test do
1515
gem 'apparition'
1616
gem 'capybara', '~> 3.14'
17+
gem 'db-query-matchers', '0.9.0'
1718

1819
gem 'simplecov', require: false # Test coverage generator. Go to /coverage/ after running tests
1920
gem 'cucumber-rails', '~> 1.5', require: false

‎Gemfile.lock

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,9 @@ GEM
136136
cucumber-tag_expressions (1.1.1)
137137
cucumber-wire (0.0.1)
138138
database_cleaner (1.7.0)
139+
db-query-matchers (0.9.0)
140+
activesupport (>= 4.0, <= 6.0)
141+
rspec (~> 3.0)
139142
devise (4.7.1)
140143
bcrypt (~> 3.0)
141144
orm_adapter (~> 0.1)
@@ -323,6 +326,10 @@ GEM
323326
responders (3.0.0)
324327
actionpack (>= 5.0)
325328
railties (>= 5.0)
329+
rspec (3.8.0)
330+
rspec-core (~> 3.8.0)
331+
rspec-expectations (~> 3.8.0)
332+
rspec-mocks (~> 3.8.0)
326333
rspec-core (3.8.2)
327334
rspec-support (~> 3.8.0)
328335
rspec-expectations (3.8.4)
@@ -417,6 +424,7 @@ DEPENDENCIES
417424
cucumber
418425
cucumber-rails (~> 1.5)
419426
database_cleaner
427+
db-query-matchers (= 0.9.0)
420428
devise (~> 4.7)
421429
draper (~> 3.1)
422430
i18n-spec

‎gemfiles/rails_50.gemfile.lock

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,9 @@ GEM
115115
cucumber-tag_expressions (1.1.1)
116116
cucumber-wire (0.0.1)
117117
database_cleaner (1.7.0)
118+
db-query-matchers (0.9.0)
119+
activesupport (>= 4.0, <= 6.0)
120+
rspec (~> 3.0)
118121
devise (4.7.1)
119122
bcrypt (~> 3.0)
120123
orm_adapter (~> 0.1)
@@ -261,6 +264,10 @@ GEM
261264
responders (3.0.0)
262265
actionpack (>= 5.0)
263266
railties (>= 5.0)
267+
rspec (3.8.0)
268+
rspec-core (~> 3.8.0)
269+
rspec-expectations (~> 3.8.0)
270+
rspec-mocks (~> 3.8.0)
264271
rspec-core (3.8.2)
265272
rspec-support (~> 3.8.0)
266273
rspec-expectations (3.8.4)
@@ -334,6 +341,7 @@ DEPENDENCIES
334341
cucumber
335342
cucumber-rails (~> 1.5)
336343
database_cleaner
344+
db-query-matchers (= 0.9.0)
337345
devise (~> 4.7)
338346
draper (~> 3.1)
339347
jasmine

‎gemfiles/rails_51.gemfile.lock

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,9 @@ GEM
115115
cucumber-tag_expressions (1.1.1)
116116
cucumber-wire (0.0.1)
117117
database_cleaner (1.7.0)
118+
db-query-matchers (0.9.0)
119+
activesupport (>= 4.0, <= 6.0)
120+
rspec (~> 3.0)
118121
devise (4.7.1)
119122
bcrypt (~> 3.0)
120123
orm_adapter (~> 0.1)
@@ -261,6 +264,10 @@ GEM
261264
responders (3.0.0)
262265
actionpack (>= 5.0)
263266
railties (>= 5.0)
267+
rspec (3.8.0)
268+
rspec-core (~> 3.8.0)
269+
rspec-expectations (~> 3.8.0)
270+
rspec-mocks (~> 3.8.0)
264271
rspec-core (3.8.2)
265272
rspec-support (~> 3.8.0)
266273
rspec-expectations (3.8.4)
@@ -334,6 +341,7 @@ DEPENDENCIES
334341
cucumber
335342
cucumber-rails (~> 1.5)
336343
database_cleaner
344+
db-query-matchers (= 0.9.0)
337345
devise (~> 4.7)
338346
draper (~> 3.1)
339347
jasmine

‎gemfiles/rails_52.gemfile.lock

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,9 @@ GEM
119119
cucumber-tag_expressions (1.1.1)
120120
cucumber-wire (0.0.1)
121121
database_cleaner (1.7.0)
122+
db-query-matchers (0.9.0)
123+
activesupport (>= 4.0, <= 6.0)
124+
rspec (~> 3.0)
122125
devise (4.7.1)
123126
bcrypt (~> 3.0)
124127
orm_adapter (~> 0.1)
@@ -269,6 +272,10 @@ GEM
269272
responders (3.0.0)
270273
actionpack (>= 5.0)
271274
railties (>= 5.0)
275+
rspec (3.8.0)
276+
rspec-core (~> 3.8.0)
277+
rspec-expectations (~> 3.8.0)
278+
rspec-mocks (~> 3.8.0)
272279
rspec-core (3.8.2)
273280
rspec-support (~> 3.8.0)
274281
rspec-expectations (3.8.4)
@@ -342,6 +349,7 @@ DEPENDENCIES
342349
cucumber
343350
cucumber-rails (~> 1.5)
344351
database_cleaner
352+
db-query-matchers (= 0.9.0)
345353
devise (~> 4.7)
346354
draper (~> 3.1)
347355
jasmine

‎gemfiles/rails_60_turbolinks.gemfile.lock

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,9 @@ GEM
132132
cucumber-tag_expressions (1.1.1)
133133
cucumber-wire (0.0.1)
134134
database_cleaner (1.7.0)
135+
db-query-matchers (0.9.0)
136+
activesupport (>= 4.0, <= 6.0)
137+
rspec (~> 3.0)
135138
devise (4.7.1)
136139
bcrypt (~> 3.0)
137140
orm_adapter (~> 0.1)
@@ -284,6 +287,10 @@ GEM
284287
responders (3.0.0)
285288
actionpack (>= 5.0)
286289
railties (>= 5.0)
290+
rspec (3.8.0)
291+
rspec-core (~> 3.8.0)
292+
rspec-expectations (~> 3.8.0)
293+
rspec-mocks (~> 3.8.0)
287294
rspec-core (3.8.2)
288295
rspec-support (~> 3.8.0)
289296
rspec-expectations (3.8.4)
@@ -361,6 +368,7 @@ DEPENDENCIES
361368
cucumber
362369
cucumber-rails (~> 1.5)
363370
database_cleaner
371+
db-query-matchers (= 0.9.0)
364372
devise (~> 4.7)
365373
draper (~> 3.1)
366374
jasmine

‎lib/active_admin/resource_controller/decorators.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,8 @@ def self.wrap(decorator)
6767
def self.wrap!(parent, name)
6868
::Class.new parent do
6969
delegate :reorder, :page, :current_page, :total_pages, :limit_value,
70-
:total_count, :total_pages, :to_key, :group_values, :except,
71-
:find_each, :ransack
70+
:total_count, :total_pages, :offset, :to_key, :group_values,
71+
:except, :find_each, :ransack
7272

7373
define_singleton_method(:name) { name }
7474
end

‎lib/active_admin/views/components/paginated_collection.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ def build_pagination
9696
options[:params] = @params if @params
9797
options[:param_name] = @param_name if @param_name
9898

99-
if !@display_total && collection.respond_to?(:offset)
99+
if !@display_total
100100
# The #paginate method in kaminari will query the resource with a
101101
# count(*) to determine how many pages there should be unless
102102
# you pass in the :total_pages option. We issue a query to determine

‎spec/support/active_admin_integration_spec_helper.rb

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,32 @@ def mock_action_view(base = MockActionView)
5353
base.new(view_paths, {}, controller)
5454
end
5555

56+
# Instantiates a fake decorated controller ready to unit test for a specific action
57+
def controller_with_decorator(action, decorator_class)
58+
method = action == "index" ? :apply_collection_decorator : :apply_decorator
59+
60+
controller_class = Class.new do
61+
include ActiveAdmin::ResourceController::Decorators
62+
63+
public method
64+
end
65+
66+
active_admin_config = double(decorator_class: decorator_class)
67+
68+
if action != "index"
69+
form_presenter = double(options: { decorate: !decorator_class.nil? })
70+
71+
allow(active_admin_config).to receive(:get_page_presenter).with(:form).and_return(form_presenter)
72+
end
73+
74+
controller = controller_class.new
75+
76+
allow(controller).to receive(:active_admin_config).and_return(active_admin_config)
77+
allow(controller).to receive(:action_name).and_return(action)
78+
79+
controller
80+
end
81+
5682
def view_paths
5783
paths = ActionController::Base.view_paths
5884
# the constructor for ActionView::Base changed from Rails 6

‎spec/unit/resource_controller/decorators_spec.rb

Lines changed: 12 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,58 +1,34 @@
11
require 'rails_helper'
22

33
RSpec.describe ActiveAdmin::ResourceController::Decorators do
4-
let(:controller_class) do
5-
Class.new do
6-
include ActiveAdmin::ResourceController::Decorators
7-
8-
public :apply_decorator, :apply_collection_decorator
9-
end
10-
end
11-
12-
let(:controller) { controller_class.new }
13-
let(:active_admin_config) { double(decorator_class: decorator_class) }
14-
before do
15-
allow(controller).to receive(:active_admin_config).and_return(active_admin_config)
16-
allow(controller).to receive(:action_name).and_return(action)
17-
end
18-
194
describe '#apply_decorator' do
20-
let(:action) { 'show' }
215
let(:resource) { Post.new }
6+
let(:controller) { controller_with_decorator(action, decorator_class) }
227
subject(:applied) { controller.apply_decorator(resource) }
238

24-
context 'with a decorator configured' do
9+
context "in show action" do
10+
let(:action) { 'show' }
2511
let(:decorator_class) { PostDecorator }
26-
it { is_expected.to be_kind_of(PostDecorator) }
27-
28-
context 'with form' do
29-
let(:action) { 'update' }
3012

31-
it "does not decorate when :decorate is set to false" do
32-
form = double
33-
allow(form).to receive(:options).and_return(decorate: false)
34-
allow(active_admin_config).to receive(:get_page_presenter).and_return(form)
35-
is_expected.not_to be_kind_of(PostDecorator)
36-
end
37-
end
13+
it { is_expected.to be_kind_of(PostDecorator) }
3814
end
3915

40-
context 'with no decorator configured' do
16+
context "in update action" do
17+
let(:action) { 'update' }
4118
let(:decorator_class) { nil }
42-
it { is_expected.to be_kind_of(Post) }
19+
20+
it { is_expected.not_to be_kind_of(PostDecorator) }
4321
end
4422
end
4523

4624
describe '#apply_collection_decorator' do
4725
before { Post.create! }
48-
let(:action) { 'index' }
4926
let(:collection) { Post.where nil }
27+
let(:controller) { controller_with_decorator("index", PostDecorator) }
5028
subject(:applied) { controller.apply_collection_decorator(collection) }
5129

5230
context 'when a decorator is configured' do
5331
context 'and it is using a recent version of draper' do
54-
let(:decorator_class) { PostDecorator }
55-
5632
it 'calling certain scope collections work' do
5733
# This is an example of one of the methods that was consistently
5834
# failing before this feature existed
@@ -67,21 +43,18 @@
6743
end
6844

6945
describe 'form actions' do
70-
let(:action) { 'edit' }
7146
let(:resource) { Post.new }
72-
let(:form_presenter) { double options: { decorate: decorate_form } }
73-
let(:decorator_class) { PostDecorator }
74-
before { allow(active_admin_config).to receive(:get_page_presenter).with(:form).and_return form_presenter }
47+
let(:controller) { controller_with_decorator("edit", decorator_class) }
7548

7649
subject(:applied) { controller.apply_decorator(resource) }
7750

7851
context 'when the form is not configured to decorate' do
79-
let(:decorate_form) { false }
52+
let(:decorator_class) { nil }
8053
it { is_expected.to be_kind_of(Post) }
8154
end
8255

8356
context 'when the form is configured to decorate' do
84-
let(:decorate_form) { true }
57+
let(:decorator_class) { PostDecorator }
8558
it { is_expected.to be_kind_of(PostDecorator) }
8659
end
8760
end

‎spec/unit/views/components/paginated_collection_spec.rb

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,20 @@ def paginated_collection(*args)
227227
end
228228
end
229229

230+
it "makes no expensive COUNT queries when pagination_total is false" do
231+
require 'db-query-matchers'
232+
233+
undecorated_collection = Post.all.page(1).per(30)
234+
235+
expect { paginated_collection(undecorated_collection, pagination_total: false) }
236+
.not_to make_database_queries(matching: "SELECT COUNT(*) FROM \"posts\"")
237+
238+
decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection)
239+
240+
expect { paginated_collection(decorated_collection, pagination_total: false) }
241+
.not_to make_database_queries(matching: "SELECT COUNT(*) FROM \"posts\"")
242+
end
243+
230244
context "when specifying per_page: array option" do
231245
let(:collection) do
232246
posts = 10.times.map { Post.new }

0 commit comments

Comments
 (0)
0