Merge branch 'main' into CLCD-495/times-prev-offered

Gemfile

@@ -19,8 +19,12 @@ gem "jbuilder", "~> 2.7"
 gem "bootsnap", ">= 1.4.4", require: false
 # Gov.UK frontend components
 gem "govuk-components"
+# Gov.UK component form builder DSL
 gem "govuk_design_system_formbuilder"
+# Turbo & Stimulus
 gem "hotwire-rails"
+# Soft delete ActiveRecords objects
+gem "discard"
 
 group :development, :test do
   # Call 'byebug' anywhere in the code to stop execution and get a debugger console

Gemfile.lock

@@ -123,12 +123,14 @@ GEM
       rack-test (>= 0.6.3)
       regexp_parser (>= 1.5, < 3.0)
       xpath (~> 3.2)
-    childprocess (3.0.0)
+    childprocess (4.1.0)
     coderay (1.1.3)
     concurrent-ruby (1.1.9)
     crass (1.0.6)
     deep_merge (1.2.1)
     diff-lcs (1.4.4)
+    discard (1.2.0)
+      activerecord (>= 4.2, < 7)
     docile (1.4.0)
     dotenv (2.7.6)
     dotenv-rails (2.7.6)
@@ -143,7 +145,7 @@ GEM
     ffi (1.15.4)
     globalid (0.5.2)
       activesupport (>= 5.0)
-    govuk-components (2.1.2)
+    govuk-components (2.1.3)
       activemodel (>= 6.0)
       railties (>= 6.0)
       view_component (~> 2.39.0)
@@ -171,7 +173,7 @@ GEM
       mini_mime (>= 0.1.1)
     marcel (1.0.2)
     method_source (1.0.0)
-    mini_mime (1.1.1)
+    mini_mime (1.1.2)
     minitest (5.14.4)
     msgpack (1.4.2)
     nio4r (2.5.8)
@@ -194,7 +196,7 @@ GEM
       byebug (~> 11.0)
       pry (~> 0.13.0)
     public_suffix (4.0.6)
-    puma (5.5.0)
+    puma (5.5.2)
       nio4r (~> 2.0)
     racc (1.5.2)
     rack (2.2.3)
@@ -277,8 +279,9 @@ GEM
       sass (~> 3.5, >= 3.5.5)
     scss_lint-govuk (0.2.0)
       scss_lint
-    selenium-webdriver (3.142.7)
+    selenium-webdriver (4.0.0)
-      childprocess (>= 0.5, < 4.0)
+      childprocess (>= 0.5, < 5.0)
+      rexml (~> 3.2, >= 3.2.5)
       rubyzip (>= 1.2.2)
     semantic_range (3.0.0)
     simplecov (0.21.2)
@@ -294,10 +297,10 @@ GEM
       actionpack (>= 4.0)
       activesupport (>= 4.0)
       sprockets (>= 3.0.0)
-    stimulus-rails (0.7.0)
+    stimulus-rails (0.7.1)
       rails (>= 6.0.0)
     thor (1.1.0)
-    turbo-rails (0.8.1)
+    turbo-rails (7.1.0)
       rails (>= 6.0.0)
     tzinfo (2.0.4)
       concurrent-ruby (~> 1.0)
@@ -331,6 +334,7 @@ DEPENDENCIES
   bootsnap (>= 1.4.4)
   byebug
   capybara
+  discard
   dotenv-rails
   factory_bot_rails
   govuk-components

app/controllers/case_logs_controller.rb

@@ -1,17 +1,17 @@
 class CaseLogsController < ApplicationController
-  skip_before_action :verify_authenticity_token, if: :json_create_request?
+  skip_before_action :verify_authenticity_token, if: :json_api_request?
-  before_action :authenticate, if: :json_create_request?
+  before_action :authenticate, if: :json_api_request?
   # rubocop:disable Style/ClassVars
   @@form_handler = FormHandler.instance
   # rubocop:enable Style/ClassVars
 
   def index
