Re: [pve-devel] [PATCH v3 pve-manager 57/66] ui: allow to configure notification event -> target mapping

[Lukas Wagner <l.wagner@proxmox.com>]

Hi,

On 7/19/23 14:45, Dominik Csapak wrote:
>> +
>> +Ext.define('PVE.dc.NotificationEventsTargetSelector', {
>> +    alias: ['widget.pveNotificationEventsTargetSelector'],
>> +    extend: 'PVE.form.NotificationTargetSelector',
>> +    fieldLabel: gettext('Notification Target'),
>> +    allowBlank: true,
>> +    editable: true,
>> +    autoSelect: false,
>> +    deleteEmpty: false,
>> +    emptyText: `${Proxmox.Utils.defaultText} (${gettext("mail-to-root")})`,
>> +});
>> +
>> +Ext.define('PVE.dc.NotificationEvents', {
>> +    extend: 'Proxmox.grid.ObjectGrid',
>> +    alias: ['widget.pveNotificationEvents'],
>> +
>> +    // Taken from OptionView.js, but adapted slightly.
> 
> what exactly was adapted? i can ofc diff it myself, but it would
> be nicer to have that info either in a comment or the commit message.
> also we should factor this out and reuse it in OptionView and here?
> maybe just adding it to the ObjectGrid itself?
> (if possible)

I added some more comments to explain what has been changed. However I'm not sure
if there is an easy way to reuse the changes anywhere. Basically the changes were needed
because I'm trying to render multiple rows in the ObjectGrid from a single
key in datacenter.cfg (notify), which contains the target/policy settings.

> 
>> +    addInputPanelRow: function(name, propertyName, text, opts) {
>> +    let me = this;
>> +
>> +    opts = opts || {};
(...)
> 
> you should be able to reuse the render_value if you add this there also
> it can't trigger for the others anyway?
> 

Originally, I did not reuse render_value because `package-updates` has a different default,
`auto` instead of `always`. However, with an additional parameter for `render_value` I was
able to consolidate both variants.

>> +            break;
>> +            case 'never':
>> +            template = gettext('Never');
>> +            break;
>> +            default:
>> +            template = gettext('{1} (Automatically), notify via target \'{0}\'');
>> +            break;
>> +        }
>> +
>> +        return Ext.String.format(template, target, Proxmox.Utils.defaultText);
>> +        },
>> +        url: "/api2/extjs/cluster/options",
>> +        items: [
>> +        {
>> +            xtype: 'pveNotificationEventsPolicySelector',
> 
> as said above i'd simply make this a KVComboBox to indicate it's
> basically a seperate component

I actually decided to keep the separate component due to some other changes,
mainly the addition of a custom listener that shows/hides the warning textfield.
That would have led to a lot of code duplication that is now avoided by the
component.
However, I decided to move the `comboItems` to where the component is actually used.
> 
>> +            name: 'package-updates',
>> +            fieldLabel: gettext('Notify'),
>> +            comboItems: [
>> +            ['__default__', Proxmox.Utils.defaultText + ' (auto)'],
>> +            ['auto', gettext('Automatically')],
>> +            ['always', gettext('Always')],
>> +            ['never', gettext('Never')],
>> +            ],
>> +        },
>> +        {
>> +            xtype: 'pveNotificationEventsTargetSelector',
>> +            name: 'target-package-updates',
>> +        },
>> +        ],
>> +    });
>> +
>> +    // Hack: Also load the notify property to make it accessible
>> +    // for our render functions. initComponents later hides it.
>> +    me.add_text_row('notify', gettext('Notify'), {});
> 
> it should be possible to simply add it directly here with something like:
> 
> me.rows.notify = {
>      visible: false,
> };
> 
>> 
> we e.g. do something like this in dc/Optionsview and qemu/HardwareView
> (the latter is a PendingObjectGrid, but still inherits from ObjectGrid)

Thanks, that did work rather nicely. It also fixed a graphical glitch where the 'notify' row
would be visible for a split second after a page refresh.


-- 
- Lukas