Cache hearings related fields selectively

[rubaiyat22]

May help with #15417
Resolves #15580

Description

• cache all appeals in a determined order (descending order of Ids) which may prevent deadlock if two instances of job run at the same time
• only cache some hearing related expensive fields for appeal if there exists at least one open ScheduleHearingTask

breakdown of appeals with open ScheduleHearingTask vs without (PROD)

as = Task.open.where(appeal_type: Appeal.name).pluck(:appeal_id).uniq
as.count
=> 56919
Task.open.where(appeal_id: as, type: ScheduleHearingTask.name, appeal_type: "Appeal").pluck(:appeal_id).uniq.count
=> 32620 # 57% -24299 difference

las = Task.open.where(appeal_type: LegacyAppeal.name).pluck(:appeal_id).uniq
las.count
=> 209664 
Task.open.where(appeal_id: las, type: ScheduleHearingTask.name, appeal_type: "LegacyAppeal").pluck(:appeal_id).uniq.count
=> 45036 21% -164,628 # we'd be avoiding making calls to BGS for 164,628 appeals

[va-bot]

	1 Warning
⚠️	This PR appears to affect one or more integrations. Make sure to test code in UAT before merging. See these instructions for deploying a custom branch to UAT.

Generated by 🚫 Danger

[codeclimate]

Code Climate has analyzed commit d94a72d and detected 0 issues on this pull request.

View more on Code Climate.

[rubaiyat22]

LegacyAppeal.veteran_file_number can make requests to VACOLS so we can use the given vacols_case here to grab bfcorlid

[yoomlam]

First pass of suggestions. Will continue to ponder improving the overall flow.

[yoomlam]

Wondering if you can pass this around as a local variable (and as an argument to methods) rather than storing this instance variable as the state of this service.

[rubaiyat22]

yes i thought about that as well. i don't like having this as state of the service either but i was going back and forth because it requires passing this as parameter to multiple methods. I can definitely make those changes though

[yoomlam]

If you don't mind converting it into a local variable, that will make the refactor easier.

[yoomlam]

I also think there's a potential problem. Both cache_ama_appeals (for AMA) and cache_legacy_appeal_postgres_data/cache_legacy_appeal_vacols_data (for Legacy) call append_appeals_with_open_schedule_hearing_tasks, which only stores the appeal_id without the appeal_type. This means that AMA and Legacy appeal ids are clashing with each other. If cache_ama_appeals is called first, there will be AMA appeal ids in appeal_ids_with_open_sched_hearing_task when either of the Legacy methods are called, so there will be extra (AMA) appeal ids that are not relevant to Legacy appeals.

Should add a test for this to make sure that AMA appeal ids are not included in the cache update when the Legacy methods are called.

[rubaiyat22]

I converted both appeal_ids_with_schedule_hearing_tasks and vacols_id_to_appeal_id_mapping into local variables so this should not be an issue anymore

[yoomlam]

1 consideration but LTGM!

[yoomlam]

Consider deleting this method since it is only called once and is only 1 line of code.