Only allow one active staffbot request per task (#605)

* Update the choices

* mark staffbot replace as complete based on the responses

* Add a model test

* Close open requests when sending another staff request

* Fix lint errors

* Save wip

* Change the save order

* Add tests for staff and restaff

* Fix lint errors

* Fix a lint error

* Fix tests

* Fix tests

* Count number of workers instead of inquiries

* Remove some debugging convenience

* Rename the request status enums

* Set the request status to complete when there is a winner

* Exclude COMPLETE request when we are marking a winner

* Add a test showing that you can not win when the request is completed

* Simply check_responses_complete function

* Simplify a request complete logic

* No need to mark other workers as non-winner

* Change the label complete to closed

* Update the migration

* Move the request status update logic outside of mixin

* Remove test for the removed mixin functions

* Remove print statements

* Remove unneeded won_responses check

* Apply suggestions from code review

Update comments

Co-Authored-By: Adam Marcus <marcua@marcua.net>

* When there is a winner for a task. Make sure all the requests are closed

* Leave a comment instead of save

* Remove response creation for winner with no existing response

Co-authored-by: Adam Marcus <marcua@marcua.net>

orchestra/bots/staffbot.py

@@ -12,6 +12,7 @@
 from orchestra.communication.mail import html_from_plaintext
 from orchestra.communication.mail import send_mail
 from orchestra.communication.slack import format_slack_message
+from orchestra.communication.utils import close_open_staffbot_requests
 from orchestra.core.errors import TaskAssignmentError
 from orchestra.core.errors import TaskStatusError
 from orchestra.models import CommunicationPreference
@@ -107,6 +108,7 @@ def staff(self, task_id,
                     'text': error_msg
                 }])
 
+        close_open_staffbot_requests(task)
         StaffBotRequest.objects.create(
             task=task,
             required_role_counter=required_role_counter,
@@ -170,6 +172,7 @@ def restaff(self, task_id, username,
                     'text': error_msg
                 }])
 
+        close_open_staffbot_requests(task)
         StaffBotRequest.objects.create(
             task=task,
             required_role_counter=required_role_counter,

orchestra/bots/tests/test_staffbot.py

@@ -172,6 +172,37 @@ def test_staff_command_errors(self):
                          .format(task.id,
                                  'Status incompatible with new assignment'))
 
+    @patch('orchestra.bots.staffbot.send_mail')
+    @patch('orchestra.bots.staffbot.message_experts_slack_group')
+    @patch('orchestra.bots.staffbot.StaffBot._send_staffing_request_by_slack')
+    def test_staff_close_requests(self, mock_slack,
+                                  mock_experts_slack, mock_mail):
+        """
+        Test that existing staffbot requests for a task is closed when
+        a staff function is called.
+        """
+        CLOSED = StaffBotRequest.Status.CLOSED.value
+
+        bot = StaffBot()
+        task = TaskFactory()
+        init_num_request = StaffBotRequest.objects.filter(task=task).count()
+        self.assertEqual(init_num_request, 0)
+
+        bot.staff(task.id)
+        requests = StaffBotRequest.objects.filter(task=task)
+        num_request = requests.count()
+        self.assertEqual(num_request, init_num_request + 1)
+        self.assertNotEqual(requests.last().status, CLOSED)
+
+        # Calling staff on the same task should close the previous request
+        # and create a new one.
+        bot.staff(task.id)
+        requests = list(StaffBotRequest.objects.filter(task=task))
+        num_request = len(requests)
+        self.assertEqual(num_request, init_num_request + 2)
+        self.assertEqual(requests[-2].status, CLOSED)
+        self.assertNotEqual(requests[-1].status, CLOSED)
+
     @patch('orchestra.bots.staffbot.send_mail')
     @patch('orchestra.bots.staffbot.message_experts_slack_group')
     @patch('orchestra.bots.staffbot.StaffBot._send_staffing_request_by_slack')
@@ -240,6 +271,41 @@ def test_restaff_command_errors(self):
                          (bot.task_assignment_does_not_exist_error
                           .format(worker.user.username, task.id)))
 
