Re: [pmg-devel] [PATCH pmg-gui v2 2/2] rules/objects: add mode selector dropdown

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

Am 21/02/2024 um 13:24 schrieb Dominik Csapak:
> for objects and object types in rules. We add a simple dropdown for the
> 'and' and 'invert' flags, to be somewhat consistent with the
> notification matchers from pve and to make the wording more clear than
> simple and/invert.
> 
> For What matches add a special warning hint, since that behaves a bit
> special because of the mail parts.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  js/Makefile                    |  1 +
>  js/ObjectGroup.js              | 64 ++++++++++++++++++++++++++++++++--
>  js/ObjectGroupConfiguration.js |  4 +++
>  js/RuleInfo.js                 | 44 +++++++++++++++++++++++
>  js/form/ModeSelector.js        | 11 ++++++
>  5 files changed, 122 insertions(+), 2 deletions(-)
>  create mode 100644 js/form/ModeSelector.js
> 
> diff --git a/js/Makefile b/js/Makefile
> index 78f2b57..4267092 100644
> --- a/js/Makefile
> +++ b/js/Makefile
> @@ -2,6 +2,7 @@ include ../defines.mk
>  
>  JSSRC=							\
>  	Utils.js					\
> +	form/ModeSelector.js				\

Please codify in the file and class name which mode this is for, as of now
the name is rather overly generic.

Something like "MatchModeSelector" would  be already better IMO.

>  	FilterProxy.js					\
>  	LoginView.js					\
>  	RoleSelector.js					\
> diff --git a/js/ObjectGroup.js b/js/ObjectGroup.js
> index 2223ffa..b91bf97 100644
> --- a/js/ObjectGroup.js
> +++ b/js/ObjectGroup.js
> @@ -10,6 +10,7 @@ Ext.define('PMG.ObjectGroup', {
>      showDirection: false, // only important for SMTP Whitelist
>  
>      ogdata: undefined,
> +    oclass: undefined,

not a fan of those needlessly abbreviated config keys, this, and the existing
one, just makes the code harder to read.

>  
>      emptyText: gettext('Please select an object.'),
>  
> @@ -32,10 +33,14 @@ Ext.define('PMG.ObjectGroup', {
>      setObjectInfo: function(ogdata) {
>  	let me = this;
>  
> +	let mode = ogdata.invert ? 'not' : '';
> +	mode += ogdata.and ? 'all' : 'any';
> +
>  	me.ogdata = ogdata;
>  
>  	if (me.ogdata === undefined) {
>  	    me.down('#oginfo').update(me.emptyText);
> +	    me.down('#modeBox').setHidden(true);
>  	} else {
>  	    let html = '<b>' + Ext.String.htmlEncode(me.ogdata.name) + '</b>';
>  	    html += "<br><br>";
> @@ -43,6 +48,12 @@ Ext.define('PMG.ObjectGroup', {
>  
>  	    me.down('#oginfo').update(html);
>  	    me.down('#ogdata').setHidden(false);
> +	    let modeSelector = me.down('#modeSelector');
> +	    modeSelector.suspendEvents();
> +	    me.down('#modeSelector').setValue(mode);
> +	    modeSelector.resumeEvents();
> +	    me.down('#modeBox').setHidden(false);
> +	    me.down('#whatWarning').setHidden(me.oclass !== 'what');
>  	}
>      },
>  
> @@ -205,13 +216,62 @@ Ext.define('PMG.ObjectGroup', {
>  	me.dockedItems.push({
>  	    dock: 'top',
>  	    border: 1,
> -	    layout: 'anchor',
> +	    layout: 'hbox',
>  	    hidden: !!me.hideGroupInfo,
>  	    itemId: 'ogdata',
>  	    items: [
> +		{
> +		    xtype: 'container',
> +		    itemId: 'modeBox',
> +		    hidden: true,
> +		    width: 220,
> +		    padding: 10,
> +		    layout: {
> +			type: 'vbox',
> +			align: 'stretch',
> +		    },
> +		    items: [
> +			{
> +			    xtype: 'box',
> +			    html: `<b>${gettext("Match if")}</b>`,
> +			},
> +			{
> +			    xtype: 'pmgModeSelector',
> +			    itemId: 'modeSelector',
> +			    padding: '10 0 0 0',
> +			    listeners: {
> +				change: function(_field, value) {
> +				    let invert = value.startsWith('not');
> +				    let and = value.endsWith('all');
> +
> +				    let url = `${me.baseurl}/config`;

nit: I'd rather avoid the extra variable here, just an inline:

url: `${me.baseurl}/config`,

> +
> +				    Proxmox.Utils.API2Request({
> +					url,
> +					method: 'PUT',
> +					params: {
> +					    and: and ? 1 : 0,
> +					    invert: invert ? 1 : 0,

nit: while the separate variables have IMO a value here, the ternary
could be used on assignment to and/invert already, then one could just
use `param: { and, invert }` here (maybe not in a single line)

> +					},
> +					success: function() {
> +					    me.fireEvent('modeUpdate', me);
> +					},

tiny nit: could be an arrow fn:

success: () => me.fireEvent('modeUpdate', me),

> +				    });
> +				},
> +			    },
> +			},
> +			{
> +			    xtype: 'displayfield',
> +			    itemId: 'whatWarning',
> +			    hidden: true,
> +			    value: gettext("Caution: What Objects match per mail part, so be careful with any option besides 'Any matches'."),

Hmm, this is a bit hard to word better, but maybe something slightly adapted
like:

"Caution: 'What Objects' match each mail part separately, so be careful with any option besides 'Any matches'."

Also, we could hide this is 'Any matches' is selected, and making it span over
all columns would make it look better too. Maybe in some bottom docked bar,
as otherwise it can flow over the description.

btw. why does the left store reloads after every match selection change?
Maybe an explicit reload button there (simple icon without text) could be better
for that?


> +			    userCls: 'pmx-hint',
> +			},
> +		    ],
> +		},
>  		{
>  		    xtype: 'component',
> -		    anchor: '100%',
> +		    flex: 1,
>  		    itemId: 'oginfo',
>  		    style: { 'white-space': 'pre' },
>  		    padding: 10,
> diff --git a/js/ObjectGroupConfiguration.js b/js/ObjectGroupConfiguration.js
> index 1d72851..f59f5ed 100644
> --- a/js/ObjectGroupConfiguration.js
> +++ b/js/ObjectGroupConfiguration.js
> @@ -30,6 +30,7 @@ Ext.define('PMG.ObjectGroupConfiguration', {
>  
>  	var right = Ext.create('PMG.ObjectGroup', {
>  	    otype_list: me.otype_list,
> +	    oclass: me.ogclass,
>  	    border: false,
>  	    region: 'center',
>  	    listeners: {
> @@ -40,6 +41,9 @@ Ext.define('PMG.ObjectGroupConfiguration', {
>  			left.run_editor();
>  		    }
>  		},
> +		modeUpdate: function() {
> +		    left.reload();
> +		},

nit, could be an arrow fn:

modeUpdate: () => left.reload(),

>  	    },
>  	});
>  
> diff --git a/js/RuleInfo.js b/js/RuleInfo.js
> index c29c0ca..d53c1c5 100644
> --- a/js/RuleInfo.js
> +++ b/js/RuleInfo.js
> @@ -120,6 +120,8 @@ Ext.define('PMG.RuleInfo', {
>  			name: oc,
>  			oclass: oc,
>  			type: true,
> +			invert: ruledata[`${oc}-invert`],
> +			and: ruledata[`${oc}-and`],
>  			leaf: false,
>  			expanded: true,
>  			expandable: false,
> @@ -162,6 +164,25 @@ Ext.define('PMG.RuleInfo', {
>  	    return true;
>  	},
>  
> +	updateMode: function(field, value) {
> +	    let me = this;
> +	    let vm = me.getViewModel();
> +	    let oclass = field.getWidgetRecord().data.oclass;
> +
> +	    let params = {};
> +	    params[`${oclass}-invert`] = value.startsWith('not') ? 1 : 0;
> +	    params[`${oclass}-and`] = value.endsWith('all') ? 1 : 0;
> +
> +	    Proxmox.Utils.API2Request({
> +		url: `${vm.get('baseurl')}/config`,
> +		method: 'PUT',
> +		params,
> +		success: function() {
> +		    me.reload();
> +		},

nit, could be an arrow fn:

success: () => me.reload(),

> +	    });
> +	},
> +
>  	control: {
>  	    'treepanel[reference=usedobjects]': {
>  		drop: 'addDrop',
> @@ -169,6 +190,9 @@ Ext.define('PMG.RuleInfo', {
>  	    'tabpanel[reference=availobjects] > grid': {
>  		drop: 'removeDrop',
>  	    },
> +	    'pmgModeSelector': {
> +		change: 'updateMode',
> +	    },
>  	},
>      },
>  
> @@ -311,6 +335,26 @@ Ext.define('PMG.RuleInfo', {
>  		    },
>  		    flex: 1,
>  		},
> +		{
> +		    header: gettext('Match if'),
> +		    xtype: 'widgetcolumn',
> +		    width: 200,
> +		    widget: {
> +			xtype: 'pmgModeSelector',
> +		    },
> +		    onWidgetAttach: function(col, widget, rec) {
> +			if (rec.data.type && rec.data.oclass !== 'action') {
> +			    let mode = rec.data.invert ? 'not' : '';
> +			    mode += rec.data.and ? 'all' : 'any';
> +			    widget.suspendEvents();
> +			    widget.setValue(mode);
> +			    widget.resumeEvents();
> +			    widget.setHidden(false);
> +			} else {
> +			    widget.setHidden(true);
> +			}
> +		    },
> +		},
>  		{
>  		    text: '',
>  		    xtype: 'actioncolumn',