Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache hearings related fields selectively #15577

Merged
merged 23 commits into from
Nov 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .reek.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ detectors:
- AssignHearingDispositionTask#reschedule_or_schedule_later
- AssignJudgeteamRoles#process
- AsyncableJobsReporter
- CachedAppealService#ama_appeal_hearing_fields_to_cache
- CachedAppealService#cache_ama_appeals
- CachedAppealService#cache_legacy_appeal_postgres_data
- CachedAppealService#cache_legacy_appeal_vacols_data
- CachedAppealService#legacy_postgres_hearing_fields_to_cache
- CachedAppealService#poa_representative_name_for
- CachedAppealService#case_fields_for_vacols_ids
- ConvertToVirtualHearing#convert_hearing_to_virtual
Expand Down
2 changes: 1 addition & 1 deletion app/jobs/hearings/geomatch_and_cache_appeal_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def perform(appeal_id:, appeal_type:)
if appeal.closest_regional_office.present?
cached_appeal_service = CachedAppealService.new
cached_appeal_service.cache_legacy_appeal_postgres_data([appeal])
cached_appeal_service.cache_legacy_appeal_vacols_data([appeal.vacols_id])
cached_appeal_service.cache_legacy_appeal_vacols_data([appeal])
end
end

Expand Down
14 changes: 8 additions & 6 deletions app/jobs/update_cached_appeals_attributes_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ def perform
end

def cache_ama_appeals
appeals = Appeal.includes(:available_hearing_locations).where(id: open_appeals_from_tasks(Appeal.name))
appeals = Appeal.includes(:available_hearing_locations)
.where(id: open_appeals_from_tasks(Appeal.name))
.order(id: :desc) # cache most created appeals first

cached_appeals = cached_appeal_service.cache_ama_appeals(appeals)

Expand All @@ -47,14 +49,14 @@ def cache_legacy_appeals
# was previously causing this code to insert legacy appeal attributes that corresponded to NULL ID fields.
legacy_appeals = LegacyAppeal.includes(:available_hearing_locations)
.where(id: open_appeals_from_tasks(LegacyAppeal.name))
all_vacols_ids = legacy_appeals.pluck(:vacols_id).flatten
.order(id: :desc) # cache most created appeals first
yoomlam marked this conversation as resolved.
Show resolved Hide resolved

cache_postgres_data_start = Time.zone.now
cache_legacy_appeal_postgres_data(legacy_appeals)
datadog_report_time_segment(segment: "cache_legacy_appeal_postgres_data", start_time: cache_postgres_data_start)

cache_vacols_data_start = Time.zone.now
cache_legacy_appeal_vacols_data(all_vacols_ids)
cache_legacy_appeal_vacols_data(legacy_appeals)
datadog_report_time_segment(segment: "cache_legacy_appeal_vacols_data", start_time: cache_vacols_data_start)
end

Expand All @@ -67,9 +69,9 @@ def cache_legacy_appeal_postgres_data(legacy_appeals)
end
end

def cache_legacy_appeal_vacols_data(all_vacols_ids)
all_vacols_ids.in_groups_of(VACOLS_BATCH_SIZE, false).each do |batch_vacols_ids|
cached_appeals = cached_appeal_service.cache_legacy_appeal_vacols_data(batch_vacols_ids)
def cache_legacy_appeal_vacols_data(legacy_appeals)
legacy_appeals.in_groups_of(VACOLS_BATCH_SIZE, false).each do |batch_legacy_appeals|
cached_appeals = cached_appeal_service.cache_legacy_appeal_vacols_data(batch_legacy_appeals)

increment_vacols_update_count(cached_appeals.count)
end
Expand Down
100 changes: 76 additions & 24 deletions app/services/cached_appeal_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def cache_ama_appeals(appeals)

appeal_aod_status = aod_status_for_appeals(appeals)
representative_names = representative_names_for_appeals(appeals)
appeal_ids_with_schedule_hearing_tasks = appeal_ids_with_schedule_hearing_tasks(appeals.pluck(:id), Appeal.name)

appeals.map do |appeal|
{
Expand All @@ -18,12 +19,11 @@ def cache_ama_appeals(appeals)
issue_count: request_issues_to_cache[appeal.id] || 0,
docket_type: appeal.docket_type,
docket_number: appeal.docket_number,
hearing_request_type: appeal.current_hearing_request_type(readable: true),
is_aod: appeal_aod_status.include?(appeal.id),
power_of_attorney_name: representative_names[appeal.id],
suggested_hearing_location: appeal.suggested_hearing_location&.formatted_location,
veteran_name: veteran_names_to_cache[appeal.veteran_file_number]
}.merge(
ama_appeal_hearing_fields_to_cache(appeal, representative_names, appeal_ids_with_schedule_hearing_tasks)
).merge(
regional_office_fields_to_cache(appeal)
)
end
Expand All @@ -33,26 +33,41 @@ def cache_ama_appeals(appeals)

