Re: [pbs-devel] [PATCH proxmox-backup v2] ui: tape/BackupOverview: unify and improve tape restore window

[Dominik Csapak <d.csapak@proxmox.com>]

On 5/12/21 21:34, Thomas Lamprecht wrote:
> On 12.05.21 13:49, Dominik Csapak wrote:
>> adds a snapshot list to the restore window where the user can
>> select distinct snapshots to restore
>>
>> this also replaces the restore / restore single button by
>> inline actions for the media-set, backup groups and snapshots
>>
>> when clicking the action for a group/snapshot, the snapshotselector
>> gets activated and preselected/filtered so that it includes those
>> snapshots (can be overridden from the user)
>>
>> this change means that we now load the media set everytime we open
>> the window, since we want to have the list of snapshots.
>> (we could build that from the tree again, or hold it there somewhere as
>> list, but i do not think we gain much with that, and it simplifies the
>> datastore selection code a bit)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes from v1:
>> * fixed snapshot selection checkbox (was in wrong context, looup needs
>>    to happen on the referenceHolder)
>> * removed unnecessary 'node' variable in 'restore' function
>> * fixed 'media-set' text in window (was the text of the node we clicked on)
>>
> 
> Recently it seem to me like we alternate between "place every new function and
> it's use in 42 separate patches" and "dump^W squash three-zillion changes into
> one" like a metronome, none of those is  ideal IMO.
> 
> IOW, this really needs to be two patches, one for the content overview changes
> and the restore widow.

ok will do

> 
> Also, from my last reply to the v1 thread:
> 
>> IMO slightly better than radio-group/checkbox as it removes a UI control
>> element completely, so less choice/confusion, but without scarifying flexibility,
>> so IMO a good idea.
> 
> Now you added the extra checkbox...  Why not just use the grid-header checkbox like
> PVE backup-job edit-window and tick it by default when restoring a whole set?
> 
> As said, reduction by one knob without losing any flexibility - seems like a win
> win to me, or do I overlook something?
> 
> With the list below the window's aspect ration feels wrong and the list a bit
> squished, I'd add a few 100s px to the window height to improve that.
> 

ok this was a misunderstanding then...

yeah using the 'check all' checkbox makes sense, but then i am unsure
how to handle filtering.

now only visible entries get restored, so if a user would (with the 
check all) filter, it would seem like still all are selected and
we generate wrong assumptions?
we *should* be able to uncheck the check all box though to reduce
confusion, and this seems to me the best way forward.

the alternative is that we restore *all* selected entries, filtered or 
not, but this can be similarly confusing since things get restored that
weren't visible

in pve's 'mass *' windows we have a behaviour like the first suggestion,
but i just noticed, depending on which column gets filtered, the
'check all' box sometimes stays checked and sometimes not...

as for the height, if there are multiple datastores on the media-set
(idk if you tested with this), there is another grid for the
datastore mapping, with that the window is already quite tall

maybe a multi-step 'wizard' would be better?

let the user first select the basic choices (drive,user,etc.)
then a window with a full-size grid of the snapshots, and then
depending on what he selected, either a target datastore
or datastore mapping input?

(or the snapshots first, and the rest of the info on the second panel?)



