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

[Dominik Csapak]

On 7/23/19 6:56 PM, Thomas Lamprecht wrote:
> 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? ^^

i am ok with all of them, just pick what you hate the least ;)

>   
>>>        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.
> 

yeah ok