def cache_legacy_appeal_postgres_data(legacy_appeals)
import_cached_appeals([:appeal_id, :appeal_type], POSTGRES_LEGACY_CACHED_COLUMNS) do
appeal_ids_with_schedule_hearing_tasks = appeal_ids_with_schedule_hearing_tasks(
legacy_appeals.pluck(:id),
LegacyAppeal.name
)

legacy_appeals.map do |appeal|
{
vacols_id: appeal.vacols_id,
appeal_id: appeal.id,
appeal_type: LegacyAppeal.name,
docket_type: appeal.docket_name, # "legacy"
power_of_attorney_name: poa_representative_name_for(appeal),
suggested_hearing_location: appeal.suggested_hearing_location&.formatted_location
docket_type: appeal.docket_name # "legacy"
}.merge(
legacy_postgres_hearing_fields_to_cache(appeal, appeal_ids_with_schedule_hearing_tasks)
).merge(
regional_office_fields_to_cache(appeal)
)
end
end
end

# rubocop:disable Metrics/MethodLength
def cache_legacy_appeal_vacols_data(vacols_ids)
# rubocop:disable Metrics/AbcSize
def cache_legacy_appeal_vacols_data(legacy_appeals)
import_cached_appeals([:vacols_id], VACOLS_CACHED_COLUMNS) do
vacols_id_to_appeal_id_mapping = legacy_appeals.pluck(:vacols_id, :id).to_h
vacols_ids = vacols_id_to_appeal_id_mapping.keys
appeal_ids_with_schedule_hearing_tasks = appeal_ids_with_schedule_hearing_tasks(
vacols_id_to_appeal_id_mapping.values,
LegacyAppeal.name
)

vacols_folders = folder_fields_for_vacols_ids(vacols_ids)
vacols_cases = case_fields_for_vacols_ids(vacols_ids)
vacols_cases = case_fields_for_vacols_ids(
vacols_ids, appeal_ids_with_schedule_hearing_tasks, vacols_id_to_appeal_id_mapping
)

issue_counts_to_cache = issues_counts_for_vacols_folders(vacols_ids)
aod_status_to_cache = VACOLS::Case.aod(vacols_ids)
Expand All @@ -62,7 +77,6 @@ def cache_legacy_appeal_vacols_data(vacols_ids)

vacols_folders.map do |folder|
vacols_case = vacols_cases[folder[:vacols_id]]

{
vacols_id: folder[:vacols_id],
case_type: vacols_case[:status],
Expand All @@ -71,11 +85,13 @@ def cache_legacy_appeal_vacols_data(vacols_ids)
hearing_request_type: vacols_case[:hearing_request_type],
issue_count: issue_counts_to_cache[folder[:vacols_id]] || 0,
is_aod: aod_status_to_cache[folder[:vacols_id]],
power_of_attorney_name: vacols_case[:power_of_attorney_name],
veteran_name: veteran_names_to_cache[folder[:correspondent_id]]
}
end
end
end
# rubocop:enable Metrics/AbcSize
# rubocop:enable Metrics/MethodLength

def warning_msgs
Expand Down Expand Up @@ -140,12 +156,11 @@ def import_cached_appeals(conflict_columns, columns)
end

# bypass PowerOfAttorney model completely and always prefer BGS cache
def poa_representative_name_for(appeal)
bgs_poa = fetch_bgs_power_of_attorney_by_file_number(appeal.veteran_file_number)
# both representative_name calls can result in BGS connection error
bgs_poa&.representative_name || appeal.representative_name
yoomlam marked this conversation as resolved.
Show resolved Hide resolved
def poa_representative_name_for(veteran_file_number, appeal_id)
bgs_poa = fetch_bgs_power_of_attorney_by_file_number(veteran_file_number)
bgs_poa&.representative_name
rescue Errno::ECONNRESET, Savon::HTTPError => error
warning_msgs << "#{appeal.class.name} #{appeal.id}: #{error}" if warning_msgs.count < 100
warning_msgs << "LegacyAppeal #{appeal_id}: #{error}" if warning_msgs.count < 100
nil
end

Expand Down Expand Up @@ -199,10 +214,10 @@ def representative_names_for_appeals(appeals)
).pluck("claimants.decision_review_id, bgs_power_of_attorneys.representative_name").to_h
end

