Re: [pve-devel] [PATCH v3 proxmox-widget-toolkit 61/66] notification: add gui for sendmail notification endpoints

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

some comments/nits inline:

On 7/17/23 17:00, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>   src/Makefile                         |   4 +
>   src/Schema.js                        |   8 ++
>   src/data/model/NotificationConfig.js |   8 ++
>   src/panel/NotificationConfigView.js  | 192 +++++++++++++++++++++++++++
>   src/panel/SendmailEditPanel.js       | 140 +++++++++++++++++++
>   src/window/EndpointEditBase.js       |  50 +++++++
>   6 files changed, 402 insertions(+)
>   create mode 100644 src/data/model/NotificationConfig.js
>   create mode 100644 src/panel/NotificationConfigView.js
>   create mode 100644 src/panel/SendmailEditPanel.js
>   create mode 100644 src/window/EndpointEditBase.js
> 
> diff --git a/src/Makefile b/src/Makefile
> index baa90ec..e83038f 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -22,6 +22,7 @@ JSSRC=					\
>   	data/ObjectStore.js		\
>   	data/RRDStore.js		\
>   	data/TimezoneStore.js		\
> +	data/model/NotificationConfig.js	\
>   	data/model/Realm.js		\
>   	data/model/Certificates.js	\
>   	data/model/ACME.js		\
> @@ -59,6 +60,7 @@ JSSRC=					\
>   	panel/InfoWidget.js		\
>   	panel/LogView.js		\
>   	panel/NodeInfoRepoStatus.js	\
> +	panel/NotificationConfigView.js	\
>   	panel/JournalView.js		\
>   	panel/PermissionView.js		\
>   	panel/PruneKeepPanel.js		\
> @@ -68,6 +70,7 @@ JSSRC=					\
>   	panel/ACMEAccount.js		\
>   	panel/ACMEPlugin.js		\
>   	panel/ACMEDomains.js		\
> +	panel/SendmailEditPanel.js	\
>   	panel/StatusView.js		\
>   	panel/TfaView.js		\
>   	panel/NotesView.js		\
> @@ -83,6 +86,7 @@ JSSRC=					\
>   	window/ACMEAccount.js		\
>   	window/ACMEPluginEdit.js	\
>   	window/ACMEDomains.js		\
> +	window/EndpointEditBase.js		\
>   	window/FileBrowser.js		\
>   	window/AuthEditBase.js		\
>   	window/AuthEditOpenId.js	\
> diff --git a/src/Schema.js b/src/Schema.js
> index b247b1e..99bb3fa 100644
> --- a/src/Schema.js
> +++ b/src/Schema.js
> @@ -37,6 +37,14 @@ Ext.define('Proxmox.Schema', { // a singleton
>   	}
>       },
>   
> +    notificationEndpointTypes: {
> +	sendmail: {
> +	    name: gettext('Sendmail'),
> +	    ipanel: 'pmxSendmailEditPanel',
> +	    iconCls: 'fa-envelope-o',
> +	},
> +    },
> +
>       pxarFileTypes: {
>   	b: { icon: 'cube', label: gettext('Block Device') },
>   	c: { icon: 'tty', label: gettext('Character Device') },
> diff --git a/src/data/model/NotificationConfig.js b/src/data/model/NotificationConfig.js
> new file mode 100644
> index 0000000..c2ce843
> --- /dev/null
> +++ b/src/data/model/NotificationConfig.js
> @@ -0,0 +1,8 @@
> +Ext.define('proxmox-notification-endpoints', {
> +    extend: 'Ext.data.Model',
> +    fields: ['name', 'type', 'comment'],
> +    proxy: {
> +        type: 'proxmox',
> +    },
> +    idProperty: 'name',
> +});
> diff --git a/src/panel/NotificationConfigView.js b/src/panel/NotificationConfigView.js
> new file mode 100644
> index 0000000..f6e6a8b
> --- /dev/null
> +++ b/src/panel/NotificationConfigView.js
> @@ -0,0 +1,192 @@
> +Ext.define('Proxmox.panel.NotificationConfigView', {
> +    extend: 'Ext.panel.Panel',
> +    alias: 'widget.pmxNotificationConfigView',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +    layout: {
> +	type: 'border',
> +    },
> +
> +    items: [
> +	{
> +	    region: 'center',
> +	    border: false,
> +	    xtype: 'pmxNotificationEndpointView',
> +	    cbind: {
> +		baseUrl: '{baseUrl}',
> +	    },
> +	},
> +    ],
> +});
> +
> +Ext.define('Proxmox.panel.NotificationEndpointView', {
> +    extend: 'Ext.grid.Panel',
> +    alias: 'widget.pmxNotificationEndpointView',
> +
> +    title: gettext('Notification Targets'),
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	openEditWindow: function(endpointType, endpoint) {
> +	    let me = this;
> +
> +	    if (endpoint === 'mail-to-root') {
> +		return;
> +	    }
> +
> +	    Ext.create('Proxmox.window.EndpointEditBase', {
> +		baseUrl: me.getView().baseUrl,
> +		type: endpointType,
> +
> +		name: endpoint,
> +		listeners: {
> +		    destroy: () => me.reload(),
> +		},
> +	    }).show();

you could use 'autoShow: true' instead

> +	},
> +
> +	openEditForSelectedItem: function() {
> +	    let me = this;
> +	    let view = me.getView();
> +
> +	    let selection = view.getSelection();
> +	    if (selection.length < 1) return;

this is against your style guide, please don't use single line if statements.

> +	    let endpointName = selection[0].data.name;
> +	    let type = selection[0].data.type;
> +
> +	    me.openEditWindow(type, endpointName);

for this, you don't really need to extract it into seperate variables, simply call it
with

me.openEditWindow(selection[0].data.type, selection[0].data.name);

should still fit in one line ;)

> +	},
> +
> +	reload: function() {
> +	    let me = this;
> +	    let view = me.getView();
> +	    view.getStore().rstore.load();
> +	},
> +
> +	testEndpoint: function() {
> +	    let me = this;
> +	    let view = me.getView();
> +
> +	    let selection = view.getSelection();
> +	    if (selection.length < 1) return;

again style issue

> +	    let target = selection[0].data.name;
> +
> +	    Ext.Msg.confirm(
> +		gettext("Notification Target Test"),
> +		gettext(`Do you want to send a test notification to '${target}'?`),
> +		function(decision) {
> +		    if (decision !== "yes") {
> +			return;
> +		    }
> +
> +		    Proxmox.Utils.API2Request({
> +			method: 'POST',
> +			url: `${view.baseUrl}/targets/${target}/test`,
> +
> +			success: function(response, opt) {
> +			    Ext.Msg.show({
> +				title: gettext('Notification Target Test'),
> +				message: gettext(`Sent test notification to '${target}'.`),
> +				buttons: Ext.Msg.OK,
> +				icon: Ext.Msg.INFO,
> +			    });
> +			},
> +			autoErrorAlert: true,
> +		    });
> +	    });
> +	},
> +    },
> +
> +    listeners: {
> +	itemdblclick: 'openEditForSelectedItem',
> +	activate: 'reload',
> +    },
> +
> +    emptyText: gettext('No notification targets configured'),
> +
> +    columns: [
> +	{
> +	    dataIndex: 'name',
> +	    text: gettext('Target Name'),
> +	    renderer: Ext.String.htmlEncode,
> +	    flex: 1,
> +	},
> +	{
> +	    dataIndex: 'type',
> +	    text: gettext('Type'),
> +	    renderer: Ext.String.htmlEncode,
> +	    flex: 1,
> +	},
> +	{
> +	    dataIndex: 'comment',
> +	    text: gettext('Comment'),
> +	    renderer: Ext.String.htmlEncode,
> +	    flex: 1,
> +	},
> +    ],
> +
> +    store: {
> +	type: 'diff',
> +	autoDestroy: true,
> +	autoDestroyRstore: true,
> +	rstore: {
> +	    type: 'update',
> +	    storeid: 'proxmox-notification-endpoints',
> +	    model: 'proxmox-notification-endpoints',
> +	    autoStart: true,
> +	},
> +	sorters: 'name',
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	let menuItems = [];
> +	for (const [endpointType, config] of Object.entries(
> +	    Proxmox.Schema.notificationEndpointTypes).sort()) {
> +	    menuItems.push({
> +		text: config.name,
> +		iconCls: 'fa fa-fw ' + (config.iconCls || 'fa-bell-o'),
> +		handler: () => me.controller.openEditWindow(endpointType),
> +	    });
> +	}
> +
> +	Ext.apply(me, {
> +	    tbar: [
> +		{
> +		    text: gettext('Add'),
> +		    menu: menuItems,
> +		},
> +		{
> +		    xtype: 'proxmoxButton',
> +		    text: gettext('Modify'),
> +		    handler: 'openEditForSelectedItem',
> +		    enableFn: rec => rec.data.name !== 'mail-to-root',
> +		    disabled: true,
> +		},
> +		{
> +		    xtype: 'proxmoxStdRemoveButton',
> +		    callback: 'reload',
> +		    enableFn: rec => rec.data.name !== 'mail-to-root',
> +		    getUrl: function(rec) {
> +			if (rec.data.type === 'group') {
> +			    return `${me.baseUrl}/groups/${rec.getId()}`;
> +			}
> +
> +			return `${me.baseUrl}/endpoints/${rec.data.type}/${rec.getId()}`;
> +		    },
> +		},
> +		'-',
> +		{
> +		    xtype: 'proxmoxButton',
> +		    text: gettext('Test'),
> +		    handler: 'testEndpoint',
> +		    disabled: true,
> +		},
> +	    ],
> +	});
> +
> +	me.callParent();
> +	me.store.rstore.proxy.setUrl(`/api2/json/${me.baseUrl}/targets`);

would it maybe nicer in this case to have the store defined in the initcomponent
before the callParent to be able to inject it there?

no hard feelings though

> +    },
> +});
> diff --git a/src/panel/SendmailEditPanel.js b/src/panel/SendmailEditPanel.js
> new file mode 100644
> index 0000000..9444a8c
> --- /dev/null
> +++ b/src/panel/SendmailEditPanel.js
> @@ -0,0 +1,140 @@
> +Ext.define('Proxmox.panel.SendmailEditPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pmxSendmailEditPanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    type: 'sendmail',
> +
> +    columnT: [

if you only need need one column, why not simply use items/advancedItems and increase
the width of the window/inputpanel?

> +	{
> +	    xtype: 'pmxDisplayEditField',
> +	    name: 'name',
> +	    cbind: {
> +		value: '{name}',
> +		editable: '{isCreate}',
> +	    },
> +	    fieldLabel: gettext('Endpoint Name'),
> +	    allowBlank: false,
> +	},
> +	{
> +	    xtype: 'pmxUserSelector',
> +	    name: 'mailto-user',
> +	    reference: 'mailto-user',
> +	    multiSelect: true,
> +	    allowBlank: true,
> +	    editable: false,
> +	    skipEmptyText: true,
> +	    fieldLabel: gettext('User(s)'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	    validator: function(value) {
> +		let up = this.up('pmxSendmailEditPanel');
> +		let other = up.down('[name=mailto]');
> +
> +		if (!value && !other.getValue()) {
> +		    return gettext('Either mailto or mailto-user must be set');
> +		}
> +
> +		return true;
> +	    },

the validator/gettext could be factored out since it's basically the same as below,
but it's ok this way if thats too much work


> +	    listConfig: {
> +		width: 600,
> +		columns: [
> +		    {
> +			header: gettext('User'),
> +			sortable: true,
> +			dataIndex: 'userid',
> +			renderer: Ext.String.htmlEncode,
> +			flex: 1,
> +		    },
> +		    {
> +			header: gettext('E-Mail'),
> +			sortable: true,
> +			dataIndex: 'email',
> +			renderer: Ext.String.htmlEncode,
> +			flex: 1,
> +		    },
> +		    {
> +			header: gettext('Comment'),
> +			sortable: false,
> +			dataIndex: 'comment',
> +			renderer: Ext.String.htmlEncode,
> +			flex: 1,
> +		    },
> +		],
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    fieldLabel: gettext('Additional Recipient(s)'),
> +	    name: 'mailto',
> +	    reference: 'mailto',
> +	    allowBlank: true,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	    autoEl: {
> +		tag: 'div',
> +		'data-qtip': gettext(
> +		    'Multiple recipients must be separated by spaces, commas or semicolons',
> +		),
> +	    },
> +	    validator: function(value) {
> +		let up = this.up('pmxSendmailEditPanel');
> +		let other = up.down('[name=mailto-user]');
> +
> +		if (!value && !other.getValue().length) {
> +		    return gettext('Either mailto or mailto-user must be set');
> +		}
> +
> +		return true;
> +	    },
> +	},
> +    ],
> +
> +    column1: [],
> +
> +    column2: [],
> +
> +    columnB: [

why in columnB instead of columnT if it's the only one in use?

> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'comment',
> +	    fieldLabel: gettext('Comment'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +    ],
> +    advancedColumnB: [
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    fieldLabel: gettext('Author'),
> +	    name: 'author',
> +	    allowBlank: true,
> +	    emptyText: gettext('Proxmox VE'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    fieldLabel: gettext('From Address'),
> +	    name: 'from-address',
> +	    allowBlank: true,
> +	    emptyText: gettext('Defaults to datacenter configuration, or root@$hostname'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +    ],
> +
> +    onGetValues: (values) => {
> +	if (values.mailto) {
> +	    values.mailto = values.mailto.split(/[\s,;]+/);
> +	}
> +	return values;
> +    },
> +});
> +
> diff --git a/src/window/EndpointEditBase.js b/src/window/EndpointEditBase.js
> new file mode 100644
> index 0000000..81e5951
> --- /dev/null
> +++ b/src/window/EndpointEditBase.js
> @@ -0,0 +1,50 @@
> +Ext.define('Proxmox.window.EndpointEditBase', {
> +    extend: 'Proxmox.window.Edit',
> +
> +    isAdd: true,
> +
> +    fieldDefaults: {
> +	labelWidth: 120,
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	me.isCreate = !me.name;
> +
> +	if (!me.baseUrl) {
> +	    throw "baseUrl not set";
> +	}
> +
> +	me.url = `/api2/extjs${me.baseUrl}/endpoints/${me.type}`;
> +
> +	if (me.isCreate) {
> +	    me.method = 'POST';
> +	} else {
> +	    me.url += `/${me.name}`;
> +	    me.method = 'PUT';
> +	}
> +
> +	let endpointConfig = Proxmox.Schema.notificationEndpointTypes[me.type];
> +	if (!endpointConfig) {
> +	    throw 'unknown endpoint type';
> +	}
> +
> +	me.subject = endpointConfig.name;
> +
> +	Ext.apply(me, {
> +	    items: [{
> +		name: me.name,
> +		xtype: endpointConfig.ipanel,
> +		isCreate: me.isCreate,
> +		type: me.type,
> +	    }],
> +	});
> +
> +	me.callParent();
> +
> +	if (!me.isCreate) {
> +	    me.load();
> +	}
> +    },
> +});