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

Optional actions on CrashManager #41855

Closed
wants to merge 2 commits into from

Conversation

Elkasitu
Copy link
Contributor

This PR implements:

  1. Actions to be executed in case of an Exception during a business flow (except_orm) in the form of buttons.

  2. Two actions implemented in the case of an Automated Action that crashes:
    - Disable the action (archive)
    - Go to the form view of the action in edit mode (presumably to fix the action)

Crash during a business flow:
crashmanager1

Opening the action form view in edit mode:
crashmanager2

Disabling the action itself:
crashmanager3

Supersedes (partially) #32876

Adrian Torres added 2 commits December 13, 2019 14:13
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 an
`action` 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 `action` attribute is essentially a list of dictionaries 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
    - 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
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
@Elkasitu
Copy link
Contributor Author

PS: I'm aware this is an IMP but it was requested for 13.0 ...

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Dec 13, 2019
@C3POdoo C3POdoo added the RD research & development, internal work label Dec 13, 2019
@@ -13,4 +13,39 @@
<field name="active" eval="False" />
</record>
</data>
<data>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized this isn't doable in stable, we can get rid of this action and only keep the one that opens the form view of the automated action

@beledouxdenis
Copy link
Contributor

beledouxdenis commented Dec 17, 2019

I want to insist that this is just my opinion, trying to be as constructive as possible about possibilities and alternatives that could be considered to improve this feature. It might be a crappy opinion, do not take my words as the absolute truth. I was "dropped" in this task without knowing the discussions that may have already occured prior to my intervention, and I might say things that have already been discarded during these possible past discussions I am not aware of.

In my opinion, things could be done to help the user to understand what happens.

Currently, the buttons "Disable Action" and "Go to action" appears, and for a lambda, non-developer, user, I would say "magically":

  • what's the reason these buttons appear for this traceback while they generally do not appear,
  • what do they do, I want to know before I click on them because I am a coward.

Besides, as you figured out, this would never be able to land in stable (if we want to, as you stated, this is rather an improvement than an actual bugfix) because it relies:

  • On a new model (transient),
  • it requires a new "in-database" qweb view.

I believe this would be possible to overcome both above problems by improving the JS/static Qweb template CrashManager.error, allowing it to:

  • Change the content of the label "An error occurred" through options, for instance to "An error occured during the execution of the automation action '[name of the automated action]'",
    (This could appear even if you are not an admin)
  • Allow to pass "action buttons" to add in addition to the already shown button "Copy to clipboard", either by options as you did, either by inheritance (t-extend?).
  • Allow to pass new explanation paragraphs in addition to
    Please use the copy button to report the error to your support service.
    
    So you are able to explain what each action button does (your scary danger zone).

It would be feasible to extend the static qweb template CrashManager.error to add the buttons below the "Copy to clipboard" (a bit like what we do for standard form smart buttons), and then to extend the related CrashManager js widget to implement the behavior of these new buttons, inside the base_automation module, leaving the base CrashManager widget untouched.
I just state possibilities. I do not have the pretention telling my feeling about where the buttons should be is better than yours, neither that the base CrashManager should be left untouched. I just say this is a possibility.
It could also be a mix, adding the buttons along the "OK", as you did, but then still add their explanation in the body of the dialog, by inheriting the static xml template within the base automation module.

That way:

  • The user could directly and understandably (like, in the first dialog that is shown) know the exception is due to an automated action, and that he can take actions to solve it (Disable / Go to automated action) with an explanation of what does each button.
  • Avoid the double dialog chain, to just add explanations, which is not the best user experience,
  • It could land in stable, as this does not add any transient mode nor "in-database" templates, only static templates,
  • Bonus: Non-admin users can be warned which automated action is failing, and notify their sys admin with this information, while before, there wouldn't be any visible change for them. We could even consider a "Share to my admin" button : D.

@Elkasitu
Copy link
Contributor Author

@beledouxdenis Don't worry there have been many differing opinions on this task and a lot of back-and-forth which is why the task was not merged despite it having been "ready" months ago, there's even opinions that dispute the usefulness of this feature and that it would bring more problems than it would fix :^)

Idk if you're aware but the main goal of the task was to aid migrations: after the migration of a db with a lot of customizations, many different things can fail (automations, custom views, etc.) the goal was to provide something generic to disable these customizations on-the-fly so that business can continue.

We thought injecting actions into the CrashManager as buttons was great because actions are already generic enough that we don't need to do much to get something working, this was intended for master though and the original implementation also covered views that crash (see #32876), and we will probably reuse this behavior in the future.

As for the explanation of what the button does, it already exists even though it is more subtle than what you suggest: the action dict injected into the exception can contain a 'description' key that will be displayed as a tooltip when you hover over the button. I think my original implementation did actually append the description to the dialog body but after a verbal discussion with @VincentSchippefilt we went for the tooltip (but my memory is not that great so I may be misremembering).

Your proposal on using static xml templates makes sense for a stable but it'd mean reimplementing the whole shebang and Antony didn't want me working too much on this right now since there are other priorities (namely your branch on mapped optimizations).

With all of that said, I think it's an OK compromise to just have the "Go to action" button and then add / refine in master, what do you think?

@beledouxdenis
Copy link
Contributor

I have written #42481 implementing what I suggested in my previous comment.

During it I noticed there is an issue with the self in self._add_postmortem_action(e) within the method base_automation_onchange. There self is the record on which is called the automated action, and not the automated action. It should be

action_rule._add_postmortem_action(e)

When I have the chance to talk with Antony, I will discuss with him how we should proceed, if we take your implementation strictly, or the one with my suggestions, or a mix of both.

@beledouxdenis
Copy link
Contributor

Closed with 8259039

@robodoo robodoo added closed 💔 and removed CI 🤖 Robodoo has seen passing statuses labels Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants