From 5e201d35534a3f050bd733c14225f6b42fdcdabb Mon Sep 17 00:00:00 2001
From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com>
Date: Thu, 7 Oct 2021 09:16:00 +0100
Subject: [PATCH] CLDC-344: Validations (#36)

* Form with model so that we can validate (hypothetically)

* Put turbo frame back

* Turbo can update form errors if submitting returns 422

* Refactor and fix specs

* String interpolation over concatenation
---
 Gemfile.lock                                  | 10 ++---
 app/controllers/case_logs_controller.rb       | 27 ++++++------
 app/helpers/conditional_questions_helper.rb   |  6 +--
 .../conditional_question_controller.js        |  2 +-
 app/models/case_log.rb                        |  2 +
 app/models/form.rb                            | 10 +++++
 app/views/form/page.html.erb                  |  9 ++--
 config/routes.rb                              |  3 +-
 spec/features/case_log_spec.rb                | 42 +++++++++----------
 9 files changed, 58 insertions(+), 53 deletions(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index 1e4ad2c71..b21879881 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -26,7 +26,7 @@ GIT
 
 GIT
   remote: https://github.com/rspec/rspec-rails.git
-  revision: 211d7d990e9762e229d8a86249b88c2a7604e8b0
+  revision: fdcd1df0b13f9b6547336b4d37dffb66f70f7228
   branch: main
   specs:
     rspec-rails (5.1.0.pre)
@@ -143,7 +143,7 @@ GEM
     ffi (1.15.4)
     globalid (0.5.2)
       activesupport (>= 5.0)
-    govuk-components (2.1.1)
+    govuk-components (2.1.2)
       activemodel (>= 6.0)
       railties (>= 6.0)
       view_component (~> 2.39.0)
@@ -175,9 +175,9 @@ GEM
     minitest (5.14.4)
     msgpack (1.4.2)
     nio4r (2.5.8)
-    nokogiri (1.12.4-x86_64-darwin)
+    nokogiri (1.12.5-x86_64-darwin)
       racc (~> 1.4)
-    nokogiri (1.12.4-x86_64-linux)
+    nokogiri (1.12.5-x86_64-linux)
       racc (~> 1.4)
     overcommit (0.58.0)
       childprocess (>= 0.6.3, < 5)
@@ -294,7 +294,7 @@ GEM
       actionpack (>= 4.0)
       activesupport (>= 4.0)
       sprockets (>= 3.0.0)
-    stimulus-rails (0.6.0)
+    stimulus-rails (0.6.1)
       rails (>= 6.0.0)
     thor (1.1.0)
     turbo-rails (0.8.1)
diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb
index d84e6187e..45bc05ee4 100644
--- a/app/controllers/case_logs_controller.rb
+++ b/app/controllers/case_logs_controller.rb
@@ -20,22 +20,19 @@ class CaseLogsController < ApplicationController
     render :edit
   end
 
-  def next_page
+  def submit_form
     form = Form.new(2021, 2022)
-    @case_log = CaseLog.find(params[:case_log_id])
-    previous_page = params[:previous_page]
+    @case_log = CaseLog.find(params[:id])
+    previous_page = params[:case_log][:previous_page]
     questions_for_page = form.questions_for_page(previous_page).keys
     answers_for_page = page_params(questions_for_page).select { |k, _v| questions_for_page.include?(k) }
-    @case_log.update!(answers_for_page)
-    next_page = form.next_page(previous_page)
-    redirect_path = if next_page == :check_answers
-                      subsection = form.subsection_for_page(previous_page)
-                      "case_log_#{subsection}_check_answers_path"
-                    else
-                      "case_log_#{next_page}_path"
-                    end
-
-    redirect_to(send(redirect_path, @case_log))
+    if @case_log.update(answers_for_page)
+      redirect_path = form.next_page_redirect_path(previous_page)
+      redirect_to(send(redirect_path, @case_log))
+    else
+      page_info = form.all_pages[previous_page]
+      render "form/page", locals: { form: form, page_key: previous_page, page_info: page_info }, status: :unprocessable_entity
+    end
   end
 
   def check_answers
@@ -51,13 +48,13 @@ class CaseLogsController < ApplicationController
   form.all_pages.map do |page_key, page_info|
     define_method(page_key) do
       @case_log = CaseLog.find(params[:case_log_id])
-      render "form/page", locals: { case_log_id: @case_log.id, form: form, page_key: page_key, page_info: page_info }
+      render "form/page", locals: { form: form, page_key: page_key, page_info: page_info }
     end
   end
 
 private
 
   def page_params(questions_for_page)
-    params.permit(questions_for_page)
+    params.require(:case_log).permit(questions_for_page)
   end
 end
diff --git a/app/helpers/conditional_questions_helper.rb b/app/helpers/conditional_questions_helper.rb
index 8967207f4..c77119243 100644
--- a/app/helpers/conditional_questions_helper.rb
+++ b/app/helpers/conditional_questions_helper.rb
@@ -6,10 +6,6 @@ module ConditionalQuestionsHelper
   end
 
   def display_question_key_div(page_info, question_key)
-    if conditional_questions_for_page(page_info).include?(question_key)
-      "<div id=#{question_key}_div style='display:none;'>".html_safe
-    else
-      "<div id=#{question_key}_div>".html_safe
-    end
+    "style='display:none;'".html_safe if conditional_questions_for_page(page_info).include?(question_key)
   end
 end
diff --git a/app/javascript/controllers/conditional_question_controller.js b/app/javascript/controllers/conditional_question_controller.js
index 1b0073476..0d058da12 100644
--- a/app/javascript/controllers/conditional_question_controller.js
+++ b/app/javascript/controllers/conditional_question_controller.js
@@ -16,7 +16,7 @@ export default class extends Controller {
           div.style.display = "block"
         } else {
           div.style.display = "none"
-          let buttons = document.getElementsByName(key)
+          let buttons = document.getElementsByName(`case_log[${key}]`)
           Object.entries(buttons).forEach(([idx, button]) => {
             button.checked = false;
           })
diff --git a/app/models/case_log.rb b/app/models/case_log.rb
index 3f860cc08..c05ed887b 100644
--- a/app/models/case_log.rb
+++ b/app/models/case_log.rb
@@ -1,3 +1,5 @@
 class CaseLog < ApplicationRecord
   enum status: { "in progress" => 0, "submitted" => 1 }
+
+  # validates :tenant_age, presence: true
 end
diff --git a/app/models/form.rb b/app/models/form.rb
index 586df20f5..9812d890f 100644
--- a/app/models/form.rb
+++ b/app/models/form.rb
@@ -53,6 +53,16 @@ class Form
     pages_for_subsection(subsection).keys[previous_page_idx + 1] || :check_answers
   end
 
+  def next_page_redirect_path(previous_page)
+    next_page = next_page(previous_page)
+    if next_page == :check_answers
+      subsection = subsection_for_page(previous_page)
+      "case_log_#{subsection}_check_answers_path"
+    else
+      "case_log_#{next_page}_path"
+    end
+  end
+
   def previous_page(current_page)
     subsection = subsection_for_page(current_page)
     current_page_idx = pages_for_subsection(subsection).keys.index(current_page)
diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb
index 6e2ecac5c..2bca4d982 100644
--- a/app/views/form/page.html.erb
+++ b/app/views/form/page.html.erb
@@ -1,6 +1,6 @@
 <% previous_page = form.previous_page(page_key) %>
 <% content_for :before_content do %>
-  <%= govuk_back_link href: "/case_logs/#{case_log_id}/#{previous_page}" do %>
+  <%= govuk_back_link href: "/case_logs/#{@case_log.id}/#{previous_page}" do %>
     Back
   <% end %>
 <% end %>
@@ -11,16 +11,15 @@
       <%= page_info["header"] %>
     </h1>
   <% end %>
-  <%= form_with action: '/case_logs', method: "next_page", builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %>
-
+  <%= form_with model: @case_log, method: "submit_form", builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %>
+    <%= f.govuk_error_summary %>
     <% page_info["questions"].map do |question_key, question| %>
-      <%= display_question_key_div(page_info, question_key)%>
+      <div id=<%= question_key + "_div " %><%= display_question_key_div(page_info, question_key) %> >
         <%= render partial: "form/#{question["type"]}_question", locals: { question_key: question_key, question: question, f: f } %>
       </div>
     <% end %>
 
     <%= f.hidden_field :previous_page, value: page_key %>
-    <%= f.hidden_field :case_log_id, value: case_log_id %>
     <%= f.govuk_submit "Save and continue"  %>
   <% end %>
 <% end %>
diff --git a/config/routes.rb b/config/routes.rb
index 99d2c48ab..609124146 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -3,11 +3,12 @@ Rails.application.routes.draw do
   get "about", to: "about#index"
   get "/", to: "test#index"
 
+  post '/case_logs/:id', to: "case_logs#submit_form"
+
   form = Form.new(2021, 2022)
   resources :case_logs do
     form.all_pages.keys.map do |page|
       get page.to_s, to: "case_logs##{page}"
-      post page.to_s, to: "case_logs#next_page"
       form.all_subsections.keys.map do |subsection|
         get "#{subsection}/check_answers", to: "case_logs#check_answers"
       end
diff --git a/spec/features/case_log_spec.rb b/spec/features/case_log_spec.rb
index 5faab619f..46dde63c7 100644
--- a/spec/features/case_log_spec.rb
+++ b/spec/features/case_log_spec.rb
@@ -17,12 +17,12 @@ RSpec.describe "Test Features" do
 
   def answer_all_questions_in_income_subsection
     visit("/case_logs/#{empty_case_log.id}/net_income")
-    fill_in("net_income", with: 18_000)
-    choose("net-income-frequency-yearly-field")
+    fill_in("case-log-net-income-field", with: 18_000)
+    choose("case-log-net-income-frequency-yearly-field")
     click_button("Save and continue")
-    choose("net-income-uc-proportion-all-field")
+    choose("case-log-net-income-uc-proportion-all-field")
     click_button("Save and continue")
-    choose("housing-benefit-housing-benefit-but-not-universal-credit-field")
+    choose("case-log-housing-benefit-housing-benefit-but-not-universal-credit-field")
     click_button("Save and continue")
   end
 
@@ -80,19 +80,19 @@ RSpec.describe "Test Features" do
     it "displays the household questions when you click into that section" do
       visit("/case_logs/#{id}")
       click_link("Household characteristics")
-      expect(page).to have_field("tenant-code-field")
+      expect(page).to have_field("case-log-tenant-code-field")
       click_button("Save and continue")
-      expect(page).to have_field("tenant-age-field")
+      expect(page).to have_field("case-log-tenant-age-field")
       click_button("Save and continue")
-      expect(page).to have_field("tenant-gender-male-field")
+      expect(page).to have_field("case-log-tenant-gender-male-field")
       visit page.driver.request.env["HTTP_REFERER"]
-      expect(page).to have_field("tenant-age-field")
+      expect(page).to have_field("case-log-tenant-age-field")
     end
 
     describe "form questions" do
       it "can be accessed by url" do
         visit("/case_logs/#{id}/tenant_age")
-        expect(page).to have_field("tenant-age-field")
+        expect(page).to have_field("case-log-tenant-age-field")
       end
 
       it "updates model attributes correctly for each question" do
@@ -103,11 +103,11 @@ RSpec.describe "Test Features" do
           visit("/case_logs/#{id}/#{question}")
           case type
           when "text"
-            fill_in(question.to_s, with: answer)
+            fill_in("case-log-#{question.to_s.dasherize}-field", with: answer)
           when "radio"
-            choose("#{question.to_s.dasherize}-#{answer.parameterize}-field")
+            choose("case-log-#{question.to_s.dasherize}-#{answer.parameterize}-field")
           else
-            fill_in(question.to_s, with: answer)
+            fill_in("case-log-#{question.to_s.dasherize}-field", with: answer)
           end
           expect { click_button("Save and continue") }.to change {
             case_log.reload.send(question.to_s)
@@ -126,7 +126,7 @@ RSpec.describe "Test Features" do
       it "go back to tenant code page from tenant age page" do
         visit("/case_logs/#{id}/tenant_age")
         click_link(text: "Back")
-        expect(page).to have_field("tenant-code-field")
+        expect(page).to have_field("case-log-tenant-code-field")
       end
     end
   end
@@ -150,7 +150,7 @@ RSpec.describe "Test Features" do
     context "when the user needs to check their answers for a subsection" do
       def fill_in_number_question(case_log_id, question, value)
         visit("/case_logs/#{case_log_id}/#{question}")
-        fill_in(question.to_s, with: value)
+        fill_in("case-log-#{question.to_s.dasherize}-field", with: value)
         click_button("Save and continue")
       end
 
@@ -175,7 +175,7 @@ RSpec.describe "Test Features" do
 
       it "should display answers given by the user for the question in the subsection" do
         fill_in_number_question(empty_case_log.id, "tenant_age", 28)
-        choose("tenant-gender-non-binary-field")
+        choose("case-log-tenant-gender-non-binary-field")
         click_button("Save and continue")
         visit("/case_logs/#{empty_case_log.id}/#{subsection}/check_answers")
         expect(page).to have_content("28")
@@ -229,14 +229,14 @@ RSpec.describe "Test Features" do
       it "shows conditional questions if the required answer is selected and hides it again when a different answer option is selected", js: true do
         visit("/case_logs/#{id}/armed_forces")
         # Something about our styling makes the selenium webdriver think the actual radio buttons are not visible so we allow label click here
-        choose("armed-forces-yes-a-regular-field", allow_label_click: true)
+        choose("case-log-armed-forces-yes-a-regular-field", allow_label_click: true)
         expect(page).to have_selector("#armed_forces_injured_div")
-        choose("armed-forces-injured-no-field", allow_label_click: true)
-        expect(find_field("armed-forces-injured-no-field", visible: false).checked?).to be_truthy
-        choose("armed-forces-no-field", allow_label_click: true)
+        choose("case-log-armed-forces-injured-no-field", allow_label_click: true)
+        expect(find_field("case-log-armed-forces-injured-no-field", visible: false).checked?).to be_truthy
+        choose("case-log-armed-forces-no-field", allow_label_click: true)
         expect(page).not_to have_selector("#armed_forces_injured_div")
-        choose("armed-forces-yes-a-regular-field", allow_label_click: true)
-        expect(find_field("armed-forces-injured-no-field", visible: false).checked?).to be_falsey
+        choose("case-log-armed-forces-yes-a-regular-field", allow_label_click: true)
+        expect(find_field("case-log-armed-forces-injured-no-field", visible: false).checked?).to be_falsey
       end
     end
   end