[pve-devel] [PATCH pve-manager] fixes #1503 Add role CRUD to the GUI.

[Thomas Lamprecht]

Am 05/08/2018 um 09:22 AM schrieb Dominik Csapak:
> looks good in general, a few comments inline (most are nitpicks)
> 
> On 05/07/2018 05:41 PM, René Jochum wrote:
>> As given in the subject this implements role create/update/delete over
>> the manager.
>>
>> There's currently no coler highlightning for "special" roles.
>>
>> I decided to call "special" roles "Builtin" any feedback on that is much
>> appreciated.

I like Builtin, is more expressive about what they really are, as they
ain't that special they're just, as you say, builtin, so good 
suggestion! Maybe use 'Built-In', seems more used doing a quick internet 
search, but I'm actually OK with both.

>>
>>[snip]
>> @@ -58,8 +90,34 @@ Ext.define('PVE.dc.RoleView', {
>>           listeners: {
>>           activate: function() {
>>               store.load();
>> +        },
>> +        itemdblclick: run_editor,
>> +        },
>> +        tbar: [
>> +        {
>> +            text: gettext('Create'),
>> +            handler: function() {
>> +            var win = Ext.create('PVE.dc.GroupEdit', {});
> 
> this should be 'PVE.dc.RoleEdit'

Was wondering why nothing popped up after creation 'til I saw
Create: Group in the window title ^^

> 
>> +            win.on('destroy', reload);
>> +            win.show();
>> +            }
>> +        },
>> +        {
>> +            xtype: 'proxmoxButton',
>> +            text: gettext('Edit'),
>> +            disabled: true,
>> +            selModel: sm,
>> +            handler: run_editor
> 
> for this we probably want an 'enableFn' so that we only enable the edit 
> button on non builtin entries, like this:
> 
> enableFn: function(record) {
>      return record.data.special !== '1';
> }

Same for the Remove button below, please.
We cannot remove builtin roles, so there it should be disabled.

Oh, and personally it would make the creation window wider, I get
scrollbars here and it looks a bit stuffed once multiple permissions are
selected, I know that it won't fix it entirely as they will be always
cut off, but now it looks just too stuffed, IMO.

> 
>> +        },
>> +        {
>> +            xtype: 'proxmoxStdRemoveButton',
>> +            selModel: sm,
>> +            callback: function() {
>> +            reload();
>> +            },
>> +            baseurl: '/access/roles/'
>>           }
>> -        }
>> +        ]
>>       });
>>       me.callParent();