From bb875f56ea57ee9d89de1a331d7650c1992e4ce8 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Fri, 30 Jul 2021 12:19:33 -0400 Subject: [PATCH 01/25] wrong start - need to change task factory --- .../investigate_duplicate_judge_tasks_spec.rb | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 spec/fixes/investigate_duplicate_judge_tasks_spec.rb diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb new file mode 100644 index 00000000000..e3406776ea1 --- /dev/null +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -0,0 +1,54 @@ + + +feature "duplicate JudgeAssignTask investigation" do + before do + User.authenticate!(css_id: "BVAAABSHIRE") + end + + # Ticket: https://github.com/department-of-veterans-affairs/dsva-vacols/issues/212# + # Target state: there should be no more than 1 open JudgeAssignTask + describe "Judge reassigns JudgeAssignTask in first tab and complete task in second tab" do + let(:judge) { create(:user) } + let!(:vacols_judge) { create(:staff, :judge_role, sdomainid: judge.css_id) } + let(:judge_team) { JudgeTeam.create_for_judge(judge) } + + let(:attorney) { create(:user, :with_vacols_attorney_record) } + let(:appeal) { create(:appeal, :at_attorney_drafting, associated_judge: judge, associated_attorney: attorney) } + + scenario "Caseflow creates multiple JudgeDecisionReview and JudgeAssign tasks" do + # open a window and visit case details page, window A + visit "/queue/appeals/#{appeal.uuid}" + binding.pry + + # open a second window and visit case details page, window B + second_window = open_new_window + within_window second_window do + visit "/queue/appeals/#{appeal.uuid}" + end + + # in window A, reassign the JudgeAssignTask to another judge + # window A: expect message - task reassigned + click_dropdown(prompt: "Select an action", text: "Re-assign to a judge") + expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE) + first_judge_assign_task = appeal.tasks.select{ |task| task.type == "JudgeAssignTask" }[0] + # test that JudgeAssignTask is cancelled + expect(first_judge_assign_task.status).to eq("cancelled") + + within_window second_window do + # in window B, complete the JudgeAssignTask + # window B: expect message - attorney now has task + click_dropdown(prompt: "Select an action", text: "Assign to attorney") + expect(page).to have_content(COPY::ASSIGN_TASK_SUCCESS_MESSAGE) + + # below will fail before fix is made - app currently allows the invalid flow of a JudgeAssignTask going from cancelled to complete + expect(first_judge_assign_task.status).to eq("cancelled") + end + + appeal.reload + + # test that appeal has multiple JudgeAssignTasks + # test that appeal has multiple JudgeDecisionReviewTasks + # test that appeal has multiple AttorneyTasks + end + end +end \ No newline at end of file From 2e11eb4990bba93db517425a8396acaaec8ba5c2 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 2 Aug 2021 18:03:25 -0400 Subject: [PATCH 02/25] test of invalid JudgeAssignTask status --- .../investigate_duplicate_judge_tasks_spec.rb | 48 +++++++++++-------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index e3406776ea1..d38ae2d8d21 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -1,24 +1,27 @@ - +# frozen_string_literal: true feature "duplicate JudgeAssignTask investigation" do before do - User.authenticate!(css_id: "BVAAABSHIRE") + User.authenticate!(user: judge_user) end # Ticket: https://github.com/department-of-veterans-affairs/dsva-vacols/issues/212# # Target state: there should be no more than 1 open JudgeAssignTask describe "Judge reassigns JudgeAssignTask in first tab and complete task in second tab" do - let(:judge) { create(:user) } - let!(:vacols_judge) { create(:staff, :judge_role, sdomainid: judge.css_id) } - let(:judge_team) { JudgeTeam.create_for_judge(judge) } - - let(:attorney) { create(:user, :with_vacols_attorney_record) } - let(:appeal) { create(:appeal, :at_attorney_drafting, associated_judge: judge, associated_attorney: attorney) } + let(:judge_user) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "Anna Juarez") } + let!(:judge_staff) { create(:staff, :judge_role, user: judge_user) } + let(:judge_team) { JudgeTeam.create_for_judge(judge_user) } + let(:attorney_user) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "Steven Ahr") } + let!(:attorney_staff) { create(:staff, :attorney_role, user: attorney_user) } + + let(:root_task) { create(:root_task) } + let(:judge_assign_task) { create(:ama_judge_assign_task, parent: root_task, assigned_to: judge_user) } + let(:appeal) { judge_assign_task.appeal } + let(:uuid) { appeal.uuid } scenario "Caseflow creates multiple JudgeDecisionReview and JudgeAssign tasks" do # open a window and visit case details page, window A visit "/queue/appeals/#{appeal.uuid}" - binding.pry # open a second window and visit case details page, window B second_window = open_new_window @@ -26,22 +29,27 @@ visit "/queue/appeals/#{appeal.uuid}" end - # in window A, reassign the JudgeAssignTask to another judge - # window A: expect message - task reassigned + # in window A, reassign the JudgeAssignTask to another judge + first_judge_assign_task_id = appeal.tasks.select { |task| task.type == "JudgeAssignTask" }[0].id click_dropdown(prompt: "Select an action", text: "Re-assign to a judge") - expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE) - first_judge_assign_task = appeal.tasks.select{ |task| task.type == "JudgeAssignTask" }[0] - # test that JudgeAssignTask is cancelled - expect(first_judge_assign_task.status).to eq("cancelled") + click_dropdown(prompt: "Select a user", text: judge_user.full_name) + fill_in "taskInstructions", with: "reassign this task! teamwork makes the dreamwork!" + click_on "Submit" + + expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user.full_name) + expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") within_window second_window do - # in window B, complete the JudgeAssignTask - # window B: expect message - attorney now has task click_dropdown(prompt: "Select an action", text: "Assign to attorney") - expect(page).to have_content(COPY::ASSIGN_TASK_SUCCESS_MESSAGE) + click_dropdown(prompt: "Select a user", text: "Other") + click_dropdown(prompt: "Select a user", text: attorney_user.full_name) + fill_in "taskInstructions", with: "assign to attorney" + click_on "Submit" + + expect(page).to have_content(COPY::ASSIGN_TASK_SUCCESS_MESSAGE, attorney_user.full_name) # below will fail before fix is made - app currently allows the invalid flow of a JudgeAssignTask going from cancelled to complete - expect(first_judge_assign_task.status).to eq("cancelled") + # expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") end appeal.reload @@ -51,4 +59,4 @@ # test that appeal has multiple AttorneyTasks end end -end \ No newline at end of file +end From a0e6eff5fa31881312322a99c23080ff12138ad1 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 3 Aug 2021 13:56:34 -0400 Subject: [PATCH 03/25] WIP - can't figure out how to select correct dropdown --- .../investigate_duplicate_judge_tasks_spec.rb | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index d38ae2d8d21..8acf4ee1f14 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -13,6 +13,8 @@ let(:judge_team) { JudgeTeam.create_for_judge(judge_user) } let(:attorney_user) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "Steven Ahr") } let!(:attorney_staff) { create(:staff, :attorney_role, user: attorney_user) } + let(:attorney_user_second) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "John Lee") } + let!(:attorney_staff_second) { create(:staff, :attorney_role, user: attorney_user_second) } let(:root_task) { create(:root_task) } let(:judge_assign_task) { create(:ama_judge_assign_task, parent: root_task, assigned_to: judge_user) } @@ -38,7 +40,9 @@ expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user.full_name) expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") + visit "/queue/appeals/#{appeal.uuid}" + # in window B, complete the JudgeAssignTask within_window second_window do click_dropdown(prompt: "Select an action", text: "Assign to attorney") click_dropdown(prompt: "Select a user", text: "Other") @@ -48,15 +52,27 @@ expect(page).to have_content(COPY::ASSIGN_TASK_SUCCESS_MESSAGE, attorney_user.full_name) - # below will fail before fix is made - app currently allows the invalid flow of a JudgeAssignTask going from cancelled to complete - # expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") + visit "/queue/appeals/#{appeal.uuid}" + # Complete JudgeAssignTask to create an invalid number of AttorneyTasks + # TODO - figure out how to select an option from the second dropdown + # below correctly selects the second task action dropdown + find("#currently-active-tasks > div:nth-child(4) > table > tbody > tr:nth-child(3) > td.taskContainerStyling.taskActionsContainerStyling > div > div > div > div > div > div").click + # below - could not successfully select the "assign to attorney" dropdown because there were two dropdowns with this option + click_dropdown(text: "Assign to attorney") + # click_dropdown(prompt: "Select a user", text: attorney_user_second.full_name) + # fill_in "taskInstructions", with: "mimic incorrect flow" + # click_on "Submit" + + # below will fail before fix is made + # app currently allows the invalid flow of a JudgeAssignTask going from cancelled to complete + expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") end - appeal.reload + visit "/queue/appeals/#{appeal.uuid}" + # see what dropdowns are there - # test that appeal has multiple JudgeAssignTasks - # test that appeal has multiple JudgeDecisionReviewTasks - # test that appeal has multiple AttorneyTasks + # TODO: test that appeal has multiple JudgeDecisionReviewTasks + # TODO: test that appeal has multiple AttorneyTasks end end end From 2dfbe3026a2b0205c0c96f39736457e243e01c3c Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 3 Aug 2021 15:29:30 -0400 Subject: [PATCH 04/25] add validation for jdr task --- .../tasks/judge_decision_review_task.rb | 21 +----- spec/models/tasks/judge_task_spec.rb | 72 ++++++++++++++----- 2 files changed, 57 insertions(+), 36 deletions(-) diff --git a/app/models/tasks/judge_decision_review_task.rb b/app/models/tasks/judge_decision_review_task.rb index 103b45cd7f5..2b700230a73 100644 --- a/app/models/tasks/judge_decision_review_task.rb +++ b/app/models/tasks/judge_decision_review_task.rb @@ -9,7 +9,8 @@ # and create a new JudgeAssignTask, because another assignment by a judge is needed. class JudgeDecisionReviewTask < JudgeTask - before_create :verify_user_task_unique + validate :only_open_task_of_type, on: :create, + unless: :skip_check_for_only_open_task_of_type? def additional_available_actions(user) return [] unless assigned_to == user @@ -31,24 +32,6 @@ def self.label COPY::JUDGE_DECISION_REVIEW_TASK_LABEL end - # Use the existence of another open JudgeDecisionReviewTask to prevent duplicates since there should only - # ever be one open JudgeDecisionReviewTask at a time for an appeal. - def verify_user_task_unique - return if !open? - - if appeal.tasks.open.where( - type: type, - assigned_to: assigned_to, - parent: parent - ).any? && assigned_to.is_a?(User) - fail( - Caseflow::Error::DuplicateUserTask, - docket_number: appeal.docket_number, - task_type: self.class.name - ) - end - end - private def ama_judge_actions(user) diff --git a/spec/models/tasks/judge_task_spec.rb b/spec/models/tasks/judge_task_spec.rb index 2ec00329bc8..3bb2782271d 100644 --- a/spec/models/tasks/judge_task_spec.rb +++ b/spec/models/tasks/judge_task_spec.rb @@ -16,6 +16,9 @@ let!(:first_assign_task) do create(:ama_judge_assign_task, assigned_to: judge, appeal: appeal) end + let!(:first_review_task) do + create(:ama_judge_decision_review_task, assigned_to: judge, appeal: appeal) + end context "when one judge assign task is open for an appeal" do it "throws an error when a second task is created" do @@ -24,11 +27,18 @@ end end end + + context "when one judge decision review task is open for an appeal" do + it "throws an error when a second task is created" do + expect { create(:ama_judge_decision_review_task, assigned_to: judge2, appeal: appeal) }.to raise_error do |err| + expect(err).to be_a(Caseflow::Error::MultipleOpenTasksOfSameTypeError) + end + end + end end describe "reassign" do let(:root_task) { create(:root_task) } - let(:task) { create(:ama_judge_assign_task, parent: root_task) } let(:old_assignee) { task.assigned_to } let(:new_assignee) { create(:user) } let(:params) do @@ -40,25 +50,53 @@ end subject { task.reassign(params, old_assignee) } - context "when a judge task is reassigned successfully" do - it "should not violate the only_open_task_of_type validation" do - expect { subject }.to_not raise_error - expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + context "when the task is a judge decision review task" do + let(:task) { create(:ama_judge_decision_review_task, parent: root_task) } + context "when a judge decision review task is reassigned successfully" do + it "should not violate the only_open_task_of_type validation" do + expect { subject }.to_not raise_error + expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + end end - end - context "when a judge task throws an error during attempted reassignment" do - let(:params) do - { - assigned_to_id: nil, - assigned_to_type: new_assignee.class.name, - instructions: "instructions" - } + context "when a judge decision review task throws an error during attempted reassignment" do + let(:params) do + { + assigned_to_id: nil, + assigned_to_type: new_assignee.class.name, + instructions: "instructions" + } + end + + it "sets the thread local variable of skip_only_open_task_of_type to nil" do + expect { subject }.to raise_error ActiveRecord::RecordNotFound + expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + end end + end + + context "when the task is a judge assign task" do + let(:task) { create(:ama_judge_assign_task, parent: root_task) } + context "when a judge assign task is reassigned successfully" do + it "should not violate the only_open_task_of_type validation" do + expect { subject }.to_not raise_error + expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + end + end + + context "when a judge assign task throws an error during attempted reassignment" do + let(:params) do + { + assigned_to_id: nil, + assigned_to_type: new_assignee.class.name, + instructions: "instructions" + } + end - it "sets the thread local variable of skip_only_open_task_of_type to nil" do - expect { subject }.to raise_error ActiveRecord::RecordNotFound - expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + it "sets the thread local variable of skip_only_open_task_of_type to nil" do + expect { subject }.to raise_error ActiveRecord::RecordNotFound + expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + end end end end @@ -246,7 +284,7 @@ it "should fail creation of second JudgeDecisionReviewTask" do expect(root_task.appeal.tasks.count).to eq(2), root_task.appeal.tasks.to_a.to_s jdrt.update(status: o_status) - expect { subject }.to raise_error(Caseflow::Error::DuplicateUserTask) + expect { subject }.to raise_error(Caseflow::Error::MultipleOpenTasksOfSameTypeError) expect(root_task.appeal.tasks.count).to eq(2), root_task.appeal.tasks.to_a.to_s end end From 947c08ba03944724a0ff58e735095b8df2bffb2e Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 3 Aug 2021 16:25:18 -0400 Subject: [PATCH 05/25] add validation to attorney appeal --- app/models/tasks/attorney_task.rb | 3 +++ spec/models/tasks/attorney_task_spec.rb | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/app/models/tasks/attorney_task.rb b/app/models/tasks/attorney_task.rb index aa5429f519b..d0f097871d1 100644 --- a/app/models/tasks/attorney_task.rb +++ b/app/models/tasks/attorney_task.rb @@ -7,6 +7,9 @@ # - adding admin actions (like translating documents) class AttorneyTask < Task + validate :only_open_task_of_type, on: :create, + unless: :skip_check_for_only_open_task_of_type? + validates :assigned_by, presence: true validates :parent, presence: true, if: :ama? diff --git a/spec/models/tasks/attorney_task_spec.rb b/spec/models/tasks/attorney_task_spec.rb index 4d1d7181c60..50fa0931db2 100644 --- a/spec/models/tasks/attorney_task_spec.rb +++ b/spec/models/tasks/attorney_task_spec.rb @@ -51,6 +51,15 @@ expect(subject.errors.messages[:assigned_to].first).to eq "has to be an attorney" end end + + context "when an AttorneyTask is already open for the appeal" do + let!(:attorney_task) { create(:ama_attorney_task, appeal: appeal, parent: parent) } + it "throws an error when a second task is created" do + expect { subject }.to raise_error do |error| + expect(error).to be_a(Caseflow::Error::MultipleOpenTasksOfSameTypeError) + end + end + end end context ".update" do From 90ca288e23f3ab6bf30748e2746c701f8d0af5af Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 3 Aug 2021 17:06:10 -0400 Subject: [PATCH 06/25] refactor task validation to remove thread local variables --- app/models/task.rb | 14 ++++++-------- app/models/tasks/attorney_task.rb | 2 +- app/models/tasks/judge_assign_task.rb | 2 +- app/models/tasks/judge_decision_review_task.rb | 2 +- spec/models/tasks/judge_task_spec.rb | 12 ++++++------ 5 files changed, 15 insertions(+), 17 deletions(-) diff --git a/app/models/task.rb b/app/models/task.rb index 174cf288ce4..9036d96f943 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -95,6 +95,8 @@ class << self; undef_method :open; end scope :with_cached_appeals, -> { joins(Task.joins_with_cached_appeals_clause) } + attr_accessor :skip_check_for_only_open_task_of_type + ############################################################################################ ## class methods class << self @@ -562,7 +564,7 @@ def verify_user_can_update!(user) def reassign(reassign_params, current_user) # We do not validate the number of tasks in this scenario because when a # task is reassigned, more than one open task of the same type must exist during the reassignment. - Thread.current.thread_variable_set(:skip_check_for_only_open_task_of_type, true) + self.skip_check_for_only_open_task_of_type = true replacement = dup.tap do |task| begin ActiveRecord::Base.transaction do @@ -573,10 +575,10 @@ def reassign(reassign_params, current_user) task.save! end - # The ensure block guarantees that the thread-local variable check_for_open_task_type - # does not leak outside of this method + # The ensure block guarantees that the variable skip_check_for_only_open_task_of_type is reset, + # even if an error occurs during reassignment ensure - Thread.current.thread_variable_set(:skip_check_for_only_open_task_of_type, nil) + self.skip_check_for_only_open_task_of_type = nil end end @@ -834,10 +836,6 @@ def assignee_status_is_valid_on_create true end - def skip_check_for_only_open_task_of_type? - Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type) - end - def only_open_task_of_type if appeal.reload.tasks.open.of_type(type).any? fail Caseflow::Error::MultipleOpenTasksOfSameTypeError, task_type: type diff --git a/app/models/tasks/attorney_task.rb b/app/models/tasks/attorney_task.rb index d0f097871d1..f2558f5c43b 100644 --- a/app/models/tasks/attorney_task.rb +++ b/app/models/tasks/attorney_task.rb @@ -8,7 +8,7 @@ class AttorneyTask < Task validate :only_open_task_of_type, on: :create, - unless: :skip_check_for_only_open_task_of_type? + unless: :skip_check_for_only_open_task_of_type validates :assigned_by, presence: true validates :parent, presence: true, if: :ama? diff --git a/app/models/tasks/judge_assign_task.rb b/app/models/tasks/judge_assign_task.rb index ce0e24a897e..81b4067be1f 100644 --- a/app/models/tasks/judge_assign_task.rb +++ b/app/models/tasks/judge_assign_task.rb @@ -15,7 +15,7 @@ class JudgeAssignTask < JudgeTask validate :only_open_task_of_type, on: :create, - unless: :skip_check_for_only_open_task_of_type? + unless: :skip_check_for_only_open_task_of_type def additional_available_actions(_user) [Constants.TASK_ACTIONS.ASSIGN_TO_ATTORNEY.to_h] diff --git a/app/models/tasks/judge_decision_review_task.rb b/app/models/tasks/judge_decision_review_task.rb index 2b700230a73..0873879941d 100644 --- a/app/models/tasks/judge_decision_review_task.rb +++ b/app/models/tasks/judge_decision_review_task.rb @@ -10,7 +10,7 @@ class JudgeDecisionReviewTask < JudgeTask validate :only_open_task_of_type, on: :create, - unless: :skip_check_for_only_open_task_of_type? + unless: :skip_check_for_only_open_task_of_type def additional_available_actions(user) return [] unless assigned_to == user diff --git a/spec/models/tasks/judge_task_spec.rb b/spec/models/tasks/judge_task_spec.rb index 3bb2782271d..9b9ba762363 100644 --- a/spec/models/tasks/judge_task_spec.rb +++ b/spec/models/tasks/judge_task_spec.rb @@ -55,7 +55,7 @@ context "when a judge decision review task is reassigned successfully" do it "should not violate the only_open_task_of_type validation" do expect { subject }.to_not raise_error - expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + expect(task.skip_check_for_only_open_task_of_type).to be_nil end end @@ -68,19 +68,19 @@ } end - it "sets the thread local variable of skip_only_open_task_of_type to nil" do + it "sets the local variable of skip_check_for_only_open_task_of_type to nil" do expect { subject }.to raise_error ActiveRecord::RecordNotFound - expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + expect(task.skip_check_for_only_open_task_of_type).to be_nil end end end - + context "when the task is a judge assign task" do let(:task) { create(:ama_judge_assign_task, parent: root_task) } context "when a judge assign task is reassigned successfully" do it "should not violate the only_open_task_of_type validation" do expect { subject }.to_not raise_error - expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + expect(task.skip_check_for_only_open_task_of_type).to be_nil end end @@ -95,7 +95,7 @@ it "sets the thread local variable of skip_only_open_task_of_type to nil" do expect { subject }.to raise_error ActiveRecord::RecordNotFound - expect(Thread.current.thread_variable_get(:skip_check_for_only_open_task_of_type)).to be_nil + expect(task.skip_check_for_only_open_task_of_type).to be_nil end end end From bc92b17476f635626964d97e7f62a8a8954360f0 Mon Sep 17 00:00:00 2001 From: yoomlam Date: Tue, 3 Aug 2021 16:21:39 -0500 Subject: [PATCH 07/25] spec fixed --- .../investigate_duplicate_judge_tasks_spec.rb | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index 8acf4ee1f14..69cc2bff353 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -24,6 +24,7 @@ scenario "Caseflow creates multiple JudgeDecisionReview and JudgeAssign tasks" do # open a window and visit case details page, window A visit "/queue/appeals/#{appeal.uuid}" + appeal.reload.treee # open a second window and visit case details page, window B second_window = open_new_window @@ -37,6 +38,7 @@ click_dropdown(prompt: "Select a user", text: judge_user.full_name) fill_in "taskInstructions", with: "reassign this task! teamwork makes the dreamwork!" click_on "Submit" + appeal.reload.treee expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user.full_name) expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") @@ -49,23 +51,25 @@ click_dropdown(prompt: "Select a user", text: attorney_user.full_name) fill_in "taskInstructions", with: "assign to attorney" click_on "Submit" + appeal.reload.treee expect(page).to have_content(COPY::ASSIGN_TASK_SUCCESS_MESSAGE, attorney_user.full_name) visit "/queue/appeals/#{appeal.uuid}" # Complete JudgeAssignTask to create an invalid number of AttorneyTasks - # TODO - figure out how to select an option from the second dropdown - # below correctly selects the second task action dropdown - find("#currently-active-tasks > div:nth-child(4) > table > tbody > tr:nth-child(3) > td.taskContainerStyling.taskActionsContainerStyling > div > div > div > div > div > div").click - # below - could not successfully select the "assign to attorney" dropdown because there were two dropdowns with this option - click_dropdown(text: "Assign to attorney") - # click_dropdown(prompt: "Select a user", text: attorney_user_second.full_name) - # fill_in "taskInstructions", with: "mimic incorrect flow" - # click_on "Submit" + # select the second task action dropdown + assign_task_row = find("table[summary='layout table'] tr:nth-child(3)") + # select the "assign to attorney" dropdown because there were two dropdowns with this option + click_dropdown({text: "Assign to attorney"}, assign_task_row) + click_dropdown(prompt: "Select a user", text: "Other") + click_dropdown(prompt: "Select a user", text: attorney_user_second.full_name) + fill_in "taskInstructions", with: "mimic incorrect flow" + click_on "Submit" + appeal.reload.treee # below will fail before fix is made # app currently allows the invalid flow of a JudgeAssignTask going from cancelled to complete - expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") + expect(Task.find(first_judge_assign_task_id).status).to eq("completed") end visit "/queue/appeals/#{appeal.uuid}" From 17976e1104391936468f1a9359a769ce9c1c7f80 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 3 Aug 2021 17:31:45 -0400 Subject: [PATCH 08/25] remove ensure block from task reassign --- app/models/task.rb | 20 +++++++------------- spec/models/tasks/judge_task_spec.rb | 12 ------------ 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/app/models/task.rb b/app/models/task.rb index 9036d96f943..4a0c6c145f8 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -566,19 +566,13 @@ def reassign(reassign_params, current_user) # task is reassigned, more than one open task of the same type must exist during the reassignment. self.skip_check_for_only_open_task_of_type = true replacement = dup.tap do |task| - begin - ActiveRecord::Base.transaction do - task.assigned_by_id = self.class.child_assigned_by_id(parent, current_user) - task.assigned_to = self.class.child_task_assignee(parent, reassign_params) - task.instructions = flattened_instructions(reassign_params) - task.status = Constants.TASK_STATUSES.assigned - - task.save! - end - # The ensure block guarantees that the variable skip_check_for_only_open_task_of_type is reset, - # even if an error occurs during reassignment - ensure - self.skip_check_for_only_open_task_of_type = nil + ActiveRecord::Base.transaction do + task.assigned_by_id = self.class.child_assigned_by_id(parent, current_user) + task.assigned_to = self.class.child_task_assignee(parent, reassign_params) + task.instructions = flattened_instructions(reassign_params) + task.status = Constants.TASK_STATUSES.assigned + + task.save! end end diff --git a/spec/models/tasks/judge_task_spec.rb b/spec/models/tasks/judge_task_spec.rb index 9b9ba762363..69f3c9c0aa7 100644 --- a/spec/models/tasks/judge_task_spec.rb +++ b/spec/models/tasks/judge_task_spec.rb @@ -55,7 +55,6 @@ context "when a judge decision review task is reassigned successfully" do it "should not violate the only_open_task_of_type validation" do expect { subject }.to_not raise_error - expect(task.skip_check_for_only_open_task_of_type).to be_nil end end @@ -67,11 +66,6 @@ instructions: "instructions" } end - - it "sets the local variable of skip_check_for_only_open_task_of_type to nil" do - expect { subject }.to raise_error ActiveRecord::RecordNotFound - expect(task.skip_check_for_only_open_task_of_type).to be_nil - end end end @@ -80,7 +74,6 @@ context "when a judge assign task is reassigned successfully" do it "should not violate the only_open_task_of_type validation" do expect { subject }.to_not raise_error - expect(task.skip_check_for_only_open_task_of_type).to be_nil end end @@ -92,11 +85,6 @@ instructions: "instructions" } end - - it "sets the thread local variable of skip_only_open_task_of_type to nil" do - expect { subject }.to raise_error ActiveRecord::RecordNotFound - expect(task.skip_check_for_only_open_task_of_type).to be_nil - end end end end From fb3349561fa536c8c7b78015374de34077a820fc Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 3 Aug 2021 17:41:30 -0400 Subject: [PATCH 09/25] clean up tests --- spec/models/tasks/judge_task_spec.rb | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/spec/models/tasks/judge_task_spec.rb b/spec/models/tasks/judge_task_spec.rb index 69f3c9c0aa7..f8d1683a5b2 100644 --- a/spec/models/tasks/judge_task_spec.rb +++ b/spec/models/tasks/judge_task_spec.rb @@ -57,16 +57,6 @@ expect { subject }.to_not raise_error end end - - context "when a judge decision review task throws an error during attempted reassignment" do - let(:params) do - { - assigned_to_id: nil, - assigned_to_type: new_assignee.class.name, - instructions: "instructions" - } - end - end end context "when the task is a judge assign task" do @@ -76,16 +66,6 @@ expect { subject }.to_not raise_error end end - - context "when a judge assign task throws an error during attempted reassignment" do - let(:params) do - { - assigned_to_id: nil, - assigned_to_type: new_assignee.class.name, - instructions: "instructions" - } - end - end end end From 43fc305123b26cb85db8db206d601f4abf592187 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 9 Aug 2021 16:44:19 -0400 Subject: [PATCH 10/25] continue test clean-up --- spec/feature/queue/task_queue_spec.rb | 10 ---------- spec/models/concerns/task_tree_render_module_spec.rb | 4 ++-- spec/models/tasks/colocated_task_spec.rb | 5 ++--- 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/spec/feature/queue/task_queue_spec.rb b/spec/feature/queue/task_queue_spec.rb index c137a566722..d3c25e84792 100644 --- a/spec/feature/queue/task_queue_spec.rb +++ b/spec/feature/queue/task_queue_spec.rb @@ -17,16 +17,6 @@ ) end - let!(:attorney_task_new_docs) do - create( - :ama_attorney_task, - :on_hold, - assigned_to: attorney_user, - placed_on_hold_at: 4.days.ago, - appeal: attorney_task.appeal - ) - end - let!(:colocated_task) do create( :ama_colocated_task, diff --git a/spec/models/concerns/task_tree_render_module_spec.rb b/spec/models/concerns/task_tree_render_module_spec.rb index 8f8a4d60187..1299f664c14 100644 --- a/spec/models/concerns/task_tree_render_module_spec.rb +++ b/spec/models/concerns/task_tree_render_module_spec.rb @@ -5,7 +5,7 @@ let(:root_task) { appeal.root_task } let!(:ama_judge_assign_task) { create(:ama_judge_assign_task, parent: root_task, created_at: 1.day.ago.round) } let!(:ama_attorney_task) { create(:ama_attorney_task, parent: root_task) } - let!(:ama_attorney_task_no_parent) { create(:ama_attorney_task, appeal: appeal) } + let!(:task_no_parent) { create(:track_veteran_task, appeal: appeal) } context "#tree is called on an appeal" do it "returns all tasks for the appeal" do @@ -131,7 +131,7 @@ def tree2(obj, *atts, **kwargs) initial_count = appeal.tasks.count + 3 expect(appeal.tree.lines.count).to eq initial_count - ama_attorney_task_no_parent.delete + task_no_parent.delete appeal.reload expect(appeal.tree.lines.count).to eq initial_count - 1 end diff --git a/spec/models/tasks/colocated_task_spec.rb b/spec/models/tasks/colocated_task_spec.rb index a838ac44916..b4161871306 100644 --- a/spec/models/tasks/colocated_task_spec.rb +++ b/spec/models/tasks/colocated_task_spec.rb @@ -611,8 +611,7 @@ create(:judge_address_motion_to_vacate_task, appeal: appeal, assigned_to: judge, - parent: vacate_motion_mail_task - ) + parent: vacate_motion_mail_task) end let(:post_decision_motion_params) do { @@ -629,7 +628,7 @@ Appeal.find_by(stream_docket_number: appeal.docket_number, stream_type: Constants.AMA_STREAM_TYPES.vacate) end let(:attorney_task) { AttorneyTask.find_by(assigned_to: attorney) } - let(:parent) { create(:ama_judge_decision_review_task, assigned_to: judge, appeal: vacate_stream ) } + let(:parent) { attorney_task.parent } let(:params_list) do [{ From ef27df7db6cc0b2ea3c6102521ba6426a1b48b16 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 9 Aug 2021 17:39:41 -0400 Subject: [PATCH 11/25] update tests --- spec/feature/queue/judge_assignment_spec.rb | 7 +++-- spec/models/appeal_spec.rb | 30 ++++++++++++++++----- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/spec/feature/queue/judge_assignment_spec.rb b/spec/feature/queue/judge_assignment_spec.rb index c128a2ef148..38839e61598 100644 --- a/spec/feature/queue/judge_assignment_spec.rb +++ b/spec/feature/queue/judge_assignment_spec.rb @@ -232,8 +232,11 @@ click_on "Assign 2 cases" expect(page).to have_content("#{attorney_one.full_name} (0)") expect(page).to have_content("Error assigning tasks") - expect(page).to have_content("Docket (#{appeal_two.docket_number}) already "\ - "has an open task type of #{JudgeDecisionReviewTask.name}") + + # rubocop:disable Layout/LineLength + expect(page).to have_content("Looks like this appeal already has an open JudgeDecisionReviewTask and this action cannot be completed.") + # rubocop:enable Layout/LineLength + visit "/queue/#{judge_one.user.css_id}/assign" expect(page).to have_content("#{attorney_one.full_name} (0)") diff --git a/spec/models/appeal_spec.rb b/spec/models/appeal_spec.rb index 8ee4156d8b8..eb3687bcb45 100644 --- a/spec/models/appeal_spec.rb +++ b/spec/models/appeal_spec.rb @@ -250,15 +250,23 @@ context "#latest_attorney_case_review" do let(:appeal) { create(:appeal) } - let(:task1) { create(:ama_attorney_task, appeal: appeal) } - let(:task2) { create(:ama_attorney_task, appeal: appeal) } + let!(:task1) { create(:ama_attorney_task, appeal: appeal) } let!(:attorney_case_review1) { create(:attorney_case_review, task: task1, created_at: 2.days.ago) } - let!(:attorney_case_review2) { create(:attorney_case_review, task: task2, created_at: 1.day.ago) } + + before do + task1.update!(status: Constants.TASK_STATUSES.completed) + if appeal.tasks.open.of_type("JudgeDecisionReviewTask").any? + appeal.tasks.open.of_type("JudgeDecisionReviewTask")[0].update!(status: Constants.TASK_STATUSES.completed) + end + appeal.reload + task2 = create(:ama_attorney_task, appeal: appeal) + create(:attorney_case_review, task: task2, created_at: 1.day.ago) + end subject { appeal.latest_attorney_case_review } it "returns the latest record" do - expect(subject).to eq attorney_case_review2 + expect(subject).to_not eq attorney_case_review1 end end @@ -909,8 +917,16 @@ let!(:attorney) { create(:user) } let!(:attorney2) { create(:user) } let!(:appeal) { create(:appeal) } - let!(:task) { create(:ama_attorney_task, assigned_to: attorney, appeal: appeal, created_at: 1.day.ago) } - let!(:task2) { create(:ama_attorney_task, assigned_to: attorney2, appeal: appeal) } + let(:judge_decision_review_task) { create(:ama_judge_decision_review_task, appeal: appeal) } + let!(:task) do + create(:ama_attorney_task, parent: judge_decision_review_task, assigned_to: attorney, + appeal: appeal, created_at: 1.day.ago) + end + + before do + task.update!(status: Constants.TASK_STATUSES.completed) + create(:ama_attorney_task, parent: judge_decision_review_task, assigned_to: attorney2, appeal: appeal) + end subject { appeal.assigned_attorney } @@ -919,7 +935,7 @@ end it "should know the right assigned attorney with a cancelled task" do - task2.update(status: "cancelled") + appeal.tasks.open.of_type("AttorneyTask").last.update!(status: Constants.TASK_STATUSES.cancelled) expect(subject).to eq attorney end end From 57161941ac4c479fdd07a5027f2f0b36e59ddfda Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 9 Aug 2021 17:56:49 -0400 Subject: [PATCH 12/25] update case details spec --- spec/feature/queue/case_details_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/feature/queue/case_details_spec.rb b/spec/feature/queue/case_details_spec.rb index 3bb22fed31e..2062c93e23d 100644 --- a/spec/feature/queue/case_details_spec.rb +++ b/spec/feature/queue/case_details_spec.rb @@ -1219,7 +1219,6 @@ def wait_for_page_render ) end let!(:attorney_task) { create(:ama_attorney_task, parent: judge_task, assigned_to: user) } - let!(:attorney_task2) { create(:ama_attorney_task, parent: root_task, assigned_to: user) } before do FeatureToggle.enable!(:view_nod_date_updates) @@ -1227,7 +1226,6 @@ def wait_for_page_render # the updated_at attribute needs to be set here due to the set_timestamps hook in the task model assign_task.update!(status: Constants.TASK_STATUSES.completed, closed_at: "2019-01-01") attorney_task.update!(status: Constants.TASK_STATUSES.completed, closed_at: "2019-02-01") - attorney_task2.update!(status: Constants.TASK_STATUSES.completed, closed_at: "2019-03-01") judge_task.update!(status: Constants.TASK_STATUSES.completed, closed_at: Time.zone.now) nod_date_update.update!(updated_at: "2019-01-05") end @@ -1243,14 +1241,14 @@ def wait_for_page_render it "should sort tasks and nod date updates properly" do visit "/queue/appeals/#{appeal.uuid}" case_timeline_rows = page.find_all("table#case-timeline-table tbody tr") + # binding.pry first_row_with_date = case_timeline_rows[1] second_row_with_date = case_timeline_rows[2] third_row_with_date = case_timeline_rows[3] - fourth_row_with_date = case_timeline_rows[4] + # fourth_row_with_date = case_timeline_rows[4] expect(first_row_with_date).to have_content("01/01/2020") - expect(second_row_with_date).to have_content("03/01/2019") - expect(third_row_with_date).to have_content("02/01/2019") - expect(fourth_row_with_date).to have_content("01/05/2019") + expect(second_row_with_date).to have_content("02/01/2019") + expect(third_row_with_date).to have_content("01/05/2019") end it "should NOT display judge & attorney tasks" do From 0957ccce0ab8c7edde5da2cf2653f56d08842471 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 9 Aug 2021 18:26:17 -0400 Subject: [PATCH 13/25] override validation to maintain docket switch flow --- .../docket_switch/docket_switch_abstract_attorney_task.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb b/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb index ff1f5774735..7e2c85e9fd5 100644 --- a/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb +++ b/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb @@ -38,4 +38,11 @@ def default_actions ] end end + + private + + def only_open_task_of_type + # This overrides the validation inherited from the parent class, AttorneyTask, + # to allow the docket switch flow to remain functional + end end From c7a88d0ce34accd611cf1da22a937a811d928caf Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 9 Aug 2021 19:22:46 -0400 Subject: [PATCH 14/25] tweaks --- app/models/task.rb | 20 ++++++++++++-------- spec/feature/queue/judge_assignment_spec.rb | 1 - 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/models/task.rb b/app/models/task.rb index 4a0c6c145f8..fd34f0ef7ac 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -564,15 +564,19 @@ def verify_user_can_update!(user) def reassign(reassign_params, current_user) # We do not validate the number of tasks in this scenario because when a # task is reassigned, more than one open task of the same type must exist during the reassignment. - self.skip_check_for_only_open_task_of_type = true + @skip_check_for_only_open_task_of_type = true replacement = dup.tap do |task| - ActiveRecord::Base.transaction do - task.assigned_by_id = self.class.child_assigned_by_id(parent, current_user) - task.assigned_to = self.class.child_task_assignee(parent, reassign_params) - task.instructions = flattened_instructions(reassign_params) - task.status = Constants.TASK_STATUSES.assigned - - task.save! + begin + ActiveRecord::Base.transaction do + task.assigned_by_id = self.class.child_assigned_by_id(parent, current_user) + task.assigned_to = self.class.child_task_assignee(parent, reassign_params) + task.instructions = flattened_instructions(reassign_params) + task.status = Constants.TASK_STATUSES.assigned + + task.save! + end + ensure + @skip_check_for_only_open_task_of_type = nil end end diff --git a/spec/feature/queue/judge_assignment_spec.rb b/spec/feature/queue/judge_assignment_spec.rb index 38839e61598..76b7aa07675 100644 --- a/spec/feature/queue/judge_assignment_spec.rb +++ b/spec/feature/queue/judge_assignment_spec.rb @@ -237,7 +237,6 @@ expect(page).to have_content("Looks like this appeal already has an open JudgeDecisionReviewTask and this action cannot be completed.") # rubocop:enable Layout/LineLength - visit "/queue/#{judge_one.user.css_id}/assign" expect(page).to have_content("#{attorney_one.full_name} (0)") case_rows = page.find_all("tr[id^='table-row-']") From 3cb33e9034a008e92b07c8912ace2dc24a4e7b8f Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 9 Aug 2021 19:29:24 -0400 Subject: [PATCH 15/25] tweaks --- spec/feature/queue/case_details_spec.rb | 2 -- spec/feature/queue/judge_assignment_spec.rb | 5 ++--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/spec/feature/queue/case_details_spec.rb b/spec/feature/queue/case_details_spec.rb index 2062c93e23d..ea8da59202c 100644 --- a/spec/feature/queue/case_details_spec.rb +++ b/spec/feature/queue/case_details_spec.rb @@ -1241,11 +1241,9 @@ def wait_for_page_render it "should sort tasks and nod date updates properly" do visit "/queue/appeals/#{appeal.uuid}" case_timeline_rows = page.find_all("table#case-timeline-table tbody tr") - # binding.pry first_row_with_date = case_timeline_rows[1] second_row_with_date = case_timeline_rows[2] third_row_with_date = case_timeline_rows[3] - # fourth_row_with_date = case_timeline_rows[4] expect(first_row_with_date).to have_content("01/01/2020") expect(second_row_with_date).to have_content("02/01/2019") expect(third_row_with_date).to have_content("01/05/2019") diff --git a/spec/feature/queue/judge_assignment_spec.rb b/spec/feature/queue/judge_assignment_spec.rb index 76b7aa07675..c7e1ffa0214 100644 --- a/spec/feature/queue/judge_assignment_spec.rb +++ b/spec/feature/queue/judge_assignment_spec.rb @@ -233,9 +233,8 @@ expect(page).to have_content("#{attorney_one.full_name} (0)") expect(page).to have_content("Error assigning tasks") - # rubocop:disable Layout/LineLength - expect(page).to have_content("Looks like this appeal already has an open JudgeDecisionReviewTask and this action cannot be completed.") - # rubocop:enable Layout/LineLength + expect(page).to have_content("Looks like this appeal already has an open JudgeDecisionReviewTask "\ + "and this action cannot be completed.") visit "/queue/#{judge_one.user.css_id}/assign" expect(page).to have_content("#{attorney_one.full_name} (0)") From 3fc0fb9ca2ff82672ab846b4bb9d36b399e9f70a Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 10 Aug 2021 12:06:16 -0400 Subject: [PATCH 16/25] debug spec to actually reassign judge task to a different judge --- .../investigate_duplicate_judge_tasks_spec.rb | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index 69cc2bff353..64680dbe0ba 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -11,6 +11,12 @@ let(:judge_user) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "Anna Juarez") } let!(:judge_staff) { create(:staff, :judge_role, user: judge_user) } let(:judge_team) { JudgeTeam.create_for_judge(judge_user) } + let(:judge_user_second) do + create(:user, station_id: User::BOARD_STATION_ID, css_id: "BVAAABSHIRE", + full_name: "Aaron Judge_HearingsAndCases Abshire") + end + let!(:judge_staff_second) { create(:staff, :judge_role, user: judge_user_second) } + let(:attorney_user) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "Steven Ahr") } let!(:attorney_staff) { create(:staff, :attorney_role, user: attorney_user) } let(:attorney_user_second) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "John Lee") } @@ -23,26 +29,26 @@ scenario "Caseflow creates multiple JudgeDecisionReview and JudgeAssign tasks" do # open a window and visit case details page, window A - visit "/queue/appeals/#{appeal.uuid}" + visit "/queue/appeals/#{uuid}" appeal.reload.treee # open a second window and visit case details page, window B second_window = open_new_window within_window second_window do - visit "/queue/appeals/#{appeal.uuid}" + visit "/queue/appeals/#{uuid}" end # in window A, reassign the JudgeAssignTask to another judge first_judge_assign_task_id = appeal.tasks.select { |task| task.type == "JudgeAssignTask" }[0].id click_dropdown(prompt: "Select an action", text: "Re-assign to a judge") - click_dropdown(prompt: "Select a user", text: judge_user.full_name) + click_dropdown(prompt: "Select a user", text: judge_user_second.full_name) fill_in "taskInstructions", with: "reassign this task! teamwork makes the dreamwork!" click_on "Submit" appeal.reload.treee - expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user.full_name) + expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user_second.full_name) expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") - visit "/queue/appeals/#{appeal.uuid}" + visit "/queue/appeals/#{uuid}" # in window B, complete the JudgeAssignTask within_window second_window do @@ -55,28 +61,21 @@ expect(page).to have_content(COPY::ASSIGN_TASK_SUCCESS_MESSAGE, attorney_user.full_name) - visit "/queue/appeals/#{appeal.uuid}" + visit "/queue/appeals/#{uuid}" # Complete JudgeAssignTask to create an invalid number of AttorneyTasks - # select the second task action dropdown - assign_task_row = find("table[summary='layout table'] tr:nth-child(3)") - # select the "assign to attorney" dropdown because there were two dropdowns with this option - click_dropdown({text: "Assign to attorney"}, assign_task_row) + click_dropdown(prompt: "Select an action", text: "Assign to attorney") click_dropdown(prompt: "Select a user", text: "Other") click_dropdown(prompt: "Select a user", text: attorney_user_second.full_name) fill_in "taskInstructions", with: "mimic incorrect flow" click_on "Submit" appeal.reload.treee - # below will fail before fix is made - # app currently allows the invalid flow of a JudgeAssignTask going from cancelled to complete + # app currently allows the invalid flow of a JudgeAssignTask going from cancelled to completed expect(Task.find(first_judge_assign_task_id).status).to eq("completed") end - visit "/queue/appeals/#{appeal.uuid}" - # see what dropdowns are there - - # TODO: test that appeal has multiple JudgeDecisionReviewTasks - # TODO: test that appeal has multiple AttorneyTasks + visit "/queue/appeals/#{uuid}" + appeal.reload.treee end end end From 9c07fb9faa1565e2ffadaf94172887b0a5e49ebe Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 10 Aug 2021 16:26:37 -0400 Subject: [PATCH 17/25] refine tests in response to feedback --- spec/models/appeal_spec.rb | 8 +++----- spec/models/tasks/judge_task_spec.rb | 6 +++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/spec/models/appeal_spec.rb b/spec/models/appeal_spec.rb index eb3687bcb45..4f7b8e4c051 100644 --- a/spec/models/appeal_spec.rb +++ b/spec/models/appeal_spec.rb @@ -252,13 +252,11 @@ let(:appeal) { create(:appeal) } let!(:task1) { create(:ama_attorney_task, appeal: appeal) } let!(:attorney_case_review1) { create(:attorney_case_review, task: task1, created_at: 2.days.ago) } - - before do + let!(:attorney_case_review2) do task1.update!(status: Constants.TASK_STATUSES.completed) if appeal.tasks.open.of_type("JudgeDecisionReviewTask").any? - appeal.tasks.open.of_type("JudgeDecisionReviewTask")[0].update!(status: Constants.TASK_STATUSES.completed) + appeal.tasks.open.find_by(type: "JudgeDecisionReviewTask").completed! end - appeal.reload task2 = create(:ama_attorney_task, appeal: appeal) create(:attorney_case_review, task: task2, created_at: 1.day.ago) end @@ -266,7 +264,7 @@ subject { appeal.latest_attorney_case_review } it "returns the latest record" do - expect(subject).to_not eq attorney_case_review1 + expect(subject).to eq attorney_case_review2 end end diff --git a/spec/models/tasks/judge_task_spec.rb b/spec/models/tasks/judge_task_spec.rb index f8d1683a5b2..5f7bc6ec6c9 100644 --- a/spec/models/tasks/judge_task_spec.rb +++ b/spec/models/tasks/judge_task_spec.rb @@ -52,8 +52,8 @@ context "when the task is a judge decision review task" do let(:task) { create(:ama_judge_decision_review_task, parent: root_task) } - context "when a judge decision review task is reassigned successfully" do - it "should not violate the only_open_task_of_type validation" do + context "when the judge decision review task is reassigned successfully" do + it "does not violate the only_open_task_of_type validation" do expect { subject }.to_not raise_error end end @@ -61,7 +61,7 @@ context "when the task is a judge assign task" do let(:task) { create(:ama_judge_assign_task, parent: root_task) } - context "when a judge assign task is reassigned successfully" do + context "when the judge assign task is reassigned successfully" do it "should not violate the only_open_task_of_type validation" do expect { subject }.to_not raise_error end From e7288b3fd94dcddcc3da4d3526456c6d58b10ebf Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Tue, 10 Aug 2021 17:04:35 -0400 Subject: [PATCH 18/25] add wait statements --- spec/fixes/investigate_duplicate_judge_tasks_spec.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index 64680dbe0ba..92091e15326 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -6,8 +6,8 @@ end # Ticket: https://github.com/department-of-veterans-affairs/dsva-vacols/issues/212# - # Target state: there should be no more than 1 open JudgeAssignTask - describe "Judge reassigns JudgeAssignTask in first tab and complete task in second tab" do + # Target state: JudgeAssignTask should not change status from cancelled to completed + describe "Judge reassigns JudgeAssignTask in first tab and completes the same task in second tab" do let(:judge_user) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "Anna Juarez") } let!(:judge_staff) { create(:staff, :judge_role, user: judge_user) } let(:judge_team) { JudgeTeam.create_for_judge(judge_user) } @@ -30,12 +30,14 @@ scenario "Caseflow creates multiple JudgeDecisionReview and JudgeAssign tasks" do # open a window and visit case details page, window A visit "/queue/appeals/#{uuid}" + expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee # open a second window and visit case details page, window B second_window = open_new_window within_window second_window do visit "/queue/appeals/#{uuid}" + expect(page).to have_content(appeal.veteran.first_name, wait: 30) end # in window A, reassign the JudgeAssignTask to another judge @@ -44,6 +46,7 @@ click_dropdown(prompt: "Select a user", text: judge_user_second.full_name) fill_in "taskInstructions", with: "reassign this task! teamwork makes the dreamwork!" click_on "Submit" + expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user_second.full_name) @@ -68,6 +71,7 @@ click_dropdown(prompt: "Select a user", text: attorney_user_second.full_name) fill_in "taskInstructions", with: "mimic incorrect flow" click_on "Submit" + expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee # app currently allows the invalid flow of a JudgeAssignTask going from cancelled to completed From d9f31c3a854532eb6f555f0c6f8cd04d9be3f169 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Wed, 11 Aug 2021 11:32:17 -0400 Subject: [PATCH 19/25] continue response to feedback --- .../docket_switch/docket_switch_abstract_attorney_task.rb | 5 +++-- spec/models/appeal_spec.rb | 5 ++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb b/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb index 7e2c85e9fd5..e38fe7eadd6 100644 --- a/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb +++ b/app/models/tasks/docket_switch/docket_switch_abstract_attorney_task.rb @@ -42,7 +42,8 @@ def default_actions private def only_open_task_of_type - # This overrides the validation inherited from the parent class, AttorneyTask, - # to allow the docket switch flow to remain functional + # This overrides the validation inherited from the parent class, AttorneyTask. + # Docket switch requires an appeal to have two DocketSwitchAbstractAttorneyTasks, + # with the parent assigned to COTB and the child assigned to a user. end end diff --git a/spec/models/appeal_spec.rb b/spec/models/appeal_spec.rb index 4f7b8e4c051..0b91c8b2bd4 100644 --- a/spec/models/appeal_spec.rb +++ b/spec/models/appeal_spec.rb @@ -920,9 +920,8 @@ create(:ama_attorney_task, parent: judge_decision_review_task, assigned_to: attorney, appeal: appeal, created_at: 1.day.ago) end - - before do - task.update!(status: Constants.TASK_STATUSES.completed) + let!(:task2) do + task.completed! create(:ama_attorney_task, parent: judge_decision_review_task, assigned_to: attorney2, appeal: appeal) end From f2d9ea09bc1929de38028c4b330db3465b4b47d4 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Wed, 11 Aug 2021 12:15:53 -0400 Subject: [PATCH 20/25] respond to feedback --- spec/models/appeal_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/appeal_spec.rb b/spec/models/appeal_spec.rb index 0b91c8b2bd4..0865afe35a2 100644 --- a/spec/models/appeal_spec.rb +++ b/spec/models/appeal_spec.rb @@ -932,7 +932,7 @@ end it "should know the right assigned attorney with a cancelled task" do - appeal.tasks.open.of_type("AttorneyTask").last.update!(status: Constants.TASK_STATUSES.cancelled) + task.cancelled! expect(subject).to eq attorney end end From f57d4746a9d1f111cd6c652dd8bd5a15286ded65 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Wed, 11 Aug 2021 12:38:10 -0400 Subject: [PATCH 21/25] fix typo --- spec/models/appeal_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/appeal_spec.rb b/spec/models/appeal_spec.rb index 0865afe35a2..95dfdcdc214 100644 --- a/spec/models/appeal_spec.rb +++ b/spec/models/appeal_spec.rb @@ -932,7 +932,7 @@ end it "should know the right assigned attorney with a cancelled task" do - task.cancelled! + task2.cancelled! expect(subject).to eq attorney end end From 642e963676ed885caffca32090b270f598dcadd0 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Wed, 11 Aug 2021 11:37:45 -0400 Subject: [PATCH 22/25] respond to feedback --- spec/fixes/investigate_duplicate_judge_tasks_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index 92091e15326..8f535f41f95 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -6,7 +6,7 @@ end # Ticket: https://github.com/department-of-veterans-affairs/dsva-vacols/issues/212# - # Target state: JudgeAssignTask should not change status from cancelled to completed + # Desired Target state: JudgeAssignTask should not change status from cancelled to completed describe "Judge reassigns JudgeAssignTask in first tab and completes the same task in second tab" do let(:judge_user) { create(:user, station_id: User::BOARD_STATION_ID, full_name: "Anna Juarez") } let!(:judge_staff) { create(:staff, :judge_role, user: judge_user) } @@ -74,7 +74,7 @@ expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee - # app currently allows the invalid flow of a JudgeAssignTask going from cancelled to completed + # BUG: app currently allows the invalid flow of a JudgeAssignTask going from cancelled to completed expect(Task.find(first_judge_assign_task_id).status).to eq("completed") end From 31ecbbf90a560621e7b53806fa82a53a73aad3e5 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Fri, 13 Aug 2021 10:30:58 -0400 Subject: [PATCH 23/25] update feature test to reflect changes from fix --- spec/fixes/investigate_duplicate_judge_tasks_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index 8f535f41f95..d6b15888af3 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -41,6 +41,7 @@ end # in window A, reassign the JudgeAssignTask to another judge + binding.pry first_judge_assign_task_id = appeal.tasks.select { |task| task.type == "JudgeAssignTask" }[0].id click_dropdown(prompt: "Select an action", text: "Re-assign to a judge") click_dropdown(prompt: "Select a user", text: judge_user_second.full_name) @@ -49,6 +50,7 @@ expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee + binding.pry expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user_second.full_name) expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") visit "/queue/appeals/#{uuid}" @@ -74,8 +76,8 @@ expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee - # BUG: app currently allows the invalid flow of a JudgeAssignTask going from cancelled to completed - expect(Task.find(first_judge_assign_task_id).status).to eq("completed") + # BUG FIX: app should no longer allow the invalid flow of a JudgeAssignTask going from cancelled to completed + expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") end visit "/queue/appeals/#{uuid}" From b2f07639109b47252ec039e0e7861862e986dbc5 Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Fri, 13 Aug 2021 10:59:29 -0400 Subject: [PATCH 24/25] remove breakpoints --- spec/fixes/investigate_duplicate_judge_tasks_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index d6b15888af3..a1d66a7b279 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -41,7 +41,6 @@ end # in window A, reassign the JudgeAssignTask to another judge - binding.pry first_judge_assign_task_id = appeal.tasks.select { |task| task.type == "JudgeAssignTask" }[0].id click_dropdown(prompt: "Select an action", text: "Re-assign to a judge") click_dropdown(prompt: "Select a user", text: judge_user_second.full_name) @@ -50,7 +49,6 @@ expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee - binding.pry expect(page).to have_content(COPY::REASSIGN_TASK_SUCCESS_MESSAGE, judge_user_second.full_name) expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") visit "/queue/appeals/#{uuid}" From 8ee4a226646a751567e8712f0869ead32120b18f Mon Sep 17 00:00:00 2001 From: Eileen McFarland Date: Mon, 16 Aug 2021 16:40:51 -0400 Subject: [PATCH 25/25] change assertions for feature test --- spec/fixes/investigate_duplicate_judge_tasks_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb index a1d66a7b279..99fe57c0baa 100644 --- a/spec/fixes/investigate_duplicate_judge_tasks_spec.rb +++ b/spec/fixes/investigate_duplicate_judge_tasks_spec.rb @@ -53,7 +53,7 @@ expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") visit "/queue/appeals/#{uuid}" - # in window B, complete the JudgeAssignTask + # in window B, complete the JudgeAssignTask - should not be able to do this due to fix within_window second_window do click_dropdown(prompt: "Select an action", text: "Assign to attorney") click_dropdown(prompt: "Select a user", text: "Other") @@ -74,8 +74,11 @@ expect(page).to have_content(appeal.veteran.first_name, wait: 30) appeal.reload.treee - # BUG FIX: app should no longer allow the invalid flow of a JudgeAssignTask going from cancelled to completed - expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") + # FUTURE FIX: app should no longer allow the invalid flow of a JudgeAssignTask going from cancelled to completed + # expect(Task.find(first_judge_assign_task_id).status).to eq("cancelled") + # Successful Bug Fix: Appeal should now have only one open AttorneyTask + open_attorney_tasks = appeal.tasks.select { |task| task.type == "AttorneyTask" && task.open? } + expect(open_attorney_tasks.count).to eq(1) end visit "/queue/appeals/#{uuid}"