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 ESMTPS id 340C46750F; Wed, 26 Aug 2020 20:20:26 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 273FC112D2; Wed, 26 Aug 2020 20:20:26 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 49D75112C2; Wed, 26 Aug 2020 20:20:24 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 18753448A0; Wed, 26 Aug 2020 20:20:24 +0200 (CEST) To: Proxmox Backup Server development discussion , Hannes Laimer , pve-devel@lists.proxmox.com References: <20200819095247.85518-1-h.laimer@proxmox.com> <20200819095247.85518-2-h.laimer@proxmox.com> From: Thomas Lamprecht Message-ID: <0df49164-e1dd-f09d-053e-84aa71056dc1@proxmox.com> Date: Wed, 26 Aug 2020 20:20:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:80.0) Gecko/20100101 Thunderbird/80.0 MIME-Version: 1.0 In-Reply-To: <20200819095247.85518-2-h.laimer@proxmox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.214 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 -2.602 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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. [item.id] Subject: Re: [pve-devel] [pbs-devel] [PATCH v2 proxmox-widget-toolbox 1/6] safe-destroy: moved here from pve-manager and generalized it 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: Wed, 26 Aug 2020 18:20:26 -0000 On 19.08.20 11:52, Hannes Laimer wrote: > Signed-off-by: Hannes Laimer > --- > v1->v2: - kept name > - introduced purgeable and taskName as config in SafeDestroy, in > order to avoid having downstream logic here > - fixed eslint related issues again, nothing in the commit message about what changed in-between moving. As of now we have no idea what you change, effectively you sneak in hard to reviewable code changes. I want a: * plain move, touch only the really essential things, which should boil down to the "Ext.define('Proxmox.window.SafeDestroy" line) * then do the respective changes you want to do, ideally not all in one commit but semantically grouped together. Also, indentation is way of - I know the scheme in use is extra, and with some editors a bit a PITA to configure, but please ensure you do so - even if we change it for javascript files, we won't do so for perl, and perl will stick around for quite a few years... > > src/Makefile | 1 + > src/window/SafeDestroy.js | 183 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 184 insertions(+) > create mode 100644 src/window/SafeDestroy.js > > diff --git a/src/Makefile b/src/Makefile > index 12dda30..ea71647 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -43,6 +43,7 @@ JSSRC= \ > panel/GaugeWidget.js \ > window/Edit.js \ > window/PasswordEdit.js \ > + window/SafeDestroy.js \ > window/TaskViewer.js \ > window/LanguageEdit.js \ > window/DiskSmart.js \ > diff --git a/src/window/SafeDestroy.js b/src/window/SafeDestroy.js > new file mode 100644 > index 0000000..81c7c27 > --- /dev/null > +++ b/src/window/SafeDestroy.js > @@ -0,0 +1,183 @@ > +/* Popup a message window > + * where the user has to manually enter the resource ID > + * to enable the destroy button > + */ > +Ext.define('Proxmox.window.SafeDestroy', { > + extend: 'Ext.window.Window', > + alias: 'proxmoxSafeDestroy', > + > + title: gettext('Confirm'), > + modal: true, > + buttonAlign: 'center', > + bodyPadding: 10, > + width: 450, > + layout: { type: 'hbox' }, > + defaultFocus: 'confirmField', > + showProgress: false, > + > + config: { > + item: { > + id: undefined, > + purgeable: false, > + }, > + url: undefined, > + taskName: undefined, > + params: {}, > + }, > + > + getParams: function() { > + const me = this; > + const purgeCheckbox = me.lookupReference('purgeCheckbox'); > + if (purgeCheckbox.checked) { > + me.params.purge = 1; > + } > + if (Ext.Object.isEmpty(me.params)) { > + return ''; > + } > + return '?' + Ext.Object.toQueryString(me.params); > + }, > + > + controller: { > + xclass: 'Ext.app.ViewController', > + control: { > + 'field[name=confirm]': { > + change: function(f, value) { > + var view = this.getView(); > + var removeButton = this.lookupReference('removeButton'); > + if (value === view.getItem().id.toString()) { > + removeButton.enable(); > + } else { > + removeButton.disable(); > + } > + }, > + specialkey: function(field, event) { > + var removeButton = this.lookupReference('removeButton'); > + if (!removeButton.isDisabled() && > + event.getKey() === event.ENTER) { > + removeButton.fireEvent('click', removeButton, event); > + } > + }, > + }, > + 'button[reference=removeButton]': { > + click: function() { > + const view = this.getView(); > + Proxmox.Utils.API2Request({ > + url: view.getUrl() + view.getParams(), > + method: 'DELETE', > + waitMsgTarget: view, > + failure: function(response, opts) { > + view.close(); > + Ext.Msg.alert('Error', response.htmlStatus); > + }, > + success: function(response, options) { > + const hasProgressBar = !!(view.showProgress && > + response.result.data); > + > + if (hasProgressBar) { > + // stay around so we can trigger our close > + // events when background action is completed > + view.hide(); > + > + var upid = response.result.data; > + var win = > + Ext.create('Proxmox.window.TaskProgress', { > + upid: upid, > + listeners: { > + destroy: function() { > + view.close(); > + }, > + }, > + }); > + win.show(); > + } else { > + view.close(); > + } > + }, > + }); > + }, > + }, > + }, > + }, > + > + items: [ > + { > + xtype: 'component', > + cls: [Ext.baseCSSPrefix + 'message-box-icon', > + Ext.baseCSSPrefix + 'message-box-warning', > + Ext.baseCSSPrefix + 'dlg-icon'], > + }, > + { > + xtype: 'container', > + flex: 1, > + layout: { > + type: 'vbox', > + align: 'stretch', > + }, > + items: [ > + { > + xtype: 'component', > + reference: 'messageCmp', > + }, > + { > + itemId: 'confirmField', > + reference: 'confirmField', > + xtype: 'textfield', > + name: 'confirm', > + labelWidth: 300, > + hideTrigger: true, > + allowBlank: false, > + }, > + { > + xtype: 'proxmoxcheckbox', > + name: 'purge', > + reference: 'purgeCheckbox', > + boxLabel: gettext('Wipe'), > + checked: false, > + autoEl: { > + tag: 'div', > + 'data-qtip': gettext('Wipe disk after the removal of mountpoint'), > + }, > + }, > + ], > + }, > + ], > + buttons: [ > + { > + reference: 'removeButton', > + text: gettext('Remove'), > + disabled: true, > + }, > + ], > + > + initComponent: function() { > + const me = this; why some "var" uses are const now and some (above) ain't? I mean, in general I find it nice if things which are and probably will stay read only are marked as const, and "var" should definitively die... But, the "me = this" in the initComponent should stay writable, it is often modified and that's OK here - so I'd like to keep that as a pattern. So please use let me = this; here > + me.callParent(); > + > + const item = me.getItem(); > + > + if (!Ext.isDefined(item.id)) { > + throw "no ID specified"; > + } > + > + const messageCmp = me.lookupReference('messageCmp'); > + let msg; > + > + if (Ext.isDefined(me.getTaskName())) { > + msg = Proxmox.Utils.format_task_description(me.getTaskName(), item.id); > + messageCmp.setHtml(msg); > + } else { > + throw "no task name specified"; > + } > + > + if (!item.purgeable) { > + const purgeCheckbox = me.lookupReference('purgeCheckbox'); > + purgeCheckbox.setDisabled(true); > + purgeCheckbox.setHidden(true); > + } > + > + const confirmField = me.lookupReference('confirmField'); > + msg = gettext('Please enter the ID to confirm') + > + ' (' + item.id + ')'; > + confirmField.setFieldLabel(msg); > + }, > +}); >