From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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 5D876669B5
 for <pbs-devel@lists.proxmox.com>; Fri,  6 Nov 2020 19:50:27 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 4DB6B2509A
 for <pbs-devel@lists.proxmox.com>; Fri,  6 Nov 2020 19:49:57 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 040722508E
 for <pbs-devel@lists.proxmox.com>; Fri,  6 Nov 2020 19:49:56 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id BF7DF46059
 for <pbs-devel@lists.proxmox.com>; Fri,  6 Nov 2020 19:49:55 +0100 (CET)
To: Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>,
 =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= <f.gruenbichler@proxmox.com>
References: <20201106120158.3388409-1-f.gruenbichler@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Message-ID: <851a0971-6dce-31ff-4a53-b5b4955e510f@proxmox.com>
Date: Fri, 6 Nov 2020 19:49:54 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:83.0) Gecko/20100101
 Thunderbird/83.0
MIME-Version: 1.0
In-Reply-To: <20201106120158.3388409-1-f.gruenbichler@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.113 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -0.001 Looks like a legit reply (A)
 RCVD_IN_DNSWL_MED        -2.3 Sender listed at https://www.dnswl.org/,
 medium trust
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pbs-devel] [PATCH proxmox-backup 1/3] www: add AuthidSelector
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 06 Nov 2020 18:50:27 -0000

some "after applied" review below, maybe for the next time when we do not=
 need
to rush such things in ;-)

On 06.11.20 13:01, Fabian Gr=C3=BCnbichler wrote:
> similar to TokenSelector, but with different fields / mapping of data.
>=20

this should be quite easily mergable with the existing user selector, I'l=
l see
if I can do that.

> Signed-off-by: Fabian Gr=C3=BCnbichler <f.gruenbichler@proxmox.com>
> ---
>  www/Makefile               |  1 +
>  www/form/AuthidSelector.js | 98 ++++++++++++++++++++++++++++++++++++++=

>  2 files changed, 99 insertions(+)
>  create mode 100644 www/form/AuthidSelector.js
>=20
> diff --git a/www/Makefile b/www/Makefile
> index 9fd014b4..f8516a23 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -8,6 +8,7 @@ JSSRC=3D							\
>  	Utils.js					\
>  	form/UserSelector.js				\
>  	form/TokenSelector.js				\
> +	form/AuthidSelector.js				\
>  	form/RemoteSelector.js				\
>  	form/DataStoreSelector.js			\
>  	form/CalendarEvent.js				\
> diff --git a/www/form/AuthidSelector.js b/www/form/AuthidSelector.js
> new file mode 100644
> index 00000000..89389834
> --- /dev/null
> +++ b/www/form/AuthidSelector.js
> @@ -0,0 +1,98 @@
> +Ext.define('pbs-authids', {
> +    extend: 'Ext.data.Model',
> +    fields: [
> +	'authid', 'comment', 'type',
> +    ],
> +    idProperty: 'authid',
> +});
> +
> +Ext.define('PBS.form.AuthidSelector', {
> +    extend: 'Proxmox.form.ComboGrid',
> +    alias: 'widget.pbsAuthidSelector',
> +
> +    allowBlank: false,
> +    autoSelect: false,
> +    valueField: 'authid',
> +    displayField: 'authid',
> +
> +    editable: true,
> +    anyMatch: true,
> +    forceSelection: true,
> +
> +    store: {
> +	model: 'pbs-authids',
> +	params: {
> +	    enabled: 1,
> +	},
> +	sorters: 'authid',
> +    },
> +
> +    initComponent: function() {
> +	let me =3D this;
> +	me.userStore =3D Ext.create('Ext.data.Store', {
> +	    model: 'pbs-users-with-tokens',
> +	});
> +	me.userStore.on('load', this.onLoad, this);
> +	me.userStore.load();
> +
> +	me.callParent();
> +    },
> +
> +    onLoad: function(store, data, success) {
> +	if (!success) return;
> +
> +	let authidStore =3D this.store;
> +
> +	let records =3D [];
> +	Ext.Array.each(data, function(user) {

could be just:

for (const record of data) {
   ...
}
> +	let u =3D {};
> +	u.authid =3D user.data.userid;
> +	u.comment =3D user.data.comment;
> +	u.type =3D 'u';
> +	records.push(u);
> +	let tokens =3D user.data.tokens || [];
> +	Ext.Array.each(tokens, function(token) {

could be just
for (const token of tokens) {
   ...
}

> +	    let r =3D {};
> +	    r.authid =3D token.tokenid;
> +	    r.comment =3D token.comment;
> +	    r.type =3D 't';
> +	    records.push(r);

besides indentation being completely off, please write such things as
let r =3D {
    authid: token.tokenid,
    comment: token.comment,
    type: 't',
};

you could also push this directly without constructing r first.

> +	});
> +	});
> +
> +	authidStore.loadData(records);

you need to validate the field after loading data like this, else it's al=
ways shown as
invalid even if set to an OK value.

> +    },
> +
> +    listConfig: {

for more columns it often can be better to increase the default width of =
the
picker grid, e.g.:

width: 500,

> +	columns: [
> +	    {
> +		header: gettext('Type'),
> +		sortable: true,
> +		dataIndex: 'type',
> +		renderer: function(value) {
> +		    switch (value) {
> +			case 'u': return gettext('User');
> +			case 't': return gettext('API Token');
> +			default: return Proxmox.Utils.unknownText;
> +		    }
> +		},
> +		flex: 1,

if the size is really fixed, we use also fixed widths to  give dynamic da=
ta more space
and improve layout, i.e.:

width: 80,

> +	    },
> +	    {
> +		header: gettext('Auth ID'),
> +		sortable: true,
> +		dataIndex: 'authid',
> +		renderer: Ext.String.htmlEncode,
> +		flex: 1,
> +	    },
> +	    {
> +		header: gettext('Comment'),
> +		sortable: false,
> +		dataIndex: 'comment',
> +		renderer: Ext.String.htmlEncode,
> +		flex: 1,
> +	    },
> +	],
> +    },
> +});
> +
>=20