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

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

[Timothy Nicholson]

Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
---
This patch introduces a confirmation dialog for applying the SDN configuration. As stated in the bugzilla entry [1], the main point of this dialog is to warn the user that any pending network configuration changes on the node level will also be applied. If the user clicks yes, the configuration is applied as normal. A conditional warning that states which nodes have pending changes would 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

 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,
+			    msg: gettext('Any pending node network changes will also be applied. Proceed?'),
+			    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);
+					},
+				    });
+				}
+			    }
+			})
 		    },
 		},
 	    ],
-- 
2.39.5


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

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

[Lukas Wagner]

On  2024-10-30 13:02, Timothy Nicholson wrote:
> Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
> ---
> This patch introduces a confirmation dialog for applying the SDN configuration. As stated in the bugzilla entry [1], the main point of this dialog is to warn the user that any pending network configuration changes on the node level will also be applied. If the user clicks yes, the configuration is applied as normal. A conditional warning that states which nodes have pending changes would 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.

Most of this should be part of the commit message itself, not the 'patch comment' (everything below the ---).
The patch comment is discarded by `git am` when the patch is applied. It should contain information
targeted at reviewers, such as open questions or the patch revision history. In the commit message,
which becomes part of the commit log, you put any information about the change that is relevant in the 
long term, e.g. the 'why' and 'how', so that we can understand how a line of code came to be in the
future. See [1] for more details.

Hope this clears things up! :)

[1] https://pve.proxmox.com/wiki/Developer_Documentation#Preparing_Patches

> 
> [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=5810
> 
>  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,
> +			    msg: gettext('Any pending node network changes will also be applied. Proceed?'),
> +			    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);
> +					},
> +				    });
> +				}
> +			    }
> +			})
>  		    },
>  		},
>  	    ],

-- 
- Lukas


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