Re: [pve-devel] [pbs-devel] [PATCH v2 proxmox-widget-toolbox 1/6] safe-destroy: moved here from pve-manager and generalized it

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 19.08.20 11:52, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> 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);
> +    },
> +});
>