[pve-devel] [PATCH] firewall autodisable GUI update

[Alen Grizonic]

Yes, I agree. Some part of the code has to be let out.

Other comments inline:

On 06/29/2015 07:21 PM, Dietmar Maurer wrote:
> comments inline
>
>>   	var rows = {};
>>   
>> +        var submit_twice = function(enable) {
>> +
>> +	    var me = this;
>> +
>> +            var form = me.formPanel.getForm();
>> +
>> +            var values = me.getValues();
>> +
>> +            if ((values.enable == 1) && (enable != 2)) {
>> +                values.enable = 2;
>> +            } else if (enable == 2) {
>> +                values.enable = 1;
>> +            }
>> +
> -----from-here
>> +            Ext.Object.each(values, function(name, val) {
>> +                if (values.hasOwnProperty(name)) {
>> +                    if (Ext.isArray(val) && !val.length) {
>> +                        values[name] = '';
>> +                    }
>> +                }
>> +            });
> -----
>
> Above code is not really required, because this is a special form.

Removed.

>
>> +
>> +            if (me.digest) {
>> +                if (values.enable == 2) {
>> +                    me.digest = "";
>> +                } else {
>> +                    values.digest = me.digest;
>> +                }
>> +            }
> are you sure this work? I think the digest changes after the first call?

The digest is not changed after the first call, so it has to be removed 
/ cleared, otherwise is not working correctly.

>
>> +
>> +            if (me.backgroundDelay) {
>> +                values.background_delay = me.backgroundDelay;
>> +            }
> we do not need backgroundDelay here.

Removed.

>
>> +
>> +            var url =  me.url;
>> +            if (me.method === 'DELETE') {
>> +                url = url + "?" + Ext.Object.toQueryString(values);
>> +                values = undefined;
>> +            }
> AFAIK we never send 'DELETE' here, so above code is not required.

Removed.

>
>> +
>> +            PVE.Utils.API2Request({
>> +                url: url,
>> +                waitMsgTarget: me,
>> +                method: me.method || (me.backgroundDelay ? 'POST' : 'PUT'),
>> +                params: values,
>> +                failure: function(response, options) {
>> +                    if (me.onFailedHook) {
>> +                        me.onFailedHook(response);
> why do we need that onFailedHook? (or onFailerHook??)

The onFailed/onFailureHook was used for the notification message. Ok. 
Now is used in the other way.

>
>> +                    } else {
>> +                        if (response.result && response.result.errors) {
>> +                            form.markInvalid(response.result.errors);
>> +                        }
>> +                        Ext.Msg.alert(gettext('Error'), response.htmlStatus);
>> +                    }
>> +                },
>> +                success: function(response, options) {
>> +                    me.close();
> we want to close after the second call.

True. Corrected.

>
>> +                    if ((me.backgroundDelay || me.showProgress) &&
>> +                        response.result.data) {
>> +                        var upid = response.result.data;
>> +                        var win = Ext.create('PVE.window.TaskProgress', {
>> +                            upid: upid
>> +                        });
>> +                        win.show();
>> +                    }
> I think we do not need above code here.

Removed.

>
>> +                    if (values.enable == 2) {
>> +                        submit_twice.call(me, 2);
>> +                    }
>> +                }
>> +            });
>> +        };
>> +
>>   	var add_boolean_row = function(name, text, defaultValue, labelWidth) {
>>   	    rows[name] = {
>>   		header: text,
>> @@ -41,8 +113,12 @@ Ext.define('PVE.FirewallOptions', {
>>   			checked: defaultValue ? true : false,
>>   			name: name,
>>   			uncheckedValue: 0,
>> -			fieldLabel: text
>> -		    }
>> +                        fieldLabel: text,
>> +                    },
>> +                    onFailedHook: function() {
>> +                        confirm ("Connection lost: Disabling firewall (in 60
>> seconds).") ;
>> +                    },
>> +                    submit: submit_twice
>>   		}
>>   	    };
>>   	};
>> diff --git a/www/manager/window/Edit.js b/www/manager/window/Edit.js
>> index 3e69da9..5d52a65 100644
>> --- a/www/manager/window/Edit.js
>> +++ b/www/manager/window/Edit.js
>> @@ -24,6 +24,8 @@ Ext.define('PVE.window.Edit', {
>>   
>>       showProgress: false,
>>   
>> +    onFailerHook: undefined,
>> +
> I guess we do not need that here.

Removed.

>
>>       isValid: function() {
>>   	var me = this;
>>   
>> -- 
>> 2.1.4
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>