From 488f79a5d32265c5117798818f8ee1a8cb2df887 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Mon, 16 May 2022 15:48:10 +0100 Subject: [PATCH 1/4] CLDC-1101: Case log filter by organisation (#522) * Rename org filter * Ensure filters work when ID is passed * UI init * Lint * Fix filter nesting * Reduce width * Set checked status of filters correctly * Test filter presence * Add filter test * Rubocop * Tweak styles for autocomplete within filter Co-authored-by: Paul Robert Lloyd --- app/controllers/case_logs_controller.rb | 5 +-- app/frontend/styles/_filter-layout.scss | 6 +-- app/frontend/styles/_filter.scss | 15 +++++++ app/frontend/styles/application.scss | 8 ++++ app/helpers/filters_helper.rb | 8 ++++ app/models/case_log.rb | 2 +- app/models/organisation.rb | 2 +- app/models/user.rb | 10 ++++- app/views/case_logs/_log_filters.erb | 23 ++++++++++- app/views/filters/_radio_filter.html.erb | 20 ++++++--- app/views/filters/_select_filter.html.erb | 7 ++++ spec/helpers/filters_helper_spec.rb | 30 ++++++++++++-- spec/models/case_log_spec.rb | 25 ++++++++++++ spec/models/user_spec.rb | 8 ++++ spec/requests/case_logs_controller_spec.rb | 47 +++++++++++++++++++++- 15 files changed, 195 insertions(+), 21 deletions(-) create mode 100644 app/views/filters/_select_filter.html.erb diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index fa33ad5df..24ecb1e06 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -126,7 +126,7 @@ private if session[:case_logs_filters].present? filters = JSON.parse(session[:case_logs_filters]) filters.each do |category, values| - next if values.reject(&:empty?).blank? + next if Array(values).reject(&:empty?).blank? query = query.public_send("filter_by_#{category}", values, current_user) end @@ -137,8 +137,7 @@ private def set_session_filters new_filters = session[:case_logs_filters].present? ? JSON.parse(session[:case_logs_filters]) : {} - %i[status years].each { |filter| new_filters[filter] = params[filter] if params[filter].present? } - new_filters[:user] = [params[:user]] if params[:user].present? + current_user.case_logs_filters.each { |filter| new_filters[filter] = params[filter] if params[filter].present? } session[:case_logs_filters] = new_filters.to_json end diff --git a/app/frontend/styles/_filter-layout.scss b/app/frontend/styles/_filter-layout.scss index 0dbd1f028..b9fb838ef 100644 --- a/app/frontend/styles/_filter-layout.scss +++ b/app/frontend/styles/_filter-layout.scss @@ -3,7 +3,7 @@ } .app-filter-layout__filter { - @include govuk-media-query(desktop) { + @include govuk-media-query(wide) { float: left; min-width: govuk-grid-width("one-quarter"); } @@ -12,7 +12,7 @@ .js-enabled .app-filter-layout__filter { outline: 0 none; - @include govuk-media-query($until: desktop) { + @include govuk-media-query($until: wide) { background-color: govuk-colour("light-grey"); bottom: govuk-spacing(1); border: 1px solid govuk-colour("mid-grey"); @@ -32,7 +32,7 @@ } .app-filter-layout__content { - @include govuk-media-query(desktop) { + @include govuk-media-query(wide) { float: right; max-width: calc(#{govuk-grid-width("three-quarters")} - #{govuk-spacing(6)}); width: 100%; diff --git a/app/frontend/styles/_filter.scss b/app/frontend/styles/_filter.scss index 2b4f17953..42b7e2f6a 100644 --- a/app/frontend/styles/_filter.scss +++ b/app/frontend/styles/_filter.scss @@ -98,4 +98,19 @@ background-color: govuk-colour("white"); } } + + .autocomplete__input { + @include govuk-font(16); + background-color: govuk-colour("white"); + } + + .autocomplete__wrapper { + @include govuk-media-query(wide) { + max-width: 20ex; + } + } + + .autocomplete__option { + @include govuk-font(16); + } } diff --git a/app/frontend/styles/application.scss b/app/frontend/styles/application.scss index abe376e99..b9de7492c 100644 --- a/app/frontend/styles/application.scss +++ b/app/frontend/styles/application.scss @@ -11,6 +11,14 @@ $govuk-image-url-function: frontend-image-url; $govuk-global-styles: true; $govuk-new-link-styles: true; +// Add additional breakpoint named `wide` +$govuk-breakpoints: ( + mobile: 320px, + tablet: 641px, + desktop: 769px, + wide: 921px, +); + @import "govuk-frontend-styles"; @import "govuk-prototype-styles"; diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index c142b6e6d..d39467b76 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -4,6 +4,8 @@ module FiltersHelper selected_filters = JSON.parse(session[:case_logs_filters]) return true if selected_filters.blank? && filter == "user" && value == :all + return true if selected_filters.blank? && filter == "organisation_select" && value == :all + return true if selected_filters["organisation"].present? && filter == "organisation_select" && value == :specific_org return false if selected_filters[filter].blank? selected_filters[filter].include?(value.to_s) @@ -14,4 +16,10 @@ module FiltersHelper CaseLog.statuses.keys.map { |status| statuses[status] = status.humanize } statuses end + + def selected_option(filter) + return false unless session[:case_logs_filters] + + JSON.parse(session[:case_logs_filters])[filter] + end end diff --git a/app/models/case_log.rb b/app/models/case_log.rb index ff2a962c4..df91376cf 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -34,7 +34,7 @@ class CaseLog < ApplicationRecord belongs_to :managing_organisation, class_name: "Organisation" belongs_to :created_by, class_name: "User" - scope :for_organisation, ->(org) { where(owning_organisation: org).or(where(managing_organisation: org)) } + scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org).or(where(managing_organisation: org)) } scope :filter_by_status, ->(status, _user = nil) { where status: } scope :filter_by_years, lambda { |years, _user = nil| first_year = years.shift diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 8096c6be1..c52c3d922 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -18,7 +18,7 @@ class Organisation < ApplicationRecord validates :provider_type, presence: true def case_logs - CaseLog.for_organisation(self) + CaseLog.filter_by_organisation(self) end def completed_case_logs diff --git a/app/models/user.rb b/app/models/user.rb index 27a884fa4..09fc93c3c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -36,7 +36,7 @@ class User < ApplicationRecord if support? CaseLog.all else - CaseLog.for_organisation(organisation) + CaseLog.filter_by_organisation(organisation) end end @@ -88,4 +88,12 @@ class User < ApplicationRecord ROLES.except(:support) end + + def case_logs_filters + if support? + %i[status years user organisation] + else + %i[status years user] + end + end end diff --git a/app/views/case_logs/_log_filters.erb b/app/views/case_logs/_log_filters.erb index 4e294c81e..45354fb40 100644 --- a/app/views/case_logs/_log_filters.erb +++ b/app/views/case_logs/_log_filters.erb @@ -6,12 +6,31 @@
<%= form_with url: "/logs", html: { method: :get } do |f| %> <% years = {"2021": "2021/22", "2022": "2022/23"} %> - <% all_or_yours = {"all": "All", "yours": "Yours"} %> + <% all_or_yours = {"all": { label: "All" }, "yours": { label: "Yours" } } %> <%= render partial: "filters/checkbox_filter", locals: { f: f, options: years, label: "Collection year", category: "years" } %> <%= render partial: "filters/checkbox_filter", locals: { f: f, options: status_filters, label: "Status", category: "status" } %> <%= render partial: "filters/radio_filter", locals: { f: f, options: all_or_yours, label: "Logs", category: "user", } %> + <% if @current_user.support? %> + <%= render partial: "filters/radio_filter", locals: { + f: f, + options: { + "all": { label: "All" }, + "specific_org": { + label: "Specific organisation", + conditional_filter: { + type: "select", + label: "Organisation", + category: "organisation", + options: [OpenStruct.new(id: "", name: "Select an option")] + Organisation.all.map { |org| OpenStruct.new(id: org.id, name: org.name) } + } + } + }, + label: "Organisation", + category: "organisation_select" + } %> + <% end %> <%= f.govuk_submit "Apply filters", class: "govuk-!-margin-bottom-0" %> <% end %>
- \ No newline at end of file + diff --git a/app/views/filters/_radio_filter.html.erb b/app/views/filters/_radio_filter.html.erb index aff0f6ed6..abb55eafe 100644 --- a/app/views/filters/_radio_filter.html.erb +++ b/app/views/filters/_radio_filter.html.erb @@ -1,8 +1,18 @@ <%= f.govuk_radio_buttons_fieldset category.to_sym, legend: { text: label, size: "s" }, small: true, form_group: { classes: "app-filter__group" } do %> - <% options.map do |key, option| %> - <%= f.govuk_radio_button category, key.to_s, - label: { text: option }, - checked: filter_selected?(category, key), - size: "s" %> + <% options.map do |key, option| %> + <%= f.govuk_radio_button category, key.to_s, + label: { text: option[:label] }, + checked: filter_selected?(category, key), + size: "s" do %> + <% if option[:conditional_filter] %> + <%= render partial: "filters/#{option[:conditional_filter][:type]}_filter", locals: { + f:, + collection: option[:conditional_filter][:options], + category: option[:conditional_filter][:category], + label: option[:conditional_filter][:label], + secondary: true, + } %> + <% end %> <% end %> + <% end %> <% end %> diff --git a/app/views/filters/_select_filter.html.erb b/app/views/filters/_select_filter.html.erb new file mode 100644 index 000000000..e2c71988e --- /dev/null +++ b/app/views/filters/_select_filter.html.erb @@ -0,0 +1,7 @@ +<%= f.govuk_collection_select category.to_sym, + collection, + :id, + :name, + label: { hidden: secondary }, + options: { disabled: [""], selected: selected_option(category) }, + "data-controller": "accessible-autocomplete" %> diff --git a/spec/helpers/filters_helper_spec.rb b/spec/helpers/filters_helper_spec.rb index 296e336a4..e665e32a0 100644 --- a/spec/helpers/filters_helper_spec.rb +++ b/spec/helpers/filters_helper_spec.rb @@ -16,8 +16,8 @@ RSpec.describe FiltersHelper do context "when looking at the all value" do it "returns true if no filters have been set yet" do - expect(filter_selected?("user", :all)).to be_truthy - expect(filter_selected?("user", :yours)).to be_falsey + expect(filter_selected?("user", :all)).to be true + expect(filter_selected?("user", :yours)).to be false end end end @@ -28,11 +28,33 @@ RSpec.describe FiltersHelper do end it "returns false for non selected filters" do - expect(filter_selected?("status", "completed")).to be_falsey + expect(filter_selected?("status", "completed")).to be false end it "returns true for selected filter" do - expect(filter_selected?("status", "in_progress")).to be_truthy + expect(filter_selected?("status", "in_progress")).to be true + end + end + + context "when support user is using the organisation filter" do + before do + session[:case_logs_filters] = { "organisation": "1" }.to_json + end + + it "returns true for the parent organisation_select filter" do + expect(filter_selected?("organisation_select", :specific_org)).to be true + expect(filter_selected?("organisation_select", :all)).to be false + end + end + + context "when support user has not set the organisation_select filter" do + before do + session[:case_logs_filters] = {}.to_json + end + + it "defaults to all organisations" do + expect(filter_selected?("organisation_select", :all)).to be true + expect(filter_selected?("organisation_select", :specific_org)).to be false end end end diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 4a8bc8677..005e8c057 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -1893,6 +1893,31 @@ RSpec.describe CaseLog do end end + context "when filtering by organisation" do + let(:organisation_1) { FactoryBot.create(:organisation) } + let(:organisation_2) { FactoryBot.create(:organisation) } + let(:organisation_3) { FactoryBot.create(:organisation) } + + before do + FactoryBot.create(:case_log, :in_progress, owning_organisation: organisation_1, managing_organisation: organisation_1) + FactoryBot.create(:case_log, :completed, owning_organisation: organisation_1, managing_organisation: organisation_2) + FactoryBot.create(:case_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_1) + FactoryBot.create(:case_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_2) + end + + it "filters by given organisation id" do + expect(described_class.filter_by_organisation([organisation_1.id]).count).to eq(3) + expect(described_class.filter_by_organisation([organisation_1.id, organisation_2.id]).count).to eq(4) + expect(described_class.filter_by_organisation([organisation_3.id]).count).to eq(0) + end + + it "filters by given organisation" do + expect(described_class.filter_by_organisation([organisation_1]).count).to eq(3) + expect(described_class.filter_by_organisation([organisation_1, organisation_2]).count).to eq(4) + expect(described_class.filter_by_organisation([organisation_3]).count).to eq(0) + end + end + context "when filtering on status" do it "allows filtering on a single status" do expect(described_class.filter_by_status(%w[in_progress]).count).to eq(2) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 97b1fdaf1..4cbebcdae 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -97,6 +97,10 @@ RSpec.describe User, type: :model do data_coordinator: 2, }) end + + it "can filter case logs by user, year and status" do + expect(user.case_logs_filters).to eq(%i[status years user]) + end end context "when the user is a Customer Support person" do @@ -119,6 +123,10 @@ RSpec.describe User, type: :model do support: 99, }) end + + it "can filter case logs by user, year, status and organisation" do + expect(user.case_logs_filters).to eq(%i[status years user organisation]) + end end end diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb index 9a9dd60e1..293a3997d 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -186,6 +186,7 @@ RSpec.describe CaseLogsController, type: :request do context "when filtering" do context "with status filter" do + let(:organisation_2) { FactoryBot.create(:organisation) } let!(:in_progress_case_log) do FactoryBot.create(:case_log, :in_progress, owning_organisation: organisation, @@ -193,7 +194,7 @@ RSpec.describe CaseLogsController, type: :request do end let!(:completed_case_log) do FactoryBot.create(:case_log, :completed, - owning_organisation: organisation, + owning_organisation: organisation_2, managing_organisation: organisation) end @@ -209,6 +210,12 @@ RSpec.describe CaseLogsController, type: :request do expect(page).not_to have_link(completed_case_log.id.to_s) end + it "filters on organisation" do + get "/logs?organisation[]=#{organisation_2.id}", headers: headers, params: {} + expect(page).to have_link(completed_case_log.id.to_s) + expect(page).not_to have_link(in_progress_case_log.id.to_s) + end + it "does not reset the filters" do get "/logs?status[]=in_progress", headers: headers, params: {} expect(page).to have_link(in_progress_case_log.id.to_s) @@ -344,6 +351,44 @@ RSpec.describe CaseLogsController, type: :request do it "shows the download csv link" do expect(page).to have_link("Download (CSV)", href: "/logs.csv") end + + it "does not show the organisation filter" do + expect(page).not_to have_field("organisation-field") + end + end + + context "when the user is a customer support user" do + let(:user) { FactoryBot.create(:user, :support) } + let(:org_1) { FactoryBot.create(:organisation) } + let(:org_2) { FactoryBot.create(:organisation) } + let(:tenant_code_1) { "TC5638" } + let(:tenant_code_2) { "TC8745" } + + before do + FactoryBot.create(:case_log, :in_progress, owning_organisation: org_1, tenant_code: tenant_code_1) + FactoryBot.create(:case_log, :in_progress, owning_organisation: org_2, tenant_code: tenant_code_2) + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end + + it "does show the organisation filter" do + get "/logs", headers:, params: {} + expect(page).to have_field("organisation-field") + end + + it "shows all logs by default" do + get "/logs", headers:, params: {} + expect(page).to have_content(tenant_code_1) + expect(page).to have_content(tenant_code_2) + end + + context "when filtering by organisation" do + it "only show the selected organisations logs" do + get "/logs?organisation_select=specific_org&organisation=#{org_1.id}", headers:, params: {} + expect(page).to have_content(tenant_code_1) + expect(page).not_to have_content(tenant_code_2) + end + end end context "when there are more than 20 logs" do From be86ff9f8a39ee9340a7caf15a792c3732ea1396 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Tue, 17 May 2022 11:08:09 +0100 Subject: [PATCH 2/4] CLDC-1224: Tenancy type and tenancy length (#578) * Fix value mapping for tenancy type 22/23 * Update secure helper * Null safe * Spec update * Update mandatory * Use helper --- app/models/case_log.rb | 29 +++- config/forms/2022_2023.json | 13 +- .../validations/tenancy_validations_spec.rb | 148 ++++++++++++++---- spec/requests/case_logs_controller_spec.rb | 3 + 4 files changed, 155 insertions(+), 38 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index df91376cf..6796be006 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -191,9 +191,20 @@ class CaseLog < ApplicationRecord tshortfall_known == 1 end + def is_fixed_term_tenancy? + [4, 6].include?(tenancy) + end + def is_secure_tenancy? + return unless collection_start_year + # 1: Secure (including flexible) - tenancy == 1 + if collection_start_year < 2022 + tenancy == 1 + else + # 6: Secure - fixed term, 7: Secure - lifetime + [6, 7].include?(tenancy) + end end def is_assured_shorthold_tenancy? @@ -461,9 +472,19 @@ private end def dynamically_not_required - previous_la_known_field = postcode_known? ? %w[previous_la_known] : [] - tshortfall_field = tshortfall_unknown? ? %w[tshortfall] : [] - previous_la_known_field + tshortfall_field + not_required = [] + not_required << "previous_la_known" if postcode_known? + not_required << "tshortfall" if tshortfall_unknown? + not_required << "tenancylength" if tenancylength_optional? + + not_required + end + + def tenancylength_optional? + return false unless collection_start_year + return true if collection_start_year < 2022 + + collection_start_year >= 2022 && !is_fixed_term_tenancy? end def set_derived_fields! diff --git a/config/forms/2022_2023.json b/config/forms/2022_2023.json index 7c7c2e66b..c6ed9dcad 100644 --- a/config/forms/2022_2023.json +++ b/config/forms/2022_2023.json @@ -994,7 +994,7 @@ "hint_text": "", "type": "radio", "answer_options": { - "1": { + "4": { "value": "Assured Shorthold Tenancy (AST) - Fixed term" }, "6": { @@ -1041,7 +1041,7 @@ "hint_text": "This is also known as an ‘introductory period’.", "type": "radio", "answer_options": { - "1": { + "4": { "value": "Assured Shorthold Tenancy (AST) - Fixed term" }, "6": { @@ -1095,10 +1095,13 @@ }, "depends_on": [ { - "tenancy": 1 + "tenancy": 4 }, { "tenancy": 6 + }, + { + "tenancy": 3 } ] }, @@ -6297,7 +6300,7 @@ "label": true, "i18n_template": "la" }, - { + { "key": "soft_min_for_period", "label": false, "i18n_template": "soft_min_for_period" @@ -6336,7 +6339,7 @@ "label": true, "i18n_template": "la" }, - { + { "key": "soft_max_for_period", "label": false, "i18n_template": "soft_max_for_period" diff --git a/spec/models/validations/tenancy_validations_spec.rb b/spec/models/validations/tenancy_validations_spec.rb index 09b3c5314..9e80fc2d1 100644 --- a/spec/models/validations/tenancy_validations_spec.rb +++ b/spec/models/validations/tenancy_validations_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Validations::TenancyValidations do subject(:tenancy_validator) { validator_class.new } let(:validator_class) { Class.new { include Validations::TenancyValidations } } - let(:record) { FactoryBot.create(:case_log) } + let(:record) { FactoryBot.create(:case_log, startdate: Time.zone.local(2021, 5, 1)) } describe "fixed term tenancy validations" do context "when fixed term tenancy" do @@ -62,44 +62,134 @@ RSpec.describe Validations::TenancyValidations do end end - context "when type of tenancy is secure" do - let(:expected_error) { I18n.t("validations.tenancy.length.secure") } + context "when the collection start year is before 2022" do + context "when type of tenancy is secure" do + let(:expected_error) { I18n.t("validations.tenancy.length.secure") } - before { record.tenancy = 1 } + before { record.tenancy = 1 } - context "when tenancy length is greater than 1" do - it "adds an error" do - record.tenancylength = 1 - tenancy_validator.validate_fixed_term_tenancy(record) - expect(record.errors["tenancylength"]).to include(match(expected_error)) - expect(record.errors["tenancy"]).to include(match(expected_error)) + context "when tenancy length is greater than 1" do + it "adds an error" do + record.tenancylength = 1 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to include(match(expected_error)) + expect(record.errors["tenancy"]).to include(match(expected_error)) + end end - end - context "when tenancy length is less than 100" do - it "adds an error" do - record.tenancylength = 100 - tenancy_validator.validate_fixed_term_tenancy(record) - expect(record.errors["tenancylength"]).to include(match(expected_error)) - expect(record.errors["tenancy"]).to include(match(expected_error)) + context "when tenancy length is less than 100" do + it "adds an error" do + record.tenancylength = 100 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to include(match(expected_error)) + expect(record.errors["tenancy"]).to include(match(expected_error)) + end + end + + context "when tenancy length is between 2-99" do + it "does not add an error" do + record.tenancylength = 3 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to be_empty + expect(record.errors["tenancy"]).to be_empty + end + end + + context "when tenancy length has not been answered" do + it "does not add an error" do + record.tenancylength = nil + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to be_empty + expect(record.errors["tenancy"]).to be_empty + end end end + end - context "when tenancy length is between 2-99" do - it "does not add an error" do - record.tenancylength = 3 - tenancy_validator.validate_fixed_term_tenancy(record) - expect(record.errors["tenancylength"]).to be_empty - expect(record.errors["tenancy"]).to be_empty + context "when the collection start year is 2022 or later" do + let(:record) { FactoryBot.create(:case_log, startdate: Time.zone.local(2022, 5, 1)) } + + context "when type of tenancy is Secure - fixed term" do + let(:expected_error) { I18n.t("validations.tenancy.length.secure") } + + before { record.tenancy = 6 } + + context "when tenancy length is greater than 1" do + it "adds an error" do + record.tenancylength = 1 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to include(match(expected_error)) + expect(record.errors["tenancy"]).to include(match(expected_error)) + end + end + + context "when tenancy length is less than 100" do + it "adds an error" do + record.tenancylength = 100 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to include(match(expected_error)) + expect(record.errors["tenancy"]).to include(match(expected_error)) + end + end + + context "when tenancy length is between 2-99" do + it "does not add an error" do + record.tenancylength = 3 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to be_empty + expect(record.errors["tenancy"]).to be_empty + end + end + + context "when tenancy length has not been answered" do + it "does not add an error" do + record.tenancylength = nil + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to be_empty + expect(record.errors["tenancy"]).to be_empty + end end end - context "when tenancy length has not been answered" do - it "does not add an error" do - record.tenancylength = nil - tenancy_validator.validate_fixed_term_tenancy(record) - expect(record.errors["tenancylength"]).to be_empty - expect(record.errors["tenancy"]).to be_empty + context "when type of tenancy is Secure - lifetime" do + let(:expected_error) { I18n.t("validations.tenancy.length.secure") } + + before { record.tenancy = 7 } + + context "when tenancy length is greater than 1" do + it "adds an error" do + record.tenancylength = 1 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to include(match(expected_error)) + expect(record.errors["tenancy"]).to include(match(expected_error)) + end + end + + context "when tenancy length is less than 100" do + it "adds an error" do + record.tenancylength = 100 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to include(match(expected_error)) + expect(record.errors["tenancy"]).to include(match(expected_error)) + end + end + + context "when tenancy length is between 2-99" do + it "does not add an error" do + record.tenancylength = 3 + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to be_empty + expect(record.errors["tenancy"]).to be_empty + end + end + + context "when tenancy length has not been answered" do + it "does not add an error" do + record.tenancylength = nil + tenancy_validator.validate_fixed_term_tenancy(record) + expect(record.errors["tenancylength"]).to be_empty + expect(record.errors["tenancy"]).to be_empty + end end end end diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb index 293a3997d..3c91db3e4 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -239,6 +239,7 @@ RSpec.describe CaseLogsController, type: :request do owning_organisation: organisation, mrcdate: Time.zone.local(2022, 2, 1), startdate: Time.zone.local(2022, 12, 1), + tenancy: 6, managing_organisation: organisation) end @@ -267,6 +268,7 @@ RSpec.describe CaseLogsController, type: :request do owning_organisation: organisation, mrcdate: Time.zone.local(2022, 2, 1), startdate: Time.zone.local(2022, 12, 1), + tenancy: 6, managing_organisation: organisation) end let!(:case_log_2022_in_progress) do @@ -274,6 +276,7 @@ RSpec.describe CaseLogsController, type: :request do owning_organisation: organisation, mrcdate: Time.zone.local(2022, 2, 1), startdate: Time.zone.local(2022, 12, 1), + tenancy: 6, managing_organisation: organisation) end From d32150e2895f07bfcc6a9ba64f44a251ff9eb302 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Tue, 17 May 2022 12:16:28 +0100 Subject: [PATCH 3/4] Remove joint tenancy hitn text and add don't know option (#579) --- config/forms/2022_2023.json | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/config/forms/2022_2023.json b/config/forms/2022_2023.json index c6ed9dcad..93177a301 100644 --- a/config/forms/2022_2023.json +++ b/config/forms/2022_2023.json @@ -951,7 +951,7 @@ "joint": { "check_answer_label": "Is this a joint tenancy?", "header": "Is this a joint tenancy?", - "hint_text": "For example, 27 3 2021.", + "hint_text": "", "type": "radio", "answer_options": { "1": { @@ -959,6 +959,9 @@ }, "2": { "value": "No" + }, + "3": { + "value": "Don’t know" } } } From 27b9971a4a78d5959452aeb1c3556969ec601303 Mon Sep 17 00:00:00 2001 From: Ted-U <92022120+Ted-U@users.noreply.github.com> Date: Tue, 17 May 2022 14:40:42 +0100 Subject: [PATCH 4/4] Export improvements (#581) * improved export - set start point for reference * remove completed status from exports * Added exception handling to export * Improved tests, replaced rescue block with Sentry Co-authored-by: Kat --- .../exports/case_log_export_service.rb | 22 ++++++---- db/migrate/20220516111514_add_started_at.rb | 5 +++ db/schema.rb | 3 +- .../exports/case_log_export_service_spec.rb | 41 +++++++++++++++++++ 4 files changed, 63 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20220516111514_add_started_at.rb diff --git a/app/services/exports/case_log_export_service.rb b/app/services/exports/case_log_export_service.rb index 9b786d28e..e8aca786e 100644 --- a/app/services/exports/case_log_export_service.rb +++ b/app/services/exports/case_log_export_service.rb @@ -6,10 +6,12 @@ module Exports end def export_case_logs - case_logs = retrieve_case_logs - export = save_export_run + current_time = Time.zone.now + case_logs = retrieve_case_logs(current_time) + export = save_export_run(current_time) write_master_manifest(export) write_export_data(case_logs) + export.save! end def is_omitted_field?(field_name) @@ -22,14 +24,14 @@ module Exports private - def save_export_run + def save_export_run(current_time) today = Time.zone.today last_daily_run_number = LogsExport.where(created_at: today.beginning_of_day..today.end_of_day).maximum(:daily_run_number) last_daily_run_number = 0 if last_daily_run_number.nil? export = LogsExport.new export.daily_run_number = last_daily_run_number + 1 - export.save! + export.started_at = current_time export end @@ -49,9 +51,15 @@ module Exports @storage_service.write_file(file_path, string_io) end - def retrieve_case_logs - params = { from: Time.current.beginning_of_day, to: Time.current, status: CaseLog.statuses[:completed] } - CaseLog.where("updated_at >= :from and updated_at <= :to and status = :status", params) + def retrieve_case_logs(current_time) + recent_export = LogsExport.order("started_at").last + if recent_export + params = { from: recent_export.started_at, to: current_time } + CaseLog.where("updated_at >= :from and updated_at <= :to", params) + else + params = { to: current_time } + CaseLog.where("updated_at <= :to", params) + end end def build_manifest_csv_io diff --git a/db/migrate/20220516111514_add_started_at.rb b/db/migrate/20220516111514_add_started_at.rb new file mode 100644 index 000000000..1eedcf22a --- /dev/null +++ b/db/migrate/20220516111514_add_started_at.rb @@ -0,0 +1,5 @@ +class AddStartedAt < ActiveRecord::Migration[7.0] + def change + add_column :logs_exports, :started_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 240e514df..be67fb5f8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2022_05_13_123456) do +ActiveRecord::Schema[7.0].define(version: 2022_05_16_111514) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -261,6 +261,7 @@ ActiveRecord::Schema[7.0].define(version: 2022_05_13_123456) do create_table "logs_exports", force: :cascade do |t| t.integer "daily_run_number" t.datetime "created_at", default: -> { "CURRENT_TIMESTAMP" } + t.datetime "started_at" end create_table "organisation_las", force: :cascade do |t| diff --git a/spec/services/exports/case_log_export_service_spec.rb b/spec/services/exports/case_log_export_service_spec.rb index 4fb431df4..3f4007c70 100644 --- a/spec/services/exports/case_log_export_service_spec.rb +++ b/spec/services/exports/case_log_export_service_spec.rb @@ -46,10 +46,17 @@ RSpec.describe Exports::CaseLogExportService do end context "and case logs are available for export" do + let(:time_now) { Time.zone.now } + before do + Timecop.freeze(time_now) case_log end + after do + LogsExport.destroy_all + end + it "generates an XML export file with the expected filename" do expect(storage_service).to receive(:write_file).with(expected_data_filename, any_args) export_service.export_case_logs @@ -63,6 +70,30 @@ RSpec.describe Exports::CaseLogExportService do export_service.export_case_logs expect(actual_content).to eq(expected_content) end + + it "creates a logs export record in a database with correct time" do + export_service.export_case_logs + records_from_db = ActiveRecord::Base.connection.execute("select started_at, id from logs_exports ").to_a + expect(records_from_db[0]["started_at"]).to eq(time_now) + expect(records_from_db.count).to eq(1) + end + + it "gets the logs for correct timeframe" do + start_time = Time.zone.local(2022, 4, 13, 2, 2, 2) + export = LogsExport.new(started_at: start_time, daily_run_number: 1) + export.save! + params = { from: start_time, to: time_now } + allow(CaseLog).to receive(:where).with("updated_at >= :from and updated_at <= :to", params).once.and_return([]) + export_service.export_case_logs + end + + context "when this is the first export" do + it "gets the logs for the timeframe up until the current time" do + params = { to: time_now } + allow(CaseLog).to receive(:where).with("updated_at <= :to", params).once.and_return([]) + export_service.export_case_logs + end + end end context "and a previous export has run the same day" do @@ -75,5 +106,15 @@ RSpec.describe Exports::CaseLogExportService do export_service.export_case_logs end end + + context "when export has an error" do + it "does not save a record in the database" do + allow(storage_service).to receive(:write_file).and_raise(StandardError.new("This is an exception")) + export = LogsExport.new + allow(LogsExport).to receive(:new).and_return(export) + expect(export).not_to receive(:save!) + expect { export_service.export_case_logs }.to raise_error(StandardError) + end + end end end