From 4a2b41ca700cad8bd08e1414ba6cb42e4876f5bb Mon Sep 17 00:00:00 2001 From: Thomas NOEL Date: Tue, 12 Jun 2018 16:49:12 +0200 Subject: [PATCH] workflows: only log condition errors as functional errors (#24472) --- tests/test_form_pages.py | 3 ++- tests/test_workflows.py | 50 +++++++++++++++++++++++++++++++++++++++- wcs/conditions.py | 13 +++++++++-- wcs/logged_errors.py | 2 +- wcs/wf/jump.py | 3 ++- wcs/workflows.py | 3 ++- 6 files changed, 67 insertions(+), 7 deletions(-) diff --git a/tests/test_form_pages.py b/tests/test_form_pages.py index eeb93707..897a3498 100644 --- a/tests/test_form_pages.py +++ b/tests/test_form_pages.py @@ -4492,7 +4492,8 @@ def test_logged_errors(pub): assert LoggedError.count() == 1 error = LoggedError.get_on_index( - '34-12-None-None-zerodivisionerror-integer-division-or-modulo-by-zero', 'tech_id') + '34-12-just_submitted-_jump-failed-to-evaluate-condition-10-python-integer-division-or-modulo-by-zero-zerodivisionerror', + 'tech_id') assert error.occurences_count == 2 assert len(LoggedError.get_ids_with_indexed_value('formdef_id', '34')) == 1 diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 81a4d2c6..e6f187ae 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -19,6 +19,7 @@ from wcs.formdef import FormDef from wcs import sessions from wcs.fields import (StringField, DateField, MapField, FileField, ItemField, ItemsField, CommentField) +from wcs.logged_errors import LoggedError from wcs.roles import Role from wcs.workflows import (Workflow, WorkflowStatusItem, SendmailWorkflowStatusItem, SendSMSWorkflowStatusItem, @@ -258,7 +259,6 @@ def test_jump_date_conditions(pub): 'value': 'utils.time_delta(utils.time.localtime(), "2015-01-04").days > 0'} assert item.must_jump(formdata) is True - def test_jump_count_condition(pub): FormDef.wipe() formdef = FormDef() @@ -278,6 +278,34 @@ def test_jump_count_condition(pub): item.condition = {'type': 'python', 'value': 'form_objects.count < 2'} assert item.must_jump(formdata) is False +def test_jump_bad_python_condition(pub): + FormDef.wipe() + formdef = FormDef() + formdef.name = 'foobar' + formdef.store() + pub.substitutions.feed(formdef) + formdef.data_class().wipe() + formdata = formdef.data_class()() + item = JumpWorkflowStatusItem() + + LoggedError.wipe() + item.condition = {'type': 'python', 'value': 'form_var_foobar == 0'} + assert item.must_jump(formdata) is False + assert LoggedError.count() == 1 + logged_error = LoggedError.select()[0] + assert logged_error.summary.startswith('Failed to evaluate condition') + assert 'form_var_foobar == 0' in logged_error.summary + assert "name 'form_var_foobar' is not defined" in logged_error.summary + + LoggedError.wipe() + item.condition = {'type': 'python', 'value': '~ invalid ~'} + assert item.must_jump(formdata) is False + assert LoggedError.count() == 1 + logged_error = LoggedError.select()[0] + assert logged_error.summary.startswith('Failed to evaluate condition') + assert '~ invalid ~' in logged_error.summary + assert 'unexpected EOF while parsing' in logged_error.summary + def test_jump_django_conditions(pub): FormDef.wipe() formdef = FormDef() @@ -289,6 +317,7 @@ def test_jump_django_conditions(pub): pub.substitutions.feed(formdata) item = JumpWorkflowStatusItem() + LoggedError.wipe() item.condition = {'type': 'django', 'value': '1 < 2'} assert item.must_jump(formdata) is True @@ -301,8 +330,15 @@ def test_jump_django_conditions(pub): item.condition = {'type': 'django', 'value': 'form_var_foo|first|upper == "X"'} assert item.must_jump(formdata) is False + assert LoggedError.count() == 0 + item.condition = {'type': 'django', 'value': '~ invalid ~'} assert item.must_jump(formdata) is False + assert LoggedError.count() == 1 + logged_error = LoggedError.select()[0] + assert logged_error.summary.startswith('Failed to evaluate condition') + assert '~ invalid ~' in logged_error.summary + assert "Could not parse the remainder: '~' from '~'" in logged_error.summary def test_check_auth(pub): user = pub.user_class(name='foo') @@ -3504,3 +3540,15 @@ def test_workflow_action_condition(two_pubs): choice.condition = {'type': 'python', 'value': 'form_name == "baz"'} workflow.store() assert len(FormDef.get(formdef.id).data_class().get_actionable_ids([role.id])) == 2 + + # bad condition + LoggedError.wipe() + choice.condition = {'type': 'python', 'value': 'foobar == barfoo'} + workflow.store() + assert len(FormDef.get(formdef.id).data_class().get_actionable_ids([role.id])) == 0 + assert LoggedError.count() == 1 + logged_error = LoggedError.select()[0] + assert logged_error.occurences_count > 1 # should be 2... == 12 with pickle, 4 with sql + assert logged_error.summary.startswith('Failed to evaluate condition') + assert 'foobar == barfoo' in logged_error.summary + assert "name 'foobar' is not defined" in logged_error.summary diff --git a/wcs/conditions.py b/wcs/conditions.py index 0a9d5761..8f8f009c 100644 --- a/wcs/conditions.py +++ b/wcs/conditions.py @@ -30,7 +30,7 @@ class Condition(object): record_errors = True log_errors = False - def __init__(self, condition, context=None): + def __init__(self, condition, context={}): if not condition: condition = {} self.type = condition.get('type') @@ -53,7 +53,16 @@ class Condition(object): if self.log_errors: get_logger().warn('failed to evaluate %r (%r)', self, e) if self.record_errors: - get_publisher().notify_of_exception(sys.exc_info()) + from wcs.logged_errors import LoggedError + message = '%s (%s)' % (e, e.__class__.__name__) + summary = _('Failed to evaluate condition "%(value)s" (%(type)s): %(message)s') % { + 'value': self.value, + 'type': self.type, + 'message': message, + } + LoggedError.record(summary, + formdata=self.context.get('formdata'), + status_item=self.context.get('status_item')) raise RuntimeError() def evaluate_python(self, local_variables): diff --git a/wcs/logged_errors.py b/wcs/logged_errors.py index 4030d2b6..44802258 100644 --- a/wcs/logged_errors.py +++ b/wcs/logged_errors.py @@ -74,7 +74,7 @@ class LoggedError(XmlStorableObject): if status_item: error.status_item_id = status_item.id - if status_item.parent: + if getattr(status_item, 'parent', None): error.status_id = status_item.parent.id if status: error.status_id = status.id diff --git a/wcs/wf/jump.py b/wcs/wf/jump.py index 47ea8a4b..9afec9fc 100644 --- a/wcs/wf/jump.py +++ b/wcs/wf/jump.py @@ -218,8 +218,9 @@ class JumpWorkflowStatusItem(WorkflowStatusJumpItem): must_jump = True if self.condition: + context = {'formdata': formdata, 'status_item': self} try: - must_jump = Condition(self.condition).evaluate() + must_jump = Condition(self.condition, context).evaluate() except RuntimeError: must_jump = False diff --git a/wcs/workflows.py b/wcs/workflows.py index 9f6e6677..52d13285 100644 --- a/wcs/workflows.py +++ b/wcs/workflows.py @@ -1583,8 +1583,9 @@ class WorkflowStatusItem(XmlSerialisable): return False def check_condition(self, formdata): + context = {'formdata': formdata, 'status_item': self} try: - return Condition(self.condition).evaluate() + return Condition(self.condition, context).evaluate() except RuntimeError: return False -- 2.17.1