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

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

On 27.10.20 11:55, 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>
> ---
>  www/BackupGroupChangeOwner.js | 88 +++++++++++++++++++++++++++++++++++
>  www/DataStoreContent.js       | 26 ++++++++++-
>  www/Makefile                  |  1 +
>  3 files changed, 114 insertions(+), 1 deletion(-)
>  create mode 100644 www/BackupGroupChangeOwner.js
> 

the whitespace/indentation is quite off, as you noticed yourself, so just for
the record.

> diff --git a/www/BackupGroupChangeOwner.js b/www/BackupGroupChangeOwner.js
> new file mode 100644
> index 00000000..5db7d115
> --- /dev/null
> +++ b/www/BackupGroupChangeOwner.js
> @@ -0,0 +1,88 @@
> +Ext.define('PBS.ChangeOwnerInputPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: 'widget.pbsChangeOwnerInputPanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    onGetValues: function(values) {
> +        var me = this;

we try to avoid "var" nowadays for new additions, it's "always function visible"
scope is often unexpected, rather use "let" or "const".

> +
> +        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";
> +	    }
> +
> +	    this.reload();

a POST request on initialization, what is reload responsible for?

Data should not be submitted this way, the edit window is responsible for that.

> +	},
> +
> +        reload: function() {
> +            var view = this.getView();
> +
> +            let params = view.getValues();
> +
> +            Proxmox.Utils.API2Request({
> +                url: view.url,
> +                method: "POST",
> +                params: params,
> +            });
> +        },
> +    },
> +
> +    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",
> +
> +    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",

try to not mix single quote and double quote in the same concatenation.
You can use Template literals (Template strings)[0]:

   url: `/api2/extjs/admin/datastore/${me.datastore}/change-owner`,

[0]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

> +            title: "Change Owner",
> +            items: [{
> +                xtype: "pbsChangeOwnerInputPanel",
> +                url: '/api2/extjs/admin/datastore/' + me.datastore + "/change-owner",

same here

> +                backup_type: me.backup_type,
> +                backup_id: me.backup_id,
> +            }],
> +        });
> +
> +        me.callParent();
> +    },
> +});
> diff --git a/www/DataStoreContent.js b/www/DataStoreContent.js
> index 28dfd743..594b1540 100644
> --- a/www/DataStoreContent.js
> +++ b/www/DataStoreContent.js
> @@ -267,6 +267,24 @@ Ext.define('PBS.DataStoreContent', {
>  	    }
>  	},
>  
> +	onChangeOwner: function(view, rI, cI, item, e, rec) {
> +            view = this.getView();

why re-assign view?

> +
> +            if (!(rec && rec.data)) return;

try to also put a single line if in braces: if (cond) {
    return;
}

> +            let data = rec.data;
> +	    if (rec.parentNode.id !== 'root') return;
> +
> +	    if (!view.datastore) return;

why not combine those returns

if (!rec || !rec.data || rec.parentNode.id !== 'root' || !view.datastore) {
    return;
}



> +
> +            let win = Ext.create('PBS.BackupGroupChangeOwner', {
> +                datastore: view.datastore,
> +                backup_type: data.backup_type,
> +                backup_id: data.backup_id,
> +            });
> +            win.on('destroy', this.reload, this);
> +            win.show();

can be fine, albeit I prefer to use `autoShow: true,` when defining the window.

> +	},
> +
>  	onPrune: function(view, rI, cI, item, e, rec) {
>  	    view = this.getView();
>  
> @@ -505,7 +523,13 @@ Ext.define('PBS.DataStoreContent', {
>  		    tooltip: gettext('Verify'),
>  		    getClass: (v, m, rec) => rec.data.leaf ? 'pmx-hidden' : 'fa fa-search',
>  		    isDisabled: (v, r, c, i, rec) => !!rec.data.leaf,
> -		},
> +                },
> +                {
> +		    handler: 'onChangeOwner',
> +		    tooltip: gettext('Change Owner'),
> +		    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',
>  		    tooltip: gettext('Prune'),
> diff --git a/www/Makefile b/www/Makefile
> index e04af930..b84fdf5d 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -40,6 +40,7 @@ JSSRC=							\
>  	VersionInfo.js					\
>  	SystemConfiguration.js				\
>  	Subscription.js					\
> +	BackupGroupChangeOwner.js 					\
>  	DataStorePrune.js				\
>  	DataStoreStatistic.js				\
>  	DataStoreContent.js				\
>