From: Dominik Csapak <d.csapak@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 2/2] ui/cli: added support for the removal of mount-units
Date: Fri, 14 Aug 2020 10:13:13 +0200 [thread overview]
Message-ID: <9e05e98d-05a6-5370-8dcc-5b761c387f28@proxmox.com> (raw)
In-Reply-To: <20200813105853.144386-3-h.laimer@proxmox.com>
aside from dietmars comments, ui code looks mostly ok
some comments inline
On 8/13/20 12:58 PM, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/bin/proxmox_backup_manager/disk.rs | 29 +++-
> www/DirectoryList.js | 150 ++++++++++--------
> www/Makefile | 1 +
> www/window/SafeRemove.js | 209 +++++++++++++++++++++++++
> 4 files changed, 325 insertions(+), 64 deletions(-)
> create mode 100644 www/window/SafeRemove.js
>
> diff --git a/src/bin/proxmox_backup_manager/disk.rs b/src/bin/proxmox_backup_manager/disk.rs
> index a93a6f6b..b7eec592 100644
> --- a/src/bin/proxmox_backup_manager/disk.rs
> +++ b/src/bin/proxmox_backup_manager/disk.rs
> @@ -319,6 +319,32 @@ async fn create_datastore_disk(
> Ok(Value::Null)
> }
>
> +#[api(
> + input: {
> + properties: {
> + name: {
> + schema: DATASTORE_SCHEMA,
> + },
> + },
> + },
> +)]
> +/// Remove a Filesystem mounted under '/mnt/datastore/<name>'.".
> +async fn delete_datastore_disk(
> + mut param: Value,
> + rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> +
> + param["node"] = "localhost".into();
> +
> + let info = &api2::node::disks::directory::API_METHOD_DELETE_DATASTORE_DISK;
> + match info.handler {
> + ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
> + _ => unreachable!(),
> + };
> +
> + Ok(Value::Null)
> +}
> +
> pub fn filesystem_commands() -> CommandLineInterface {
>
> let cmd_def = CliCommandMap::new()
> @@ -327,7 +353,8 @@ pub fn filesystem_commands() -> CommandLineInterface {
> CliCommand::new(&API_METHOD_CREATE_DATASTORE_DISK)
> .arg_param(&["name"])
> .completion_cb("disk", complete_disk_name)
> - );
> + ).insert("remove", CliCommand::new(&API_METHOD_DELETE_DATASTORE_DISK)
> + .arg_param(&["name"]));
>
> cmd_def.into()
> }
> diff --git a/www/DirectoryList.js b/www/DirectoryList.js
> index 00531fd0..6ec91e6a 100644
> --- a/www/DirectoryList.js
> +++ b/www/DirectoryList.js
> @@ -8,83 +8,107 @@ Ext.define('PBS.admin.Directorylist', {
> emptyText: gettext('No Mount-Units found'),
>
> controller: {
> - xclass: 'Ext.app.ViewController',
> + xclass: 'Ext.app.ViewController',
>
> - createDirectory: function() {
> - let me = this;
> - Ext.create('PBS.window.CreateDirectory', {
> - listeners: {
> - destroy: function() {
> - me.reload();
> - },
> - },
> - }).show();
> - },
> + createDirectory: function () {
> + console.log(this);
i guess this is leftover?
> + let me = this;
> + Ext.create('PBS.window.CreateDirectory', {
> + listeners: {
> + destroy: function () {
> + me.reload();
> + },
> + },
> + }).show();
> + },
>
> - reload: function() {
> - let me = this;
> - let store = me.getView().getStore();
> - store.load();
> - store.sort();
> - },
> + removeDir: function () {
> + let me = this;
> + let view = me.getView();
> + let rec = view.getSelection();
> + let dialog = Ext.create('PBS.window.SafeDestroy', {
> + url: rec[0].data.path.replace(
> + '/mnt/datastore/',
> + '/nodes/localhost/disks/directory/'),
> + item: {type: 'Dir', id: rec[0].data.path.replace('/mnt/datastore/', '')},
that seems brittle... would it not be better to return the 'id' from the
api instead of modifying the path in the frontend?
> + note: 'Data and partitions on the disk will be left untouched.'
i think it would be better to put that text in a 'gettext' so it can be
translated
> + });
> + dialog.on('destroy', function() {
> + me.reload();
> + });
> + dialog.show();
i would here prefer the same codestyle as in createDirectory, so
Ext.create('', {
...
listeners: {
destroy: ...
},
}).show();
> + },
>
> - init: function(view) {
> - let me = this;
> - Proxmox.Utils.monStoreErrors(view, view.getStore(), true);
> - me.reload();
> - },
> - },
> + reload: function () {
> + let me = this;
> + let store = me.getView().getStore();
> + store.load();
> + store.sort();
> + },
>
> + init: function (view) {
> + let me = this;
> + Proxmox.Utils.monStoreErrors(view, view.getStore(), true);
> + me.reload();
> + },
> + },
>
> rootVisible: false,
> useArrows: true,
>
> tbar: [
> - {
> - text: gettext('Reload'),
> - iconCls: 'fa fa-refresh',
> - handler: 'reload',
> - },
> - {
> - text: gettext('Create') + ': Directory',
> - handler: 'createDirectory',
> - },
> + {
> + text: gettext('Reload'),
> + iconCls: 'fa fa-refresh',
> + handler: 'reload',
> + },
> + {
> + text: gettext('Create') + ': Directory',
> + handler: 'createDirectory',
> + },
> + {
> + xtype: 'proxmoxButton',
> + text: gettext('Remove'),
> + handler: 'removeDir',
> + disabled: true,
> + iconCls: 'fa fa-trash-o'
> + }
> ],
>
> columns: [
> - {
> - text: gettext('Path'),
> - dataIndex: 'path',
> - flex: 1,
> - },
> - {
> - header: gettext('Device'),
> - flex: 1,
> - dataIndex: 'device',
> - },
> - {
> - header: gettext('Filesystem'),
> - width: 100,
> - dataIndex: 'filesystem',
> - },
> - {
> - header: gettext('Options'),
> - width: 100,
> - dataIndex: 'options',
> - },
> - {
> - header: gettext('Unit File'),
> - hidden: true,
> - dataIndex: 'unitfile',
> - },
> + {
> + text: gettext('Path'),
> + dataIndex: 'path',
> + flex: 1,
> + },
> + {
> + header: gettext('Device'),
> + flex: 1,
> + dataIndex: 'device',
> + },
> + {
> + header: gettext('Filesystem'),
> + width: 100,
> + dataIndex: 'filesystem',
> + },
> + {
> + header: gettext('Options'),
> + width: 100,
> + dataIndex: 'options',
> + },
> + {
> + header: gettext('Unit File'),
> + hidden: true,
> + dataIndex: 'unitfile',
> + },
> ],
>
> store: {
> - fields: ['path', 'device', 'filesystem', 'options', 'unitfile'],
> - proxy: {
> - type: 'proxmox',
> - url: '/api2/json/nodes/localhost/disks/directory',
> - },
> - sorters: 'path',
> + fields: ['path', 'device', 'filesystem', 'options', 'unitfile'],
> + proxy: {
> + type: 'proxmox',
> + url: '/api2/json/nodes/localhost/disks/directory',
> + },
> + sorters: 'path',
> },
> });
> diff --git a/www/Makefile b/www/Makefile
> index edce8cb3..57db54ee 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -23,6 +23,7 @@ JSSRC= \
> window/SyncJobEdit.js \
> window/ACLEdit.js \
> window/DataStoreEdit.js \
> + window/SafeRemove.js \
> window/CreateDirectory.js \
> window/ZFSCreate.js \
> window/FileBrowser.js \
> diff --git a/www/window/SafeRemove.js b/www/window/SafeRemove.js
> new file mode 100644
> index 00000000..fdbefeae
> --- /dev/null
> +++ b/www/window/SafeRemove.js
> @@ -0,0 +1,209 @@
> +/* Popup a message window
> + * where the user has to manually enter the resource ID
> + * to enable the destroy button
> + * (mostly taken from PVE-Manager(git.proxmox.com/git/pve-manager.git))
> + */
i would rather prefer to refactor that safedestroy window into
the widget-toolkit
aside from handling the new type is there any reason
not to refactor and reuse?
i diffed over the two files and mostly
saw the different checkbox (which can be handled by showing/hiding the
correct one, or have the text and parameter as arguments)
and the note (which can also be shown/hidden by type i guess)
> +Ext.define('PBS.window.SafeDestroy', {
> + extend: 'Ext.window.Window',
> +
> + title: gettext('Confirm'),
> + modal: true,
> + buttonAlign: 'center',
> + bodyPadding: 10,
> + width: 450,
> + layout: {type: 'hbox'},
> + defaultFocus: 'confirmField',
> + showProgress: false,
> +
> + config: {
> + item: {
> + id: undefined,
> + type: undefined
> + },
> + url: undefined,
> + note: undefined,
> + params: {}
> + },
> +
> + getParams: function () {
> + var me = this;
> + var wipeCheckbox = me.lookupReference('wipeCheckbox');
> + if (wipeCheckbox.checked) {
> + me.params.wipe = 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 () {
> + var 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) {
> + var hasProgressBar = view.showProgress &&
> + response.result.data ? true : false;
> +
> + 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: 'wipe',
> + reference: 'wipeCheckbox',
> + boxLabel: gettext('Wipe'),
> + checked: false,
> + autoEl: {
> + tag: 'div',
> + 'data-qtip': gettext('Wipe disk after the removal of mountpoint')
> + }
> + },
> + {
> + xtype: 'container',
> + flex: 1,
> + layout: {
> + type: 'vbox',
> + align: 'middle'
> + },
> + height: 25,
> + items: [
> + {
> + xtype: 'component',
> + reference: 'noteCmp'
> + },
> + ]
> + },
> + ]
> + }
> + ],
> + buttons: [
> + {
> + reference: 'removeButton',
> + text: gettext('Remove'),
> + disabled: true
> + }
> + ],
> +
> + initComponent: function () {
> + var me = this;
> +
> + me.callParent();
> +
> + var item = me.getItem();
> +
> + if (!Ext.isDefined(item.id)) {
> + throw "no ID specified";
> + }
> +
> + if (!Ext.isDefined(item.type)) {
> + throw "no Disk type specified";
> + }
> +
> + var messageCmp = me.lookupReference('messageCmp');
> + var noteCmp = me.lookupReference('noteCmp');
> + var msg;
> +
> + if (item.type === 'Dir') {
> + msg = `Directory ${item.id} - Remove`
> + } else {
> + throw "unknown item type specified";
> + }
> +
> + if(me.getNote()) {
> + noteCmp.setHtml(`<small>${me.getNote()}</small>`);
> + }
> +
> + messageCmp.setHtml(msg);
> +
> + if (item.type === 'Dir') {
> + let wipeCheckbox = me.lookupReference('wipeCheckbox');
> + wipeCheckbox.setDisabled(true);
> + wipeCheckbox.setHidden(true);
> + }
> +
> + var confirmField = me.lookupReference('confirmField');
> + msg = gettext('Please enter the ID to confirm') +
> + ' (' + item.id + ')';
> + confirmField.setFieldLabel(msg);
> + }
> +});
>
prev parent reply other threads:[~2020-08-14 8:13 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-13 10:58 [pbs-devel] [PATCH proxmox-backup v3 0/2] removal of mount-units/directories Hannes Laimer
2020-08-13 10:58 ` [pbs-devel] [PATCH proxmox-backup v3 1/2] api2/node/../disks/directory: added DELETE endpoint for removal of mount-units Hannes Laimer
2020-08-14 5:38 ` [pbs-devel] applied: " Dietmar Maurer
2020-08-13 10:58 ` [pbs-devel] [PATCH proxmox-backup v3 2/2] ui/cli: added support for the " Hannes Laimer
2020-08-14 5:41 ` Dietmar Maurer
2020-08-14 8:13 ` Dominik Csapak [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9e05e98d-05a6-5370-8dcc-5b761c387f28@proxmox.com \
--to=d.csapak@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal