[PATCH 2 of 2] pullrequest: internally use username iso userid when adding reviewers

[Mads Kiilerich]

On 06/22/2015 10:17 PM, Thomas De Schampheleire wrote:
> On Mon, Jun 22, 2015 at 7:21 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 06/22/2015 06:45 PM, Thomas De Schampheleire wrote:
>>> # HG changeset patch
>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>> # Date 1434744060 -7200
>>> #      Fri Jun 19 22:01:00 2015 +0200
>>> # Node ID 0e58e20f94d3a3fbb84f588397c2def50dd3b35d
>>> # Parent  68032f8bd7f52c676bb5143a03b0dfd5df242f0c
>>> pullrequest: internally use username iso userid when adding reviewers
>>>
>>> When using API calls to create/update pullrequests and associated
>>> reviewers,
>>> it is not convenient to need to know the user ids. Rather, change the form
>>> handling so that usernames are used as form data.
>>
>> I agree that this change would be inconvenient if you happen to know the
>> username but not the user ID.
>>
>> But ...
>> * this leaves us with exactly the opposite problem: what if you know the
>> user ID but not the username?
> My use case is that of a human user, and it makes little sense for
> this human to remember user IDs.

A human user of the API? That seems like a rare case. It must have some 
wrapping that _could_ look the user id up first. (Further explanation 
below confirms that.)

> But there may be other use cases, indeed.
>
>
>> * usernames are chosen by users and can change and are not a "primary key"
>> of the user - it is thus not suitable for any "update" API ... but fine for
>> a search/get API.
> I'm not convinced about this argument: at any given point in time the
> current username is a primary key.

It is a unique key, yes. But not a primary key. It can change and do 
thus not unambiguously specify an entity and is thus not 100% reliable.

I agree it is fine for many practical purposes, but we should not make 
the API less than 100% reliable.

> My use case is a review-request-creation wrapper. A developer requests
> a review of some commits to certain reviewers which he enters by
> username. A change in username would obviously be a need-to-know for
> the creator of the review, but is not possible in my case because LDAP
> is used and usernames are fixed.

You might use a LDAP attribute that is immutable or have a convention 
that it will stay stable. We use sAMAccountName - that is an attribute 
that _can_ change, even though it rarely happens and often is a bad idea.

For us, these sAMAccountNames doesn't matter that much. People often 
have email addresses and other 'handles' that are different from that. 
(For weird reasons, I got 'madski' when I joined Unity. It happened to 
grew on me and now I like it, but I could probably have changed by 
'public' name to kiilerix or madsk or kiilerich, and nobody would know 
what 'madski' was.)

I think our use cases are equally important and valid. Both should be 
supported, but they also show that we can't rely on usernames as primary 
keys.

>
>> * Kallithea usernames do not necessarily make sense - it is a local and kind
>> of arbitrary namespace. I would like to make usernames more optional as
>> "local short nickname" and move towards using the 3 other ways we have to
>> identify users: email addresses, full name and internal ID.
>>
>> We could perhaps for convenience make a generic feature in the json API that
>> user_id can be specified as 'username:bob' or 'email:bob at example.com' ...
>> and similarly that repo_id can be specifed as
>> 'reponame:opensource/kallithea'.
>
> In my case, expecting users to enter a full-email address is a lot of
> typing, likewise for the full name, and the internal ID is not known.

Depending on how friendly your tool is, I would guess it could and 
should look the users up through the API first and show their username 
and full name before creating the pull request?

> The shorthands can work for me: the wrapper can add the 'username:'
> prefix, but they should not only be recognized in the JSON API but
> also in the regular controller code. Recall that I'm creating/updating
> a pull request using the API whitelist, thus directly calling a
> controller URL with some form data.
>
> Who should resolve such strings, the get_instance method ?

I think that could make sense. Or perhaps a "get_instance_highlevel" 
function that could be used in the more user facing places, perhaps 
replacing get_by_username and get_by_email.

/Mads