Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Master post mortem crash manager adt #32876

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Elkasitu
Copy link
Contributor

commented Apr 23, 2019

  • add description option, to be injected into the CrashManager's body
  • rename name to label
  • log deactivated automated actions
  • rename act_xmlid field to action
  • tests
  • simplify spec

@robodoo robodoo added the seen 🙂 label Apr 23, 2019

@C3POdoo C3POdoo added the RD label Apr 23, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 23, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from fd21557 to 13a333f Apr 24, 2019

@robodoo robodoo removed the CI 🤖 label Apr 24, 2019

@Elkasitu Elkasitu marked this pull request as ready for review Apr 24, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from 13a333f to 15c7659 Apr 24, 2019

@Elkasitu Elkasitu requested a review from lcontzen Apr 25, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from 15c7659 to bd82098 Apr 25, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 25, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from ba25a5e to f798e5a May 2, 2019

@robodoo robodoo removed the CI 🤖 label May 2, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from f798e5a to 6759158 May 2, 2019

@robodoo robodoo added the CI 🤖 label May 2, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from 6759158 to 0de3a60 May 3, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 3, 2019

@Elkasitu Elkasitu requested a review from VincentSchippefilt May 8, 2019

@VincentSchippefilt
Copy link
Contributor

left a comment

I reviewed the javascript parts, it's OK on the principle and requires minor changes

Show resolved Hide resolved addons/web/static/src/js/core/dialog.js Outdated
Show resolved Hide resolved addons/web/static/src/js/services/crash_manager.js Outdated
Show resolved Hide resolved addons/web/static/src/js/services/crash_manager.js Outdated
Show resolved Hide resolved addons/web/static/src/js/core/dialog.js Outdated

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from 0de3a60 to 5e00978 May 9, 2019

@robodoo robodoo removed the CI 🤖 label May 9, 2019

@Elkasitu

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@VincentSchippefilt Let me know if it's OK now and I'll start writing some JS tests

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch 2 times, most recently from 91ada89 to ca4a8de May 9, 2019

'target': 'new',
'views': [[
self.env.ref(
'base_automation.base_automation_failure_wizard_form').id,

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 10, 2019

Member

Why don't you open the failed automation itself in its form view?
There is a button on the form to deactivate it (to avoid the problem).
The user also has the possibility to edit the code (to fix the problem).

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu May 10, 2019

Author Contributor

Could be done, but it provides no context to whoever is fixing it and more importantly, it does not display the warning message saying "if you do this you'll most likely corrupt your data".

At this point I don't care about the approach as long as everyone agrees on a spec

@@ -309,7 +340,26 @@ def base_automation_onchange(self):
action_rule = self.env['base.automation'].browse(action_rule_id)
result = {}
server_action = action_rule.action_server_id.with_context(active_model=self._name, onchange_self=self)
res = server_action.run()
action = {

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 10, 2019

Member

This dictionary definition should only be evaluated in case of an exception.

Show resolved Hide resolved addons/base_automation/models/base_automation.py
'form']],
},
'options': {'additional_context': {'active_ids': self.ids}},
'visible': self.user_has_groups('base.group_system'),

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 10, 2019

Member

If the action is not visible, maybe it is simpler to not put the action in the exception at all...

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu May 10, 2019

Author Contributor

Yes, I did this so that the burden of checking access rights is on the implementation of post-mortem actions instead of on the one who defines a post-mortem action, the __init__ will discard it if it's not visible.

In hindsight, it might be better to just call it groups and pass a list of groups and then letting the __init__ call user_has_groups

if action and action.get('visible', True):
self.action = action
else:
self.action = False

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 10, 2019

Member

The permission is defined where the action is defined.
This code processes something that says: "I made some effort to compute something that is useless."

Show resolved Hide resolved odoo/exceptions.py
'msg': msg,
}
vals = [self, message, '']
_logger.info(message)

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 10, 2019

Member

An Exception is essentially a data structure.
Logging in its initializer is inappropriate IMHO: you can instantiate an exception without raising it.

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu May 10, 2019

Author Contributor

True, but given your comment below, should we even log this and if so, is info really the right log level?

'parent': view.inherit_id.id or not_avail,
'msg': msg,
}
vals = [self, message, '']

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 10, 2019

Member

Putting all that data in the error message is useless if the error provides a direct link to the view.

