[pve-devel] [PATCH manager 0/7] ui: qemu/HardwareView: fix GUI permission checks and make eslint happier

[pve-devel] [PATCH manager 0/7] ui: qemu/HardwareView: fix GUI permission checks and make eslint happier

[Aaron Lauterer]

This series fixes two permission checks in the HardwareView panel:

* Add -> EFI Disk button
* Edit button for CDROM drives

to only show them as enabled if the necessary permissions are available.

Additionally it contains a few patches to make eslint happy.

Aaron Lauterer (7):
  ui: qemu/HardwareView: eslint: enforce "no-mixed-operators" rule
  ui: qemu/HardwareView: eslint: enforce "max-len" rule
  ui: qemu/HardwareView: eslint: enforce "no-useless-concat" rule
  ui: qemu/HardwareView: eslint: enforce "no-shadow" rule
  ui: qemu/HardwareView: change heuristic perms to const
  ui: qemu/HardwareView: check EFI Disk button permissions
  ui: qemu/HardwareView: add CDROM permission check to edit button

 www/manager6/qemu/HardwareView.js | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH manager 1/7] ui: qemu/HardwareView: eslint: enforce "no-mixed-operators" rule

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 116ab32f..51c77246 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -593,11 +593,11 @@ Ext.define('PVE.qemu.HardwareView', {
 
 	    var isEfi = key === 'efidisk0';
 
-	    remove_btn.setDisabled(rec.data.delete || rowdef.never_delete === true || isUnusedDisk && !diskCap);
+	    remove_btn.setDisabled(rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap));
 	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
 	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
 
-	    edit_btn.setDisabled(rec.data.delete || !rowdef.editor || isCloudInit || !isCDRom && !diskCap);
+	    edit_btn.setDisabled(rec.data.delete || !rowdef.editor || isCloudInit || (!isCDRom && !diskCap));
 
 	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
 
-- 
2.20.1

[pve-devel] applied: [PATCH manager 1/7] ui: qemu/HardwareView: eslint: enforce "no-mixed-operators" rule

[Thomas Lamprecht]

On 01.02.21 15:21, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  www/manager6/qemu/HardwareView.js | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH manager 2/7] ui: qemu/HardwareView: eslint: enforce "max-len" rule

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 51c77246..fa72d9d3 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -593,7 +593,9 @@ Ext.define('PVE.qemu.HardwareView', {
 
 	    var isEfi = key === 'efidisk0';
 
-	    remove_btn.setDisabled(rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap));
+	    remove_btn.setDisabled(rec.data.delete ||
+				   rowdef.never_delete === true ||
+				   (isUnusedDisk && !diskCap));
 	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
 	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
 
-- 
2.20.1

Re: [pve-devel] [PATCH manager 2/7] ui: qemu/HardwareView: eslint: enforce "max-len" rule

[Thomas Lamprecht]

On 01.02.21 15:21, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  www/manager6/qemu/HardwareView.js | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 51c77246..fa72d9d3 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -593,7 +593,9 @@ Ext.define('PVE.qemu.HardwareView', {
>  
>  	    var isEfi = key === 'efidisk0';
>  
> -	    remove_btn.setDisabled(rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap));
> +	    remove_btn.setDisabled(rec.data.delete ||
> +				   rowdef.never_delete === true ||
> +				   (isUnusedDisk && !diskCap));

If a method call is split over multiple lines the first line should only
be the method itself.

As we have an expression here, not really multiple parameters, either of the
following two would be fine:

remove_btn.setDisabled(
    rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap)
);

or:

remove_btn.setDisabled(
    rec.data.delete ||
    rowdef.never_delete === true ||
    (isUnusedDisk && !diskCap)
);


>  	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
>  	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
>  
> 

Re: [pve-devel] [PATCH manager 2/7] ui: qemu/HardwareView: eslint: enforce "max-len" rule

[Aaron Lauterer]

On 2/3/21 8:40 AM, Thomas Lamprecht wrote:
> On 01.02.21 15:21, Aaron Lauterer wrote:
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>>   www/manager6/qemu/HardwareView.js | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
>> index 51c77246..fa72d9d3 100644
>> --- a/www/manager6/qemu/HardwareView.js
>> +++ b/www/manager6/qemu/HardwareView.js
>> @@ -593,7 +593,9 @@ Ext.define('PVE.qemu.HardwareView', {
>>   
>>   	    var isEfi = key === 'efidisk0';
>>   
>> -	    remove_btn.setDisabled(rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap));
>> +	    remove_btn.setDisabled(rec.data.delete ||
>> +				   rowdef.never_delete === true ||
>> +				   (isUnusedDisk && !diskCap));
> 
> If a method call is split over multiple lines the first line should only
> be the method itself.
> 
> As we have an expression here, not really multiple parameters, either of the
> following two would be fine:
> 
> remove_btn.setDisabled(
>      rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap)
> );
> 
> or:
> 
> remove_btn.setDisabled(
>      rec.data.delete ||
>      rowdef.never_delete === true ||
>      (isUnusedDisk && !diskCap)
> );
> 
> 

Maybe we want to add a eslint rule to catch these? I am usually getting this wrong and having a linter rule will help to catch it early on. AFAICT the 'function-paren-newline' set to 'multiline' or 'multiline-arguments' [0] should work.


[0] https://eslint.org/docs/rules/function-paren-newline

>>   	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
>>   	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
>>   
>>
> 

Re: [pve-devel] [PATCH manager 2/7] ui: qemu/HardwareView: eslint: enforce "max-len" rule

[Aaron Lauterer]

On 2/3/21 10:50 AM, Aaron Lauterer wrote:
> 
> 
> On 2/3/21 8:40 AM, Thomas Lamprecht wrote:
>> On 01.02.21 15:21, Aaron Lauterer wrote:
>>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>>> ---
>>>   www/manager6/qemu/HardwareView.js | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
>>> index 51c77246..fa72d9d3 100644
>>> --- a/www/manager6/qemu/HardwareView.js
>>> +++ b/www/manager6/qemu/HardwareView.js
>>> @@ -593,7 +593,9 @@ Ext.define('PVE.qemu.HardwareView', {
>>>           var isEfi = key === 'efidisk0';
>>> -        remove_btn.setDisabled(rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap));
>>> +        remove_btn.setDisabled(rec.data.delete ||
>>> +                   rowdef.never_delete === true ||
>>> +                   (isUnusedDisk && !diskCap));
>>
>> If a method call is split over multiple lines the first line should only
>> be the method itself.
>>
>> As we have an expression here, not really multiple parameters, either of the
>> following two would be fine:
>>
>> remove_btn.setDisabled(
>>      rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap)
>> );
>>
>> or:
>>
>> remove_btn.setDisabled(
>>      rec.data.delete ||
>>      rowdef.never_delete === true ||
>>      (isUnusedDisk && !diskCap)
>> );
>>
>>
> 
> Maybe we want to add a eslint rule to catch these? I am usually getting this wrong and having a linter rule will help to catch it early on. AFAICT the 'function-paren-newline' set to 'multiline' or 'multiline-arguments' [0] should work.
> 
> 
> [0] https://eslint.org/docs/rules/function-paren-newline

Of course, this won't work in this use case as they are not parameters. I'll keep looking if there is a rule for this situation that could help.

> 
>>>           remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
>>>           remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
>>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

[pve-devel] [PATCH manager 3/7] ui: qemu/HardwareView: eslint: enforce "no-useless-concat" rule

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index fa72d9d3..cc707a2a 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -609,7 +609,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	};
 
 	Ext.apply(me, {
-	    url: '/api2/json/' + 'nodes/' + nodename + '/qemu/' + vmid + '/pending',
+	    url: '/api2/json/nodes/' + nodename + '/qemu/' + vmid + '/pending',
 	    interval: 5000,
 	    selModel: sm,
 	    run_editor: run_editor,
-- 
2.20.1

Re: [pve-devel] [PATCH manager 3/7] ui: qemu/HardwareView: eslint: enforce "no-useless-concat" rule

[Dominik Csapak]

i'd prefer we use template strings if we touch this line
this could be

`/api2/json/nodes/${nodename}/qemu/${vmid}/pending`

On 2/1/21 3:21 PM, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>   www/manager6/qemu/HardwareView.js | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index fa72d9d3..cc707a2a 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -609,7 +609,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   	};
>   
>   	Ext.apply(me, {
> -	    url: '/api2/json/' + 'nodes/' + nodename + '/qemu/' + vmid + '/pending',
> +	    url: '/api2/json/nodes/' + nodename + '/qemu/' + vmid + '/pending',
>   	    interval: 5000,
>   	    selModel: sm,
>   	    run_editor: run_editor,
> 

Re: [pve-devel] [PATCH manager 3/7] ui: qemu/HardwareView: eslint: enforce "no-useless-concat" rule

[Thomas Lamprecht]

On 02.02.21 14:06, Dominik Csapak wrote:
> i'd prefer we use template strings if we touch this line
> this could be
> 
> `/api2/json/nodes/${nodename}/qemu/${vmid}/pending`

+1

[pve-devel] [PATCH manager 4/7] ui: qemu/HardwareView: eslint: enforce "no-shadow" rule

[Aaron Lauterer]

`confid` is overwritten in each step anyways, so it should be no
problem to use it in the outer scope.

Let's play it safe for `sm` and rename it in the function.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

Not sure if my solutions are okay or if another approach is preferable.
Didn't find any patches in the manager repo that deal with the
`no-shadow` rule as a guide though.

 www/manager6/qemu/HardwareView.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index cc707a2a..213a946f 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -220,7 +220,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	};
 
 	PVE.Utils.forEachBus(undefined, function(type, id) {
-	    var confid = type + id;
+	    confid = type + id;
 	    rows[confid] = {
 		group: 10,
 		iconCls: 'hdd-o',
@@ -531,8 +531,8 @@ Ext.define('PVE.qemu.HardwareView', {
 	let isAtLimit = (type) => counts[type] >= PVE.Utils.hardware_counts[type];
 
 	var set_button_status = function() {
-	    var sm = me.getSelectionModel();
-	    var rec = sm.getSelection()[0];
+	    var selection_model = me.getSelectionModel();
+	    var rec = selection_model.getSelection()[0];
 
 	    // en/disable hardwarebuttons
 	    counts = {};
-- 
2.20.1

Re: [pve-devel] [PATCH manager 4/7] ui: qemu/HardwareView: eslint: enforce "no-shadow" rule

[Dominik Csapak]

the global confid variable is only there
so we could reuse the name

so i'd prefer we remove that
and use 'let confid' in the relevant blocks
(with var this would have failed)

On 2/1/21 3:21 PM, Aaron Lauterer wrote:
> `confid` is overwritten in each step anyways, so it should be no
> problem to use it in the outer scope.
> 
> Let's play it safe for `sm` and rename it in the function.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> Not sure if my solutions are okay or if another approach is preferable.
> Didn't find any patches in the manager repo that deal with the
> `no-shadow` rule as a guide though.
> 
>   www/manager6/qemu/HardwareView.js | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index cc707a2a..213a946f 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -220,7 +220,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   	};
>   
>   	PVE.Utils.forEachBus(undefined, function(type, id) {
> -	    var confid = type + id;
> +	    confid = type + id;
>   	    rows[confid] = {
>   		group: 10,
>   		iconCls: 'hdd-o',
> @@ -531,8 +531,8 @@ Ext.define('PVE.qemu.HardwareView', {
>   	let isAtLimit = (type) => counts[type] >= PVE.Utils.hardware_counts[type];
>   
>   	var set_button_status = function() {
> -	    var sm = me.getSelectionModel();
> -	    var rec = sm.getSelection()[0];
> +	    var selection_model = me.getSelectionModel();
> +	    var rec = selection_model.getSelection()[0];
>   
>   	    // en/disable hardwarebuttons
>   	    counts = {};
> 

Re: [pve-devel] [PATCH manager 4/7] ui: qemu/HardwareView: eslint: enforce "no-shadow" rule

[Thomas Lamprecht]

On 02.02.21 14:07, Dominik Csapak wrote:
> the global confid variable is only there
> so we could reuse the name
> 
> so i'd prefer we remove that
> and use 'let confid' in the relevant blocks
> (with var this would have failed)

+1

It's general also really good to check the variables and transform all to `let`or
`const` where possible.
The current code base big use of `var` stems from the pre ES6 time where neither
`let` nor `const` existed.

I added a section to our style guide to document that:
https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables

[pve-devel] [PATCH manager 5/7] ui: qemu/HardwareView: change heuristic perms to const

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 213a946f..80d6eec1 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -558,9 +558,9 @@ Ext.define('PVE.qemu.HardwareView', {
 	    });
 
 	    // heuristic only for disabling some stuff, the backend has the final word.
-	    var noSysConsolePerm = !caps.nodes['Sys.Console'];
-	    var noVMConfigHWTypePerm = !caps.vms['VM.Config.HWType'];
-	    var noVMConfigNetPerm = !caps.vms['VM.Config.Network'];
+	    const noSysConsolePerm = !caps.nodes['Sys.Console'];
+	    const noVMConfigHWTypePerm = !caps.vms['VM.Config.HWType'];
+	    const noVMConfigNetPerm = !caps.vms['VM.Config.Network'];
 
 
 	    me.down('#addusb').setDisabled(noSysConsolePerm || isAtLimit('usb'));
-- 
2.20.1

[pve-devel] applied: [PATCH manager 5/7] ui: qemu/HardwareView: change heuristic perms to const

[Thomas Lamprecht]

On 01.02.21 15:21, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  www/manager6/qemu/HardwareView.js | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH manager 6/7] ui: qemu/HardwareView: check EFI Disk button permissions

[Aaron Lauterer]

Make sure that the `Add EFI Disk` button is disabled if the user does
not have the needed permissions.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 80d6eec1..252a8e72 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -561,6 +561,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    const noSysConsolePerm = !caps.nodes['Sys.Console'];
 	    const noVMConfigHWTypePerm = !caps.vms['VM.Config.HWType'];
 	    const noVMConfigNetPerm = !caps.vms['VM.Config.Network'];
+	    const noVMConfigDiskPerm = !caps.vms['VM.Config.Disk'];
 
 
 	    me.down('#addusb').setDisabled(noSysConsolePerm || isAtLimit('usb'));
@@ -569,7 +570,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    me.down('#addserial').setDisabled(noVMConfigHWTypePerm || isAtLimit('serial'));
 	    me.down('#addnet').setDisabled(noVMConfigNetPerm || isAtLimit('net'));
 	    me.down('#addrng').setDisabled(noSysConsolePerm || isAtLimit('rng'));
-	    efidisk_menuitem.setDisabled(isAtLimit('efidisk'));
+	    efidisk_menuitem.setDisabled(noVMConfigDiskPerm || isAtLimit('efidisk'));
 	    me.down('#addci').setDisabled(noSysConsolePerm || hasCloudInit);
 
 	    if (!rec) {
-- 
2.20.1

[pve-devel] applied: [PATCH manager 6/7] ui: qemu/HardwareView: check EFI Disk button permissions

[Thomas Lamprecht]

On 01.02.21 15:21, Aaron Lauterer wrote:
> Make sure that the `Add EFI Disk` button is disabled if the user does
> not have the needed permissions.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  www/manager6/qemu/HardwareView.js | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [PATCH manager 7/7] ui: qemu/HardwareView: add CDROM permission check to edit button

[Aaron Lauterer]

Add CDROM permission check to disable the Edit button if they are not
present.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 252a8e72..56bdc0a1 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -600,7 +600,11 @@ Ext.define('PVE.qemu.HardwareView', {
 	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
 	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
 
-	    edit_btn.setDisabled(rec.data.delete || !rowdef.editor || isCloudInit || (!isCDRom && !diskCap));
+	    edit_btn.setDisabled(rec.data.delete ||
+				 !rowdef.editor ||
+				 isCloudInit ||
+				 !caps.vms['VM.Config.CDROM'] ||
+				 (!isCDRom && !diskCap));
 
 	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
 
-- 
2.20.1

Re: [pve-devel] [PATCH manager 7/7] ui: qemu/HardwareView: add CDROM permission check to edit button

[Dominik Csapak]

well this is hardly readable anymore... (not your fault)
but, would it now not be disabled if i have no CDROM perms
even if it is a disk and i have perms for that,
because of short-circuiting ?

On 2/1/21 3:21 PM, Aaron Lauterer wrote:
> Add CDROM permission check to disable the Edit button if they are not
> present.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>   www/manager6/qemu/HardwareView.js | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 252a8e72..56bdc0a1 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -600,7 +600,11 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
>   	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
>   
> -	    edit_btn.setDisabled(rec.data.delete || !rowdef.editor || isCloudInit || (!isCDRom && !diskCap));
> +	    edit_btn.setDisabled(rec.data.delete ||
> +				 !rowdef.editor ||
> +				 isCloudInit ||
> +				 !caps.vms['VM.Config.CDROM'] ||
> +				 (!isCDRom && !diskCap));
>   
>   	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
>   
> 

Re: [pve-devel] [PATCH manager 7/7] ui: qemu/HardwareView: add CDROM permission check to edit button

[Aaron Lauterer]

On 2/2/21 2:13 PM, Dominik Csapak wrote:
> well this is hardly readable anymore... (not your fault)
> but, would it now not be disabled if i have no CDROM perms
> even if it is a disk and i have perms for that,
> because of short-circuiting ?

Oh yeah, thanks for catching that. One more comment inline


> 
> On 2/1/21 3:21 PM, Aaron Lauterer wrote:
>> Add CDROM permission check to disable the Edit button if they are not
>> present.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>>   www/manager6/qemu/HardwareView.js | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
>> index 252a8e72..56bdc0a1 100644
>> --- a/www/manager6/qemu/HardwareView.js
>> +++ b/www/manager6/qemu/HardwareView.js
>> @@ -600,7 +600,11 @@ Ext.define('PVE.qemu.HardwareView', {
>>           remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
>>           remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
>> -        edit_btn.setDisabled(rec.data.delete || !rowdef.editor || isCloudInit || (!isCDRom && !diskCap));
>> +        edit_btn.setDisabled(rec.data.delete ||
>> +                 !rowdef.editor ||
>> +                 isCloudInit ||
>> +                 !caps.vms['VM.Config.CDROM'] ||

It doesn't really help readability but should work if we additionally check if the current selection is a cdrom. This should prevent short-circuiting in that case

			(isCDRom && !caps.vms['VM.Config.CDROM'])) ||


>> +                 (!isCDRom && !diskCap));
>>           resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH manager 7/7] ui: qemu/HardwareView: add CDROM permission check to edit button

[Thomas Lamprecht]

On 02.02.21 14:41, Aaron Lauterer wrote:
> 
> 
> On 2/2/21 2:13 PM, Dominik Csapak wrote:
>> well this is hardly readable anymore... (not your fault)
>> but, would it now not be disabled if i have no CDROM perms
>> even if it is a disk and i have perms for that,
>> because of short-circuiting ?
> 
> Oh yeah, thanks for catching that. One more comment inline
> 
> 
>>
>> On 2/1/21 3:21 PM, Aaron Lauterer wrote:
>>> Add CDROM permission check to disable the Edit button if they are not
>>> present.
>>>
>>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>>> ---
>>>   www/manager6/qemu/HardwareView.js | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
>>> index 252a8e72..56bdc0a1 100644
>>> --- a/www/manager6/qemu/HardwareView.js
>>> +++ b/www/manager6/qemu/HardwareView.js
>>> @@ -600,7 +600,11 @@ Ext.define('PVE.qemu.HardwareView', {
>>>           remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
>>>           remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
>>> -        edit_btn.setDisabled(rec.data.delete || !rowdef.editor || isCloudInit || (!isCDRom && !diskCap));
>>> +        edit_btn.setDisabled(rec.data.delete ||
>>> +                 !rowdef.editor ||
>>> +                 isCloudInit ||
>>> +                 !caps.vms['VM.Config.CDROM'] ||
> 
> It doesn't really help readability but should work if we additionally check if the current selection is a cdrom. This should prevent short-circuiting in that case
> 
>             (isCDRom && !caps.vms['VM.Config.CDROM'])) ||

please also split all lines consistently, i.e., if this would stay as is then it should
be written as

edit_btn.setDisabled(
    rec.data.delete ||
    !rowdef.editor ||
    isCloudInit ||
    !caps.vms['VM.Config.CDROM'] ||
    (!isCDRom && !diskCap)
);

less indentation but also less crowded (as method call and first argument aren't glued together)