From b31d0d8a064aa6a9465d62bc137274449da40c91 Mon Sep 17 00:00:00 2001
From: Thomas NOEL <tnoel@entrouvert.com>
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 | 18 +++++++----
 tests/test_form_pages.py       |  3 +-
 tests/test_workflows.py        | 58 +++++++++++++++++++++++++++++++++-
 wcs/admin/logged_errors.py     | 30 ++++++++++++++++--
 wcs/conditions.py              | 10 ++++--
 wcs/logged_errors.py           | 30 +++++++++++++++---
 wcs/wf/jump.py                 |  3 +-
 wcs/workflows.py               |  3 +-
 8 files changed, 136 insertions(+), 19 deletions(-)

diff --git a/tests/test_backoffice_pages.py b/tests/test_backoffice_pages.py
index efc58466..39b174bc 100644
--- a/tests/test_backoffice_pages.py
+++ b/tests/test_backoffice_pages.py
@@ -3907,14 +3907,17 @@ 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' in resp.body
+    assert '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
-    resp = resp2.click('ZeroDivisionError')
-    assert 'integer division or modulo by zero' in resp.body
+    assert 'Failed to evaluate condition' in resp2.body
+    assert 'error ZeroDivisionError' in resp2.body
+    resp = resp2.click('Failed to evaluate condition')
+    assert 'ZeroDivisionError: integer division or modulo by zero' in resp.body
+    assert 'Python Expression: <pre>1/0</pre>' in resp.body
     assert not 'Acked' in resp.body
     resp = resp.click('Ack').follow()
     assert 'Acked' in resp.body
@@ -3933,9 +3936,10 @@ 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' in resp2.body
+    assert 'error ZeroDivisionError' in resp2.body
     resp2 = resp.click('1 error')
-    resp = resp2.click('ZeroDivisionError')
+    resp = resp2.click('Failed to evaluate condition')
     assert 'href="http://example.net/backoffice/management/test/' in resp.body
 
     # very long error string (check it creates a viable tech_id)
@@ -3950,7 +3954,7 @@ def test_backoffice_logged_errors(pub):
 
     # remove formdef
     FormDef.wipe()
-    resp = resp2.click('ZeroDivisionError')
+    resp = resp2.click('Failed to evaluate condition')
     assert not 'href="http://example.net/backoffice/management/test/' in resp.body
 
 def test_backoffice_private_status_and_history(pub):
diff --git a/tests/test_form_pages.py b/tests/test_form_pages.py
index eeb93707..39d4bf5b 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-ZeroDivisionError-integer-division-or-modulo-by-zero',
+        '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..b94319b3 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,38 @@ 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'
+    assert logged_error.exception_class == 'NameError'
+    assert logged_error.exception_message == "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'
+    assert logged_error.exception_class == 'SyntaxError'
+    assert logged_error.exception_message == 'unexpected EOF while parsing (<string>, 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 +321,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 +334,17 @@ 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'
+    assert logged_error.exception_class == 'TemplateSyntaxError'
+    assert logged_error.exception_message == "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 +3546,17 @@ 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'
+    assert logged_error.exception_class == 'NameError'
+    assert logged_error.exception_message == "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..106da872 100644
--- a/wcs/admin/logged_errors.py
+++ b/wcs/admin/logged_errors.py
@@ -70,6 +70,22 @@ class LoggedErrorDirectory(Directory):
                             _(status_item.description))
                 r += htmltext('</ul>')
 
+
+        if self.error.expression or self.error.expression_type:
+            expression_title = {
+                'python': N_('Python Expression'),
+                'django': N_('Django Expression'),
+                'template': N_('Template'),
+                'text': N_('Text'),
+            }.get(self.error.expression_type, N_('Unknown'))
+            r += htmltext('  <li>%s <code>%s</code></li>') % (
+                    _('%s:') % expression_title, self.error.expression)
+
+        if self.error.exception_class or self.error.exception_message:
+            r += htmltext('  <li>%s <code>%s: %s</code></li>') % (_('Error message:'),
+                                                                self.error.exception_class,
+                                                                self.error.exception_message)
+
         if formdef:
             formdata = self.error.get_formdata()
             if formdata:
@@ -159,7 +175,12 @@ class LoggedErrorsDirectory(Directory):
                 '%(count)d error', '%(count)d errors', len(errors)) % {'count': len(errors)}
         r += htmltext('<ul>')
         for error in errors[:3]:
-            r += htmltext('<li><a href="logged-errors/%s/">%s</a></li>') % (error.id, error.summary)
+            r += htmltext('<li><a href="logged-errors/%s/">%s</a> ') % (error.id, error.summary)
+            if error.exception_class or error.exception_message:
+                r += htmltext(_('error %(class)s (%(message)s)')) % {
+                    'class': error.exception_class,
+                    'message': error.exception_message,
+                }
             r += htmltext('</li>')
         if len(errors) > 3:
             r += htmltext('<li>...</li>')
@@ -180,7 +201,12 @@ class LoggedErrorsDirectory(Directory):
         r += htmltext('<h2>%s</h2>') % _('Logged Errors')
         r += htmltext('<ul class="biglist">')
         for error in self.get_errors(formdef_id=self.formdef_id, workflow_id=self.workflow_id):
-            r += htmltext('<li><strong><a href="%s/">%s</a></strong>') % (error.id, error.summary)
+            r += htmltext('<li><strong><a href="%s/">%s</a></strong> ') % (error.id, error.summary)
+            if error.exception_class or error.exception_message:
+                r += htmltext(_('error %(class)s (%(message)s)')) % {
+                    'class': error.exception_class,
+                    'message': error.exception_message,
+                }
             r += htmltext('<p class="details badge">%s</p>') % error.occurences_count
             r += htmltext('</li>')
 
diff --git a/wcs/conditions.py b/wcs/conditions.py
index 0a9d5761..9c55b795 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,13 @@ 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')
+                LoggedError.record(summary,
+                                   formdata=self.context.get('formdata'),
+                                   status_item=self.context.get('status_item'),
+                                   expression=self.value, expression_type=self.type,
+                                   exception=e)
             raise RuntimeError()
 
     def evaluate_python(self, local_variables):
diff --git a/wcs/logged_errors.py b/wcs/logged_errors.py
index 4030d2b6..93a0cd13 100644
--- a/wcs/logged_errors.py
+++ b/wcs/logged_errors.py
@@ -33,7 +33,11 @@ class LoggedError(XmlStorableObject):
     workflow_id = None
     status_id = None
     status_item_id = None
+    expression = None
+    expression_type = None
     traceback = None
+    exception_class = None
+    exception_message = None
     occurences_count = 0
     first_occurence_timestamp = None
     latest_occurence_timestamp = None
@@ -42,6 +46,8 @@ class LoggedError(XmlStorableObject):
     # declarations for serialization
     XML_NODES = [
             ('summary', 'str'), ('traceback', 'str'),
+            ('exception_class', 'str'), ('exception_message', 'str'),
+            ('expression', 'str'), ('expression_type', 'str'),
             ('formdata_id', 'str'), ('formdef_id', 'str'), ('workflow_id', 'str'),
             ('status_id', 'str'), ('status_item_id', 'str'),
             ('occurences_count', 'int'),
@@ -51,10 +57,17 @@ 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, exception=None):
         error = cls()
         error.summary = error_summary
         error.traceback = plain_error_msg
+        error.expression = expression
+        error.expression_type = expression_type
+
+        if exception:
+            error.exception_class = exception.__class__.__name__
+            error.exception_message = str(exception)
 
         if formdata:
             error.formdata_id = str(formdata.id)
@@ -74,7 +87,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 +123,17 @@ 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)
+        if self.exception_class:
+            tech_id += '-%s' % self.exception_class
+        if self.exception_message:
+            tech_id += '-%s' % simplify(self.exception_message)
+        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