-    @submitted_case_logs = CaseLog.where(status: 1)
+    @completed_case_logs = CaseLog.where(status: 2)
-    @in_progress_case_logs = CaseLog.where(status: 0)
+    @in_progress_case_logs = CaseLog.where(status: 1)
   end
 
   def create
-    case_log = CaseLog.create(create_params)
+    case_log = CaseLog.create(api_case_log_params)
     respond_to do |format|
       format.html { redirect_to case_log }
       format.json do
@@ -24,6 +24,18 @@ class CaseLogsController < ApplicationController
     end
   end
 
+  def update
+    if case_log = CaseLog.find_by(id: params[:id])
+      if case_log.update(api_case_log_params)
+        render json: case_log, status: :ok
+      else
+        render json: { errors: case_log.errors.full_messages }, status: :unprocessable_entity
+      end
+    else
+      render json: { error: "Case Log #{params[:id]} not found" }, status: :not_found
+    end
+  end
+
   # We don't have a dedicated non-editable show view
   def show
     edit
@@ -51,12 +63,24 @@ class CaseLogsController < ApplicationController
     end
   end
 
+  def destroy
+    if case_log = CaseLog.find_by(id: params[:id])
+      if case_log.discard
+        head :no_content
+      else
+        render json: { errors: case_log.errors.full_messages }, status: :unprocessable_entity
+      end
+    else
+      render json: { error: "Case Log #{params[:id]} not found" }, status: :not_found
+    end
+  end
+
   def check_answers
     form = @@form_handler.get_form("2021_2022")
     @case_log = CaseLog.find(params[:case_log_id])
     current_url = request.env["PATH_INFO"]
     subsection = current_url.split("/")[-2]
-    render "form/check_answers", locals: { case_log: @case_log, subsection: subsection, form: form }
+    render "form/check_answers", locals: { subsection: subsection, form: form }
   end
 
   form = @@form_handler.get_form("2021_2022")
@@ -69,6 +93,8 @@ class CaseLogsController < ApplicationController
 
 private
 
+  API_ACTIONS = %w[create update destroy].freeze
+
   def question_responses(questions_for_page)
     questions_for_page.each_with_object({}) do |(question_key, question_info), result|
       question_params = params["case_log"][question_key]
@@ -83,15 +109,15 @@ private
     end
   end
 
-  def json_create_request?
+  def json_api_request?
-    (request["action"] == "create") && request.format.json?
+    API_ACTIONS.include?(request["action"]) && request.format.json?
   end
 
   def authenticate
     http_basic_authenticate_or_request_with name: ENV["API_USER"], password: ENV["API_KEY"]
   end
 
-  def create_params
+  def api_case_log_params
     return {} unless params[:case_log]
 
     params.require(:case_log).permit(CaseLog.editable_fields)

app/helpers/tasklist_helper.rb

@@ -15,7 +15,7 @@ module TasklistHelper
 
   def get_subsection_status(subsection_name, case_log, questions)
     if subsection_name == "declaration"
-      return all_questions_completed(case_log) ? :not_started : :cannot_start_yet
+      return case_log.completed? ? :not_started : :cannot_start_yet
     end
 
     return :not_started if questions.all? { |question| case_log[question].blank? }
@@ -47,10 +47,6 @@ module TasklistHelper
 
 private
 
-  def all_questions_completed(case_log)
-    case_log.attributes.all? { |_question, answer| answer.present? }
-  end
-
   def is_incomplete?(subsection, case_log, questions)
     status = get_subsection_status(subsection, case_log, questions)
     %i[not_started in_progress].include?(status)

app/models/case_log.rb

@@ -32,29 +32,59 @@ class CaseLogValidator < ActiveModel::Validator
 end
 
 class CaseLog < ApplicationRecord
