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

Don't create BvaDispatchTask if QualityReviewTask is open #16154

Merged
merged 17 commits into from Apr 22, 2021
10 changes: 10 additions & 0 deletions app/models/tasks/bva_dispatch_task.rb
Expand Up @@ -24,9 +24,19 @@ def task_is_assigned_to_organization_user_administers?(user)

class << self
def create_from_root_task(root_task)
return unless ready_for_dispatch?(root_task.appeal)

create!(assigned_to: BvaDispatch.singleton, parent_id: root_task.id, appeal: root_task.appeal)
end

TASK_TYPES_BLOCKING_DISPATCH = [:QualityReviewTask].freeze

def ready_for_dispatch?(appeal)
return false if appeal.tasks.open.where(type: TASK_TYPES_BLOCKING_DISPATCH).any?

Comment on lines +35 to +36
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving room for other checks we should do.

true
end

def outcode(appeal, params, user)
if appeal.is_a?(Appeal)
AmaAppealDispatch.new(appeal: appeal, user: user, params: params).call
Expand Down
30 changes: 19 additions & 11 deletions app/workflows/complete_case_review.rb
Expand Up @@ -11,7 +11,7 @@ def initialize(case_review_class:, params:)
def call
@success = case_review.valid?

create_quality_review_task if success
create_next_task if success
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming method. This method creates a QRTask or BvaDispatchTask.


FormResponse.new(success: success, errors: [response_errors], extra: completed_case_review)
end
Expand All @@ -28,24 +28,32 @@ def review_class
case_review_class.constantize
end

def create_quality_review_task
# Some situations do not want to move onto the QR task
# Where the appeal is a Legacy appeal
# We are in attorney checkout instead of Judge
# The case review is on a flow that already did QR creation
return if case_review.appeal.is_a?(LegacyAppeal) ||
!case_review.is_a?(JudgeCaseReview) ||
case_review.task.parent.is_a?(QualityReviewTask) ||
case_review.task.parent.is_a?(BvaDispatchTask)
def create_next_task
return if irrelevant_scenario_to_create_next_task?

root_task = case_review.task.root_task
if QualityReviewCaseSelector.select_case_for_quality_review?
QualityReviewTask.create_from_root_task(root_task)
else
elsif open_qr_tasks?
BvaDispatchTask.create_from_root_task(root_task)
end
end

# Some situations do not want to move onto the next task:
# - Where the appeal is a Legacy appeal
# - We are in attorney checkout instead of Judge
# - The case review is on a flow that already did QR creation
def irrelevant_scenario_to_create_next_task?
case_review.appeal.is_a?(LegacyAppeal) ||
!case_review.is_a?(JudgeCaseReview) ||
case_review.task.parent.is_a?(QualityReviewTask) ||
case_review.task.parent.is_a?(BvaDispatchTask)
end
Comment on lines +42 to +51
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moving code. Didn't change semantics.


def open_qr_tasks?
case_review.task.appeal.tasks.open.where(type: :QualityReviewTask).blank?
end

def response_errors
return if success

Expand Down
82 changes: 82 additions & 0 deletions spec/feature/queue/judge_checkout_flow_spec.rb
@@ -1,5 +1,8 @@
# frozen_string_literal: true

require "helpers/sanitized_json_configuration.rb"
require "helpers/sanitized_json_importer.rb"

RSpec.feature "Judge checkout flow", :all_dbs do
let(:attorney_user) { create(:default_user) }
let!(:vacols_atty) { create(:staff, :attorney_role, sdomainid: attorney_user.css_id) }
Expand All @@ -14,6 +17,85 @@
BvaDispatch.singleton.add_user(create(:user))
end

# Replicates bug in prod: https://github.com/department-of-veterans-affairs/caseflow/issues/13416
# Scenario: judge opens the Case Details page for the same appeal in two tabs;
# In tab 1, judge goes through checkout and the appeal is (randomly) selected for quality review;
# Then in tab 2, judge goes through checkout and appeal is NOT selected for quality review
# and should NOT create a BvaDispatchTask because the QualityReviewTask is not complete.
context "given an AMA appeal that is selected for quality review" do
before do
FeatureToggle.enable!(:special_issues_revamp)

Organization.create!(id: 212, url: "bvajlmarch", name: "BVAJLMARCH")

# force skipping Quality Review (to mimic tab 2)
allow(QualityReviewCaseSelector).to receive(:select_case_for_quality_review?).and_return(false)
end
after { FeatureToggle.disable!(:special_issues_revamp) }
let!(:appeal) do
sji = SanitizedJsonImporter.from_file("spec/records/appeal-dispatch_before_quality_review_complete.json",
verbosity: 0)
sji.import
Comment on lines +36 to +38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying this out for the first time: https://github.com/department-of-veterans-affairs/caseflow/wiki/Exporting-and-Importing-Appeals
The JSON file is sanitized prod data.


