diff --git a/app/sidekiq/terms_of_use/sign_up_service_updater_job.rb b/app/sidekiq/terms_of_use/sign_up_service_updater_job.rb index 307797272e8..d0edfc38f8e 100644 --- a/app/sidekiq/terms_of_use/sign_up_service_updater_job.rb +++ b/app/sidekiq/terms_of_use/sign_up_service_updater_job.rb @@ -7,8 +7,7 @@ module TermsOfUse class SignUpServiceUpdaterJob include Sidekiq::Job - sidekiq_options unique_for: 2.days - sidekiq_options retry: 15 # 2.1 days using exponential backoff + sidekiq_options retry: 5 # ~17 mins sidekiq_retries_exhausted do |job, exception| Rails.logger.warn( diff --git a/config/features.yml b/config/features.yml index 01a1dcc3390..d103dda9ffa 100644 --- a/config/features.yml +++ b/config/features.yml @@ -320,6 +320,9 @@ features: decision_review_delay_evidence: actor_type: user description: Ensures that NOD and SC evidence is not received in Central Mail before the appeal itself + decision_review_updated_pii_rules: + actor_type: user + description: Uses udpated rules for when to clear PII from appeals_api records dependency_verification: actor_type: user description: Feature gates the dependency verification modal for updating the diaries service. diff --git a/modules/appeals_api/app/models/appeals_api/higher_level_review.rb b/modules/appeals_api/app/models/appeals_api/higher_level_review.rb index 0a6251cda7a..e718556a414 100644 --- a/modules/appeals_api/app/models/appeals_api/higher_level_review.rb +++ b/modules/appeals_api/app/models/appeals_api/higher_level_review.rb @@ -5,9 +5,11 @@ module AppealsApi class HigherLevelReview < ApplicationRecord + include AppealScopes include HlrStatus include PdfOutputPrep include ModelValidations + required_claimant_headers %w[ X-VA-NonVeteranClaimant-First-Name X-VA-NonVeteranClaimant-Last-Name diff --git a/modules/appeals_api/app/models/appeals_api/notice_of_disagreement.rb b/modules/appeals_api/app/models/appeals_api/notice_of_disagreement.rb index 31a07d91ec8..a5fbe41e75f 100644 --- a/modules/appeals_api/app/models/appeals_api/notice_of_disagreement.rb +++ b/modules/appeals_api/app/models/appeals_api/notice_of_disagreement.rb @@ -5,6 +5,7 @@ module AppealsApi class NoticeOfDisagreement < ApplicationRecord + include AppealScopes include NodStatus include PdfOutputPrep include ModelValidations diff --git a/modules/appeals_api/app/models/appeals_api/supplemental_claim.rb b/modules/appeals_api/app/models/appeals_api/supplemental_claim.rb index 2bfafbf9bae..d994814374a 100644 --- a/modules/appeals_api/app/models/appeals_api/supplemental_claim.rb +++ b/modules/appeals_api/app/models/appeals_api/supplemental_claim.rb @@ -4,9 +4,11 @@ module AppealsApi class SupplementalClaim < ApplicationRecord + include AppealScopes include ScStatus include PdfOutputPrep include ModelValidations + required_claimant_headers %w[X-VA-NonVeteranClaimant-First-Name X-VA-NonVeteranClaimant-Last-Name] attr_readonly :auth_headers diff --git a/modules/appeals_api/app/models/concerns/appeals_api/appeal_scopes.rb b/modules/appeals_api/app/models/concerns/appeals_api/appeal_scopes.rb new file mode 100644 index 00000000000..ddc4860526d --- /dev/null +++ b/modules/appeals_api/app/models/concerns/appeals_api/appeal_scopes.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module AppealsApi + module AppealScopes + extend ActiveSupport::Concern + + included do + scope :without_status_updates_since, lambda { |time| + status_update_table = AppealsApi::StatusUpdate.table_name + join_clause = <<~JOIN + LEFT JOIN #{status_update_table} + ON #{table_name}.id = CAST(#{status_update_table}.statusable_id as uuid) + AND #{status_update_table}.id IS NULL + AND #{status_update_table}.statusable_type = '#{sanitize_sql(klass.name)}' + JOIN + where("#{table_name}.updated_at <= ?", time) + .where.not(id: joins(join_clause).where("#{status_update_table}.created_at >= ?", time).distinct.pluck(:id)) + } + + scope :with_pii, -> { where.not(form_data_ciphertext: nil).or(where.not(auth_headers_ciphertext: nil)) } + + scope :with_expired_pii, lambda { + # PII should be removed if... + # (1) appeal is in any state and the status last changed 45+ days ago, or... + with_pii.without_status_updates_since(45.days.ago) + # (2) appeal is in 'complete' or 'success' status and status last changed 7+ days ago, or... + .or(with_pii.where(table_name => { status: %w[complete success] }) + .without_status_updates_since(7.days.ago)) + # (3) appeal has 'Unidentified Mail' error and status last changed 7+ days ago. + .or(with_pii.where(table_name => { status: 'error' }) + .where(klass.arel_table[:detail].matches('%%Unidentified Mail%%')) + .without_status_updates_since(7.days.ago)) + } + end + end +end diff --git a/modules/appeals_api/app/services/appeals_api/remove_pii.rb b/modules/appeals_api/app/services/appeals_api/remove_pii.rb index 0828cb63f4a..39fd91d8673 100644 --- a/modules/appeals_api/app/services/appeals_api/remove_pii.rb +++ b/modules/appeals_api/app/services/appeals_api/remove_pii.rb @@ -41,13 +41,13 @@ def valid_form_type? end def records_to_be_expunged - @records_to_be_expunged ||= - form_type.where.not(form_data_ciphertext: nil) - .or( - form_type.where.not( - auth_headers_ciphertext: nil - ) - ).pii_expunge_policy + @records_to_be_expunged ||= if Flipper.enabled?(:decision_review_updated_pii_rules) + form_type.with_expired_pii + else + form_type.where.not(form_data_ciphertext: nil) + .or(form_type.where.not(auth_headers_ciphertext: nil)) + .pii_expunge_policy + end end def records_were_not_cleared(result) diff --git a/modules/appeals_api/spec/services/appeals_api/remove_pii_spec.rb b/modules/appeals_api/spec/services/appeals_api/remove_pii_spec.rb index 659808ce983..8599a4838f5 100644 --- a/modules/appeals_api/spec/services/appeals_api/remove_pii_spec.rb +++ b/modules/appeals_api/spec/services/appeals_api/remove_pii_spec.rb @@ -3,8 +3,158 @@ require 'rails_helper' require AppealsApi::Engine.root.join('spec', 'spec_helper.rb') +def update_appeal_status(appeal, status, code: nil, detail: nil) + # At the time of writing, the `update_status` method for each appeal model involves kicking off a sidekiq job to + # create a matching StatusUpdate record. This is unwieldy in tests, so this method approximates the + # `appeal.update_status!` method without involving sidekiq. + appeal.update!(status:, code:, detail:) + + AppealsApi::StatusUpdate.create!( + from: appeal.status, + to: status, + status_update_time: Time.zone.now, + statusable_id: appeal.id, + statusable_type: appeal.class.name, + code:, + detail: + ) + + appeal +end + +shared_examples 'removes expired PII' do + let(:now) { Time.zone.now } + let(:code) { 'DOC202' } + let(:detail) { "Upstream status: #{VBADocuments::UploadSubmission::ERROR_UNIDENTIFIED_MAIL}" } + let(:misc_appeal_types) do + %i[minimal_notice_of_disagreement extra_notice_of_disagreement_v2 extra_notice_of_disagreement_v0 + higher_level_review_v1 extra_higher_level_review_v2 minimal_higher_level_review_v0 + extra_supplemental_claim minimal_supplemental_claim_v0] + end + + def create_appeals = [FactoryBot.create(v0_factory), FactoryBot.create(v2_factory)] + + def create_misc_appeals = create_appeals + misc_appeal_types.map { |f| FactoryBot.create(f) } + + let!(:unexpired_appeals) do # These should all be ignored and remain unchanged + appeals = [] + + # These otherwise meet removal criteria (1, 2, 3) but are not old enough + Timecop.freeze(now) do + appeals += create_misc_appeals + appeals += create_misc_appeals.map { |appeal| update_appeal_status(appeal, 'complete') } + appeals += create_misc_appeals.map { |appeal| update_appeal_status(appeal, 'success') } + appeals += create_misc_appeals.map { |appeal| update_appeal_status(appeal, 'error', code:, detail:) } + end + + # These are old enough to meet removal criteria around status updates (1) but will be disqualified by having more + # recent updates to the model + oldest_appeals = [] + Timecop.freeze(now - 45.days) do + oldest_appeals += create_misc_appeals.map { |appeal| update_appeal_status(appeal, 'processing') } + end + Timecop.freeze(now - 30.days) { oldest_appeals.map { |appeal| appeal.update(updated_at: Time.zone.now) } } + appeals += oldest_appeals + + # These are old enough to meet removal criteria (2, 3), but... + Timecop.freeze(now - 7.days) do + appeals += create_misc_appeals # (2) they are not in a 'success' or 'complete' state + appeals += create_misc_appeals.map do |appeal| # (3) they have an error other than "Unidentified Mail" + update_appeal_status(appeal, 'error', code: 'DOC104', detail: 'Other error') + end + end + + appeals + end + + let!(:expired_oldest_appeals) do + Timecop.freeze(now - 45.days) do + create_appeals + # These should be selected even though there are no status updates + create_appeals.map { |appeal| update_appeal_status(appeal, 'submitted') } + end + end + + let!(:expired_errored_appeals) do + Timecop.freeze(now - 7.days) do + create_appeals.map { |appeal| update_appeal_status(appeal, 'error', code:, detail:) } + end + end + + let!(:expired_done_appeals) do + Timecop.freeze(now - 7.days) do + successes = create_appeals.map { |appeal| update_appeal_status(appeal, 'success') } + completes = create_appeals.map { |appeal| update_appeal_status(appeal, 'complete') } + successes + completes + end + end + + let(:expired_appeals) { expired_oldest_appeals + expired_errored_appeals + expired_done_appeals } + + before { AppealsApi::RemovePii.new(form_type:).run! } + + it 'does not remove unexpired PII' do + unexpired_appeals.each do |appeal| + appeal.reload + expect(appeal.form_data).to be_present + expect(appeal.auth_headers).to be_present if appeal.api_version == 'V2' + end + end + + it 'removes oldest expired PII' do + expired_oldest_appeals.each do |appeal| + appeal.reload + expect(appeal.form_data).to be_blank + expect(appeal.auth_headers).to be_blank + end + end + + it 'removes old complete/success PII' do + expired_done_appeals.each do |appeal| + appeal.reload + expect(appeal.form_data).to be_blank + expect(appeal.auth_headers).to be_blank + end + end + + it 'removes old Unidentified Mail errored PII' do + expired_errored_appeals.each do |appeal| + appeal.reload + expect(appeal.form_data).to be_blank + expect(appeal.auth_headers).to be_blank + end + end +end + describe AppealsApi::RemovePii do + describe '#run! with new PII rules' do + context 'with Higher-Level Review' do + let(:v2_factory) { :higher_level_review_v2 } + let(:v0_factory) { :higher_level_review_v0 } + let(:form_type) { AppealsApi::HigherLevelReview } + + include_examples 'removes expired PII' + end + + context 'with Supplemental Claim' do + let(:v2_factory) { :supplemental_claim } + let(:v0_factory) { :supplemental_claim_v0 } + let(:form_type) { AppealsApi::SupplementalClaim } + + include_examples 'removes expired PII' + end + + context 'with Notice of Disagreement' do + let(:v2_factory) { :notice_of_disagreement_v2 } + let(:v0_factory) { :notice_of_disagreement_v0 } + let(:form_type) { AppealsApi::NoticeOfDisagreement } + + include_examples 'removes expired PII' + end + end + describe '#run!' do + before { Flipper.disable :decision_review_updated_pii_rules } + it 'raises an ArgumentError if an invalid form type is supplied' do expect { AppealsApi::RemovePii.new(form_type: 'Invalid').run! }.to raise_error(ArgumentError) end diff --git a/spec/sidekiq/terms_of_use/sign_up_service_updater_job_spec.rb b/spec/sidekiq/terms_of_use/sign_up_service_updater_job_spec.rb index 65086fbcbc0..25a6fbd12bc 100644 --- a/spec/sidekiq/terms_of_use/sign_up_service_updater_job_spec.rb +++ b/spec/sidekiq/terms_of_use/sign_up_service_updater_job_spec.rb @@ -23,8 +23,8 @@ allow(Sidekiq::AttrPackage).to receive(:delete) end - it 'retries 15 times after failure' do - expect(described_class.get_sidekiq_options['retry']).to eq(15) + it 'retries 5 times after failure' do + expect(described_class.get_sidekiq_options['retry']).to eq(5) end it 'logs a message when retries have been exhausted' do @@ -45,8 +45,6 @@ ) end - it { is_expected.to be_unique } - context 'when the terms of use agreement is accepted' do before do allow(service_instance).to receive(:agreements_accept)