+  include Discard::Model
+  default_scope -> { kept }
+
   validate :instance_validations
   before_save :update_status!
 
   attr_writer :previous_page
 
-  enum status: { "in progress" => 0, "submitted" => 1 }
+  enum status: { "not_started" => 0, "in_progress" => 1, "completed" => 2 }
 
-  AUTOGENERATED_FIELDS = %w[status created_at updated_at id].freeze
+  AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze
 
   def instance_validations
     validates_with CaseLogValidator, ({ previous_page: @previous_page } || {})
   end
 
+  def self.editable_fields
+    attribute_names - AUTOGENERATED_FIELDS
+  end
+
+  def completed?
+    status == "completed"
+  end
+
+  def not_started?
+    status == "not_started"
+  end
+
+  def in_progress?
+    status == "in_progress"
+  end
+
+  private
+
   def update_status!
-    self.status = (all_fields_completed? && errors.empty? ? "submitted" : "in progress")
+    self.status = if all_fields_completed? && errors.empty?
+      "completed"
+    elsif all_fields_nil?
+      "not_started"
+    else
+      "in_progress"
+    end
   end
 
   def all_fields_completed?
-    mandatory_fields = attributes.except(*AUTOGENERATED_FIELDS)
     mandatory_fields.none? { |_key, val| val.nil? }
   end
 
-  def self.editable_fields
+  def all_fields_nil?
-    attribute_names - AUTOGENERATED_FIELDS
+    mandatory_fields.all? { |_key, val| val.nil? }
+  end
+
+  def mandatory_fields
+    attributes.except(*AUTOGENERATED_FIELDS)
   end
 end

app/views/case_logs/index.html.erb

@@ -10,10 +10,10 @@
       <%= render partial: "log_list", locals: { case_logs: @in_progress_case_logs, title: "Logs you need to complete", date_title: "Last Changed" } %>
     <% end %>
 
-    <% if @submitted_case_logs.present? %>
+    <% if @completed_case_logs.present? %>
-      <%= render partial: "log_list", locals: { case_logs: @submitted_case_logs, title: "Logs you've submitted", date_title: "Date Submitted" } %>
+      <%= render partial: "log_list", locals: { case_logs: @completed_case_logs, title: "Logs you've submitted", date_title: "Date Submitted" } %>
     <% end %>
 
-    <p><a href="#" class="govuk-link">See all completed logs (<%= @submitted_case_logs.count %>)</a></p>
+    <p><a href="#" class="govuk-link">See all completed logs (<%= @completed_case_logs.count %>)</a></p>
   </div>
 </div>

app/views/form/_check_answers_table.html.erb

@@ -4,10 +4,10 @@
       <%= question_info["check_answer_label"].to_s %>
     <dt>
     <dd class="govuk-summary-list__value">
-      <%= case_log[question_title] %>
+      <%= @case_log[question_title] %>
     </dd>
     <dd class="govuk-summary-list__actions">
-      <%= create_update_answer_link(case_log[question_title], case_log["id"], page)%>
+      <%= create_update_answer_link(@case_log[question_title], @case_log.id, page)%>
     </dd>
   </div>
 </dl>

app/views/form/check_answers.html.erb

@@ -2,11 +2,11 @@
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-two-thirds-from-desktop">
       <h1 class="govuk-heading-l">Check the answers you gave for <%= subsection.humanize(capitalize: false) %></h1>
-      <%= display_answered_questions_summary(subsection, case_log, form) %>
+      <%= display_answered_questions_summary(subsection, @case_log, form) %>
       <% form.pages_for_subsection(subsection).each do |page, page_info| %>
         <% page_info["questions"].each do |question_title, question_info| %>
