[pve-devel] [PATCH manager] ui: fw: Close #2815: Add warning if fw is disabled

[Dominik Csapak]

Looks mosty ok, but i noticed two things:

1. We use this grid also on datacenter and node level, so we should 
adapt the message for those somehow

2. is inline

On 6/23/20 11:31 AM, Dominic Jäger wrote:
> Currently people add firewall rules but forget to activate the firewall on
> guest level. This commit adds a warning to the top bar of the firewall panel to
> make them aware of this if necessary.
> 
> It seems a little cheaper but still sufficient to check only if some rule
> exists and not for every rule if it is really enabled.
> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
>   www/manager6/grid/FirewallRules.js | 32 ++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/www/manager6/grid/FirewallRules.js b/www/manager6/grid/FirewallRules.js
> index ec2d1c84..7cb2baf0 100644
> --- a/www/manager6/grid/FirewallRules.js
> +++ b/www/manager6/grid/FirewallRules.js
> @@ -483,8 +483,26 @@ Ext.define('PVE.FirewallRules', {
>   	    throw "no list_refs_url specified";
>   	}
>   
> +	let checkWarning = function () {
> +	    Proxmox.Utils.API2Request({
> +		url: me.base_url.replace('rules', 'options'),
> +		method: 'GET',
> +		failure: function (response) {
> +		    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> +		},
> +		success: function (response) {
> +		    let warningRequired = me.store.getCount() != 0 && !response.result.data.enable;
> +		    me.down('displayfield[name=fw-warning]').setVisible(warningRequired)
> +		},
> +	    });
> +
> +	};
> +
>   	var store = Ext.create('Ext.data.Store',{
> -	    model: 'pve-fw-rule'
> +	    model: 'pve-fw-rule',
> +	    listeners: {
> +		    'load': checkWarning,
> +	    },
>   	});
>   
>   	var reload = function() {
> @@ -606,12 +624,22 @@ Ext.define('PVE.FirewallRules', {
>   	    }
>   	});
>   
> +	me.warningField = Ext.create('Ext.form.field.Display',{
> +		xtype: 'displayfield',
> +		dock: 'top',
> +		userCls: 'pmx-hint',
> +		name: 'fw-warning',
> +		margin: '10 0 0 0',

i guess you wanted the margin on the left side not top?
the order is: top right bottom left, so you'd have to do
margin: '0 0 0 10'

with this currently the tbar changes height when the warning is 
displayed, which looks odd...

(i don't think we need any margin/padding in tbar)

also the 'dock' property is not necessary (the tbar is already docked)

> +		value: gettext('Warning: Firewall still disabled at guest level! This can be changed in Firewall->Options.'),
> +		hidden: true,
> +	});
> +
>   	var tbar = me.tbar_prefix ? [ me.tbar_prefix ] : [];
>   	tbar.push(me.addBtn, me.copyBtn);
>   	if (me.groupBtn) {
>   	    tbar.push(me.groupBtn);
>   	}
> -	tbar.push(me.removeBtn, me.editBtn);
> +	tbar.push(me.removeBtn, me.editBtn, me.warningField);
>   
>   	var render_errors = function(name, value, metaData, record) {
>   	    var errors = record.data.errors;
>