Show resolved Hide resolved odoo/http.py
@@ -1367,6 +1367,10 @@ def load_views(self, views, options=None):
:param options['action_id']: id of the action to get the filters
:return: dictionary with fields_views, fields and optionally filters
"""
# populate the context with essential information for debugging problematic views
# all the way up the stack
self = self.with_context(loaded_view=[self._name, views, options])

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 10, 2019

Member

That sucks. Looks like a lazy way to fix a problem 😉

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu May 10, 2019

Author Contributor

The alternative would be passing down these 3 parameters down to fields_view_get, post_process_and_fields and then to post_process (which will be called recursively), given that and the fact that the task is supposed to be backportable, I thought this was the best approach

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

The size of loaded_view is not dynamic. I recommend to use a tuple instead of a list.

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from ca4a8de to 1faee50 May 10, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 10, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from 9f26f1d to 34cfb05 May 14, 2019

Elkasitu added some commits Apr 15, 2019

[IMP] web: introduce actionable exceptions
With this commit, it is possible for the CrashManager to propose actions
to the user(s) concerned by the crash/error.

If whatever triggered the exception properly wraps the exception with a
`metadata` attribute, the web client will display an extra button
in the error dialog, which will then launch an action programmed
by a developer.

This action can be anything, from a server action, to a view action, to
a redirect action...

This `metadata` attribute is essentially a dictionary containing up to 4
entries, those that are required are marked with (*):

    - label (*) : the label of the button displayed on the CrashManager
    that will trigger the action
    - action (*) : an action xmlid or a dict representing an action
    - options : any additional context data for the execution of the action
    - visible : whether the button should be visible or not, e.g. by
    checking that the current user has the right permissions
    - description : a message describing the action button to the user

This allows admins of databases to perform actions to fix problems on
their own or to fix their own customizations without external
intervention, it can also be used to provide support members a quick way
into the heart of a technical problem.

Task-ID: 1931559
[IMP] base_automation: trigger actionable exception
Rationale:
With base_automation, users can trigger custom code on standard
workflows that will suit their specific needs (i.e. setting a field
after confirming a SO).  Sometimes, during these *simple* actions, an
error can occur due to various variables, such as a recent migration,
programming errors, module upgrades, etc., in such cases, it would be
great to drop the custom behavior temporarily to restore the standard
behavior, this would allow clients to keep doing business despite the
reduced features.

With this patch, if a base_automation triggers a crash, the crash
manager will propose (to users of the base_system group) the ability to
deactivate the infringing base_automation in order to continue their
workflow without the need to wait for external support.

Task-ID: 1931559

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 14, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-post-mortem-crash-manager-adt branch from 34cfb05 to d11932f May 15, 2019

@robodoo robodoo removed the CI 🤖 label May 15, 2019

Elkasitu added some commits Apr 23, 2019

[IMP] base: implement ViewError
This kind of error is triggered whenever the display of a backend view
failed for whatever reason, additionally, this raising this error will
automatically allow the admin / support staff to go to the problematic
view's technical view and fix it / disable it.

Task-ID 1931559

@robodoo robodoo added the CI 🤖 label May 15, 2019

'views': [[
self.env.ref(
'base_automation.base_automation_failure_wizard_form'
).id, 'form']],

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

indentation sucks. Simply define

view = self.env.ref('base_automation.base_automation_failure_wizard_form')

above and use (view.id, 'form') here

'views': [[
self.env.ref(
'base_automation.base_automation_failure_wizard_form').id,
'form']],

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

same comment as above

'\t<field name="shitty_field"/>\n'
'</xpath>'
)
shitty_view.active = True

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

Nice trick 👍

'parent': view.inherit_id.id or not_avail,
'msg': msg,
}
_logger.debug(log)

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

The logger can handle the string interpolation, only when actually logged:

_logger.debug(what, msg=msg, view_name=..., viewid=..., ...)
not_avail = _('n/a')
log = (
"%(msg)s\n\n" +
_("Error context:\nView `%(view_name)s`") +

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

The log messages should not be translated.

@@ -1158,7 +1170,8 @@ def postprocess_and_fields(self, model, node, view_id):
msg_lines.append(msg_fmt % name)
for line in lines:
msg_lines.append(line_fmt % line)
self.raise_view_error("\n".join(msg_lines), view_id)
message = "\n".join(msg_lines)
raise ViewError(message, self.browse(view_id))

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

no log_view_error here?

self.name = name
self.value = value
self.args = (name, value)
self.actions = [] or actions

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member
Suggested change
self.actions = [] or actions
self.actions = actions or []
"to modify and fix it and/or disable it."
)
}])
except_orm.__init__(*vals)

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

I prefer explicit variables that are defined and modified above

except_orm.__init__(self, msg, value, actions)
@@ -1367,6 +1367,10 @@ def load_views(self, views, options=None):
:param options['action_id']: id of the action to get the filters
:return: dictionary with fields_views, fields and optionally filters
"""
# populate the context with essential information for debugging problematic views
# all the way up the stack
self = self.with_context(loaded_view=[self._name, views, options])

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

The size of loaded_view is not dynamic. I recommend to use a tuple instead of a list.

# to properly find the view id that causes the error
view.env[model].with_context(inherit_branding=True).load_views(*loaded_view[1:])
except ViewError as e:
vals = [self, e.name, e.value, e.actions]

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo May 23, 2019

Member

Why don't you reintroduce a private method like view._raise_view_error(msg) on ir.ui.view to:

  • retry the view construction like above and simply reraise e, or
  • log the error and raise a ViewError with the right action.

This would automatically factor out the logging part, and ViewError would have no actual code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.