In general it looks good and does work, some stuff inline.
> Stefan Reiter <s.rei...@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.rei...@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
