launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32527
[Merge] ~ines-almeida/launchpad:merge-button/add-merge-functionality into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:merge-button/add-merge-functionality into launchpad:master with ~ines-almeida/launchpad:merge-button/add-merge-type-model as a prerequisite.
Commit message:
Add merge functionality to merge proposals
Adds new endpoint that calls turnip backend to request a merge proposal to be merged. The merge is blocked by certain criteria and functionality is hidden behind feature flag.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/486054
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:merge-button/add-merge-functionality into launchpad:master.
diff --git a/lib/lp/code/errors.py b/lib/lp/code/errors.py
index d6c514f..9180797 100644
--- a/lib/lp/code/errors.py
+++ b/lib/lp/code/errors.py
@@ -19,7 +19,11 @@ __all__ = [
"BranchTypeError",
"BuildAlreadyPending",
"BuildNotAllowedForDistro",
+ "BranchMergeProposalConflicts",
"BranchMergeProposalExists",
+ "BranchMergeProposalFeatureDisabled",
+ "BranchMergeProposalMergeFailed",
+ "BranchMergeProposalNotMergeable",
"CannotDeleteBranch",
"CannotDeleteGitRepository",
"CannotHaveLinkedBranch",
@@ -261,6 +265,34 @@ class BranchMergeProposalExists(InvalidBranchMergeProposal):
self.existing_proposal = existing_proposal
+@error_status(http.client.CONFLICT)
+class BranchMergeProposalConflicts(Exception):
+ """Raised if merge proposal merge failed due to conflicts."""
+
+ pass
+
+
+@error_status(http.client.UNAUTHORIZED)
+class BranchMergeProposalFeatureDisabled(NotImplementedError):
+ """Raised if merge method is called while feature flag is disabled."""
+
+ pass
+
+
+@error_status(http.client.BAD_REQUEST)
+class BranchMergeProposalNotMergeable(Exception):
+ """Raised if merge proposal is not mergeable."""
+
+ pass
+
+
+@error_status(http.client.BAD_REQUEST)
+class BranchMergeProposalMergeFailed(Exception):
+ """Raised if merge proposal merge failed."""
+
+ pass
+
+
class InvalidNamespace(Exception):
"""Raised when someone tries to lookup a namespace with a bad name.
diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index 4de6d3a..49e6a24 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -594,6 +594,49 @@ class IBranchMergeProposalView(Interface):
and can eventually be merged.
"""
+ def isApproved():
+ """Check if the merge proposal has been approved.
+
+ A merge proposal is considered approved if it has at least one approval
+ vote from a trusted reviewer and no disapproval votes.
+ """
+
+ def CIChecksPassed():
+ """Check if all CI checks have passed for the latest pushed commit."""
+
+ def hasNoConflicts():
+ """Check if the merge proposal has any conflicts."""
+
+ def hasNoPendingPrerequisite():
+ """Check if the prerequisite branch has been merged into the target.
+
+ :raises NotImplementedError: If using Bazaar branches.
+ """
+
+ def diffIsUpToDate():
+ """Check if the preview diff is up to date, i.e., that there are no
+ pending diff update jobs.
+ """
+
+ def checkMergeCriteria():
+ """Check if the merge proposal meets all criteria for merging.
+
+ The hard criteria are:
+ - Is in progress (not merged, nor superseded, nor rejected)
+ - Has no conflicts
+ - Has no pending prerequisite
+ - Has no pending preview diff
+
+ The criteria skipped by using force=True tag:
+ - Has at least one approval and no disapprovals
+ - CI checks have passed
+
+ :param force: Whether to skip approval and CI checks
+
+ :return: A tuple of (bool, list) where the bool indicates if all
+ criteria are met, and the list contains failed criteria.
+ """
+
def getUnlandedSourceBranchRevisions():
"""Return a sequence of `BranchRevision` objects.
@@ -830,6 +873,30 @@ class IBranchMergeProposalEdit(Interface):
the details are updated.
"""
+ def personCanMerge(person):
+ """Check if a person has permission to merge this proposal.
+
+ We assume that a person can merge a proposal if they can push to the
+ target ref, as that would mean they have the necessary permissions
+ to manually merge.
+ """
+
+ @call_with(person=REQUEST_USER)
+ @export_write_operation()
+ @operation_for_version("devel")
+ def merge(person, force=False):
+ """Request to merge this proposal.
+
+ :param person: The person requesting the merge.
+ :param force: Whether to skip acceptance criteria.
+
+ :raises NotImplementedError: If using Bazaar branches.
+ :raises Unauthorized: If the person doesn't have permission to merge.
+ :raises BranchMergeProposalNotMergeable: If the proposal doesn't meet
+ all merge criteria.
+ :raises BranchMergeProposalMergeFailed: If the merge fails.
+ """
+
class IBranchMergeProposalAnyAllowedPerson(IBugLinkTarget):
@operation_parameters(
diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py
index 71c00ed..a1d20db 100644
--- a/lib/lp/code/interfaces/githosting.py
+++ b/lib/lp/code/interfaces/githosting.py
@@ -169,3 +169,28 @@ class IGitHostingClient(Interface):
service.
:param logger: An optional logger.
"""
+
+ def merge(
+ repo,
+ target_branch,
+ target_commit_sha1,
+ source_branch,
+ source_commit_sha1,
+ commiter,
+ commit_message,
+ source_repo=None,
+ logger=None,
+ ):
+ """Request a merge from source_branch to target_branch
+
+ :param repo: name of the target repository on the hosting service.
+ :param target_branch: name of target branch.
+ :param target_commit_sha1: commit set as target in target branch
+ :param source_branch: name of source branch, to be merged into target
+ :param source_commit_sha1: latest commit in source branch
+ :param committer: person that requests the merge
+ :param commit_message: custom message for merge commit
+ :param source_repo: name of the source repository on the hosting
+ service (if any)
+ :param logger: An optional logger.
+ """
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 0396b59..7b49aed 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -7,6 +7,7 @@ __all__ = [
"BranchMergeProposal",
"BranchMergeProposalGetter",
"is_valid_transition",
+ "PROPOSAL_MERGE_ENABLED_FEATURE_FLAG",
]
import sys
@@ -38,12 +39,17 @@ from lp.code.enums import (
BranchSubscriptionNotificationLevel,
CodeReviewNotificationLevel,
CodeReviewVote,
+ GitPermissionType,
MergeType,
+ RevisionStatusResult,
)
from lp.code.errors import (
BadBranchMergeProposalSearchContext,
BadStateTransition,
BranchMergeProposalExists,
+ BranchMergeProposalFeatureDisabled,
+ BranchMergeProposalMergeFailed,
+ BranchMergeProposalNotMergeable,
DiffNotFound,
UserNotBranchReviewer,
WrongBranchMergeProposal,
@@ -66,6 +72,7 @@ from lp.code.interfaces.codereviewcomment import ICodeReviewComment
from lp.code.interfaces.codereviewinlinecomment import (
ICodeReviewInlineCommentSet,
)
+from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.interfaces.gitref import IGitRef
from lp.code.mail.branch import RecipientReason
from lp.code.model.branchrevision import BranchRevision
@@ -88,6 +95,7 @@ from lp.services.database.enumcol import DBEnum
from lp.services.database.interfaces import IPrimaryStore, IStore
from lp.services.database.sqlbase import quote
from lp.services.database.stormbase import StormBase
+from lp.services.features import getFeatureFlag
from lp.services.helpers import shortlist
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
@@ -99,6 +107,8 @@ from lp.services.webapp.errorlog import ScriptRequest
from lp.services.xref.interfaces import IXRefSet
from lp.soyuz.enums import re_bug_numbers, re_lp_closes
+PROPOSAL_MERGE_ENABLED_FEATURE_FLAG = "merge_proposal.merge.enabled"
+
def is_valid_transition(proposal, from_state, next_state, user=None):
"""Is it valid for this user to move this proposal to to next_state?
@@ -854,6 +864,184 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
# or superseded, then it is valid to be merged.
return self.queue_status not in FINAL_STATES
+ def isApproved(self):
+ """See `IBranchMergeProposal`."""
+
+ # TODO ines-almeida 2025-05-09: the criteria for a merge proposal to
+ # be 'approved' should eventually be defined by the repository owners.
+ # For now we enforce that we need at least one approval from a trusted
+ # reviewer, and no disapprovals (regardless of reviewer)
+
+ dissaproved_states = [
+ CodeReviewVote.DISAPPROVE,
+ CodeReviewVote.NEEDS_FIXING,
+ CodeReviewVote.NEEDS_INFO,
+ CodeReviewVote.NEEDS_RESUBMITTING,
+ ]
+ approved = False
+ for vote in self.votes:
+ if not vote.comment:
+ continue
+ elif vote.comment.vote in dissaproved_states:
+ return False
+ elif (
+ vote.comment.vote == CodeReviewVote.APPROVE
+ and self.merge_target.isPersonTrustedReviewer(vote.reviewer)
+ ):
+ approved = True
+ return approved
+
+ def CIChecksPassed(self):
+ """See `IBranchMergeProposal`."""
+
+ # We don't run CI checks on bazaar repos
+ if self.source_branch is not None:
+ return True
+
+ reports = self.getStatusReports(self.source_git_commit_sha1)
+ if reports.is_empty():
+ return True
+
+ return reports.last().result == RevisionStatusResult.SUCCEEDED
+
+ def hasNoConflicts(self):
+ """See `IBranchMergeProposal`."""
+
+ if self.preview_diff is None:
+ return True
+
+ return (
+ self.preview_diff.conflicts is None
+ or self.preview_diff.conflicts == ""
+ )
+
+ def hasNoPendingPrerequisite(self):
+ """See `IBranchMergeProposal`."""
+
+ if self.merge_prerequisite is None:
+ return True
+
+ if self.target_branch:
+ raise NotImplementedError("Bazaar branches are not supported")
+
+ # Check if prerequisite has been merged into the target branch
+ # The response returns a list of commits that were merged, meaning
+ # that if the list is empty, the merge hasn't occured
+ hosting_client = getUtility(IGitHostingClient)
+ response = hosting_client.detectMerges(
+ self.target_git_repository_id,
+ self.target_git_commit_sha1,
+ [self.prerequisite_git_commit_sha1],
+ )
+ return bool(response)
+
+ def diffIsUpToDate(self):
+ """See `IBranchMergeProposal`."""
+
+ return self.next_preview_diff_job is None
+
+ def checkMergeCriteria(self, force=False):
+ """See `IBranchMergeProposal`."""
+
+ # TODO ines-almeida 2025-05-05: the mergeability criteria should be
+ # to some extent defined by the repository owner. While that
+ # capability is not in place, we will set a few hard rules for one
+ # to merge a MP from Launchpad
+
+ # List of hard rule criteria (defined by Launchpad)
+ criteria = [
+ (self.isInProgress, "Invalid status for merging"),
+ (self.hasNoConflicts, "Diff contains conflicts"),
+ (self.hasNoPendingPrerequisite, "Prerequisit branch not merged"),
+ (self.diffIsUpToDate, "New changes were pushed too recently"),
+ ]
+
+ # Criteria defined by project owners (skipped if 'force' merge)
+ if not force:
+ criteria.extend(
+ [
+ (self.isApproved, "Proposal has not been approved"),
+ (self.CIChecksPassed, "CI checks failed"),
+ ]
+ )
+
+ result = True
+ failed_checks = []
+ for check, error_message in criteria:
+ if not check():
+ failed_checks.append(error_message)
+ result = False
+
+ return result, failed_checks
+
+ def personCanMerge(self, person):
+ """See `IBranchMergeProposal`."""
+
+ if self.source_branch is not None:
+ raise NotImplementedError(
+ "Merging Bazaar branches is not supported"
+ )
+
+ permissions = self.target_git_repository.checkRefPermissions(
+ person, [self.target_git_path]
+ )
+ return GitPermissionType.CAN_PUSH in permissions[self.target_git_path]
+
+ def merge(self, person, force=False):
+ """See `IBranchMergeProposal`."""
+
+ if not getFeatureFlag(PROPOSAL_MERGE_ENABLED_FEATURE_FLAG):
+ raise BranchMergeProposalFeatureDisabled()
+
+ if self.source_branch is not None:
+ raise NotImplementedError(
+ "Merging Bazaar branches is not supported"
+ )
+
+ if not self.personCanMerge(person):
+ raise Unauthorized()
+
+ can_be_merged, failed_checks = self.checkMergeCriteria(force)
+ if not can_be_merged:
+ raise BranchMergeProposalNotMergeable(
+ "Merge proposal cannot be merged in its current state: %s"
+ % ", ".join(failed_checks)
+ )
+
+ hosting_client = getUtility(IGitHostingClient)
+ response = hosting_client.merge(
+ self.target_git_repository_id,
+ self.target_git_ref.name,
+ self.target_git_commit_sha1,
+ self.source_git_ref.name,
+ self.source_git_commit_sha1,
+ person,
+ self.commit_message,
+ source_repo=self.source_git_repository_id,
+ )
+
+ # It shouldn't be possible for the `merge_commit` to not exist in the
+ # response given the request didn't raise, but we do a check anyways.
+ if "merge_commit" not in response:
+ raise BranchMergeProposalMergeFailed(
+ "Failed to merge the proposal."
+ )
+
+ merge_commit = response["merge_commit"]
+ # Edge case, if `merge_commit` is None it means the merge proposal had
+ # already been merged by the time this endpoint was run. Might happen
+ # in the odd case that someone changes the status of a proposal from
+ # 'merged' back to another state and then tries merging again.
+ if merge_commit is None:
+ return
+
+ self.markAsMerged(
+ merge_reporter=person,
+ merged_revision_id=merge_commit,
+ date_merged=UTC_NOW,
+ merge_type=MergeType.REGULAR_MERGE,
+ )
+
def _reviewProposal(
self, reviewer, next_state, revision_id, _date_reviewed=None
):
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index 68a4a29..cc5f84b 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -19,6 +19,7 @@ from six import ensure_text
from zope.interface import implementer
from lp.code.errors import (
+ BranchMergeProposalConflicts,
CannotRepackRepository,
CannotRunGitGC,
GitReferenceDeletionFault,
@@ -261,6 +262,63 @@ class GitHostingClient:
"Failed to get merge diff from Git repository: %s" % str(e)
)
+ def merge(
+ self,
+ repo,
+ target_branch,
+ target_commit_sha1,
+ source_branch,
+ source_commit_sha1,
+ commiter,
+ commit_message,
+ source_repo=None,
+ logger=None,
+ ):
+ """See `IGitHostingClient`."""
+
+ if logger is not None:
+ logger.info(
+ "Requesting merge for %s from %s to %s" % repo,
+ quote(source_branch),
+ quote(target_branch),
+ )
+
+ if source_repo:
+ repo = f"{repo}:{source_repo}"
+
+ url = "/repo/%s/merge/%s:%s" % (
+ repo,
+ quote(target_branch),
+ quote(source_branch),
+ )
+
+ json_data = {
+ "committer_name": commiter.display_name,
+ "committer_email": commiter.preferredemail.email,
+ "commit_message": commit_message,
+ "target_commit_sha1": target_commit_sha1,
+ "source_commit_sha1": source_commit_sha1,
+ }
+
+ if logger is not None:
+ logger.info("Sending request to turnip '%s'" % url)
+
+ try:
+ return self._post(url, json=json_data)
+ except requests.RequestException as e:
+ if e.response is not None:
+ if e.response.status_code == 404:
+ raise GitRepositoryScanFault(
+ "Repository or branch not found"
+ )
+ elif e.response.status_code == 409:
+ raise BranchMergeProposalConflicts(
+ "Merge conflict detected"
+ )
+ raise GitRepositoryScanFault(
+ "Failed to merge from Git repository: %s" % str(e)
+ )
+
def detectMerges(
self, path, target, sources, previous_target=None, logger=None
):
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index e1ef1eb..c020e98 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -36,10 +36,14 @@ from lp.code.enums import (
CodeReviewNotificationLevel,
CodeReviewVote,
MergeType,
+ RevisionStatusResult,
)
from lp.code.errors import (
BadStateTransition,
BranchMergeProposalExists,
+ BranchMergeProposalFeatureDisabled,
+ BranchMergeProposalMergeFailed,
+ BranchMergeProposalNotMergeable,
DiffNotFound,
WrongBranchMergeProposal,
)
@@ -60,6 +64,7 @@ from lp.code.interfaces.branchmergeproposal import (
IBranchMergeProposalJobSource,
)
from lp.code.model.branchmergeproposal import (
+ PROPOSAL_MERGE_ENABLED_FEATURE_FLAG,
BranchMergeProposal,
BranchMergeProposalGetter,
is_valid_transition,
@@ -80,6 +85,7 @@ from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProductSet
from lp.services.config import config
from lp.services.database.constants import UTC_NOW
+from lp.services.features.testing import FeatureFixture
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.runner import JobRunner
from lp.services.webapp import canonical_url
@@ -3607,4 +3613,425 @@ class TestWebservice(WebServiceTestCase):
self.assertEqual(comment_date, inline_comment.get("date"))
+class TestBranchMergeProposalApproval(WithVCSScenarios, TestCaseWithFactory):
+ """Test the isApproved method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ team = self.factory.makeTeam()
+ target = self.makeBranch(owner=team)
+
+ self.registrant = self.factory.makePerson(member_of=[team])
+ self.reviewer = self.factory.makePerson(member_of=[team])
+
+ self.proposal = self.makeBranchMergeProposal(
+ target=target, registrant=self.registrant
+ )
+
+ def test_isApproved_no_votes(self):
+ # A proposal with no votes is not approved
+ self.assertFalse(self.proposal.isApproved())
+
+ def test_isApproved_with_untrusted_approval(self):
+ # A proposal with one untrusted approval is not approved
+ untrusted_reviewer = self.factory.makePerson()
+ with person_logged_in(untrusted_reviewer):
+ self.proposal.createComment(
+ owner=untrusted_reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.assertFalse(self.proposal.isApproved())
+
+ def test_isApproved_with_trusted_approval(self):
+ # A proposal with one trusted approval is approved
+ with person_logged_in(self.reviewer):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.assertTrue(self.proposal.isApproved())
+
+ def test_isApproved_with_disapproval_votes(self):
+ # A proposal with both approvals and disapprovals is not approved
+ with person_logged_in(self.reviewer):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+
+ with person_logged_in(self.registrant):
+ self.proposal.createComment(
+ owner=self.registrant,
+ vote=CodeReviewVote.DISAPPROVE,
+ )
+ self.assertFalse(self.proposal.isApproved())
+
+ def test_isApproved_approval_after_disapproval(self):
+ # If a person approves after disapproving a proposal, it counts having
+ # approved it
+ with person_logged_in(self.reviewer):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.DISAPPROVE,
+ )
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.assertTrue(self.proposal.isApproved())
+
+ def test_isApproved_disapproval_after_approval(self):
+ # If a person disapproves after approving a proposal, it counts having
+ # disapproved it
+ with person_logged_in(self.reviewer):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.DISAPPROVE,
+ )
+ self.assertFalse(self.proposal.isApproved())
+
+
+class TestBranchMergeProposalCIChecks(TestCaseWithFactory):
+ """Test the CIChecksPassed method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.proposal = self.factory.makeBranchMergeProposalForGit()
+
+ def test_CIChecksPassed_bazaar_repo(self):
+ # No CI checks on bazaar repos
+ proposal = self.factory.makeBranchMergeProposal()
+ self.assertTrue(proposal.CIChecksPassed())
+
+ def test_CIChecksPassed_no_reports(self):
+ # If there are no CI reports, checks are considered passed
+ self.assertTrue(self.proposal.CIChecksPassed())
+
+ def test_CIChecksPassed_success(self):
+ # If the latest CI report is successful, checks are passed
+ self.factory.makeRevisionStatusReport(
+ git_repository=self.proposal.source_git_repository,
+ commit_sha1=self.proposal.source_git_commit_sha1,
+ result=RevisionStatusResult.SUCCEEDED,
+ )
+ self.assertTrue(self.proposal.CIChecksPassed())
+
+ def test_CIChecksPassed_failure(self):
+ # If the latest CI report is a failure, checks are not passed
+ self.factory.makeRevisionStatusReport(
+ git_repository=self.proposal.source_git_repository,
+ commit_sha1=self.proposal.source_git_commit_sha1,
+ result=RevisionStatusResult.FAILED,
+ )
+ self.assertFalse(self.proposal.CIChecksPassed())
+
+ def test_CIChecksPassed_running(self):
+ # If the latest CI report is a failure, checks are not passed
+ self.factory.makeRevisionStatusReport(
+ git_repository=self.proposal.source_git_repository,
+ commit_sha1=self.proposal.source_git_commit_sha1,
+ result=RevisionStatusResult.RUNNING,
+ )
+ self.assertFalse(self.proposal.CIChecksPassed())
+
+
+class TestBranchMergeProposalConflicts(WithVCSScenarios, TestCaseWithFactory):
+ """Test the hasNoConflicts method of BranchMergeProposal."""
+
+ # layer = DatabaseFunctionalLayer
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.proposal = self.makeBranchMergeProposal()
+
+ def test_hasNoConflicts_no_diff(self):
+ # If there is no preview diff, there are no conflicts
+ self.assertTrue(self.proposal.hasNoConflicts())
+
+ def test_hasNoConflicts_no_conflicts(self):
+ # If the preview diff has no conflicts, hasNoConflicts returns False
+ self.factory.makePreviewDiff(merge_proposal=self.proposal)
+ transaction.commit()
+ self.assertTrue(self.proposal.hasNoConflicts())
+
+ def test_hasNoConflicts_with_conflicts(self):
+ # If the preview diff has conflicts, hasNoConflicts returns False
+ self.factory.makePreviewDiff(
+ merge_proposal=self.proposal,
+ conflicts="Merge conflicts found",
+ )
+ transaction.commit()
+ self.assertFalse(self.proposal.hasNoConflicts())
+
+
+class TestBranchMergeProposalPrerequisites(TestCaseWithFactory):
+ """Test hasNoPendingPrerequisite method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+
+ def test_hasNoPendingPrerequisite_no_prerequisite(self):
+ # If there is no prerequisite branch, there are no pending
+ # prerequisites
+ proposal = self.factory.makeBranchMergeProposalForGit()
+ self.assertTrue(proposal.hasNoPendingPrerequisite())
+
+ def test_hasNoPendingPrerequisite_merged(self):
+ # If the prerequisite has been merged, there are no pending
+ # prerequisites
+ merged_prerequisite = self.factory.makeGitRefs()[0]
+ proposal = self.factory.makeBranchMergeProposalForGit(
+ prerequisite_ref=merged_prerequisite
+ )
+
+ mock_requestMergesResponse = {
+ proposal.target_git_commit_sha1: merged_prerequisite.commit_sha1
+ }
+ self.useFixture(GitHostingFixture(merges=mock_requestMergesResponse))
+ self.assertTrue(proposal.hasNoPendingPrerequisite())
+
+ def test_hasNoPendingPrerequisite_not_merged(self):
+ # If the prerequisite has not been merged, there are pending
+ # prerequisites
+ prerequisite = self.factory.makeGitRefs()[0]
+ proposal = self.factory.makeBranchMergeProposalForGit(
+ prerequisite_ref=prerequisite
+ )
+
+ self.useFixture(GitHostingFixture())
+ self.assertFalse(proposal.hasNoPendingPrerequisite())
+
+ def test_hasNoPendingPrerequisite_no_prerequisits_bazaar(self):
+ # If there is no prerequisite branch, there are no pending
+ # prerequisites
+ proposal = self.factory.makeBranchMergeProposal()
+ self.assertTrue(proposal.hasNoPendingPrerequisite())
+
+ def test_hasNoPendingPrerequisite_bazaar(self):
+ # If there is a prerequisite branch in a bazaar MP, raise not
+ # Implemented
+ prerequisite = self.factory.makeBranch()
+ proposal = self.factory.makeBranchMergeProposal(
+ prerequisite_branch=prerequisite
+ )
+ self.assertRaises(
+ NotImplementedError,
+ proposal.hasNoPendingPrerequisite,
+ )
+
+
+class TestBranchMergeProposalDiffStatus(WithVCSScenarios, TestCaseWithFactory):
+ """Test the diffIsUpToDate method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.proposal = self.makeBranchMergeProposal()
+
+ def test_diffIsUpToDate_no_job(self):
+ # If there is no pending diff job, the diff is up to date
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ self.assertTrue(self.proposal.diffIsUpToDate())
+
+ def test_diffIsUpToDate_with_job(self):
+ # If there is a pending diff job, the diff is not up to date
+ self.assertFalse(self.proposal.diffIsUpToDate())
+
+
+class TestBranchMergeProposalMergeCriteria(
+ WithVCSScenarios, TestCaseWithFactory
+):
+ """Test the checkMergeCriteria method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+
+ team = self.factory.makeTeam()
+ target = self.makeBranch(owner=team)
+ self.reviewer = self.factory.makePerson(member_of=[team])
+
+ self.proposal = self.makeBranchMergeProposal(target=target)
+ self.useFixture(GitHostingFixture())
+
+ def test_checkMergeCriteria_all_passed(self):
+ # If all criteria are met, checkMergeCriteria returns (True, [])
+ with person_logged_in(self.reviewer):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ result, failed_checks = self.proposal.checkMergeCriteria()
+ self.assertTrue(result)
+ self.assertEqual([], failed_checks)
+
+ def test_checkMergeCriteria_some_failed(self):
+ # If some criteria are not met, checkMergeCriteria returns (False,
+ # [failed_checks])
+ result, failed_checks = self.proposal.checkMergeCriteria()
+ self.assertFalse(result)
+ self.assertTrue(len(failed_checks) > 0)
+
+
+class TestBranchMergeProposalMergePermissions(TestCaseWithFactory):
+ """Test the personCanMerge method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.proposal = self.factory.makeBranchMergeProposalForGit()
+ self.useFixture(GitHostingFixture())
+
+ def test_personCanMerge_no_permission(self):
+ # A person without push permission cannot merge
+ person = self.factory.makePerson()
+ proposal = removeSecurityProxy(self.proposal)
+ self.assertFalse(proposal.personCanMerge(person))
+
+ def test_personCanMerge_with_permission(self):
+ # A person with push permission can merge
+ person = self.factory.makePerson()
+ proposal = removeSecurityProxy(self.proposal)
+ rule = removeSecurityProxy(
+ self.factory.makeGitRule(repository=proposal.target_git_repository)
+ )
+ self.factory.makeGitRuleGrant(
+ rule=rule, grantee=person, can_create=True, can_push=True
+ )
+ self.assertTrue(proposal.personCanMerge(person))
+
+ def test_personCanMerge_bazaar(self):
+ # Check checking permissions for merging bazaar repo raises error
+ person = self.factory.makePerson()
+ proposal = removeSecurityProxy(self.factory.makeBranchMergeProposal())
+ self.assertRaises(
+ NotImplementedError,
+ proposal.personCanMerge,
+ person,
+ )
+
+
+class TestBranchMergeProposalMerge(TestCaseWithFactory):
+ """Test the requestMerge method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+ self.useFixture(
+ FeatureFixture({PROPOSAL_MERGE_ENABLED_FEATURE_FLAG: "on"})
+ )
+
+ self.proposal = removeSecurityProxy(
+ self.factory.makeBranchMergeProposalForGit()
+ )
+
+ self.person = self.factory.makePerson()
+ self.reviewer = self.proposal.target_git_repository.owner
+
+ rule = removeSecurityProxy(
+ self.factory.makeGitRule(
+ repository=self.proposal.target_git_repository
+ )
+ )
+ self.factory.makeGitRuleGrant(
+ rule=rule, grantee=self.person, can_create=True, can_push=True
+ )
+
+ def test_merge_feature_flag(self):
+ # Without feature flag enabled, merge fails
+
+ self.useFixture(
+ FeatureFixture({PROPOSAL_MERGE_ENABLED_FEATURE_FLAG: ""})
+ )
+
+ self.assertRaises(
+ BranchMergeProposalFeatureDisabled,
+ self.proposal.merge,
+ self.person,
+ )
+
+ def test_merge_success(self):
+ # A successful merge request updates the proposal status
+
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+
+ with person_logged_in(self.person):
+ self.proposal.merge(self.person)
+ self.assertEqual(
+ BranchMergeProposalStatus.MERGED,
+ self.proposal.queue_status,
+ )
+
+ def test_merge_no_permission(self):
+ # A person without permission cannot request a merge
+ person = self.factory.makePerson()
+ self.assertRaises(
+ Unauthorized,
+ self.proposal.merge,
+ person,
+ )
+
+ def test_merge_not_mergeable(self):
+ # A proposal that doesn't meet merge criteria cannot be merged
+ self.assertRaises(
+ BranchMergeProposalNotMergeable,
+ self.proposal.merge,
+ self.person,
+ )
+
+ def test_merge_bazaar_not_supported(self):
+ # Bazaar branches are not supported
+ proposal = removeSecurityProxy(self.factory.makeBranchMergeProposal())
+ self.assertRaises(
+ NotImplementedError,
+ proposal.merge,
+ self.person,
+ )
+
+ def test_merge_turnip_failure(self):
+ # Test merge failed from git hosting system
+
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+
+ self.hosting_fixture.merge.failure = BranchMergeProposalMergeFailed(
+ "Merge proposal failed to merge"
+ )
+
+ with person_logged_in(self.person):
+ self.assertRaises(
+ BranchMergeProposalMergeFailed,
+ self.proposal.merge,
+ self.person,
+ )
+
+
load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index f484afd..9a974f2 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -26,6 +26,7 @@ from zope.interface import implementer
from zope.security.proxy import removeSecurityProxy
from lp.code.errors import (
+ BranchMergeProposalConflicts,
CannotRepackRepository,
CannotRunGitGC,
GitReferenceDeletionFault,
@@ -37,6 +38,7 @@ from lp.code.errors import (
)
from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.model.githosting import RefCopyOperation
+from lp.code.tests.helpers import GitHostingFixture
from lp.services.job.interfaces.job import IRunnableJob, JobStatus
from lp.services.job.model.job import Job
from lp.services.job.runner import BaseRunnableJob, JobRunner
@@ -46,7 +48,7 @@ from lp.services.timeout import (
set_default_timeout_function,
)
from lp.services.webapp.url import urlappend
-from lp.testing import TestCase
+from lp.testing import TestCase, TestCaseWithFactory
from lp.testing.layers import ZopelessDatabaseLayer
@@ -704,3 +706,101 @@ class TestGitHostingClient(TestCase):
self.client.collectGarbage,
"/repo/123",
)
+
+
+class TestGitHostingClientMerge(TestCaseWithFactory):
+ """Test the merge method of GitHostingClient."""
+
+ layer = ZopelessDatabaseLayer
+
+ @contextmanager
+ def mockRequests(self, method, set_default_timeout=True, **kwargs):
+ with responses.RequestsMock() as requests_mock:
+ requests_mock.add(method, re.compile(r".*"), **kwargs)
+ original_timeout_function = get_default_timeout_function()
+ if set_default_timeout:
+ set_default_timeout_function(lambda: 60.0)
+ try:
+ yield
+ finally:
+ set_default_timeout_function(original_timeout_function)
+ self.requests = [call.request for call in requests_mock.calls]
+
+ def test_merge_success(self):
+ """Test successful merge"""
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+ self.client = getUtility(IGitHostingClient)
+
+ repository = self.factory.makeGitRepository()
+ person = self.factory.makePerson()
+ response = self.client.merge(
+ repository.id,
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ )
+ self.assertIn("merge_commit", response)
+
+ def test_merge_conflict(self):
+ """Test merge with conflicts"""
+ self.client = getUtility(IGitHostingClient)
+
+ repository = self.factory.makeGitRepository()
+ person = self.factory.makePerson()
+
+ with self.mockRequests("POST", status=409):
+ self.assertRaises(
+ BranchMergeProposalConflicts,
+ self.client.merge,
+ repository.id,
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ )
+
+ def test_merge_not_found(self):
+ """Test merge with non-existent repository"""
+ self.client = getUtility(IGitHostingClient)
+
+ person = self.factory.makePerson()
+ with self.mockRequests("POST", status=404):
+ self.assertRaises(
+ GitRepositoryScanFault,
+ self.client.merge,
+ "non-existent",
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ )
+
+ def test_merge_network_error(self):
+ """Test merge with network error"""
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+ self.hosting_fixture.merge.failure = GitRepositoryScanFault(
+ "Network error"
+ )
+ self.client = getUtility(IGitHostingClient)
+
+ repository = self.factory.makeGitRepository()
+ person = self.factory.makePerson()
+
+ self.assertRaises(
+ GitRepositoryScanFault,
+ self.client.merge,
+ repository.id,
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ )
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index cf0f74b..63e28a6 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -383,6 +383,9 @@ class GitHostingFixture(fixtures.Fixture):
self.detectMerges = fake_method_factory(
result=({} if merges is None else merges)
)
+ self.merge = fake_method_factory(
+ result=({"merge_commit": "fake-sha1"})
+ )
self.getBlob = fake_method_factory(result=blob)
self.delete = fake_method_factory()
self.disable_memcache = disable_memcache
diff --git a/lib/lp/services/features/flags.py b/lib/lp/services/features/flags.py
index f96ec91..d904851 100644
--- a/lib/lp/services/features/flags.py
+++ b/lib/lp/services/features/flags.py
@@ -322,6 +322,14 @@ flag_info = sorted(
"",
"",
),
+ (
+ "merge_proposal.merge.enabled",
+ "boolean",
+ "If true, users can merge their merge proposals using the API",
+ "",
+ "",
+ "",
+ ),
]
)
Follow ups