[pve-devel] [PATCH-SERIES v5 manager] fix #2745: allow specifying remove parameter for manual backup

[pve-devel] [PATCH-SERIES v5 manager] fix #2745: allow specifying remove parameter for manual backup

[Fabian Ebner]

Changes from v4:
    * dropped already applied patches
    * added a few small follow-ups and preparation:
        * set loading mask early enough
        * switch to two-column layout
    * changes for the main patch:
        * fix labels and gettext usage
        * only show retention settings when prune checkbox is set

Fabian Ebner (6):
  ui: backup window: avoid issuing API call with null/empty parameter
  ui: backup window: also set initialDefaults variable in failure case
  ui: backup window: set loading mask early enough
  ui: backup window: switch to proxmox input panel
  ui: backup window: switch to two-column layout
  fix #2745: ui: backup window: allow specifying remove parameter for
    manual backup

 www/manager6/window/Backup.js | 124 ++++++++++++++++++++++++++++++----
 1 file changed, 112 insertions(+), 12 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v5 manager 1/6] ui: backup window: avoid issuing API call with null/empty parameter

[Fabian Ebner]

could be triggered when there are no backup storages at all configured or if
the 'Backup now' button is clicked before the storage selector from the guests
'Backup' tab could load its store.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 www/manager6/window/Backup.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index a6dc1798..72e8cb48 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -46,6 +46,10 @@ Ext.define('PVE.window.Backup', {
 	    allowBlank: false,
 	    listeners: {
 		change: function(f, v) {
+		    if (v === null || v === undefined || v === '') {
+			return;
+		    }
+
 		    let store = f.getStore();
 		    let rec = store.findRecord('storage', v, 0, false, true, true);
 
-- 
2.20.1

[pve-devel] [PATCH v5 manager 2/6] ui: backup window: also set initialDefaults variable in failure case

[Fabian Ebner]

so that mailto and mode are not overwritten if the first /vzdump/defaults call
failed, but a later one succeeds.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 www/manager6/window/Backup.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 72e8cb48..76a4987e 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -80,6 +80,7 @@ Ext.define('PVE.window.Backup', {
 			    initialDefaults = true;
 			},
 			failure: function(response, opts) {
+			    initialDefaults = true;
 			    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
 			},
 		    });
-- 
2.20.1

[pve-devel] [PATCH v5 manager 3/6] ui: backup window: set loading mask early enough

[Fabian Ebner]

but not too early. Because of an ExtJS bug/limitation, it can only happen after
the window is rendered, so use an afterrender listener. Without setting the
mask there, the window will be active already before the storage selectors
change listener triggers, which can only happen after the storage selectors
store is loaded.

Made noticable by the new "filling in defaults" behavior, but the issue was
already present earlier, where the compression selector for PBS storages would
be disabled late, after the window was already active.

Also move the setValue call into the afterrender listener, so ordering is easy
to verify/more stable.

Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 www/manager6/window/Backup.js | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 76a4987e..8d9824f3 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -46,6 +46,10 @@ Ext.define('PVE.window.Backup', {
 	    allowBlank: false,
 	    listeners: {
 		change: function(f, v) {
+		    if (!initialDefaults) {
+			me.setLoading(false);
+		    }
+
 		    if (v === null || v === undefined || v === '') {
 			return;
 		    }
@@ -87,7 +91,6 @@ Ext.define('PVE.window.Backup', {
 		},
 	    },
 	});
-	storagesel.setValue(me.storage);
 
 	me.formPanel = Ext.create('Ext.form.Panel', {
 	    bodyPadding: 10,
@@ -172,6 +175,13 @@ Ext.define('PVE.window.Backup', {
 	    border: false,
 	    items: [me.formPanel],
 	    buttons: [helpBtn, '->', submitBtn],
+	    listeners: {
+		afterrender: function() {
+		    /// cleared within the storage selector's change listener
+		    me.setLoading(gettext('Please wait...'));
+		    storagesel.setValue(me.storage);
+		},
+	    },
 	});
 
 	me.callParent();
-- 
2.20.1

[pve-devel] [PATCH v5 manager 4/6] ui: backup window: switch to proxmox input panel

[Fabian Ebner]

It's not an Ext.form.Panel, so there is no fieldDefaults.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 www/manager6/window/Backup.js | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 8d9824f3..2625a718 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -92,13 +92,9 @@ Ext.define('PVE.window.Backup', {
 	    },
 	});
 
-	me.formPanel = Ext.create('Ext.form.Panel', {
+	me.formPanel = Ext.create('Proxmox.panel.InputPanel', {
 	    bodyPadding: 10,
 	    border: false,
-	    fieldDefaults: {
-		labelWidth: 100,
-		anchor: '100%',
-	    },
 	    items: [
 		storagesel,
 		modeSelector,
@@ -107,13 +103,11 @@ Ext.define('PVE.window.Backup', {
 	    ],
 	});
 
-	var form = me.formPanel.getForm();
-
 	var submitBtn = Ext.create('Ext.Button', {
 	    text: gettext('Backup'),
 	    handler: function() {
 		var storage = storagesel.getValue();
-		var values = form.getValues();
+		let values = me.formPanel.getValues();
 		var params = {
 		    storage: storage,
 		    vmid: me.vmid,
@@ -169,7 +163,6 @@ Ext.define('PVE.window.Backup', {
 
 	Ext.apply(me, {
 	    title: title,
-	    width: 350,
 	    modal: true,
 	    layout: 'auto',
 	    border: false,
-- 
2.20.1

[pve-devel] [PATCH v5 manager 5/6] ui: backup window: switch to two-column layout

[Fabian Ebner]

in preparation to add more items in the future, e.g. removal settings, note
container.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 www/manager6/window/Backup.js | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 2625a718..a1e34674 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -95,9 +95,11 @@ Ext.define('PVE.window.Backup', {
 	me.formPanel = Ext.create('Proxmox.panel.InputPanel', {
 	    bodyPadding: 10,
 	    border: false,
-	    items: [
+	    column1: [
 		storagesel,
 		modeSelector,
+	    ],
+	    column2: [
 		compressionSelector,
 		mailtoField,
 	    ],
-- 
2.20.1

[pve-devel] [PATCH v5 manager 6/6] fix #2745: ui: backup window: allow specifying remove parameter for manual backup

[Fabian Ebner]

and also show the retention options that will be used for a given storage. A
user with Datastore.AllocateSpace and VM.Backup can already remove backups from
the GUI manually, so it shouldn't be a problem if they can set the remove flag
when starting a manual backup in the GUI.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v4:
    * Don't use gettext on a variable and use 'Keep Last' instead of
     'keep-last', etc. as labels.
    * Only show retention settings when the prune checkbox is set.
    * Do not hide zero/undefined retention options, now that there's more space
      with the two-column layout.
    * Use labelWidth 110 for retention options, so German (and maybe other)
      translations of labels do not wrap

 www/manager6/window/Backup.js | 92 ++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index a1e34674..7048c366 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -36,6 +36,38 @@ Ext.define('PVE.window.Backup', {
 	    emptyText: Proxmox.Utils.noneText,
 	});
 
+	const keepNames = [
+	    ['keep-last', gettext('Keep Last')],
+	    ['keep-hourly', gettext('Keep Hourly')],
+	    ['keep-daily', gettext('Keep Daily')],
+	    ['keep-weekly', gettext('Keep Weekly')],
+	    ['keep-monthly', gettext('Keep Monthly')],
+	    ['keep-yearly', gettext('Keep Yearly')],
+	];
+
+	let pruneSettings = keepNames.map(
+	    name => Ext.create('Ext.form.field.Display', {
+		name: name[0],
+		fieldLabel: name[1],
+		hidden: true,
+	    }),
+	);
+
+	let removeCheckbox = Ext.create('Proxmox.form.Checkbox', {
+	    name: 'remove',
+	    checked: false,
+	    hidden: true,
+	    uncheckedValue: 0,
+	    fieldLabel: gettext('Prune'),
+	    autoEl: {
+		tag: 'div',
+		'data-qtip': gettext('Prune older backups afterwards'),
+	    },
+	    handler: function(checkbox, value) {
+		pruneSettings.forEach(field => field.setHidden(!value));
+	    },
+	});
+
 	let initialDefaults = false;
 
 	var storagesel = Ext.create('PVE.form.StorageSelector', {
@@ -82,6 +114,30 @@ Ext.define('PVE.window.Backup', {
 			    }
 
 			    initialDefaults = true;
+
+			    // always update storage dependent properties
+			    if (data['prune-backups'] !== undefined) {
+				const keepParams = PVE.Parser.parsePropertyString(
+				    data["prune-backups"],
+				);
+				if (!keepParams['keep-all']) {
+				    removeCheckbox.setHidden(false);
+				    pruneSettings.forEach(function(field) {
+					const keep = keepParams[field.name];
+					if (keep) {
+					    field.setValue(keep);
+					} else {
+					    field.reset();
+					}
+				    });
+				    return;
+				}
+			    }
+
+			    // no defaults or keep-all=1
+			    removeCheckbox.setHidden(true);
+			    removeCheckbox.setValue(false);
+			    pruneSettings.forEach(field => field.reset());
 			},
 			failure: function(response, opts) {
 			    initialDefaults = true;
@@ -98,11 +154,45 @@ Ext.define('PVE.window.Backup', {
 	    column1: [
 		storagesel,
 		modeSelector,
+		removeCheckbox,
 	    ],
 	    column2: [
 		compressionSelector,
 		mailtoField,
 	    ],
+	    columnB: [{
+		layout: 'hbox',
+		border: false,
+		defaults: {
+		    border: false,
+		    layout: 'anchor',
+		    flex: 1,
+		},
+		items: [
+		    {
+			padding: '0 10 0 0',
+			defaults: {
+			    labelWidth: 110,
+			},
+			items: [
+			    pruneSettings[0],
+			    pruneSettings[2],
+			    pruneSettings[4],
+			],
+		    },
+		    {
+			padding: '0 0 0 10',
+			defaults: {
+			    labelWidth: 110,
+			},
+			items: [
+			    pruneSettings[1],
+			    pruneSettings[3],
+			    pruneSettings[5],
+			],
+		    },
+		],
+	    }],
 	});
 
 	var submitBtn = Ext.create('Ext.Button', {
@@ -114,7 +204,7 @@ Ext.define('PVE.window.Backup', {
 		    storage: storage,
 		    vmid: me.vmid,
 		    mode: values.mode,
-		    remove: 0,
+		    remove: values.remove,
 		};
 
 		if (values.mailto) {
-- 
2.20.1

[pve-devel] [PATCH v5 manager 7/7] ui: backup window: also reset removal fields when defaults API call fails

[Fabian Ebner]

to avoid showing settings for the wrong storage.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Sorry for the delayed fix-up/follow-up to the series.

Could also be squashed into patch #6.

 www/manager6/window/Backup.js | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 7048c366..77be6caf 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -141,6 +141,11 @@ Ext.define('PVE.window.Backup', {
 			},
 			failure: function(response, opts) {
 			    initialDefaults = true;
+
+			    removeCheckbox.setHidden(true);
+			    removeCheckbox.setValue(false);
+			    pruneSettings.forEach(field => field.reset());
+
 			    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
 			},
 		    });
-- 
2.20.1

[pve-devel] applied-series: [PATCH-SERIES v5 manager] fix #2745: allow specifying remove parameter for manual backup

[Thomas Lamprecht]

On 06.05.21 14:16, Fabian Ebner wrote:
> Changes from v4:
>     * dropped already applied patches
>     * added a few small follow-ups and preparation:
>         * set loading mask early enough
>         * switch to two-column layout
>     * changes for the main patch:
>         * fix labels and gettext usage
>         * only show retention settings when prune checkbox is set
> 
> Fabian Ebner (6):
>   ui: backup window: avoid issuing API call with null/empty parameter
>   ui: backup window: also set initialDefaults variable in failure case
>   ui: backup window: set loading mask early enough
>   ui: backup window: switch to proxmox input panel
>   ui: backup window: switch to two-column layout
>   fix #2745: ui: backup window: allow specifying remove parameter for
>     manual backup
> 
>  www/manager6/window/Backup.js | 124 ++++++++++++++++++++++++++++++----
>  1 file changed, 112 insertions(+), 12 deletions(-)
> 



applied series, thanks!

Added a label for the retention settings to ensure the user knows that they come from
the storage. Also added a log line about what actual retention keep-settings are used,
as else I could not be determined in the task-log if they came from a storage or node
vzdump.conf.