kallithea Changeset - bcc67f909d9f

Changeset - bcc67f909d9f

Parent rev.

Child rev.

[Not reviewed]

default

0 3 0

Søren Løvborg - 9 years ago 2017-02-27 15:38:20
sorenl@unity3d.com

pullrequests: pass around reviewer User objects, not IDs

This moves reviewer user validation into the controller and eliminates
the UserInvalidException.

3 files changed with 53 insertions and 54 deletions:

kallithea/controllers/pullrequests.py

kallithea/lib/exceptions.py

kallithea/model/pull_request.py

0 comments (0 inline, 0 general)

kallithea/controllers/pullrequests.py

➞

Show inline comments

@@ @@ -33,47 +33,60 @@ import re @@
 from pylons import request, tmpl_context as c
 from pylons.i18n.translation import _
 from webob.exc import HTTPFound, HTTPNotFound, HTTPForbidden, HTTPBadRequest
 from kallithea.config.routing import url
 from kallithea.lib import helpers as h
 from kallithea.lib import diffs
 from kallithea.lib.auth import LoginRequired, HasRepoPermissionLevelDecorator, \
     NotAnonymous
 from kallithea.lib.base import BaseRepoController, render, jsonify
 from kallithea.lib.compat import json, OrderedDict
 from kallithea.lib.diffs import LimitedDiffContainer
 from kallithea.lib.exceptions import UserInvalidException
 from kallithea.lib.page import Page
 from kallithea.lib.utils import action_logger
 from kallithea.lib.vcs.exceptions import EmptyRepositoryError, ChangesetDoesNotExistError
 from kallithea.lib.vcs.utils import safe_str
 from kallithea.lib.vcs.utils.hgcompat import unionrepo
 from kallithea.model.db import PullRequest, ChangesetStatus, ChangesetComment, \
     PullRequestReviewer, Repository, User
 from kallithea.model.pull_request import PullRequestModel
 from kallithea.model.meta import Session
 from kallithea.model.repo import RepoModel
 from kallithea.model.comment import ChangesetCommentsModel
 from kallithea.model.changeset_status import ChangesetStatusModel
 from kallithea.model.forms import PullRequestForm, PullRequestPostForm
 from kallithea.lib.utils2 import safe_int
 from kallithea.controllers.changeset import _ignorews_url, _context_url, \
     create_comment
 from kallithea.controllers.compare import CompareController
 from kallithea.lib.graphmod import graph_data
 log = logging.getLogger(__name__)
 def _get_reviewer(user_id):
     """Look up user by ID and validate it as a potential reviewer."""
     try:
         user = User.get(int(user_id))
     except ValueError:
         user = None
     if user is None or user.is_default_user:
         h.flash(_('Invalid reviewer "%s" specified') % user_id, category='error')
         raise HTTPBadRequest()
     return user
 class PullrequestsController(BaseRepoController):
     def _get_repo_refs(self, repo, rev=None, branch=None, branch_rev=None):
         """return a structure with repo's interesting changesets, suitable for
         the selectors in pullrequest.html
         rev: a revision that must be in the list somehow and selected by default
         branch: a branch that must be in the list and selected by default - even if closed
         branch_rev: a revision of which peers should be preferred and available."""
         # list named branches that has been merged to this named branch - it should probably merge back
         peers = []
@@ @@ -354,54 +367,51 @@ class PullrequestsController(BaseRepoCon @@
             msg = _('Cannot create pull request - criss cross merge detected, please merge a later %s revision to %s'
                     ) % (other_ref_name, org_ref_name)
         if ancestor_rev is None:
             h.flash(msg, category='error', logf=log.error)
             raise HTTPNotFound
         revisions = [cs_.raw_id for cs_ in cs_ranges]
         # hack: ancestor_rev is not an other_rev but we want to show the
         # requested destination and have the exact ancestor
         other_ref = '%s:%s:%s' % (other_ref_type, other_ref_name, ancestor_rev)
-        reviewer_ids = []
+        reviewers = []
         title = _form['pullrequest_title']
         if not title:
             if org_repo == other_repo:
                 title = '%s to %s' % (org_display, other_display)
             else:
                 title = '%s#%s to %s#%s' % (org_repo.repo_name, org_display,
                                             other_repo.repo_name, other_display)
         description = _form['pullrequest_desc'].strip() or _('No description')
         try:
             created_by = User.get(request.authuser.user_id)
             pull_request = PullRequestModel().create(
                 created_by, org_repo, org_ref, other_repo, other_ref, revisions,
-                title, description, reviewer_ids)
+                title, description, reviewers)
             Session().commit()
             h.flash(_('Successfully opened new pull request'),
                     category='success')
         except UserInvalidException as u:
             h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
             raise HTTPBadRequest()
         except Exception:
             h.flash(_('Error occurred while creating pull request'),
                     category='error')
             log.error(traceback.format_exc())
             raise HTTPFound(location=url('pullrequest_home', repo_name=repo_name))
         raise HTTPFound(location=pull_request.url())
