From 6530993da5a5be7ee212764cd6dd43e47fed00f1 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_backoffice_pages.py | 9 +++--- tests/test_form_pages.py | 3 +- tests/test_workflows.py | 54 +++++++++++++++++++++++++++++++++- wcs/admin/logged_errors.py | 8 +++++ wcs/conditions.py | 11 +++++-- wcs/logged_errors.py | 21 ++++++++++--- wcs/wf/jump.py | 3 +- wcs/workflows.py | 3 +- 8 files changed, 98 insertions(+), 14 deletions(-) diff --git a/tests/test_backoffice_pages.py b/tests/test_backoffice_pages.py index efc58466..4b740a10 100644 --- a/tests/test_backoffice_pages.py +++ b/tests/test_backoffice_pages.py @@ -3907,14 +3907,15 @@ def test_backoffice_logged_errors(pub): app = login(get_app(pub)) resp = app.get('/backoffice/forms/%s/' % formdef.id) - assert 'ZeroDivisionError' in resp.body + assert 'Failed to evaluate condition (error ZeroDivisionError)' in resp.body resp = resp.click('1 error') resp = app.get('/backoffice/workflows/%s/' % workflow.id) resp2 = resp.click('1 error') - assert 'ZeroDivisionError' in resp2.body + assert 'Failed to evaluate condition (error ZeroDivisionError)' in resp2.body resp = resp2.click('ZeroDivisionError') - assert 'integer division or modulo by zero' in resp.body + assert 'ZeroDivisionError: integer division or modulo by zero' in resp.body + assert 'Expression (type python): 
1/0
' in resp.body assert not 'Acked' in resp.body resp = resp.click('Ack').follow() assert 'Acked' in resp.body @@ -3933,7 +3934,7 @@ def test_backoffice_logged_errors(pub): app = login(get_app(pub)) resp = app.get('/backoffice/workflows/%s/' % workflow.id) - assert 'ZeroDivisionError' in resp.body + assert 'Failed to evaluate condition (error ZeroDivisionError)' in resp2.body resp2 = resp.click('1 error') resp = resp2.click('ZeroDivisionError') assert 'href="http://example.net/backoffice/management/test/' in resp.body diff --git a/tests/test_form_pages.py b/tests/test_form_pages.py index eeb93707..0a51b3da 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-error-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..f4e3f91b 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,36 @@ 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 == 'Failed to evaluate condition (error NameError)' + assert logged_error.error_message == "NameError: name 'form_var_foobar' is not defined" + assert logged_error.expression == 'form_var_foobar == 0' + assert logged_error.expression_type == 'python' + + 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 == 'Failed to evaluate condition (error SyntaxError)' + assert logged_error.error_message == "SyntaxError: unexpected EOF while parsing (, line 1)" + assert logged_error.expression == '~ invalid ~' + assert logged_error.expression_type == 'python' + def test_jump_django_conditions(pub): FormDef.wipe() formdef = FormDef() @@ -289,6 +319,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 +332,16 @@ 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 == 'Failed to evaluate condition (error TemplateSyntaxError)' + assert logged_error.error_message == "TemplateSyntaxError: Could not parse the remainder: '~' from '~'" + assert logged_error.expression == '~ invalid ~' + assert logged_error.expression_type == 'django' def test_check_auth(pub): user = pub.user_class(name='foo') @@ -3504,3 +3543,16 @@ 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 == 'Failed to evaluate condition (error NameError)' + assert logged_error.error_message == "NameError: name 'foobar' is not defined" + assert logged_error.expression == 'foobar == barfoo' + assert logged_error.expression_type == 'python' diff --git a/wcs/admin/logged_errors.py b/wcs/admin/logged_errors.py index 4ddf1cdb..1d90b8aa 100644 --- a/wcs/admin/logged_errors.py +++ b/wcs/admin/logged_errors.py @@ -70,6 +70,14 @@ class LoggedErrorDirectory(Directory): _(status_item.description)) r += htmltext('') + if self.error.expression or self.error.expression_type: + r += htmltext('
  • %s 
    %s
    • ') % ( + _('Expression (type %s):') % self.error.expression_type, + self.error.expression) + + if self.error.error_message: + r += htmltext('
  • %s %s
    • ') % (_('Error message:'), self.error.error_message) + if formdef: formdata = self.error.get_formdata() if formdata: diff --git a/wcs/conditions.py b/wcs/conditions.py index 0a9d5761..9417a2f5 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,14 @@ 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 + summary = _('Failed to evaluate condition (error %s)' % e.__class__.__name__) + LoggedError.record(summary, + formdata=self.context.get('formdata'), + status_item=self.context.get('status_item'), + expression=self.value, + expression_type=self.type, + error_message='%s: %s' % (e.__class__.__name__, e)) raise RuntimeError() def evaluate_python(self, local_variables): diff --git a/wcs/logged_errors.py b/wcs/logged_errors.py index 4030d2b6..d4723a05 100644 --- a/wcs/logged_errors.py +++ b/wcs/logged_errors.py @@ -33,6 +33,9 @@ class LoggedError(XmlStorableObject): workflow_id = None status_id = None status_item_id = None + expression = None + expression_type = None + error_message = None traceback = None occurences_count = 0 first_occurence_timestamp = None @@ -42,6 +45,7 @@ class LoggedError(XmlStorableObject): # declarations for serialization XML_NODES = [ ('summary', 'str'), ('traceback', 'str'), + ('expression', 'str'), ('expression_type', 'str'), ('error_message', 'str'), ('formdata_id', 'str'), ('formdef_id', 'str'), ('workflow_id', 'str'), ('status_id', 'str'), ('status_item_id', 'str'), ('occurences_count', 'int'), @@ -51,10 +55,14 @@ class LoggedError(XmlStorableObject): @classmethod def record(cls, error_summary, plain_error_msg=None, formdata=None, - formdef=None, workflow=None, status=None, status_item=None): + formdef=None, workflow=None, status=None, status_item=None, + expression=None, expression_type=None, error_message=None): error = cls() error.summary = error_summary error.traceback = plain_error_msg + error.expression = expression + error.expression_type = expression_type + error.error_message = error_message if formdata: error.formdata_id = str(formdata.id) @@ -74,7 +82,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 @@ -110,8 +118,13 @@ class LoggedError(XmlStorableObject): @property def tech_id(self): - return ('%s-%s-%s-%s-%s' % (self.formdef_id, self.workflow_id, self.status_id, - self.status_item_id, simplify(self.summary)))[:200] + tech_id = '%s-%s-' % (self.formdef_id, self.workflow_id) + if self.status_id: + tech_id += '%s-' % self.status_id + if self.status_item_id: + tech_id += '%s-' % self.status_item_id + tech_id += '%s' % simplify(self.summary) + return tech_id[:200] def get_formdef(self): return FormDef.get(self.formdef_id, ignore_errors=True) 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.18.0