[Kimchi-devel] [WIP PATCH] Storage DL/UL

[Crístian Viana]

Hi! I tested this patch and here's my overall feedback:

- When the button "OK" is clicked and there is some error (e.g. the URL 
field is empty, the URL text doesn't start with a correct
protocol), the button gets disabled and it never gets enabled again 
unless the user closes and reopens the dialog.
- When I click on the button "OK" to upload a file, the dialog doesn't 
close and nothing is shown. This is not the same behaviour when 
downloading a file: the dialog is closed immediately as soon as the 
submit button is clicked.
- When a download finishes, the progress box doesn't update the message 
"Downloading...". That message should disappear and the storage volume 
box should look like the other existing volumes in that pool.
- The correct text when uploading a file should be "Upload a file" 
(instead of "...an file").
- IMO the submit button's label should be a verb (e.g. "Add") instead of 
"OK".
- IMO the menu item "Add Volume" should be the first element on the menu.

The feedback related to the code is below:

> -            {'fc_host': libvirt.VIR_CONNECT_LIST_NODE_DEVICES_CAP_FC_HOST,
> +            {#'fc_host': libvirt.VIR_CONNECT_LIST_NODE_DEVICES_CAP_FC_HOST,

If you're not using that code anymore, remove it instead of commenting 
it. Git keeps track of the code history.

> +.volume-progress .progress-status {
> +}
> +

What's this for? It's an empty declaration.

> +    var onTaskResponse = function(result) {
> +        var taskStatus = result['status'];
> +        switch(taskStatus) {
> +        case 'running':
> +            onProgress(result);
> +            setTimeout(function() {
> +                trackTask(result['id']);
> +            }, 1500);
> +            break;
> +        case 'finished':
> +            taskID = -1;
> +            var volumeName = result['target_url'].split('/').pop();
> +            var volumeBox = $('#volume' + kimchi.selectedSP + ' [data-volume-name="' + volumeName + '"]');
> +            $('.progress-status', volumeBox).text(i18n['KCHPOOL6015M']);
> +            break;
> +        case 'failed':
> +            taskID = -1;
> +            onError(result);
> +            break;
> +        default:
> +            break;
> +        }
> +    };
> +
> +    var trackTask = function(task) {
> +        kimchi.getTask(task, onTaskResponse, function(resp) {});
> +    };

Does this tracking approach work for multiple simultaneous 
downloads/uploads?

> +
> +    var uploadFile = function() {
> +        var blobFile = $('#volume-input-file')[0].files[0];
> +        var fileName = blobFile.name;
> +        var fd = new FormData();
> +        fd.append('name', fileName);
> +        fd.append('file', blobFile);
> +        kimchi.uploadVolumeToSP({
> +            sp: kimchi.selectedSP,
> +            formData: fd
> +        }, function(resp) {
> +        });
Why isn't an error handler used here as well, just like the download 
function call?

I tested your patches on top of my [still] local branch with the patch 
"storagevolume: Use default value for param 'name' when appropriate" 
which I sent today for review on this mailing list. Without it, the 
download and upload functions don't work because the parameter 'name' is 
not passed correctly to the backend. That's why I sent this patch today.