Re: [pve-devel] [PATCH v5 manager 7/7] fix #1710: ui: storage: add download from url button

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

comments inline,
some things would have been caught with 'eslint',
so i would recommend running it on the files you edit/add,
sans the errors from existing code

(most errors can even be auto-fixed with '--fix')


On 5/12/21 10:22, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   www/manager6/storage/Browser.js     |   8 +
>   www/manager6/storage/ContentView.js | 282 +++++++++++++++++++++++++---
>   2 files changed, 265 insertions(+), 25 deletions(-)
> 
> diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
> index 5fee94c7..c0d647d3 100644
> --- a/www/manager6/storage/Browser.js
> +++ b/www/manager6/storage/Browser.js
> @@ -53,6 +53,9 @@ Ext.define('PVE.storage.Browser', {
>   	    let plugin = res.plugintype;
>   	    let contents = res.content.split(',');
>   
> +	    let enableUpload = !!caps.storage['Datastore.AllocateTemplate'];
> +	    let enableDownloadUrl = !!(caps.nodes['Sys.Audit'] && caps.nodes['Sys.Modify']);

you use this always with 'enableUpload' anyway so you could add
it here already

let enableDownloadUrl = enableUpload && ...;

> +
>   	    if (contents.includes('backup')) {
>   		me.items.push({
>   		    xtype: 'pveStorageBackupView',
> @@ -91,6 +94,8 @@ Ext.define('PVE.storage.Browser', {
>   		    itemId: 'contentIso',
>   		    content: 'iso',
>   		    pluginType: plugin,
> +		    enableUploadButton: enableUpload,
> +		    enableDownloadUrlButton: enableUpload && enableDownloadUrl,
>   		    useUploadButton: true,
>   		});
>   	    }
> @@ -101,6 +106,9 @@ Ext.define('PVE.storage.Browser', {
>   		    iconCls: 'fa fa-file-o lxc',
>   		    itemId: 'contentVztmpl',
>   		    pluginType: plugin,
> +		    enableUploadButton: enableUpload,
> +		    enableDownloadUrlButton: enableUpload && enableDownloadUrl,
> +		    useUploadButton: true,
>   		});
>   	    }
>   	    if (contents.includes('snippets')) {
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index dd6df4b1..9125c0bb 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -191,6 +191,214 @@ Ext.define('PVE.storage.Upload', {
>       },
>   });
>   
> +Ext.define('PVE.storage.DownloadUrl', {
> +    extend: 'Proxmox.window.Edit',
> +    alias: 'widget.pveStorageDownloadUrl',
> +
> +    isCreate: true,
> +
> +    showTaskViewer: true,
> +
> +    title: gettext('Download from URL'),
> +    submitText: gettext('Download'),
> +
> +    id: 'download-url',
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	urlChange: function(field) {
> +	    let me = Ext.getCmp('download-url');
> +	    field = me.down('[name=url]');

there is a better way to get the 'view' and fields when you have
a controller

instead of giving the view an id and do a getCmp, you can
simply do a

let me = this; // this is the controller here
let view = me.getView(); // this is the view (== window)

simply use view then instead of me

also if you add a 'reference' on the field,
you can simply do a

let field = me.lookup('fieldreference'); // works on both controller and 
view

> +	    field.setValidation("Waiting for response...");
> +	    field.validate();
> +	    me.setValues({size: ""});
> +	    Proxmox.Utils.API2Request({
> +		url: `/nodes/${me.nodename}/query-url-metadata`,
> +		method: 'GET',
> +		params: {
> +		    url: field.getValue(),
> +		    'verify-certificates': me.getValues()['verify-certificates'],
> +		},
> +		failure: function(res, opt) {
> +		    field.setValidation(res.result.message);
> +		    field.validate();
> +		    me.setValues({
> +			size: "",
> +			mimetype: "",
> +		    });
> +		},
> +		success: function(res, opt) {
> +		    field.setValidation();
> +		    field.validate();
> +
> +		    let data = res.result.data;
> +		    me.setValues({
> +			filename: data.filename || "",
> +			size: data.size && Proxmox.Utils.format_size(data.size) || "",

this throws an eslint warning:

no-mixed-operators - Unexpected mix of '&&' and '||'.

better use braces

> +			mimetype: data.mimetype || "",
> +		    }); > +		},
> +	    });
> +	},
> +
> +	hashChange: function(field) {
> +	    let cecksum = Ext.getCmp('downloadUrlChecksum');

nitpick: typo s/cecksum/checksum/

> +	    if (field.getValue() === '__default__') {
> +		cecksum.setDisabled(true);
> +		cecksum.setValue("");
> +		cecksum.allowBlank = true;
> +	    } else {
> +		cecksum.setDisabled(false);
> +		cecksum.allowBlank = false;
> +	    }
> +	},
> +
> +	typeChange: function(field) {
> +	    let me = Ext.getCmp('download-url');

same here:

let me = this;
let view = me.getView();

> +	    let content = me.getValues()['content'];

this throws eslint error:

dot-notation - ["content"] is better written in dot notation.

> +	    let type = field.getValue();
> +
> +	    const types = {
> +		iso: [
> +		    'application/octet-stream',
> +		    'application/x-iso9660-image',
> +		    'application/x-ima',
> +		],
> +		vztmpl: [
> +		    'application/octet-stream',
> +		    'application/gzip',
> +		    'application/tar',
> +		    'application/tar+gzip',
> +		    'application/x-gzip',
> +		    'application/x-gtar',
> +		    'application/x-tgz',
> +		    'application/x-tar',
> +		],
> +	    };
> +
> +	    if (type === "" || (types[content] && types[content].includes(type))) {
> +		field.setValidation();
> +		field.setDisabled(true);
> +	    } else {
> +		field.setDisabled(false);
> +		field.setValidation("Invalid type");
> +	    }
> +	    field.validate();
> +	},
> +    },
> +
> +    items: [
> +	{
> +	    xtype: 'inputpanel',
> +	    waitMsgTarget: true,
> +	    border: false,
> +	    columnT: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'url',
> +		    allowBlank: false,
> +		    fieldLabel: gettext('URL'),
> +		    listeners: {
> +			change: {
> +			    buffer: 500,
> +			    fn: 'urlChange',
> +			},
> +		    },
> +		},
> +		{
> +		    xtype: 'textfield',
> +		    name: 'filename',
> +		    allowBlank: false,
> +		    fieldLabel: gettext('File name'),
> +		},
> +	    ],
> +	    column1: [
> +		{
> +		    xtype: 'pveContentTypeSelector',
> +		    fieldLabel: gettext('Content'),
> +		    name: 'content',
> +		    allowBlank: false,
> +		},
> +	    ],
> +	    column2: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'size',
> +		    disabled: true,
> +		    fieldLabel: gettext('File size'),
> +		    emptyText: gettext('unknown'),
> +		},
> +	    ],
> +	    advancedColumn1: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'checksum',
> +		    fieldLabel: gettext('Checksum'),
> +		    allowBlank: true,
> +		    disabled: true,
> +		    emptyText: gettext('none'),
> +		    id: 'downloadUrlChecksum',
> +		},
> +		{
> +		    xtype: 'pveHashAlgorithmSelector',
> +		    name: 'checksum-algorithm',
> +		    fieldLabel: gettext('Hash algorithm'),
> +		    allowBlank: true,
> +		    hasNoneOption: true,
> +		    value: '__default__',
> +		    listeners: {
> +			change: 'hashChange',
> +		    },
> +		},
> +	    ],
> +	    advancedColumn2: [
> +		{
> +		    xtype: 'textfield',
> +		    fieldLabel: gettext('MIME type'),
> +		    name: 'mimetype',
> +		    disabled: true,
> +		    editable: false,
> +		    emptyText: gettext('unknown'),
> +		    listeners: {
> +			change: 'typeChange',
> +		    },
> +		},
> +		{
> +		    xtype: 'proxmoxcheckbox',
> +		    name: 'verify-certificates',
> +		    fieldLabel: gettext('Verify certificates'),
> +		    uncheckedValue: 0,
> +		    checked: true,
> +		    listeners: {
> +			change: 'urlChange',
> +		    },
> +		},
> +	    ],
> +	},
> +    ],
> +
> +    initComponent: function() {
> +        var me = this;
> +
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +	if (!me.storage) {
> +	    throw "no storage ID specified";
> +	}
> +
> +	me.url = `/nodes/${me.nodename}/storage/${me.storage}/download-url`;
> +	me.method = 'POST';

this could go statically into the class on top:

method: 'POST',



> +
> +	let contentTypeSel = me.items[0].column1[0];
> +	contentTypeSel.cts = me.contents;
> +	contentTypeSel.value = me.contents[0] || '';

this and the url could go into our 'cbind' mixin
(check for example panel/GuestStatusView.js)

> +
> +        me.callParent();
> +    },
> +});
> +
>   Ext.define('PVE.storage.ContentView', {
>       extend: 'Ext.grid.GridPanel',
>   
> @@ -249,36 +457,60 @@ Ext.define('PVE.storage.ContentView', {
>   
>   	Proxmox.Utils.monStoreErrors(me, store);
>   
> -	let uploadButton = Ext.create('Proxmox.button.Button', {
> -	    text: gettext('Upload'),
> -	    handler: function() {
> -		let win = Ext.create('PVE.storage.Upload', {
> -		    nodename: nodename,
> -		    storage: storage,
> -		    contents: [content],
> -		});
> -		win.show();
> -		win.on('destroy', reload);
> -	    },
> -	});
> -
> -	let removeButton = Ext.create('Proxmox.button.StdRemoveButton', {
> -	    selModel: sm,
> -	    delay: 5,
> -	    callback: function() {
> -		reload();
> -	    },
> -	    baseurl: baseurl + '/',
> -	});
> -
>   	if (!me.tbar) {
>   	    me.tbar = [];
>   	}
>   	if (me.useUploadButton) {
> -	    me.tbar.push(uploadButton);
> +	    me.tbar.push(
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Upload'),
> +		    disabled: !me.enableUploadButton,
> +		    handler: function() {
> +			Ext.create('PVE.storage.Upload', {
> +			    nodename: nodename,
> +			    storage: storage,
> +			    contents: [content],
> +			    autoShow: true,
> +			    listeners:{
> +				destroy: () => reload(),
> +			    }

the reload makes more sense on the 'taskDone' callback
since that really only triggers when the download is finished,
instead of when the window is closed (which may not have
changed the content at all)

> +			});
> +		    },
> +		},
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Download from URL'),
> +		    disabled: !me.enableDownloadUrlButton,
> +		    handler: function() {
> +			Ext.create('PVE.storage.DownloadUrl', {
> +			    nodename: nodename,
> +			    storage: storage,
> +			    contents: [content],
> +			    autoShow: true,
> +			    listeners: {
> +				destroy: () => reload(),
> +			    },

same here

> +			});
> +		    },
> +		},
> +		'-',
> +	    );
>   	}
> -	if (!me.useCustomRemoveButton) {
> -	    me.tbar.push(removeButton);
> +	if (me.useCustomRemoveButton) {
> +	    // custom remove button was inserted as first element
> +	    // -> place it at the end of tbar
> +	    me.tbar.push(me.tbar.shift());

could we not simply 'unshift' our upload/download button instead?
that way, this code would not have to change and we would not
move other stuff around...

> +	} else {
> +	    me.tbar.push({
> +		xtype: 'proxmoxStdRemoveButton',
> +		selModel: sm,
> +		delay: 5,
> +		callback: function() {
> +		    reload();
> +		},
> +		baseurl: baseurl + '/',
> +	    });
>   	}
>   	me.tbar.push(
>   	    '->',
>