Re: [pve-devel] [PATCH manager 1/1] fix #1710: add retrieve from url button for storage

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

one high level comment here:

we try to avoid a long 'initComponent' in favor
of declaring classes schematically with extjs
mvvm (model, view, viewmodel), similar to mvc
(model view controller)

for the edit window this would mean it looking like this:

Ext.define(....
    ...

    controller: {
	xclass: 'Ext.app.ViewController',

	fooChange: ...
    },

    items: [
	{
	    xtype: 'foo',
	    property1: 'bar',
	    listener: {
		change: 'fooChange',
	    },
	},
	...
    ],

});

this makes it easier to separate layout/hierarchy and the
actual behaviours (i.e. some events or actions)

more comments inline

On 4/28/21 16:13, Lorenz Stechauner wrote:
> Add PVE.storage.Retrieve window and PVE.form.hashAlgorithmSelector.
> Users are now able to download/retrieve any .iso/... file onto their
> storages and verify file integrity with checksums.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   www/manager6/Makefile                      |   1 +
>   www/manager6/form/HashAlgorithmSelector.js |  21 +++
>   www/manager6/storage/ContentView.js        | 161 +++++++++++++++++++++
>   3 files changed, 183 insertions(+)
>   create mode 100644 www/manager6/form/HashAlgorithmSelector.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index afed3283..8e6557d8 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -38,6 +38,7 @@ JSSRC= 							\
>   	form/GlobalSearchField.js			\
>   	form/GroupSelector.js				\
>   	form/GuestIDSelector.js				\
> +	form/HashAlgorithmSelector.js			\
>   	form/HotplugFeatureSelector.js			\
>   	form/IPProtocolSelector.js			\
>   	form/IPRefSelector.js				\
> diff --git a/www/manager6/form/HashAlgorithmSelector.js b/www/manager6/form/HashAlgorithmSelector.js
> new file mode 100644
> index 00000000..4a72cc08
> --- /dev/null
> +++ b/www/manager6/form/HashAlgorithmSelector.js
> @@ -0,0 +1,21 @@
> +Ext.define('PVE.form.hashAlgorithmSelector', {
> +    extend: 'Proxmox.form.KVComboBox',
> +    alias: ['widget.pveHashAlgorithmSelector'],
> +    comboItems: [],
> +    hasNoneOption: false,
> +    initComponent: function() {
> +	var me = this;
> +	me.comboItems = [
> +	    ['md5', 'MD5'],
> +	    ['sha1', 'SHA-1'],
> +	    ['sha224', 'SHA-224'],
> +	    ['sha256', 'SHA-256'],
> +	    ['sha384', 'SHA-384'],
> +	    ['sha512', 'SHA-512'],
> +	];
> +	if (me.hasNoneOption) {
> +	    me.comboItems.unshift(['none', 'None']);
> +	}

why do you make this optional, the only user always adds it
afaics? or are there more potential users in the future?
in that case, the user may want to configure the
whole list (e.g. no md5, only none/sha256, etc.)

> +	this.callParent();
> +    },
> +});
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index dd6df4b1..7187ebbe 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -191,6 +191,153 @@ Ext.define('PVE.storage.Upload', {
>       },
>   });
>   
> +Ext.define('PVE.storage.Retrieve', {
> +    extend: 'Proxmox.window.Edit',
> +    alias: 'widget.pveStorageRetrieve',
> +
> +    resizable: false,

this can be omitted here, it's already set
to false in the edit window

> +
> +    modal: true,

same here

> +
> +    isCreate: true,
> +
> +    showTaskViewer: true,
> +    upidFieldName: 'upid',

this is only necessary because the api call does two
things yeah?

if we change to a 'get_url_info' + 'retrive' model,
this is not necessary at all

> +
> +    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}/retrieve`;
> +	me.method = 'POST';
> +
> +	let defaultContent = me.contents[0] || '';
> +
> +	let urlField = Ext.create('Ext.form.field.Text', {
> +	    name: 'url',
> +	    allowBlank: false,
> +	    fieldLabel: gettext('URL'),
> +	});
> +
> +	let fileNameField = Ext.create('Ext.form.field.Text', {
> +	    name: 'filename',
> +	    allowBlank: false,
> +	    fieldLabel: gettext('File name'),
> +	});
> +
> +	let fileSizeField = Ext.create('Ext.form.field.Text', {
> +	    name: 'size',
> +	    disabled: true,
> +	    fieldLabel: gettext('File size'),
> +	    emptyText: gettext('unknown'),
> +	});
> +
> +	let checksumField = Ext.create('Ext.form.field.Text', {
> +	    name: 'checksum',
> +	    fieldLabel: gettext('Checksum'),
> +	    allowBlank: true,
> +	    disabled: true,
> +	    emptyText: gettext('none'),
> +	});
> +
> +	let checksumAlgField = Ext.create('PVE.form.hashAlgorithmSelector', {
> +	    name: 'checksumalg',
> +	    fieldLabel: gettext('Hash algorithm'),
> +	    allowBlank: true,
> +	    hasNoneOption: true,
> +	    value: 'none',
> +	});
> +
> +	let inputPanel = Ext.create('Proxmox.panel.InputPanel', {
> +	    method: 'POST',
> +	    waitMsgTarget: true,
> +	    border: false,
> +	    columnT: [
> +		urlField,
> +		fileNameField,
> +	    ],
> +	    column1: [
> +		{
> +		    xtype: 'pveContentTypeSelector',
> +		    cts: me.contents,
> +		    fieldLabel: gettext('Content'),
> +		    name: 'content',
> +		    value: defaultContent,
> +		    allowBlank: false,
> +		},
> +	    ],
> +	    column2: [
> +		fileSizeField,
> +	    ],
> +	    advancedColumn1: [
> +		checksumField,
> +		checksumAlgField,
> +	    ],
> +	    advancedColumn2: [
> +		{
> +		    xtype: 'checkbox',
> +		    name: 'insecure',
> +		    fieldLabel: gettext('Trust invalid certificates'),
> +		    labelWidth: 150,
> +		},

this btw. does not work for the filename+size, so i cannot
download a file from a url with an untrusted cert (via gui)
since the field is never valid

> +	    ],
> +	});
> +
> +	urlField.on('change', function() {
> +	    urlField.setValidation("Waiting for response...");
> +	    urlField.validate();
> +	    fileSizeField.setValue("");
> +	    Proxmox.Utils.API2Request({
> +		url: me.url,
> +		method: 'POST',
> +		params: {
> +		    metaonly: 1,
> +		    url: me.getValues()['url'],
> +		    content: me.getValues()['content'],
> +		},
> +		failure: function(res, opt) {
> +		    urlField.setValidation(res.result.message);
> +		    urlField.validate();
> +		    fileSizeField.setValue("");
> +		},
> +		success: function(res, opt) {
> +		    urlField.setValidation();
> +		    urlField.validate();
> +
> +		    let data = res.result.data;
> +		    fileNameField.setValue(data.filename);
> +		    fileSizeField.setValue(Proxmox.Utils.format_size(data.size));
> +		},
> +	    });
> +	});

this needs some buffering
as it is now, it makes a request on every key stroke
i.e. when i type in:

http://www.google.com

it makes 21(!) requests

e.g. textfield has a 'checkChangeBuffer' property
https://docs.sencha.com/extjs/6.0.1/classic/Ext.form.field.Text.html#cfg-checkChangeBuffer

or the buffer property of the listener:
https://docs.sencha.com/extjs/6.0.1/classic/Ext.util.Observable.html#method-addListener

also it would probably be good if the window
gets a load mask when the request gets done
so the user has a feedback if its taking long

> +
> +	checksumAlgField.on('change', function() {
> +	    if (this.getValue() === 'none') {
> +		checksumField.setDisabled(true);
> +		checksumField.setValue("");
> +		checksumField.allowBlank = true;
> +	    } else {
> +		checksumField.setDisabled(false);
> +		checksumField.allowBlank = false;
> +	    }
> +	});
> +
> +	Ext.apply(me, {
> +	    title: gettext('Retrieve from URL'),
> +	    items: inputPanel,
> +	    submitText: gettext('Download'),
> +	});
> +
> +        me.callParent();
> +    },
> +});
> +
>   Ext.define('PVE.storage.ContentView', {
>       extend: 'Ext.grid.GridPanel',
>   
> @@ -262,6 +409,19 @@ Ext.define('PVE.storage.ContentView', {
>   	    },
>   	});
>   
> +	let retrieveButton = Ext.create('Proxmox.button.Button', {
> +	    text: gettext('Retrieve from URL'),
> +	    handler: function() {
> +		let win = Ext.create('PVE.storage.Retrieve', {
> +		    nodename: nodename,
> +		    storage: storage,
> +		    contents: [content],
> +		});
> +		win.show();
> +		win.on('destroy', reload);

this can be simplified to

Ext.create('PVE.storage.Retrieve', {
	nodename,
	storage,
	contents: [content],
	autoShow: true,
	listeners: {
		destroy: () => reload(),
	},
});

caution, a listener (.on(), changes the scope of the function)

> +	    },
> +	});
> +
>   	let removeButton = Ext.create('Proxmox.button.StdRemoveButton', {
>   	    selModel: sm,
>   	    delay: 5,
> @@ -276,6 +436,7 @@ Ext.define('PVE.storage.ContentView', {
>   	}
>   	if (me.useUploadButton) {
>   	    me.tbar.push(uploadButton);
> +	    me.tbar.push(retrieveButton);
>   	}
>   	if (!me.useCustomRemoveButton) {
>   	    me.tbar.push(removeButton);
>