def case_fields_for_vacols_ids(vacols_ids)
def case_fields_for_vacols_ids(vacols_ids, appeal_ids_with_schedule_hearing_tasks, vacols_id_to_appeal_id_mapping)
# array of arrays will become hash with bfkey as key.
# [
# [ 123, { location: 57, status: "Original", hearing_request_type: "Video", former_travel: true} ],
# [ 123,{ location: 57, status: "Original", hearing_request_type: "Video", former_travel: true} ],
# [ 456, { location: 2, status: "Court Remand", hearing_request_type: "Video", former_travel: true } ],
# ...
# ]
Expand All @@ -213,19 +228,17 @@ def case_fields_for_vacols_ids(vacols_ids)
# ...
# }
VACOLS::Case.where(bfkey: vacols_ids).map do |vacols_case|
original_request = original_hearing_request_type_for_vacols_case(vacols_case)
changed_request = changed_hearing_request_type_for_all_vacols_ids[vacols_case.bfkey]
# Replicates LegacyAppeal#current_hearing_request_type
current_request = HearingDay::REQUEST_TYPES.key(changed_request)&.to_sym || original_request
appeal_id = vacols_id_to_appeal_id_mapping[vacols_case.bfkey]
appeal_has_schedule_hearing_task = appeal_ids_with_schedule_hearing_tasks.include?(appeal_id)

[
vacols_case.bfkey,
{
location: vacols_case.bfcurloc,
status: VACOLS::Case::TYPES[vacols_case.bfac],
hearing_request_type: LegacyAppeal::READABLE_HEARING_REQUEST_TYPES[current_request],
former_travel: original_request == :travel_board && current_request != :travel_board
}
status: VACOLS::Case::TYPES[vacols_case.bfac]
}.merge(
appeal_has_schedule_hearing_task ? vacols_hearing_fields_to_cache(vacols_case, appeal_id) : {}
)
]
end.to_h
end
Expand Down Expand Up @@ -272,4 +285,43 @@ def veteran_names_for_correspondent_ids(correspondent_ids)
[corr.stafkey, "#{corr.snamel.split(' ').last}, #{corr.snamef}"]
end.to_h
end

# get all ids of appeals which have open ScheduleHearingTasks
def appeal_ids_with_schedule_hearing_tasks(appeal_ids, appeal_type)
ScheduleHearingTask.open
.where(appeal_id: appeal_ids, appeal_type: appeal_type)
.pluck(:appeal_id)
.uniq
end

def ama_appeal_hearing_fields_to_cache(appeal, representative_names, appeal_ids_with_schedule_hearing_tasks)
return {} if !appeal_ids_with_schedule_hearing_tasks.include?(appeal.id)

{
hearing_request_type: appeal.current_hearing_request_type(readable: true),
power_of_attorney_name: representative_names[appeal.id],
suggested_hearing_location: appeal.suggested_hearing_location&.formatted_location
}
end

def legacy_postgres_hearing_fields_to_cache(appeal, appeal_ids_with_schedule_hearing_tasks)
return {} if !appeal_ids_with_schedule_hearing_tasks.include?(appeal.id)

{ suggested_hearing_location: appeal.suggested_hearing_location&.formatted_location }
end

def vacols_hearing_fields_to_cache(vacols_case, appeal_id)
original_request = original_hearing_request_type_for_vacols_case(vacols_case)
changed_request = changed_hearing_request_type_for_all_vacols_ids[vacols_case.bfkey]
# Replicates LegacyAppeal#current_hearing_request_type
current_request = HearingDay::REQUEST_TYPES.key(changed_request)&.to_sym || original_request

veteran_file_number = LegacyAppeal.veteran_file_number_from_bfcorlid(vacols_case.bfcorlid)

{
hearing_request_type: LegacyAppeal::READABLE_HEARING_REQUEST_TYPES[current_request],
former_travel: original_request == :travel_board && current_request != :travel_board,
power_of_attorney_name: poa_representative_name_for(veteran_file_number, appeal_id)
}
end
end
2 changes: 2 additions & 0 deletions spec/jobs/hearings/geomatch_and_cache_appeal_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

context "with AMA appeal" do
let(:appeal) { create(:appeal) }
let!(:schedule_hearing_task) { create(:schedule_hearing_task, appeal: appeal) }

it "throws an error" do
expect { subject }.to raise_error ArgumentError
Expand All @@ -25,6 +26,7 @@
vacols_case: vacols_case
)
end
let!(:schedule_hearing_task) { create(:schedule_hearing_task, appeal: appeal) }

context "when closest regional office is not set" do
it "sets the closest regional office field" do
Expand Down
7 changes: 3 additions & 4 deletions spec/jobs/update_cached_appeals_attributes_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@
end
end

