Re: [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On September 16, 2021 9:19 am, Thomas Lamprecht wrote:
> FYI, it seems you sent the series to PVE devel by mistake.

dang, yeah, shell history got me there I guess.

> 
> On 15.09.21 15:41, Fabian Grünbichler wrote:
>> like for manual pulls, but persisted in the sync job config and visible
>> in the relevant GUI parts.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> 
>> Notes:
>>     GUI is read-only for now (and defaults to no filtering on creation), as this is
>>     a rather advanced feature that requires a complex GUI to be user-friendly
>>     (regex-freeform, type-combobox, remote group scanning + selector with
>>     additional freeform input).
> 
> above paragraph would belong into the commit message IMO.
> 
>>     
>>     I did test the API manually though to see whether it works as expected, and
>>     updating the filter list by overwriting with a new one passed in as multiple
>>     parameters works as expected.
>>     
>>     if we want to make this configurable over the GUI, we probably want to switch
>>     the job edit window to a tabpanel and add a second grid tab for selecting
>>     the groups
> 
> we could also get away by adding it in the advanced section for now.

it has the problem of "adding an arbitrary amount of filters", which 
might take up a lot of space, which might make it nicer to move to a 
separate tab. I am not yet sure which direction to go for this.

>> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
>> index 47e65ae3..2399f11f 100644
>> --- a/www/window/SyncJobEdit.js
>> +++ b/www/window/SyncJobEdit.js
>> @@ -199,6 +199,18 @@ Ext.define('PBS.window.SyncJobEdit', {
>>  	],
>>  
>>  	columnB: [
>> +	    {
>> +		fieldLabel: gettext('Backup Groups'),
>> +		xtype: 'displayfield',
>> +		name: 'groups',
>> +		renderer: function(value, metadata, record) {
>> +		    if (!value) return gettext('All');
>> +		    return Ext.String.htmlEncode(value, metadata, record);
> 
> Ext.String.htmlEncode only takes a single parameter
> https://docs.sencha.com/extjs/7.0.0/classic/Ext.String.html#method-htmlEncode

seems like I copied that from an earlier mistake - render_optional_owner 
- made by me as well ;)

> besides that you could use a arrow fn here, the following would seem quite ok for me for
> a renderer:
> 
> renderer: v => v ? Ext.String.htmlEncode(v) : gettext('All'),

yeah, that looks cleaner.

>> +		},
>> +		cbind: {
>> +		    hidden: '{isCreate}',
>> +		},
>> +	    },
>>  	    {
>>  		fieldLabel: gettext('Comment'),
>>  		xtype: 'proxmoxtextfield',
>> 
> 
>