Re: [pve-devel] [PATCH v2 manager 2/5] ui: refactor UploadToStorage.js

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

comments inline:

On 7/22/21 15:06, Lorenz Stechauner wrote:
> this also removes the "content" selector from the window.
> as far as it seems, this selector was never able to select
> more than one entry, so it was useless.
> 
> the check for FormData() is also removed, because this is
> supported by all major browsers for a long time. therefore
> doStandardSubmit() is also not necessary.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   www/manager6/storage/ContentView.js    |   2 +-
>   www/manager6/window/UploadToStorage.js | 360 ++++++++++++++-----------
>   2 files changed, 209 insertions(+), 153 deletions(-)
> 
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index ca0ad664..00a94f3c 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -69,7 +69,7 @@ Ext.define('PVE.storage.ContentView', {
>   			Ext.create('PVE.window.UploadToStorage', {
>   			    nodename: nodename,
>   			    storage: storage,
> -			    contents: [content],
> +			    content: content,
>   			    autoShow: true,
>   			    taskDone: () => reload(),
>   			});
> diff --git a/www/manager6/window/UploadToStorage.js b/www/manager6/window/UploadToStorage.js
> index 3c35020a..fb9850b3 100644
> --- a/www/manager6/window/UploadToStorage.js
> +++ b/www/manager6/window/UploadToStorage.js
> @@ -1,191 +1,247 @@
>   Ext.define('PVE.window.UploadToStorage', {
>       extend: 'Ext.window.Window',
>       alias: 'widget.pveStorageUpload',
> +    mixins: ['Proxmox.Mixin.CBind'],
>   
>       resizable: false,
> -
>       modal: true,
>   
> -    initComponent: function() {
> -        var me = this;
> +    title: gettext('Upload'),
>   
> -	if (!me.nodename) {
> -	    throw "no node name specified";
> -	}
> -	if (!me.storage) {
> -	    throw "no storage ID specified";
> -	}
> +    acceptedExtensions: {
> +	iso: ['.img', '.iso'],
> +	vztmpl: ['.tar.gz', '.tar.xz'],
> +    },
> +
> +    cbindData: function(initialConfig) {
> +	const me = this;
> +	return {
> +	    nodename: me.nodename,
> +	    storage: me.storage,
> +	    content: me.content,

these should not be necessary, all properties of the view should 
automatically also be cbindable (maybe a default needs to be set)

> +	    extensions: me.acceptedExtensions[me.content].join(', '),

i'd prefer to check if me.content is actually a valid type,
and maybe erroring out if not (e.g 'throw ...')

> +	};
> +    },
>   
> -	let baseurl = `/nodes/${me.nodename}/storage/${me.storage}/upload`;
> +    cbind: {
> +	url: `/nodes/{nodename}/storage/{storage}/upload`,
> +    },

we normally do this in the cbindData function above

me.url = ``;

>   
> -	let pbar = Ext.create('Ext.ProgressBar', {
> -            text: 'Ready',
> -	    hidden: true,
> -	});
> +    viewModel: {
> +	data: {
> +	    size: '-',
> +	    mimetype: '-',
> +	    filename: '',
> +	},
> +    },
>   
> -	let acceptedExtensions = {
> -	    iso: ".img, .iso",
> -	    vztmpl: ".tar.gz, .tar.xz",
> -	};
> +    controller: {
> +	submit: function(button) {
> +	    const view = this.getView();
> +	    const form = Ext.getCmp('formPanel').getForm();
> +	    const abortBtn = Ext.getCmp('abortBtn');
> +	    const pbar = Ext.getCmp('progressBar');
> +
> +	    const updateProgress = function(per, bytes) {
> +		let text = (per * 100).toFixed(2) + '%';
> +		if (bytes) {
> +		    text += " (" + Proxmox.Utils.format_size(bytes) + ')';
> +		}
> +		pbar.updateProgress(per, text);
> +	    };
>   
> -	let defaultContent = me.contents[0] || '';
> -
> -	let fileField = Ext.create('Ext.form.field.File', {
> -	    name: 'filename',
> -	    buttonText: gettext('Select File...'),
> -	    allowBlank: false,
> -	    setAccept: function(content) {
> -		let acceptString = acceptedExtensions[content] || '';
> -		this.fileInputEl.set({
> -		    accept: acceptString,
> -		});
> -	    },
> -	    listeners: {
> -		afterrender: function(cmp) {
> -		    cmp.setAccept(defaultContent);
> -		},
> -	    },
> -	});
> +	    const fd = new FormData();
> +
> +	    button.setDisabled(true);
> +	    abortBtn.setDisabled(false);
>   
> -	me.formPanel = Ext.create('Ext.form.Panel', {
> +	    const contentField = form.findField('content');
> +	    fd.append("content", contentField.getValue());
> +	    contentField.setDisabled(true);

why use a separate input field for this?
you could simply access view.content, no?
would make the code here and below a bit shorter.

> +
> +	    const fileField = form.findField('file');
> +	    const file = fileField.fileInputEl.dom.files[0];
> +	    fileField.setDisabled(true);
> +
> +	    const filenameField = form.findField('filename');
> +	    const filename = filenameField.getValue();
> +	    filenameField.setDisabled(true);

why do you get the filename field here?
we already have the info in the viewModel (since we 'bind' the value)

const vm = view.getViewModel();
const filename = vm.get('filename');


> +
> +	    const algorithmField = form.findField('checksum-algorithm');
> +	    algorithmField.setDisabled(true);
> +	    if (algorithmField.getValue() !== '__default__') {
> +		fd.append("checksum-algorithm", algorithmField.getValue());
> +
> +		const checksumField = form.findField('checksum');
> +		fd.append("checksum", checksumField.getValue());
> +		checksumField.setDisabled(true);
> +	    }

it seems this belongs in the next patch?
at least here it would fail since it would not find the field

> +
> +	    fd.append("filename", file, filename);
> +
> +	    pbar.setVisible(true);
> +	    updateProgress(0);
> +
> +	    const xhr = new XMLHttpRequest();
> +	    view.xhr = xhr;
> +
> +	    xhr.addEventListener("load", function(e) {
> +		if (xhr.status === 200) {
> +		    view.close();
> +		    return;
> +		}
> +		const err = Ext.htmlEncode(xhr.statusText);
> +		let msg = `${gettext('Error')} ${xhr.status.toString()}: ${err}`;
> +		if (xhr.responseText !== "") {
> +		    const result = Ext.decode(xhr.responseText);
> +		    result.message = msg;
> +		    msg = Proxmox.Utils.extractRequestError(result, true);
> +		}
> +		Ext.Msg.alert(gettext('Error'), msg, btn => view.close());
> +	    }, false);
> +
> +	    xhr.addEventListener("error", function(e) {
> +		const err = e.target.status.toString();
> +		const msg = `Error '${err}' occurred while receiving the document.`;
> +		Ext.Msg.alert(gettext('Error'), msg, btn => view.close());
> +	    });
> +
> +	    xhr.upload.addEventListener("progress", function(evt) {
> +		if (evt.lengthComputable) {
> +		    const percentComplete = evt.loaded / evt.total;
> +		    updateProgress(percentComplete, evt.loaded);
> +		}
> +	    }, false);
> +
> +	    xhr.open("POST", `/api2/json${view.url}`, true);
> +	    xhr.send(fd);
> +	},
> +
> +	validitychange: function(f, valid) {
> +	    const submitBtn = Ext.getCmp('submitBtn');
> +	    submitBtn.setDisabled(!valid);
> +	},
> +
> +	fileChange: function(input) {
> +	    const vm = this.getViewModel();
> +	    const name = input.value.replace(/^.*(\/|\\)/, '');
> +	    const fileInput = input.fileInputEl.dom;
> +	    vm.set('filename', name);
> +	    vm.set('size', (fileInput.files[0] && Proxmox.Utils.format_size(fileInput.files[0].size)) || '-');
> +	    vm.set('mimetype', (fileInput.files[0] && fileInput.files[0].type) || '-');
> +	},
> +    },
> +
> +    items: [
> +	{
> +	    xtype: 'form',
> +	    id: 'formPanel',
>   	    method: 'POST',
>   	    waitMsgTarget: true,
>   	    bodyPadding: 10,
>   	    border: false,
> -	    width: 300,
> +	    width: 400,
>   	    fieldDefaults: {
>   		labelWidth: 100,
>   		anchor: '100%',
>               },
>   	    items: [
>   		{
> -		    xtype: 'pveContentTypeSelector',
> -		    cts: me.contents,
> -		    fieldLabel: gettext('Content'),
> -		    name: 'content',
> -		    value: defaultContent,
> +		    xtype: 'filefield',
> +		    name: 'file',
> +		    buttonText: gettext('Select File'),
>   		    allowBlank: false,
> +		    fieldLabel: gettext('File'),
> +		    cbind: {
> +			accept: '{extensions}',
> +		    },
>   		    listeners: {
> -			change: function(cmp, newValue, oldValue) {
> -			    fileField.setAccept(newValue);
> -			},
> +			change: 'fileChange',
>   		    },
>   		},
> -		fileField,
> -		pbar,
> -	    ],
> -	});
> -
> -	let form = me.formPanel.getForm();
> -
> -	let doStandardSubmit = function() {
> -	    form.submit({
> -		url: "/api2/htmljs" + baseurl,
> -		waitMsg: gettext('Uploading file...'),
> -		success: function(f, action) {
> -		    me.close();
> +		{
> +		    xtype: 'textfield',
> +		    name: 'filename',
> +		    allowBlank: false,
> +		    fieldLabel: gettext('File name'),
> +		    bind: {
> +			value: '{filename}',
> +		    },
>   		},
> -		failure: function(f, action) {
> -		    var msg = PVE.Utils.extractFormActionError(action);
> -                    Ext.Msg.alert(gettext('Error'), msg);
> +		{
> +		    xtype: 'displayfield',
> +		    name: 'size',
> +		    fieldLabel: gettext('File size'),
> +		    bind: {
> +			value: '{size}',
> +		    },
>   		},
> -	    });
> -	};
> -
> -	let updateProgress = function(per, bytes) {
> -	    var text = (per * 100).toFixed(2) + '%';
> -	    if (bytes) {
> -		text += " (" + Proxmox.Utils.format_size(bytes) + ')';
> -	    }
> -	    pbar.updateProgress(per, text);
> -	};
> -
> -	let abortBtn = Ext.create('Ext.Button', {
> +		{
> +		    xtype: 'displayfield',
> +		    name: 'mimetype',
> +		    fieldLabel: gettext('MIME type'),
> +		    bind: {
> +			value: '{mimetype}',
> +		    },
> +		},
> +		{
> +		    xtype: 'progressbar',
> +		    text: 'Ready',
> +		    hidden: true,
> +		    id: 'progressBar',
> +		},
> +		{
> +		    xtype: 'hiddenfield',
> +		    name: 'content',
> +		    cbind: {
> +			value: '{content}',
> +		    },
> +		},
> +	    ],
> +	   listeners: {
> +		validitychange: 'validitychange',
> +	   },
> +	},
> +    ],
> +
> +    buttons: [
> +	{
> +	    xtype: 'button',
>   	    text: gettext('Abort'),
> +	    id: 'abortBtn',
>   	    disabled: true,
>   	    handler: function() {
> -		me.close();
> +		const me = this;
> +		me.up('pveStorageUpload').close();
>   	    },
> -	});
> -
> -	let submitBtn = Ext.create('Ext.Button', {
> +	},
> +	{
>   	    text: gettext('Upload'),
> +	    id: 'submitBtn',
>   	    disabled: true,
> -	    handler: function(button) {
> -		var fd;
> -		try {
> -		    fd = new FormData();
> -		} catch (err) {
> -		    doStandardSubmit();
> -		    return;
> -		}
> +	    handler: 'submit',
> +	},
> +    ],
> +
> +    listeners: {
> +	close: function() {
> +	    const me = this;
> +	    if (me.xhr) {
> +		me.xhr.abort();
> +		delete me.xhr;
> +	    }
> +	},
> +    },
>   
> -		button.setDisabled(true);
> -		abortBtn.setDisabled(false);
> -
> -		var field = form.findField('content');
> -		fd.append("content", field.getValue());
> -		field.setDisabled(true);
> -
> -		field = form.findField('filename');
> -		var file = field.fileInputEl.dom;
> -		fd.append("filename", file.files[0]);
> -		field.setDisabled(true);
> -
> -		pbar.setVisible(true);
> -		updateProgress(0);
> -
> -		let xhr = new XMLHttpRequest();
> -		me.xhr = xhr;
> -
> -		xhr.addEventListener("load", function(e) {
> -		    if (xhr.status === 200) {
> -			me.close();
> -			return;
> -		    }
> -		    let err = Ext.htmlEncode(xhr.statusText);
> -		    let msg = `${gettext('Error')} ${xhr.status.toString()}: ${err}`;
> -		    if (xhr.responseText !== "") {
> -			let result = Ext.decode(xhr.responseText);
> -			result.message = msg;
> -			msg = Proxmox.Utils.extractRequestError(result, true);
> -		    }
> -		    Ext.Msg.alert(gettext('Error'), msg, btn => me.close());
> -		}, false);
> -
> -		xhr.addEventListener("error", function(e) {
> -		    let err = e.target.status.toString();
> -		    let msg = `Error '${err}' occurred while receiving the document.`;
> -		    Ext.Msg.alert(gettext('Error'), msg, btn => me.close());
> -		});
> -
> -		xhr.upload.addEventListener("progress", function(evt) {
> -		    if (evt.lengthComputable) {
> -			let percentComplete = evt.loaded / evt.total;
> -			updateProgress(percentComplete, evt.loaded);
> -		    }
> -		}, false);
> -
> -		xhr.open("POST", `/api2/json${baseurl}`, true);
> -		xhr.send(fd);
> -	    },
> -	});
> -
> -	form.on('validitychange', (f, valid) => submitBtn.setDisabled(!valid));
> -
> -	Ext.apply(me, {
> -	    title: gettext('Upload'),
> -	    items: me.formPanel,
> -	    buttons: [abortBtn, submitBtn],
> -	    listeners: {
> -		close: function() {
> -		    if (me.xhr) {
> -			me.xhr.abort();
> -			delete me.xhr;
> -		    }
> -		},
> -	    },
> -	});
> +    initComponent: function() {
> +        const me = this;
> +
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +	if (!me.storage) {
> +	    throw "no storage ID specified";
> +	}
>   
>           me.callParent();
>       },
>