# Remove extraneous tasks
Task.find(2_001_653_201).destroy
Task.find(2_001_648_871).destroy
Task.find(2_001_618_026).destroy

sji.imported_records[Appeal.table_name].first.tap do |appeal|
appeal.root_task.update(status: :on_hold)

# Withdraw a remanded decision issue because CircleCI's Capybara fails clicking boxes for the 2nd issue
decision_issue = appeal.reload.decision_issues.remanded.order(:id).first
decision_issue.update(disposition: :withdrawn)
end.reload
end
let(:jdr_task) { Task.find(2_001_579_253) }

it "prevents appeal dispatch when judge performs checkout again" do
User.authenticate!(user: User.find_by_css_id("WHITEYVACO"))

# Change JudgeDecisionReviewTask status so that "Ready for Dispatch" action is available
jdr_task.update(status: :assigned)
appeal.request_issues.update_all(closed_at: nil)
visit "queue/appeals/#{appeal.uuid}"

# Restore JudgeDecisionReviewTask status from the first checkout
jdr_task.update(status: :completed)
# Note that the judge can continue and complete checkout because frontend was loaded before jdr_task was complete

click_dropdown(text: Constants.TASK_ACTIONS.JUDGE_AMA_CHECKOUT.label)
# Special Issues page
expect(page).to have_content("Select special issues")
find("label", text: "No Special Issues").click
click_on "Continue"

# Decision Issues page
click_on "Continue"
expect(page).to have_content("Issue 1")
find("label", text: "No notice sent").click
find("label", text: "Pre AOJ").click
click_on "Continue"

# Evaluate Decision page
expect(page).to have_content("Evaluate Decision")

find("label", text: Constants::JUDGE_CASE_REVIEW_OPTIONS["COMPLEXITY"]["easy"]).click
text_to_click = "3 - #{Constants::JUDGE_CASE_REVIEW_OPTIONS['QUALITY']['meets_expectations']}"
find("label", text: text_to_click).click
find("#logically_organized", visible: false).sibling("label").click
find("#issues_are_not_addressed", visible: false).sibling("label").click
fill_in "additional-factors", with: generate_words(5)
# Submit POST request
click_on "Continue"

expect(page).to have_content(COPY::JUDGE_CHECKOUT_DISPATCH_SUCCESS_MESSAGE_TITLE % appeal.veteran_full_name)

# The bug was that a BvaDispatchTask is created while the QualityReviewTask is open. It should not be created.
expect(appeal.tasks.open.where(type: :BvaDispatchTask).count).to eq 0
end
end

context "given a valid ama appeal with single issue" do
let!(:appeal) do
create(
Expand Down
20 changes: 16 additions & 4 deletions spec/models/tasks/bva_dispatch_task_spec.rb
Expand Up @@ -12,14 +12,16 @@
end
end

let(:root_task) { create(:root_task) }
subject { BvaDispatchTask.create_from_root_task(root_task) }
Comment on lines +15 to +16
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactoring common code to DRY it up.


context "when valid root_task passed as argument" do
let(:root_task) { create(:root_task) }
before do
BvaDispatch.singleton.add_user(create(:user))
end

it "should create a BvaDispatchTask assigned to a User with a parent task assigned to the BvaDispatch org" do
parent_task = BvaDispatchTask.create_from_root_task(root_task)
parent_task = subject
expect(parent_task.assigned_to.class).to eq(BvaDispatch)
expect(parent_task.status).to eq(Constants.TASK_STATUSES.on_hold)
expect(parent_task.children.count).to eq 1
Expand All @@ -30,14 +32,24 @@
end

context "when organization-level BvaDispatchTask already exists" do
let(:root_task) { create(:root_task) }
before do
BvaDispatch.singleton.add_user(create(:user))
BvaDispatchTask.create_from_root_task(root_task)
end

it "should raise an error" do
expect { BvaDispatchTask.create_from_root_task(root_task) }.to raise_error(Caseflow::Error::DuplicateOrgTask)
expect { subject }.to raise_error(Caseflow::Error::DuplicateOrgTask)
end
end

context "when an open QualityReviewTask exists" do
before do
BvaDispatch.singleton.add_user(create(:user))
create(:quality_review_task, parent: root_task)
end

it "should not create BvaDispatchTask" do
expect(subject).to be_nil
end
end
end
Expand Down