Re: [pmg-devel] [PATCH pmg-api/gui/docs, proxmox-widget-toolkit] Extend rule system

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

For now some higher level review inline from my side, not sure if I get to
a more in-depth for each patch soonish.

Am 07/04/2023 um 15:42 schrieb Leo Nunner:
> This series introduces two new features for the PMG rule system: object
> negation, and match groups. Negation allows the user to negate objects
> inside a rule, meaning that they will evaluate to true if their
> condition is NOT fulfilled.

Do we really don't have any test system for the rule system, if that'd be some
technical debt to address probably rather sooner than later.

> 
> The second feature, match groups, allows objects to be chained together.
> A match group functions as a container for multiple other objects; it
> will only evaluate to true if all its members evaluate to true. Match groups
> are connected via logical 'or' to all other objects inside the rule;
> take the following rule system:
> 
> - Rule
>     - Match Group
> 	- Object 1
> 	- Object 2
>     - Object 3
> 
> It will match if either (Object 1 && Object 2), or if Object 3
> evaluate to true.

Why not allow or matches? Most mail filter tools (like e.g., the x-sieve
fronted of open-xchange) allow to configure if all or any of a group's matching
rules must trigger for the whole group to be true.

I mean, OR matching can be workarounded by duplicated rules but if we add
groups with AND combination, then OR is something that I'd think is also
expected to be there.

> 
> To properly achieve match groups inside the GUI, the rule overview was
> reworked as a tree, instead of a simple list. It can now be
> collapsed/expanded, and each object type (except actions) has a single
> subfolder called 'Match Group'.
> 
> In combination, these features should cover quite a few use cases, and
> make it possible to model more complex rule systems.
> 
> pmg-api:
> 
> Leo Nunner (8):

can we please drop the weird feature prefix, doesn't really add anything and is
relatively hard to parse as the subsystem is missing – PMG is somewhat small
enough in scope, so one can guess that; but I'd rather have that explicit.

Also, order and separation is a bit odd as some things that are implementing a
single semantic work are split, and others like the api expand+add are merged,
also making the whole thing work only after exposing it via the API just feels
plain wrong and is actually risky if someone overlooks this and applies the
first few patches already.

So, order/separation could be IMO better if:

commit 2 ("parse negation value into objects"), and 4 ("implement matching
logic") are squashed into a "rule system: implement match negation" as they
implement the internal/backend functionallity, and then exposing that via the
api could be done in the next commit. The one adding the field to the rule db
schema could be also squashed into the commit 2 & 4, but less hard feelings
there, albeit I'd note in the commit message that it's a preparation for the
next commits.


>   feature: negation: add field to database
if not squashed:

rule db: add negation field

>   feature: negation: parse negation value into objects
would always squash

>   feature: negation: expand/implement API endpoints

That one could be actually split, the part that extends the existing API could
be squashed and the addition of the new "get/set object group" is rather
unrelated and something different, so should probably be it's own commit, i.e.
with a subject like:

api: rules: make object group settings editable

or:

api: rule group: make existing rules updateable


>   feature: negation: implement matching logic

needs to order before exposing it via the API and should be suqashed

>   feature: match groups: add field to database
>   feature: match groups: parse field into objects
>   feature: match groups: update API endpoints
>   feature: match groups: implement matching logic

same for above w.r.t. s/feature/<actual subsys>/ for the commit subject and
order/sepration.