Re: [pve-devel] [PATCH proxmox-widget-toolkit 1/5] ui refactoring: refactored SafeDestroy from pve-manager into proxmox-widget-toolkit

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

You say refactored: What, besides the name, did you change and why?

On 18.08.20 10:40, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/Makefile             |   1 +
>  src/window/SafeRemove.js | 193 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 194 insertions(+)
>  create mode 100644 src/window/SafeRemove.js
> 

> +
> +    initComponent: function () {
> +        const me = this;
> +        me.callParent();
> +
> +        const item = me.getItem();
> +
> +        if (!Ext.isDefined(item.id)) {
> +            throw "no ID specified";
> +        }
> +
> +        if (!Ext.isDefined(item.type)) {
> +            throw "no Disk type specified";

does this always have to by a disk type? It can be a VM, or something completely different.

> +        }
> +
> +        const messageCmp = me.lookupReference('messageCmp');
> +        let msg;
> +
> +        if (item.type === 'VM') {
> +            msg = Proxmox.Utils.format_task_description('qmdestroy', item.id);
> +        } else if (item.type === 'CT') {
> +            msg = Proxmox.Utils.format_task_description('vzdestroy', item.id);
> +        } else if (item.type === 'CephPool') {
> +            msg = Proxmox.Utils.format_task_description('cephdestroypool', item.id);
> +        } else if (item.type === 'Image') {
> +            msg = Proxmox.Utils.format_task_description('unknownimgdel', item.id);
> +        } else {
> +            throw "unknown item type specified";
> +        }

I'd like to avoid having downstream logic here, that adds cyclic coupling.
Why not either do:
* a minimal overwrite in PVE, would have the additional benefit of not needing
  to touch all use sites there.
* replace the "type" property with a "task" property, avoiding the need to map.

> +
> +        if (!(item.type === 'VM' || item.type === 'CT')) {
> +            let purgeCheckbox = me.lookupReference('purgeCheckbox');
> +            purgeCheckbox.setDisabled(true);
> +            purgeCheckbox.setHidden(true);

If we touch this we could also make this more explicit with a default off "purgeable"
(or similar name) config property.

> +        }
> +
> +        messageCmp.setHtml(msg);
> +
> +        const confirmField = me.lookupReference('confirmField');
> +        msg = gettext('Please enter the ID to confirm') +
> +            ' (' + item.id + ')';
> +        confirmField.setFieldLabel(msg);
> +    }
> +});
>