>>   www/tape/BackupOverview.js     |  91 ++++----------
>>   www/tape/window/TapeRestore.js | 211 ++++++++++++++++++++++++++-------
>>   2 files changed, 197 insertions(+), 105 deletions(-)
>>
>> diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
>> index c028d58d..97725600 100644
>> --- a/www/tape/BackupOverview.js
>> +++ b/www/tape/BackupOverview.js
>> @@ -16,58 +16,16 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>>   	    }).show();
>>   	},
>>   
>> -	restoreSingle: function(button, record) {
>> +	restore: function(view, rI, cI, item, e, rec) {
>>   	    let me = this;
>> -	    let view = me.getView();
>> -	    let selection = view.getSelection();
>> -	    if (!selection || selection.length < 1) {
>> -		return;
>> -	    }
>> -
>> -	    let node = selection[0];
>> -	    if (node.data.restoreid === undefined) {
>> -		return;
>> -	    }
>> -	    let restoreid = node.data.restoreid;
>> -	    let mediaset = node.data.text;
>> -	    let uuid = node.data['media-set-uuid'];
>> -	    let datastores = [node.data.store];
>> -
>> -	    Ext.create('PBS.TapeManagement.TapeRestoreWindow', {
>> -		mediaset,
>> -		uuid,
>> -		list: [
>> -		    restoreid,
>> -		],
>> -		datastores,
>> -		listeners: {
>> -		    destroy: function() {
>> -			me.reload();
>> -		    },
>> -		},
>> -	    }).show();
>> -	},
>>   
>> -	restore: function(button, record) {
>> -	    let me = this;
>> -	    let view = me.getView();
>> -	    let selection = view.getSelection();
>> -	    if (!selection || selection.length < 1) {
>> -		return;
>> -	    }
>> -
>> -	    let node = selection[0];
>> -	    let mediaset = node.data.text;
>> -	    let uuid = node.data['media-set-uuid'];
>> -	    let datastores = node.data.datastores;
>> -	    while (!datastores && node.get('depth') > 2) {
>> -		node = node.parentNode;
>> -		datastores = node.data.datastores;
>> -	    }
>> +	    let mediaset = rec.data.is_media_set ? rec.data.text : rec.data['media-set'];
>> +	    let uuid = rec.data['media-set-uuid'];
>> +	    let restoreid = rec.data.restoreid;
>>   	    Ext.create('PBS.TapeManagement.TapeRestoreWindow', {
>>   		mediaset,
>>   		uuid,
>> -		datastores,
>> +		prefilter: restoreid,
>>   		listeners: {
>>   		    destroy: function() {
>>   			me.reload();
>> @@ -99,6 +57,7 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>>   		if (data[pool][media_set] === undefined) {
>>   		    data[pool][media_set] = entry;
>>   		    data[pool][media_set].text = media_set;
>> +		    data[pool][media_set].restore =true;
>>   		    data[pool][media_set].tapes = 1;
>>   		    data[pool][media_set]['seq-nr'] = undefined;
>>   		    data[pool][media_set].is_media_set = true;
>> @@ -155,14 +114,15 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>>   	    let view = me.getView();
>>   
>>   	    Proxmox.Utils.setErrorMask(view, true);
>> -	    const media_set = node.data['media-set-uuid'];
>> +	    const media_set_uuid = node.data['media-set-uuid'];
>> +	    const media_set = node.data.text;
>>   
>>   	    try {
>>   		let list = await PBS.Async.api2({
>>   		    method: 'GET',
>>   		    url: `/api2/extjs/tape/media/content`,
>>   		    params: {
>> -			'media-set': media_set,
>> +			'media-set': media_set_uuid,
>>   		    },
>>   		});
>>   
>> @@ -179,9 +139,11 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>>   
>>   		for (let entry of list.result.data) {
>>   		    entry.text = entry.snapshot;
>> +		    entry.restore = true;
>>   		    entry.leaf = true;
>>   		    entry.children = [];
>>   		    entry.restoreid = `${entry.store}:${entry.snapshot}`;
>> +		    entry['media-set'] = media_set;
>>   		    let iconCls = PBS.Utils.get_type_icon_cls(entry.snapshot);
>>   		    if (iconCls !== '') {
>>   			entry.iconCls = `fa ${iconCls}`;
>> @@ -217,6 +179,9 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>>   			    text,
>>   			    'media-set-uuid': entry['media-set-uuid'],
>>   			    leaf: false,
>> +			    restore: true,
>> +			    restoreid: `${store}:${text}`,
>> +			    'media-set': media_set,
>>   			    iconCls: `fa ${iconCls}`,
>>   			    children: [],
>>   			});
>> @@ -287,22 +252,6 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>>   	    text: gettext('New Backup'),
>>   	    handler: 'backup',
>>   	},
>> -	{
>> -	    xtype: 'proxmoxButton',
>> -	    disabled: true,
>> -	    text: gettext('Restore Media Set'),
>> -	    handler: 'restore',
>> -	    parentXType: 'treepanel',
>> -	    enableFn: (rec) => !!rec.data['media-set-uuid'],
>> -	},
>> -	{
>> -	    xtype: 'proxmoxButton',
>> -	    disabled: true,
>> -	    text: gettext('Restore Snapshot'),
>> -	    handler: 'restoreSingle',
>> -	    parentXType: 'treepanel',
>> -	    enableFn: (rec) => !!rec.data.restoreid,
>> -	},
>>       ],
>>   
>>       columns: [
>> @@ -313,6 +262,18 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>>   	    sortable: false,
>>   	    flex: 3,
>>   	},
>> +	{
>> +	    header: gettext('Actions'),
>> +	    xtype: 'actioncolumn',
>> +	    items: [
>> +		{
>> +		    handler: 'restore',
>> +		    tooltip: gettext('Restore'),
>> +		    getClass: (v, m, rec) => rec.data.restore ? 'fa fa-fw fa-undo' : 'pmx-hidden',
>> +		    isDisabled: (v, r, c, i, rec) => !rec.data.restore,
>> +                },
>> +	    ],
>> +	},
>>   	{
>>   	    text: gettext('Tapes'),
>>   	    dataIndex: 'tapes',
>> diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
>> index 7e4f5cae..1844e5eb 100644
>> --- a/www/tape/window/TapeRestore.js
>> +++ b/www/tape/window/TapeRestore.js
>> @@ -10,14 +10,13 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
>>       showTaskViewer: true,
>>       isCreate: true,
>>   
>> +    allowSnapshots: false,
>> +
>>       cbindData: function(config) {
>>   	let me = this;
>> -	me.isSingle = false;
>> -	me.listText = "";
>> -	if (me.list !== undefined) {
>> -	    me.isSingle = true;
>> -	    me.listText = me.list.join('<br>');
>> -	    me.title = gettext('Restore Snapshot');
>> +	if (me.prefilter !== undefined) {
>> +	    me.allowSnapshots = true;
>> +	    me.title = gettext('Restore Snapshot(s)');
>>   	}
>>   	return {};
>>       },
>> @@ -45,8 +44,8 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
>>   		}
>>   		delete values.mapping;
>>   
>> -		if (me.up('window').list !== undefined) {
>> -		    values.snapshots = me.up('window').list;
>> +		if (Ext.isString(values.snapshots)) {
>> +		    values.snapshots = values.snapshots.split(',');
>>   		}
>>   
>>   		values.store = datastores.join(',');
>> @@ -71,15 +70,6 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
>>   			value: '{uuid}',
>>   		    },
>>   		},
>> -		{
>> -		    xtype: 'displayfield',
>> -		    fieldLabel: gettext('Snapshot(s)'),
>> -		    submitValue: false,
>> -		    cbind: {
>> -			hidden: '{!isSingle}',
>> -			value: '{listText}',
>> -		    },
>> -		},
>>   		{
>>   		    xtype: 'pbsDriveSelector',
>>   		    fieldLabel: gettext('Drive'),
>> @@ -138,6 +128,30 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
>>   		    defaultBindProperty: 'value',
>>   		    hidden: true,
>>   		},
>> +		{
>> +		    boxLabel: gettext('Snapshot Selection'),
>> +		    labelWidth: 200,
>> +		    xtype: 'proxmoxcheckbox',
>> +		    isFormField: false,
>> +		    cbind: {
>> +			value: '{allowSnapshots}',
>> +		    },
>> +		    listeners: {
>> +			change: function(cb, value) {
>> +			    let me = this;
>> +			    me.up('window').lookup('snapshotGrid').setDisabled(!value);
>> +			},
>> +		    },
>> +		},
>> +		{
>> +		    xtype: 'pbsTapeSnapshotGrid',
>> +		    reference: 'snapshotGrid',
>> +		    name: 'snapshots',
>> +		    cbind: {
>> +			disabled: '{!allowSnapshots}',
>> +			prefilter: '{prefilter}',
>> +		    },
>> +		},
>>   	    ],
>>   	},
>>       ],
>> @@ -171,29 +185,39 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
>>   	let me = this;
>>   
>>   	me.callParent();
>> -	if (me.datastores) {
>> -	    me.setDataStores(me.datastores);
>> -	} else {
>> -	    // use timeout so that the window is rendered already
>> -	    // for correct masking
>> -	    setTimeout(function() {
>> -		Proxmox.Utils.API2Request({
>> -		    waitMsgTarget: me,
>> -		    url: `/tape/media/content?media-set=${me.uuid}`,
>> -		    success: function(response, opt) {
>> -			let datastores = {};
>> -			for (const content of response.result.data) {
>> -			    datastores[content.store] = true;
>> -			}
>> -			me.setDataStores(Object.keys(datastores));
>> -		    },
>> -		    failure: function() {
>> -			// ignore failing api call, maybe catalog is missing
>> -			me.setDataStores();
>> -		    },
>> -		});
>> -	    }, 10);
>> -	}
>> +	// use timeout so that the window is rendered already
>> +	// for correct masking
>> +	setTimeout(function() {
>> +	    Proxmox.Utils.API2Request({
>> +		waitMsgTarget: me,
>> +		url: `/tape/media/content?media-set=${me.uuid}`,
>> +		success: function(response, opt) {
>> +		    let datastores = {};
>> +		    for (const content of response.result.data) {
>> +			datastores[content.store] = true;
>> +		    }
>> +		    me.setDataStores(Object.keys(datastores));
>> +		    let grid = me.lookup('snapshotGrid');
>> +		    let store = grid.getStore();
>> +		    store.setData(response.result.data);
>> +		    if (me.prefilter !== undefined) {
>> +			store.each((rec) => {
>> +			    let restoreid = `${rec.data.store}:${rec.data.snapshot}`;
>> +			    if (restoreid.indexOf(me.prefilter) !== -1) {
>> +				rec.set('include', true);
>> +			    }
>> +			});
>> +
>> +			grid.setFilter(me.prefilter);
>> +		    }
>> +		    store.sort('snapshot');
>> +		},
>> +		failure: function() {
>> +		    // ignore failing api call, maybe catalog is missing
>> +		    me.setDataStores();
>> +		},
>> +	    });
>> +	}, 10);
>>       },
>>   });
>>   
>> @@ -202,6 +226,10 @@ Ext.define('PBS.TapeManagement.DataStoreMappingGrid', {
>>       alias: 'widget.pbsDataStoreMappingField',
>>       mixins: ['Ext.form.field.Field'],
>>   
>> +    maxHeight: 150,
>> +    scrollable: true,
>> +    margin: '0 0 10 0',
>> +
>>       getValue: function() {
>>   	let me = this;
>>   	let datastores = [];
>> @@ -308,3 +336,106 @@ Ext.define('PBS.TapeManagement.DataStoreMappingGrid', {
>>   	},
>>       ],
>>   });
>> +
>> +Ext.define('PBS.TapeManagement.SnapshotGrid', {
>> +    extend: 'Ext.grid.Panel',
>> +    alias: 'widget.pbsTapeSnapshotGrid',
>> +    mixins: ['Ext.form.field.Field'],
>> +
>> +    getValue: function() {
>> +	let me = this;
>> +	let snapshots = [];
>> +
>> +	me.getStore().each((rec) => {
>> +	    if (rec.data.include) {
>> +		let store = rec.data.store;
>> +		let snap = rec.data.snapshot;
>> +		snapshots.push(`${store}:${snap}`);
>> +	    }
>> +	});
>> +
>> +	return snapshots;
>> +    },
>> +
>> +    setValue: function(value) {
>> +	let me = this;
>> +	// not implemented
>> +	return me;
>> +    },
>> +
>> +    getErrors: function(value) {
>> +	let me = this;
>> +	let firstSelected = me.getStore().findBy((rec) => !!rec.data.include);
>> +
>> +	if (firstSelected === -1) {
>> +	    me.addCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']);
>> +	    let errorMsg = gettext("Need at least one snapshot");
>> +	    me.getActionEl().dom.setAttribute('data-errorqtip', errorMsg);
>> +
>> +	    return [errorMsg];
>> +	}
>> +	me.removeCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']);
>> +	me.getActionEl().dom.setAttribute('data-errorqtip', "");
>> +	return [];
>> +    },
>> +
>> +    scrollable: true,
>> +    height: 200,
>> +
>> +    viewConfig: {
>> +	emptyText: gettext('No Snapshots'),
>> +	markDirty: false,
>> +    },
>> +
>> +    setFilter: function(value) {
>> +	let me = this;
>> +	me.down('textfield').setValue(value);
>> +    },
>> +
>> +    tbar: [
>> +	{
>> +	    fieldLabel: gettext('Filter'),
>> +	    xtype: 'textfield',
>> +	    isFormField: false,
>> +	    listeners: {
>> +		change: function(field, value) {
>> +		    let me = this;
>> +		    let grid = me.up('grid');
>> +		    let store = grid.getStore();
>> +		    store.clearFilter();
>> +		    store.filterBy((rec) => {
>> +			let restoreid = `${rec.data.store}:${rec.data.snapshot}`;
>> +			return restoreid.indexOf(value) !== -1;
>> +		    });
>> +		    grid.checkChange();
>> +		},
>> +	    },
>> +	},
>> +    ],
>> +
>> +    store: { data: [] },
>> +
>> +    columns: [
>> +	{
>> +	    xtype: 'checkcolumn',
>> +	    text: gettext('Include'),
>> +	    dataIndex: 'include',
>> +	    listeners: {
>> +		checkchange: function(cb, value) {
>> +		    let grid = this.up('grid');
>> +		    grid.checkChange();
>> +		},
>> +	    },
>> +	},
>> +	{
>> +	    text: gettext('Source Datastore'),
>> +	    dataIndex: 'store',
>> +	    flex: 1,
>> +	},
>> +	{
>> +	    text: gettext('Snapshot'),
>> +	    dataIndex: 'snapshot',
>> +	    flex: 4,
>> +	},
>> +    ],
>> +});
>>
>