[pve-devel] [PATCH proxmox-widget-toolkit] combogrid: add handling for historic set values currently not available

[Dominik Csapak]

looks mostly good, some comments inline

On 7/22/19 5:16 PM, Thomas Lamprecht wrote:
> We can often run into situations where a value set in the past is not
> valid anymore. An example could be a deleted network bridge, e.g., we
> set the vNIC of a VM to 'vmbr1' but then we decide to obsolete that
> and delete that one, one would now expect that the field gets marked
> as invalid when editing the VM's vNIC, so add that behavior.
> 
> As sometimes this can be valid and wanted behavior (e.g., usb
> passthrough, which is hot-pluggable), also add a switch do restore
> the old behavior.
> 
> Note that the empty value is not handled by this change, we let the
> existing "allowBlank" config switch handle that one.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
> 
> note that this alone is not fully enough to imrpove our network selectors
> behaviour, for that additionally a fix where we mask out any bits which do not
> define the network in a separate patch
> 
> But as this is itself an improvement, IMO, and makes the behaior a bit less
> confusing I sent it already out
> 
>   form/ComboGrid.js | 52 ++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/form/ComboGrid.js b/form/ComboGrid.js
> index a0c762c..608f613 100644
> --- a/form/ComboGrid.js
> +++ b/form/ComboGrid.js
> @@ -29,6 +29,7 @@ Ext.define('Proxmox.form.ComboGrid', {
>   
>       config: {
>   	skipEmptyText: false,
> +	notFoundIsValid: false,

as i mentioned already off-list, i am not a huge fan of negated variables

counter-proposal: 'valueMustExist' with default of true ?

>   	deleteEmpty: false,
>       },
>   
> @@ -291,6 +292,43 @@ Ext.define('Proxmox.form.ComboGrid', {
>           return picker;
>       },
>   
> +    isValueInStore: function(value) {
> +	var me = this;
> +	var store = me.store;
> +	var found = false;
> +
> +	if (!store) {
> +	    return found;
> +	}
> +
> +	if (Ext.isArray(value)) {
> +	    Ext.Array.each(value, function(v) {
> +		if (store.findRecord(me.valueField, v)) {
> +		    found = true;
> +		    return false; // break
> +		}
> +	    });
> +	} else {
> +	    found = !!store.findRecord(me.valueField, value);
> +	}
> +
> +	return found;
> +    },
> +
> +    validator: function (value) {
> +	var me = this;
> +
> +	if (!value) {
> +	    return true; // handled later by allowEmpty in the getErrors call chain
> +	} > +
> +	if (!(me.notFoundIsValid || me.isValueInStore(value))) {
> +	    return gettext('Invalid Value');
> +	}
> +
> +	return true;
> +    },

we have to be careful here, since if a child overwrites validator
(afaics, only usbselector and nodeselector use a custom validator)
we may have to do a 'callParent' there somewhere...

the other option would probably be to overwrite 'getErrors' and do
a callParent there

> +
>       initComponent: function() {
>   	var me = this;
>   
> @@ -364,16 +402,7 @@ Ext.define('Proxmox.form.ComboGrid', {
>   		}
>   		var found = false;
>   		if (def) {
> -		    if (Ext.isArray(def)) {
> -			Ext.Array.each(def, function(v) {
> -			    if (store.findRecord(me.valueField, v)) {
> -				found = true;
> -				return false; // break
> -			    }
> -			});
> -		    } else {
> -			found = store.findRecord(me.valueField, def);
> -		    }
> +		    found = me.isValueInStore(def);
>   		}
>   
>   		if (!found) {
> @@ -383,6 +412,9 @@ Ext.define('Proxmox.form.ComboGrid', {
>   			me.setValue(def, true);
>   		    } else {
>   			me.setValue(def);
> +			if (!me.notFoundIsValid) {
> +			    me.markInvalid(gettext('Invalid Value'));
> +			}
>   		    }
>   		}
>   	    }
>