From b4b4133491c5002b9647a0ccbc098e779df8061c Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 22 Oct 2024 10:27:54 +0200 Subject: [PATCH 1/7] always return the Django model from `payload_doc` and not the serialized JSON --- .../expression/repeater_generators.py | 23 +++---- .../motech/repeaters/expression/repeaters.py | 6 +- corehq/motech/repeaters/expression/tests.py | 62 ++++++++++++++++--- 3 files changed, 63 insertions(+), 28 deletions(-) diff --git a/corehq/motech/repeaters/expression/repeater_generators.py b/corehq/motech/repeaters/expression/repeater_generators.py index 138ebe8c70aa..cddf995b582a 100644 --- a/corehq/motech/repeaters/expression/repeater_generators.py +++ b/corehq/motech/repeaters/expression/repeater_generators.py @@ -16,15 +16,20 @@ def content_type(self): return 'application/json' def get_payload(self, repeat_record, payload_doc, parsed_expression): - result = parsed_expression(payload_doc, EvaluationContext(payload_doc)) + result = self._parse_payload(payload_doc, parsed_expression) return json.dumps(result, cls=DjangoJSONEncoder) + def _parse_payload(self, payload_doc, parsed_expression): + payload_doc_json = payload_doc.to_json() + return parsed_expression(payload_doc_json, EvaluationContext(payload_doc_json)) + def get_url(self, repeat_record, url_template, payload_doc): if not toggles.UCR_EXPRESSION_REGISTRY.enabled(repeat_record.domain): return "" required_template_vars = [fn for _, fn, _, _ in Formatter().parse(url_template) if fn is not None] - context = EvaluationContext(payload_doc) + payload_doc_json = payload_doc.to_json() + context = EvaluationContext(payload_doc_json) expressions = { expression.name: expression.wrapped_definition(context) for expression in UCRExpression.objects.filter( @@ -35,23 +40,13 @@ def get_url(self, repeat_record, url_template, payload_doc): } return url_template.format( **{ - template_var: expressions[template_var](payload_doc) if template_var in expressions else "" + template_var: expressions[template_var](payload_doc_json) if template_var in expressions else "" for template_var in required_template_vars } ) -class FormExpressionPayloadGenerator(ExpressionPayloadGenerator): - def get_payload(self, repeat_record, payload_doc, parsed_expression): - result = self._parse_payload(payload_doc, parsed_expression) - return json.dumps(result, cls=DjangoJSONEncoder) - - def _parse_payload(self, payload_doc, parsed_expression): - payload_doc_json = payload_doc.to_json() - return parsed_expression(payload_doc_json, EvaluationContext(payload_doc_json)) - - -class ArcGISFormExpressionPayloadGenerator(FormExpressionPayloadGenerator): +class ArcGISFormExpressionPayloadGenerator(ExpressionPayloadGenerator): def get_url(self, repeat_record, url_template, payload_doc): if not ( diff --git a/corehq/motech/repeaters/expression/repeaters.py b/corehq/motech/repeaters/expression/repeaters.py index 73169bd9023e..438163bfb6bb 100644 --- a/corehq/motech/repeaters/expression/repeaters.py +++ b/corehq/motech/repeaters/expression/repeaters.py @@ -22,7 +22,6 @@ from corehq.motech.repeaters.expression.repeater_generators import ( ArcGISFormExpressionPayloadGenerator, ExpressionPayloadGenerator, - FormExpressionPayloadGenerator, ) from corehq.motech.repeaters.models import ( OptionValue, @@ -156,7 +155,7 @@ def form_class_name(self): @memoized def payload_doc(self, repeat_record): - return CommCareCase.objects.get_case(repeat_record.payload_id, repeat_record.domain).to_json() + return CommCareCase.objects.get_case(repeat_record.payload_id, repeat_record.domain) def allowed_to_forward(self, payload): allowed = super().allowed_to_forward(payload) @@ -183,7 +182,6 @@ def allowed_to_forward(self, payload): class FormExpressionRepeater(BaseExpressionRepeater): friendly_name = _("Configurable Form Repeater") - payload_generator_classes = (FormExpressionPayloadGenerator,) class Meta: app_label = 'repeaters' @@ -282,7 +280,7 @@ def get_evaluation_context(domain, repeat_record, payload_doc, response): 'success': is_success_response(response), 'payload': { 'id': repeat_record.payload_id, - 'doc': payload_doc, + 'doc': payload_doc.to_json(), }, 'response': { 'status_code': response.status_code, diff --git a/corehq/motech/repeaters/expression/tests.py b/corehq/motech/repeaters/expression/tests.py index d7821456b1fc..80367dfe5628 100644 --- a/corehq/motech/repeaters/expression/tests.py +++ b/corehq/motech/repeaters/expression/tests.py @@ -315,8 +315,6 @@ class FormExpressionRepeaterTest(BaseExpressionRepeaterTest): """ - case_id = uuid.uuid4().hex - def _create_repeater(self): self.repeater = FormExpressionRepeater( domain=self.domain, @@ -351,10 +349,9 @@ def _create_repeater(self): ) self.repeater.save() - @property - def expected_payload(self): + def expected_payload(self, case_id): return json.dumps({ - 'case_id': self.case_id, + 'case_id': case_id, 'properties': { 'meta_gps_point': '1.1 2.2 3.3 4.4' } @@ -369,14 +366,59 @@ def test_filter_forms(self): def test_payload(self): instance_id = uuid.uuid4().hex + case_id = uuid.uuid4().hex xform_xml = self.xform_xml_template.format( self.xmlns, instance_id, - self._create_case_block(self.case_id), + self._create_case_block(case_id), ) submit_form_locally(xform_xml, self.domain) repeat_record = self.repeat_records(self.domain).all()[0] - self.assertEqual(repeat_record.get_payload(), self.expected_payload) + self.assertEqual(repeat_record.get_payload(), self.expected_payload(case_id)) + + def test_process_response(self): + instance_id = uuid.uuid4().hex + case_id = uuid.uuid4().hex + xform_xml = self.xform_xml_template.format( + self.xmlns, + instance_id, + self._create_case_block(case_id), + ) + submit_form_locally(xform_xml, self.domain) + repeat_record = self.repeat_records(self.domain).all()[0] + self.repeater.case_action_filter_expression = { + "type": "boolean_expression", + "expression": { + "type": "jsonpath", + "jsonpath": "response.status_code", + }, + "operator": "eq", + "property_value": 200, + } + self.repeater.case_action_expression = { + 'type': 'dict', + 'properties': { + 'create': False, + 'case_id': { + 'type': 'jsonpath', + 'jsonpath': 'payload.doc.form.case.@case_id', + }, + 'properties': { + 'type': 'dict', + 'properties': { + 'type': 'dict', + 'prop_from_response': { + 'type': 'jsonpath', + 'jsonpath': 'response.body.aValue', + } + } + } + } + } + response = MockResponse(200, '{"aValue": "aResponseValue"}') + self.repeater.handle_response(response, repeat_record) + case = CommCareCase.objects.get_case(case_id, self.domain) + self.assertEqual(case.get_case_property('prop_from_response'), 'aResponseValue') class ArcGISExpressionRepeaterTest(FormExpressionRepeaterTest): @@ -456,8 +498,7 @@ def _create_repeater(self): ) self.repeater.save() - @property - def expected_payload(self): + def expected_payload(self, case_id): return { 'features': json.dumps([{ 'attributes': { @@ -476,10 +517,11 @@ def expected_payload(self): } def test_send_request_error_handling(self): + case_id = uuid.uuid4().hex xform_xml = self.xform_xml_template.format( self.xmlns, uuid.uuid4().hex, - self._create_case_block(self.case_id), + self._create_case_block(case_id), ) with patch( 'corehq.motech.repeaters.models.simple_request', From 582fb11d4e458ea873d30ace4fe13765c7808cb6 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 22 Oct 2024 10:35:25 +0200 Subject: [PATCH 2/7] fix and test `ArcGISFormExpressionPayloadGenerator.get_url` --- .../expression/repeater_generators.py | 6 +-- corehq/motech/repeaters/expression/tests.py | 40 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/corehq/motech/repeaters/expression/repeater_generators.py b/corehq/motech/repeaters/expression/repeater_generators.py index cddf995b582a..1e0d93f1c1b2 100644 --- a/corehq/motech/repeaters/expression/repeater_generators.py +++ b/corehq/motech/repeaters/expression/repeater_generators.py @@ -49,11 +49,9 @@ def get_url(self, repeat_record, url_template, payload_doc): class ArcGISFormExpressionPayloadGenerator(ExpressionPayloadGenerator): def get_url(self, repeat_record, url_template, payload_doc): - if not ( - toggles.UCR_EXPRESSION_REGISTRY.enabled(repeat_record.domain) - and toggles.ARCGIS_INTEGRATION.enabled(repeat_record.domain) - ): + if not toggles.ARCGIS_INTEGRATION.enabled(repeat_record.domain): return "" + return super().get_url(repeat_record, url_template, payload_doc) @property def content_type(self): diff --git a/corehq/motech/repeaters/expression/tests.py b/corehq/motech/repeaters/expression/tests.py index 80367dfe5628..3a9ec1ad9646 100644 --- a/corehq/motech/repeaters/expression/tests.py +++ b/corehq/motech/repeaters/expression/tests.py @@ -594,6 +594,46 @@ def test_error_response_with_empty_error(self): self.assertEqual(resp.reason, "[No error message given by ArcGIS]") self.assertEqual(resp.text, "") + @flag_enabled("UCR_EXPRESSION_REGISTRY") + @flag_enabled("ARCGIS_INTEGRATION") + def test_custom_url(self): + self.repeater.url_template = "/{variable1}/a_thing/delete?case_id={case_id}&{missing_variable}='foo'" + + UCRExpression.objects.create( + name='variable1', + domain=self.domain, + expression_type="named_expression", + definition={ + "type": "jsonpath", + "jsonpath": "form.person_name" + }, + ) + UCRExpression.objects.create( + name='case_id', + domain=self.domain, + expression_type="named_expression", + definition={ + "type": "jsonpath", + "jsonpath": "form.case.@case_id" + }, + ) + + case_id = uuid.uuid4().hex + xform_xml = self.xform_xml_template.format( + self.xmlns, + uuid.uuid4().hex, + self._create_case_block(case_id), + ) + submit_form_locally(xform_xml, self.domain) + repeat_record = self.repeat_records(self.domain).all()[0] + + expected_url = self.connection.url + f"/Timmy/a_thing/delete?case_id={case_id}&='foo'" + + self.assertEqual( + self.repeater.get_url(repeat_record), + expected_url + ) + def test_doctests(): import corehq.motech.repeaters.expression.repeaters as module From 3d38fc4740f5f225b3420b4ffd336b1ef0725c2c Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 22 Oct 2024 10:37:31 +0200 Subject: [PATCH 3/7] rename method, move it to function --- .../repeaters/expression/repeater_generators.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/corehq/motech/repeaters/expression/repeater_generators.py b/corehq/motech/repeaters/expression/repeater_generators.py index 1e0d93f1c1b2..f54773e2c36e 100644 --- a/corehq/motech/repeaters/expression/repeater_generators.py +++ b/corehq/motech/repeaters/expression/repeater_generators.py @@ -16,13 +16,9 @@ def content_type(self): return 'application/json' def get_payload(self, repeat_record, payload_doc, parsed_expression): - result = self._parse_payload(payload_doc, parsed_expression) + result = _generate_payload(payload_doc, parsed_expression) return json.dumps(result, cls=DjangoJSONEncoder) - def _parse_payload(self, payload_doc, parsed_expression): - payload_doc_json = payload_doc.to_json() - return parsed_expression(payload_doc_json, EvaluationContext(payload_doc_json)) - def get_url(self, repeat_record, url_template, payload_doc): if not toggles.UCR_EXPRESSION_REGISTRY.enabled(repeat_record.domain): return "" @@ -58,7 +54,7 @@ def content_type(self): return 'application/x-www-form-urlencoded' def get_payload(self, repeat_record, payload_doc, parsed_expression): - payload = self._parse_payload(payload_doc, parsed_expression) + payload = _generate_payload(payload_doc, parsed_expression) conn_settings = repeat_record.repeater.connection_settings api_token = conn_settings.plaintext_password formatted_payload = { @@ -67,3 +63,8 @@ def get_payload(self, repeat_record, payload_doc, parsed_expression): 'token': api_token, } return formatted_payload + + +def _generate_payload(payload_doc, parsed_expression): + payload_doc_json = payload_doc.to_json() + return parsed_expression(payload_doc_json, EvaluationContext(payload_doc_json)) From f9f0fb138814e8201504321ecf41bd53a0f868fb Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 22 Oct 2024 11:35:35 +0200 Subject: [PATCH 4/7] update attempt message when form is submitted from response --- .../motech/repeaters/expression/repeaters.py | 16 ++++++---- corehq/motech/repeaters/expression/tests.py | 8 +++-- corehq/motech/repeaters/models.py | 31 ++++++++++--------- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/corehq/motech/repeaters/expression/repeaters.py b/corehq/motech/repeaters/expression/repeaters.py index 438163bfb6bb..d3b746eda435 100644 --- a/corehq/motech/repeaters/expression/repeaters.py +++ b/corehq/motech/repeaters/expression/repeaters.py @@ -107,27 +107,30 @@ def get_url(self, repeat_record): return base_url def handle_response(self, response, repeat_record): - super().handle_response(response, repeat_record) + attempt = super().handle_response(response, repeat_record) if self.case_action_filter_expression and is_response(response): try: - self._process_response_as_case_update(response, repeat_record) + message = self._process_response_as_case_update(response, repeat_record) except Exception as e: notify_exception(None, "Error processing response from Repeater request", e) + else: + attempt.message += f"\n\n{message}" + attempt.save() def _process_response_as_case_update(self, response, repeat_record): domain = repeat_record.domain context = get_evaluation_context(domain, repeat_record, self.payload_doc(repeat_record), response) if not self.parsed_case_action_filter(context.root_doc, context): - return False + return "Response did not match filter" - self._perform_case_update(domain, context) - return True + form_id = self._perform_case_update(domain, context) + return f"Response generated a form: {form_id}" def _perform_case_update(self, domain, context): data = self.parsed_case_action_expression(context.root_doc, context) if data: data = data if isinstance(data, list) else [data] - handle_case_update( + form, _ = handle_case_update( domain=domain, data=data, user=UserDuck('system', ''), @@ -135,6 +138,7 @@ def _perform_case_update(self, domain, context): is_creation=False, xmlns=REPEATER_RESPONSE_XMLNS, ) + return form.form_id @property def device_id(self): diff --git a/corehq/motech/repeaters/expression/tests.py b/corehq/motech/repeaters/expression/tests.py index 3a9ec1ad9646..cff94d0611a1 100644 --- a/corehq/motech/repeaters/expression/tests.py +++ b/corehq/motech/repeaters/expression/tests.py @@ -213,13 +213,15 @@ def test_process_response_filter(self): "operator": "eq", "property_value": 201, } - self.repeater._perform_case_update = Mock() + self.repeater._perform_case_update = Mock(return_value="fake_form_id") response = MockResponse(200) - self.assertFalse(self.repeater._process_response_as_case_update(response, repeat_record)) + message = self.repeater._process_response_as_case_update(response, repeat_record) + self.assertEqual(message, "Response did not match filter") self.repeater._perform_case_update.assert_not_called() response = MockResponse(201) - self.assertTrue(self.repeater._process_response_as_case_update(response, repeat_record)) + message = self.repeater._process_response_as_case_update(response, repeat_record) + self.assertEqual(message, "Response generated form: fake_form_id") self.repeater._perform_case_update.assert_called() def test_process_response(self): diff --git a/corehq/motech/repeaters/models.py b/corehq/motech/repeaters/models.py index 3b29f78e33ff..28e304db8a46 100644 --- a/corehq/motech/repeaters/models.py +++ b/corehq/motech/repeaters/models.py @@ -477,19 +477,19 @@ def handle_response(self, result, repeat_record): result may be either a response object or an exception """ if isinstance(result, RequestConnectionError): - repeat_record.handle_timeout(result) + return repeat_record.handle_timeout(result) elif isinstance(result, Exception): - repeat_record.handle_exception(result) + return repeat_record.handle_exception(result) elif is_success_response(result): - repeat_record.handle_success(result) + return repeat_record.handle_success(result) elif not is_response(result) or ( 500 <= result.status_code < 600 or result.status_code in HTTP_STATUS_4XX_RETRY ): - repeat_record.handle_failure(result) + return repeat_record.handle_failure(result) else: message = format_response(result) - repeat_record.handle_payload_error(message) + return repeat_record.handle_payload_error(message) def get_headers(self, repeat_record): # to be overridden @@ -1049,10 +1049,11 @@ def add_success_attempt(self, response): # although the Couch RepeatRecord did the same. code = getattr(response, "status_code", None) state = State.Empty if code == 204 else State.Success - self.attempt_set.create(state=state, message=format_response(response) or '') + attempt = self.attempt_set.create(state=state, message=format_response(response) or '') self.state = state self.next_check = None self.save() + return attempt def add_client_failure_attempt(self, message, retry=True): """ @@ -1060,7 +1061,7 @@ def add_client_failure_attempt(self, message, retry=True): service is assumed to be in a good state, so do not back off, so that this repeat record does not hold up the rest. """ - self._add_failure_attempt(message, MAX_ATTEMPTS, retry) + return self._add_failure_attempt(message, MAX_ATTEMPTS, retry) def add_server_failure_attempt(self, message): """ @@ -1073,7 +1074,7 @@ def add_server_failure_attempt(self, message): days and will hold up all other payloads. """ - self._add_failure_attempt(message, MAX_BACKOFF_ATTEMPTS) + return self._add_failure_attempt(message, MAX_BACKOFF_ATTEMPTS) def _add_failure_attempt(self, message, max_attempts, retry=True): if retry and self.num_attempts < max_attempts: @@ -1085,9 +1086,10 @@ def _add_failure_attempt(self, message, max_attempts, retry=True): self.state = state self.next_check = (attempt.created_at + wait) if state == State.Fail else None self.save() + return attempt def add_payload_error_attempt(self, message, traceback_str): - self.attempt_set.create( + attempt = self.attempt_set.create( state=State.InvalidPayload, message=message, traceback=traceback_str, @@ -1095,6 +1097,7 @@ def add_payload_error_attempt(self, message, traceback_str): self.state = State.InvalidPayload self.next_check = None self.save() + return attempt @property def attempts(self): @@ -1224,23 +1227,23 @@ def handle_success(self, response): response.status_code, self.repeater_type ) - self.add_success_attempt(response) + return self.add_success_attempt(response) def handle_failure(self, response): log_repeater_error_in_datadog(self.domain, response.status_code, self.repeater_type) - self.add_server_failure_attempt(format_response(response)) + return self.add_server_failure_attempt(format_response(response)) def handle_exception(self, exception): log_repeater_error_in_datadog(self.domain, None, self.repeater_type) - self.add_client_failure_attempt(str(exception)) + return self.add_client_failure_attempt(str(exception)) def handle_timeout(self, exception): log_repeater_timeout_in_datadog(self.domain) - self.add_server_failure_attempt(str(exception)) + return self.add_server_failure_attempt(str(exception)) def handle_payload_error(self, message, traceback_str=''): log_repeater_error_in_datadog(self.domain, status_code=None, repeater_type=self.repeater_type) - self.add_payload_error_attempt(message, traceback_str) + return self.add_payload_error_attempt(message, traceback_str) def cancel(self): self.state = State.Cancelled From 47f9f875d66aea81e1b72cbc94db628de919cc58 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 22 Oct 2024 11:38:13 +0200 Subject: [PATCH 5/7] escape attempt message + convert linebreaks --- .../repeaters/partials/bootstrap3/attempt_history.html | 2 +- .../repeaters/partials/bootstrap5/attempt_history.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/motech/repeaters/templates/repeaters/partials/bootstrap3/attempt_history.html b/corehq/motech/repeaters/templates/repeaters/partials/bootstrap3/attempt_history.html index 50ac2128445b..b2fe598f96e0 100644 --- a/corehq/motech/repeaters/templates/repeaters/partials/bootstrap3/attempt_history.html +++ b/corehq/motech/repeaters/templates/repeaters/partials/bootstrap3/attempt_history.html @@ -30,7 +30,7 @@ {% endif %} {% if attempt.message %}
- {{ attempt.message }} + {{ attempt.message|escape|linebreaksbr }}
{% endif %} diff --git a/corehq/motech/repeaters/templates/repeaters/partials/bootstrap5/attempt_history.html b/corehq/motech/repeaters/templates/repeaters/partials/bootstrap5/attempt_history.html index 441d5a388500..ccec66bd4b46 100644 --- a/corehq/motech/repeaters/templates/repeaters/partials/bootstrap5/attempt_history.html +++ b/corehq/motech/repeaters/templates/repeaters/partials/bootstrap5/attempt_history.html @@ -30,7 +30,7 @@ {% endif %} {% if attempt.message %}
{# todo B5: css:well, inline style #} - {{ attempt.message }} + {{ attempt.message|escape|linebreaksbr }}
{% endif %} From 99e6718b86f2047506e4378621977c82af9a4fad Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 22 Oct 2024 12:52:56 +0200 Subject: [PATCH 6/7] update test --- corehq/motech/repeaters/expression/tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/motech/repeaters/expression/tests.py b/corehq/motech/repeaters/expression/tests.py index cff94d0611a1..cfb99dfa0bc6 100644 --- a/corehq/motech/repeaters/expression/tests.py +++ b/corehq/motech/repeaters/expression/tests.py @@ -221,7 +221,7 @@ def test_process_response_filter(self): response = MockResponse(201) message = self.repeater._process_response_as_case_update(response, repeat_record) - self.assertEqual(message, "Response generated form: fake_form_id") + self.assertEqual(message, "Response generated a form: fake_form_id") self.repeater._perform_case_update.assert_called() def test_process_response(self): From b5a8eb75b329a14f8a8868f42b03f8ffb570665f Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 22 Oct 2024 12:54:42 +0200 Subject: [PATCH 7/7] Bootstrap 5 Migration - Rebuilt diffs --- .../repeaters/partials/attempt_history.html.diff.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/repeaters/partials/attempt_history.html.diff.txt b/corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/repeaters/partials/attempt_history.html.diff.txt index 7137393b64b7..ca5b5f43bfb4 100644 --- a/corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/repeaters/partials/attempt_history.html.diff.txt +++ b/corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/repeaters/partials/attempt_history.html.diff.txt @@ -15,7 +15,7 @@ {% if attempt.message %} -
+
{# todo B5: css:well, inline style #} - {{ attempt.message }} + {{ attempt.message|escape|linebreaksbr }}
{% endif %} @@ -53,7 +53,7 @@