[pve-devel] [PATCH manager] fix #2574: filter duplicate storage paths in ACL selector

[Dominik Csapak]

lgtm, one comment inline

On 2/4/20 11:09 AM, Fabian Grünbichler wrote:
> we don't need one entry per storage per node, since ACLs just operate
> on the storage ID and don't care about the node.
> 
> use the fact that record IDs are unique in ExtJS stores, and just re-use
> our ACL 'ID' as record ID.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> we could also re-define contains to filter by value, or use
> Ext.Array.findBy but all of that seems less straightforward to me..

another alternative would be to define a 'model' and use idProperty 
(which defaults to 'id') there:

Ext.define('model-name', {
   extends "modelclass" (cant remember just now),

   fields: ['foo','bar'],
   idProperty: 'foo',
});

> 
>   www/manager6/data/PermPathStore.js    | 28 ++++++++++++++-------------
>   www/manager6/form/PermPathSelector.js |  4 ++--
>   2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/www/manager6/data/PermPathStore.js b/www/manager6/data/PermPathStore.js
> index 90837d1c..2e87434e 100644
> --- a/www/manager6/data/PermPathStore.js
> +++ b/www/manager6/data/PermPathStore.js
> @@ -1,15 +1,15 @@
>   Ext.define('PVE.data.PermPathStore', {
>       extend: 'Ext.data.Store',
>       alias: 'store.pvePermPath',
> -    fields: [ 'value' ],
> +    fields: [ 'id' ],
>       autoLoad: false,
>       data: [
> -	{'value':  '/'},
> -	{'value':  '/access'},
> -	{'value': '/nodes'},
> -	{'value': '/pool'},
> -	{'value': '/storage'},
> -	{'value': '/vms'}
> +	{'id':  '/'},
> +	{'id':  '/access'},
> +	{'id': '/nodes'},
> +	{'id': '/pool'},
> +	{'id': '/storage'},
> +	{'id': '/vms'}
>       ],
>   
>       constructor: function(config) {
> @@ -23,22 +23,24 @@ Ext.define('PVE.data.PermPathStore', {
>   	PVE.data.ResourceStore.each(function(record) {
>   	    switch (record.get('type')) {
>   		case 'node':
> -		    me.add({value: '/nodes/' + record.get('text')});
> +		    me.add({id: '/nodes/' + record.get('text')});
>   		    break;
>   
>   		case 'qemu':
> -		    me.add({value: '/vms/' + record.get('vmid')});
> +		    me.add({id: '/vms/' + record.get('vmid')});
>   		    break;
>   
>   		case 'lxc':
> -		    me.add({value: '/vms/' + record.get('vmid')});
> +		    me.add({id: '/vms/' + record.get('vmid')});
>   		    break;
>   
>   		case 'storage':
> -		    me.add({value: '/storage/' + record.get('storage')});
> +		    /* multiple resource entries for storages in a cluster,
> +		     * only one ACL path! */
> +		    me.add({id: '/storage/' + record.get('storage')});
>   		    break;
>   		case 'pool':
> -		    me.add({value: '/pool/' + record.get('pool')});
> +		    me.add({id: '/pool/' + record.get('pool')});
>   		    break;
>   	    }
>   	});
> @@ -48,7 +50,7 @@ Ext.define('PVE.data.PermPathStore', {
>   	me.fireEvent('datachanged', me);
>   
>   	me.sort({
> -	    property: 'value',
> +	    property: 'id',
>   	    direction: 'ASC'
>   	});
>       }
> diff --git a/www/manager6/form/PermPathSelector.js b/www/manager6/form/PermPathSelector.js
> index 928a9621..21ca01b3 100644
> --- a/www/manager6/form/PermPathSelector.js
> +++ b/www/manager6/form/PermPathSelector.js
> @@ -2,8 +2,8 @@ Ext.define('PVE.form.PermPathSelector', {
>       extend: 'Ext.form.field.ComboBox',
>       xtype: 'pvePermPathSelector',
>   
> -    valueField: 'value',
> -    displayField: 'value',
> +    valueField: 'id',
> +    displayField: 'id',
>       typeAhead: true,
>       queryMode: 'local',
>       store: {
>