From 669f8338c5e68640776f8e300d5154a99864f8e5 Mon Sep 17 00:00:00 2001 From: Oscar Richardson <116292912+oscar-richardson-softwire@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:02:00 +0100 Subject: [PATCH] CLDC-4461: Hide confidential addresses - fix confidential to non confidential edge case (#3373) * Fix bug where the UPRN/address questions were not shown after switching from a confidential to a non-confidential scheme * Improve tests * Hide address data in CSV download for logs in confidential schemes * Action PR comments * Action second round of PR comments --- .../lettings_log_variables.rb | 1 - app/services/csv/lettings_log_csv_service.rb | 38 ++++++++++ .../lettings/pages/address_fallback_spec.rb | 73 ++++++++++++++++++ .../lettings/pages/address_search_spec.rb | 73 ++++++++++++++++++ .../lettings_log_derived_fields_spec.rb | 42 +++++++++- .../lettings/year2026/row_parser_spec.rb | 13 +++- .../csv/lettings_log_csv_service_spec.rb | 76 +++++++++++++++++++ 7 files changed, 308 insertions(+), 8 deletions(-) diff --git a/app/models/derived_variables/lettings_log_variables.rb b/app/models/derived_variables/lettings_log_variables.rb index 4b6ed27af..230b2a1dc 100644 --- a/app/models/derived_variables/lettings_log_variables.rb +++ b/app/models/derived_variables/lettings_log_variables.rb @@ -185,7 +185,6 @@ module DerivedVariables::LettingsLogVariables reset_address_fields! self.uprn_selection = nil self.postcode_known = nil - self.manual_address_entry_selected = nil end clear_gender_description_unless_gender_not_same_as_sex! if form.start_year_2026_or_later? diff --git a/app/services/csv/lettings_log_csv_service.rb b/app/services/csv/lettings_log_csv_service.rb index dd3be8595..6521fa52c 100644 --- a/app/services/csv/lettings_log_csv_service.rb +++ b/app/services/csv/lettings_log_csv_service.rb @@ -280,6 +280,32 @@ module Csv SCHEME_AND_LOCATION_ATTRIBUTES = %w[scheme_code scheme_service_name scheme_confidential SCHTYPE scheme_registered_under_care_act scheme_owning_organisation_name scheme_primary_client_group scheme_has_other_client_group scheme_secondary_client_group scheme_support_type scheme_intended_stay scheme_created_at location_code location_postcode location_name location_units location_type_of_unit location_mobility_type location_local_authority location_startdate].freeze + # TODO: (CLDC-4462): delete once address data for logs in confidential schemes is wiped. + # Interim measure: logs in a confidential scheme that were created before the + # confidential address feature may still hold property address/UPRN data that is no + # longer collected. Blank those columns in the download until the data is wiped (the + # local authority, derived from the scheme location, is intentionally retained). + ADDRESS_FIELDS_HIDDEN_FOR_CONFIDENTIAL_SCHEME = %w[ + uprn + uprn_known + uprn_confirmed + uprn_selection + address_line1 + address_line2 + town_or_city + county + postcode_full + postcode_known + address_line1_input + postcode_full_input + address_line1_as_entered + address_line2_as_entered + town_or_city_as_entered + county_as_entered + postcode_full_as_entered + la_as_entered + ].freeze + def lettings_log_attributes ordered_questions = FormHandler.instance.ordered_questions_for_year(@year, "lettings") soft_validations_attributes = soft_validations_attributes(ordered_questions) @@ -344,6 +370,8 @@ module Csv def value(attribute, log) attribute = "rent_type" if attribute == "rent_type_detail" # rent_type_detail is the requested column header for rent_type, so as not to confuse with renttype. It can be exported as label or code. + return nil if hide_confidential_scheme_address?(attribute, log) # TODO: (CLDC-4462): delete once address data for logs in confidential schemes is wiped. + if CUSTOM_CALL_CHAINS.key? attribute.to_sym call_chain = CUSTOM_CALL_CHAINS[attribute.to_sym][@export_type.to_sym] call_chain.reduce(log) { |object, next_call| object&.public_send(next_call) } @@ -376,6 +404,16 @@ module Csv end end + # TODO: (CLDC-4462): delete once address data for logs in confidential schemes is wiped. + def hide_confidential_scheme_address?(attribute, log) + ADDRESS_FIELDS_HIDDEN_FOR_CONFIDENTIAL_SCHEME.include?(attribute) && confidential_scheme_ids.include?(log.scheme_id) + end + + # TODO: (CLDC-4462): delete once address data for logs in confidential schemes is wiped. + def confidential_scheme_ids + @confidential_scheme_ids ||= Scheme.where(sensitive: "Yes").pluck(:id).to_set + end + def person_details_not_known?(log, attribute) details_known_field = PERSON_DETAILS.find { |key, _value| key == attribute }[1]["details_known_field"] log[details_known_field] == 1 # 1 for lettings logs, 2 for sales logs diff --git a/spec/models/form/lettings/pages/address_fallback_spec.rb b/spec/models/form/lettings/pages/address_fallback_spec.rb index c4f720ba3..51add5b8f 100644 --- a/spec/models/form/lettings/pages/address_fallback_spec.rb +++ b/spec/models/form/lettings/pages/address_fallback_spec.rb @@ -26,4 +26,77 @@ RSpec.describe Form::Lettings::Pages::AddressFallback, type: :model do it "has correct depends_on" do expect(page.depends_on).to eq([{ "manual_address_entry_selected" => true, "is_address_asked?" => true }]) end + + context "when routing to the page" do + let(:form) { FormHandler.instance.forms["current_lettings"] } + let(:subsection) { instance_double(Form::Subsection, form:, enabled?: true) } + + context "when the log is general needs" do + let(:log) { build(:lettings_log, needstype: 1) } + + it "is routed to when the address is being entered manually" do + log.manual_address_entry_selected = true + expect(page).to be_routed_to(log, nil) + end + + it "is not routed to when the address is not being entered manually" do + log.manual_address_entry_selected = false + expect(page).not_to be_routed_to(log, nil) + end + + it "is not routed to when `manual_address_entry_selected` is nil" do + log.manual_address_entry_selected = nil + expect(page).not_to be_routed_to(log, nil) + end + end + + context "when the log is supported housing" do + let(:log) { build(:lettings_log, needstype: 2) } + + context "and the collection year is 2026 or later" do + before do + allow(form).to receive(:start_year_2026_or_later?).and_return(true) + end + + it "is routed to when the address is being entered manually" do + log.manual_address_entry_selected = true + expect(page).to be_routed_to(log, nil) + end + + it "is not routed to when the address is not being entered manually" do + log.manual_address_entry_selected = false + expect(page).not_to be_routed_to(log, nil) + end + + it "is not routed to when `manual_address_entry_selected` is nil" do + log.manual_address_entry_selected = nil + expect(page).not_to be_routed_to(log, nil) + end + end + + context "and the collection year is before 2026" do + before do + allow(form).to receive(:start_year_2026_or_later?).and_return(false) + end + + it "is not routed to, even when the address is being entered manually" do + log.manual_address_entry_selected = true + expect(page).not_to be_routed_to(log, nil) + end + end + + context "when the scheme has confidential information" do + let(:log) { build(:lettings_log, needstype: 2, scheme: build(:scheme, sensitive: 1)) } + + before do + allow(form).to receive(:start_year_2026_or_later?).and_return(true) + end + + it "is not routed to, even when the address is being entered manually" do + log.manual_address_entry_selected = true + expect(page).not_to be_routed_to(log, nil) + end + end + end + end end diff --git a/spec/models/form/lettings/pages/address_search_spec.rb b/spec/models/form/lettings/pages/address_search_spec.rb index 0bf54bae8..640ce6498 100644 --- a/spec/models/form/lettings/pages/address_search_spec.rb +++ b/spec/models/form/lettings/pages/address_search_spec.rb @@ -39,4 +39,77 @@ RSpec.describe Form::Lettings::Pages::AddressSearch, type: :model do expect(page.question_number).to eq(16) end end + + context "when routing to the page" do + let(:form) { FormHandler.instance.forms["current_lettings"] } + let(:subsection) { instance_double(Form::Subsection, form:, enabled?: true) } + + context "when the log is general needs" do + let(:log) { build(:lettings_log, needstype: 1) } + + it "is routed to when the address is not being entered manually" do + log.manual_address_entry_selected = false + expect(page).to be_routed_to(log, nil) + end + + it "is not routed to when the address is being entered manually" do + log.manual_address_entry_selected = true + expect(page).not_to be_routed_to(log, nil) + end + + it "is not routed to when `manual_address_entry_selected` is nil" do + log.manual_address_entry_selected = nil + expect(page).not_to be_routed_to(log, nil) + end + end + + context "when the log is supported housing" do + let(:log) { build(:lettings_log, needstype: 2) } + + context "and the collection year is 2026 or later" do + before do + allow(form).to receive(:start_year_2026_or_later?).and_return(true) + end + + it "is routed to when the address is not being entered manually" do + log.manual_address_entry_selected = false + expect(page).to be_routed_to(log, nil) + end + + it "is not routed to when the address is being entered manually" do + log.manual_address_entry_selected = true + expect(page).not_to be_routed_to(log, nil) + end + + it "is not routed to when `manual_address_entry_selected` is nil" do + log.manual_address_entry_selected = nil + expect(page).not_to be_routed_to(log, nil) + end + end + + context "and the collection year is before 2026" do + before do + allow(form).to receive(:start_year_2026_or_later?).and_return(false) + end + + it "is not routed to, even when the address is not being entered manually" do + log.manual_address_entry_selected = false + expect(page).not_to be_routed_to(log, nil) + end + end + + context "when the scheme has confidential information" do + let(:log) { build(:lettings_log, needstype: 2, scheme: build(:scheme, sensitive: 1)) } + + before do + allow(form).to receive(:start_year_2026_or_later?).and_return(true) + end + + it "is not routed to, even when the address is not being entered manually" do + log.manual_address_entry_selected = false + expect(page).not_to be_routed_to(log, nil) + end + end + end + end end diff --git a/spec/models/lettings_log_derived_fields_spec.rb b/spec/models/lettings_log_derived_fields_spec.rb index 50d0d86e5..c418c13cf 100644 --- a/spec/models/lettings_log_derived_fields_spec.rb +++ b/spec/models/lettings_log_derived_fields_spec.rb @@ -1637,8 +1637,12 @@ RSpec.describe LettingsLog, type: :model do .and change { log.read_attribute(:county) }.from(county).to(nil) .and change { log.read_attribute(:postcode_full) }.from(postcode_full).to(nil) .and change { log.read_attribute(:uprn_selection) }.from(uprn_selection).to(nil) - .and change { log.read_attribute(:postcode_known) }.from(postcode_known).to(nil) - .and change { log.read_attribute(:manual_address_entry_selected) }.from(manual_address_entry_selected).to(nil) + .and(change { log.read_attribute(:postcode_known) }.from(postcode_known).to(nil)) + end + + it "does not reset `manual_address_entry_selected`" do + expect { log.set_derived_fields! } + .not_to(change { log.read_attribute(:manual_address_entry_selected) }) end context "when the log is a new-build first let" do @@ -1664,8 +1668,12 @@ RSpec.describe LettingsLog, type: :model do .and change { log.read_attribute(:county) }.from(county).to(nil) .and change { log.read_attribute(:postcode_full) }.from(postcode_full).to(nil) .and change { log.read_attribute(:uprn_selection) }.from(uprn_selection).to(nil) - .and change { log.read_attribute(:postcode_known) }.from(postcode_known).to(nil) - .and change { log.read_attribute(:manual_address_entry_selected) }.from(manual_address_entry_selected).to(nil) + .and(change { log.read_attribute(:postcode_known) }.from(postcode_known).to(nil)) + end + + it "does not reset `manual_address_entry_selected`" do + expect { log.set_derived_fields! } + .not_to(change { log.read_attribute(:manual_address_entry_selected) }) end end end @@ -1690,6 +1698,32 @@ RSpec.describe LettingsLog, type: :model do .to change(log, :manual_address_entry_selected).from(false).to(true) end end + + context "when a log is changed from a confidential to a non-confidential scheme" do + # The confidential logic must leave manual_address_entry_selected in a + # routable state (either true or false), otherwise neither address page routes after the + # switch and the address question is never shown again. + let(:confidential_scheme) { create(:scheme, sensitive: 1) } + let(:non_confidential_scheme) { create(:scheme, sensitive: 0) } + let(:confidential_location) { create(:location, scheme: confidential_scheme) } + let(:non_confidential_location) { create(:location, scheme: non_confidential_scheme) } + + before do + log.assign_attributes(manual_address_entry_selected: true) + log.scheme = confidential_scheme + log.location = confidential_location + log.set_derived_fields! + + log.scheme = non_confidential_scheme + log.location = non_confidential_location + log.set_derived_fields! + end + + it "asks the address question again with a routable value for `manual_address_entry_selected` (i.e., not nil)" do + expect(log.is_address_asked?).to be true + expect(log.manual_address_entry_selected).to be true + end + end end describe "#infer_at_most_one_relationship!" do diff --git a/spec/services/bulk_upload/lettings/year2026/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2026/row_parser_spec.rb index adcc262fb..2248daf45 100644 --- a/spec/services/bulk_upload/lettings/year2026/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2026/row_parser_spec.rb @@ -12,7 +12,7 @@ RSpec.describe BulkUpload::Lettings::Year2026::RowParser do let(:owning_org) { create(:organisation, :with_old_visible_id) } let(:managing_org) { create(:organisation, :with_old_visible_id, rent_periods: [4, 1]) } # Pinned non-confidential so the address/UPRN validations run as these tests expect; - # the confidential-scheme behaviour is covered by its own describe below (sensitive: 1). + # the confidential scheme behaviour is covered by its own describe below (sensitive: 1). let(:scheme) { create(:scheme, :with_old_visible_id, owning_organisation: owning_org, sensitive: 0) } let(:postcode_first_part) { "AA1".freeze } let(:postcode_second_part) { "1AA".freeze } @@ -1887,11 +1887,11 @@ RSpec.describe BulkUpload::Lettings::Year2026::RowParser do end context "when no UPRN or address fields are provided" do - let(:attributes) { base_attributes.merge({ field_18: nil, field_19: nil, field_21: nil, field_23: nil, field_24: nil }) } + let(:attributes) { base_attributes.merge({ field_18: nil, field_19: nil, field_20: nil, field_21: nil, field_22: nil, field_23: nil, field_24: nil }) } it "does not require the address or UPRN (no not answered errors)" do parser.valid? - %i[field_18 field_19 field_21 field_23 field_24].each do |field| + %i[field_18 field_19 field_20 field_21 field_22 field_23 field_24].each do |field| expect(parser.errors[field]).to be_empty end end @@ -1901,7 +1901,9 @@ RSpec.describe BulkUpload::Lettings::Year2026::RowParser do log.valid? expect(log.read_attribute(:uprn)).to be_nil expect(log.read_attribute(:address_line1)).to be_nil + expect(log.read_attribute(:address_line2)).to be_nil expect(log.read_attribute(:town_or_city)).to be_nil + expect(log.read_attribute(:county)).to be_nil expect(log.read_attribute(:postcode_full)).to be_nil end @@ -1917,7 +1919,9 @@ RSpec.describe BulkUpload::Lettings::Year2026::RowParser do base_attributes.merge({ field_18: "123456789012", field_19: "1 Test Street", + field_20: "Test District", field_21: "Testville", + field_22: "Testshire", field_23: postcode_first_part, field_24: postcode_second_part, field_25: "E09000008", @@ -1929,6 +1933,9 @@ RSpec.describe BulkUpload::Lettings::Year2026::RowParser do log.valid? expect(log.read_attribute(:uprn)).to be_nil expect(log.read_attribute(:address_line1)).to be_nil + expect(log.read_attribute(:address_line2)).to be_nil + expect(log.read_attribute(:town_or_city)).to be_nil + expect(log.read_attribute(:county)).to be_nil expect(log.read_attribute(:postcode_full)).to be_nil end diff --git a/spec/services/csv/lettings_log_csv_service_spec.rb b/spec/services/csv/lettings_log_csv_service_spec.rb index f2fd786c6..2e52328f4 100644 --- a/spec/services/csv/lettings_log_csv_service_spec.rb +++ b/spec/services/csv/lettings_log_csv_service_spec.rb @@ -193,6 +193,82 @@ RSpec.describe Csv::LettingsLogCsvService do end end + # TODO: (CLDC-4462): delete once address data for logs in confidential schemes is wiped. + describe "confidential scheme behaviour" do + let(:year) { 2026 } + let(:owning_organisation) { create(:organisation) } + let(:scheme) { create(:scheme, sensitive: 1, owning_organisation:) } + let(:location) { create(:location, scheme:) } + # Every hidden field that actually appears as a column in this export. + let(:hidden_columns) { described_class::ADDRESS_FIELDS_HIDDEN_FOR_CONFIDENTIAL_SCHEME & attribute_line } + let(:log) do + create( + :lettings_log, + :ignore_validation_errors, + needstype: 2, + owning_organisation:, + managing_organisation: owning_organisation, + assigned_to: user, + scheme:, + location:, + startdate: Time.zone.local(2026, 5, 1), + ).tap do |confidential_log| + # Simulate a log created before the confidential address feature that still holds + # property address data in the database. Populate every hidden address column so + # that the blanking is observable (a nil column would pass the assertion vacuously). + confidential_log.update_columns( + uprn: "123456789012", + uprn_known: 1, + uprn_confirmed: 1, + uprn_selection: "123456789012", + address_line1: "1 Secret Street", + address_line2: "Flat 2", + town_or_city: "Secretville", + county: "Secretshire", + postcode_full: "AB1 2CD", + postcode_known: 1, + address_line1_input: "1 Secret Street input", + postcode_full_input: "AB1 2CD", + address_line1_as_entered: "1 Secret Street as entered", + address_line2_as_entered: "Flat 2 as entered", + town_or_city_as_entered: "Secretville as entered", + county_as_entered: "Secretshire as entered", + postcode_full_as_entered: "AB1 2CD", + la_as_entered: "la as entered", + la: "E09000003", + ) + end + end + + def csv_value(attribute) + content_line[attribute_line.index(attribute)] + end + + context "when a log's scheme is confidential" do + it "blanks every hidden address and UPRN column" do + expect(hidden_columns).not_to be_empty + hidden_columns.each do |attribute| + expect(csv_value(attribute)).to be_nil, "expected the #{attribute} column to be blank for a confidential-scheme log" + end + end + + it "still exports the local authority" do + expect(csv_value("la")).to eq("E09000003") + end + end + + context "when the scheme is not confidential" do + let(:scheme) { create(:scheme, sensitive: 0, owning_organisation:) } + + it "exports every one of those columns as normal" do + expect(hidden_columns).not_to be_empty + hidden_columns.each do |attribute| + expect(csv_value(attribute)).not_to be_nil, "expected the #{attribute} column to be populated for a non-confidential-scheme log" + end + end + end + end + describe "the full CSV output" do context "when the requested log year is 2026" do let(:year) { 2026 }