Merge pull request #35047 from dimagi/em/repeat-record-report-bulk-ac…

…tions-bugfix

Fix Bulk Actions on Repeat Records Report

corehq/apps/data_interfaces/tasks.py

@@ -20,6 +20,7 @@
 from corehq.apps.users.models import CouchUser
 from corehq.form_processor.models import XFormInstance
 from corehq.apps.case_importer.do_import import SubmitCaseBlockHandler, RowAndCase
+from corehq.motech.repeaters.const import State
 from corehq.motech.repeaters.models import RepeatRecord
 from corehq.sql_db.util import get_db_aliases_for_partitioned_query
 from corehq.toggles import DISABLE_CASE_UPDATE_RULE_SCHEDULED_TASK
@@ -212,29 +213,16 @@ def task_generate_ids_and_operate_on_payloads(
     payload_id: Optional[str],
     repeater_id: Optional[str],
     domain: str,
-    action,  # type: Literal['resend', 'cancel', 'requeue']  # 3.8+
+    action: Literal['resend', 'cancel', 'requeue'],
+    state: Literal[None, State.Pending, State.Cancelled] = None,
 ) -> dict:
-    repeat_record_ids = _get_repeat_record_ids(payload_id, repeater_id, domain)
+    repeat_record_ids = RepeatRecord.objects.get_repeat_record_ids(
+        domain, repeater_id=repeater_id, state=state, payload_id=payload_id,
+    )
     return operate_on_payloads(repeat_record_ids, domain, action,
                                task=task_generate_ids_and_operate_on_payloads)
 
 
-def _get_repeat_record_ids(payload_id, repeater_id, domain):
-    if payload_id:
-        queryset = RepeatRecord.objects.filter(
-            domain=domain,
-            payload_id=payload_id,
-        )
-    elif repeater_id:
-        queryset = RepeatRecord.objects.filter(
-            domain=domain,
-            repeater__id=repeater_id,
-        )
-    else:
-        return []
-    return list(queryset.order_by().values_list("id", flat=True))
-
-
 @task
 def bulk_case_reassign_async(domain, user_id, owner_id, download_id, report_url):
     task = bulk_case_reassign_async

corehq/apps/data_interfaces/tests/test_tasks.py

@@ -1,4 +1,4 @@
-from unittest.case import TestCase
+from django.test import SimpleTestCase
 from unittest.mock import patch
 
 from corehq.apps.data_interfaces.tasks import (
@@ -7,7 +7,7 @@
 )
 
 
-class TestTasks(TestCase):
+class TestTasks(SimpleTestCase):
 
     @patch('corehq.apps.data_interfaces.utils.DownloadBase')
     @patch('corehq.apps.data_interfaces.utils._get_sql_repeat_record')
@@ -43,13 +43,14 @@ def test_task_operate_on_payloads_no_payload_ids(self):
 
     @patch('corehq.apps.data_interfaces.utils.DownloadBase')
     @patch('corehq.apps.data_interfaces.utils._get_sql_repeat_record')
-    @patch('corehq.apps.data_interfaces.tasks._get_repeat_record_ids')
+    @patch('corehq.apps.data_interfaces.tasks.RepeatRecord.objects.get_repeat_record_ids')
     def test_task_generate_ids_and_operate_on_payloads_no_action(
         self,
         get_repeat_record_ids_mock,
         unused_1,
         unused_2,
     ):
+
         get_repeat_record_ids_mock.return_value = ['c0ffee', 'deadbeef']
         response = task_generate_ids_and_operate_on_payloads(
             payload_id='c0ffee',
@@ -70,7 +71,9 @@ def test_task_generate_ids_and_operate_on_payloads_no_action(
             }
         })
 
-    def test_task_generate_ids_and_operate_on_payloads_no_data(self):
+    @patch('corehq.apps.data_interfaces.tasks.RepeatRecord.objects.get_repeat_record_ids')
+    def test_task_generate_ids_and_operate_on_payloads_no_data(self, get_repeat_record_ids_mock):
+        get_repeat_record_ids_mock.return_value = []
         response = task_generate_ids_and_operate_on_payloads(
             payload_id=None,
             repeater_id=None,

corehq/apps/data_interfaces/tests/test_utils.py

@@ -1,70 +1,12 @@
-from datetime import datetime
 from unittest.mock import Mock, patch
-from uuid import uuid4
 
-from django.test import SimpleTestCase, TestCase
+from django.test import TestCase
 
-from corehq.apps.data_interfaces.tasks import (
-    _get_repeat_record_ids,
-    task_generate_ids_and_operate_on_payloads,
-)
-from corehq.apps.data_interfaces.utils import (
-    _get_sql_repeat_record,
-    operate_on_payloads,
-)
-from corehq.motech.models import ConnectionSettings
-from corehq.motech.repeaters.models import FormRepeater, RepeatRecord
+from corehq.apps.data_interfaces.tasks import task_generate_ids_and_operate_on_payloads
+from corehq.apps.data_interfaces.utils import operate_on_payloads
 
-DOMAIN = 'test-domain'
-
-
-class TestUtils(SimpleTestCase):
-
-    def test__get_ids_no_data(self):
-        response = _get_repeat_record_ids(None, None, 'test_domain')
-        self.assertEqual(response, [])
-
-    @patch('corehq.apps.data_interfaces.tasks.RepeatRecord.objects.filter')
-    def test__get_ids_payload_id_in_data(self, get_by_payload_id):
-        payload_id = Mock()
-        _get_repeat_record_ids(payload_id, None, 'test_domain')
-
-        self.assertEqual(get_by_payload_id.call_count, 1)
-        get_by_payload_id.assert_called_with(domain='test_domain', payload_id=payload_id)
-
-    @patch('corehq.apps.data_interfaces.tasks.RepeatRecord.objects.filter')
-    def test__get_ids_payload_id_not_in_data(self, iter_by_repeater):
-        REPEATER_ID = 'c0ffee'
-        _get_repeat_record_ids(None, REPEATER_ID, 'test_domain')
-
-        iter_by_repeater.assert_called_with(domain='test_domain', repeater__id=REPEATER_ID)
-        self.assertEqual(iter_by_repeater.call_count, 1)
 
-    @patch('corehq.motech.repeaters.models.RepeatRecord.objects')
-    def test__validate_record_record_does_not_exist(self, mock_objects):
-        mock_objects.get.side_effect = [RepeatRecord.DoesNotExist]
-        response = _get_sql_repeat_record('test_domain', '1234')
-
-        mock_objects.get.assert_called_once_with(domain='test_domain', id='1234')
-        self.assertIsNone(response)
-
-    @patch('corehq.motech.repeaters.models.RepeatRecord.objects')
-    def test__validate_record_invalid_domain(self, mock_objects):
-        mock_objects.get.side_effect = RepeatRecord.DoesNotExist
-        response = _get_sql_repeat_record('test_domain', '1234')
-
-        mock_objects.get.assert_called_once_with(domain='test_domain', id='1234')
-        self.assertIsNone(response)
-
-    @patch('corehq.motech.repeaters.models.RepeatRecord.objects')
-    def test__validate_record_success(self, mock_objects):
-        mock_record = Mock()
-        mock_record.domain = 'test_domain'
-        mock_objects.get.return_value = mock_record
-        response = _get_sql_repeat_record('test_domain', '1234')
-
-        mock_objects.get.assert_called_once_with(domain='test_domain', id='1234')
-        self.assertEqual(response, mock_record)
+DOMAIN = 'test-domain'
 
 
 class TestTasks(TestCase):
@@ -75,22 +17,22 @@ def setUp(self):
         self.mock_payload_ids = [self.mock_payload_one.id,
                                  self.mock_payload_two.id]
 
-    @patch('corehq.apps.data_interfaces.tasks._get_repeat_record_ids')
+    @patch('corehq.apps.data_interfaces.tasks.RepeatRecord.objects.get_repeat_record_ids')
     @patch('corehq.apps.data_interfaces.tasks.operate_on_payloads')
     def test_generate_ids_and_operate_on_payloads_success(
         self,
         mock_operate_on_payloads,
-        mock__get_ids,
+        mock_get_repeat_record_ids,
     ):
-        mock__get_ids.return_value = record_ids = [1, 2, 3]
+        mock_get_repeat_record_ids.return_value = record_ids = [1, 2, 3]
         payload_id = 'c0ffee'
         repeater_id = 'deadbeef'
         task_generate_ids_and_operate_on_payloads(
             payload_id, repeater_id, 'test_domain', 'test_action')
 
-        mock__get_ids.assert_called_once()
-        mock__get_ids.assert_called_with(
-            'c0ffee', 'deadbeef', 'test_domain')
+        mock_get_repeat_record_ids.assert_called_once()
+        mock_get_repeat_record_ids.assert_called_with(
+            'test_domain', repeater_id='deadbeef', state=None, payload_id='c0ffee')
 
         mock_operate_on_payloads.assert_called_once()
         mock_operate_on_payloads.assert_called_with(
@@ -469,43 +411,3 @@ def _check_requeue(self, mock_payload_one, mock_payload_two,
         self.assertEqual(mock_payload_one.requeue.call_count, 1)
         self.assertEqual(mock_payload_two.requeue.call_count, 0)
         self.assertEqual(response, expected_response)
-
-
-class TestGetRepeatRecordIDs(TestCase):
-
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        cls.instance_id = str(uuid4())
-        url = 'https://www.example.com/api/'
-        conn = ConnectionSettings.objects.create(domain=DOMAIN, name=url, url=url)
-        cls.repeater = FormRepeater(
-            domain=DOMAIN,
-            connection_settings_id=conn.id,
-            include_app_id_param=False,
-        )
-        cls.repeater.save()
-        cls.create_repeat_records()
-
-    @classmethod
-    def create_repeat_records(cls):
-        now = datetime.now()
-        cls.sql_records = [RepeatRecord(
-            domain=DOMAIN,
-            repeater_id=cls.repeater.id,
-            payload_id=cls.instance_id,
-            registered_at=now,
-        ) for __ in range(3)]
-        RepeatRecord.objects.bulk_create(cls.sql_records)
-
-    def test_no_payload_id_no_repeater_id_sql(self):
-        result = _get_repeat_record_ids(payload_id=None, repeater_id=None, domain=DOMAIN)
-        self.assertEqual(result, [])
-
-    def test_payload_id_sql(self):
-        result = _get_repeat_record_ids(payload_id=self.instance_id, repeater_id=None, domain=DOMAIN)
-        self.assertEqual(set(result), {r.pk for r in self.sql_records})
-
-    def test_repeater_id_sql(self):
-        result = _get_repeat_record_ids(payload_id=None, repeater_id=self.repeater.repeater_id, domain=DOMAIN)
-        self.assertEqual(set(result), {r.pk for r in self.sql_records})

corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/javascript/repeaters/js/repeat_record_report.js.diff.txt

@@ -1,36 +1,37 @@
 --- 
 +++ 
-@@ -1,5 +1,5 @@
+@@ -1,6 +1,6 @@
  /* globals ace */
+ 'use strict';
 -hqDefine('repeaters/js/bootstrap3/repeat_record_report', function () {
 +hqDefine('repeaters/js/bootstrap5/repeat_record_report', function () {
-     var initialPageData = hqImport("hqwebapp/js/initial_page_data"),
+     const initialPageData = hqImport("hqwebapp/js/initial_page_data"),
          selectAll = document.getElementById('select-all'),
-         cancelAll = document.getElementById('cancel-all'),
-@@ -138,7 +138,7 @@
-         $('#confirm-button').on('click', function () {
-             var itemsToSend = getCheckboxes(), action = getAction(), $btn;
+         selectPending = document.getElementById('select-pending'),
+@@ -142,7 +142,7 @@
+                 action = getAction();
+             let $btn;
 
 -            $popUp.modal('hide');
 +            $popUp.modal('hide');  /* todo B5: plugin:modal */
-             if (action == 'resend') {
+             if (action === 'resend') {
                  $btn = $('#resend-all-button');
                  $btn.disableButton();
-@@ -159,7 +159,7 @@
-                 if (isActionPossibleForCheckedItems(action)) {
-                     $('#warning').addClass('hide');
-                     $('#not-allowed').addClass('hide');
+@@ -167,7 +167,7 @@
+                 // leaving as is to preserve behavior
+                 if (isActionPossibleForCheckedItems(action, checkedRecords)) {
+                     hideAllWarnings();
 -                    $popUp.modal('show');
 +                    $popUp.modal('show');  /* todo B5: plugin:modal */
                  } else {
-                     $('#warning').addClass('hide');
-                     $('#not-allowed').removeClass('hide');
-@@ -244,7 +244,7 @@
+                     showWarning('not-allowed');
+                 }
+@@ -240,7 +240,7 @@
                      } else {
                          btn.text(gettext('Failed'));
                          btn.addClass('btn-danger');
 -                        $('#payload-error-modal').modal('show');
 +                        $('#payload-error-modal').modal('show');  /* todo B5: plugin:modal */
-                         $('#payload-error-modal .error-message').text(data.failure_reason);
+                         $('#payload-error-modal .error-message').text(response.failure_reason);
                      }
                  },

corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/repeaters/repeat_record_report.html.diff.txt

@@ -50,29 +50,29 @@
                {% trans 'Requeue' %}
              </button>
 -            <label class="checkbox" >
--              <input type="checkbox" id="select-all" data-id="{{ form_query_string|urlencode }}">
+-              <input type="checkbox" id="select-all">
 +            <label class="checkbox" >  {# todo B5: css:checkbox #}
-+              <input type="checkbox" id="select-all" data-id="{{ form_query_string|urlencode }}">  {# todo B5: css:checkbox #}
++              <input type="checkbox" id="select-all">  {# todo B5: css:checkbox #}
                  {% blocktrans %}Select all {{ total }} payloads{% endblocktrans %}
              </label>
 -            <label class="checkbox" >
--              <input type="checkbox" id="cancel-all" data-id="{{ form_query_string_pending|urlencode }}">
+-              <input type="checkbox" id="select-pending">
 +            <label class="checkbox" >  {# todo B5: css:checkbox #}
-+              <input type="checkbox" id="cancel-all" data-id="{{ form_query_string_pending|urlencode }}">  {# todo B5: css:checkbox #}
++              <input type="checkbox" id="select-pending">  {# todo B5: css:checkbox #}
                  {% blocktrans %}Select {{ total_pending }} pending payloads{% endblocktrans %}
              </label>
 -            <label class="checkbox" >
--              <input type="checkbox" id="requeue-all" data-id="{{ form_query_string_cancelled|urlencode }}">
+-              <input type="checkbox" id="select-cancelled">
 +            <label class="checkbox" >  {# todo B5: css:checkbox #}
-+              <input type="checkbox" id="requeue-all" data-id="{{ form_query_string_cancelled|urlencode }}">  {# todo B5: css:checkbox #}
++              <input type="checkbox" id="select-cancelled">  {# todo B5: css:checkbox #}
                  {% blocktrans %}Select {{ total_cancelled }} cancelled payloads{% endblocktrans %}
              </label>
            </div>
          </div>
        {% endif %}
    </div>
--  <div id="warning" class="alert alert-warning hide">
-+  <div id="warning" class="alert alert-warning d-none">
+-  <div id="no-selection" class="alert alert-warning hide">
++  <div id="no-selection" class="alert alert-warning d-none">
      {% blocktrans %}Please select at least one payload{% endblocktrans %}
    </div>
 -  <div id="not-allowed" class="alert alert-warning hide">
@@ -98,7 +98,7 @@
              {% blocktrans %}No{% endblocktrans %}
            </button>
            <button type="button" id="confirm-button" data-action="" data-flag="" class="btn btn-primary">
-@@ -98,8 +98,8 @@
+@@ -100,8 +100,8 @@
      <div class="modal fade full-screen-modal" id="view-record-payload-modal" tabindex="-1" role="dialog">
          <div class="modal-dialog" role="document">
              <div class="modal-content">
@@ -109,7 +109,7 @@
                      </button>
                      <h4 class="modal-title">{% trans "Payload" %}</h4>
                  </div>
-@@ -107,7 +107,7 @@
+@@ -109,7 +109,7 @@
                      <div class="payload"></div>
                  </div>
                  <div class="modal-footer">
@@ -118,7 +118,7 @@
                      {% trans "Close" %}
                      </button>
                  </div>
-@@ -119,8 +119,8 @@
+@@ -121,8 +121,8 @@
      <div class="modal fade" id="payload-error-modal" tabindex="-1" role="dialog">
          <div class="modal-dialog" role="document">
              <div class="modal-content">
@@ -127,9 +127,9 @@
 +                <div class="modal-header">  {# todo B5: css:modal-header #}
 +                    <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span>  {# todo B5: css:close #}
                      </button>
-                     <h4 class="modal-title">{% trans "Payload Error" %}</h4>
+                     <h4 class="modal-title">{% trans "Error" %}</h4>
                  </div>
-@@ -128,7 +128,7 @@
+@@ -130,7 +130,7 @@
                      <div class="error-message"></div>
                  </div>
                  <div class="modal-footer">

corehq/motech/repeaters/exceptions.py

@@ -23,3 +23,7 @@ def __init__(self, repeater_type):
 
     def __str__(self):
         return self.message
+
+
+class BulkActionMissingParameters(Exception):
+    pass

corehq/motech/repeaters/models.py

@@ -973,6 +973,17 @@ def iter_partition(self, start, partition, total_partitions):
     def get_domains_with_records(self):
         return self.order_by().values_list("domain", flat=True).distinct()
 
+    def get_repeat_record_ids(self, domain, repeater_id=None, state=None, payload_id=None):
+        where = models.Q(domain=domain)
+        if repeater_id:
+            where &= models.Q(repeater__id=repeater_id)
+        if state:
+            where &= models.Q(state=state)
+        if payload_id:
+            where &= models.Q(payload_id=payload_id)
+
+        return list(self.filter(where).order_by().values_list("id", flat=True))
+
 
 class RepeatRecord(models.Model):
     domain = models.CharField(max_length=126)