From 6e30d5f624b753ce25b9010cff13e60c2c01e5d0 Mon Sep 17 00:00:00 2001 From: Simone Orsi Date: Thu, 19 Oct 2023 08:39:00 +0200 Subject: [PATCH 1/5] edi: fix job return msg --- edi_oca/models/edi_backend.py | 29 ++++++++++--------- edi_oca/models/edi_exchange_record.py | 4 +-- edi_oca/tests/test_backend_jobs.py | 41 ++++++++++++++++++++++++--- 3 files changed, 54 insertions(+), 20 deletions(-) diff --git a/edi_oca/models/edi_backend.py b/edi_oca/models/edi_backend.py index 9280821b5f..f5510c0da0 100644 --- a/edi_oca/models/edi_backend.py +++ b/edi_oca/models/edi_backend.py @@ -239,8 +239,9 @@ def exchange_generate(self, exchange_record, store=True, force=False, **kw): {"edi_exchange_state": state, "exchange_error": error} ) exchange_record.notify_action_complete("generate", message=message) - return output + return message + # TODO: unify to all other checkes that return something def _check_exchange_generate(self, exchange_record, force=False): exchange_record.ensure_one() if ( @@ -288,10 +289,11 @@ def exchange_send(self, exchange_record): # In case already sent: skip sending and check the state check = self._output_check_send(exchange_record) if not check: - return False + return "Nothing to do. Likely already sent." state = exchange_record.edi_exchange_state error = False message = None + res = "" try: self._exchange_send(exchange_record) except self._swallable_exceptions(): @@ -300,7 +302,7 @@ def exchange_send(self, exchange_record): error = _get_exception_msg() state = "output_error_on_send" message = exchange_record._exchange_status_message("send_ko") - res = False + res = f"Error: {error}" else: # TODO: maybe the send handler should return desired message and state message = exchange_record._exchange_status_message("send_ok") @@ -310,7 +312,7 @@ def exchange_send(self, exchange_record): if self.output_sent_processed_auto else "output_sent" ) - res = True + res = message finally: exchange_record.write( { @@ -456,22 +458,21 @@ def exchange_process(self, exchange_record): # In case already processed: skip processing and check the state check = self._exchange_process_check(exchange_record) if not check: - return False + return "Nothing to do. Likely already processed." old_state = state = exchange_record.edi_exchange_state error = False message = None try: - self._exchange_process(exchange_record) - except self._swallable_exceptions(): + res = self._exchange_process(exchange_record) + except self._swallable_exceptions() as err: if self.env.context.get("_edi_process_break_on_error"): raise error = _get_exception_msg() state = "input_processed_error" - res = False + res = f"Error: {error}" else: error = None state = "input_processed" - res = True finally: exchange_record.write( { @@ -505,7 +506,7 @@ def exchange_receive(self, exchange_record): # In case already processed: skip processing and check the state check = self._exchange_receive_check(exchange_record) if not check: - return False + return "Nothing to do. Likely already received." state = exchange_record.edi_exchange_state error = False message = None @@ -519,19 +520,19 @@ def exchange_receive(self, exchange_record): error = _get_exception_msg() state = "validate_error" message = exchange_record._exchange_status_message("validate_ko") - res = False - except self._swallable_exceptions(): + res = f"Validation error: {error}" + except self._swallable_exceptions() as err: if self.env.context.get("_edi_receive_break_on_error"): raise error = _get_exception_msg() state = "input_receive_error" message = exchange_record._exchange_status_message("receive_ko") - res = False + res = f"Input error: {error}" else: message = exchange_record._exchange_status_message("receive_ok") error = None state = "input_received" - res = True + res = message finally: exchange_record.write( { diff --git a/edi_oca/models/edi_exchange_record.py b/edi_oca/models/edi_exchange_record.py index 3c4a808de9..7b55ee892d 100644 --- a/edi_oca/models/edi_exchange_record.py +++ b/edi_oca/models/edi_exchange_record.py @@ -277,9 +277,9 @@ def _exchange_status_messages(self): "send_ko": _( "An error happened while sending. Please check exchange record info." ), - "process_ok": _("Exchange processed successfully "), + "process_ok": _("Exchange processed successfully"), "process_ko": _("Exchange processed with errors"), - "receive_ok": _("Exchange received successfully "), + "receive_ok": _("Exchange received successfully"), "receive_ko": _("Exchange not received"), "ack_received": _("ACK file received."), "ack_missing": _("ACK file is required for this exchange but not found."), diff --git a/edi_oca/tests/test_backend_jobs.py b/edi_oca/tests/test_backend_jobs.py index 26699985d2..6cb4fcbac8 100644 --- a/edi_oca/tests/test_backend_jobs.py +++ b/edi_oca/tests/test_backend_jobs.py @@ -2,6 +2,8 @@ # @author: Simone Orsi # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). +import mock + from odoo.addons.queue_job.tests.common import JobMixin from .common import EDIBackendCommonTestCase @@ -19,14 +21,30 @@ def test_output(self): "res_id": self.partner.id, } record = self.backend.create_record("test_csv_output", vals) - self.backend.with_delay().exchange_generate(record) + self.assertEqual(record.edi_exchange_state, "new") + job = self.backend.with_delay().exchange_generate(record) created = job_counter.search_created() self.assertEqual(len(created), 1) self.assertEqual( created.name, "Generate output content for given exchange record." ) - self.backend.with_delay().exchange_send(record) + with mock.patch.object( + type(self.backend), "_exchange_generate" + ) as mocked_generate, mock.patch.object( + type(self.backend), "_validate_data" + ) as mocked_validate: + mocked_generate.return_value = "filecontent" + mocked_validate.return_value = None + res = job.perform() + self.assertEqual(res, None) + self.assertEqual(record.edi_exchange_state, "output_pending") + job = self.backend.with_delay().exchange_send(record) created = job_counter.search_created() + with mock.patch.object(type(self.backend), "_exchange_send") as mocked: + mocked.return_value = "ok" + res = job.perform() + self.assertEqual(res, "Exchange sent") + self.assertEqual(record.edi_exchange_state, "output_sent") self.assertEqual(created[0].name, "Send exchange file.") def test_input(self): @@ -36,10 +54,25 @@ def test_input(self): "res_id": self.partner.id, } record = self.backend.create_record("test_csv_input", vals) - self.backend.with_delay().exchange_receive(record) + job = self.backend.with_delay().exchange_receive(record) created = job_counter.search_created() self.assertEqual(len(created), 1) self.assertEqual(created.name, "Retrieve an incoming document.") - self.backend.with_delay().exchange_process(record) + with mock.patch.object( + type(self.backend), "_exchange_receive" + ) as mocked_receive, mock.patch.object( + type(self.backend), "_validate_data" + ) as mocked_validate: + mocked_receive.return_value = "filecontent" + mocked_validate.return_value = None + res = job.perform() + # the state is not input_pending hence there's nothing to do + self.assertEqual(res, "Nothing to do. Likely already received.") + record.edi_exchange_state = "input_pending" + res = job.perform() + # the state is not input_pending hence there's nothing to do + self.assertEqual(res, "Exchange received successfully") + self.assertEqual(record.edi_exchange_state, "input_received") + job = self.backend.with_delay().exchange_process(record) created = job_counter.search_created() self.assertEqual(created[0].name, "Process an incoming document.") From b0bd0e8b380c884ccfe7c2d3e11a8c20138f244b Mon Sep 17 00:00:00 2001 From: Simone Orsi Date: Thu, 19 Oct 2023 08:54:21 +0200 Subject: [PATCH 2/5] edi: make send failed job retryable Send jobs might fail due to an external service being not responsive. If the job is simply failed, a new one will be spawned and might encour in the same error again, possibly leading to an high number of duplicated jobs for the same record. Yet, when a RetryableJobError is raised, the job will be set back into pending state and will be nicely retried based on jobs configuration. --- edi_oca/models/edi_backend.py | 14 ++++++++++++-- edi_oca/models/edi_exchange_record.py | 8 ++++++++ edi_oca/tests/test_backend_jobs.py | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/edi_oca/models/edi_backend.py b/edi_oca/models/edi_backend.py index f5510c0da0..aa90fda4da 100644 --- a/edi_oca/models/edi_backend.py +++ b/edi_oca/models/edi_backend.py @@ -13,6 +13,7 @@ from odoo import _, exceptions, fields, models, tools from odoo.addons.component.exception import NoComponentError +from odoo.addons.queue_job.exception import RetryableJobError from ..exceptions import EDIValidationError @@ -303,6 +304,11 @@ def exchange_send(self, exchange_record): state = "output_error_on_send" message = exchange_record._exchange_status_message("send_ko") res = f"Error: {error}" + except self._send_retryable_exceptions() as err: + error = _get_exception_msg() + raise RetryableJobError( + error, **exchange_record._job_retry_params() + ) from err else: # TODO: maybe the send handler should return desired message and state message = exchange_record._exchange_status_message("send_ok") @@ -335,6 +341,10 @@ def _swallable_exceptions(self): exceptions.ValidationError, ) + def _send_retryable_exceptions(self): + # IOError is a base class for all connection errors + return (IOError,) + def _output_check_send(self, exchange_record): if exchange_record.direction != "output": raise exceptions.UserError( @@ -464,7 +474,7 @@ def exchange_process(self, exchange_record): message = None try: res = self._exchange_process(exchange_record) - except self._swallable_exceptions() as err: + except self._swallable_exceptions(): if self.env.context.get("_edi_process_break_on_error"): raise error = _get_exception_msg() @@ -521,7 +531,7 @@ def exchange_receive(self, exchange_record): state = "validate_error" message = exchange_record._exchange_status_message("validate_ko") res = f"Validation error: {error}" - except self._swallable_exceptions() as err: + except self._swallable_exceptions(): if self.env.context.get("_edi_receive_break_on_error"): raise error = _get_exception_msg() diff --git a/edi_oca/models/edi_exchange_record.py b/edi_oca/models/edi_exchange_record.py index 7b55ee892d..59d7763c4f 100644 --- a/edi_oca/models/edi_exchange_record.py +++ b/edi_oca/models/edi_exchange_record.py @@ -571,3 +571,11 @@ def with_delay(self, **kw): params = self._job_delay_params() params.update(kw) return super().with_delay(**params) + + def delayable(self, **kw): + params = self._job_delay_params() + params.update(kw) + return super().delayable(**params) + + def _job_retry_params(self): + return {} diff --git a/edi_oca/tests/test_backend_jobs.py b/edi_oca/tests/test_backend_jobs.py index 6cb4fcbac8..ece90b9624 100644 --- a/edi_oca/tests/test_backend_jobs.py +++ b/edi_oca/tests/test_backend_jobs.py @@ -3,7 +3,9 @@ # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). import mock +from requests.exceptions import ConnectionError as ReqConnectionError +from odoo.addons.queue_job.exception import RetryableJobError from odoo.addons.queue_job.tests.common import JobMixin from .common import EDIBackendCommonTestCase @@ -47,6 +49,22 @@ def test_output(self): self.assertEqual(record.edi_exchange_state, "output_sent") self.assertEqual(created[0].name, "Send exchange file.") + def test_output_fail_retry(self): + job_counter = self.job_counter() + vals = { + "model": self.partner._name, + "res_id": self.partner.id, + "edi_exchange_state": "output_pending", + } + record = self.backend.create_record("test_csv_output", vals) + record._set_file_content("ABC") + job = self.backend.with_delay().exchange_send(record) + job_counter.search_created() + with mock.patch.object(type(self.backend), "_exchange_send") as mocked: + mocked.side_effect = ReqConnectionError("Connection broken") + with self.assertRaises(RetryableJobError): + job.perform() + def test_input(self): job_counter = self.job_counter() vals = { From 53ab3d7cb601cf03c7b54099fcc6da4484176890 Mon Sep 17 00:00:00 2001 From: Simone Orsi Date: Tue, 21 Nov 2023 11:21:50 +0100 Subject: [PATCH 3/5] edi: test_same_code_same_backend mute sql logger --- edi_oca/tests/test_exchange_type.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/edi_oca/tests/test_exchange_type.py b/edi_oca/tests/test_exchange_type.py index 7655c47bf1..d7bfd8c890 100644 --- a/edi_oca/tests/test_exchange_type.py +++ b/edi_oca/tests/test_exchange_type.py @@ -5,6 +5,8 @@ from freezegun import freeze_time +from odoo.tools import mute_logger + from .common import EDIBackendCommonTestCase @@ -23,6 +25,7 @@ def test_ack_for(self): self.exchange_type_out_ack.ack_for_type_ids.ids, ) + @mute_logger("odoo.sql_db") def test_same_code_same_backend(self): with self.assertRaises(Exception) as err: self.exchange_type_in.copy({"code": "test_csv_input"}) From 38ac75d44f9ed6633e0ae8fcd63437f9587d34f1 Mon Sep 17 00:00:00 2001 From: Simone Orsi Date: Wed, 5 Jul 2023 10:03:58 +0200 Subject: [PATCH 4/5] edi: add generate_ok message When actions are automated, is nice to see that data has been generated for an exchange on the related record. --- edi_oca/models/edi_backend.py | 1 + edi_oca/models/edi_exchange_record.py | 1 + edi_oca/tests/test_backend_jobs.py | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/edi_oca/models/edi_backend.py b/edi_oca/models/edi_backend.py index aa90fda4da..4fd2106cf8 100644 --- a/edi_oca/models/edi_backend.py +++ b/edi_oca/models/edi_backend.py @@ -230,6 +230,7 @@ def exchange_generate(self, exchange_record, store=True, force=False, **kw): except UnicodeDecodeError: pass if output: + message = exchange_record._exchange_status_message("generate_ok") try: self._validate_data(exchange_record, output) except EDIValidationError: diff --git a/edi_oca/models/edi_exchange_record.py b/edi_oca/models/edi_exchange_record.py index 59d7763c4f..88eb197a00 100644 --- a/edi_oca/models/edi_exchange_record.py +++ b/edi_oca/models/edi_exchange_record.py @@ -273,6 +273,7 @@ def _constrain_backend(self): def _exchange_status_messages(self): return { # status: message + "generate_ok": _("Exchange data generated"), "send_ok": _("Exchange sent"), "send_ko": _( "An error happened while sending. Please check exchange record info." diff --git a/edi_oca/tests/test_backend_jobs.py b/edi_oca/tests/test_backend_jobs.py index ece90b9624..70c835dabd 100644 --- a/edi_oca/tests/test_backend_jobs.py +++ b/edi_oca/tests/test_backend_jobs.py @@ -38,7 +38,7 @@ def test_output(self): mocked_generate.return_value = "filecontent" mocked_validate.return_value = None res = job.perform() - self.assertEqual(res, None) + self.assertEqual(res, "Exchange data generated") self.assertEqual(record.edi_exchange_state, "output_pending") job = self.backend.with_delay().exchange_send(record) created = job_counter.search_created() From c2ff53fec3ded054b47067035d0df6b0087b1966 Mon Sep 17 00:00:00 2001 From: Simone Orsi Date: Tue, 21 Nov 2023 17:13:58 +0100 Subject: [PATCH 5/5] edi_exchange_template: adapt tests exchange_generate now returns status string, not the output. --- .../tests/test_edi_backend_output.py | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/edi_exchange_template_oca/tests/test_edi_backend_output.py b/edi_exchange_template_oca/tests/test_edi_backend_output.py index 988b4f3601..e28f6ed106 100644 --- a/edi_exchange_template_oca/tests/test_edi_backend_output.py +++ b/edi_exchange_template_oca/tests/test_edi_backend_output.py @@ -1,6 +1,5 @@ # Copyright 2020 ACSONE SA/NV () # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html). -import base64 from lxml import etree @@ -139,11 +138,12 @@ def test_get_template(self): def test_generate_file(self): output = self.backend.exchange_generate(self.record1) expected = "{0.ref} - {0.name}".format(self.partner) - self.assertEqual(output.strip(), expected) - file_content = base64.b64decode(self.record1.exchange_file).decode() + self.assertEqual(output, "Exchange data generated") + file_content = self.record1._get_file_content() self.assertEqual(file_content.strip(), expected) output = self.backend.exchange_generate(self.record2) - doc = etree.fromstring(output) + file_content = self.record2._get_file_content() + doc = etree.fromstring(file_content) self.assertEqual(doc.tag, "Record") self.assertEqual(doc.attrib, {"ref": self.partner.ref}) self.assertEqual(doc.getchildren()[0].tag, "Name") @@ -156,16 +156,17 @@ def test_prettify(self): self.tmpl_out2.template_id.arch = ( '1' ) - result = self.tmpl_out2.exchange_generate(self.record2) - self.assertEqual(result, b"1") + output = self.tmpl_out2.exchange_generate(self.record2) + self.assertEqual(output, b"1") self.tmpl_out2.prettify = True - result = self.tmpl_out2.exchange_generate(self.record2) - self.assertEqual(result, b"\n 1\n\n") + output = self.tmpl_out2.exchange_generate(self.record2) + self.assertEqual(output, b"\n 1\n\n") def test_generate_file_report(self): output = self.backend.exchange_generate(self.record3) - self.assertTrue(output) + self.assertEqual(output, "Exchange data generated") + file_content = self.record3._get_file_content() self.assertEqual( self.report._render([self.record3.res_id])[0].strip().decode("UTF-8"), - output.strip(), + file_content.strip(), )