--- a/kallithea/controllers/pullrequests.py	Tue Feb 28 17:19:00 2017 +0100
+++ b/kallithea/controllers/pullrequests.py	Fri Mar 03 15:34:31 2017 +0100
@@ -28,7 +28,6 @@
 import logging
 import traceback
 import formencode
-import re
 
 from pylons import request, tmpl_context as c
 from pylons.i18n.translation import _
@@ -48,7 +47,7 @@
 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.pull_request import CreatePullRequestAction, CreatePullRequestIterationAction, PullRequestModel
 from kallithea.model.meta import Session
 from kallithea.model.repo import RepoModel
 from kallithea.model.comment import ChangesetCommentsModel
@@ -331,181 +330,55 @@
         # heads up: org and other might seem backward here ...
         org_ref = _form['org_ref'] # will have merge_rev as rev but symbolic name
         org_repo = Repository.guess_instance(_form['org_repo'])
-        (org_ref_type,
-         org_ref_name,
-         org_rev) = org_ref.split(':')
-        org_display = h.short_ref(org_ref_type, org_ref_name)
-        if org_ref_type == 'rev':
-            cs = org_repo.scm_instance.get_changeset(org_rev)
-            org_ref = 'branch:%s:%s' % (cs.branch, cs.raw_id)
 
         other_ref = _form['other_ref'] # will have symbolic name and head revision
         other_repo = Repository.guess_instance(_form['other_repo'])
-        (other_ref_type,
-         other_ref_name,
-         other_rev) = other_ref.split(':')
-        if other_ref_type == 'rev':
-            cs = other_repo.scm_instance.get_changeset(other_rev)
-            other_ref_name = cs.raw_id[:12]
-            other_ref = '%s:%s:%s' % (other_ref_type, other_ref_name, cs.raw_id)
-        other_display = h.short_ref(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, other_rev, # org and other "swapped"
-                                              org_repo.scm_instance, org_rev,
-                                              )
-        ancestor_rev = msg = None
-        if not cs_ranges:
-            msg = _('Cannot create empty pull request')
-        elif not ancestor_revs:
-            ancestor_rev = org_repo.scm_instance.EMPTY_CHANGESET
-        elif len(ancestor_revs) == 1:
-            ancestor_rev = ancestor_revs[0]
-        else:
-            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)
 
         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')
+        description = _form['pullrequest_desc'].strip()
+        owner = User.get(request.authuser.user_id)
+
         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, reviewers)
+            cmd = CreatePullRequestAction(org_repo, other_repo, org_ref, other_ref, title, description, owner, reviewers)
+        except CreatePullRequestAction.ValidationError as e:
+            h.flash(str(e), category='error', logf=log.error)
+            raise HTTPNotFound
+
+        try:
+            pull_request = cmd.execute()
             Session().commit()
-            h.flash(_('Successfully opened new pull request'),
-                    category='success')
         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))
 
+        h.flash(_('Successfully opened new pull request'),
+                category='success')
         raise HTTPFound(location=pull_request.url())
 
     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)
