[pve-devel] [PATCH manager] ui: improve vm/container migration user experience

[pve-devel] [PATCH manager] ui: improve vm/container migration user experience

[Philipp Hufnagl]

After the implementation of fix #474, it has been suggested that
instead of requiring the user to click a checkbox allowing migration,
it should be allowed automatically and and a warning should be displayed

Further it has been discussed to rename the feature from "transfer" to
"migrate". However and API change would break already implemented usage
and so it has been decided to call it (for now) transfer everywhere to
avoid confusion

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 www/manager6/grid/PoolMembers.js | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 224daca3..d6fa0278 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -35,6 +35,20 @@ Ext.define('PVE.pool.AddVM', {
 	    ],
 	});
 
+	let transferWarning = Ext.create('Ext.form.field.Display', {
+	    userCls: 'pmx-hint',
+	    value: gettext('One or more vms or container will be removed from their old pool'),
+	    hidden: true,
+	});
+
+	let transfer = Ext.create('Ext.form.field.Checkbox', {
+	    name: 'transfer',
+	    boxLabel: gettext('Allow Transfer'),
+	    inputValue: 1,
+	    hidden: true,
+	    value: 0,
+	});
+
 	var vmGrid = Ext.create('widget.grid', {
 	    store: vmStore,
 	    border: true,
@@ -46,9 +60,15 @@ Ext.define('PVE.pool.AddVM', {
 		listeners: {
 		    selectionchange: function(model, selected, opts) {
 			var selectedVms = [];
+			var isTransfer = false;
 			selected.forEach(function(vm) {
 			    selectedVms.push(vm.data.vmid);
+			    if (vm.data.pool !== '') {
+				isTransfer = true;
+			    }
 			});
+			transfer.setValue(isTransfer);
+			transferWarning.setHidden(!isTransfer);
 			vmsField.setValue(selectedVms);
 		    },
 		},
@@ -90,15 +110,10 @@ Ext.define('PVE.pool.AddVM', {
 	    ],
 	});
 
-	let transfer = Ext.create('Ext.form.field.Checkbox', {
-	    name: 'transfer',
-	    boxLabel: gettext('Allow Transfer'),
-	    inputValue: 1,
-	    value: 0,
-	});
+
 	Ext.apply(me, {
 	    subject: gettext('Virtual Machine'),
-	    items: [vmsField, vmGrid, transfer],
+	    items: [vmsField, vmGrid, transferWarning, transfer],
 	});
 
 	me.callParent();
-- 
2.39.2

Re: [pve-devel] [PATCH manager] ui: improve vm/container migration user experience

[Thomas Lamprecht]

subject is quite confusing, this isn't for what's commonly understood
w.r.t. the "VM/CT migration", i.e., live migration to another host.

You at least need to use the word "pool" somewhere..

Am 05/09/2023 um 15:51 schrieb Philipp Hufnagl:
> After the implementation of fix #474, it has been suggested that
> instead of requiring the user to click a checkbox allowing migration,
> it should be allowed automatically and and a warning should be displayed
> 
> Further it has been discussed to rename the feature from "transfer" to
> "migrate". However and API change would break already implemented usage

Not sure if I'd call that a discussion, I stated:

> We normally use "move" or "migrate", not "transfer", or "reassign" (like for
> moving a guest disk to another guest) and it has some merits to not expand the
> commonly used (parameter) naming scheme to much, but oh well it's already released
> and a naming nit that doesn't matters _that_ much.

And overlooked the "renaming to "migrate" or "migrate from other pool" part
in your reply (while at it, still missing the adding the API default).

This was not intended to be a final instruction to definitively use "migrate"
here, but rather open the discussion for a possible better name, or even keep
that if there's consensus that "transfer" or "reassign" is fine.

> and so it has been decided to call it (for now) transfer everywhere to
> avoid confusion

That again conflicts with your subject message and your statement that
you will rename it. Besides that, by whom has it been decided where?
This again sounds like there was some actual discussion, but I don't
see any.

If you want to keep transfer for now, ok, but do so consistently (e.g., also
in the commit message's subject here then – but still with "pool" mentioned,
otherwise one cannot possible know what this is actually about..)

> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  www/manager6/grid/PoolMembers.js | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
> index 224daca3..d6fa0278 100644
> --- a/www/manager6/grid/PoolMembers.js
> +++ b/www/manager6/grid/PoolMembers.js
> @@ -35,6 +35,20 @@ Ext.define('PVE.pool.AddVM', {
>  	    ],
>  	});
>  
> +	let transferWarning = Ext.create('Ext.form.field.Display', {
> +	    userCls: 'pmx-hint',
> +	    value: gettext('One or more vms or container will be removed from their old pool'),

if you use the word "vms" you'd need to write it as "VMs", but rather:

gettext('Virtual guests are removed from current pools'),

> +	    hidden: true,
> +	});
> +
> +	let transfer = Ext.create('Ext.form.field.Checkbox', {
> +	    name: 'transfer',
> +	    boxLabel: gettext('Allow Transfer'),
> +	    inputValue: 1,
> +	    hidden: true,
> +	    value: 0,
> +	});

no need for this, just add:

extraRequestParams: {
     transfer: 1,
}

to the edit window config.

> +
>  	var vmGrid = Ext.create('widget.grid', {
>  	    store: vmStore,
>  	    border: true,
> @@ -46,9 +60,15 @@ Ext.define('PVE.pool.AddVM', {
>  		listeners: {
>  		    selectionchange: function(model, selected, opts) {
>  			var selectedVms = [];
> +			var isTransfer = false;
>  			selected.forEach(function(vm) {
>  			    selectedVms.push(vm.data.vmid);
> +			    if (vm.data.pool !== '') {

I'd prefer:

if (vm.data.pool?.length) {

as !== '' seems rather brittle.

> +				isTransfer = true;
> +			    }
>  			});
> +			transfer.setValue(isTransfer);
> +			transferWarning.setHidden(!isTransfer);
>  			vmsField.setValue(selectedVms);
>  		    },
>  		},
> @@ -90,15 +110,10 @@ Ext.define('PVE.pool.AddVM', {
>  	    ],
>  	});
>  
> -	let transfer = Ext.create('Ext.form.field.Checkbox', {
> -	    name: 'transfer',
> -	    boxLabel: gettext('Allow Transfer'),
> -	    inputValue: 1,
> -	    value: 0,
> -	});
> +

adding extra empty line here

>  	Ext.apply(me, {
>  	    subject: gettext('Virtual Machine'),
> -	    items: [vmsField, vmGrid, transfer],
> +	    items: [vmsField, vmGrid, transferWarning, transfer],
>  	});
>  
>  	me.callParent();

Re: [pve-devel] [PATCH manager] ui: improve vm/container migration user experience

[Philipp Hufnagl]

On 9/6/23 11:18, Thomas Lamprecht wrote:

> subject is quite confusing, this isn't for what's commonly understood
> w.r.t. the "VM/CT migration", i.e., live migration to another host.
>
> You at least need to use the word "pool" somewhere..
>
> Am 05/09/2023 um 15:51 schrieb Philipp Hufnagl:
>> After the implementation of fix #474, it has been suggested that
>> instead of requiring the user to click a checkbox allowing migration,
>> it should be allowed automatically and and a warning should be displayed
>>
>> Further it has been discussed to rename the feature from "transfer" to
>> "migrate". However and API change would break already implemented usage
> Not sure if I'd call that a discussion, I stated:
>
>> We normally use "move" or "migrate", not "transfer", or "reassign" (like for
>> moving a guest disk to another guest) and it has some merits to not expand the
>> commonly used (parameter) naming scheme to much, but oh well it's already released
>> and a naming nit that doesn't matters _that_ much.
> And overlooked the "renaming to "migrate" or "migrate from other pool" part
> in your reply (while at it, still missing the adding the API default).
>
> This was not intended to be a final instruction to definitively use "migrate"
> here, but rather open the discussion for a possible better name, or even keep
> that if there's consensus that "transfer" or "reassign" is fine.
>
>> and so it has been decided to call it (for now) transfer everywhere to
>> avoid confusion
> That again conflicts with your subject message and your statement that
> you will rename it. Besides that, by whom has it been decided where?
> This again sounds like there was some actual discussion, but I don't
> see any.
>
> If you want to keep transfer for now, ok, but do so consistently (e.g., also
> in the commit message's subject here then – but still with "pool" mentioned,
> otherwise one cannot possible know what this is actually about..)

I spoke with Dominik Csapak of list about this. "transfer" can not 
simply be removed from the API since it would break programs who would 
use it. Therefor we agreed, that renaming would just lead lead to a 
confusing mix of terms.

I thought it would be better to call it "migration" in the patch but I 
will call it "transfer" there as well and also mention "pool"