-    def create_new_iteration(self, old_pull_request, new_rev, title, description, reviewer_ids):
+    def create_new_iteration(self, old_pull_request, new_rev, title, description, reviewers):
         org_repo = old_pull_request.org_repo
         org_ref_type, org_ref_name, org_rev = old_pull_request.org_ref.split(':')
         new_org_rev = self._get_ref_rev(org_repo, 'rev', new_rev)
         other_repo = old_pull_request.other_repo
         other_ref_type, other_ref_name, other_rev = old_pull_request.other_ref.split(':') # other_rev is ancestor
         #assert other_ref_type == 'branch', other_ref_type # TODO: what if not?
         new_other_rev = self._get_ref_rev(other_repo, other_ref_type, other_ref_name)
         cs_ranges, _cs_ranges_not, ancestor_revs = CompareController._get_changesets(org_repo.scm_instance.alias,
             other_repo.scm_instance, new_other_rev, # org and other "swapped"
             org_repo.scm_instance, new_org_rev)
@@ @@ -469,28 +479,25 @@ class PullrequestsController(BaseRepoCon @@
         title = '%s (v%s)' % (title.strip(), v)
         # using a mail-like separator, insert new iteration info in description with latest first
         descriptions = description.replace('\r\n', '\n').split('\n-- \n', 1)
         description = descriptions[0].strip() + '\n\n-- \n' + '\n'.join(infos)
         if len(descriptions) > 1:
             description += '\n\n' + descriptions[1].strip()
         try:
             created_by = User.get(request.authuser.user_id)
             pull_request = PullRequestModel().create(
                 created_by, org_repo, new_org_ref, other_repo, new_other_ref, revisions,
                 title, description, reviewer_ids)
         except UserInvalidException as u:
             h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
             raise HTTPBadRequest()
                 title, description, reviewers)
         except Exception:
             h.flash(_('Error occurred while creating pull request'),
                     category='error')
             log.error(traceback.format_exc())
             raise HTTPFound(location=old_pull_request.url())
         ChangesetCommentsModel().create(
             text=_('Closed, next iteration: %s .') % pull_request.url(canonical=True),
             repo=old_pull_request.other_repo_id,
             author=request.authuser.user_id,
             pull_request=old_pull_request.pull_request_id,
             closing_pr=True)
