# HG changeset patch # User Søren Løvborg # Date 2017-04-11 14:37:43 # Node ID 0452c45a546cdac2de58ef30135b8e25327ec63c # Parent e075f2cc4f8c8a3b0f2992ea61eccb0ca87ae598 pullrequests: fix broken "new PR iteration" handling of ancestor changes An earlier refactor (5d60c9a391cd) flubbed this code and accidentally assumed the most recent common ancestor would not change when iterating, which is of course only the case when there are no merges from 'other' to 'org' among the new revisions. This was not only not caught during manual testing (nor review), but neither did the test suite test this. That has now also been rectified. diff --git a/kallithea/controllers/pullrequests.py b/kallithea/controllers/pullrequests.py --- a/kallithea/controllers/pullrequests.py +++ b/kallithea/controllers/pullrequests.py @@ -362,8 +362,9 @@ class PullrequestsController(BaseRepoCon def create_new_iteration(self, old_pull_request, new_rev, title, description, reviewers): owner = User.get(request.authuser.user_id) new_org_rev = self._get_ref_rev(old_pull_request.org_repo, 'rev', new_rev) + new_other_rev = self._get_ref_rev(old_pull_request.other_repo, old_pull_request.other_ref_parts[0], old_pull_request.other_ref_parts[1]) try: - cmd = CreatePullRequestIterationAction(old_pull_request, new_org_rev, title, description, owner, reviewers) + cmd = CreatePullRequestIterationAction(old_pull_request, new_org_rev, new_other_rev, title, description, owner, reviewers) except CreatePullRequestAction.ValidationError as e: h.flash(str(e), category='error', logf=log.error) raise HTTPNotFound 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 @@ -297,7 +297,7 @@ class CreatePullRequestIterationAction(o return True - def __init__(self, old_pull_request, new_org_rev, title, description, owner, reviewers): + def __init__(self, old_pull_request, new_org_rev, new_other_rev, title, description, owner, reviewers): self.old_pull_request = old_pull_request org_repo = old_pull_request.org_repo @@ -308,8 +308,9 @@ class CreatePullRequestIterationAction(o #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) + new_other_ref = '%s:%s:%s' % (other_ref_type, other_ref_name, new_other_rev) - self.create_action = CreatePullRequestAction(org_repo, other_repo, new_org_ref, old_pull_request.other_ref, None, None, owner, reviewers) + self.create_action = CreatePullRequestAction(org_repo, other_repo, new_org_ref, new_other_ref, None, None, owner, reviewers) # Generate complete title/description diff --git a/kallithea/tests/functional/test_pullrequests.py b/kallithea/tests/functional/test_pullrequests.py --- a/kallithea/tests/functional/test_pullrequests.py +++ b/kallithea/tests/functional/test_pullrequests.py @@ -5,7 +5,7 @@ from tg.util.webtest import test_context from kallithea.tests.base import * from kallithea.tests.fixture import Fixture -from kallithea.model.db import User +from kallithea.model.db import PullRequest, User from kallithea.model.meta import Session from kallithea.controllers.pullrequests import PullrequestsController @@ -208,6 +208,87 @@ class TestPullrequestsController(TestCon status=400) response.mustcontain('Invalid reviewer "%s" specified' % invalid_user_id) + + def test_iteration_refs(self): + # Repo graph excerpt: + # o fb95b340e0d0 webvcs + # /: + # o : 41d2568309a0 default + # : : + # : o 5ec21f21aafe webvcs + # : : + # : o 9e6119747791 webvcs + # : : + # o : 3d1091ee5a53 default + # :/ + # o 948da46b29c1 default + + self.log_user() + + # create initial PR + response = self.app.post( + url(controller='pullrequests', action='create', repo_name=HG_REPO), + { + 'org_repo': HG_REPO, + 'org_ref': 'rev:9e6119747791:9e6119747791ff886a5abe1193a730b6bf874e1c', + 'other_repo': HG_REPO, + 'other_ref': 'branch:default:3d1091ee5a533b1f4577ec7d8a226bb315fb1336', + 'pullrequest_title': 'title', + 'pullrequest_desc': 'description', + '_authentication_token': self.authentication_token(), + }, + status=302) + pr1_id = int(re.search('/pull-request/(\d+)/', response.location).group(1)) + pr1 = PullRequest.get(pr1_id) + + assert pr1.org_ref == 'branch:webvcs:9e6119747791ff886a5abe1193a730b6bf874e1c' + assert pr1.other_ref == 'branch:default:948da46b29c125838a717f6a8496eb409717078d' + + Session().rollback() # invalidate loaded PR objects before issuing next request. + + # create PR 2 (new iteration with same ancestor) + response = self.app.post( + url(controller='pullrequests', action='post', repo_name=HG_REPO, pull_request_id=pr1_id), + { + 'updaterev': '5ec21f21aafe95220f1fc4843a4a57c378498b71', + 'pullrequest_title': 'title', + 'pullrequest_desc': 'description', + 'owner': TEST_USER_REGULAR_LOGIN, + '_authentication_token': self.authentication_token(), + }, + status=302) + pr2_id = int(re.search('/pull-request/(\d+)/', response.location).group(1)) + pr1 = PullRequest.get(pr1_id) + pr2 = PullRequest.get(pr2_id) + + assert pr2_id != pr1_id + assert pr1.status == PullRequest.STATUS_CLOSED + assert pr2.org_ref == 'branch:webvcs:5ec21f21aafe95220f1fc4843a4a57c378498b71' + assert pr2.other_ref == pr1.other_ref + + Session().rollback() # invalidate loaded PR objects before issuing next request. + + # create PR 3 (new iteration with new ancestor) + response = self.app.post( + url(controller='pullrequests', action='post', repo_name=HG_REPO, pull_request_id=pr2_id), + { + 'updaterev': 'fb95b340e0d03fa51f33c56c991c08077c99303e', + 'pullrequest_title': 'title', + 'pullrequest_desc': 'description', + 'owner': TEST_USER_REGULAR_LOGIN, + '_authentication_token': self.authentication_token(), + }, + status=302) + pr3_id = int(re.search('/pull-request/(\d+)/', response.location).group(1)) + pr2 = PullRequest.get(pr2_id) + pr3 = PullRequest.get(pr3_id) + + assert pr3_id != pr2_id + assert pr2.status == PullRequest.STATUS_CLOSED + assert pr3.org_ref == 'branch:webvcs:fb95b340e0d03fa51f33c56c991c08077c99303e' + assert pr3.other_ref == 'branch:default:41d2568309a05f422cffb8008e599d385f8af439' + + @pytest.mark.usefixtures("test_context_fixture") # apply fixture for all test methods class TestPullrequestsGetRepoRefs(TestController):