# HG changeset patch # User Søren Løvborg # Date 1488551671 -3600 # Node ID 5d60c9a391cd49ac47695dce7856a335f6da5f7d # Parent 33b71a130b16340262420a57ab99dfadff6d72cb pullrequests: introduce "action objects" for PR creation Inspired by the command design pattern, this attempts the following: * move policy and business logic from controllers into the model, * move validation, authorization and execution logic closer together in the code, * establish a reusable pattern for modelling higher-level concepts (like "create a new PR iteration"), * make error handling more well-defined, and independent of the controller layer, and * provide clear separation between, one one hand, the validation and authorization of a request, and on the other, the actual execution. diff -r 33b71a130b16 -r 5d60c9a391cd kallithea/controllers/pullrequests.py --- 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 diff -r 33b71a130b16 -r 5d60c9a391cd kallithea/model/pull_request.py --- 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