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

Stuck Appeals Job now finds appeals where on_hold tasks do not have an active descendant #15595

Merged
merged 3 commits into from
Nov 12, 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
13 changes: 13 additions & 0 deletions app/queries/appeals_with_no_tasks_or_all_tasks_on_hold_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ def call
appeals_with_zero_tasks,
appeals_with_one_task,
appeals_with_two_tasks_not_distribution,
appeals_with_fully_on_hold_subtree,
dispatched_appeals_on_hold
].flatten.uniq
end
Expand Down Expand Up @@ -54,6 +55,18 @@ def appeals_with_two_tasks_not_distribution
.having("count(tasks) = 2 AND count(case when tasks.type = ? then 1 end) = 0", DistributionTask.name)
end

# Confirm that all subtrees have an active task
def appeals_with_fully_on_hold_subtree
Appeal.where(id:
Task.left_outer_joins(:children).on_hold
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left_outer_joins(:children) asks rails to grab children tasks.
the outer allows us to capture on_hold tasks without any children.
Uses the Rails associations to understand what children means.
we can't use includes as we never invoke the :children symbol later - its used invisibly to Rails in the having

https://blog.saeloun.com/2020/01/08/activerecord-database-performance-n-1-includes-preload-eager-load-pluck.html

.where.not(type: [RootTask.name, TrackVeteranTask.name, EvidenceSubmissionWindowTask.name, TimedHoldTask.name])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: TrackVeteranTask, EvidenceSubmissionWindowTask, and TimedHoldTask can be on-hold without active children tasks. Since these reside under RootTask, the RootTask can be on-hold without active children as well.

.group("tasks.id")
.having(
"count(case when children_tasks.status in (?) then 1 end) = 0",
Task.open_statuses
Comment on lines +63 to +66
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautifully fast tricksy count logic stolen reused from below - thanks @pkarman!

).select(:appeal_id).distinct)
end

def tasks_for(klass_name)
Task.select(:appeal_id).where(appeal_type: klass_name)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,20 @@
schedule_hearing_task.completed!
appeal
end
let!(:appeal_with_fully_on_hold_subtree) do
appeal = create(:appeal, :with_post_intake_tasks)
task = create(:privacy_act_task, appeal: appeal, parent: appeal.root_task)
task.descendants.each(&:on_hold!)
appeal
end
let!(:appeal_with_failed_reactivated_task) do
appeal = create(:appeal, :with_post_intake_tasks)
task1 = create(:privacy_act_task, appeal: appeal, parent: appeal.root_task)
task1.descendants.each(&:on_hold!)
task2 = create(:privacy_act_task, appeal: appeal, parent: task1)
task2.descendants.each(&:completed!)
appeal
end
let!(:appeal_with_decision_documents) do
appeal = create(:appeal, :with_post_intake_tasks)
create(:decision_document, appeal: appeal)
Expand All @@ -37,6 +51,8 @@
appeal_with_zero_tasks,
appeal_with_one_task,
appeal_with_all_tasks_on_hold,
appeal_with_fully_on_hold_subtree,
appeal_with_failed_reactivated_task,
appeal_with_two_tasks_not_distribution,
dispatched_appeal_on_hold
]
Expand Down Expand Up @@ -68,6 +84,18 @@
it { is_expected.to eq(true) }
end

context "appeal_with_fully_on_hold_subtree" do
let(:appeal) { appeal_with_fully_on_hold_subtree }

it { is_expected.to eq(true) }
end

context "appeal_with_failed_reactivated_task" do
let(:appeal) { appeal_with_failed_reactivated_task }

it { is_expected.to eq(true) }
end

context "appeal_with_decision_documents" do
let(:appeal) { appeal_with_decision_documents }

Expand Down
10 changes: 8 additions & 2 deletions spec/services/stuck_appeals_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,24 @@
create(:bva_dispatch_task, :completed, appeal: appeal)
appeal
end
let!(:appeal_with_fully_on_hold_subtree) do
appeal = create(:appeal, :with_post_intake_tasks)
task = create(:privacy_act_task, appeal: appeal, parent: appeal.root_task)
task.descendants.each(&:on_hold!)
appeal
end
let!(:appeal_with_closed_root_open_child) do
appeal = create(:appeal, :with_post_intake_tasks)
appeal.root_task.completed!
appeal
end

describe "#call" do
it "reports 3 appeals stuck" do
it "reports 5 appeals stuck" do
subject.call

expect(subject.report?).to eq(true)
expect(subject.report).to match(/AppealsWithNoTasksOrAllTasksOnHoldQuery: 3/)
expect(subject.report).to match(/AppealsWithNoTasksOrAllTasksOnHoldQuery: 4/)
expect(subject.report).to match(/AppealsWithClosedRootTaskOpenChildrenQuery: 1/)
end
end
Expand Down