-        ancestor_rev = msg = None
-        if not cs_ranges:
-            msg = _('Cannot create empty pull request update') # cannot happen!
-        elif not ancestor_revs:
-            msg = _('Cannot create pull request update - no common ancestor found') # cannot happen
-        elif len(ancestor_revs) == 1:
-            ancestor_rev = ancestor_revs[0]
-        else:
-            msg = _('Cannot create pull request update - 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)
+        owner = User.get(request.authuser.user_id)
+        new_org_rev = self._get_ref_rev(old_pull_request.org_repo, 'rev', new_rev)
+        try:
+            cmd = CreatePullRequestIterationAction(old_pull_request, new_org_rev, title, description, owner, reviewers)
+        except CreatePullRequestAction.ValidationError as e:
+            h.flash(str(e), category='error', logf=log.error)
             raise HTTPNotFound
 
-        old_revisions = set(old_pull_request.revisions)
-        revisions = [cs.raw_id for cs in cs_ranges]
-        new_revisions = [r for r in revisions if r not in old_revisions]
-        lost = old_revisions.difference(revisions)
-
-        infos = ['This is a new iteration of %s "%s".' %
-                 (h.canonical_url('pullrequest_show', repo_name=old_pull_request.other_repo.repo_name,
-                      pull_request_id=old_pull_request.pull_request_id),
-                  old_pull_request.title)]
-
-        if lost:
-            infos.append(_('Missing changesets since the previous iteration:'))
-            for r in old_pull_request.revisions:
-                if r in lost:
-                    rev_desc = org_repo.get_changeset(r).message.split('\n')[0]
-                    infos.append('  %s %s' % (h.short_id(r), rev_desc))
-
-        if new_revisions:
-            infos.append(_('New changesets on %s %s since the previous iteration:') % (org_ref_type, org_ref_name))
-            for r in reversed(revisions):
-                if r in new_revisions:
-                    rev_desc = org_repo.get_changeset(r).message.split('\n')[0]
-                    infos.append('  %s %s' % (h.short_id(r), h.shorter(rev_desc, 80)))
-
-            if ancestor_rev == other_rev:
-                infos.append(_("Ancestor didn't change - diff since previous iteration:"))
-                infos.append(h.canonical_url('compare_url',
-                                 repo_name=org_repo.repo_name, # other_repo is always same as repo_name
-                                 org_ref_type='rev', org_ref_name=h.short_id(org_rev), # use old org_rev as base
-                                 other_ref_type='rev', other_ref_name=h.short_id(new_org_rev),
-                                 )) # note: linear diff, merge or not doesn't matter
-            else:
-                infos.append(_('This iteration is based on another %s revision and there is no simple diff.') % other_ref_name)
-        else:
-           infos.append(_('No changes found on %s %s since previous iteration.') % (org_ref_type, org_ref_name))
-           # TODO: fail?
-
-        # hack: ancestor_rev is not an other_ref but we want to show the
-        # requested destination and have the exact ancestor
-        new_other_ref = '%s:%s:%s' % (other_ref_type, other_ref_name, ancestor_rev)
-        new_org_ref = '%s:%s:%s' % (org_ref_type, org_ref_name, new_org_rev)
-
         try:
-            title, old_v = re.match(r'(.*)\(v(\d+)\)\s*$', title).groups()
-            v = int(old_v) + 1
-        except (AttributeError, ValueError):
-            v = 2
-        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, reviewers)
+            pull_request = cmd.execute()
+            Session().commit()
         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)
-        PullRequestModel().close_pull_request(old_pull_request.pull_request_id)
-
-        Session().commit()
         h.flash(_('New pull request iteration created'),
                 category='success')
-
         raise HTTPFound(location=pull_request.url())
 
     # pullrequest_post for PR editing

--- a/kallithea/model/pull_request.py	Tue Feb 28 17:19:00 2017 +0100
+++ b/kallithea/model/pull_request.py	Fri Mar 03 15:34:31 2017 +0100
@@ -27,7 +27,9 @@
 
 import logging
 import datetime
+import re
 
+from pylons import request
 from pylons.i18n.translation import _
 
 from sqlalchemy.orm import joinedload
@@ -35,7 +37,7 @@
 from kallithea.model.meta import Session
 from kallithea.lib import helpers as h
 from kallithea.model.db import PullRequest, PullRequestReviewer, Notification, \
-    ChangesetStatus
+    ChangesetStatus, User
 from kallithea.model.notification import NotificationModel
 from kallithea.lib.utils2 import extract_mentioned_users, safe_unicode
 
@@ -50,47 +52,6 @@
 
 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
-
-        #reset state to under-review
-        from kallithea.model.changeset_status import ChangesetStatusModel
-        from kallithea.model.comment import ChangesetCommentsModel
-        comment = ChangesetCommentsModel().create(
-            text=u'',
-            repo=org_repo,
-            author=created_by,
-            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,
-        )
-
-        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.
         """
@@ -183,3 +144,239 @@
         pull_request = PullRequest.guess_instance(pull_request)
         pull_request.status = PullRequest.STATUS_CLOSED
         pull_request.updated_on = datetime.datetime.now()
+
+
+class CreatePullRequestAction(object):
+
+    class ValidationError(Exception):
+        pass
+
+    class Empty(ValidationError):
+        pass
+
+    class AmbiguousAncestor(ValidationError):
+        pass
+
+    class Unauthorized(ValidationError):
+        pass
+
+    @staticmethod
+    def is_user_authorized(org_repo, other_repo):
+        """Performs authorization check with only the minimum amount of
+        information needed for such a check, rather than a full command
+        object.
+        """
+        if (h.HasRepoPermissionLevel('read')(org_repo.repo_name) and
+            h.HasRepoPermissionLevel('read')(other_repo.repo_name)):
+            return True
+
+        return False
+
+    def __init__(self, org_repo, other_repo, org_ref, other_ref, title, description, owner, reviewers):
+        from kallithea.controllers.compare import CompareController
+        reviewers = set(reviewers)
+        _assert_valid_reviewers(reviewers)
+
+        (org_ref_type,
+         org_ref_name,
+         org_rev) = org_ref.split(':')
+        org_display = h.short_ref(org_ref_type, org_ref_name)
+        if org_ref_type == 'rev':
+            cs = org_repo.scm_instance.get_changeset(org_rev)
+            org_ref = 'branch:%s:%s' % (cs.branch, cs.raw_id)
+
+        (other_ref_type,
+         other_ref_name,
+         other_rev) = other_ref.split(':')
+        if other_ref_type == 'rev':
+            cs = other_repo.scm_instance.get_changeset(other_rev)
+            other_ref_name = cs.raw_id[:12]
+            other_ref = '%s:%s:%s' % (other_ref_type, other_ref_name, cs.raw_id)
+        other_display = h.short_ref(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, other_rev, # org and other "swapped"
+                                              org_repo.scm_instance, org_rev,
+                                              )
+        if not cs_ranges:
+            raise self.Empty(_('Cannot create empty pull request'))
+
+        if not ancestor_revs:
+            ancestor_rev = org_repo.scm_instance.EMPTY_CHANGESET
+        elif len(ancestor_revs) == 1:
+            ancestor_rev = ancestor_revs[0]
+        else:
+            raise self.AmbiguousAncestor(
+                _('Cannot create pull request - criss cross merge detected, please merge a later %s revision to %s')
+                % (other_ref_name, org_ref_name))
+
+        self.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)
+
+        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 = description or _('No description')
+
+        self.org_repo = org_repo
+        self.other_repo = other_repo
+        self.org_ref = org_ref
+        self.other_ref = other_ref
+        self.title = title
+        self.description = description
+        self.owner = owner
+        self.reviewers = reviewers
+
+        if not CreatePullRequestAction.is_user_authorized(self.org_repo, self.other_repo):
+            raise self.Unauthorized(_('You are not authorized to create the pull request'))
+
+    def execute(self):
+        created_by = User.get(request.authuser.user_id)
+
+        pr = PullRequest()
+        pr.org_repo = self.org_repo
+        pr.org_ref = self.org_ref
+        pr.other_repo = self.other_repo
+        pr.other_ref = self.other_ref
+        pr.revisions = self.revisions
+        pr.title = self.title
+        pr.description = self.description
+        pr.owner = self.owner
+        Session().add(pr)
+        Session().flush() # make database assign pull_request_id
+
+        #reset state to under-review
+        from kallithea.model.changeset_status import ChangesetStatusModel
+        from kallithea.model.comment import ChangesetCommentsModel
+        comment = ChangesetCommentsModel().create(
+            text=u'',
+            repo=self.org_repo,
+            author=created_by,
+            pull_request=pr,
+            send_email=False,
+            status_change=ChangesetStatus.STATUS_UNDER_REVIEW,
+        )
+        ChangesetStatusModel().set_status(
+            self.org_repo,
+            ChangesetStatus.STATUS_UNDER_REVIEW,
+            created_by,
+            comment,
+            pull_request=pr,
+        )
+
+        mention_recipients = extract_mentioned_users(self.description)
+        PullRequestModel().add_reviewers(created_by, pr, self.reviewers, mention_recipients)
+
+        return pr
+
+
+class CreatePullRequestIterationAction(object):
+    @staticmethod
+    def is_user_authorized(old_pull_request):
+        """Performs authorization check with only the minimum amount of
+        information needed for such a check, rather than a full command
+        object.
+        """
+        if h.HasPermissionAny('hg.admin')():
+            return True
+
+        # Authorized to edit the old PR?
+        if request.authuser.user_id != old_pull_request.owner_id:
+            return False
+
+        # Authorized to create a new PR?
+        if not CreatePullRequestAction.is_user_authorized(old_pull_request.org_repo, old_pull_request.other_repo):
+            return False
+
+        return True
+
+    def __init__(self, old_pull_request, new_org_rev, title, description, owner, reviewers):
+        self.old_pull_request = old_pull_request
+
+        org_repo = old_pull_request.org_repo
+        org_ref_type, org_ref_name, org_rev = old_pull_request.org_ref.split(':')
+
+        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_org_ref = '%s:%s:%s' % (org_ref_type, org_ref_name, new_org_rev)
+
+        self.create_action = CreatePullRequestAction(org_repo, other_repo, new_org_ref, old_pull_request.other_ref, None, None, owner, reviewers)
+
+        # Generate complete title/description
+
+        old_revisions = set(old_pull_request.revisions)
+        revisions = self.create_action.revisions
+        new_revisions = [r for r in revisions if r not in old_revisions]
+        lost = old_revisions.difference(revisions)
+
+        infos = ['This is a new iteration of %s "%s".' %
+                 (h.canonical_url('pullrequest_show', repo_name=old_pull_request.other_repo.repo_name,
+                      pull_request_id=old_pull_request.pull_request_id),
+                  old_pull_request.title)]
+
+        if lost:
+            infos.append(_('Missing changesets since the previous iteration:'))
+            for r in old_pull_request.revisions:
+                if r in lost:
+                    rev_desc = org_repo.get_changeset(r).message.split('\n')[0]
+                    infos.append('  %s %s' % (h.short_id(r), rev_desc))
+
+        if new_revisions:
+            infos.append(_('New changesets on %s %s since the previous iteration:') % (org_ref_type, org_ref_name))
+            for r in reversed(revisions):
+                if r in new_revisions:
+                    rev_desc = org_repo.get_changeset(r).message.split('\n')[0]
+                    infos.append('  %s %s' % (h.short_id(r), h.shorter(rev_desc, 80)))
+
+            if self.create_action.other_ref == old_pull_request.other_ref:
+                infos.append(_("Ancestor didn't change - diff since previous iteration:"))
+                infos.append(h.canonical_url('compare_url',
+                                 repo_name=org_repo.repo_name, # other_repo is always same as repo_name
+                                 org_ref_type='rev', org_ref_name=h.short_id(org_rev), # use old org_rev as base
+                                 other_ref_type='rev', other_ref_name=h.short_id(new_org_rev),
+                                 )) # note: linear diff, merge or not doesn't matter
+            else:
+                infos.append(_('This iteration is based on another %s revision and there is no simple diff.') % other_ref_name)
+        else:
+           infos.append(_('No changes found on %s %s since previous iteration.') % (org_ref_type, org_ref_name))
+           # TODO: fail?
+
+        try:
+            title, old_v = re.match(r'(.*)\(v(\d+)\)\s*$', title).groups()
+            v = int(old_v) + 1
+        except (AttributeError, ValueError):
+            v = 2
+        self.create_action.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()
+        self.create_action.description = description
+
+        if not CreatePullRequestIterationAction.is_user_authorized(self.old_pull_request):
+            raise CreatePullRequestAction.Unauthorized(_('You are not authorized to create the pull request'))
+
+    def execute(self):
+        pull_request = self.create_action.execute()
+
+        # Close old iteration
+        from kallithea.model.comment import ChangesetCommentsModel
+        ChangesetCommentsModel().create(
+            text=_('Closed, next iteration: %s .') % pull_request.url(canonical=True),
+            repo=self.old_pull_request.other_repo_id,
+            author=request.authuser.user_id,
+            pull_request=self.old_pull_request.pull_request_id,
+            closing_pr=True)
+        PullRequestModel().close_pull_request(self.old_pull_request.pull_request_id)
+        return pull_request