[pve-devel] [PATCH manager 2/2] fix #1594: add "Run now" button to cluster backup page

[Tim Marx]

In general it looks good and does work, some stuff inline.

> Stefan Reiter <s.reiter at proxmox.com> hat am 31. Juli 2019 13:23 geschrieben:
> 
>  
> Includes a "confirm" dialog to not accidentally run a potentially large backup
> job.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>  www/manager6/dc/Backup.js | 67 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 6810d92f..7b2c52a6 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -410,13 +410,40 @@ Ext.define('PVE.dc.BackupView', {
>  		return;
>  	    }
>  
> -            var win = Ext.create('PVE.dc.BackupEdit',{
> -                jobid: rec.data.id
> -            });
> -            win.on('destroy', reload);
> -            win.show();
> +	    var win = Ext.create('PVE.dc.BackupEdit', {
> +		jobid: rec.data.id
> +	    });
> +	    win.on('destroy', reload);
> +	    win.show();
>  	};
>  

Maybe send clean up/formatting as extra patch, just to keep patches focused.

> +	var run_backup_now = function(jobid) {
> +	    var allNodes = PVE.data.ResourceStore.getNodes();

There is much more info available which you could use to make this a little bit less error prone e.g. why requesting anything to a node which is offline?

> +	    var standalone = allNodes.length === 1;
> +	    allNodes.forEach(node => {
> +		Proxmox.Utils.API2Request({
> +		    url: '/cluster/backup/' + jobid + '/' + node.node + '/run',
> +		    method: 'POST',
> +		    waitMsgTarget: me,

This won't work, because it will be set/reset for each request. So it happens, that there a still pending calls but the UI suggest all is done because there is no loading indicator displayed. This is especially confusing if you test it with a backup job where there is a node involved which is offline/not responding and then suddenly an error message pops up even though you navigated to a different tab.

> +		    failure: function (response, opts) {
> +			Ext.Msg.alert('Error', node.node + ': ' + response.htmlStatus);
> +		    },
> +		    success: function (response, opts) {
> +			var data = response.result.data;
> +			if (!standalone) {
> +			    // Task has been started on multiple nodes
> +			    // User has to figure out which to monitor themselves

This is actually not true, because standalone does only state that there are at least 2 nodes in the cluster, but this does not state anything about how many tasks will be spawned. If you take a look at the bulk start/stop ui, there you'll have an example how this is handled if more than one task is involved. If you handle the masking of the grid correctly I would be ok with it not showing a TaskViewer at all, to not make this a bigger expense than it needs to be.

> +			    return;
> +			}
> +			var win = Ext.create('Proxmox.window.TaskViewer', {
> +				upid: data
> +			});
> +			win.show();
> +		    }
> +		});
> +	    });
> +	}
> +
>  	var edit_btn = new Proxmox.button.Button({
>  	    text: gettext('Edit'),
>  	    disabled: true,
> @@ -424,6 +451,33 @@ Ext.define('PVE.dc.BackupView', {
>  	    handler: run_editor
>  	});
>  
> +	var caps = Ext.state.Manager.get('GuiCap');
> +	var run_btn = new Proxmox.button.Button({
> +	    text: gettext('Run now'),
> +	    disabled: true,
> +	    hidden: !caps.nodes['Sys.Modify'],
> +	    selModel: sm,
> +	    handler: function() {
> +		var rec = sm.getSelection()[0];
> +		if (!rec) {
> +		    return;
> +		}
> +
> +		Ext.Msg.show({
> +		    title: gettext('Confirm'),
> +		    icon: Ext.Msg.QUESTION,
> +		    msg: gettext('Start the selected backup job now?'),
> +		    buttons: Ext.Msg.YESNO,
> +		    callback: function(btn) {
> +			if (btn !== 'yes') {
> +			    return;
> +			}
> +			run_backup_now(rec.data.id);
> +		    }
> +		});
> +	    }
> +	});
> +
>  	var remove_btn = Ext.create('Proxmox.button.StdRemoveButton', {
>  	    selModel: sm,
>  	    baseurl: '/cluster/backup',
> @@ -452,7 +506,8 @@ Ext.define('PVE.dc.BackupView', {
>  		    }
>  		},
>  		remove_btn,
> -		edit_btn
> +		edit_btn,
> +		run_btn
>  	    ],
>  	    columns: [
>  		{
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel