Re: [pbs-devel] [PATCH proxmox-backup v7 8/8] ui: add MetricServerView and use it

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 02/06/2022 um 14:18 schrieb Dominik Csapak:
> simple CRUD interface to show/add/edit/delete metric servers
> 
> it's a bit different from PVE's so that it's harder to reuse that to
> copy it. If we need it again, we can still refactor and combine them.
> 

looks OK, just some code style nits inline.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/Makefile                   |   1 +
>  www/NavigationTree.js          |   6 ++
>  www/config/MetricServerView.js | 145 +++++++++++++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 www/config/MetricServerView.js
> 
> diff --git a/www/Makefile b/www/Makefile
> index 3a36daba..757c1fd5 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -62,6 +62,7 @@ JSSRC=							\
>  	config/WebauthnView.js				\
>  	config/CertificateView.js			\
>  	config/NodeOptionView.js			\
> +	config/MetricServerView.js			\
>  	window/ACLEdit.js				\
>  	window/BackupFileDownloader.js			\
>  	window/BackupGroupChangeOwner.js		\
> diff --git a/www/NavigationTree.js b/www/NavigationTree.js
> index 5f44aace..dc69f312 100644
> --- a/www/NavigationTree.js
> +++ b/www/NavigationTree.js
> @@ -68,6 +68,12 @@ Ext.define('PBS.store.NavigationStore', {
>  			path: 'pbsCertificateConfiguration',
>  			leaf: true,
>  		    },
> +		    {
> +			text: gettext('Metric Server'),
> +			iconCls: 'fa fa-bar-chart',
> +			path: 'pbsMetricServerView',
> +			leaf: true,
> +		    },

I'd rather put this as tab in the general "Configuration" panel, this is slightly too
niche to take up space in the main navigation tree directly.

>  		    {
>  			text: gettext('Subscription'),
>  			iconCls: 'fa fa-support',
> diff --git a/www/config/MetricServerView.js b/www/config/MetricServerView.js
> new file mode 100644
> index 00000000..b904a427
> --- /dev/null
> +++ b/www/config/MetricServerView.js
> @@ -0,0 +1,145 @@
> +Ext.define('PBS.config.MetricServerView', {
> +    extend: 'Ext.grid.Panel',
> +    alias: ['widget.pbsMetricServerView'],
> +
> +    stateful: true,
> +    stateId: 'grid-metricserver',
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	render_type: function(value) {

I'd prefer camelCase for new functions where sensible, like ExtJS uses everywhere and part of
our code base also does.

> +	    switch (value) {
> +		case 'influxdb-http': return "InfluxDB (HTTP)";
> +		case 'influxdb-udp': return "InfluxDB (UDP)";
> +		default: return Proxmox.Utils.unknownText;
> +	    }

I'd like to avoid switch case in JS, at least most of the time, there seldom nice and either
use more space while still being more noisy/harder to read than objects or require having
multiple statements on a single line, both not to nice. If JS would have a match statement
allowing to return a expression like we got in rust I'd be sold though.

Rather just use a object. with multiple such similar metric type derived info one could also
use a nested, schema like one

{
    indlufxdb: {
        xtype: '...',
        name: '...',
    },
}

While this is also using quite some vertical space it's at least easy to read due to being
like a static schema definition.

The renderer could then be also just inline, e.g.:
v => PBS.Schema.metricServer[v]?.name ?? Proxmox.Utils.unknownText,

but not to hard feelings for that.

> +	},
> +
> +	get_xtype: function(value) {
> +	    switch (value) {

same here (and thus the schema proposal)

> +		case 'influxdb-http': return "InfluxDbHttp";
> +		case 'influxdb-udp': return "InfluxDbUdp";
> +		default: throw "invalid type";
> +	    }
> +	},
> +
> +	editWindow: function(xtype, id) {
> +	    let me = this;
> +	    Ext.create(`PBS.window.${xtype}Edit`, {
> +		serverid: id,
> +		autoShow: true,
> +		autoLoad: true,
> +		listeners: {
> +		    destroy: () => me.reload(),
> +		},
> +	    });
> +	},
> +
> +	addServer: function(button) {
> +	    this.editWindow(this.get_xtype(button.type));
> +	},
> +
> +	editServer: function() {
> +	    let me = this;
> +	    let view = me.getView();
> +	    let selection = view.getSelection();
> +	    if (!selection || selection.length < 1) {
> +		return;
> +	    }
> +
> +	    let cfg = selection[0].data;
> +
> +	    let xtype = me.get_xtype(cfg.type);
> +	    me.editWindow(xtype, cfg.name);
> +	},
> +
> +	reload: function() {
> +	    this.getView().getStore().load();
> +	},
> +    },
> +
> +    store: {
> +	autoLoad: true,
> +	id: 'metricservers',
> +	proxy: {
> +	    type: 'proxmox',
> +	    url: '/api2/json/admin/metricserver',
> +	},
> +    },
> +
> +    columns: [
> +	{
> +	    text: gettext('Name'),
> +	    flex: 2,
> +	    dataIndex: 'name',
> +	},
> +	{
> +	    text: gettext('Type'),
> +	    width: 150,
> +	    dataIndex: 'type',
> +	    renderer: 'render_type',
> +	},
> +	{
> +	    text: gettext('Enabled'),
> +	    dataIndex: 'disable',
> +	    width: 100,
> +	    renderer: Proxmox.Utils.format_neg_boolean,
> +	},
> +	{
> +	    text: gettext('Target Server'),
> +	    width: 200,
> +	    dataIndex: 'server',
> +	},
> +	{
> +	    text: gettext('Comment'),
> +	    flex: 3,
> +	    dataIndex: 'comment',
> +	    renderer: Ext.htmlEncode,
> +	},
> +    ],
> +
> +    tbar: [
> +	{
> +	    text: gettext('Add'),
> +	    menu: [
> +		{
> +		    text: 'InfluxDB (HTTP)',
> +		    type: 'influxdb-http',
> +		    iconCls: 'fa fa-fw fa-bar-chart',
> +		    handler: 'addServer',
> +		},
> +		{
> +		    text: 'InfluxDB (UDP)',
> +		    type: 'influxdb-udp',
> +		    iconCls: 'fa fa-fw fa-bar-chart',
> +		    handler: 'addServer',
> +		},
> +	    ],
> +	},
> +	{
> +	    text: gettext('Edit'),
> +	    xtype: 'proxmoxButton',
> +	    handler: 'editServer',
> +	    disabled: true,
> +	},
> +	{
> +	    xtype: 'proxmoxStdRemoveButton',
> +	    getUrl: (rec) => `/api2/extjs/config/metricserver/${rec.data.type}/${rec.data.name}`,
> +	    getRecordName: (rec) => rec.data.name,
> +	    callback: 'reload',
> +	},
> +    ],
> +
> +    listeners: {
> +	itemdblclick: 'editServer',
> +    },
> +
> +    initComponent: function() {
> +	var me = this;
> +
> +	me.callParent();
> +
> +	Proxmox.Utils.monStoreErrors(me, me.getStore());
> +    },
> +});