Merge pull request #2023 from govuk-forms/isolate-form-document-steps

Make form_document_step internal to the Step model

Author: stephencdaly

app/controllers/forms/step_controller.rb

@@ -4,8 +4,8 @@ class StepController < BaseController
 
     def set_request_logging_attributes
       super
-      CurrentRequestLoggingAttributes.question_number = @step.step_number if @step&.step_number
-      CurrentRequestLoggingAttributes.answer_type = @step&.form_document_step&.answer_type if @step&.form_document_step&.answer_type
+      CurrentRequestLoggingAttributes.question_number = @step&.step_number
+      CurrentRequestLoggingAttributes.answer_type = @step&.answer_type
     end
 
     def show

app/models/step.rb

@@ -1,5 +1,6 @@
 class Step
-  attr_accessor :form_document_step, :question
+  attr_accessor :question
+  private attr_reader :form_document_step
 
   GOTO_PAGE_ERROR_NAMES = %w[cannot_have_goto_page_before_routing_page goto_page_doesnt_exist].freeze
 
@@ -8,6 +9,8 @@ def initialize(form_document_step:, question:)
     @question = question
   end
 
+  delegate :answer_type, to: :form_document_step
+
   def id
     form_document_step&.id.to_s
   end

spec/lib/json_submission_generator_spec.rb

@@ -63,24 +63,24 @@
           "submitted_at" => "2022-09-14T07:00:00.000Z",
           "answers" => [
             {
-              "question_id" => text_step.form_document_step.id,
+              "question_id" => text_step.id,
               "question_text" => "What is the meaning of life?",
               "answer_text" => text_question.text,
             },
             {
-              "question_id" => name_step.form_document_step.id,
+              "question_id" => name_step.id,
               "question_text" => "What is your name?",
               "first_name" => name_question.first_name,
               "last_name" => name_question.last_name,
               "answer_text" => name_question.show_answer,
             },
             {
-              "question_id" => file_step.form_document_step.id,
+              "question_id" => file_step.id,
               "question_text" => "Upload a file",
               "answer_text" => "test_#{submission_reference}.txt",
             },
             {
-              "question_id" => address_step.form_document_step.id,
+              "question_id" => address_step.id,
               "question_text" => "What is your address?",
               "address1" => address_question.address1,
               "address2" => "",
@@ -90,7 +90,7 @@
               "answer_text" => address_question.show_answer,
             },
             {
-              "question_id" => selection_step.form_document_step.id,
+              "question_id" => selection_step.id,
               "question_text" => "Select your options",
               "selections" => ["Option 1", "Option 2"],
               "answer_text" => "Option 1, Option 2",
@@ -115,13 +115,13 @@
             "submitted_at" => "2022-09-14T07:00:00.000Z",
             "answers" => [
               {
-                "question_id" => repeatable_step.form_document_step.id,
+                "question_id" => repeatable_step.id,
                 "question_text" => "What is the meaning of life?",
                 "can_have_multiple_answers" => true,
                 "answer_text" => [first_answer.text, second_answer.text],
               },
               {
-                "question_id" => name_step.form_document_step.id,
+                "question_id" => name_step.id,
                 "question_text" => "What is your name?",
                 "first_name" => name_question.first_name,
                 "last_name" => name_question.last_name,
@@ -148,7 +148,7 @@
 
       it "generates JSON without including the submission reference in the filename for the file upload question" do
         expect(parsed_json["answers"]).to include({
-          "question_id" => file_step.form_document_step.id,
+          "question_id" => file_step.id,
           "question_text" => "Upload a file",
           "answer_text" => "test.txt",
         })

spec/lib/ses_email_formatter_spec.rb

@@ -91,14 +91,13 @@
 
     context "when there is an error formatting an answer" do
       before do
-        text_step.form_document_step.id = 99
         allow(text_step).to receive(:show_answer_in_email).and_raise(NoMethodError, "undefined method 'strip' for an instance of Array")
       end
 
       it "raises an error with the page id" do
         expect {
           ses_email_formatter.build_question_answers_section_html
-        }.to raise_error(SesEmailFormatter::FormattingError, "could not format answer for question step 99")
+        }.to raise_error(SesEmailFormatter::FormattingError, "could not format answer for question step #{text_step.id}")
       end
     end
   end
@@ -170,14 +169,13 @@
 
     context "when there is an error formatting an answer" do
       before do
-        text_step.form_document_step.id = 99
         allow(text_step).to receive(:show_answer_in_email).and_raise(NoMethodError, "undefined method 'strip' for an instance of Array")
       end
 
       it "raises an error with the page id" do
         expect {
           ses_email_formatter.build_question_answers_section_plain_text
-        }.to raise_error(SesEmailFormatter::FormattingError, "could not format answer for question step 99")
+        }.to raise_error(SesEmailFormatter::FormattingError, "could not format answer for question step #{text_step.id}")
       end
     end
   end

spec/models/step_spec.rb

@@ -49,16 +49,16 @@
   describe "#state" do
     it "returns an array of instance variable values" do
       expected_state = [
-        step.form_document_step,
-        step.question,
+        form_document_step,
+        question,
       ]
 
       expect(step.state).to match_array(expected_state)
     end
 
     it "changes when an instance variable is modified" do
       original_state = step.state.dup
-      step.form_document_step = build(:v2_question_page_step, position: 2)
+      step.question = build(:name)
       expect(step.state).not_to eq(original_state)
     end
   end