From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <l.nunner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id B898493C67
 for <pmg-devel@lists.proxmox.com>; Tue, 11 Apr 2023 13:04:08 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 9EBDD3485E
 for <pmg-devel@lists.proxmox.com>; Tue, 11 Apr 2023 13:04:08 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pmg-devel@lists.proxmox.com>; Tue, 11 Apr 2023 13:04:07 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4DD2C46E12
 for <pmg-devel@lists.proxmox.com>; Tue, 11 Apr 2023 13:04:07 +0200 (CEST)
Message-ID: <a1e0810c-8b4e-da56-ca3e-94e90cf9857d@proxmox.com>
Date: Tue, 11 Apr 2023 13:04:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
To: Thomas Lamprecht <t.lamprecht@proxmox.com>, pmg-devel@lists.proxmox.com
References: <20230407134258.199691-1-l.nunner@proxmox.com>
 <745e3d07-95e1-a649-7832-1127f6e0c921@proxmox.com>
Content-Language: en-US
From: Leo Nunner <l.nunner@proxmox.com>
In-Reply-To: <745e3d07-95e1-a649-7832-1127f6e0c921@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 1.490 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -3.246 Looks like a legit reply (A)
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pmg-devel] [PATCH pmg-api/gui/docs,
 proxmox-widget-toolkit] Extend rule system
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
 <pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>, 
 <mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>, 
 <mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Tue, 11 Apr 2023 11:04:08 -0000

On 2023-04-11 11:52, Thomas Lamprecht wrote:
> 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.
We have regression tests for PMG, which should already cover this AFAIK.
>> 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.

Not sure if I follow…

- Rule
    - Who Objects
        - Object 1
    - When Objects
        - Object 2

You would want something like this to match if either Who *or* When
produce a match? IIRC those are connected via AND right now.

>> 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.
>
> […]
I'll change the commits around accordingly (and keep the formatting in
mind for future patches), thanks!