Re: [pbs-devel] [PATCH v2 proxmox-backup] gui: Add button for changing backup group owner

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 29.10.20 10:16, Dylan Whyte wrote:
> Extension of fix #2847
> 
> Adds an action button to the datastore content view,
> to change the owner of a backup.
> 
> Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
> ---
> 
> v2:
> - Fix whitespace
> - BackupGroupChangeOwner: remove unnecessary reload function
> - Use let insread of var in "onGetValues"
> - Use template literals for URL
> - Set autoShow true when creating the window
> 
> In response to comment about reassigning view in onChangeOwner, the
> view that is passed to the handler function (Ext.view.Table) is
> necessary in the handler definiation/call, but is not used
> within the implementaion of onChangeOwner. Thus it is reassigned to the
> change owner window view. This follows the format of the other action
> handlers in the file.

getting quite there, seems to work out OK, a few comments left, mostly regarding
code structure and missing gettext's - sorry to overlook those when commenting
v1.

> 
>  www/BackupGroupChangeOwner.js | 74 +++++++++++++++++++++++++++++++++++
>  www/DataStoreContent.js       | 27 ++++++++++++-
>  www/Makefile                  |  1 +
>  3 files changed, 101 insertions(+), 1 deletion(-)
>  create mode 100644 www/BackupGroupChangeOwner.js
> 
> diff --git a/www/BackupGroupChangeOwner.js b/www/BackupGroupChangeOwner.js
> new file mode 100644
> index 00000000..0d24bfe0
> --- /dev/null
> +++ b/www/BackupGroupChangeOwner.js
> @@ -0,0 +1,74 @@
> +Ext.define('PBS.ChangeOwnerInputPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: 'widget.pbsChangeOwnerInputPanel',
> +    mixins: ['Proxmox.Mixin.CBind'],

not sure if it's worth to define this as separate component with only one single
field as element?

I'd rather declare it inline in the edit window definition below, the params
checking from init here is done there anyway,

You could check out 'PBS.window.VerifyJobEdit' as example for this.

> +
> +    onGetValues: function(values) {
> +	let me = this;
> +
> +	values["backup-type"] = me.backup_type;
> +	values["backup-id"] = me.backup_id;
> +	return values;
> +    },
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	init: function(view) {
> +	    if (!view.url) {
> +		throw "no url specified";
> +	    }
> +	    if (!view.backup_type) {
> +		throw "no backup_type specified";
> +	    }
> +	    if (!view.backup_id) {
> +		throw "no backup_id specified";
> +	    }
> +	},
> +    },
> +
> +    column1: [
> +	{
> +	    xtype: 'textfield',
> +	    name: 'new-owner',
> +	    fieldLabel: gettext('Userid'),
> +	    minLength: 8,
> +	    allowBlank: false,
> +	},
> +    ],
> +
> +});
> +
> +Ext.define('PBS.BackupGroupChangeOwner', {
> +    extend: 'Proxmox.window.Edit',
> +
> +    method: 'POST',
> +    submitText: "Change Owner",

use gettext

> +
> +    initComponent: function() {
> +	var me = this;
> +
> +	if (!me.datastore) {
> +	    throw "no datastore specified";
> +	}
> +	if (!me.backup_type) {
> +	    throw "no backup_type specified";
> +	}
> +	if (!me.backup_id) {
> +	    throw "no backup_id specified";
> +	}
> +
> +	Ext.apply(me, {
> +	    url: `/api2/extjs/admin/datastore/${me.datastore}/change-owner`,
> +	    title: "Change Owner",

title can be above, in declarative config part, also, please use gettext

> +	    items: [{
> +		xtype: "pbsChangeOwnerInputPanel",
> +		url: `/api2/extjs/admin/datastore/${me.datastore}/change-owner`,
> +		backup_type: me.backup_type,
> +		backup_id: me.backup_id,
> +	    }],
> +	});
> +
> +	me.callParent();
> +    },
> +});
> diff --git a/www/DataStoreContent.js b/www/DataStoreContent.js
> index 9da18845..35aa2b11 100644
> --- a/www/DataStoreContent.js
> +++ b/www/DataStoreContent.js
> @@ -268,6 +268,25 @@ Ext.define('PBS.DataStoreContent', {
>  	    }
>  	},
>  
> +	onChangeOwner: function(view, rI, cI, item, e, rec) {
> +	    view = this.getView();
> +
> +	    if (!rec || !rec.data || rec.parentNode.id !== 'root' || !view.datastore) {
> +		return;
> +	    }
> +
> +	    let data = rec.data;
> +
> +	    let win = Ext.create('PBS.BackupGroupChangeOwner', {
> +		datastore: view.datastore,
> +		backup_type: data.backup_type,
> +		backup_id: data.backup_id,
> +		autoShow: true,
> +	    });
> +	    win.on('destroy', this.reload, this);
> +	    win.show();

once you set autoShow above win.show call is not required anymore ;-)

> +	},
> +
>  	onPrune: function(view, rI, cI, item, e, rec) {
>  	    view = this.getView();
>  
> @@ -582,7 +601,13 @@ Ext.define('PBS.DataStoreContent', {
>  		    getTip: (v, m, rec) => Ext.String.format(gettext("Verify '{0}'"), v),
>  		    getClass: (v, m, rec) => rec.data.leaf ? 'pmx-hidden' : 'pve-icon-verify-lettering',
>  		    isDisabled: (v, r, c, i, rec) => !!rec.data.leaf,
> -		},
> +                },
> +                {
> +		    handler: 'onChangeOwner',
> +		    tooltip: gettext('Change Owner'),

I recently adapted those to a more dynamic one, showing the snapshot/group ID
inside the tooltip by using the getTip config property.

Maybe it makes sense to adapt that here too?

getTip: (v, m, rec) => Ext.String.format(gettext("Change owner of '{0}'"), v),


> +		    getClass: (v, m, rec) => rec.parentNode.id ==='root' ? 'fa fa-user' : 'pmx-hidden',
> +		    isDisabled: (v, r, c, i, rec) => rec.parentNode.id !=='root',
> +                },
>  		{
>  		    handler: 'onPrune',
>  		    getTip: (v, m, rec) => Ext.String.format(gettext("Prune '{0}'"), v),
> diff --git a/www/Makefile b/www/Makefile
> index cba8bed5..aabc8d9c 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -46,6 +46,7 @@ JSSRC=							\
>  	DataStorePrune.js				\
>  	DataStoreContent.js				\
>  	DataStorePanel.js				\
> +	BackupGroupChangeOwner.js			\
>  	ServerStatus.js					\
>  	ServerAdministration.js				\
>  	Dashboard.js					\
>