-          <% if total_questions(subsection, case_log, form).include?(question_title) %>
+          <% if total_questions(subsection, @case_log, form).include?(question_title) %>
-            <%= render partial: 'form/check_answers_table', locals: { question_title: question_title, question_info: question_info, case_log: case_log, page: page } %>
+            <%= render partial: 'form/check_answers_table', locals: { question_title: question_title, question_info: question_info, case_log: @case_log, page: page } %>
           <%end %>
         <%end %>
       <% end %>

db/migrate/20211014154616_add_discarded_at_to_case_logs.rb

@@ -0,0 +1,6 @@
+class AddDiscardedAtToCaseLogs < ActiveRecord::Migration[6.1]
+  def change
+    add_column :case_logs, :discarded_at, :datetime
+    add_index :case_logs, :discarded_at
+  end
+end

db/schema.rb

@@ -131,6 +131,8 @@ ActiveRecord::Schema.define(version: 2021_10_15_090040) do
     t.boolean "reasonable_preference_reason_avoid_hardship"
     t.boolean "reasonable_preference_reason_do_not_know"
     t.integer "property_number_of_times_relet"
+    t.datetime "discarded_at"
+    t.index ["discarded_at"], name: "index_case_logs_on_discarded_at"
   end
 
 end

spec/factories/case_log.rb

@@ -2,14 +2,14 @@ FactoryBot.define do
   factory :case_log do
     sequence(:id) { |i| i }
     trait :in_progress do
-      status { 0 }
+      status { 1 }
       tenant_code { "TH356" }
       property_postcode { "SW2 6HI" }
       previous_postcode { "P0 5ST" }
       tenant_age { "12" }
     end
-    trait :submitted do
+    trait :completed do
-      status { 1 }
+      status { 2 }
       tenant_code { "BZ737" }
       property_postcode { "NW1 7TY" }
     end

spec/helpers/tasklist_helper_spec.rb

@@ -1,8 +1,9 @@
 require "rails_helper"
 
 RSpec.describe TasklistHelper do
-  let!(:empty_case_log) { FactoryBot.create(:case_log) }
+  let(:empty_case_log) { FactoryBot.build(:case_log) }
-  let!(:case_log) { FactoryBot.create(:case_log, :in_progress) }
+  let(:case_log) { FactoryBot.build(:case_log, :in_progress) }
+  let(:completed_case_log) { FactoryBot.build(:case_log, :completed) }
   form_handler = FormHandler.instance
   let(:form) { form_handler.get_form("test_form") }
 
@@ -35,7 +36,6 @@ RSpec.describe TasklistHelper do
     end
 
     it "returns not started if the subsection is declaration and all the questions are completed" do
-      completed_case_log = CaseLog.new(case_log.attributes.map { |key, value| Hash[key, value || "value"] }.reduce(:merge))
       status = get_subsection_status("declaration", completed_case_log, declaration_questions)
       expect(status).to eq(:not_started)
     end

spec/models/case_log_spec.rb

@@ -14,4 +14,21 @@ RSpec.describe Form, type: :model do
       expect { CaseLog.create!(tenant_age: 0) }.to raise_error(ActiveRecord::RecordInvalid)
     end
   end
+
+  describe "status" do
+    let!(:empty_case_log) { FactoryBot.create(:case_log) }
+    let!(:in_progress_case_log) { FactoryBot.create(:case_log, :in_progress) }
+
+    it "is set to not started for an empty case log" do
+      expect(empty_case_log.not_started?).to be(true)
+      expect(empty_case_log.in_progress?).to be(false)
+      expect(empty_case_log.completed?).to be(false)
+    end
+
+    it "is set to not started for an empty case log" do
+      expect(in_progress_case_log.in_progress?).to be(true)
+      expect(in_progress_case_log.not_started?).to be(false)
+      expect(in_progress_case_log.completed?).to be(false)
+    end
+  end
 end

spec/requests/case_log_controller_spec.rb

@@ -1,19 +1,12 @@
 require "rails_helper"
 
 RSpec.describe CaseLogsController, type: :request do
