Re: [pve-devel] [PATCH manager v2] fix #5810: ui: show confirmation/warning dialog for sdn apply

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 04.11.24 um 13:24 schrieb Timothy Nicholson:
> This patch introduces a confirmation dialog for applying the SDN 
> configuration. This is to warn the user that any pending network 
> changes on the node level will also be applied.
> 
> Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
> ---
> As stated in the Bugzilla entry [1], the main purpose of this
> warning is to warn the user that any node network configuration 
> changes will also be applied. A conditional warning that states 
> whether any nodes have pending network changes (or even which nodes
> have changes) would definitely be nice, however this information is
> not readily available in the frontend, meaning there would need to 
> be an API call for each node to find pending changes. I'll leave it 
> open to discussion whether this would be worth it.
> [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=5810

yeah, getting this conditionally would be much nicer, but this is also
much cheaper, so if this really is deemed a problem then I'm fine with
warning unconditionally for now.

> changes since v1 [2]:
> -  split into commit message and comment, thanks Lukas for the feedback.
> [2]: https://lore.proxmox.com/pve-devel/20241030120205.85845-1-t.nicholson@proxmox.com/
> 
>  www/manager6/sdn/StatusView.js | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/www/manager6/sdn/StatusView.js b/www/manager6/sdn/StatusView.js
> index 970aa919..46ebe69a 100644
> --- a/www/manager6/sdn/StatusView.js
> +++ b/www/manager6/sdn/StatusView.js
> @@ -41,14 +41,24 @@ Ext.define('PVE.sdn.StatusView', {
>  		{
>  		    text: gettext('Apply'),
>  		    handler: function() {
> -			Proxmox.Utils.API2Request({
> -			    url: '/cluster/sdn/',
> -			    method: 'PUT',
> -			    waitMsgTarget: me,
> -			    failure: function(response, opts) {
> -				Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> -			    },
> -			});
> +			Ext.Msg.show({
> +			    title: gettext('Confirm'),
> +			    icon: Ext.Msg.WARNING,

maybe use "QUESTION" for the icon to avoid making this seem less safe than
it will be for most users.

> +			    msg: gettext('Any pending node network changes will also be applied. Proceed?'),

No hard feelings, but could maybe be good to mention the action one
confirms here, e.g. something like:

"Applying pending SDN changes will also apply any pending local node network changes. Proceed?"

Or multiline, like:

"Really apply pending SDN changes?"

Note: This will also apply any pending local node network changes."

> +			    buttons: Ext.Msg.YESNO,
> +			    callback: function(btn) {
> +				if (btn === 'yes') {
> +				    Proxmox.Utils.API2Request({
> +					url: '/cluster/sdn/',
> +					method: 'PUT',
> +					waitMsgTarget: me,
> +					failure: function(response, opts) {
> +					    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> +					},

Above could be redouce if you touch this anyway, i.e. to:

failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),

> +				    });
> +				}
> +			    }
> +			})
>  		    },
>  		},
>  	    ],



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel