Added support for adapters in guard handler

[xispa]

Description of the issue/feature this PR addresses

This Pull Request allows to adapt the behavior of core's guard handler.

Use case

Imagine you want to be more restrictive regarding the verification of results in such a way that you only want the transition verify to be available if the submitted result is in-range. For this to work, you'd need to change the guard expresion from the workflow definition in your add-on, reimport workflows.xml, and update role mappings for pre-existing objects. This improvement makes the task far easier:

1. Create a new adapter in your add-on:

from bika.lims.api.analysis import is_out_of_range
from bika.lims.interfaces import IGuardAdapter

class AnalysisGuardAdapter(object):
    implements(IGuardAdapter)

    def __init__(self, context):
        self.context = context

    def guard(self, action):
        """Returns False if action is verify and result is not in range
        """
        if action == "verify":
            return not is_out_of_range(self.context)[0]
        return True

2. Register the adapter:

  <adapter
    name="my.addon.analysis_guard"
    for="bika.lims.interfaces.IAnalysis"
    factory=".adapters.AnalysisGuardAdapter"
    provides="bika.lims.interfaces.IGuardAdapter"
  />

Current behavior before PR

No support for adapters in guard handler

Desired behavior after PR is merged

Support for adapters in guard_handler

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

[ramonski]

From an architectural point of view, I would rather query all named adapters, where the name is the transition.
If such an adapter is found, I would let handle this adapter the guarding solely.
It would leave then the whole logic to the adapter without bailing out conditionally in the pre_guard or after_guard (which I think should be triggered events instead adapter calls).
I think that it would be much more explicit then and also more performant, as only one adapter lookup would be involved instead of always two.
However, regarding the events, I would welcome such before/after events to eventually purge a cache or something like this...

What do you think?

[ramonski]

Maybe it get clearer when we think about the purpose of the after_guard adapter.
An eventual implementation in an Add-on would rely on the current logic of the core handler, why probably only the pre_handler would be used by everyone (to avoid side-effects).
Furthermore, the logic is restrictive only, e.g. that you can only deny a transition, but can not grant it or disable the check completely. Therefore, you would have to override it again in your Add-on...

[xispa]

If such an adapter is found, I would let handle this adapter the guarding solely.
It would leave then the whole logic to the adapter without bailing out conditionally in the pre_guard or after_guard (which I think should be triggered events instead adapter calls).

Ouch!... You are totally right. before+after guards do not make sense at all. It does for events, indeed, but not here. Nevertheless, I still think the adapter's guard should be restrictive. The reason is that you might have multiple add-ons, each one with it's own guard for that same type. What you do In case that one guard evaluates to "True" and another one to "False"?. The only solution I can think of is to give "priority" (with a sort parameter in the adapter) so the guard with lowest priority gets evaluted first and if the first one returns "False", return "False".... you see, you end-up with same problem: the "restrictive" is unavoidable.

[xispa]

If we want to support multiple guard adapters (because different add-ons might have their own), then I cannot use a named adapter, because only the first adapter with that name will be returned by zope.

If in turn, we do not want to explicitily pass the action to the adapter's guard function, then we have to use subscriber adapters instead.

If we use subscriber adapters, the snippet to registry them in zcml would be more confusing because the second parameter will always be * symbol.

Therefore, I thought was better to just pass the action to the guard function from the adapter and let the add-on handle that by its own.

[ramonski]

Ok, the usage in multiple Add-ons is indeed something I did not consider, so somehow all Adapters need to be considered. So it is ok to make the logic restrictive unless an adapter returns False.

[ramonski]

Thanks! 👍