@@ @@ -509,60 +516,60 @@ class PullrequestsController(BaseRepoCon @@
     def post(self, repo_name, pull_request_id):
         pull_request = PullRequest.get_or_404(pull_request_id)
         if pull_request.is_closed():
             raise HTTPForbidden()
         assert pull_request.other_repo.repo_name == repo_name
         #only owner or admin can update it
         owner = pull_request.owner_id == request.authuser.user_id
         repo_admin = h.HasRepoPermissionLevel('admin')(c.repo_name)
         if not (h.HasPermissionAny('hg.admin')() or repo_admin or owner):
             raise HTTPForbidden()
         _form = PullRequestPostForm()().to_python(request.POST)
         reviewer_ids = set(int(s) for s in _form['review_members'])
         org_reviewer_ids = set(int(s) for s in _form['org_review_members'])
         current_reviewer_ids = set(prr.user_id for prr in pull_request.reviewers)
         other_added = [User.get(u) for u in current_reviewer_ids - org_reviewer_ids]
         other_removed = [User.get(u) for u in org_reviewer_ids - current_reviewer_ids]
         cur_reviewers = set(pull_request.get_reviewer_users())
         new_reviewers = set(_get_reviewer(s) for s in _form['review_members'])
         old_reviewers = set(_get_reviewer(s) for s in _form['org_review_members'])
         other_added = cur_reviewers - old_reviewers
         other_removed = old_reviewers - cur_reviewers
         if other_added:
             h.flash(_('Meanwhile, the following reviewers have been added: %s') %
                     (', '.join(u.username for u in other_added)),
                     category='warning')
         if other_removed:
             h.flash(_('Meanwhile, the following reviewers have been removed: %s') %
                     (', '.join(u.username for u in other_removed)),
                     category='warning')
         if _form['updaterev']:
             return self.create_new_iteration(pull_request,
                                       _form['updaterev'],
                                       _form['pullrequest_title'],
                                       _form['pullrequest_desc'],
                                       reviewer_ids)
                                       new_reviewers)
         added_reviewers = new_reviewers - old_reviewers - cur_reviewers
         removed_reviewers = (old_reviewers - new_reviewers) & cur_reviewers
         old_description = pull_request.description
         pull_request.title = _form['pullrequest_title']
         pull_request.description = _form['pullrequest_desc'].strip() or _('No description')
         pull_request.owner = User.get_by_username(_form['owner'])
         user = User.get(request.authuser.user_id)
         add_reviewer_ids = reviewer_ids - org_reviewer_ids - current_reviewer_ids
         remove_reviewer_ids = (org_reviewer_ids - reviewer_ids) & current_reviewer_ids
         try:
             PullRequestModel().mention_from_description(user, pull_request, old_description)
             PullRequestModel().add_reviewers(user, pull_request, add_reviewer_ids)
             PullRequestModel().remove_reviewers(user, pull_request, remove_reviewer_ids)
         except UserInvalidException as u:
             h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
             raise HTTPBadRequest()
         PullRequestModel().mention_from_description(user, pull_request, old_description)
         PullRequestModel().add_reviewers(user, pull_request, added_reviewers)
         PullRequestModel().remove_reviewers(user, pull_request, removed_reviewers)
         Session().commit()
         h.flash(_('Pull request updated'), category='success')
         raise HTTPFound(location=pull_request.url())
     @LoginRequired()
     @NotAnonymous()
     @HasRepoPermissionLevelDecorator('read')
     @jsonify
     def delete(self, repo_name, pull_request_id):
         pull_request = PullRequest.get_or_404(pull_request_id)

kallithea/lib/exceptions.py

➞

Show inline comments

@@ @@ -90,22 +90,18 @@ class HTTPLockedRC(HTTPClientError): @@
                                          'user `%s`' % (reponame, username))
         super(HTTPLockedRC, self).__init__(*args, **kwargs)
 class IMCCommitError(Exception):
     pass
 class UserCreationError(Exception):
     pass
 class UserInvalidException(Exception):
     pass
 class RepositoryCreationError(Exception):
     pass
 class HgsubversionImportError(Exception):
     pass

kallithea/model/pull_request.py

➞

Show inline comments

@@ @@ -25,49 +25,45 @@ Original author and date, and relevant c @@
 :license: GPLv3, see LICENSE.md for more details.
 """
 import logging
 import datetime
 from pylons.i18n.translation import _
 from sqlalchemy.orm import joinedload
 from kallithea.model.meta import Session
 from kallithea.lib import helpers as h
 from kallithea.lib.exceptions import UserInvalidException
 from kallithea.model.db import PullRequest, PullRequestReviewer, Notification, \
-    ChangesetStatus, User
     ChangesetStatus
 from kallithea.model.notification import NotificationModel
 from kallithea.lib.utils2 import extract_mentioned_users, safe_unicode
 log = logging.getLogger(__name__)
 class PullRequestModel(object):
 def _assert_valid_reviewers(seq):
     """Sanity check: elements are actual User objects, and not the default user."""
     assert not any(user.is_default_user for user in seq)
     def _get_valid_reviewers(self, seq):
         """ Generate User objects from a sequence of user IDs, usernames or
         User objects. Raises UserInvalidException if the DEFAULT user is
         specified, or if a given ID or username does not match any user.
         """
         for user_spec in seq:
             user = User.guess_instance(user_spec)
             if user is None or user.is_default_user:
                 raise UserInvalidException(user_spec)
             yield user
 class PullRequestModel(object):
     def create(self, created_by, org_repo, org_ref, other_repo, other_ref,
                revisions, title, description, reviewers):
         reviewers = set(reviewers)
         _assert_valid_reviewers(reviewers)
         pr = PullRequest()
         pr.org_repo = org_repo
         pr.org_ref = org_ref
         pr.other_repo = other_repo
         pr.other_ref = other_ref
         pr.revisions = revisions
         pr.title = title
         pr.description = description
         pr.owner = created_by
         Session().add(pr)
         Session().flush() # make database assign pull_request_id
@@ @@ -81,36 +77,40 @@ class PullRequestModel(object): @@
             pull_request=pr,
             send_email=False,
             status_change=ChangesetStatus.STATUS_UNDER_REVIEW,
+        )
         ChangesetStatusModel().set_status(
             org_repo,
             ChangesetStatus.STATUS_UNDER_REVIEW,
             created_by,
             comment,
             pull_request=pr,
+        )
         reviewers = set(self._get_valid_reviewers(reviewers))
         mention_recipients = extract_mentioned_users(description)
         self.add_reviewers(created_by, pr, reviewers, mention_recipients)
         return pr
     def add_reviewers(self, user, pr, reviewers, mention_recipients=None):
         """Add reviewer and send notification to them.
         """
         reviewer_users = set(self._get_valid_reviewers(reviewers))
         reviewers = set(reviewers)
         _assert_valid_reviewers(reviewers)
         if mention_recipients is not None:
             mention_recipients = set(mention_recipients) - reviewers
             _assert_valid_reviewers(mention_recipients)
         #members
-        for reviewer in reviewer_users:
+        for reviewer in reviewers:
             prr = PullRequestReviewer(reviewer, pr)
             Session().add(prr)
         #notification to reviewers
         pr_url = pr.url(canonical=True)
         threading = ['%s-pr-%s@%s' % (pr.other_repo.repo_name,
                                       pr.pull_request_id,
                                       h.canonical_hostname())]
         subject = safe_unicode(
             h.link_to(
               _('%(user)s wants you to review pull request %(pr_nice_id)s: %(pr_title)s') % \
                 {'user': user.username,
@@ @@ -142,48 +142,44 @@ class PullRequestModel(object): @@
             'pr_owner': pr.owner,
             'pr_owner_username': pr.owner.username,
             'pr_username': user.username,
             'threading': threading,
             'is_mention': False,
+            }
         if reviewers:
             NotificationModel().create(created_by=user, subject=subject, body=body,
                                        recipients=reviewers,
                                        type_=Notification.TYPE_PULL_REQUEST,
                                        email_kwargs=email_kwargs)
         mention_recipient_users = set()
         if mention_recipients:
             mention_recipient_users = set(self._get_valid_reviewers(mention_recipients))
             mention_recipient_users.difference_update(reviewers)
         if mention_recipient_users:
             email_kwargs['is_mention'] = True
             subject = _('[Mention]') + ' ' + subject
             # FIXME: this subject is wrong and unused!
             NotificationModel().create(created_by=user, subject=subject, body=body,
-                                       recipients=mention_recipient_users,
+                                       recipients=mention_recipients,
                                        type_=Notification.TYPE_PULL_REQUEST,
                                        email_kwargs=email_kwargs)
     def mention_from_description(self, user, pr, old_description=''):
         mention_recipients = (extract_mentioned_users(pr.description) -
                               extract_mentioned_users(old_description))
         log.debug("Mentioning %s", mention_recipients)
         self.add_reviewers(user, pr, set(), mention_recipients)
     def remove_reviewers(self, user, pull_request, reviewer_ids):
         """Remove users in the given user_id list from being reviewers of the PR."""
     def remove_reviewers(self, user, pull_request, reviewers):
         """Remove specified users from being reviewers of the PR."""
         PullRequestReviewer.query() \
             .filter_by(pull_request=pull_request) \
-            .filter(PullRequestReviewer.user_id.in_(reviewer_ids)) \
+            .filter(PullRequestReviewer.user_id.in_(r.user_id for r in reviewers)) \
             .delete(synchronize_session='fetch') # the default of 'evaluate' is not available
     def delete(self, pull_request):
         pull_request = PullRequest.guess_instance(pull_request)
         Session().delete(pull_request)
     def close_pull_request(self, pull_request):
         pull_request = PullRequest.guess_instance(pull_request)
         pull_request.status = PullRequest.STATUS_CLOSED
         pull_request.updated_on = datetime.datetime.now()

0 comments (0 inline, 0 general)