From b6c6b4275c7adb194b37ff1c3f43c8563fe7bb43 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 5 Jul 2024 15:16:27 +0100 Subject: [PATCH 01/14] Don't use TRN in match When validating that all claims for a given payment belong to the same claimant we can no longer use TRN as some journeys don't capture a trn. Eg if we have two claims one for ECP and one for IRP then the ECP claim will have a TRN but the IRP claim will not and so this check will fail. After discussion with the policy team we've decided to use the NINO as part of this check. There is some concern that applicants may mistype their NINO, if this proves an issue in practice we can just drop the NINO from the list of compared attributes and rely on the matching bank details. We've also removed the check on top ups that a candidate matching claim's policy is in the `OTHER_CLAIMABLE_POLICIES` list of the claim we're checking against. This check was in place to support some polices having trns and some not having trns and as we're now matching on NINO it is no longer needed. --- app/models/claim.rb | 15 ++++++++ .../claim/claims_preventing_payment_finder.rb | 17 ++------- app/models/payment.rb | 14 ++------ app/models/payroll_run.rb | 2 +- app/models/topup.rb | 2 ++ .../claims_preventing_payment_finder_spec.rb | 17 +++++++-- spec/models/claim_spec.rb | 36 ++++++++++++++++--- spec/models/decision_spec.rb | 2 +- spec/models/payment_spec.rb | 9 +---- spec/models/payroll_run_spec.rb | 4 +-- 10 files changed, 73 insertions(+), 45 deletions(-) diff --git a/app/models/claim.rb b/app/models/claim.rb index 717632158a..0869d7738c 100644 --- a/app/models/claim.rb +++ b/app/models/claim.rb @@ -82,6 +82,9 @@ class Claim < ApplicationRecord }.freeze DECISION_DEADLINE = 12.weeks DECISION_DEADLINE_WARNING_POINT = 2.weeks + CLAIMANT_MATCHING_ATTRIBUTES = %i[ + national_insurance_number + ] # Use AcademicYear as custom ActiveRecord attribute type attribute :academic_year, AcademicYear::Type.new @@ -178,6 +181,12 @@ class Claim < ApplicationRecord scope :current_academic_year, -> { by_academic_year(AcademicYear.current) } scope :failed_bank_validation, -> { where(hmrc_bank_validation_succeeded: false) } + scope :with_same_claimant, ->(claim) do + CLAIMANT_MATCHING_ATTRIBUTES.reduce(where.not(id: claim.id)) do |scope, attr| + scope.where(attr => claim.public_send(attr)) + end + end + delegate :award_amount, to: :eligibility scope :payrollable, -> { approved.not_awaiting_qa.left_joins(:payments).where(payments: nil).order(submitted_at: :asc) } @@ -401,6 +410,12 @@ def set_a_reminder? ) end + def same_claimant?(other_claim) + CLAIMANT_MATCHING_ATTRIBUTES.all? do |attr| + public_send(attr) == other_claim.public_send(attr) + end + end + private def normalise_ni_number diff --git a/app/models/claim/claims_preventing_payment_finder.rb b/app/models/claim/claims_preventing_payment_finder.rb index d0f467962a..716c1b9092 100644 --- a/app/models/claim/claims_preventing_payment_finder.rb +++ b/app/models/claim/claims_preventing_payment_finder.rb @@ -15,9 +15,6 @@ def initialize(claim) # The returned claims have different payment or tax details to those # provided by `claim`, and hence `claim` cannot be paid in the same payment # as the returned claims. - # - # NOTE: This only works for ECP/LUPP and TSLR cross policy as this requires a TRN - # Driven by: Policies.policies_claimable(policy) using OTHER_CLAIMABLE_POLICIES config otherwise this just returns [] def claims_preventing_payment @claims_preventing_payment ||= find_claims_preventing_payment end @@ -25,20 +22,12 @@ def claims_preventing_payment private def find_claims_preventing_payment - return [] if claim.policy == Policies::InternationalRelocationPayments - - eligibility_ids = claim.policy.policies_claimable.map { |policy| - policy::Eligibility.where(teacher_reference_number: claim.eligibility.teacher_reference_number) - }.flatten.map(&:id) + return [] if claim.personal_data_removed? - payrollable_claims_from_same_claimant = Claim.payrollable.where(eligibility_id: eligibility_ids) + payrollable_claims_from_same_claimant = Claim.payrollable.with_same_claimant(claim) payrollable_topup_claims_from_same_claimant = Topup.includes(:claim).payrollable - .select { |t| - claim.policy.policy_eligibilities_claimable.map(&:to_s).include?(t.claim.eligibility_type) && - t.claim.eligibility.teacher_reference_number == claim.eligibility.teacher_reference_number - } - .map(&:claim) + .select { |t| claim.same_claimant?(t.claim) }.map(&:claim) [payrollable_claims_from_same_claimant, payrollable_topup_claims_from_same_claimant].reduce([], :concat).select do |other_claim| Payment::PERSONAL_CLAIM_DETAILS_ATTRIBUTES_FORBIDDING_DISCREPANCIES.any? do |attribute| diff --git a/app/models/payment.rb b/app/models/payment.rb index e9fdf3027c..9136ea9eb4 100644 --- a/app/models/payment.rb +++ b/app/models/payment.rb @@ -29,7 +29,6 @@ class Payment < ApplicationRecord postcode has_student_loan banking_name - national_insurance_number ] PERSONAL_CLAIM_DETAILS_ATTRIBUTES_FORBIDDING_DISCREPANCIES = %i[ date_of_birth @@ -37,10 +36,7 @@ class Payment < ApplicationRecord bank_sort_code bank_account_number building_society_roll_number - ] - - PERSONAL_ELIGIBILITY_DETAILS_ATTRIBUTES_FORBIDDING_DISCREPANCIES = %i[ - teacher_reference_number + national_insurance_number ] delegate(*(PERSONAL_CLAIM_DETAILS_ATTRIBUTES_PERMITTING_DISCREPANCIES + PERSONAL_CLAIM_DETAILS_ATTRIBUTES_FORBIDDING_DISCREPANCIES), to: :claim_for_personal_details) @@ -61,15 +57,9 @@ def personal_details_must_be_consistent attribute_values.uniq.count > 1 && !attribute_values.all?(&:blank?) } - mismatching_eligibility_attributes = PERSONAL_ELIGIBILITY_DETAILS_ATTRIBUTES_FORBIDDING_DISCREPANCIES.select { |attribute| - attribute_values = claims.map(&:eligibility).map(&attribute) - attribute_values.uniq.count > 1 && !attribute_values.all?(&:blank?) - } - - if mismatching_attributes.any? || mismatching_eligibility_attributes.any? + if mismatching_attributes.any? claims_sentence = claims.map(&:reference).to_sentence attributes_list = mismatching_attributes.map { |attribute| Claim.human_attribute_name(attribute).downcase } - attributes_list += mismatching_eligibility_attributes.map { |attribute| claims.first.eligibility.class.human_attribute_name(attribute).downcase } attributes_sentence = attributes_list.to_sentence errors.add(:claims, "#{claims_sentence} have different values for #{attributes_sentence}") diff --git a/app/models/payroll_run.rb b/app/models/payroll_run.rb index e40b085a05..2c80007bbf 100644 --- a/app/models/payroll_run.rb +++ b/app/models/payroll_run.rb @@ -63,7 +63,7 @@ def self.create_with_claims!(claims, topups, attrs = {}) end def self.group_by_field(obj) - obj.is_a?(Claim) ? obj.eligibility.teacher_reference_number : obj.teacher_reference_number + obj.national_insurance_number end def download_triggered? diff --git a/app/models/topup.rb b/app/models/topup.rb index dc746d2a19..c6c996dee6 100644 --- a/app/models/topup.rb +++ b/app/models/topup.rb @@ -10,6 +10,8 @@ class Topup < ApplicationRecord validates :award_amount, presence: {message: "Enter top up amount"} validate :award_amount_must_be_in_range, on: :create + delegate :national_insurance_number, to: :claim + def payrolled? payment.present? end diff --git a/spec/models/claim/claims_preventing_payment_finder_spec.rb b/spec/models/claim/claims_preventing_payment_finder_spec.rb index 5311772fdc..14ea1c1e9f 100644 --- a/spec/models/claim/claims_preventing_payment_finder_spec.rb +++ b/spec/models/claim/claims_preventing_payment_finder_spec.rb @@ -11,6 +11,7 @@ bank_account_number: "32828838", bank_sort_code: "183828", first_name: "Boris", + national_insurance_number: "QQ123456C", eligibility_attributes: {teacher_reference_number: generate(:teacher_reference_number)} } end @@ -42,10 +43,20 @@ end it "includes the other claim where a topup is payrollable" do - eligibility_attributes = inconsistent_personal_details.delete(:eligibility_attributes) + lup_eligibility = create( + :levelling_up_premium_payments_eligibility, + :eligible, + award_amount: 1500.0 + ) - lup_eligibility = create(:levelling_up_premium_payments_eligibility, :eligible, award_amount: 1500.0, **eligibility_attributes) - other_claim = create(:claim, :approved, inconsistent_personal_details.merge(policy: Policies::LevellingUpPremiumPayments, eligibility: lup_eligibility)) + other_claim = create( + :claim, + :approved, + inconsistent_personal_details.merge( + policy: Policies::LevellingUpPremiumPayments, + eligibility: lup_eligibility + ) + ) create(:payment, claims: [other_claim]) other_claim.topups.create(award_amount: "500.00", created_by: user) diff --git a/spec/models/claim_spec.rb b/spec/models/claim_spec.rb index ba2eca6132..a966149c75 100644 --- a/spec/models/claim_spec.rb +++ b/spec/models/claim_spec.rb @@ -34,6 +34,16 @@ end end end + + describe "::with_same_claimant" do + let(:claim_1) { create(:claim, national_insurance_number: "AB123456A") } + let(:claim_2) { create(:claim, national_insurance_number: "AB123456A") } + let(:claim_3) { create(:claim, national_insurance_number: "CD123456A") } + + subject { Claim.with_same_claimant(claim_1) } + + it { is_expected.to contain_exactly(claim_2) } + end end it "validates academic years are formated like '2020/2021'" do @@ -326,11 +336,11 @@ expect(claim_with_decision.approvable?).to eq false end - it "returns false when there exists another payrollable claim with the same teacher reference number but with inconsistent attributes that would prevent us from running payroll" do - teacher_reference_number = generate(:teacher_reference_number) - create(:claim, :approved, eligibility_attributes: {teacher_reference_number: teacher_reference_number}, date_of_birth: 20.years.ago) + it "returns false when there exists another payrollable claim with the same national insurance number but with inconsistent attributes that would prevent us from running payroll" do + national_insurance_number = generate(:national_insurance_number) + create(:claim, :approved, national_insurance_number: national_insurance_number, date_of_birth: 20.years.ago) - expect(create(:claim, :submitted, eligibility_attributes: {teacher_reference_number: teacher_reference_number}, date_of_birth: 30.years.ago).approvable?).to eq false + expect(create(:claim, :submitted, national_insurance_number: national_insurance_number, date_of_birth: 30.years.ago).approvable?).to eq false end context "when the claim is held" do @@ -1280,4 +1290,22 @@ it { is_expected.to be_a(claim.policy::DqtRecord) } end end + + describe "#same_claimant?" do + subject { claim.same_claimant?(other_claim) } + + let(:claim) { create(:claim, national_insurance_number: "AA12345A") } + + context "with the same claimant" do + let(:other_claim) { create(:claim, national_insurance_number: "AA12345A") } + + it { is_expected.to be true } + end + + context "with a different claimant" do + let(:other_claim) { create(:claim, national_insurance_number: "BB12345B") } + + it { is_expected.to be false } + end + end end diff --git a/spec/models/decision_spec.rb b/spec/models/decision_spec.rb index 24adf8c05b..6b691dbfea 100644 --- a/spec/models/decision_spec.rb +++ b/spec/models/decision_spec.rb @@ -71,7 +71,7 @@ it "prevents a claim with matching bank details from being approved" do personal_details = { - eligibility_attributes: {teacher_reference_number: generate(:teacher_reference_number)}, + national_insurance_number: "QQ123456C", bank_sort_code: "112233" } diff --git a/spec/models/payment_spec.rb b/spec/models/payment_spec.rb index eedf7abd15..ce94dacec4 100644 --- a/spec/models/payment_spec.rb +++ b/spec/models/payment_spec.rb @@ -96,13 +96,6 @@ expect(subject).to be_valid end - it "is invalid when claims' teacher reference numbers do not match" do - claims[0].eligibility.teacher_reference_number = "9988776" - - expect(subject).not_to be_valid - expect(subject.errors[:claims]).to eq(["#{claims[0].reference} and #{claims[1].reference} have different values for teacher reference number"]) - end - it "is invalid when claims' dates of birth do not match" do claims[0].date_of_birth = 20.years.ago.to_date @@ -170,7 +163,7 @@ it "is valid when claims' National Insurance numbers do not match" do claims[0].national_insurance_number = "JM102019D" - expect(subject).to be_valid + expect(subject).not_to be_valid end end diff --git a/spec/models/payroll_run_spec.rb b/spec/models/payroll_run_spec.rb index ac6d7dd487..b343ea6373 100644 --- a/spec/models/payroll_run_spec.rb +++ b/spec/models/payroll_run_spec.rb @@ -98,8 +98,8 @@ build(:claim, :approved, eligibility_attributes: {student_loan_repayment_amount: 1000}) ]) payment_5 = build(:payment, claims: [ - build(:claim, :approved, policy: Policies::LevellingUpPremiumPayments, bank_sort_code: "123456", bank_account_number: "12345678", eligibility_attributes: {teacher_reference_number: "1234567"}), - build(:claim, :approved, bank_sort_code: "123456", bank_account_number: "12345678", eligibility_attributes: {student_loan_repayment_amount: 1000, teacher_reference_number: "1234567"}) + build(:claim, :approved, policy: Policies::LevellingUpPremiumPayments, bank_sort_code: "123456", bank_account_number: "12345678", national_insurance_number: "1234567"), + build(:claim, :approved, bank_sort_code: "123456", bank_account_number: "12345678", national_insurance_number: "1234567", eligibility_attributes: {student_loan_repayment_amount: 1000}) ]) payroll_run = PayrollRun.create!(created_by: user, payments: [payment_1, payment_2, payment_3, payment_4, payment_5]) From 09f14a933d321cc4cb67f74614f0d7927afce7ef Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Mon, 1 Jul 2024 13:57:40 +0100 Subject: [PATCH 02/14] Set claim for a specific policy We'll be wanting to have a sub class of the personal_data_scrubber for each policy. We're updating the tests to set the policy to make pulling out shared examples easier. --- .../claim/personal_data_scrubber_spec.rb | 59 +++++++++++++------ 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/spec/models/claim/personal_data_scrubber_spec.rb b/spec/models/claim/personal_data_scrubber_spec.rb index 88e72b4511..d815613cec 100644 --- a/spec/models/claim/personal_data_scrubber_spec.rb +++ b/spec/models/claim/personal_data_scrubber_spec.rb @@ -3,59 +3,84 @@ RSpec.describe Claim::PersonalDataScrubber, type: :model do subject(:personal_data_scrubber) { described_class.new.scrub_completed_claims } + let(:policy) { Policies::LevellingUpPremiumPayments } + let(:eligibility_factory) { "#{policy.to_s.underscore}_eligibility" } let(:user) { create(:dfe_signin_user) } let!(:journey_configuration) { create(:journey_configuration, :additional_payments) } let(:current_academic_year) { AcademicYear.current } let(:last_academic_year) { Time.zone.local(current_academic_year.start_year, 8, 1) } it "does not delete details from a submitted claim" do - claim = create(:claim, :submitted, updated_at: last_academic_year) + claim = create( + :claim, + :submitted, + policy: policy, + updated_at: last_academic_year + ) expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "does not delete details from a submitted but held claim" do - claim = create(:claim, :submitted, :held, updated_at: last_academic_year) + claim = create( + :claim, + :submitted, + :held, + policy: policy, + updated_at: last_academic_year + ) expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "does not delete details from a claim with an approval, but undone" do - claim = create(:claim, :submitted, updated_at: last_academic_year) + claim = create( + :claim, + :submitted, + policy: policy, + updated_at: last_academic_year + ) create(:decision, :approved, :undone, claim: claim) expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "does not delete details from an approved but unpaid claim" do - claim = create(:claim, :approved, updated_at: last_academic_year) + claim = create( + :claim, + :approved, + policy: policy, + updated_at: last_academic_year + ) expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "does not delete details from a newly rejected claim" do - claim = create(:claim, :rejected) + claim = create(:claim, :rejected, policy: policy) expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "does not delete details from a newly paid claim" do - claim = create(:claim, :approved) + claim = create(:claim, :approved, policy: policy) create(:payment, :confirmed, :with_figures, claims: [claim]) expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "does not delete details from a claim with a rejection which is old but undone" do - claim = create(:claim, :submitted) + claim = create(:claim, :submitted, policy: policy) create(:decision, :rejected, :undone, claim: claim, created_at: last_academic_year) expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "does not delete details from a claim that has a payment, but has a payrollable topup" do - lup_eligibility = create(:levelling_up_premium_payments_eligibility, :eligible, award_amount: 1500.0) - claim = create(:claim, :approved, policy: Policies::LevellingUpPremiumPayments, eligibility: lup_eligibility) + eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) + + claim = create(:claim, :approved, policy: policy, eligibility: eligibility) + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) create(:topup, payment: nil, claim: claim, award_amount: 500, created_by: user) @@ -66,8 +91,8 @@ claim = nil travel_to 2.months.ago do - lup_eligibility = create(:levelling_up_premium_payments_eligibility, :eligible, award_amount: 1500.0) - claim = create(:claim, :approved, policy: Policies::LevellingUpPremiumPayments, eligibility: lup_eligibility) + eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) + claim = create(:claim, :approved, policy: policy, eligibility: eligibility) create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) end @@ -81,8 +106,8 @@ claim = nil travel_to 2.months.ago do - lup_eligibility = create(:levelling_up_premium_payments_eligibility, :eligible, award_amount: 1500.0) - claim = create(:claim, :approved, policy: Policies::LevellingUpPremiumPayments, eligibility: lup_eligibility) + eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) + claim = create(:claim, :approved, policy: policy, eligibility: eligibility) create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) end @@ -94,7 +119,7 @@ it "deletes expected details from an old rejected claim, setting a personal_data_removed_at timestamp" do freeze_time do - claim = create(:claim, :submitted) + claim = create(:claim, :submitted, policy: policy) create(:decision, :rejected, claim: claim, created_at: last_academic_year) claim.update_attribute :hmrc_bank_validation_responses, ["test"] @@ -126,7 +151,7 @@ it "deletes expected details from an old paid claim, setting a personal_data_removed_at timestamp" do freeze_time do - claim = create(:claim, :approved) + claim = create(:claim, :approved, policy: policy) create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) claim.update_attribute :hmrc_bank_validation_responses, ["test"] @@ -160,7 +185,7 @@ # Initialise the scrubber, and create a claim scrubber = Claim::PersonalDataScrubber.new - claim = create(:claim, :submitted) + claim = create(:claim, :submitted, policy: policy) create(:decision, :rejected, claim: claim) travel_to(last_academic_year) do @@ -179,7 +204,7 @@ it "also deletes expected details from the scrubbed claims’ amendments, setting a personal_data_removed_at timestamp on the amendments" do claim, amendment = nil travel_to last_academic_year - 1.week do - claim = create(:claim, :submitted) + claim = create(:claim, :submitted, policy: policy) amendment = create(:amendment, claim: claim, claim_changes: { "teacher_reference_number" => [generate(:teacher_reference_number).to_s, claim.eligibility.teacher_reference_number], "payroll_gender" => ["male", claim.payroll_gender], From 0f32b52f11520765e41760d7eaa99658f1bcc778 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Mon, 1 Jul 2024 14:28:53 +0100 Subject: [PATCH 03/14] Move examples to shared examples Pulls the examples in this class into shared examples. We'll soon be introducing subclasses for this class so we'll want some shared examples --- .../claim/personal_data_scrubber_spec.rb | 240 +----------------- ..._personal_data_scrubber_shared_examples.rb | 239 +++++++++++++++++ 2 files changed, 243 insertions(+), 236 deletions(-) create mode 100644 spec/support/claim_personal_data_scrubber_shared_examples.rb diff --git a/spec/models/claim/personal_data_scrubber_spec.rb b/spec/models/claim/personal_data_scrubber_spec.rb index d815613cec..da2101dfc8 100644 --- a/spec/models/claim/personal_data_scrubber_spec.rb +++ b/spec/models/claim/personal_data_scrubber_spec.rb @@ -1,240 +1,8 @@ require "rails_helper" RSpec.describe Claim::PersonalDataScrubber, type: :model do - subject(:personal_data_scrubber) { described_class.new.scrub_completed_claims } - - let(:policy) { Policies::LevellingUpPremiumPayments } - let(:eligibility_factory) { "#{policy.to_s.underscore}_eligibility" } - let(:user) { create(:dfe_signin_user) } - let!(:journey_configuration) { create(:journey_configuration, :additional_payments) } - let(:current_academic_year) { AcademicYear.current } - let(:last_academic_year) { Time.zone.local(current_academic_year.start_year, 8, 1) } - - it "does not delete details from a submitted claim" do - claim = create( - :claim, - :submitted, - policy: policy, - updated_at: last_academic_year - ) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a submitted but held claim" do - claim = create( - :claim, - :submitted, - :held, - policy: policy, - updated_at: last_academic_year - ) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a claim with an approval, but undone" do - claim = create( - :claim, - :submitted, - policy: policy, - updated_at: last_academic_year - ) - create(:decision, :approved, :undone, claim: claim) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from an approved but unpaid claim" do - claim = create( - :claim, - :approved, - policy: policy, - updated_at: last_academic_year - ) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a newly rejected claim" do - claim = create(:claim, :rejected, policy: policy) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a newly paid claim" do - claim = create(:claim, :approved, policy: policy) - create(:payment, :confirmed, :with_figures, claims: [claim]) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a claim with a rejection which is old but undone" do - claim = create(:claim, :submitted, policy: policy) - create(:decision, :rejected, :undone, claim: claim, created_at: last_academic_year) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a claim that has a payment, but has a payrollable topup" do - eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) - - claim = create(:claim, :approved, policy: policy, eligibility: eligibility) - - create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - create(:topup, payment: nil, claim: claim, award_amount: 500, created_by: user) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a claim that has a payment, but has a payrolled topup without payment confirmation" do - claim = nil - - travel_to 2.months.ago do - eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) - claim = create(:claim, :approved, policy: policy, eligibility: eligibility) - create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - end - - payment2 = create(:payment, :with_figures, claims: [claim], scheduled_payment_date: nil) - create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "deletes expected details from a claim with multiple payments all of which have been confirmed" do - claim = nil - - travel_to 2.months.ago do - eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) - claim = create(:claim, :approved, policy: policy, eligibility: eligibility) - create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - end - - payment2 = create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) - - expect { personal_data_scrubber }.to change { claim.reload.attributes } - end - - it "deletes expected details from an old rejected claim, setting a personal_data_removed_at timestamp" do - freeze_time do - claim = create(:claim, :submitted, policy: policy) - create(:decision, :rejected, claim: claim, created_at: last_academic_year) - claim.update_attribute :hmrc_bank_validation_responses, ["test"] - - personal_data_scrubber - cleaned_claim = Claim.find(claim.id) - - expect(cleaned_claim.first_name).to be_nil - expect(cleaned_claim.middle_name).to be_nil - expect(cleaned_claim.surname).to be_nil - expect(cleaned_claim.date_of_birth).to be_nil - expect(cleaned_claim.address_line_1).to be_nil - expect(cleaned_claim.address_line_2).to be_nil - expect(cleaned_claim.address_line_3).to be_nil - expect(cleaned_claim.address_line_4).to be_nil - expect(cleaned_claim.postcode).to be_nil - expect(cleaned_claim.payroll_gender).to be_nil - expect(cleaned_claim.national_insurance_number).to be_nil - expect(cleaned_claim.bank_sort_code).to be_nil - expect(cleaned_claim.bank_account_number).to be_nil - expect(cleaned_claim.building_society_roll_number).to be_nil - expect(cleaned_claim.banking_name).to be_nil - expect(cleaned_claim.hmrc_bank_validation_responses).to be_nil - expect(cleaned_claim.mobile_number).to be_nil - expect(cleaned_claim.teacher_id_user_info).to be_nil - expect(cleaned_claim.dqt_teacher_status).to be_nil - expect(cleaned_claim.personal_data_removed_at).to eq(Time.zone.now) - end - end - - it "deletes expected details from an old paid claim, setting a personal_data_removed_at timestamp" do - freeze_time do - claim = create(:claim, :approved, policy: policy) - create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - claim.update_attribute :hmrc_bank_validation_responses, ["test"] - - personal_data_scrubber - cleaned_claim = Claim.find(claim.id) - - expect(cleaned_claim.first_name).to be_nil - expect(cleaned_claim.middle_name).to be_nil - expect(cleaned_claim.surname).to be_nil - expect(cleaned_claim.date_of_birth).to be_nil - expect(cleaned_claim.address_line_1).to be_nil - expect(cleaned_claim.address_line_2).to be_nil - expect(cleaned_claim.address_line_3).to be_nil - expect(cleaned_claim.address_line_4).to be_nil - expect(cleaned_claim.postcode).to be_nil - expect(cleaned_claim.payroll_gender).to be_nil - expect(cleaned_claim.national_insurance_number).to be_nil - expect(cleaned_claim.bank_sort_code).to be_nil - expect(cleaned_claim.bank_account_number).to be_nil - expect(cleaned_claim.building_society_roll_number).to be_nil - expect(cleaned_claim.banking_name).to be_nil - expect(cleaned_claim.hmrc_bank_validation_responses).to be_nil - expect(cleaned_claim.mobile_number).to be_nil - expect(cleaned_claim.teacher_id_user_info).to be_nil - expect(cleaned_claim.dqt_teacher_status).to be_nil - expect(cleaned_claim.personal_data_removed_at).to eq(Time.zone.now) - end - end - - it "only scrubs claims from the previous academic year" do - # Initialise the scrubber, and create a claim - scrubber = Claim::PersonalDataScrubber.new - - claim = create(:claim, :submitted, policy: policy) - create(:decision, :rejected, claim: claim) - - travel_to(last_academic_year) do - claim = create(:claim, :submitted) - create(:decision, :rejected, claim: claim) - end - - freeze_time do - scrubber.scrub_completed_claims - claims = Claim.order(created_at: :asc) - expect(claims.first.personal_data_removed_at).to eq(Time.zone.now) - expect(claims.last.personal_data_removed_at).to be_nil - end - end - - it "also deletes expected details from the scrubbed claims’ amendments, setting a personal_data_removed_at timestamp on the amendments" do - claim, amendment = nil - travel_to last_academic_year - 1.week do - claim = create(:claim, :submitted, policy: policy) - amendment = create(:amendment, claim: claim, claim_changes: { - "teacher_reference_number" => [generate(:teacher_reference_number).to_s, claim.eligibility.teacher_reference_number], - "payroll_gender" => ["male", claim.payroll_gender], - "date_of_birth" => [25.years.ago.to_date, claim.date_of_birth], - "student_loan_plan" => ["plan_1", claim.student_loan_plan], - "bank_sort_code" => ["457288", claim.bank_sort_code], - "bank_account_number" => ["84818482", claim.bank_account_number], - "building_society_roll_number" => ["123456789/ABCD", claim.building_society_roll_number] - }) - create(:decision, :approved, claim: claim, created_at: last_academic_year) - create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - end - - freeze_time do - original_trn_change = amendment.claim_changes["teacher_reference_number"] - - personal_data_scrubber - - cleaned_amendment = Amendment.find(amendment.id) - - expect(cleaned_amendment.claim_changes.keys).to match_array(%w[teacher_reference_number payroll_gender date_of_birth student_loan_plan bank_sort_code bank_account_number building_society_roll_number]) - expect(cleaned_amendment.notes).not_to be_nil - expect(cleaned_amendment.claim_changes["teacher_reference_number"]).to eq(original_trn_change) - expect(cleaned_amendment.claim_changes["date_of_birth"]).to eq(nil) - expect(cleaned_amendment.claim_changes["payroll_gender"]).to eq(nil) - expect(cleaned_amendment.claim_changes["bank_sort_code"]).to eq(nil) - expect(cleaned_amendment.claim_changes["bank_account_number"]).to eq(nil) - expect(cleaned_amendment.claim_changes["building_society_roll_number"]).to eq(nil) - - expect(cleaned_amendment.personal_data_removed_at).to eq(Time.zone.now) - end - end + it_behaves_like( + "a claim personal data scrubber", + Policies::LevellingUpPremiumPayments + ) end diff --git a/spec/support/claim_personal_data_scrubber_shared_examples.rb b/spec/support/claim_personal_data_scrubber_shared_examples.rb new file mode 100644 index 0000000000..c61348d2fb --- /dev/null +++ b/spec/support/claim_personal_data_scrubber_shared_examples.rb @@ -0,0 +1,239 @@ +require "rails_helper" + +RSpec.shared_examples "a claim personal data scrubber" do |policy| + subject(:personal_data_scrubber) { described_class.new.scrub_completed_claims } + + let(:eligibility_factory) { "#{policy.to_s.underscore}_eligibility" } + let(:user) { create(:dfe_signin_user) } + let!(:journey_configuration) { create(:journey_configuration, :additional_payments) } + let(:current_academic_year) { AcademicYear.current } + let(:last_academic_year) { Time.zone.local(current_academic_year.start_year, 8, 1) } + + it "does not delete details from a submitted claim" do + claim = create( + :claim, + :submitted, + policy: policy, + updated_at: last_academic_year + ) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a submitted but held claim" do + claim = create( + :claim, + :submitted, + :held, + policy: policy, + updated_at: last_academic_year + ) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a claim with an approval, but undone" do + claim = create( + :claim, + :submitted, + policy: policy, + updated_at: last_academic_year + ) + create(:decision, :approved, :undone, claim: claim) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from an approved but unpaid claim" do + claim = create( + :claim, + :approved, + policy: policy, + updated_at: last_academic_year + ) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a newly rejected claim" do + claim = create(:claim, :rejected, policy: policy) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a newly paid claim" do + claim = create(:claim, :approved, policy: policy) + create(:payment, :confirmed, :with_figures, claims: [claim]) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a claim with a rejection which is old but undone" do + claim = create(:claim, :submitted, policy: policy) + create(:decision, :rejected, :undone, claim: claim, created_at: last_academic_year) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a claim that has a payment, but has a payrollable topup" do + eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) + + claim = create(:claim, :approved, policy: policy, eligibility: eligibility) + + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + create(:topup, payment: nil, claim: claim, award_amount: 500, created_by: user) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a claim that has a payment, but has a payrolled topup without payment confirmation" do + claim = nil + + travel_to 2.months.ago do + eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) + claim = create(:claim, :approved, policy: policy, eligibility: eligibility) + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + end + + payment2 = create(:payment, :with_figures, claims: [claim], scheduled_payment_date: nil) + create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "deletes expected details from a claim with multiple payments all of which have been confirmed" do + claim = nil + + travel_to 2.months.ago do + eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) + claim = create(:claim, :approved, policy: policy, eligibility: eligibility) + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + end + + payment2 = create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) + + expect { personal_data_scrubber }.to change { claim.reload.attributes } + end + + it "deletes expected details from an old rejected claim, setting a personal_data_removed_at timestamp" do + freeze_time do + claim = create(:claim, :submitted, policy: policy) + create(:decision, :rejected, claim: claim, created_at: last_academic_year) + claim.update_attribute :hmrc_bank_validation_responses, ["test"] + + personal_data_scrubber + cleaned_claim = Claim.find(claim.id) + + expect(cleaned_claim.first_name).to be_nil + expect(cleaned_claim.middle_name).to be_nil + expect(cleaned_claim.surname).to be_nil + expect(cleaned_claim.date_of_birth).to be_nil + expect(cleaned_claim.address_line_1).to be_nil + expect(cleaned_claim.address_line_2).to be_nil + expect(cleaned_claim.address_line_3).to be_nil + expect(cleaned_claim.address_line_4).to be_nil + expect(cleaned_claim.postcode).to be_nil + expect(cleaned_claim.payroll_gender).to be_nil + expect(cleaned_claim.national_insurance_number).to be_nil + expect(cleaned_claim.bank_sort_code).to be_nil + expect(cleaned_claim.bank_account_number).to be_nil + expect(cleaned_claim.building_society_roll_number).to be_nil + expect(cleaned_claim.banking_name).to be_nil + expect(cleaned_claim.hmrc_bank_validation_responses).to be_nil + expect(cleaned_claim.mobile_number).to be_nil + expect(cleaned_claim.teacher_id_user_info).to be_nil + expect(cleaned_claim.dqt_teacher_status).to be_nil + expect(cleaned_claim.personal_data_removed_at).to eq(Time.zone.now) + end + end + + it "deletes expected details from an old paid claim, setting a personal_data_removed_at timestamp" do + freeze_time do + claim = create(:claim, :approved, policy: policy) + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + claim.update_attribute :hmrc_bank_validation_responses, ["test"] + + personal_data_scrubber + cleaned_claim = Claim.find(claim.id) + + expect(cleaned_claim.first_name).to be_nil + expect(cleaned_claim.middle_name).to be_nil + expect(cleaned_claim.surname).to be_nil + expect(cleaned_claim.date_of_birth).to be_nil + expect(cleaned_claim.address_line_1).to be_nil + expect(cleaned_claim.address_line_2).to be_nil + expect(cleaned_claim.address_line_3).to be_nil + expect(cleaned_claim.address_line_4).to be_nil + expect(cleaned_claim.postcode).to be_nil + expect(cleaned_claim.payroll_gender).to be_nil + expect(cleaned_claim.national_insurance_number).to be_nil + expect(cleaned_claim.bank_sort_code).to be_nil + expect(cleaned_claim.bank_account_number).to be_nil + expect(cleaned_claim.building_society_roll_number).to be_nil + expect(cleaned_claim.banking_name).to be_nil + expect(cleaned_claim.hmrc_bank_validation_responses).to be_nil + expect(cleaned_claim.mobile_number).to be_nil + expect(cleaned_claim.teacher_id_user_info).to be_nil + expect(cleaned_claim.dqt_teacher_status).to be_nil + expect(cleaned_claim.personal_data_removed_at).to eq(Time.zone.now) + end + end + + it "only scrubs claims from the previous academic year" do + # Initialise the scrubber, and create a claim + scrubber = Claim::PersonalDataScrubber.new + + claim = create(:claim, :submitted, policy: policy) + create(:decision, :rejected, claim: claim) + + travel_to(last_academic_year) do + claim = create(:claim, :submitted) + create(:decision, :rejected, claim: claim) + end + + freeze_time do + scrubber.scrub_completed_claims + claims = Claim.order(created_at: :asc) + expect(claims.first.personal_data_removed_at).to eq(Time.zone.now) + expect(claims.last.personal_data_removed_at).to be_nil + end + end + + it "also deletes expected details from the scrubbed claims’ amendments, setting a personal_data_removed_at timestamp on the amendments" do + claim, amendment = nil + travel_to last_academic_year - 1.week do + claim = create(:claim, :submitted, policy: policy) + amendment = create(:amendment, claim: claim, claim_changes: { + "teacher_reference_number" => [generate(:teacher_reference_number).to_s, claim.eligibility.teacher_reference_number], + "payroll_gender" => ["male", claim.payroll_gender], + "date_of_birth" => [25.years.ago.to_date, claim.date_of_birth], + "student_loan_plan" => ["plan_1", claim.student_loan_plan], + "bank_sort_code" => ["457288", claim.bank_sort_code], + "bank_account_number" => ["84818482", claim.bank_account_number], + "building_society_roll_number" => ["123456789/ABCD", claim.building_society_roll_number] + }) + create(:decision, :approved, claim: claim, created_at: last_academic_year) + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + end + + freeze_time do + original_trn_change = amendment.claim_changes["teacher_reference_number"] + + personal_data_scrubber + + cleaned_amendment = Amendment.find(amendment.id) + + expect(cleaned_amendment.claim_changes.keys).to match_array(%w[teacher_reference_number payroll_gender date_of_birth student_loan_plan bank_sort_code bank_account_number building_society_roll_number]) + expect(cleaned_amendment.notes).not_to be_nil + expect(cleaned_amendment.claim_changes["teacher_reference_number"]).to eq(original_trn_change) + expect(cleaned_amendment.claim_changes["date_of_birth"]).to eq(nil) + expect(cleaned_amendment.claim_changes["payroll_gender"]).to eq(nil) + expect(cleaned_amendment.claim_changes["bank_sort_code"]).to eq(nil) + expect(cleaned_amendment.claim_changes["bank_account_number"]).to eq(nil) + expect(cleaned_amendment.claim_changes["building_society_roll_number"]).to eq(nil) + + expect(cleaned_amendment.personal_data_removed_at).to eq(Time.zone.now) + end + end +end From 5561904813e21c4fd4456d033edb39a9c80472ff Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Tue, 2 Jul 2024 14:21:22 +0100 Subject: [PATCH 04/14] Extract policy specific claim data scrubber Different policies will have different rules around what data we need to retain. This commit adds policy specific subclasses for handling removing personal data. Subclasses can implement policy specific behaviour by redefining the PERSONAL_DATA_ATTRIBUTES_TO_DELETE constant or overwriting the `scrub_completed_claims` method. --- ...elete_personal_data_from_old_claims_job.rb | 4 +- app/models/claim/personal_data_scrubber.rb | 21 +++++-- .../claim_personal_data_scrubber.rb | 6 ++ .../claim_personal_data_scrubber.rb | 6 ++ .../claim_personal_data_scrubber.rb | 10 +++ .../claim_personal_data_scrubber.rb | 6 ++ .../claim_personal_data_scrubber.rb | 6 ++ config/analytics.yml | 1 + ...urther_education_payments_eligibilities.rb | 5 ++ db/schema.rb | 1 + .../eligibilities.rb | 6 ++ .../claim/personal_data_scrubber_spec.rb | 8 --- .../claim_personal_data_scrubber_spec.rb | 8 +++ .../claim_personal_data_scrubber_spec.rb | 8 +++ .../claim_personal_data_scrubber_spec.rb | 10 +++ .../claim_personal_data_scrubber_spec.rb | 40 ++++++++++++ .../claim_personal_data_scrubber_spec.rb | 8 +++ ..._personal_data_scrubber_shared_examples.rb | 61 +++++++++---------- 18 files changed, 169 insertions(+), 46 deletions(-) create mode 100644 app/models/policies/early_career_payments/claim_personal_data_scrubber.rb create mode 100644 app/models/policies/further_education_payments/claim_personal_data_scrubber.rb create mode 100644 app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb create mode 100644 app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb create mode 100644 app/models/policies/student_loans/claim_personal_data_scrubber.rb create mode 100644 db/migrate/20240702105328_add_award_amount_to_further_education_payments_eligibilities.rb create mode 100644 spec/factories/policies/further_education_payments/eligibilities.rb delete mode 100644 spec/models/claim/personal_data_scrubber_spec.rb create mode 100644 spec/models/policies/early_career_payments/claim_personal_data_scrubber_spec.rb create mode 100644 spec/models/policies/further_education_payments/claim_personal_data_scrubber_spec.rb create mode 100644 spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb create mode 100644 spec/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber_spec.rb create mode 100644 spec/models/policies/student_loans/claim_personal_data_scrubber_spec.rb diff --git a/app/jobs/delete_personal_data_from_old_claims_job.rb b/app/jobs/delete_personal_data_from_old_claims_job.rb index 13760e8d97..8f9fb6e87c 100644 --- a/app/jobs/delete_personal_data_from_old_claims_job.rb +++ b/app/jobs/delete_personal_data_from_old_claims_job.rb @@ -5,6 +5,8 @@ class DeletePersonalDataFromOldClaimsJob < CronJob def perform Rails.logger.info "Deleting personal data from old claims which have been rejected or paid" - Claim::PersonalDataScrubber.new.scrub_completed_claims + Policies::POLICIES.each do |policy| + policy::ClaimPersonalDataScrubber.new.scrub_completed_claims + end end end diff --git a/app/models/claim/personal_data_scrubber.rb b/app/models/claim/personal_data_scrubber.rb index 881fb7d7cc..06bac41b1f 100644 --- a/app/models/claim/personal_data_scrubber.rb +++ b/app/models/claim/personal_data_scrubber.rb @@ -3,6 +3,9 @@ # was scheduled to be paid more than two months ago. # # Attributes are set to nil, and personal_data_removed_at is set to the current timestamp. +# +# Inherit policy specifc data scrubbers from this class +# `app/models/policy/{some_policy}/claim_personal_data_scrubber.rb` class Claim class PersonalDataScrubber @@ -29,7 +32,7 @@ class PersonalDataScrubber ] def scrub_completed_claims - Claim.transaction do + ApplicationRecord.transaction do scrub_claims(old_rejected_claims) scrub_claims(old_paid_claims) end @@ -37,6 +40,14 @@ def scrub_completed_claims private + def policy + self.class.module_parent + end + + def claim_scope + Claim.by_policy(policy) + end + def scrub_claims(claims) claims.includes(:amendments).each do |claim| scrub_amendments_personal_data(claim) @@ -46,13 +57,13 @@ def scrub_claims(claims) end def attribute_values_to_set - PERSONAL_DATA_ATTRIBUTES_TO_DELETE.map { |attr| [attr, nil] }.to_h.merge( + self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE.map { |attr| [attr, nil] }.to_h.merge( personal_data_removed_at: Time.zone.now ) end def old_rejected_claims - Claim.joins(:decisions) + claim_scope.joins(:decisions) .where(personal_data_removed_at: nil) .where( "(decisions.undone = false AND decisions.result = :rejected AND decisions.created_at < :minimum_time)", @@ -65,7 +76,7 @@ def old_paid_claims claim_ids_with_payrollable_topups = Topup.payrollable.pluck(:claim_id) claim_ids_with_payrolled_topups_without_payment_confirmation = Topup.joins(payment: [:payroll_run]).where(payments: {scheduled_payment_date: nil}).pluck(:claim_id) - Claim.approved.joins(payments: [:payroll_run]) + claim_scope.approved.joins(payments: [:payroll_run]) .where(personal_data_removed_at: nil) .where.not(id: claim_ids_with_payrollable_topups + claim_ids_with_payrolled_topups_without_payment_confirmation) .where("payments.scheduled_payment_date < :minimum_time", minimum_time: minimum_time) @@ -86,7 +97,7 @@ def scrub_amendments_personal_data(claim) end def scrub_amendment_personal_data(amendment) - attributes_to_scrub = PERSONAL_DATA_ATTRIBUTES_TO_DELETE.map(&:to_s) & amendment.claim_changes.keys + attributes_to_scrub = self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE.map(&:to_s) & amendment.claim_changes.keys personal_data_mask = attributes_to_scrub.to_h { |attribute| [attribute, nil] } amendment.claim_changes.merge!(personal_data_mask) diff --git a/app/models/policies/early_career_payments/claim_personal_data_scrubber.rb b/app/models/policies/early_career_payments/claim_personal_data_scrubber.rb new file mode 100644 index 0000000000..8c26ee459c --- /dev/null +++ b/app/models/policies/early_career_payments/claim_personal_data_scrubber.rb @@ -0,0 +1,6 @@ +module Policies + module EarlyCareerPayments + class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + end + end +end diff --git a/app/models/policies/further_education_payments/claim_personal_data_scrubber.rb b/app/models/policies/further_education_payments/claim_personal_data_scrubber.rb new file mode 100644 index 0000000000..f6e974c135 --- /dev/null +++ b/app/models/policies/further_education_payments/claim_personal_data_scrubber.rb @@ -0,0 +1,6 @@ +module Policies + module FurtherEducationPayments + class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + end + end +end diff --git a/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb b/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb new file mode 100644 index 0000000000..f3c8978bd5 --- /dev/null +++ b/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb @@ -0,0 +1,10 @@ +module Policies + module InternationalRelocationPayments + class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + def scrub_completed_claims + # FIXME RL: temp NOOP until business decsion around whether TRN is + # required in the payment claim matching has been resolved + end + end + end +end diff --git a/app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb b/app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb new file mode 100644 index 0000000000..84dfc286c3 --- /dev/null +++ b/app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb @@ -0,0 +1,6 @@ +module Policies + module LevellingUpPremiumPayments + class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + end + end +end diff --git a/app/models/policies/student_loans/claim_personal_data_scrubber.rb b/app/models/policies/student_loans/claim_personal_data_scrubber.rb new file mode 100644 index 0000000000..5b50d9d32e --- /dev/null +++ b/app/models/policies/student_loans/claim_personal_data_scrubber.rb @@ -0,0 +1,6 @@ +module Policies + module StudentLoans + class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + end + end +end diff --git a/config/analytics.yml b/config/analytics.yml index a6c8dde7bf..4e821629d4 100644 --- a/config/analytics.yml +++ b/config/analytics.yml @@ -270,6 +270,7 @@ shared: - id - created_at - updated_at + - award_amount :eligible_fe_providers: - id - ukprn diff --git a/db/migrate/20240702105328_add_award_amount_to_further_education_payments_eligibilities.rb b/db/migrate/20240702105328_add_award_amount_to_further_education_payments_eligibilities.rb new file mode 100644 index 0000000000..0c1fd81385 --- /dev/null +++ b/db/migrate/20240702105328_add_award_amount_to_further_education_payments_eligibilities.rb @@ -0,0 +1,5 @@ +class AddAwardAmountToFurtherEducationPaymentsEligibilities < ActiveRecord::Migration[7.0] + def change + add_column :further_education_payments_eligibilities, :award_amount, :decimal, precision: 7, scale: 2 + end +end diff --git a/db/schema.rb b/db/schema.rb index c282dd5529..212fb79860 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -200,6 +200,7 @@ create_table "further_education_payments_eligibilities", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.decimal "award_amount", precision: 7, scale: 2 end create_table "international_relocation_payments_eligibilities", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/spec/factories/policies/further_education_payments/eligibilities.rb b/spec/factories/policies/further_education_payments/eligibilities.rb new file mode 100644 index 0000000000..5867f6554f --- /dev/null +++ b/spec/factories/policies/further_education_payments/eligibilities.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :further_education_payments_eligibility, class: "Policies::FurtherEducationPayments::Eligibility" do + trait :eligible do + end + end +end diff --git a/spec/models/claim/personal_data_scrubber_spec.rb b/spec/models/claim/personal_data_scrubber_spec.rb deleted file mode 100644 index da2101dfc8..0000000000 --- a/spec/models/claim/personal_data_scrubber_spec.rb +++ /dev/null @@ -1,8 +0,0 @@ -require "rails_helper" - -RSpec.describe Claim::PersonalDataScrubber, type: :model do - it_behaves_like( - "a claim personal data scrubber", - Policies::LevellingUpPremiumPayments - ) -end diff --git a/spec/models/policies/early_career_payments/claim_personal_data_scrubber_spec.rb b/spec/models/policies/early_career_payments/claim_personal_data_scrubber_spec.rb new file mode 100644 index 0000000000..50897dcda2 --- /dev/null +++ b/spec/models/policies/early_career_payments/claim_personal_data_scrubber_spec.rb @@ -0,0 +1,8 @@ +require "rails_helper" + +RSpec.describe Policies::EarlyCareerPayments::ClaimPersonalDataScrubber do + it_behaves_like( + "a claim personal data scrubber", + Policies::EarlyCareerPayments + ) +end diff --git a/spec/models/policies/further_education_payments/claim_personal_data_scrubber_spec.rb b/spec/models/policies/further_education_payments/claim_personal_data_scrubber_spec.rb new file mode 100644 index 0000000000..dbcb05b0d7 --- /dev/null +++ b/spec/models/policies/further_education_payments/claim_personal_data_scrubber_spec.rb @@ -0,0 +1,8 @@ +require "rails_helper" + +RSpec.describe Policies::FurtherEducationPayments::ClaimPersonalDataScrubber do + it_behaves_like( + "a claim personal data scrubber", + Policies::FurtherEducationPayments + ) +end diff --git a/spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb b/spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb new file mode 100644 index 0000000000..ed45b7fc64 --- /dev/null +++ b/spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb @@ -0,0 +1,10 @@ +require "rails_helper" + +RSpec.describe Policies::InternationalRelocationPayments::ClaimPersonalDataScrubber do + # FIXME RL: temp disabled until business decsion around whether TRN is + # required in the payment claim matching has been resolved + # it_behaves_like( + # "a claim personal data scrubber", + # Policies::InternationalRelocationPayments + # ) +end diff --git a/spec/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber_spec.rb b/spec/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber_spec.rb new file mode 100644 index 0000000000..e02f020f92 --- /dev/null +++ b/spec/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber_spec.rb @@ -0,0 +1,40 @@ +require "rails_helper" + +RSpec.describe Policies::LevellingUpPremiumPayments::ClaimPersonalDataScrubber do + it_behaves_like( + "a claim personal data scrubber", + Policies::LevellingUpPremiumPayments + ) + + subject(:personal_data_scrubber) { described_class.new.scrub_completed_claims } + let!(:journey_configuration) { create(:journey_configuration, :additional_payments) } + let(:current_academic_year) { AcademicYear.current } + let(:last_academic_year) { Time.zone.local(current_academic_year.start_year, 8, 1) } + let(:user) { create(:dfe_signin_user) } + + it "does not delete details from a claim that has a payment, but has a payrollable topup" do + eligibility = create(:levelling_up_premium_payments_eligibility, :eligible, award_amount: 1500.0) + + claim = create(:claim, :approved, policy: Policies::LevellingUpPremiumPayments, eligibility: eligibility) + + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + create(:topup, payment: nil, claim: claim, award_amount: 500, created_by: user) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end + + it "does not delete details from a claim that has a payment, but has a payrolled topup without payment confirmation" do + claim = nil + + travel_to 2.months.ago do + eligibility = create(:levelling_up_premium_payments_eligibility, :eligible, award_amount: 1500.0) + claim = create(:claim, :approved, policy: Policies::LevellingUpPremiumPayments, eligibility: eligibility) + create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) + end + + payment2 = create(:payment, :with_figures, claims: [claim], scheduled_payment_date: nil) + create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) + + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } + end +end diff --git a/spec/models/policies/student_loans/claim_personal_data_scrubber_spec.rb b/spec/models/policies/student_loans/claim_personal_data_scrubber_spec.rb new file mode 100644 index 0000000000..1bc1df2647 --- /dev/null +++ b/spec/models/policies/student_loans/claim_personal_data_scrubber_spec.rb @@ -0,0 +1,8 @@ +require "rails_helper" + +RSpec.describe Policies::StudentLoans::ClaimPersonalDataScrubber do + it_behaves_like( + "a claim personal data scrubber", + Policies::StudentLoans + ) +end diff --git a/spec/support/claim_personal_data_scrubber_shared_examples.rb b/spec/support/claim_personal_data_scrubber_shared_examples.rb index c61348d2fb..1f13a44a15 100644 --- a/spec/support/claim_personal_data_scrubber_shared_examples.rb +++ b/spec/support/claim_personal_data_scrubber_shared_examples.rb @@ -75,45 +75,37 @@ expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end - it "does not delete details from a claim that has a payment, but has a payrollable topup" do - eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) - - claim = create(:claim, :approved, policy: policy, eligibility: eligibility) - - create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - create(:topup, payment: nil, claim: claim, award_amount: 500, created_by: user) - - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end - - it "does not delete details from a claim that has a payment, but has a payrolled topup without payment confirmation" do + it "deletes expected details from a claim with multiple payments all of which have been confirmed" do claim = nil travel_to 2.months.ago do - eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) + eligibility = build(eligibility_factory, :eligible) + # Student loans don't have a settable award amount + if eligibility.has_attribute?(:award_amount) + eligibility.award_amount = 1500.0 + end + eligibility.save! claim = create(:claim, :approved, policy: policy, eligibility: eligibility) create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) end - payment2 = create(:payment, :with_figures, claims: [claim], scheduled_payment_date: nil) - create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) + payment2 = create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - expect { personal_data_scrubber }.not_to change { claim.reload.attributes } - end + if claim.topupable? + create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) + end - it "deletes expected details from a claim with multiple payments all of which have been confirmed" do - claim = nil + expect { personal_data_scrubber }.to change { claim.reload.attributes } + end - travel_to 2.months.ago do - eligibility = create(eligibility_factory, :eligible, award_amount: 1500.0) - claim = create(:claim, :approved, policy: policy, eligibility: eligibility) - create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - end + it "does not delete expected details from a claim for a different policy" do + other_policy = Policies::POLICIES.detect { |p| p != policy } - payment2 = create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) - create(:topup, payment: payment2, claim: claim, award_amount: 500, created_by: user) + claim = create(:claim, :submitted, policy: other_policy) + create(:decision, :rejected, claim: claim, created_at: last_academic_year) + claim.update_attribute :hmrc_bank_validation_responses, ["test"] - expect { personal_data_scrubber }.to change { claim.reload.attributes } + expect { personal_data_scrubber }.not_to change { claim.reload.attributes } end it "deletes expected details from an old rejected claim, setting a personal_data_removed_at timestamp" do @@ -182,13 +174,13 @@ it "only scrubs claims from the previous academic year" do # Initialise the scrubber, and create a claim - scrubber = Claim::PersonalDataScrubber.new + scrubber = described_class.new claim = create(:claim, :submitted, policy: policy) create(:decision, :rejected, claim: claim) travel_to(last_academic_year) do - claim = create(:claim, :submitted) + claim = create(:claim, :submitted, policy: policy) create(:decision, :rejected, claim: claim) end @@ -204,15 +196,20 @@ claim, amendment = nil travel_to last_academic_year - 1.week do claim = create(:claim, :submitted, policy: policy) - amendment = create(:amendment, claim: claim, claim_changes: { - "teacher_reference_number" => [generate(:teacher_reference_number).to_s, claim.eligibility.teacher_reference_number], + claim_changes = { "payroll_gender" => ["male", claim.payroll_gender], "date_of_birth" => [25.years.ago.to_date, claim.date_of_birth], "student_loan_plan" => ["plan_1", claim.student_loan_plan], "bank_sort_code" => ["457288", claim.bank_sort_code], "bank_account_number" => ["84818482", claim.bank_account_number], "building_society_roll_number" => ["123456789/ABCD", claim.building_society_roll_number] - }) + } + + if claim.eligibility.has_attribute?(:teacher_reference_number) + claim_changes["teacher_reference_number"] = [generate(:teacher_reference_number).to_s, claim.eligibility.teacher_reference_number] + end + + amendment = create(:amendment, claim: claim, claim_changes: claim_changes) create(:decision, :approved, claim: claim, created_at: last_academic_year) create(:payment, :confirmed, :with_figures, claims: [claim], scheduled_payment_date: last_academic_year) end From a97cbc2cadcc7cf0ec5302a8c14ae1d040b1c970 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Wed, 3 Jul 2024 15:17:05 +0100 Subject: [PATCH 05/14] Move base class to policy namespace Moves the `Claims::PersonalDataScrubber` into the `Policies` namespace as this class is only subclassed by specific policies. --- .../claim_personal_data_scrubber.rb} | 4 ++-- .../early_career_payments/claim_personal_data_scrubber.rb | 2 +- .../claim_personal_data_scrubber.rb | 2 +- .../claim_personal_data_scrubber.rb | 2 +- .../claim_personal_data_scrubber.rb | 2 +- .../policies/student_loans/claim_personal_data_scrubber.rb | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) rename app/models/{claim/personal_data_scrubber.rb => policies/claim_personal_data_scrubber.rb} (98%) diff --git a/app/models/claim/personal_data_scrubber.rb b/app/models/policies/claim_personal_data_scrubber.rb similarity index 98% rename from app/models/claim/personal_data_scrubber.rb rename to app/models/policies/claim_personal_data_scrubber.rb index 06bac41b1f..98a9a5bdb3 100644 --- a/app/models/claim/personal_data_scrubber.rb +++ b/app/models/policies/claim_personal_data_scrubber.rb @@ -7,8 +7,8 @@ # Inherit policy specifc data scrubbers from this class # `app/models/policy/{some_policy}/claim_personal_data_scrubber.rb` -class Claim - class PersonalDataScrubber +module Policies + class ClaimPersonalDataScrubber PERSONAL_DATA_ATTRIBUTES_TO_DELETE = [ :first_name, :middle_name, diff --git a/app/models/policies/early_career_payments/claim_personal_data_scrubber.rb b/app/models/policies/early_career_payments/claim_personal_data_scrubber.rb index 8c26ee459c..9371bc31d3 100644 --- a/app/models/policies/early_career_payments/claim_personal_data_scrubber.rb +++ b/app/models/policies/early_career_payments/claim_personal_data_scrubber.rb @@ -1,6 +1,6 @@ module Policies module EarlyCareerPayments - class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + class ClaimPersonalDataScrubber < Policies::ClaimPersonalDataScrubber end end end diff --git a/app/models/policies/further_education_payments/claim_personal_data_scrubber.rb b/app/models/policies/further_education_payments/claim_personal_data_scrubber.rb index f6e974c135..f45b5e6b6d 100644 --- a/app/models/policies/further_education_payments/claim_personal_data_scrubber.rb +++ b/app/models/policies/further_education_payments/claim_personal_data_scrubber.rb @@ -1,6 +1,6 @@ module Policies module FurtherEducationPayments - class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + class ClaimPersonalDataScrubber < Policies::ClaimPersonalDataScrubber end end end diff --git a/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb b/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb index f3c8978bd5..3200d4b1e0 100644 --- a/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb +++ b/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb @@ -1,6 +1,6 @@ module Policies module InternationalRelocationPayments - class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + class ClaimPersonalDataScrubber < Policies::ClaimPersonalDataScrubber def scrub_completed_claims # FIXME RL: temp NOOP until business decsion around whether TRN is # required in the payment claim matching has been resolved diff --git a/app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb b/app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb index 84dfc286c3..bf5d216a06 100644 --- a/app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb +++ b/app/models/policies/levelling_up_premium_payments/claim_personal_data_scrubber.rb @@ -1,6 +1,6 @@ module Policies module LevellingUpPremiumPayments - class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + class ClaimPersonalDataScrubber < Policies::ClaimPersonalDataScrubber end end end diff --git a/app/models/policies/student_loans/claim_personal_data_scrubber.rb b/app/models/policies/student_loans/claim_personal_data_scrubber.rb index 5b50d9d32e..893c595c49 100644 --- a/app/models/policies/student_loans/claim_personal_data_scrubber.rb +++ b/app/models/policies/student_loans/claim_personal_data_scrubber.rb @@ -1,6 +1,6 @@ module Policies module StudentLoans - class ClaimPersonalDataScrubber < Claim::PersonalDataScrubber + class ClaimPersonalDataScrubber < Policies::ClaimPersonalDataScrubber end end end From 5e5a0a72d546981a51b83c794faf3794b0c76c72 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Thu, 4 Jul 2024 16:49:23 +0100 Subject: [PATCH 06/14] Extract reject and paid claim methods We'll want to use different date ranges on these querys so we're seperating the claim scoping from the date filtering. --- .../policies/claim_personal_data_scrubber.rb | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/app/models/policies/claim_personal_data_scrubber.rb b/app/models/policies/claim_personal_data_scrubber.rb index 98a9a5bdb3..1c30ba54a0 100644 --- a/app/models/policies/claim_personal_data_scrubber.rb +++ b/app/models/policies/claim_personal_data_scrubber.rb @@ -63,23 +63,43 @@ def attribute_values_to_set end def old_rejected_claims + claims_rejected_before(minimum_time) + end + + def claims_rejected_before(date) + rejected_claims.where( + "decisions.created_at < :minimum_time", + minimum_time: date + ) + end + + def rejected_claims claim_scope.joins(:decisions) .where(personal_data_removed_at: nil) .where( - "(decisions.undone = false AND decisions.result = :rejected AND decisions.created_at < :minimum_time)", - minimum_time: minimum_time, + "(decisions.undone = false AND decisions.result = :rejected)", rejected: Decision.results.fetch(:rejected) ) end def old_paid_claims + claims_paid_before(minimum_time) + end + + def claims_paid_before(date) + paid_claims.where( + "payments.scheduled_payment_date < :minimum_time", + minimum_time: date + ) + end + + def paid_claims claim_ids_with_payrollable_topups = Topup.payrollable.pluck(:claim_id) claim_ids_with_payrolled_topups_without_payment_confirmation = Topup.joins(payment: [:payroll_run]).where(payments: {scheduled_payment_date: nil}).pluck(:claim_id) claim_scope.approved.joins(payments: [:payroll_run]) .where(personal_data_removed_at: nil) .where.not(id: claim_ids_with_payrollable_topups + claim_ids_with_payrolled_topups_without_payment_confirmation) - .where("payments.scheduled_payment_date < :minimum_time", minimum_time: minimum_time) end def minimum_time From ab326cae21be2a19fa685eb0b1e3e79bf3252ab0 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 5 Jul 2024 11:50:29 +0100 Subject: [PATCH 07/14] Extract claim scrubber Extracts the scrubbing logic from the claim_personal_data_scrubber into it's own class. Claim personal data scrubber was responsible for both finding the claims to scrub and scrubbing them. This commit moves the responsibility for scrubbing the data to a separate class as we're going to want to scrub different attributes depending on the policy we're scrubbing claims for. The claim data scrubber preformed and `update_all` when nullifying the attributes on the claim, we've preserved this behaviour of skipping callbacks / validations by using `update_column`. Previously we scrubbed all the claims inside a single transaction, this commit changes the behaviour slightly in that we open a transaction for each claim we scrub. This should slightly improve throughput (and makes sense based on how the code is called) but slightly changes the behaviour in that if there is an error when removing personal data from a claim previous claim's changes won't be rolled back. Likely not an issue but worth calling out. --- app/models/claim/scrubber.rb | 40 ++++++++++++ .../policies/claim_personal_data_scrubber.rb | 39 ++---------- spec/models/claim/scrubber_spec.rb | 63 +++++++++++++++++++ 3 files changed, 109 insertions(+), 33 deletions(-) create mode 100644 app/models/claim/scrubber.rb create mode 100644 spec/models/claim/scrubber_spec.rb diff --git a/app/models/claim/scrubber.rb b/app/models/claim/scrubber.rb new file mode 100644 index 0000000000..f13ff00d9b --- /dev/null +++ b/app/models/claim/scrubber.rb @@ -0,0 +1,40 @@ +# Removes attributes from a claim and its amendments +class Claim + class Scrubber + def self.scrub!(claim, attributes_to_delete) + new(claim, attributes_to_delete).scrub! + end + + attr_reader :claim, :attributes_to_delete + + def initialize(claim, attributes_to_delete) + @claim = claim + @attributes_to_delete = attributes_to_delete.map(&:to_s) + end + + def scrub! + ApplicationRecord.transaction do + claim.amendments.each { |amendment| scrub_amendment!(amendment) } + scrub_claim! + end + end + + private + + def scrub_amendment!(amendment) + amendment_data_to_scrub = attributes_to_delete & amendment.claim_changes.keys.map(&:to_s) + personal_data_mask = amendment_data_to_scrub.to_h { |attr| [attr, nil] } + amendment.claim_changes.merge!(personal_data_mask) + amendment.personal_data_removed_at = Time.zone.now + amendment.save! + end + + def scrub_claim! + personal_data_mask = attributes_to_delete.to_h { |attr| [attr, nil] } + attributes_to_set = personal_data_mask.merge( + personal_data_removed_at: Time.zone.now + ) + claim.update_columns(attributes_to_set) + end + end +end diff --git a/app/models/policies/claim_personal_data_scrubber.rb b/app/models/policies/claim_personal_data_scrubber.rb index 1c30ba54a0..4093aae922 100644 --- a/app/models/policies/claim_personal_data_scrubber.rb +++ b/app/models/policies/claim_personal_data_scrubber.rb @@ -32,9 +32,12 @@ class ClaimPersonalDataScrubber ] def scrub_completed_claims - ApplicationRecord.transaction do - scrub_claims(old_rejected_claims) - scrub_claims(old_paid_claims) + old_rejected_claims.includes(:amendments).each do |claim| + Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) + end + + old_paid_claims.includes(:amendments).each do |claim| + Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) end end @@ -48,20 +51,6 @@ def claim_scope Claim.by_policy(policy) end - def scrub_claims(claims) - claims.includes(:amendments).each do |claim| - scrub_amendments_personal_data(claim) - end - - claims.update_all(attribute_values_to_set) - end - - def attribute_values_to_set - self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE.map { |attr| [attr, nil] }.to_h.merge( - personal_data_removed_at: Time.zone.now - ) - end - def old_rejected_claims claims_rejected_before(minimum_time) end @@ -109,21 +98,5 @@ def minimum_time def current_academic_year AcademicYear.current end - - def scrub_amendments_personal_data(claim) - claim.amendments.each do |amendment| - scrub_amendment_personal_data(amendment) - end - end - - def scrub_amendment_personal_data(amendment) - attributes_to_scrub = self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE.map(&:to_s) & amendment.claim_changes.keys - personal_data_mask = attributes_to_scrub.to_h { |attribute| [attribute, nil] } - amendment.claim_changes.merge!(personal_data_mask) - - amendment.personal_data_removed_at = Time.zone.now - - amendment.save! - end end end diff --git a/spec/models/claim/scrubber_spec.rb b/spec/models/claim/scrubber_spec.rb new file mode 100644 index 0000000000..90fbf7e634 --- /dev/null +++ b/spec/models/claim/scrubber_spec.rb @@ -0,0 +1,63 @@ +require "rails_helper" + +RSpec.describe Claim::Scrubber do + describe ".scrub!" do + it "removes the specified attributes from the claim and its amendments" do + claim = create( + :claim, + :submitted, + first_name: "John", + surname: "Doe", + date_of_birth: Date.new(1980, 1, 1), + dqt_teacher_status: { + trn: 123456, + ni_number: "AB123123A" + } + ) + + amendment_1 = create( + :amendment, + claim: claim, + claim_changes: {"first_name" => ["John", "Jane"]} + ) + + amendment_2 = create( + :amendment, + claim: claim, + claim_changes: {"surname" => ["Doe", "Smith"]} + ) + + expect do + described_class.scrub!( + claim, + %i[first_name surname date_of_birth dqt_teacher_status] + ) + end.to( + change { claim.reload.first_name } + .from("John") + .to(nil) + .and( + change { claim.reload.surname } + .from("Doe") + .to(nil) + ).and( + change { claim.reload.date_of_birth } + .from(Date.new(1980, 1, 1)) + .to(nil) + ).and( + change { claim.reload.dqt_teacher_status } + .from({"trn" => 123456, "ni_number" => "AB123123A"}) + .to(nil) + ).and( + change { amendment_1.reload.claim_changes } + .from({"first_name" => ["John", "Jane"]}) + .to({"first_name" => nil}) + ).and( + change { amendment_2.reload.claim_changes } + .from({"surname" => ["Doe", "Smith"]}) + .to({"surname" => nil}) + ) + ) + end + end +end From 162b8edbcee23f2482b05e0e0b31ca3f671f7c1f Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 5 Jul 2024 13:46:11 +0100 Subject: [PATCH 08/14] Move personal data removed at filter up a level Moves the filtering of claims by `personal_data_removed_at` up a level. This will allow subclasses to use the querys to find rejected and paid claims again if they need to make a second pass through the data. --- app/models/policies/claim_personal_data_scrubber.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/policies/claim_personal_data_scrubber.rb b/app/models/policies/claim_personal_data_scrubber.rb index 4093aae922..04a1d6d72d 100644 --- a/app/models/policies/claim_personal_data_scrubber.rb +++ b/app/models/policies/claim_personal_data_scrubber.rb @@ -32,11 +32,15 @@ class ClaimPersonalDataScrubber ] def scrub_completed_claims - old_rejected_claims.includes(:amendments).each do |claim| + old_rejected_claims + .where(personal_data_removed_at: nil) + .includes(:amendments).each do |claim| Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) end - old_paid_claims.includes(:amendments).each do |claim| + old_paid_claims + .where(personal_data_removed_at: nil) + .includes(:amendments).each do |claim| Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) end end @@ -64,7 +68,6 @@ def claims_rejected_before(date) def rejected_claims claim_scope.joins(:decisions) - .where(personal_data_removed_at: nil) .where( "(decisions.undone = false AND decisions.result = :rejected)", rejected: Decision.results.fetch(:rejected) @@ -87,7 +90,6 @@ def paid_claims claim_ids_with_payrolled_topups_without_payment_confirmation = Topup.joins(payment: [:payroll_run]).where(payments: {scheduled_payment_date: nil}).pluck(:claim_id) claim_scope.approved.joins(payments: [:payroll_run]) - .where(personal_data_removed_at: nil) .where.not(id: claim_ids_with_payrollable_topups + claim_ids_with_payrolled_topups_without_payment_confirmation) end From 97bdcd432b5bd0e2924ab588f3c01a6b2ac5b4d3 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 5 Jul 2024 13:33:55 +0100 Subject: [PATCH 09/14] Retain NI and name on IRP claims for 2 years Updates the IRP claim scrubber to retain the national insurance number and name details for 2 years before scrubbing them too. When this method is called by the job we first call super to remove the not retained PII fields from the claim then we deal with scrubbing any retained PII claims. --- .../claim_personal_data_scrubber.rb | 55 +++++++- .../claim_personal_data_scrubber_spec.rb | 117 +++++++++++++++++- ..._personal_data_scrubber_shared_examples.rb | 64 ++++------ 3 files changed, 188 insertions(+), 48 deletions(-) diff --git a/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb b/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb index 3200d4b1e0..215b4cc915 100644 --- a/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb +++ b/app/models/policies/international_relocation_payments/claim_personal_data_scrubber.rb @@ -1,9 +1,60 @@ module Policies module InternationalRelocationPayments class ClaimPersonalDataScrubber < Policies::ClaimPersonalDataScrubber + PERSONAL_DATA_ATTRIBUTES_TO_DELETE = [ + :date_of_birth, + :address_line_1, + :address_line_2, + :address_line_3, + :address_line_4, + :postcode, + :payroll_gender, + :bank_sort_code, + :bank_account_number, + :building_society_roll_number, + :banking_name, + :hmrc_bank_validation_responses, + :mobile_number, + :teacher_id_user_info, + :dqt_teacher_status + ] + + PERSONAL_DATA_ATTRIBUTES_TO_RETAIN_FOR_EXTENDED_PERIOD = [ + :first_name, + :middle_name, + :surname, + :national_insurance_number + ] + + ANY_NON_NULL_EXTENDED_PERIOD_ATTRIBUTES = + PERSONAL_DATA_ATTRIBUTES_TO_RETAIN_FOR_EXTENDED_PERIOD.map do |attr| + "#{attr} IS NOT NULL" + end.join(" OR ") + def scrub_completed_claims - # FIXME RL: temp NOOP until business decsion around whether TRN is - # required in the payment claim matching has been resolved + super + + claims_rejected_before(extended_period_end_date).where( + ANY_NON_NULL_EXTENDED_PERIOD_ATTRIBUTES + ).each do |claim| + Claim::Scrubber.scrub!( + claim, + PERSONAL_DATA_ATTRIBUTES_TO_RETAIN_FOR_EXTENDED_PERIOD + ) + end + + claims_paid_before(extended_period_end_date).where( + ANY_NON_NULL_EXTENDED_PERIOD_ATTRIBUTES + ).each do |claim| + Claim::Scrubber.scrub!( + claim, + PERSONAL_DATA_ATTRIBUTES_TO_RETAIN_FOR_EXTENDED_PERIOD + ) + end + end + + def extended_period_end_date + minimum_time - 2.years end end end diff --git a/spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb b/spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb index ed45b7fc64..a0fb4e11ef 100644 --- a/spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb +++ b/spec/models/policies/international_relocation_payments/claim_personal_data_scrubber_spec.rb @@ -1,10 +1,115 @@ require "rails_helper" RSpec.describe Policies::InternationalRelocationPayments::ClaimPersonalDataScrubber do - # FIXME RL: temp disabled until business decsion around whether TRN is - # required in the payment claim matching has been resolved - # it_behaves_like( - # "a claim personal data scrubber", - # Policies::InternationalRelocationPayments - # ) + it_behaves_like( + "a claim personal data scrubber", + Policies::InternationalRelocationPayments + ) + + it "retains name and national insurance number for 2 years" do + last_academic_year = Time.zone.local(AcademicYear.current.start_year, 8, 1) + + approved_claim = create( + :claim, + :submitted, + policy: Policies::InternationalRelocationPayments, + national_insurance_number: "AB123456C", + first_name: "John", + middle_name: "James", + surname: "Doe" + ) + + create( + :decision, + :approved, + claim: approved_claim, + created_at: last_academic_year + ) + + create( + :payment, + :confirmed, + :with_figures, + claims: [approved_claim], + scheduled_payment_date: last_academic_year + ) + + rejected_claim = create( + :claim, + :submitted, + policy: Policies::InternationalRelocationPayments, + national_insurance_number: "AB123456C", + first_name: "John", + middle_name: "James", + surname: "Doe" + ) + + create( + :decision, + :rejected, + claim: rejected_claim, + created_at: last_academic_year + ) + + claim_expected_not_to_change = create( + :claim, + :submitted, + policy: Policies::InternationalRelocationPayments, + national_insurance_number: "AB123456D", + first_name: "Jane", + surname: "Smith" + ) + + create( + :decision, + :approved, + claim: claim_expected_not_to_change, + created_at: last_academic_year, + undone: true + ) + + expect { described_class.new.scrub_completed_claims }.to( + not_change { approved_claim.reload.first_name } + .and( + not_change { approved_claim.reload.middle_name } + ).and( + not_change { approved_claim.reload.surname } + ).and( + not_change { approved_claim.reload.national_insurance_number } + ).and( + not_change { rejected_claim.reload.first_name } + ).and( + not_change { rejected_claim.reload.surname } + ).and( + not_change { rejected_claim.reload.national_insurance_number } + ) + ) + + travel_to(AcademicYear.current.start_of_autumn_term + 2.years) do + expect { described_class.new.scrub_completed_claims }.to( + change { approved_claim.reload.first_name }.to(nil) + .and( + change { approved_claim.reload.middle_name }.to(nil) + ).and( + change { approved_claim.reload.surname }.to(nil) + ).and( + change { approved_claim.reload.national_insurance_number }.to(nil) + ).and( + change { rejected_claim.reload.first_name }.to(nil) + ).and( + change { rejected_claim.reload.surname }.to(nil) + ).and( + change { rejected_claim.reload.national_insurance_number }.to(nil) + ).and( + not_change { claim_expected_not_to_change.reload.first_name } + ).and( + not_change { claim_expected_not_to_change.reload.middle_name } + ).and( + not_change { claim_expected_not_to_change.reload.surname } + ).and( + not_change { claim_expected_not_to_change.reload.national_insurance_number } + ) + ) + end + end end diff --git a/spec/support/claim_personal_data_scrubber_shared_examples.rb b/spec/support/claim_personal_data_scrubber_shared_examples.rb index 1f13a44a15..fa3840a1a6 100644 --- a/spec/support/claim_personal_data_scrubber_shared_examples.rb +++ b/spec/support/claim_personal_data_scrubber_shared_examples.rb @@ -117,25 +117,10 @@ personal_data_scrubber cleaned_claim = Claim.find(claim.id) - expect(cleaned_claim.first_name).to be_nil - expect(cleaned_claim.middle_name).to be_nil - expect(cleaned_claim.surname).to be_nil - expect(cleaned_claim.date_of_birth).to be_nil - expect(cleaned_claim.address_line_1).to be_nil - expect(cleaned_claim.address_line_2).to be_nil - expect(cleaned_claim.address_line_3).to be_nil - expect(cleaned_claim.address_line_4).to be_nil - expect(cleaned_claim.postcode).to be_nil - expect(cleaned_claim.payroll_gender).to be_nil - expect(cleaned_claim.national_insurance_number).to be_nil - expect(cleaned_claim.bank_sort_code).to be_nil - expect(cleaned_claim.bank_account_number).to be_nil - expect(cleaned_claim.building_society_roll_number).to be_nil - expect(cleaned_claim.banking_name).to be_nil - expect(cleaned_claim.hmrc_bank_validation_responses).to be_nil - expect(cleaned_claim.mobile_number).to be_nil - expect(cleaned_claim.teacher_id_user_info).to be_nil - expect(cleaned_claim.dqt_teacher_status).to be_nil + described_class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE.each do |attribute| + expect(cleaned_claim.public_send(attribute)).to be_nil + end + expect(cleaned_claim.personal_data_removed_at).to eq(Time.zone.now) end end @@ -149,25 +134,9 @@ personal_data_scrubber cleaned_claim = Claim.find(claim.id) - expect(cleaned_claim.first_name).to be_nil - expect(cleaned_claim.middle_name).to be_nil - expect(cleaned_claim.surname).to be_nil - expect(cleaned_claim.date_of_birth).to be_nil - expect(cleaned_claim.address_line_1).to be_nil - expect(cleaned_claim.address_line_2).to be_nil - expect(cleaned_claim.address_line_3).to be_nil - expect(cleaned_claim.address_line_4).to be_nil - expect(cleaned_claim.postcode).to be_nil - expect(cleaned_claim.payroll_gender).to be_nil - expect(cleaned_claim.national_insurance_number).to be_nil - expect(cleaned_claim.bank_sort_code).to be_nil - expect(cleaned_claim.bank_account_number).to be_nil - expect(cleaned_claim.building_society_roll_number).to be_nil - expect(cleaned_claim.banking_name).to be_nil - expect(cleaned_claim.hmrc_bank_validation_responses).to be_nil - expect(cleaned_claim.mobile_number).to be_nil - expect(cleaned_claim.teacher_id_user_info).to be_nil - expect(cleaned_claim.dqt_teacher_status).to be_nil + described_class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE.each do |attribute| + expect(cleaned_claim.public_send(attribute)).to be_nil + end expect(cleaned_claim.personal_data_removed_at).to eq(Time.zone.now) end end @@ -221,9 +190,24 @@ cleaned_amendment = Amendment.find(amendment.id) - expect(cleaned_amendment.claim_changes.keys).to match_array(%w[teacher_reference_number payroll_gender date_of_birth student_loan_plan bank_sort_code bank_account_number building_society_roll_number]) + expected_claim_changed_attributes = %w[ + payroll_gender + date_of_birth + student_loan_plan + bank_sort_code + bank_account_number + building_society_roll_number + ] + + if claim.eligibility.has_attribute?(:teacher_reference_number) + expected_claim_changed_attributes << "teacher_reference_number" + end + + expect(cleaned_amendment.claim_changes.keys).to match_array(expected_claim_changed_attributes) expect(cleaned_amendment.notes).not_to be_nil - expect(cleaned_amendment.claim_changes["teacher_reference_number"]).to eq(original_trn_change) + if claim.eligibility.has_attribute?(:teacher_reference_number) + expect(cleaned_amendment.claim_changes["teacher_reference_number"]).to eq(original_trn_change) + end expect(cleaned_amendment.claim_changes["date_of_birth"]).to eq(nil) expect(cleaned_amendment.claim_changes["payroll_gender"]).to eq(nil) expect(cleaned_amendment.claim_changes["bank_sort_code"]).to eq(nil) From 06640534dd20c5a1acfda0b15d2ba584138f9166 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 5 Jul 2024 14:59:52 +0100 Subject: [PATCH 10/14] Scrub PII from journey session Once we're at the point of removing PII from the claim we can safely drop any attributes stored in the journey session. We may want to consider purging the session content when the claim form is submitted instead. --- app/models/claim/scrubber.rb | 7 ++ .../policies/claim_personal_data_scrubber.rb | 4 +- .../further_education_payments_answers.rb | 4 ++ ...et_a_teacher_relocation_payment_answers.rb | 12 ++++ ..._personal_data_scrubber_shared_examples.rb | 69 +++++++++++++++++++ 5 files changed, 94 insertions(+), 2 deletions(-) diff --git a/app/models/claim/scrubber.rb b/app/models/claim/scrubber.rb index f13ff00d9b..bf9c0480bb 100644 --- a/app/models/claim/scrubber.rb +++ b/app/models/claim/scrubber.rb @@ -16,6 +16,7 @@ def scrub! ApplicationRecord.transaction do claim.amendments.each { |amendment| scrub_amendment!(amendment) } scrub_claim! + scrub_session! end end @@ -36,5 +37,11 @@ def scrub_claim! ) claim.update_columns(attributes_to_set) end + + def scrub_session! + return unless claim.journey_session + + claim.journey_session.update!(answers: {}) + end end end diff --git a/app/models/policies/claim_personal_data_scrubber.rb b/app/models/policies/claim_personal_data_scrubber.rb index 04a1d6d72d..2210870a4a 100644 --- a/app/models/policies/claim_personal_data_scrubber.rb +++ b/app/models/policies/claim_personal_data_scrubber.rb @@ -34,13 +34,13 @@ class ClaimPersonalDataScrubber def scrub_completed_claims old_rejected_claims .where(personal_data_removed_at: nil) - .includes(:amendments).each do |claim| + .includes(:amendments, :journey_session).each do |claim| Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) end old_paid_claims .where(personal_data_removed_at: nil) - .includes(:amendments).each do |claim| + .includes(:amendments, :journey_session).each do |claim| Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) end end diff --git a/spec/factories/journeys/further_education_payments/further_education_payments_answers.rb b/spec/factories/journeys/further_education_payments/further_education_payments_answers.rb index 84bdf96210..f9e13c415b 100644 --- a/spec/factories/journeys/further_education_payments/further_education_payments_answers.rb +++ b/spec/factories/journeys/further_education_payments/further_education_payments_answers.rb @@ -5,5 +5,9 @@ surname { "Bloggs" } onelogin_user_info { {email: "jo.bloggs@example.com"} } end + + trait :submittable do + # FIXME implement this trait with the details required to submit a claim + end end end diff --git a/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb b/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb index 648e80e2ef..38411a027a 100644 --- a/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb +++ b/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb @@ -80,9 +80,21 @@ with_current_school with_headteacher_details with_one_year_contract + with_subject with_start_date with_visa with_entry_date end + + trait :submittable do + eligible_teacher + with_personal_details + with_nationality + with_passport_number + with_email_details + with_mobile_details + with_bank_details + with_employment_details + end end end diff --git a/spec/support/claim_personal_data_scrubber_shared_examples.rb b/spec/support/claim_personal_data_scrubber_shared_examples.rb index fa3840a1a6..b2b7833161 100644 --- a/spec/support/claim_personal_data_scrubber_shared_examples.rb +++ b/spec/support/claim_personal_data_scrubber_shared_examples.rb @@ -217,4 +217,73 @@ expect(cleaned_amendment.personal_data_removed_at).to eq(Time.zone.now) end end + + it "removes personal details from the journey session too" do + journey = Journeys.for_policy(policy) + session_name = journey::I18N_NAMESPACE + + session_for_approved_claim = create( + "#{session_name}_session", + answers: build( + "#{session_name}_answers", + :submittable + ) + ) + + session_for_rejected_claim = create( + "#{session_name}_session", + answers: build( + "#{session_name}_answers", + :submittable + ) + ) + + approved_claim = create( + :claim, + :submitted, + policy: policy, + journey_session: session_for_approved_claim + ) + + create( + :decision, + :approved, + claim: approved_claim, + created_at: last_academic_year + ) + + create( + :payment, + :confirmed, + :with_figures, + claims: [approved_claim], + scheduled_payment_date: last_academic_year + ) + + rejected_claim = create( + :claim, + :submitted, + policy: policy, + journey_session: session_for_rejected_claim + ) + + create( + :decision, + :rejected, + claim: rejected_claim, + created_at: last_academic_year + ) + + personal_data_scrubber + + described_class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE.each do |attribute| + expect( + session_for_approved_claim.reload.answers.public_send(attribute) + ).to be_blank + + expect( + session_for_rejected_claim.reload.answers.public_send(attribute) + ).to be_blank + end + end end From 903f1506e00fea54b201a759233557961e771aa0 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 5 Jul 2024 15:06:33 +0100 Subject: [PATCH 11/14] Update spec to check all policies We want to make sure each policy is covered by this spec to ensure a claim personal data scrubber is created for any new policies we add. --- ..._personal_data_from_old_claims_job_spec.rb | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/spec/jobs/delete_personal_data_from_old_claims_job_spec.rb b/spec/jobs/delete_personal_data_from_old_claims_job_spec.rb index 9139876a3c..dc6a6f044e 100644 --- a/spec/jobs/delete_personal_data_from_old_claims_job_spec.rb +++ b/spec/jobs/delete_personal_data_from_old_claims_job_spec.rb @@ -5,18 +5,20 @@ let(:current_academic_year) { AcademicYear.current } let(:last_academic_year) { Time.zone.local(current_academic_year.start_year, 8, 1) } - it "deletes the personal data from eligible claims" do - submitted_claim = create(:claim, :submitted) - rejected_claim = create(:claim, :submitted) - create(:decision, :rejected, claim: rejected_claim, created_at: last_academic_year) - paid_claim = create(:claim, :approved) - create(:payment, :confirmed, :with_figures, claims: [paid_claim], scheduled_payment_date: last_academic_year) + Policies::POLICIES.each do |policy| + it "deletes the personal data from eligible #{policy} claims" do + submitted_claim = create(:claim, :submitted, policy: policy) + rejected_claim = create(:claim, :submitted, policy: policy) + create(:decision, :rejected, claim: rejected_claim, created_at: last_academic_year) + paid_claim = create(:claim, :approved, policy: policy) + create(:payment, :confirmed, :with_figures, claims: [paid_claim], scheduled_payment_date: last_academic_year) - DeletePersonalDataFromOldClaimsJob.new.perform + DeletePersonalDataFromOldClaimsJob.new.perform - expect(Claim.find(submitted_claim.id).personal_data_removed_at).to be_nil - expect(Claim.find(rejected_claim.id).personal_data_removed_at).to_not be_nil - expect(Claim.find(paid_claim.id).personal_data_removed_at).to_not be_nil + expect(Claim.find(submitted_claim.id).personal_data_removed_at).to be_nil + expect(Claim.find(rejected_claim.id).personal_data_removed_at).to_not be_nil + expect(Claim.find(paid_claim.id).personal_data_removed_at).to_not be_nil + end end end end From 579725c81c258a77203d0e457ecac825b6396458 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Wed, 24 Jul 2024 12:05:56 +0100 Subject: [PATCH 12/14] Remove with_employment_details Looks like this was left over from when we split up the head teacher question --- .../get_a_teacher_relocation_payment_answers.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb b/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb index 38411a027a..f9e1acda27 100644 --- a/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb +++ b/spec/factories/journeys/get_a_teacher_relocation_payment/get_a_teacher_relocation_payment_answers.rb @@ -94,7 +94,6 @@ with_email_details with_mobile_details with_bank_details - with_employment_details end end end From 86705f2ee9137755d3562f926f9cc3076c932e0c Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Wed, 24 Jul 2024 12:11:30 +0100 Subject: [PATCH 13/14] Extract named scope PR feedback suggestion from Steve --- app/models/claim.rb | 1 + app/models/policies/claim_personal_data_scrubber.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/claim.rb b/app/models/claim.rb index 0869d7738c..ca057f8702 100644 --- a/app/models/claim.rb +++ b/app/models/claim.rb @@ -180,6 +180,7 @@ class Claim < ApplicationRecord scope :unassigned, -> { where(assigned_to_id: nil) } scope :current_academic_year, -> { by_academic_year(AcademicYear.current) } scope :failed_bank_validation, -> { where(hmrc_bank_validation_succeeded: false) } + scope :unscrubbed, -> { where(personal_data_removed_at: nil) } scope :with_same_claimant, ->(claim) do CLAIMANT_MATCHING_ATTRIBUTES.reduce(where.not(id: claim.id)) do |scope, attr| diff --git a/app/models/policies/claim_personal_data_scrubber.rb b/app/models/policies/claim_personal_data_scrubber.rb index 2210870a4a..0548cf38dc 100644 --- a/app/models/policies/claim_personal_data_scrubber.rb +++ b/app/models/policies/claim_personal_data_scrubber.rb @@ -33,13 +33,13 @@ class ClaimPersonalDataScrubber def scrub_completed_claims old_rejected_claims - .where(personal_data_removed_at: nil) + .unscrubbed .includes(:amendments, :journey_session).each do |claim| Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) end old_paid_claims - .where(personal_data_removed_at: nil) + .unscrubbed .includes(:amendments, :journey_session).each do |claim| Claim::Scrubber.scrub!(claim, self.class::PERSONAL_DATA_ATTRIBUTES_TO_DELETE) end From f96cb7976c8356bd4fce722d98226556ae8a05e8 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Wed, 24 Jul 2024 13:20:32 +0100 Subject: [PATCH 14/14] Only normalise attributes if present This lets us call save when scrubbing attributes, rather than using `update_columns` which bypasses dfe analytics --- app/models/claim.rb | 10 +++++----- app/models/claim/scrubber.rb | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/claim.rb b/app/models/claim.rb index ca057f8702..613d77937a 100644 --- a/app/models/claim.rb +++ b/app/models/claim.rb @@ -149,11 +149,11 @@ class Claim < ApplicationRecord validate :building_society_roll_number_must_be_between_one_and_eighteen_digits validate :building_society_roll_number_must_be_in_a_valid_format - before_save :normalise_ni_number, if: :national_insurance_number_changed? - before_save :normalise_bank_account_number, if: :bank_account_number_changed? - before_save :normalise_bank_sort_code, if: :bank_sort_code_changed? - before_save :normalise_first_name, if: :first_name_changed? - before_save :normalise_surname, if: :surname_changed? + before_save :normalise_ni_number, if: %i[national_insurance_number national_insurance_number_changed?] + before_save :normalise_bank_account_number, if: %i[bank_account_number bank_account_number_changed?] + before_save :normalise_bank_sort_code, if: %i[bank_sort_code bank_sort_code_changed?] + before_save :normalise_first_name, if: %i[first_name first_name_changed?] + before_save :normalise_surname, if: %i[surname surname_changed?] scope :unsubmitted, -> { where(submitted_at: nil) } scope :submitted, -> { where.not(submitted_at: nil) } diff --git a/app/models/claim/scrubber.rb b/app/models/claim/scrubber.rb index bf9c0480bb..dc1dd4f8b5 100644 --- a/app/models/claim/scrubber.rb +++ b/app/models/claim/scrubber.rb @@ -35,7 +35,7 @@ def scrub_claim! attributes_to_set = personal_data_mask.merge( personal_data_removed_at: Time.zone.now ) - claim.update_columns(attributes_to_set) + claim.update!(attributes_to_set) end def scrub_session!