Merge branch 'master' into 97-add_endpoint_for_avg_number_of_days

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(

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.

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

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

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

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

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)

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

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)