+    @patch('orchestra.bots.staffbot.send_mail')
+    @patch('orchestra.bots.staffbot.message_experts_slack_group')
+    @patch('orchestra.bots.staffbot.StaffBot._send_staffing_request_by_slack')
+    def test_restaff_close_requests(self, mock_slack,
+                                    mock_experts_slack, mock_mail):
+        """
+        Test that existing staffbot requests for a task is closed when
+        a staff function is called.
+        """
+        CLOSED = StaffBotRequest.Status.CLOSED.value
+
+        bot = StaffBot()
+        task = (Task.objects
+                .filter(status=Task.Status.AWAITING_PROCESSING)
+                .first())
+        task = assign_task(self.worker.id, task.id)
+
+        init_num_request = StaffBotRequest.objects.filter(task=task).count()
+        self.assertEqual(init_num_request, 0)
+
+        bot.restaff(task.id, self.worker.user.username)
+        requests = StaffBotRequest.objects.filter(task=task)
+        num_request = requests.count()
+        self.assertEqual(num_request, init_num_request + 1)
+        self.assertNotEqual(requests.last().status, CLOSED)
+
+        # Calling restaff on the same task should close the previous request
+        # and create a new one.
+        bot.restaff(task.id, self.worker.user.username)
+        requests = list(StaffBotRequest.objects.filter(task=task))
+        num_request = len(requests)
+        self.assertEqual(num_request, init_num_request + 2)
+        self.assertEqual(requests[-2].status, CLOSED)
+        self.assertNotEqual(requests[-1].status, CLOSED)
+
     @override_settings(ORCHESTRA_MOCK_EMAILS=True)
     @patch('orchestra.bots.staffbot.send_mail')
     def test_get_staffing_request_messsage(self, mock_mail):

orchestra/communication/staffing.py

@@ -57,19 +57,17 @@ def handle_staffing_response(worker, staffing_request_inquiry_id,
         if not is_available and response.is_winner:
             raise StaffingResponseException(
                 'Cannot reject after accepting the task')
-
+        # This update will be saved in re/assing_task if necessary.
         response.is_available = is_available
 
     else:
         response = StaffingResponse.objects.create(
             request_inquiry=staffing_request_inquiry,
             is_available=is_available)
 
+    request = staffing_request_inquiry.request
     if (is_available and
-            not StaffingResponse.objects.filter(
-                request_inquiry__request=staffing_request_inquiry.request,
-                is_winner=True).exists()):
-        request = staffing_request_inquiry.request
+            request.status != StaffBotRequest.Status.CLOSED.value):
         task_assignment = get_object_or_None(
             TaskAssignment,
             task=request.task,
@@ -93,13 +91,29 @@ def handle_staffing_response(worker, staffing_request_inquiry_id,
 
 
 def check_responses_complete(request):
-    # check all responses have been complete
+    inquiries = (
+        StaffingRequestInquiry.objects.filter(request=request)
+    ).distinct()
+    num_inquired_workers = len(
+        set(inquiries.values_list(
+            'communication_preference__worker__id', flat=True)
+            )
+    )
+    responded_inquiries = inquiries.filter(
+        responses__isnull=False).distinct()
+    num_responded_workers = len(
+        set(responded_inquiries.values_list(
+            'communication_preference__worker__id', flat=True)
+            )
+    )
+
     responses = StaffingResponse.objects.filter(
         request_inquiry__request=request)
-    request_inquiries = StaffingRequestInquiry.objects.filter(
-        request=request)
-    if (responses.count() == request_inquiries.count() and
+    if (num_responded_workers >= num_inquired_workers and
             not responses.filter(is_winner=True).exists()):
+        request.status = StaffBotRequest.Status.CLOSED.value
+        request.save()
+
         # notify that all workers have rejected a task
         message_experts_slack_group(
             request.task.project.slack_group_id,
@@ -114,7 +128,7 @@ def send_staffing_requests(
     cutoff_datetime = timezone.now() - frequency
     requests = (
         StaffBotRequest.objects
-        .filter(status=StaffBotRequest.Status.PROCESSING.value)
+        .filter(status=StaffBotRequest.Status.SENDING_INQUIRIES.value)
         .filter(Q(last_inquiry_sent__isnull=True) |
                 Q(last_inquiry_sent__lte=cutoff_datetime)))
 
@@ -156,7 +170,7 @@ def _send_request_inquiries(staffbot, request, worker_batch_size,
             request.task.project.slack_group_id,
             ('All staffing requests for task {} have been sent!'
              .format(request.task)))
-        request.status = StaffBotRequest.Status.COMPLETE.value
+        request.status = StaffBotRequest.Status.DONE_SENDING_INQUIRIES.value
     request.last_inquiry_sent = timezone.now()
     request.save()
 
@@ -186,15 +200,14 @@ def send_request_inquiries(staffbot, request, worker_batch_size):
 def get_available_requests(worker):
     # We want to show a worker only requests for which there is no
     # winner or for which they have not already replied.
-    won_responses = StaffingResponse.objects.filter(is_winner=True)
     worker_provided_responses = StaffingResponse.objects.filter(
         request_inquiry__communication_preference__worker=worker)
     remaining_requests = (
         StaffBotRequest.objects
         .filter(inquiries__communication_preference__worker=worker)
+        .exclude(status=StaffBotRequest.Status.CLOSED.value)
         .exclude(task__status=Task.Status.COMPLETE)
         .exclude(task__status=Task.Status.ABORTED)
-        .exclude(inquiries__responses__in=won_responses)
         .exclude(inquiries__responses__in=worker_provided_responses)
         .distinct())
     inquiries = (