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

[Mads Kiilerich]

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?
* 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.
* 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'.

What's your thoughts and vision of the general direction this patch 
would take us ... and what is your response to my thoughts?

/Mads

> diff --git a/kallithea/controllers/pullrequests.py b/kallithea/controllers/pullrequests.py
> --- a/kallithea/controllers/pullrequests.py
> +++ b/kallithea/controllers/pullrequests.py
> @@ -374,7 +374,7 @@ class PullrequestsController(BaseRepoCon
>   
>           return redirect(pull_request.url())
>   
> -    def create_update(self, old_pull_request, updaterev, title, description, reviewers_ids):
> +    def create_update(self, old_pull_request, updaterev, title, description, reviewers):
>           org_repo = RepoModel()._get_repo(old_pull_request.org_repo.repo_name)
>           org_ref_type, org_ref_name, org_rev = old_pull_request.org_ref.split(':')
>           new_org_rev = self._get_ref_rev(org_repo, 'rev', updaterev)
> @@ -448,7 +448,7 @@ class PullrequestsController(BaseRepoCon
>                   self.authuser.user_id,
>                   old_pull_request.org_repo.repo_name, new_org_ref,
>                   old_pull_request.other_repo.repo_name, new_other_ref,
> -                revisions, reviewers_ids, title, description
> +                revisions, reviewers, title, description
>               )
>           except UserInvalidException, u:
>               h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
> @@ -490,21 +490,21 @@ class PullrequestsController(BaseRepoCon
>               raise HTTPForbidden()
>   
>           _form = PullRequestPostForm()().to_python(request.POST)
> -        reviewers_ids = [int(s) for s in _form['review_members']]
> +        reviewers = _form['review_members']
>   
>           if _form['updaterev']:
>               return self.create_update(pull_request,
>                                         _form['updaterev'],
>                                         _form['pullrequest_title'],
>                                         _form['pullrequest_desc'],
> -                                      reviewers_ids)
> +                                      reviewers)
>   
>           old_description = pull_request.description
>           pull_request.title = _form['pullrequest_title']
>           pull_request.description = _form['pullrequest_desc'].strip() or _('No description')
>           try:
>               PullRequestModel().mention_from_description(pull_request, old_description)
> -            PullRequestModel().update_reviewers(pull_request_id, reviewers_ids)
> +            PullRequestModel().update_reviewers(pull_request_id, reviewers)
>           except UserInvalidException, u:
>               h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
>               raise HTTPBadRequest()
> diff --git a/kallithea/model/pull_request.py b/kallithea/model/pull_request.py
> --- a/kallithea/model/pull_request.py
> +++ b/kallithea/model/pull_request.py
> @@ -179,8 +179,8 @@ class PullRequestModel(BaseModel):
>           log.debug("Mentioning %s" % mention_recipients)
>           self.__add_reviewers(pr, [], mention_recipients)
>   
> -    def update_reviewers(self, pull_request, reviewers_ids):
> -        reviewers_ids = set(reviewers_ids)
> +    def update_reviewers(self, pull_request, reviewers):
> +        reviewers_ids = set([self._get_user(member).user_id for member in reviewers])
>           pull_request = self.__get_pull_request(pull_request)
>           current_reviewers = PullRequestReviewers.query()\
>                               .filter(PullRequestReviewers.pull_request==
> diff --git a/kallithea/public/js/base.js b/kallithea/public/js/base.js
> --- a/kallithea/public/js/base.js
> +++ b/kallithea/public/js/base.js
> @@ -1484,13 +1484,13 @@ var addReviewMember = function(id,fname,
>           '           </div>\n'+
>           '         <div class="reviewer_gravatar gravatar">{0}</div>\n'+
>           '         <div style="float:left;">{1}</div>\n'+
> -        '         <input type="hidden" value="{2}" name="review_members" />\n'+
> +        '         <input type="hidden" value="{3}" name="review_members" />\n'+
>           '         <div class="reviewer_member_remove action_button" onclick="removeReviewMember({2})">\n'+
>           '             <i class="icon-minus-circled"></i>\n'+
>           '         </div> (add not saved)\n'+
>           '       </div>\n'+
>           '     </li>\n'
> -        ).format(gravatarelm, displayname, id);
> +        ).format(gravatarelm, displayname, id, nname);
>       // check if we don't have this ID already in
>       var ids = [];
>       $('#review_members').find('li').each(function() {
> diff --git a/kallithea/templates/pullrequests/pullrequest_show.html b/kallithea/templates/pullrequests/pullrequest_show.html
> --- a/kallithea/templates/pullrequests/pullrequest_show.html
> +++ b/kallithea/templates/pullrequests/pullrequest_show.html
> @@ -224,7 +224,7 @@
>                       ${h.gravatar(member.email, size=14)}
>                     </div>
>                     <div style="float:left;">${member.full_name} (${_('Owner') if c.pull_request.user_id == member.user_id else _('Reviewer')})</div>
> -                  <input type="hidden" value="${member.user_id}" name="review_members" />
> +                  <input type="hidden" value="${member.username}" name="review_members" />
>                     %if editable:
>                     <div class="reviewer_member_remove action_button" onclick="removeReviewMember(${member.user_id})" title="${_('Remove reviewer')}">
>                         <i class="icon-minus-circled"></i>