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 5EE5E98CAD for ; Tue, 10 Oct 2023 11:05:38 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3A1F2301DE for ; Tue, 10 Oct 2023 11:05:08 +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 ESMTPS for ; Tue, 10 Oct 2023 11:05:07 +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 CCC284493E for ; Tue, 10 Oct 2023 11:05:06 +0200 (CEST) Message-ID: <7efa009d-a474-41db-b4bd-6e6cf78606c2@proxmox.com> Date: Tue, 10 Oct 2023 11:05:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Lukas Wagner , Proxmox Backup Server development discussion References: <20231010063926.2248-1-m.frank@proxmox.com> <20231010063926.2248-2-m.frank@proxmox.com> <4e7d4f89-7dc6-4065-9089-0d74dc0d369f@proxmox.com> From: Markus Frank In-Reply-To: <4e7d4f89-7dc6-4065-9089-0d74dc0d369f@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.047 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Oct 2023 09:05:38 -0000 Thanks for the review. Comments inline On 10/10/23 10:43, Lukas Wagner wrote: > Comments inline. > > On 10/10/23 08:39, Markus Frank wrote: >> With this patch it is possible to remove systemd mount units via the webui. >> >> Signed-off-by: Markus Frank >> --- >>   www/DirectoryList.js | 79 ++++++++++++++++++++++++++++++++++++++++++++ >>   1 file changed, 79 insertions(+) >> >> diff --git a/www/DirectoryList.js b/www/DirectoryList.js >> index 00531fd0..53aa76cc 100644 >> --- a/www/DirectoryList.js >> +++ b/www/DirectoryList.js >> @@ -7,6 +7,15 @@ Ext.define('PBS.admin.Directorylist', { >>       emptyText: gettext('No Mount-Units found'), >> +    viewModel: { >> +    data: { >> +        path: '', >> +    }, >> +    formulas: { >> +        dirName: (get) => get('path')?.replace('/mnt/datastore/', '') || undefined, >> +    }, >> +    }, >> + >>       controller: { >>       xclass: 'Ext.app.ViewController', >> @@ -21,6 +30,27 @@ Ext.define('PBS.admin.Directorylist', { >>           }).show(); >>       }, >> +    removeDirectory: function() { >> +        let me = this; >> +        let vm = me.getViewModel(); >> + >> +        const dirName = vm.get('dirName'); > Nit: As per our JS style guidelines I'd use 'let' here. For objects, > 'const' only prohibits reassignemnt, but not modification of the > actual object. Okay > >> + >> +        if (!dirName) { >> +        throw "no directory name specified"; >> +        } >> + >> +        Ext.create('Proxmox.window.SafeDestroy', { >> +        url: `/nodes/localhost/disks/directory/${dirName}`, >> +        item: { id: dirName }, >> +        showProgress: true, >> +        taskName: 'dirremove', >> +        listeners: { >> +            destroy: () => me.reload(), >> +        }, >> +        }).show(); >> +    }, >> + >>       reload: function() { >>           let me = this; >>           let store = me.getView().getStore(); >> @@ -49,6 +79,45 @@ Ext.define('PBS.admin.Directorylist', { >>           text: gettext('Create') + ': Directory', >>           handler: 'createDirectory', >>       }, >> +    '->', >> +        { >> +            xtype: 'tbtext', >> +            data: { >> +                dirName: undefined, >> +            }, >> +            bind: { >> +                data: { >> +                    dirName: "{dirName}", >> +                }, >> +            }, >> +            tpl: [ >> +                '', >> +                gettext('Directory') + ' {dirName}:', >> +                '', >> +                Ext.String.format(gettext('No {0} selected'), gettext('directory')), >> +                '', >> +            ], >> +    }, > Indentation is off here (only spaces). Okay > >> +    { >> +        text: gettext('More'), >> +        iconCls: 'fa fa-bars', >> +        disabled: true, >> +        bind: { >> +        disabled: '{!dirName}', >> +        }, >> +        menu: [ >> +        { >> +            text: gettext('Remove'), >> +            itemId: 'remove', >> +            iconCls: 'fa fa-fw fa-trash-o', >> +            handler: 'removeDirectory', >> +            disabled: true, >> +            bind: { >> +            disabled: '{!dirName}', >> +            }, >> +        }, >> +        ], >> +    }, >>       ], >>       columns: [ >> @@ -79,6 +148,16 @@ Ext.define('PBS.admin.Directorylist', { >>       }, >>       ], >> +    listeners: { >> +    activate: "reload", >> +    selectionchange: function(model, selected) { >> +        let me = this; >> +        let vm = me.getViewModel(); >> + >> +        vm.set('path', selected[0]?.data.path || ''); >> +    }, >> +    }, >> + >>       store: { >>       fields: ['path', 'device', 'filesystem', 'options', 'unitfile'], >>       proxy: { > > A high-level comment about the UI changes: > In most places in our UI, we have a dedicated 'Remove' button right next > to the 'Add/Create' and 'Edit' button: > > [Add] [Edit] [Remove] > > We don't need 'Edit' here, but why not just add the button right next > to the 'Create: Directory' button? > What's the rationale of  moving the remove button to a menu on the > right-hand side? Because I think it would be better if it looks the same in pve & pbs. I think the reason for the menu in pve is that it is more difficult to accidentally remove a directory. >