diff --git a/Gemfile b/Gemfile index f28f6dee0..3039202f0 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index 328d2a9ef..176bb4202 100644 --- a/Gemfile.lock +++ b/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) - childprocess (>= 0.5, < 4.0) + selenium-webdriver (4.0.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 diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index ef73bdd7c..20cc5c64f 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -1,17 +1,17 @@ class CaseLogsController < ApplicationController - skip_before_action :verify_authenticity_token, if: :json_create_request? - before_action :authenticate, if: :json_create_request? + skip_before_action :verify_authenticity_token, if: :json_api_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) - @in_progress_case_logs = CaseLog.where(status: 0) + @completed_case_logs = CaseLog.where(status: 2) + @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? - (request["action"] == "create") && request.format.json? + def json_api_request? + 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) diff --git a/app/helpers/tasklist_helper.rb b/app/helpers/tasklist_helper.rb index 549ef2d22..3da3ddc52 100644 --- a/app/helpers/tasklist_helper.rb +++ b/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) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 876e42766..336876981 100644 --- a/app/models/case_log.rb +++ b/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 } - AUTOGENERATED_FIELDS = %w[status created_at updated_at id].freeze + enum status: { "not_started" => 0, "in_progress" => 1, "completed" => 2 } + + 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 - attribute_names - AUTOGENERATED_FIELDS + def all_fields_nil? + mandatory_fields.all? { |_key, val| val.nil? } + end + + def mandatory_fields + attributes.except(*AUTOGENERATED_FIELDS) end end diff --git a/app/views/case_logs/index.html.erb b/app/views/case_logs/index.html.erb index 35ae34399..7352c304c 100644 --- a/app/views/case_logs/index.html.erb +++ b/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? %> - <%= render partial: "log_list", locals: { case_logs: @submitted_case_logs, title: "Logs you've submitted", date_title: "Date Submitted" } %> + <% if @completed_case_logs.present? %> + <%= render partial: "log_list", locals: { case_logs: @completed_case_logs, title: "Logs you've submitted", date_title: "Date Submitted" } %> <% end %> -

See all completed logs (<%= @submitted_case_logs.count %>)

+

See all completed logs (<%= @completed_case_logs.count %>)

diff --git a/app/views/form/_check_answers_table.html.erb b/app/views/form/_check_answers_table.html.erb index 88567bc9c..3c5fb2d2c 100644 --- a/app/views/form/_check_answers_table.html.erb +++ b/app/views/form/_check_answers_table.html.erb @@ -4,10 +4,10 @@ <%= question_info["check_answer_label"].to_s %>
- <%= case_log[question_title] %> + <%= @case_log[question_title] %>
- <%= create_update_answer_link(case_log[question_title], case_log["id"], page)%> + <%= create_update_answer_link(@case_log[question_title], @case_log.id, page)%>
diff --git a/app/views/form/check_answers.html.erb b/app/views/form/check_answers.html.erb index 1ab567e44..f5b0fe93e 100644 --- a/app/views/form/check_answers.html.erb +++ b/app/views/form/check_answers.html.erb @@ -2,11 +2,11 @@

Check the answers you gave for <%= subsection.humanize(capitalize: false) %>

- <%= 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) %> - <%= render partial: 'form/check_answers_table', locals: { question_title: question_title, question_info: question_info, case_log: case_log, page: page } %> + <% 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 } %> <%end %> <%end %> <% end %> diff --git a/db/migrate/20211014154616_add_discarded_at_to_case_logs.rb b/db/migrate/20211014154616_add_discarded_at_to_case_logs.rb new file mode 100644 index 000000000..429640759 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 7202cd339..42b14adc9 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/factories/case_log.rb b/spec/factories/case_log.rb index a9aa4ab56..044a4cbac 100644 --- a/spec/factories/case_log.rb +++ b/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 - status { 1 } + trait :completed do + status { 2 } tenant_code { "BZ737" } property_postcode { "NW1 7TY" } end diff --git a/spec/helpers/tasklist_helper_spec.rb b/spec/helpers/tasklist_helper_spec.rb index 28860962f..e2ced980f 100644 --- a/spec/helpers/tasklist_helper_spec.rb +++ b/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!(:case_log) { FactoryBot.create(:case_log, :in_progress) } + let(:empty_case_log) { FactoryBot.build(:case_log) } + 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 diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 86f4e9d9b..b456e6988 100644 --- a/spec/models/case_log_spec.rb +++ b/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 diff --git a/spec/requests/case_log_controller_spec.rb b/spec/requests/case_log_controller_spec.rb index b4429c4dc..6b97103de 100644 --- a/spec/requests/case_log_controller_spec.rb +++ b/spec/requests/case_log_controller_spec.rb @@ -1,27 +1,34 @@ require "rails_helper" RSpec.describe CaseLogsController, type: :request do + 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(:headers) do + { + "Content-Type" => "application/json", + "Accept" => "application/json", + "Authorization" => basic_credentials, + } + 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(: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 - { - "Content-Type" => "application/json", - "Accept" => "application/json", - "Authorization" => basic_credentials, - } - end + let(:in_progress) { "in_progress" } + let(:completed) { "completed" } let(:params) do { @@ -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(submitted) + 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["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 diff --git a/spec/views/case_log_index_view_spec.rb b/spec/views/case_log_index_view_spec.rb index b97347cb6..cc826e3c8 100644 --- a/spec/views/case_log_index_view_spec.rb +++ b/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(//) 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(/
/) expect(rendered).to match(/Logs you've submitted/) expect(rendered).not_to match(/Logs you need to complete/) - expect(rendered).to match(submitted_log.tenant_code) - expect(rendered).to match(submitted_log.property_postcode) + expect(rendered).to match(completed_log.tenant_code) + 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(/
/) expect(rendered).to match(/Logs you've submitted/)