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

[Thomas Lamprecht]

On 7/23/19 10:15 AM, Dominik Csapak wrote:
> 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 ?

Hmm, while the positive vs. negative is a bit better I do not like the
"exists" to much, IMO it does not really fits the meaning of this.

"storeMustContainValue"

A bit long, but for those few places where we really need it maybe OK?

We use the "isValueInStore" method as main check guarded by above
config switch, so maybe: 'enforceValueInStore', or 'validateValueInStore',

I "hate" them all, so let's just pick the one seeming the most OK
and be good? ^^
 
>>       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


Yes, but in this special case that'd be a "bug" in said child panels,
but yes - we'd need to re-check them and ideally adapt them.