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

[Dominik Csapak <d.csapak@proxmox.com>]

some comments inline:

On 7/17/23 17:00, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>   www/manager6/Makefile                 |   1 +
>   www/manager6/dc/Config.js             |  12 ++
>   www/manager6/dc/NotificationEvents.js | 238 ++++++++++++++++++++++++++
>   3 files changed, 251 insertions(+)
>   create mode 100644 www/manager6/dc/NotificationEvents.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 140b20f0..452abbd4 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -158,6 +158,7 @@ JSSRC= 							\
>   	dc/Health.js					\
>   	dc/Log.js					\
>   	dc/NodeView.js					\
> +	dc/NotificationEvents.js			\
>   	dc/OptionView.js				\
>   	dc/PermissionView.js				\
>   	dc/PoolEdit.js					\
> diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
> index 04ed04f0..aa025c8d 100644
> --- a/www/manager6/dc/Config.js
> +++ b/www/manager6/dc/Config.js
> @@ -317,6 +317,18 @@ Ext.define('PVE.dc.Config', {
>   	    );
>   	}
>   
> +	if (caps.dc['Sys.Audit']) {
> +	    me.items.push(
> +		{
> +		    xtype: 'pveNotificationEvents',
> +		    title: gettext('Notifications'),
> +		    onlineHelp: 'notification_events',
> +		    iconCls: 'fa fa-bell-o',
> +		    itemId: 'notifications',
> +		},
> +	    );
> +	}
> +
>   	if (caps.dc['Sys.Audit']) {
>   	    me.items.push({
>   		xtype: 'pveDcSupport',
> diff --git a/www/manager6/dc/NotificationEvents.js b/www/manager6/dc/NotificationEvents.js
> new file mode 100644
> index 00000000..8ba0a844
> --- /dev/null
> +++ b/www/manager6/dc/NotificationEvents.js
> @@ -0,0 +1,238 @@
> +Ext.define('PVE.dc.NotificationEventsPolicySelector', {
> +    alias: ['widget.pveNotificationEventsPolicySelector'],
> +    extend: 'Proxmox.form.KVComboBox',
> +    deleteEmpty: false,
> +    value: '__default__',
> +    comboItems: [
> +	['__default__', `${Proxmox.Utils.defaultText} (always)`],
> +	['always', gettext('Always')],
> +	['never', gettext('Never')],
> +    ],
> +    defaultValue: '__default__',
> +});

mhmm.. are we sure all future things that notify have 'always' as default?
maybe we should make that text configurable?

also the third time you use this, you basically change the essence of it,
namely the options to choose from (i'd argue that should be a plain
KVComboBox there, you only save 3 lines, but there is no confusion that
it's a seperate selector anymore)


> +
> +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)

the code looks generic enough to be useful there

> +    addInputPanelRow: function(name, propertyName, text, opts) {
> +	let me = this;
> +
> +	opts = opts || {};
> +	me.rows = me.rows || {};
> +
> +	me.rows[name] = {
> +	    required: true,
> +	    defaultValue: opts.defaultValue,
> +	    header: text,
> +	    renderer: opts.renderer,
> +	    name: propertyName,
> +	    editor: {
> +		xtype: 'proxmoxWindowEdit',
> +		width: opts.width || 400,
> +		subject: text,
> +		onlineHelp: opts.onlineHelp,
> +		fieldDefaults: {
> +		    labelWidth: opts.labelWidth || 150,
> +		},
> +		setValues: function(values) {
> +		    let value = values[propertyName];
> +
> +		    if (opts.parseBeforeSet) {
> +			value = PVE.Parser.parsePropertyString(value);
> +		    }
> +
> +		    Ext.Array.each(this.query('inputpanel'), function(panel) {
> +			panel.setValues(value);
> +			panel.originalValue = {
> +			    ...value,
> +			};
> +		    });
> +		},
> +		url: opts.url,
> +		items: [{
> +		    xtype: 'inputpanel',
> +		    onGetValues: function(values) {
> +			let fields = this.config.items.map(field => field.name).filter(n => n);
> +
> +			for (const [key, value] of Object.entries(this.originalValue)) {
> +			    if (!fields.includes(key)) {
> +				values[key] = value;
> +			    }
> +			}
> +
> +			let value = {};
> +			if (Object.keys(values).length > 0) {
> +			    value[propertyName] = PVE.Parser.printPropertyString(values);
> +			} else {
> +			    Proxmox.Utils.assemble_field_data(value, { 'delete': propertyName });
> +			}
> +
> +			return value;
> +		    },
> +		    items: opts.items,
> +		}],
> +	    },
> +	};
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	// Helper function for rendering the property
> +	// Needed since the actual value is always stored in the 'notify' property
> +	let render_value = (store, target_key, mode_key) => {
> +	    let value = store.getById('notify')?.get('value') ?? {};
> +	    let target = value[target_key] ?? gettext('mail-to-root');
> +	    let template;
> +
> +	    switch (value[mode_key]) {
> +		case 'always':
> +		    template = gettext('Always, notify via target \'{0}\'');
> +		    break;
> +		case 'never':
> +		    template = gettext('Never');
> +		    break;
> +		default:
> +		    template = gettext('{1} (Always), notify via target \'{0}\'');
> +		    break;
> +	    }
> +
> +	    return Ext.String.format(template, target, Proxmox.Utils.defaultText);
> +	};
> +
> +	me.addInputPanelRow('fencing', 'notify', gettext('Node Fencing'), {
> +	    renderer: (value, metaData, record, rowIndex, colIndex, store) =>
> +		render_value(store, 'target-fencing', 'fencing'),
> +	    url: "/api2/extjs/cluster/options",
> +	    items: [
> +		{
> +		    xtype: 'pveNotificationEventsPolicySelector',
> +		    name: 'fencing',
> +		    fieldLabel: gettext('Notify'),
> +		},
> +		{
> +		    xtype: 'pveNotificationEventsTargetSelector',
> +		    name: 'target-fencing',
> +		},
> +		{
> +		    xtype: 'displayfield',
> +		    userCls: 'pmx-hint',
> +		    value: gettext('Disabling notifications is not ' +
> +			'recommended for production systems!'),
> +		},
> +	    ],
> +	});
> +
> +	me.addInputPanelRow('replication', 'notify', gettext('Replication'), {
> +	    renderer: (value, metaData, record, rowIndex, colIndex, store) =>
> +		render_value(store, 'target-replication', 'replication'),
> +	    url: "/api2/extjs/cluster/options",
> +	    items: [
> +		{
> +		    xtype: 'pveNotificationEventsPolicySelector',
> +		    name: 'replication',
> +		    fieldLabel: gettext('Notify'),
> +		},
> +		{
> +		    xtype: 'pveNotificationEventsTargetSelector',
> +		    name: 'target-replication',
> +		},
> +		{
> +		    xtype: 'displayfield',
> +		    userCls: 'pmx-hint',
> +		    value: gettext('Disabling notifications is not ' +
> +			'recommended for production systems!'),
> +		},
> +	    ],
> +	});
> +
> +	me.addInputPanelRow('updates', 'notify', gettext('Package Updates'), {
> +	    renderer: (value, metaData, record, rowIndex, colIndex, store) => {
> +		value = store.getById('notify')?.get('value') ?? {};
> +		let target = value['target-package-updates'] ?? gettext('mail-to-root');
> +		let template;
> +
> +		switch (value['package-updates']) {
> +		    case 'always':
> +			template = gettext('Always, notify via \'{0}\'');
> +			break;
> +		    case 'auto':
> +			template = gettext('Automatically, notify via target \'{0}\'');

you should be able to reuse the render_value if you add this there also
it can't trigger for the others anyway?

> +			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

> +		    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)

> +
> +	me.selModel = Ext.create('Ext.selection.RowModel', {});
> +
> +	Ext.apply(me, {
> +	    tbar: [{
> +		text: gettext('Edit'),
> +		xtype: 'proxmoxButton',
> +		disabled: true,
> +		handler: () => me.run_editor(),
> +		selModel: me.selModel,
> +	    }],
> +	    url: "/api2/json/cluster/options",
> +	    editorConfig: {
> +		url: "/api2/extjs/cluster/options",
> +	    },
> +	    interval: 5000,
> +	    cwidth1: 200,
> +	    listeners: {
> +		itemdblclick: me.run_editor,
> +	    },
> +	});
> +
> +	me.callParent();
> +
> +	me.rows.notify.visible = false;
> +
> +	me.on('activate', me.rstore.startUpdate);
> +	me.on('destroy', me.rstore.stopUpdate);
> +	me.on('deactivate', me.rstore.stopUpdate);
> +    },
> +});