From 9045d2c4213bcfe4fbddce16dab864b406383c43 Mon Sep 17 00:00:00 2001 From: Nat Dean-Lewis Date: Mon, 2 Mar 2026 11:22:27 +0000 Subject: [PATCH] CLDC-4173: cleanup and test improvements --- .../lettings_log_variables.rb | 12 ---- .../derived_variables/sales_log_variables.rb | 12 ---- .../sales/pages/buyer1_gender_same_as_sex.rb | 4 +- .../sales/pages/buyer2_gender_same_as_sex.rb | 4 +- .../questions/person_gender_same_as_sex.rb | 19 +++--- app/models/log.rb | 13 ++++ spec/factories/sales_log.rb | 3 + spec/fixtures/exports/sales_log_26_27.xml | 6 +- .../person_gender_description_spec.rb | 56 ++++++++++++++++++ .../person_gender_same_as_sex_spec.rb | 59 ++++++++++++++++--- .../sales/year2026/row_parser_spec.rb | 3 +- 11 files changed, 142 insertions(+), 49 deletions(-) diff --git a/app/models/derived_variables/lettings_log_variables.rb b/app/models/derived_variables/lettings_log_variables.rb index 6c88c3221..645eca1c7 100644 --- a/app/models/derived_variables/lettings_log_variables.rb +++ b/app/models/derived_variables/lettings_log_variables.rb @@ -450,18 +450,6 @@ private 3 if rent_type == 5 end - def clear_gender_description_unless_gender_not_same_as_sex! - # we do this as the gender same as sex page always contains the gender description box that's hidden - # default submit will send a "" for gender description. this ensure it's nil in this case - # as well as blanking it if the user writes it in mistakenly in bulk upload - (1..8).each do |person_index| - gender_same_as_sex = public_send("gender_same_as_sex#{person_index}") - if gender_same_as_sex.present? && gender_same_as_sex != 2 - self["gender_description#{person_index}"] = nil - end - end - end - def set_checkbox_values! form.questions.select { |q| q.type == "checkbox" }.each do |question| options = question.answer_keys_without_dividers diff --git a/app/models/derived_variables/sales_log_variables.rb b/app/models/derived_variables/sales_log_variables.rb index 2a41b7909..826a5c8f8 100644 --- a/app/models/derived_variables/sales_log_variables.rb +++ b/app/models/derived_variables/sales_log_variables.rb @@ -256,18 +256,6 @@ private [1, 2].include?(prevten) || [1, 2].include?(prevtenbuy2) end - def clear_gender_description_unless_gender_not_same_as_sex! - # we do this as the gender same as sex page always contains the gender description box that's hidden - # default submit will send a "" for gender description. this ensure it's nil in this case - # as well as blanking it if the user writes it in mistakenly in bulk upload - (1..6).each do |person_index| - gender_same_as_sex = public_send("gender_same_as_sex#{person_index}") - if gender_same_as_sex.present? && gender_same_as_sex != 2 - self["gender_description#{person_index}"] = nil - end - end - end - def reset_address_fields! self.uprn = nil self.uprn_known = nil diff --git a/app/models/form/sales/pages/buyer1_gender_same_as_sex.rb b/app/models/form/sales/pages/buyer1_gender_same_as_sex.rb index 5b802055e..6182a2028 100644 --- a/app/models/form/sales/pages/buyer1_gender_same_as_sex.rb +++ b/app/models/form/sales/pages/buyer1_gender_same_as_sex.rb @@ -14,8 +14,8 @@ class Form::Sales::Pages::Buyer1GenderSameAsSex < ::Form::Page def questions @questions ||= [ - Form::Sales::Questions::GenderSameAsSex1.new(nil, nil, self), - Form::Sales::Questions::GenderDescription1.new(nil, nil, self), + Form::Sales::Questions::PersonGenderSameAsSex.new(nil, nil, self, person_index: 1, buyer: true), + Form::Sales::Questions::PersonGenderDescription.new(nil, nil, self, person_index: 1), ] end end diff --git a/app/models/form/sales/pages/buyer2_gender_same_as_sex.rb b/app/models/form/sales/pages/buyer2_gender_same_as_sex.rb index 50ef13b48..426d6b3b2 100644 --- a/app/models/form/sales/pages/buyer2_gender_same_as_sex.rb +++ b/app/models/form/sales/pages/buyer2_gender_same_as_sex.rb @@ -16,8 +16,8 @@ class Form::Sales::Pages::Buyer2GenderSameAsSex < ::Form::Page def questions @questions ||= [ - Form::Sales::Questions::GenderSameAsSex2.new(nil, nil, self), - Form::Sales::Questions::GenderDescription2.new(nil, nil, self), + Form::Sales::Questions::PersonGenderSameAsSex.new(nil, nil, self, person_index: 2, buyer: true), + Form::Sales::Questions::PersonGenderDescription.new(nil, nil, self, person_index: 2), ] end end diff --git a/app/models/form/sales/questions/person_gender_same_as_sex.rb b/app/models/form/sales/questions/person_gender_same_as_sex.rb index fff76c761..907c8cce0 100644 --- a/app/models/form/sales/questions/person_gender_same_as_sex.rb +++ b/app/models/form/sales/questions/person_gender_same_as_sex.rb @@ -1,5 +1,5 @@ class Form::Sales::Questions::PersonGenderSameAsSex < ::Form::Question - def initialize(id, hsh, page, person_index:) + def initialize(id, hsh, page, person_index:, buyer: false) super(id, hsh, page) @id = "gender_same_as_sex#{person_index}" @type = "radio" @@ -7,22 +7,21 @@ class Form::Sales::Questions::PersonGenderSameAsSex < ::Form::Question @conditional_for = { "gender_description#{person_index}" => [2] } @inferred_check_answers_value = [{ "condition" => { "gender_same_as_sex#{person_index}" => 2 }, "value" => "No" }] @person_index = person_index + @buyer = buyer @question_number = QUESTION_NUMBER_FROM_YEAR[form.start_date.year] || QUESTION_NUMBER_FROM_YEAR[QUESTION_NUMBER_FROM_YEAR.keys.max] end - ANSWER_OPTIONS = { - "1" => { "value" => "Yes" }, - "2" => { "value" => "No, enter gender identity" }, - "divider" => { "value" => true }, - "3" => { "value" => "Person prefers not to say" }, - }.freeze + QUESTION_NUMBER_FROM_YEAR = { 2026 => 0 }.freeze def answer_options - ANSWER_OPTIONS + { + "1" => { "value" => "Yes" }, + "2" => { "value" => "No, enter gender identity" }, + "divider" => { "value" => true }, + "3" => { "value" => "#{@buyer ? 'Buyer' : 'Person'} prefers not to say" }, + }.freeze end - QUESTION_NUMBER_FROM_YEAR = { 2026 => 0 }.freeze - def label_from_value(value, _log = nil, _user = nil) return unless value diff --git a/app/models/log.rb b/app/models/log.rb index 195f63ba7..2d1f822c1 100644 --- a/app/models/log.rb +++ b/app/models/log.rb @@ -339,6 +339,19 @@ class Log < ApplicationRecord end end + def clear_gender_description_unless_gender_not_same_as_sex! + # we do this as the gender same as sex page always contains the gender description box that's hidden + # default submit will send a "" for gender description. this ensure it's nil in this case + # as well as blanking it if the user writes it in mistakenly in bulk upload + max_person = lettings? ? 8 : 6 + (1..max_person).each do |person_index| + gender_same_as_sex = public_send("gender_same_as_sex#{person_index}") + if gender_same_as_sex.present? && gender_same_as_sex != 2 + self["gender_description#{person_index}"] = nil + end + end + end + private # Handle logs that are older than previous collection start date diff --git a/spec/factories/sales_log.rb b/spec/factories/sales_log.rb index 37ff9e08d..d400f5784 100644 --- a/spec/factories/sales_log.rb +++ b/spec/factories/sales_log.rb @@ -310,6 +310,9 @@ FactoryBot.define do ethnic_group { 17 } sexrab2 { "X" } sex2 { "X" } + gender_same_as_sex1 { 1 } + gender_same_as_sex2 { 2 } + gender_description2 { "Non-binary" } buy2livein { "1" } ecstat1 { "1" } ecstat2 { "1" } diff --git a/spec/fixtures/exports/sales_log_26_27.xml b/spec/fixtures/exports/sales_log_26_27.xml index 14b876cea..3f03d3fdd 100644 --- a/spec/fixtures/exports/sales_log_26_27.xml +++ b/spec/fixtures/exports/sales_log_26_27.xml @@ -95,10 +95,10 @@ M 2 - + 1 - - + 2 + Non-binary diff --git a/spec/models/form/sales/questions/person_gender_description_spec.rb b/spec/models/form/sales/questions/person_gender_description_spec.rb index bafa340b2..deb03b6ff 100644 --- a/spec/models/form/sales/questions/person_gender_description_spec.rb +++ b/spec/models/form/sales/questions/person_gender_description_spec.rb @@ -23,6 +23,46 @@ RSpec.describe Form::Sales::Questions::PersonGenderDescription, type: :model do expect(question.type).to eq("text") end + context "when person 1" do + let(:person_index) { 1 } + + it "has the correct id" do + expect(question.id).to eq("gender_description1") + end + + it "has expected check answers card number" do + expect(question.check_answers_card_number).to eq(1) + end + + it "has the correct inferred_check_answers_value" do + expect(question.inferred_check_answers_value).to be_nil + end + + context "when gender_same_as_sex1 is 'Yes'" do + let(:log) { build(:sales_log, gender_same_as_sex1: 1) } + + it "is marked as derived" do + expect(question.derived?(log)).to be true + end + end + + context "when gender_same_as_sex1 is 'No'" do + let(:log) { build(:sales_log, gender_same_as_sex1: 2) } + + it "is not marked as derived" do + expect(question.derived?(log)).to be false + end + end + + context "when gender_same_as_sex1 is 'Prefers not to say'" do + let(:log) { build(:sales_log, gender_same_as_sex1: 3) } + + it "is marked as derived" do + expect(question.derived?(log)).to be true + end + end + end + context "when person 2" do let(:person_index) { 2 } @@ -37,6 +77,22 @@ RSpec.describe Form::Sales::Questions::PersonGenderDescription, type: :model do it "has the correct inferred_check_answers_value" do expect(question.inferred_check_answers_value).to be_nil end + + context "when gender_same_as_sex2 is 'Yes'" do + let(:log) { build(:sales_log, gender_same_as_sex2: 1) } + + it "is marked as derived" do + expect(question.derived?(log)).to be true + end + end + + context "when gender_same_as_sex2 is 'No'" do + let(:log) { build(:sales_log, gender_same_as_sex2: 2) } + + it "is not marked as derived" do + expect(question.derived?(log)).to be false + end + end end context "when person 3" do diff --git a/spec/models/form/sales/questions/person_gender_same_as_sex_spec.rb b/spec/models/form/sales/questions/person_gender_same_as_sex_spec.rb index 1b6ce7e1c..b95e77188 100644 --- a/spec/models/form/sales/questions/person_gender_same_as_sex_spec.rb +++ b/spec/models/form/sales/questions/person_gender_same_as_sex_spec.rb @@ -27,19 +27,64 @@ RSpec.describe Form::Sales::Questions::PersonGenderSameAsSex, type: :model do expect(question.derived?(nil)).to be false end - it "has the correct answer_options" do - expect(question.answer_options).to eq({ - "1" => { "value" => "Yes" }, - "2" => { "value" => "No, enter gender identity" }, - "divider" => { "value" => true }, - "3" => { "value" => "Person prefers not to say" }, - }) + context "when buyer is false (default)" do + it "has the correct answer_options" do + expect(question.answer_options).to eq({ + "1" => { "value" => "Yes" }, + "2" => { "value" => "No, enter gender identity" }, + "divider" => { "value" => true }, + "3" => { "value" => "Person prefers not to say" }, + }) + end + end + + context "when buyer is true" do + subject(:question) { described_class.new(question_id, question_definition, page, person_index:, buyer: true) } + + it "has the correct answer_options" do + expect(question.answer_options).to eq({ + "1" => { "value" => "Yes" }, + "2" => { "value" => "No, enter gender identity" }, + "divider" => { "value" => true }, + "3" => { "value" => "Buyer prefers not to say" }, + }) + end end it "returns correct label_from_value for 'Prefers not to say'" do expect(question.label_from_value(3)).to eq("Prefers not to say") end + it "returns nil label_from_value for nil" do + expect(question.label_from_value(nil)).to be_nil + end + + context "when person 1 (buyer)" do + subject(:question) { described_class.new(question_id, question_definition, page, person_index: 1, buyer: true) } + + let(:person_index) { 1 } + + it "has the correct id" do + expect(question.id).to eq("gender_same_as_sex1") + end + + it "has expected check answers card number" do + expect(question.check_answers_card_number).to eq(1) + end + + it "has the correct conditional_for" do + expect(question.conditional_for).to eq({ "gender_description1" => [2] }) + end + + it "has the correct inferred_check_answers_value" do + expect(question.inferred_check_answers_value).to eq([{ "condition" => { "gender_same_as_sex1" => 2 }, "value" => "No" }]) + end + + it "has the correct answer_options with Buyer label" do + expect(question.answer_options["3"]).to eq({ "value" => "Buyer prefers not to say" }) + end + end + context "when person 2" do let(:person_index) { 2 } diff --git a/spec/services/bulk_upload/sales/year2026/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2026/row_parser_spec.rb index 5032eabd3..ab7a5b72d 100644 --- a/spec/services/bulk_upload/sales/year2026/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2026/row_parser_spec.rb @@ -120,7 +120,8 @@ RSpec.describe BulkUpload::Sales::Year2026::RowParser do field_127: "R", field_128: "1", field_129: "1", - field_131: "1", + field_131: "2", + field_132: "Non-binary", } end