Skip to content
Permalink
Browse files

Remove n+1 select when showing lists of publications.

Two things were causing the n+1.  First, to determine the background colour for each listed publication, the 'root' document organisation was chosen.  With the many-to-many relationship between organisations this was expensive.  I've changed this to no longer search for the root; instead the first organisation is chosen.  This means that documents belonging to 'child' organisations may not have a background colour yet, but these can easily be added to the css if required.  There was no guarantee that even 'root' organisations had colours assigned, and most publications belong to 'root' organisation, so this is only a minor regression.

The second change is in the way the thumbnail image is chosen.  It previously used a special scope to find the first PDF attachment and using that.  Now this filtering is done in ruby, allowing us to `include` the attachments association (and thereby avoid the n+1).
  • Loading branch information...
tomafro committed May 23, 2012
1 parent e0b0cc2 commit 1d393945af0525c92d25b75b9e1b614b9f78596f
@@ -26,7 +26,7 @@ def load_policy_topics
end

def all_publications
Publication.published_in_reverse_chronological_order.includes(:document_identity)
Publication.published_in_reverse_chronological_order.includes(:document_identity, :organisations, :attachments)
end

def document_class
@@ -12,8 +12,8 @@ def change_history(document)
end

def document_organisation_class(document)
if document.organisations.first
document.organisations.first.root_organisation.slug
if organisation = document.organisations.first
organisation.slug
else
'unknown_organisation'
end
@@ -7,8 +7,6 @@ class Attachment < ActiveRecord::Base

before_save :update_file_attributes

scope :pdf, where(content_type: AttachmentUploader::PDF_CONTENT_TYPE)

def filename
url && File.basename(url)
end
@@ -31,7 +31,7 @@ def thumbnail_url
end

def thumbnailable_attachments
attachments.pdf
attachments.select {|a| a.content_type == AttachmentUploader::PDF_CONTENT_TYPE}
end

def indexable_content
@@ -20,22 +20,17 @@ class PublicationsControllerTest < ActionController::TestCase
refute_select_object(draft_publication)
end

test "should avoid n+1 queries" do
publications = []
published_publications = mock("published_publications")
published_publications.expects(:includes).with(:document_identity).returns(publications)
published_publications.expects(:order).returns(published_publications)
Publication.expects(:published).returns(published_publications)

get :index
end

test 'show displays published publications' do
published_publication = create(:published_publication)
get :show, id: published_publication.document_identity
assert_response :success
end

test 'should avoid n+1 selects when showing index' do
10.times { create(:published_publication) }
assert 10 > count_queries { get :index }
end

test "should show inapplicable nations" do
published_publication = create(:published_publication)
northern_ireland_inapplicability = published_publication.nation_inapplicabilities.create!(nation: Nation.northern_ireland, alternative_url: "http://northern-ireland.com/")
@@ -166,11 +161,17 @@ def given_two_publications_in_two_policy_topics
assert_select_object @published_in_second_policy_topic
end

test 'should avoid n+1 selects when filtering by policy topics' do
policy = create(:published_policy)
policy_topic = create(:policy_topic, policies: [policy])
10.times { create(:published_publication, related_policies: [policy]) }
assert 10 > count_queries { get :by_policy_topic, policy_topics: policy_topic }
end

test "should show a helpful message if there are no matching publications" do
policy_topic = create(:policy_topic)
get :by_policy_topic, policy_topics: policy_topic.slug

assert_select "p", text: "There are no matching publications."
end

end
@@ -15,6 +15,17 @@ def should_be_a_public_facing_controller
end
end

def count_queries
count = 0
subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
count = count + 1
end
yield
count
ensure
ActiveSupport::Notifications.unsubscribe(subscriber)
end

def govspeak_transformation_fixture(transformation, &block)
methods_to_stub = %w{govspeak_to_html govspeak_to_admin_html}
begin
@@ -143,11 +143,4 @@ class AttachmentTest < ActiveSupport::TestCase
attachment = build(:attachment, file: greenpaper_pdf)
assert_equal "pdf", attachment.file_extension
end

test "should return PDF attachments" do
greenpaper_pdf = create(:attachment, file: fixture_file_upload('greenpaper.pdf', 'application/pdf'))
sample_csv = create(:attachment, file: fixture_file_upload('sample-from-excel.csv', 'text/csv'))
two_pages_pdf = create(:attachment, file: fixture_file_upload('two-pages.pdf'))
assert_equal [greenpaper_pdf, two_pages_pdf], Attachment.pdf
end
end
@@ -7,13 +7,6 @@ class DocumentHelperTest < ActionView::TestCase
assert_equal organisations.first.slug, document_organisation_class(document)
end

test "#document_organisation_class returns the slug of the root organisation of the first document" do
parent_org = create(:ministerial_department)
child_org = create(:organisation, parent_organisations: [parent_org])
document = create(:document, organisations: [child_org])
assert_equal parent_org.slug, document_organisation_class(document)
end

test '#document_organisation_class returns "no_organisation" if doc has no organisation' do
document = create(:document)
assert_equal 'unknown_organisation', document_organisation_class(document)

0 comments on commit 1d39394

Please sign in to comment.
You can’t perform that action at this time.