context "caches hearing_request_type and former_travel correctly" do
context "caches hearings related field correctly" do
let(:appeal) { create(:appeal, closest_regional_office: "C") } # central
let(:legacy_appeal3) do # former travel, currently virtual
create(
Expand All @@ -118,8 +118,7 @@

before do
open_appeals.each do |appeal|
create_list(:bva_dispatch_task, 3, appeal: appeal)
create_list(:ama_judge_assign_task, 8, appeal: appeal)
create_list(:schedule_hearing_task, 1, appeal: appeal)
end
end

Expand Down Expand Up @@ -172,7 +171,7 @@
before do
bgs = Fakes::BGSService.new
allow(Fakes::BGSService).to receive(:new).and_return(bgs)
allow(bgs).to receive(:fetch_person_by_ssn)
yoomlam marked this conversation as resolved.
Show resolved Hide resolved
allow(bgs).to receive(:fetch_poa_by_file_number)
.and_raise(Errno::ECONNRESET, "mocked error for testing")
end
include_examples "rescues error"
Expand Down
36 changes: 33 additions & 3 deletions spec/services/cached_appeal_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,18 @@
changed_request_type: HearingDay::REQUEST_TYPES[:virtual]
)
end
let!(:schedule_hearing_task) { create(:schedule_hearing_task, appeal: legacy_appeal) }

it "caches hearing_request_type correctly", :aggregate_failures do
subject.cache_legacy_appeal_postgres_data([legacy_appeal])
subject.cache_legacy_appeal_vacols_data([vacols_case.bfkey])
subject.cache_legacy_appeal_vacols_data([legacy_appeal])

expect(CachedAppeal.find_by(vacols_id: legacy_appeal.vacols_id).hearing_request_type).to eq("Virtual")
end

it "caches former_travel correctly", :aggregate_failures do
subject.cache_legacy_appeal_postgres_data([legacy_appeal])
subject.cache_legacy_appeal_vacols_data([vacols_case.bfkey])
subject.cache_legacy_appeal_vacols_data([legacy_appeal])

expect(CachedAppeal.find_by(vacols_id: legacy_appeal.vacols_id).former_travel).to eq(true)
end
Expand Down Expand Up @@ -53,7 +54,7 @@
it "does not update appeals that were recently cached" do
subject.cache_ama_appeals([ama_appeal])
subject.cache_legacy_appeal_postgres_data([legacy_appeal])
subject.cache_legacy_appeal_vacols_data([vacols_case.bfkey])
subject.cache_legacy_appeal_vacols_data([legacy_appeal])

legacy_cached_appeal = CachedAppeal.find_by(appeal_id: legacy_appeal.id, appeal_type: LegacyAppeal.name)
ama_cached_appeal = CachedAppeal.find_by(appeal_id: ama_appeal.id, appeal_type: Appeal.name)
Expand All @@ -63,4 +64,33 @@
expect(ama_cached_appeal.updated_at.utc).to be_within(1.in_milliseconds).of(future_time)
end
end

context "in the absence of an open ScheduleHearingTask" do
context "ama appeal" do
let(:appeal) { create(:appeal) }

it "does not cache hearing related fields", :aggregate_failures do
yoomlam marked this conversation as resolved.
Show resolved Hide resolved
subject.cache_ama_appeals([appeal])

expect(CachedAppeal.find_by(appeal_id: appeal.id).hearing_request_type).to eq(nil)
expect(CachedAppeal.find_by(appeal_id: appeal.id).power_of_attorney_name).to eq(nil)
expect(CachedAppeal.find_by(appeal_id: appeal.id).suggested_hearing_location).to eq(nil)
end
end

context "legacy appeal" do
let(:vacols_case) { create(:case) }
let(:legacy_appeal) { create(:legacy_appeal, vacols_case: vacols_case) }

it "does not cache hearing realated fields", :aggregate_failures do
subject.cache_legacy_appeal_postgres_data([legacy_appeal])
subject.cache_legacy_appeal_vacols_data([legacy_appeal])

expect(CachedAppeal.find_by(vacols_id: legacy_appeal.vacols_id).hearing_request_type).to eq(nil)
expect(CachedAppeal.find_by(vacols_id: legacy_appeal.vacols_id).former_travel).to eq(nil)
expect(CachedAppeal.find_by(vacols_id: legacy_appeal.vacols_id).power_of_attorney_name).to eq(nil)
expect(CachedAppeal.find_by(vacols_id: legacy_appeal.vacols_id).suggested_hearing_location).to eq(nil)
end
end
end
end