Skip to content

Commit 97618fe

Browse files
committed
Reduce number of queries when saving form document
Previously, we would run a SQL query per page in the form to get the next page, and also to get the conditions for the page. Use `includes` so that we get the pages and all their conditions by running fewer queries. Also accept the `next_page` as an argument to Page#as_form_document_step as we already know what the next_page is when we call it from Form#as_form_document so we can avoid an additional query per page.
1 parent 33a5ab9 commit 97618fe

5 files changed

Lines changed: 23 additions & 12 deletions

File tree

app/models/form.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,11 @@ def email_task_status_service
181181
end
182182

183183
def steps
184-
pages.map(&:as_form_document_step)
184+
ordered_pages = pages.includes(:routing_conditions).to_a
185+
ordered_pages.map.with_index do |page, index|
186+
next_page = ordered_pages.fetch(index + 1, nil)
187+
page.as_form_document_step(next_page)
188+
end
185189
end
186190

187191
def start_page

app/models/page.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,11 @@ def show_optional_suffix?
8989
is_optional? && answer_type != "selection"
9090
end
9191

92-
def as_form_document_step
92+
def as_form_document_step(next_page)
9393
{
9494
"id" => id,
9595
"position" => position,
96-
"next_step_id" => next_page,
96+
"next_step_id" => next_page&.id,
9797
"type" => "question_page",
9898
"data" => slice(*%w[question_text hint_text answer_type is_optional answer_settings page_heading guidance_markdown is_repeatable]),
9999
"routing_conditions" => routing_conditions.map(&:as_form_document_condition),

spec/models/form_document/step_spec.rb

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,15 @@
44
subject(:form_document_step) { described_class.new(page_as_form_document_step) }
55

66
let(:page) { create :page }
7-
let(:page_as_form_document_step) { page.as_form_document_step }
7+
let(:next_page) { create :page }
8+
let(:page_as_form_document_step) { page.as_form_document_step(next_page) }
89

910
it "ignores any attributes that are not defined" do
1011
expect(described_class.new(foo: "bar").attributes).not_to include(:foo)
1112
end
1213

13-
it "has the position and next page ID from the original page" do
14-
expect(form_document_step).to have_attributes(position: page.position, next_step_id: page.next_page)
14+
it "has the position and next page ID from the form_document_step" do
15+
expect(form_document_step).to have_attributes(position: page.position, next_step_id: next_page.id)
1516
end
1617

1718
it "has all question attributes the original page has" do

spec/models/page_spec.rb

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -664,27 +664,33 @@
664664
let!(:second_page) { create :page, form: }
665665

666666
it "has an id" do
667-
expect(page.as_form_document_step).to match a_hash_including("id" => page.id)
667+
expect(page.as_form_document_step(second_page)).to match a_hash_including("id" => page.id)
668668
end
669669

670670
it "has a position" do
671-
expect(page.as_form_document_step).to match a_hash_including("position" => page.position)
671+
expect(page.as_form_document_step(second_page)).to match a_hash_including("position" => page.position)
672672
end
673673

674674
it "has a next_step_id" do
675-
expect(page.as_form_document_step).to match a_hash_including("next_step_id" => second_page.id)
675+
expect(page.as_form_document_step(second_page)).to match a_hash_including("next_step_id" => second_page.id)
676676
end
677677

678678
it "includes all attributes for the page as a step" do
679679
page_attributes = described_class.attribute_names - %w[id form_id next_page position created_at updated_at]
680-
expect(page.as_form_document_step["data"]).to match a_hash_including(*page_attributes)
680+
expect(page.as_form_document_step(second_page)["data"]).to match a_hash_including(*page_attributes)
681681
end
682682

683683
context "when there are conditions associated with the page" do
684684
let!(:condition) { create :condition, routing_page_id: page.id, check_page_id: page.id }
685685

686686
it "includes the routing conditions" do
687-
expect(page.reload.as_form_document_step["routing_conditions"]).to include(condition.as_form_document_condition)
687+
expect(page.reload.as_form_document_step(second_page)["routing_conditions"]).to include(condition.as_form_document_condition)
688+
end
689+
end
690+
691+
context "when the provided next_page is nil" do
692+
it "has nil next_step_id" do
693+
expect(page.as_form_document_step(nil)["next_step_id"]).to be_nil
688694
end
689695
end
690696
end

spec/services/step_summary_card_service_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
let(:form_document_content) { FormDocument::Content.from_form_document(form.live_form_document) }
99
let(:form_document_steps) { form_document_content.steps }
10-
let(:form_document_step) { FormDocument::Step.new(page.as_form_document_step) }
10+
let(:form_document_step) { FormDocument::Step.new(page.as_form_document_step(nil)) }
1111

1212
let(:form) { create :form, :live }
1313

0 commit comments

Comments
 (0)