[PATCH 3 of 4] pullrequests: add pull_request_nice_id

[Mads Kiilerich]

On 04/25/2015 08:33 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
> # Date 1429265659 -7200
> #      Fri Apr 17 12:14:19 2015 +0200
> # Node ID 3d17f0d2a96eac41e6bf4312df1f6af476cce4ad
> # Parent  dc58eab8ff5070ab035f53fc241c924dd9aae3fb
> pullrequests: add pull_request_nice_id
>
> Currently, a pull request id is referenced as #5, and the '#' symbol is
> fixed and repeated in several places.  This commit adds a new property
> pull_request_nice_id, that returns a string reference to the pull request,
> currently in the form '#5'.
>
> This property could be overridden by an organization if they need references
> in another form, for example PR-5.
>
> diff --git a/kallithea/lib/helpers.py b/kallithea/lib/helpers.py
> --- a/kallithea/lib/helpers.py
> +++ b/kallithea/lib/helpers.py
> @@ -423,7 +423,7 @@
>   #==============================================================================
>   from kallithea.lib.vcs.utils import author_name, author_email
>   from kallithea.lib.utils2 import credentials_filter, age as _age
> -from kallithea.model.db import User, ChangesetStatus
> +from kallithea.model.db import User, ChangesetStatus, PullRequest
>   
>   age = lambda  x, y=False: _age(x, y)
>   capitalize = lambda x: x.capitalize()
> @@ -736,12 +736,18 @@
>   
>       def get_pull_request():
>           pull_request_id = action_params
> +        pr = PullRequest.get(pull_request_id)
> +        # Fallback when pull request does not exist. This should normally only
> +        # occur in tests with prepopulated fixtures.

(It is kind of obvious _what_ is going on; that a default is needed when 
the PR can't be found. The part about _why_ we need to handle it is 
however very relevant. I would guess it would be better to use 
get_or_404 and fix the tests.)
> +        nice_id = pr.pull_request_nice_id if pr else '#%s' % pull_request_id

Hmm ... instead of hardcoding the default format here, how about making 
pull_request_nice_id a class method (function of pr id) so it can be 
used for non-existing PRs? Perhaps better: just make it 'UNKNOWN#%s' ...

> +
>           deleted = user_log.repository is None
>           if deleted:
>               repo_name = user_log.repository_name
>           else:
>               repo_name = user_log.repository.repo_name
> -        return link_to(_('Pull request #%s') % pull_request_id,
> +
> +        return link_to(_('Pull request %s') % nice_id,
>                       url('pullrequest_show', repo_name=repo_name,
>                       pull_request_id=pull_request_id))
>   
> @@ -1414,7 +1420,6 @@
>   def changeset_status_lbl(changeset_status):
>       return dict(ChangesetStatus.STATUSES).get(changeset_status)
>   
> -
>   def get_permission_name(key):
>       return dict(Permission.PERMS).get(key)
>   
> diff --git a/kallithea/model/comment.py b/kallithea/model/comment.py
> --- a/kallithea/model/comment.py
> +++ b/kallithea/model/comment.py
> @@ -130,9 +130,9 @@
>               comment_url = pull_request.url(canonical=True,
>                   anchor='comment-%s' % comment.comment_id)
>               subj = safe_unicode(
> -                h.link_to('Re pull request #%(pr_id)s: %(desc)s %(line)s' % \
> +                h.link_to('Re pull request %(pr_id)s: %(desc)s %(line)s' % \
>                             {'desc': desc,
> -                           'pr_id': comment.pull_request.pull_request_id,
> +                           'pr_id': comment.pull_request.pull_request_nice_id,

But ... pr_id is no longer a pr_id but a nice_id? It is nice to keep the 
diff small but we should try to stay consistent. Please use a name like 
pr_nice_id in all the places where the type no longer is a pr id.

>                              'line': line},
>                             comment_url)
>               )
> @@ -148,7 +148,7 @@
>               #set some variables for email notification
>               email_kwargs = {
>                   'pr_title': pull_request.title,
> -                'pr_id': pull_request.pull_request_id,
> +                'pr_id': pull_request.pull_request_nice_id,
>                   'status_change': status_change,
>                   'closing_pr': closing_pr,
>                   'pr_comment_url': comment_url,
> diff --git a/kallithea/model/db.py b/kallithea/model/db.py
> --- a/kallithea/model/db.py
> +++ b/kallithea/model/db.py
> @@ -1387,9 +1387,10 @@
>               pr_id = pr_repo = None
>               if stat.pull_request:
>                   pr_id = stat.pull_request.pull_request_id
> +                pr_nice_id = stat.pull_request.pull_request_nice_id
>                   pr_repo = stat.pull_request.other_repo.repo_name
>               grouped[stat.revision] = [str(stat.status), stat.status_lbl,
> -                                      pr_id, pr_repo]
> +                                      pr_id, pr_repo, pr_nice_id]
>           return grouped
>   
>       def _repo_size(self):
> @@ -2305,6 +2306,11 @@
>               .first()
>           return str(status.status) if status else ''
>   
> +    @property
> +    def pull_request_nice_id(self):
> +        '''Return a string reference to this pull request'''
> +        return '#%d' % self.pull_request_id

We talked about it and I might have suggested the name ... but it is a 
very long.

The database model generally gives table X the primary key field X_id. 
It is a long name with redundancy but convenient when defining and 
debugging queries.

I do however think it would be fine to just call this property nice_id.

/Mads