public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH manager] fix #5810: ui: show confirmation/warning dialog for sdn apply
@ 2024-10-30 12:02 Timothy Nicholson
  2024-10-30 12:47 ` Lukas Wagner
  0 siblings, 1 reply; 2+ messages in thread
From: Timothy Nicholson @ 2024-10-30 12:02 UTC (permalink / raw)
  To: pve-devel

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [pve-devel] [PATCH manager] fix #5810: ui: show confirmation/warning dialog for sdn apply
  2024-10-30 12:02 [pve-devel] [PATCH manager] fix #5810: ui: show confirmation/warning dialog for sdn apply Timothy Nicholson
@ 2024-10-30 12:47 ` Lukas Wagner
  0 siblings, 0 replies; 2+ messages in thread
From: Lukas Wagner @ 2024-10-30 12:47 UTC (permalink / raw)
  To: Proxmox VE development discussion, Timothy Nicholson

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2024-10-30 12:47 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-30 12:02 [pve-devel] [PATCH manager] fix #5810: ui: show confirmation/warning dialog for sdn apply Timothy Nicholson
2024-10-30 12:47 ` Lukas Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal