diff --git a/tests/backoffice_pages/test_all.py b/tests/backoffice_pages/test_all.py index 1603bdf56..1340e4267 100644 --- a/tests/backoffice_pages/test_all.py +++ b/tests/backoffice_pages/test_all.py @@ -5123,7 +5123,8 @@ def test_backoffice_logged_errors(pub): assert 'ZeroDivisionError' in resp2.text resp = resp2.click('Failed to evaluate condition') assert 'ZeroDivisionError: integer division or modulo by zero' in resp.text - assert 'Python Expression: 1//0' in resp.text + assert 'Condition: 1//0' in resp.text + assert 'Condition type: python' in resp.text resp = resp.click('Delete').follow() assert pub.loggederror_class.count() == 0 diff --git a/tests/form_pages/test_all.py b/tests/form_pages/test_all.py index 426e1228b..dd12c346b 100644 --- a/tests/form_pages/test_all.py +++ b/tests/form_pages/test_all.py @@ -3408,7 +3408,16 @@ def test_logged_errors(pub): ) )[0] assert error.occurences_count == 2 - assert error.expression == '2//0' + assert error.context == { + 'stack': [ + { + 'condition': '2//0', + 'condition_type': 'python', + 'source_label': 'Automatic Jump', + 'source_url': 'http://example.net/backoffice/workflows/12/status/just_submitted/items/_jump/', + } + ] + } assert pub.loggederror_class.count([Equal('formdef_id', '34')]) == 1 assert pub.loggederror_class.count([Equal('formdef_id', 'X')]) == 0 diff --git a/tests/form_pages/test_formdata.py b/tests/form_pages/test_formdata.py index b666b772e..0a2b6fbb4 100644 --- a/tests/form_pages/test_formdata.py +++ b/tests/form_pages/test_formdata.py @@ -1590,6 +1590,57 @@ def test_formdata_named_wscall_in_conditions(http_requests, pub): assert http_requests.count() == 1 +def test_formdata_error_with_wscall_in_conditions(http_requests, pub): + create_user(pub) + NamedWsCall.wipe() + + wscall = NamedWsCall() + wscall.name = 'Hello world' + wscall.request = {'url': 'http://remote.example.net/404', 'method': 'GET'} + wscall.record_on_errors = True + wscall.store() + assert wscall.slug == 'hello_world' + + FormDef.wipe() + formdef = FormDef() + formdef.name = 'test' + formdef.fields = [ + fields.PageField(id='0', label='1st page'), + fields.PageField( + id='1', + label='2nd page', + condition={'type': 'python', 'value': 'webservice.hello_world["foo"] == "bar"'}, + ), + ] + formdef.store() + formdef.data_class().wipe() + + pub.loggederror_class.wipe() + resp = login(get_app(pub), username='foo', password='foo').get('/test/') + assert '>1st page<' in resp.text + assert '>2nd page<' in resp.text + + # condition error and wscall error + assert pub.loggederror_class.count() == 2 + wscall_error, condition_error = pub.loggederror_class.select(order_by='id') + assert ( + wscall_error.context + == condition_error.context + == { + 'stack': [ + { + 'condition': 'webservice.hello_world["foo"] == "bar"', + 'condition_type': 'python', + 'source_label': 'Field: 2nd page', + 'source_url': 'http://example.net/backoffice/forms/1/fields/1/', + } + ] + } + ) + assert wscall_error.summary == '[WSCALL] 404 Not Found' + assert condition_error.summary == 'Failed to evaluate condition' + + def test_formdata_named_wscall_in_comment(pub): create_user(pub) NamedWsCall.wipe() diff --git a/tests/workflow/test_all.py b/tests/workflow/test_all.py index 98b6710d2..553009a5b 100644 --- a/tests/workflow/test_all.py +++ b/tests/workflow/test_all.py @@ -1964,6 +1964,7 @@ def test_redirect_to_url(pub): def test_workflow_action_condition(pub): + Workflow.wipe() workflow = Workflow(name='jump condition migration') st1 = workflow.add_status('Status1', 'st1') workflow.store() @@ -2046,8 +2047,16 @@ def test_workflow_action_condition(pub): 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' + assert logged_error.context == { + 'stack': [ + { + 'condition': 'foobar == barfoo', + 'condition_type': 'python', + 'source_label': 'Manual Jump', + 'source_url': 'http://example.net/backoffice/workflows/1/status/st1/items/_x/', + } + ] + } def test_workflow_field_migration(pub): diff --git a/tests/workflow/test_jump.py b/tests/workflow/test_jump.py index 90b821f0f..ed452312e 100644 --- a/tests/workflow/test_jump.py +++ b/tests/workflow/test_jump.py @@ -158,8 +158,16 @@ def test_jump_bad_python_condition(pub): 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' + assert logged_error.context == { + 'stack': [ + { + 'condition': 'form_var_foobar == 0', + 'condition_type': 'python', + 'source_label': 'Automatic Jump', + 'source_url': '', + } + ] + } pub.loggederror_class.wipe() item.condition = {'type': 'python', 'value': '~ invalid ~'} @@ -169,8 +177,16 @@ def test_jump_bad_python_condition(pub): assert logged_error.summary == 'Failed to evaluate condition' assert logged_error.exception_class == 'SyntaxError' assert logged_error.exception_message == 'invalid syntax (, line 1)' - assert logged_error.expression == '~ invalid ~' - assert logged_error.expression_type == 'python' + assert logged_error.context == { + 'stack': [ + { + 'condition': '~ invalid ~', + 'source_url': '', + 'source_label': 'Automatic Jump', + 'condition_type': 'python', + } + ] + } def test_jump_django_conditions(pub): @@ -207,8 +223,16 @@ def test_jump_django_conditions(pub): 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' + assert logged_error.context == { + 'stack': [ + { + 'condition': '~ invalid ~', + 'source_url': '', + 'source_label': 'Automatic Jump', + 'condition_type': 'django', + } + ] + } def test_timeout(pub): diff --git a/wcs/admin/logged_errors.py b/wcs/admin/logged_errors.py index 3536626e4..941c38460 100644 --- a/wcs/admin/logged_errors.py +++ b/wcs/admin/logged_errors.py @@ -28,6 +28,28 @@ from wcs.qommon.form import CheckboxesWidget, DateWidget, Form from wcs.sql_criterias import Equal, Less, NotEqual, NotNull, Null, Or +class ErrorFrame: + def __init__(self, context): + self.context = context or {} + + def source(self): + if self.context.get('source_url'): + return { + 'url': self.context.get('source_url'), + 'label': self.context.get('source_label'), + } + return None + + def get_frame_lines(self): + for key, value in self.context.items(): + key_label = { + 'condition': _('Condition'), + 'condition_type': _('Condition type'), + }.get(key) + if key_label: + yield {'label': key_label, 'value': value} + + class LoggedErrorDirectory(Directory): _q_exports = ['', 'delete', 'ack'] do_not_call_in_templates = True @@ -63,6 +85,10 @@ class LoggedErrorDirectory(Directory): 'text': _('Text'), }.get(self.error.expression_type, _('Unknown')) + def get_context_frames(self): + for frame_context in reversed(self.error.context.get('stack') or []): + yield ErrorFrame(frame_context) + def get_tabs(self): r = TemplateIO(html=True) parts = ( diff --git a/wcs/conditions.py b/wcs/conditions.py index 3c766fdf0..9dbafd32e 100644 --- a/wcs/conditions.py +++ b/wcs/conditions.py @@ -49,21 +49,22 @@ class Condition: local_variables = self.get_data() return getattr(self, 'evaluate_' + self.type)(local_variables) - def evaluate(self): - try: - return self.unsafe_evaluate() - except Exception as e: - if self.record_errors: - summary = _('Failed to evaluate condition') - get_publisher().record_error( - 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(self, source_label=None, source_url=None): + with get_publisher().error_context( + condition=self.value, condition_type=self.type, source_label=source_label, source_url=source_url + ): + try: + return self.unsafe_evaluate() + except Exception as e: + if self.record_errors: + summary = _('Failed to evaluate condition') + get_publisher().record_error( + summary, + formdata=self.context.get('formdata'), + status_item=self.context.get('status_item'), + exception=e, + ) + raise RuntimeError() def evaluate_python(self, local_variables): global_variables = get_publisher().get_global_eval_dict() diff --git a/wcs/fields/base.py b/wcs/fields/base.py index dbaf14075..8503453f3 100644 --- a/wcs/fields/base.py +++ b/wcs/fields/base.py @@ -15,6 +15,7 @@ # along with this program; if not, see . import collections +import copy import datetime import html import re @@ -236,6 +237,11 @@ class Field: for k, v in kwargs.items(): setattr(self, k.replace('-', '_'), v) + def __getstate__(self): + odict = copy.copy(self.__dict__) + odict.pop('_formdef', None) + return odict + @classmethod def init(cls): pass @@ -243,6 +249,11 @@ class Field: def get_type_label(self): return self.description + def get_admin_url(self): + if not getattr(self, '_formdef', None): + return '' + return self._formdef.get_field_admin_url(field=self) + @property def include_in_listing(self): return 'listings' in (self.display_locations or []) @@ -584,16 +595,24 @@ class Field: return changed @staticmethod - def evaluate_condition(dict_vars, formdef, condition, record_errors=True): + def evaluate_condition( + dict_vars, formdef, condition, source_label=None, source_url=None, record_errors=True + ): from .page import PageCondition - return PageCondition( - condition, {'dict_vars': dict_vars, 'formdef': formdef}, record_errors - ).evaluate() + return PageCondition(condition, {'dict_vars': dict_vars, 'formdef': formdef}, record_errors).evaluate( + source_label=source_label, source_url=source_url + ) def is_visible(self, dict, formdef): try: - return self.evaluate_condition(dict, formdef, self.condition) + return self.evaluate_condition( + dict, + formdef, + self.condition, + source_label=_('Field: %s') % self.ellipsized_label, + source_url=self.get_admin_url(), + ) except RuntimeError: return True diff --git a/wcs/fields/block.py b/wcs/fields/block.py index 251a8b02c..21eb4541d 100644 --- a/wcs/fields/block.py +++ b/wcs/fields/block.py @@ -342,9 +342,8 @@ class BlockField(WidgetField): def __getstate__(self): # do not store _block cache - odict = copy.copy(self.__dict__) - if '_block' in odict: - del odict['_block'] + odict = super().__getstate__() + odict.pop('_block', None) return odict def __setstate__(self, ndict): diff --git a/wcs/formdef.py b/wcs/formdef.py index 0a05a6e9c..e341d41ff 100644 --- a/wcs/formdef.py +++ b/wcs/formdef.py @@ -2010,6 +2010,8 @@ class FormDef(StorableObject): o.fields = pickle.load(fd, **PICKLE_KWARGS) except EOFError: pass # old format + for field in o.fields or []: + field._formdef = o # keep formdef reference return o @classmethod diff --git a/wcs/logged_errors.py b/wcs/logged_errors.py index 4f8d36f16..f7057ee38 100644 --- a/wcs/logged_errors.py +++ b/wcs/logged_errors.py @@ -18,6 +18,7 @@ import re from django.utils.formats import number_format from django.utils.timezone import now +from quixote import get_publisher from quixote.html import htmlescape, htmltext from wcs.carddef import CardDef @@ -42,6 +43,7 @@ class LoggedError: status_item_id = None expression = None expression_type = None + context = None traceback = None exception_class = None exception_message = None @@ -93,6 +95,8 @@ class LoggedError: if status: error.status_id = status.id + error.context = get_publisher().get_error_context() + error.first_occurence_timestamp = now() error.tech_id = error.build_tech_id() error.occurences_count += 1 @@ -111,6 +115,7 @@ class LoggedError: self.traceback = error.traceback self.expression = error.expression self.expression_type = error.expression_type + self.context = error.context # exception should be the same (same tech_id), record just in case self.exception_class = error.exception_class self.exception_message = error.exception_message diff --git a/wcs/publisher.py b/wcs/publisher.py index b66be30fa..167167bf4 100644 --- a/wcs/publisher.py +++ b/wcs/publisher.py @@ -605,6 +605,7 @@ class WcsPublisher(QommonPublisher): def cleanup(self): self._cached_user_fields_formdef = None self._update_related_seen = None + self._error_context = None from . import sql sql.cleanup_connection() @@ -673,6 +674,22 @@ class WcsPublisher(QommonPublisher): finally: self.keep_all_block_rows_mode = False + # stacked contexts to include in logged errors + _error_context = None + + @contextmanager + def error_context(self, **kwargs): + if not self._error_context: + self._error_context = [] + self._error_context.append(kwargs) + try: + yield True + finally: + self._error_context.pop() + + def get_error_context(self): + return {'stack': self._error_context} if self._error_context else None + def clean_deleted_users(self, **kwargs): for user_id in self.user_class.get_to_delete_ids(): self.user_class.remove_object(user_id) diff --git a/wcs/qommon/static/css/dc2/admin.scss b/wcs/qommon/static/css/dc2/admin.scss index 382190481..d5eff38ed 100644 --- a/wcs/qommon/static/css/dc2/admin.scss +++ b/wcs/qommon/static/css/dc2/admin.scss @@ -3143,3 +3143,18 @@ form div.widget[data-widget-name="model_file_mode"] { display: block; } } + +#panel-general ul.logged-error-frames { + padding: 0; + margin: 0; + li { + padding: 0; + margin: 0; + } + .logged-error-frames--context { + list-style: none; + } + > li:nth-child(2n) { + background: #eee; + } +} diff --git a/wcs/sql.py b/wcs/sql.py index e5832b865..addacd772 100644 --- a/wcs/sql.py +++ b/wcs/sql.py @@ -1177,6 +1177,7 @@ def do_loggederrors_table(): status_item_id VARCHAR, expression VARCHAR, expression_type VARCHAR, + context JSONB, traceback TEXT, exception_class VARCHAR, exception_message VARCHAR, @@ -1199,6 +1200,8 @@ def do_loggederrors_table(): # migrations if 'kind' not in existing_fields: cur.execute('''ALTER TABLE %s ADD COLUMN kind VARCHAR''' % table_name) + if 'context' not in existing_fields: + cur.execute('''ALTER TABLE %s ADD COLUMN context JSONB''' % table_name) # delete obsolete fields for field in existing_fields - needed_fields: @@ -3731,6 +3734,7 @@ class LoggedError(SqlMixin, wcs.logged_errors.LoggedError): ('status_item_id', 'varchar'), ('expression', 'varchar'), ('expression_type', 'varchar'), + ('context', 'jsonb'), ('traceback', 'text'), ('exception_class', 'varchar'), ('exception_message', 'varchar'), @@ -5111,7 +5115,7 @@ def get_period_total( # latest migration, number + description (description is not used # programmaticaly but will make sure git conflicts if two migrations are # separately added with the same number) -SQL_LEVEL = (105, 'change test result json structure') +SQL_LEVEL = (106, 'add context column to logged_errors table') def migrate_global_views(conn, cur): @@ -5239,10 +5243,11 @@ def migrate(): # 50: switch role uuid column to varchar do_role_table() migrate_legacy_roles() - if sql_level < 53: + if sql_level < 106: # 47: store LoggedErrors in SQL # 48: remove acked attribute from LoggedError # 53: add kind column to logged_errors table + # 106: add context column to logged_errors table do_loggederrors_table() if sql_level < 94: # 3: introduction of _structured for user fields diff --git a/wcs/templates/wcs/backoffice/logged-error.html b/wcs/templates/wcs/backoffice/logged-error.html index 532fe65df..45db74090 100644 --- a/wcs/templates/wcs/backoffice/logged-error.html +++ b/wcs/templates/wcs/backoffice/logged-error.html @@ -43,6 +43,17 @@ {% if error.expression or error.expression_type %}
  • {{ view.error_expression_type_label }}{% trans ":" %} {{ error.expression }}
  • {% endif %} + {% if error.context %} +
    • + {% for frame in view.get_context_frames %} +
    • {% if frame.source %}{{ frame.source.label }}{% endif %} +
        + {% for frame_context in frame.get_frame_lines %} +
      • {{ frame_context.label }}{% trans ":" %} {{ frame_context.value }}
      • + {% endfor %} +
    • + {% endfor %}
  • + {% endif %} {% if error.exception_class or error.exception_message %}
  • {% trans "Error message:" %} {{ error.exception_class }}: {{ error.exception_message }}
  • {% endif %} diff --git a/wcs/workflows.py b/wcs/workflows.py index 1c6aed678..81221182f 100644 --- a/wcs/workflows.py +++ b/wcs/workflows.py @@ -3068,7 +3068,9 @@ class WorkflowStatusItem(XmlSerialisable): return '' def get_admin_url(self): - return self.parent.get_admin_url() + 'items/%s/' % self.id + if self.parent: + return self.parent.get_admin_url() + 'items/%s/' % self.id + return '' def get_inspect_details(self): return getattr(self, 'label', '') @@ -3133,7 +3135,10 @@ class WorkflowStatusItem(XmlSerialisable): def check_condition(self, formdata, record_errors=True): context = {'formdata': formdata, 'status_item': self} try: - return Condition(self.condition, context, record_errors=record_errors).evaluate() + return Condition(self.condition, context, record_errors=record_errors).evaluate( + source_label=str(self.description), + source_url=self.get_admin_url(), + ) except RuntimeError: return False