From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with UTF8SMTPS id C9C2870864 for ; Fri, 14 May 2021 11:44:49 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with UTF8SMTP id AEBCC15873 for ; Fri, 14 May 2021 11:44:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with UTF8SMTPS id A828815865 for ; Fri, 14 May 2021 11:44:18 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with UTF8SMTP id 6FD4C46568; Fri, 14 May 2021 11:44:18 +0200 (CEST) Message-ID: Date: Fri, 14 May 2021 11:44:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:89.0) Gecko/20100101 Thunderbird/89.0 Content-Language: en-US To: Proxmox VE development discussion , Lorenz Stechauner References: <20210512082240.36224-1-l.stechauner@proxmox.com> <20210512082240.36224-8-l.stechauner@proxmox.com> From: Dominik Csapak In-Reply-To: <20210512082240.36224-8-l.stechauner@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.033 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [caps.storage, me.storage] Subject: Re: [pve-devel] [PATCH v5 manager 7/7] fix #1710: ui: storage: add download from url button X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 May 2021 09:44:49 -0000 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 > --- > 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( > '->', >