Validate that an appeal can have only one JudgeAssignTask active at a…

… time (#16072)

Bumps #15220 

### Description
Add a validation that ensures only one `JudgeAssignTask` can be created at a time. This functionality can be extended to other tasks where there should only be one, such as `ChangeHearingDispositionTask`.  I am leaving that work for another PR.

### Acceptance Criteria
- [x] Code compiles correctly
- [x] No existing tests break
- [x] There is test coverage for the new validation
- [x] It is still possible to reassign a judge to a case through the UI

### Testing Plan
1. Ensure that automated tests pass.
2. While running the site locally, test the reassign judge scenario
- Switch to user BVAAABSHIRE (VACO)
- Find an appeal that has a JudgeAssignTask already associated with it
- In the “currently active tasks” section, select “re-assign judge”
![ReassignJudge](https://user-images.githubusercontent.com/80347702/113054344-4d5cdf80-9177-11eb-9cde-f4f601b659c3.png)
- Ensure that you can still re-assign a judge without getting any error messages
3. Through rails console, test that a second JudgeAssignTask cannot be made for the same appeal
Find an appeal with a `JudgeAssignTask`.  Next, find a user that will be `assigned_to`, and `assigned_by`.
```
appeal = JudgeAssignTask.last.appeal
assigned_to = User.first
assigned_by = User.last
```
Watch the validation fail after attempting to make a second `JudgeAssignTask` for this appeal.
```
FactoryBot.create(:ama_judge_assign_task, assigned_to: assigned_to, assigned_by: assigned_by, appeal: appeal)
```

### Lessons Learned Along the Way
I wanted to document some of the bumps I hit while working on this PR.

The test initially failed on setup in the let block [where it creates the first JudgeAssignTask](https://github.com/department-of-veterans-affairs/caseflow/pull/16072/files#diff-3d062019a35dcde6fb082f68bce1d3ca970bfd5af401845e146b0616cdfb2000R18).  While debugging with Pry, I found that the validation was called twice when creating the first `JudgeAssignTask`.

While working on it with Yoom, he pointed out that I had written a validation which was called both when the record was inserted in the database and saved to the database. 
[Relevant documentation quote](https://guides.rubyonrails.org/active_record_validations.html#when-does-validation-happen-questionmark):
>Creating and saving a new record will send an SQL INSERT operation to the database. Updating an existing record will send an SQL UPDATE operation instead. Validations are typically run before these commands are sent to the database.

Since my custom validation ran on `update` and `create`, it also caused unrelated tests to fail on setup.  I changed the validation to only be called when the `JudgeAssignTask` task is saved to the database. Here's [helpful documentation about custom validations](https://guides.rubyonrails.org/active_record_validations.html#custom-methods).

Next, I locally tested reassigning a `JudgeAssignTask` to see if the functionality still works.  Initially, it did not work because the reassign flow is
1. create a duplicate task and assign it to another user
2. cancel old task
The on create validation made it impossible to do the above.  I [edited the `reassign` method](https://github.com/department-of-veterans-affairs/caseflow/pull/16072/files#diff-4095b65754098dce7229824782d43e80a7cb3f0fe45aa245102593760b51a24bR559-R563) so that it will allow two `JudgeAssignTask`s to simultaneously exist.  I found this [StackOverflow post](https://stackoverflow.com/questions/43105871/can-you-rescue-from-specific-errors-with-messages-in-ruby) helpful for my approach.


### Updated approach
After pairing with Yoom and Bar, I revised my original approach in favor of using thread-local variables.  You can read more about this approach in the associated hackmd document's [Section 5, Thread-local variables](https://hackmd.io/@mGFT9b9_R6WJvEipbrJIjA/rJfAr9iVd).  I want to highlight that thread-local variables use the [`Thread.current.thread_variable_set("foo", "bar")`](https://api.rubyonrails.org/v4.2/classes/Thread.html) syntax, whereas fiber-local variables use the `Thread.current["foo"] = "bar"` syntax.

app/models/task.rb

@@ -554,25 +554,38 @@ def verify_user_can_update!(user)
     end
   end
 
+  # rubocop:disable Metrics/AbcSize
   def reassign(reassign_params, current_user)
-    sibling = dup.tap do |task|
-      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!
+    # 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)
+    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 thread-local variable check_for_open_task_type
+      # does not leak outside of this method
+      ensure
+        Thread.current.thread_variable_set(:skip_check_for_only_open_task_of_type, nil)
+      end
     end
 
     # Preserve the open children and status of the old task
-    children.select(&:stays_with_reassigned_parent?).each { |child| child.update!(parent_id: sibling.id) }
-    sibling.update!(status: status)
-
+    children.select(&:stays_with_reassigned_parent?).each { |child| child.update!(parent_id: replacement.id) }
+    replacement.update!(status: status)
     update_with_instructions(status: Constants.TASK_STATUSES.cancelled, instructions: reassign_params[:instructions])
 
     appeal.overtime = false if appeal.overtime? && reassign_clears_overtime?
 
-    [sibling, self, sibling.children].flatten
+    [replacement, self, replacement.children].flatten
   end
+  # rubocop:enable Metrics/AbcSize
 
   def can_move_on_docket_switch?
     return false unless open_with_no_children?
@@ -817,6 +830,16 @@ 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
+    end
+  end
+
   def parent_can_have_children
     if parent&.class&.cannot_have_children
       fail Caseflow::Error::InvalidParentTask, message: "Child tasks cannot be created for #{parent.type}s"

app/models/tasks/judge_assign_task.rb

@@ -14,6 +14,9 @@
 # Historically, it can have AttorneyTask children, but AttorneyTasks should now be under JudgeDecisionReviewTasks.
 
 class JudgeAssignTask < JudgeTask
+  validate :only_open_task_of_type, on: :create,
+                                    unless: :skip_check_for_only_open_task_of_type?
+
   def additional_available_actions(_user)
     [Constants.TASK_ACTIONS.ASSIGN_TO_ATTORNEY.to_h]
   end

app/workflows/judge_assign_task_creator.rb

@@ -8,11 +8,11 @@ def initialize(appeal:, judge:)
 
   def call
     Rails.logger.info("Assigning judge task for appeal #{appeal.id}")
+    task = reassign_or_create
     Rails.logger.info("Assigned judge task with task id #{task.id} to #{task.assigned_to.css_id}")
-    Rails.logger.info("Closing distribution task for appeal #{appeal.id}")
 
+    Rails.logger.info("Closing distribution task for appeal #{appeal.id}")
     close_distribution_tasks_for_appeal if appeal.is_a?(Appeal)
-
     Rails.logger.info("Closed distribution task for appeal #{appeal.id}")
 
     task
@@ -22,8 +22,21 @@ def call
 
   attr_reader :appeal, :judge
 
-  def task
-    @task ||= JudgeAssignTask.create!(appeal: appeal, parent: appeal.root_task, assigned_to: judge)
+  def reassign_or_create
+    open_judge_assign_task = @appeal.tasks.open.find_by_type(:JudgeAssignTask)
+
+    return reassign_existing_open_task(open_judge_assign_task) if open_judge_assign_task
+
+    JudgeAssignTask.create!(appeal: appeal, parent: appeal.root_task, assigned_to: judge)
+  end
+
+  def reassign_existing_open_task(open_judge_assign_task)
+    new_task, _old_task, _new_children = open_judge_assign_task.reassign({
+                                                                           assigned_to_type: @judge.class.name,
+                                                                           assigned_to_id: @judge.id,
+                                                                           appeal: appeal
+                                                                         }, current_user)
+    new_task
   end
 
   def close_distribution_tasks_for_appeal

client/app/queue/PowerOfAttorneyDetail.jsx

@@ -129,7 +129,7 @@ export const PowerOfAttorneyDetailUnconnected = ({ powerOfAttorney, appealId, po
 
     if (recognizedAppellant && recognizedPoa) {
       return <PoaRefresh powerOfAttorney={poa} appealId={appealId} {...detailListStyling} />;
-    } else if (unrecognizedPoa) {
+    } else if (poa.representative_type === unrecognizedPoa) {
       return <em>{ COPY.CASE_DETAILS_UNRECOGNIZED_POA }</em>;
     }
   };

lib/caseflow/error.rb

@@ -104,6 +104,16 @@ def initialize(args)
     end
   end
 
+  class MultipleOpenTasksOfSameTypeError < SerializableError
+    def initialize(args)
+      @task_type = args[:task_type]
+      @code = args[:code] || 400
+      @title = "Error assigning tasks"
+      @message = args[:message] || "Looks like this appeal already has an open #{@task_type} and this action cannot " \
+                              "be completed."
+    end
+  end
+
   class InvalidUserId < SerializableError
     def initialize(args)
       @user_id = args[:user_id]

spec/feature/queue/ama_queue_spec.rb

@@ -527,7 +527,7 @@ def judge_assign_to_attorney
           "sent to #{judge_user.full_name} for review."
         )
 
-        expect(appeal.latest_attorney_case_review.overtime).to be(overtime)
+        expect(appeal.reload.latest_attorney_case_review.overtime).to be(overtime)
       end
 
       step "judge returns case to attorney for corrections" do

spec/fixes/investigate_duplicate_judge_assign_task_spec.rb

@@ -56,17 +56,15 @@
       click_on "Submit"
       expect(page).to have_content("case has been cancelled")
 
-      # Repeat same actions in second_window
+      # Attempt to repeat same actions in second_window.
+      # Should fail due to limit on number of open tasks of type JudgeAssignTask.
       within_window second_window do
         click_dropdown(prompt: "Select an action", text: "Cancel task and return to judge")
         fill_in(COPY::ADD_COLOCATED_TASK_INSTRUCTIONS_LABEL, with: "cancel once")
         click_on "Submit"
-        expect(page).to have_content("case has been cancelled")
+        expect(page).to have_content("Error assigning tasks")
+        expect(page).to have_content("Looks like this appeal already has an open JudgeAssignTask")
       end
-
-      # Notice there are 2 open JudgeAssignTasks
-      appeal.reload.treee
-      # binding.pry # Uncomment to examine the browser tabs
     end
   end
 end

spec/jobs/update_cached_appeals_attributes_job_spec.rb

@@ -18,8 +18,8 @@
 
     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(:bva_dispatch_task, appeal: appeal)
+        create(:ama_judge_assign_task, appeal: appeal)
       end
     end
 
@@ -125,7 +125,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(:ama_judge_assign_task, appeal: appeal)
       end
     end
 

spec/models/appeal_spec.rb

@@ -925,21 +925,30 @@
     end
 
     context ".assigned_judge" do
-      let!(:judge) { create(:user) }
-      let!(:judge2) { create(:user) }
-      let!(:appeal) { create(:appeal) }
-      let!(:task) { create(:ama_judge_assign_task, assigned_to: judge, appeal: appeal, created_at: 1.day.ago) }
+      let(:judge) { create(:user) }
+      let(:judge2) { create(:user) }
+      let(:judge3) { create(:user) }
+      let(:appeal) { create(:appeal) }
+      let!(:task) do
+        create(:ama_judge_assign_task, :cancelled, assigned_to: judge,
+                                                   appeal: appeal, created_at: 1.day.ago)
+      end
       let!(:task2) { create(:ama_judge_assign_task, assigned_to: judge2, appeal: appeal) }
-
       subject { appeal.assigned_judge }
 
-      it "returns the assigned judge for the most recent non-cancelled JudgeTask" do
-        expect(subject).to eq judge2
+      context "with one cancelled task" do
+        it "returns the assigned judge for the most recent open JudgeTask" do
+          expect(subject).to eq judge2
+        end
       end
 
-      it "should know the right assigned judge with a cancelled tasks" do
-        task2.update(status: "cancelled")
-        expect(subject).to eq judge
+      context "with multiple cancelled tasks" do
+        before { task2.cancelled! }
+        let!(:task3) { create(:ama_judge_assign_task, assigned_to: judge3, appeal: appeal, created_at: 1.day.ago) }
+
+        it "should return the assigned judge for the open task" do
+          expect(subject).to eq judge3
+        end
       end
     end
   end

spec/models/concerns/task_tree_render_module_spec.rb

@@ -120,6 +120,7 @@ def tree2(obj, *atts, **kwargs)
       initial_count = appeal.tasks.count + 3
       expect(appeal.tree.lines.count).to eq initial_count
 
+      ama_judge_assign_task.cancelled!
       create(:ama_judge_assign_task, appeal: appeal, parent: nil, created_at: 1.day.ago.round)
       appeal.reload
       expect(appeal.tree.lines.count).to eq initial_count + 1

spec/models/dockets/hearing_request_docket_spec.rb

@@ -226,7 +226,7 @@
       end
     end
 
-    context "when an appeal aleady has a distribution" do
+    context "when an appeal already has a distribution" do
       subject do
         HearingRequestDocket.new.distribute_appeals(distribution, priority: false, limit: 10, genpop: "any")
       end

spec/models/task_spec.rb

@@ -839,7 +839,9 @@
       }
     end
 
-    before { allow_any_instance_of(Organization).to receive(:user_has_access?).and_return(true) }
+    before do
+      allow_any_instance_of(Organization).to receive(:user_has_access?).and_return(true)
+    end
 
     subject { task.reassign(params, old_assignee) }
 

spec/models/tasks/judge_task_spec.rb

@@ -11,6 +11,58 @@
     create(:staff, :attorney_role, sdomainid: attorney.css_id)
   end
 
+  describe "only_open_task_of_type" do
+    let(:appeal) { create(:appeal) }
+    let!(:first_assign_task) do
+      create(:ama_judge_assign_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
+        expect { create(:ama_judge_assign_task, assigned_to: judge, appeal: appeal) }.to raise_error do |error|
+          expect(error).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
+      {
+        assigned_to_id: new_assignee.id,
+        assigned_to_type: new_assignee.class.name,
+        instructions: "instructions"
+      }
+    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
+      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"
+        }
+      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
+
   describe ".available_actions" do
     let(:user) { judge }
     let(:appeal) { create(:appeal, stream_type: stream_type) }