From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 79241E9D0 for ; Wed, 19 Jul 2023 14:45:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 52CF275EB for ; Wed, 19 Jul 2023 14:45:16 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 19 Jul 2023 14:45:15 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 28E0841033; Wed, 19 Jul 2023 14:45:15 +0200 (CEST) Message-ID: <93843348-c48b-d86f-da04-c5d36404aa87@proxmox.com> Date: Wed, 19 Jul 2023 14:45:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Lukas Wagner References: <20230717150051.710464-1-l.wagner@proxmox.com> <20230717150051.710464-58-l.wagner@proxmox.com> From: Dominik Csapak In-Reply-To: <20230717150051.710464-58-l.wagner@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.016 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v3 pve-manager 57/66] ui: allow to configure notification event -> target mapping X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Jul 2023 12:45:46 -0000 some comments inline: On 7/17/23 17:00, Lukas Wagner wrote: > Signed-off-by: Lukas Wagner > --- > 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); > + }, > +});