-  describe "POST #create" do
-    let(:tenant_code) { "T365" }
-    let(:tenant_age) { 35 }
-    let(:property_number_of_times_relet) { 12 }
-    let(:property_postcode) { "SE11 6TY" }
   let(:api_username) { "test_user" }
   let(:api_password) { "test_password" }
   let(:basic_credentials) do
     ActionController::HttpAuthentication::Basic
                         .encode_credentials(api_username, api_password)
   end
-    let(:in_progress) { "in progress" }
-    let(:submitted) { "submitted" }
 
   let(:headers) do
     {
@@ -23,6 +16,20 @@ RSpec.describe CaseLogsController, type: :request do
     }
   end
 
+  before do
+    allow(ENV).to receive(:[])
+    allow(ENV).to receive(:[]).with("API_USER").and_return(api_username)
+    allow(ENV).to receive(:[]).with("API_KEY").and_return(api_password)
+  end
+
+  describe "POST #create" do
+    let(:tenant_code) { "T365" }
+    let(:tenant_age) { 35 }
+    let(:property_number_of_times_relet) { 12 }
+    let(:property_postcode) { "SE11 6TY" }
+    let(:in_progress) { "in_progress" }
+    let(:completed) { "completed" }
+
     let(:params) do
       {
         "tenant_code": tenant_code,
@@ -33,9 +40,6 @@ RSpec.describe CaseLogsController, type: :request do
     end
 
     before do
-      allow(ENV).to receive(:[])
-      allow(ENV).to receive(:[]).with("API_USER").and_return(api_username)
-      allow(ENV).to receive(:[]).with("API_KEY").and_return(api_password)
       post "/case_logs", headers: headers, params: params.to_json
     end
 
@@ -78,9 +82,147 @@ RSpec.describe CaseLogsController, type: :request do
         JSON.parse(File.open("spec/fixtures/complete_case_log.json").read)
       end
 
-      it "marks the record as submitted" do
+      it "marks the record as completed" do
+        json_response = JSON.parse(response.body)
+        expect(json_response["status"]).to eq(completed)
+      end
+    end
+
+    context "request with invalid credentials" do
+      let(:basic_credentials) do
+        ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops")
+      end
+
+      it "returns 401" do
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+  end
+
+  describe "PATCH" do
+    let(:case_log) do
+      FactoryBot.create(:case_log, :in_progress, tenant_code: "Old Value", property_postcode: "Old Value")
+    end
+    let(:params) do
+      { tenant_code: "New Value" }
+    end
+    let(:id) { case_log.id }
+
+    before do
+      patch "/case_logs/#{id}", headers: headers, params: params.to_json
+    end
+
+    it "returns http success" do
+      expect(response).to have_http_status(:success)
+    end
+
+    it "updates the case log with the given fields and keeps original values where none are passed" do
+      case_log.reload
+      expect(case_log.tenant_code).to eq("New Value")
+      expect(case_log.property_postcode).to eq("Old Value")
+    end
+
+    context "invalid case log id" do
+      let(:id) { (CaseLog.order(:id).last&.id || 0) + 1 }
+
+      it "returns 404" do
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "invalid case log params" do
+      let(:params) { {tenant_age: 200} }
+
+      it "returns 422" do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+
+      it "returns an error message" do
         json_response = JSON.parse(response.body)
-        expect(json_response["status"]).to eq(submitted)
+        expect(json_response["errors"]).to eq(["Tenant age must be between 0 and 120"])
+      end
+    end
+
+    context "request with invalid credentials" do
+      let(:basic_credentials) do
+        ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops")
+      end
+
+      it "returns 401" do
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+  end
+
+  # We don't really have any meaningful distinction between PUT and PATCH here since you can update some or all
+  # fields in both cases, and both route to #Update. Rails generally recommends PATCH as it more closely matches
+  # what actually happens to an ActiveRecord object and what we're doing here, but either is allowed.
+  describe "PUT" do
+    let(:case_log) do
+      FactoryBot.create(:case_log, :in_progress, tenant_code: "Old Value", property_postcode: "Old Value")
+    end
+    let(:params) do
+      { tenant_code: "New Value" }
+    end
+    let(:id) { case_log.id }
+
+    before do
+      put "/case_logs/#{id}", headers: headers, params: params.to_json
+    end
+
+    it "returns http success" do
+      expect(response).to have_http_status(:success)
+    end
+
+    it "updates the case log with the given fields and keeps original values where none are passed" do
+      case_log.reload
+      expect(case_log.tenant_code).to eq("New Value")
+      expect(case_log.property_postcode).to eq("Old Value")
+    end
+
+    context "invalid case log id" do
+      let(:id) { (CaseLog.order(:id).last&.id || 0) + 1 }
+
+      it "returns 404" do
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "request with invalid credentials" do
+      let(:basic_credentials) do
+        ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops")
+      end
+
+      it "returns 401" do
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+  end
+
+  describe "DELETE" do
+    let!(:case_log) do
+      FactoryBot.create(:case_log, :in_progress)
+    end
+    let(:id) { case_log.id }
+
+    before do
+      delete "/case_logs/#{id}", headers: headers
+    end
+
+    it "returns http success" do
+      expect(response).to have_http_status(:success)
+    end
+
+    it "soft deletes the case log" do
+      expect { CaseLog.find(id) }.to raise_error(ActiveRecord::RecordNotFound)
+      expect(CaseLog.with_discarded.find(id)).to be_a(CaseLog)
+    end
+
+    context "invalid case log id" do
+      let(:id) { (CaseLog.order(:id).last&.id || 0) + 1 }
+
+      it "returns 404" do
+        expect(response).to have_http_status(:not_found)
       end
     end
 

spec/views/case_log_index_view_spec.rb

@@ -1,12 +1,12 @@
 require "rails_helper"
 RSpec.describe "case_logs/index" do
   let(:in_progress_log) { FactoryBot.build(:case_log, :in_progress) }
-  let(:submitted_log) { FactoryBot.build(:case_log, :submitted) }
+  let(:completed_log) { FactoryBot.build(:case_log, :completed) }
 
   context "given an in progress log list" do
     it "renders a table for in progress logs only" do
       assign(:in_progress_case_logs, [in_progress_log])
-      assign(:submitted_case_logs, [])
+      assign(:completed_case_logs, [])
       render
       expect(rendered).to match(/<table class="govuk-table">/)
       expect(rendered).to match(/Logs you need to complete/)
@@ -16,23 +16,23 @@ RSpec.describe "case_logs/index" do
     end
   end
 
-  context "given a submitted log list" do
+  context "given a completed log list" do
     it "renders a table for in progress logs only" do
       assign(:in_progress_case_logs, [])
-      assign(:submitted_case_logs, [submitted_log])
+      assign(:completed_case_logs, [completed_log])
       render
       expect(rendered).to match(/<table class="govuk-table">/)
       expect(rendered).to match(/Logs you&#39;ve submitted/)
       expect(rendered).not_to match(/Logs you need to complete/)
-      expect(rendered).to match(submitted_log.tenant_code)
+      expect(rendered).to match(completed_log.tenant_code)
-      expect(rendered).to match(submitted_log.property_postcode)
+      expect(rendered).to match(completed_log.property_postcode)
     end
   end
 
-  context "given a submitted log list and an in_progress log list" do
+  context "given a completed log list and an in_progress log list" do
     it "renders two tables, one for each status" do
       assign(:in_progress_case_logs, [in_progress_log])
-      assign(:submitted_case_logs, [submitted_log])
+      assign(:completed_case_logs, [completed_log])
       render
       expect(rendered).to match(/<table class="govuk-table">/)
       expect(rendered).to match(/Logs you&#39;ve submitted/)