launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32614
[Merge] ~ines-almeida/launchpad:merge-button/expose-endpoints into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:merge-button/expose-endpoints into launchpad:master.
Commit message:
Expose endpoints to check merge feasibility
Created canIMerge and getMergeCriteria operations to assist frontend of merge button
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/487163
Ended up changing more things than initially expected, mostly to not expose something that we will later want to change the form. The return value of checkMergeCriteria was not very flexible, and with future work on merge criteria being set by repo owners etc, I thought I'd turn it into a flexible dict where we can later add more details without breaking the existing functionality.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:merge-button/expose-endpoints into launchpad:master.
diff --git a/lib/lp/code/enums.py b/lib/lp/code/enums.py
index a1912dc..d7bcf5c 100644
--- a/lib/lp/code/enums.py
+++ b/lib/lp/code/enums.py
@@ -28,6 +28,7 @@ __all__ = [
"GitRepositoryStatus",
"GitRepositoryType",
"MergeType",
+ "MergeProposalCriteria",
"NON_CVS_RCS_TYPES",
"RevisionControlSystems",
"RevisionStatusArtifactType",
@@ -671,6 +672,25 @@ class MergeType(DBEnumeratedType):
)
+class MergeProposalCriteria(EnumeratedType):
+ """Merge proposal merge criteria
+
+ Which checks we run when verifying that a merge proposal is mergeable
+ """
+
+ IS_IN_PROGRESS = Item("is_in_progress")
+
+ HAS_NO_CONFLICTS = Item("has_no_conflicts")
+
+ DIFF_IS_UP_TO_DATE = Item("diff_is_up_to_date")
+
+ HAS_NO_PENDING_PREREQUISITIE = Item("has_no_pending_prerequisite")
+
+ IS_APPROVED = Item("is_approved")
+
+ CI_CHECKS_PASSED = Item("CI_checks_passed")
+
+
class BranchSubscriptionDiffSize(DBEnumeratedType):
"""Branch Subscription Diff Size
diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index aa0b560..6226255 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -608,17 +608,35 @@ class IBranchMergeProposalView(Interface):
"""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.
- """
+ """Check if the prerequisite branch has been merged into the target."""
def diffIsUpToDate():
"""Check if the preview diff is up to date, i.e., that there are no
pending diff update jobs.
"""
- def checkMergeCriteria():
+ @call_with(person=REQUEST_USER)
+ @export_read_operation()
+ @operation_for_version("devel")
+ def canIMerge(person):
+ """Check if logged in user 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.
+ """
+
+ @export_read_operation()
+ @operation_for_version("devel")
+ def getMergeCriteria():
+ """Get mergeability status for a merge proposal.
+
+ :return: A dictionary containing the status of merge criteria checks.
+ The 'can_be_merged' key indicates if all criteria passed, and the
+ 'criteria' key contains details about each check."
+ """
+
+ def checkMergeCriteria(force=False):
"""Check if the merge proposal meets all criteria for merging.
The hard criteria are:
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index d51c453..72e6d25 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -40,6 +40,7 @@ from lp.code.enums import (
CodeReviewNotificationLevel,
CodeReviewVote,
GitPermissionType,
+ MergeProposalCriteria,
MergeType,
RevisionStatusResult,
)
@@ -911,8 +912,9 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
if self.merge_prerequisite is None:
return True
+ # Bazaar - not implemented
if self.target_branch:
- raise NotImplementedError("Bazaar branches are not supported")
+ return None
# Check if prerequisite has been merged into the target branch
# The response returns a list of commits that were merged, meaning
@@ -930,39 +932,87 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
return self.next_preview_diff_job is None
+ def getMergeCriteria(self):
+ """See `IBranchMergeProposal`."""
+ can_be_merged, criteria = self.checkMergeCriteria()
+
+ return {
+ "can_be_merged": can_be_merged,
+ "criteria": criteria,
+ }
+
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"),
+ # to merge a MP from Launchpad.
+
+ # List MP criteria: method and error message
+ MERGE_CRITERIA = {
+ MergeProposalCriteria.IS_IN_PROGRESS: (
+ self.isInProgress,
+ "Invalid status for merging",
+ ),
+ MergeProposalCriteria.HAS_NO_CONFLICTS: (
+ self.hasNoConflicts,
+ "Diff contains conflicts",
+ ),
+ MergeProposalCriteria.DIFF_IS_UP_TO_DATE: (
+ self.diffIsUpToDate,
+ "New changes were pushed too recently",
+ ),
+ MergeProposalCriteria.HAS_NO_PENDING_PREREQUISITIE: (
+ self.hasNoPendingPrerequisite,
+ "Prerequisit branch not merged",
+ ),
+ MergeProposalCriteria.IS_APPROVED: (
+ self.isApproved,
+ "Proposal has not been approved",
+ ),
+ MergeProposalCriteria.CI_CHECKS_PASSED: (
+ self.CIChecksPassed,
+ "CI checks failed",
+ ),
+ }
+
+ criteria_to_run = [
+ MergeProposalCriteria.IS_IN_PROGRESS,
+ MergeProposalCriteria.HAS_NO_CONFLICTS,
+ MergeProposalCriteria.DIFF_IS_UP_TO_DATE,
]
+ # This is only a criteria if there is a prerequisit branch
+ if self.merge_prerequisite is not None:
+ criteria_to_run.append(
+ MergeProposalCriteria.HAS_NO_PENDING_PREREQUISITIE
+ )
+
# 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"),
- ]
- )
+ criteria_to_run.append(MergeProposalCriteria.IS_APPROVED)
+ criteria_to_run.append(MergeProposalCriteria.CI_CHECKS_PASSED)
result = True
- failed_checks = []
- for check, error_message in criteria:
- if not check():
- failed_checks.append(error_message)
+ status = {}
+ for criteria in criteria_to_run:
+ check, error = MERGE_CRITERIA[criteria]
+ passed = check()
+
+ key = str(criteria)
+ status[key] = {"passed": passed}
+ if passed is None:
+ status[key]["error"] = "Not implemented"
+ elif not passed:
+ status[key]["error"] = error
result = False
- return result, failed_checks
+ return result, status
+
+ def canIMerge(self, person):
+ """See `IBranchMergeProposal`."""
+ return self.personCanMerge(person)
def personCanMerge(self, person):
"""See `IBranchMergeProposal`."""
@@ -993,8 +1043,13 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
if not self.personCanMerge(person):
raise Unauthorized()
- can_be_merged, failed_checks = self.checkMergeCriteria(force)
+ can_be_merged, criteria = self.checkMergeCriteria(force)
if not can_be_merged:
+ failed_checks = [
+ item["error"]
+ for item in criteria.values()
+ if item["passed"] is False
+ ]
raise BranchMergeProposalNotMergeable(
"Merge proposal cannot be merged in its current state: %s"
% ", ".join(failed_checks)
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index b3e2ef3..61ff88f 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -89,6 +89,7 @@ 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
+from lp.services.webapp.interfaces import OAuthPermission
from lp.services.webhooks.testing import LogsScheduledWebhooks
from lp.services.xref.interfaces import IXRefSet
from lp.testing import (
@@ -96,6 +97,7 @@ from lp.testing import (
TestCaseWithFactory,
WebServiceTestCase,
admin_logged_in,
+ api_url,
launchpadlib_for,
login,
login_person,
@@ -110,6 +112,7 @@ from lp.testing.layers import (
LaunchpadFunctionalLayer,
LaunchpadZopelessLayer,
)
+from lp.testing.pages import webservice_for_person
class WithVCSScenarios(WithScenarios):
@@ -3810,10 +3813,7 @@ class TestBranchMergeProposalPrerequisites(TestCaseWithFactory):
proposal = self.factory.makeBranchMergeProposal(
prerequisite_branch=prerequisite
)
- self.assertRaises(
- NotImplementedError,
- proposal.hasNoPendingPrerequisite,
- )
+ self.assertIsNone(proposal.hasNoPendingPrerequisite())
class TestBranchMergeProposalDiffStatus(WithVCSScenarios, TestCaseWithFactory):
@@ -3847,14 +3847,23 @@ class TestBranchMergeProposalMergeCriteria(
super().setUp()
team = self.factory.makeTeam()
- target = self.makeBranch(owner=team)
+ self.target = self.makeBranch(owner=team)
self.reviewer = self.factory.makePerson(member_of=[team])
- self.proposal = self.makeBranchMergeProposal(target=target)
+ self.proposal = self.makeBranchMergeProposal(target=self.target)
self.useFixture(GitHostingFixture())
+ self.expected_criteria = {
+ "is_in_progress": {"passed": True},
+ "has_no_conflicts": {"passed": True},
+ "diff_is_up_to_date": {"passed": True},
+ "is_approved": {"passed": True},
+ "CI_checks_passed": {"passed": True},
+ }
+
def test_checkMergeCriteria_all_passed(self):
- # If all criteria are met, checkMergeCriteria returns (True, [])
+ # If all criteria are met, checkMergeCriteria returns a tuple with True
+ # and a dict stating which criteria checks ran and all passed
with person_logged_in(self.reviewer):
self.proposal.createComment(
owner=self.reviewer,
@@ -3862,16 +3871,163 @@ class TestBranchMergeProposalMergeCriteria(
)
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)
+ can_merge, criteria = self.proposal.checkMergeCriteria()
+ self.assertTrue(can_merge)
+ self.assertDictEqual(self.expected_criteria, criteria)
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)
+ # If some criteria are not met, checkMergeCriteria returns a tuple with
+ # False, and a dict stating which criteria failed.
+ can_merge, criteria = self.proposal.checkMergeCriteria()
+ self.assertFalse(can_merge)
+ expected_criteria = self.expected_criteria
+ expected_criteria["diff_is_up_to_date"] = {
+ "passed": False,
+ "error": "New changes were pushed too recently",
+ }
+ expected_criteria["is_approved"] = {
+ "passed": False,
+ "error": "Proposal has not been approved",
+ }
+ self.assertDictEqual(expected_criteria, criteria)
+
+ def test_checkMergeCriteria_force(self):
+ # Some criteria can be skippedf with force=True
+
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+
+ # Return False if not using force=True
+ can_merge, criteria = self.proposal.checkMergeCriteria()
+ self.assertFalse(can_merge)
+ expected_criteria = self.expected_criteria
+ expected_criteria["is_approved"] = {
+ "passed": False,
+ "error": "Proposal has not been approved",
+ }
+ self.assertDictEqual(expected_criteria, criteria)
+
+ # Return False if not using force=True
+ can_merge, criteria = self.proposal.checkMergeCriteria(force=True)
+ self.assertTrue(can_merge)
+
+ def test_merge_criteria_with_prerequisite(self):
+ # When there is a prerequisite branch, its criteria is included
+
+ prerequisite = self.makeBranch(
+ same_target_as=self.proposal.merge_target
+ )
+ proposal = self.makeBranchMergeProposal(
+ target=self.proposal.merge_target,
+ prerequisite=prerequisite,
+ )
+
+ with person_logged_in(self.reviewer):
+ proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ proposal.next_preview_diff_job.start()
+ proposal.next_preview_diff_job.complete()
+
+ # Different response for git and bazaar
+ expected_criteria = self.expected_criteria
+ if self.git:
+ expected_can_merge = False
+ expected_criteria["has_no_pending_prerequisite"] = {
+ "passed": False,
+ "error": "Prerequisit branch not merged",
+ }
+ else:
+ expected_can_merge = True
+ expected_criteria["has_no_pending_prerequisite"] = {
+ "passed": None,
+ "error": "Not implemented",
+ }
+ can_merge, criteria = proposal.checkMergeCriteria()
+ self.assertEqual(expected_can_merge, can_merge)
+ self.assertDictEqual(expected_criteria, criteria)
+
+ def test_getMergeCriteria_initial_state(self):
+ # The getMergeCriteria method returns a dict with can_be_merged and
+ # the merge criteria statuses
+ expected_criteria = {
+ "can_be_merged": False,
+ "criteria": {
+ "is_in_progress": {"passed": True},
+ "has_no_conflicts": {"passed": True},
+ "diff_is_up_to_date": {
+ "passed": False,
+ "error": "New changes were pushed too recently",
+ },
+ "is_approved": {
+ "passed": False,
+ "error": "Proposal has not been approved",
+ },
+ "CI_checks_passed": {"passed": True},
+ },
+ }
+
+ criteria = self.proposal.getMergeCriteria()
+ self.assertDictEqual(expected_criteria, criteria)
+
+ def test_getMergeCriteria_all_passed(self):
+ # When all criteria are met, can_be_merged is 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()
+
+ response = self.proposal.getMergeCriteria()
+ expected_response = {
+ "can_be_merged": True,
+ "criteria": self.expected_criteria,
+ }
+ self.assertDictEqual(expected_response, response)
+
+ def test_getMergeCriteria_with_prerequisite(self):
+ # When there is a prerequisite branch, its criteria is included
+
+ prerequisite = self.makeBranch(
+ same_target_as=self.proposal.merge_target
+ )
+ proposal = self.makeBranchMergeProposal(
+ target=self.proposal.merge_target,
+ prerequisite=prerequisite,
+ )
+
+ expected_response = {
+ "can_be_merged": False,
+ "criteria": {
+ "is_in_progress": {"passed": True},
+ "has_no_conflicts": {"passed": True},
+ "diff_is_up_to_date": {
+ "passed": False,
+ "error": "New changes were pushed too recently",
+ },
+ "has_no_pending_prerequisite": {
+ "passed": False,
+ "error": "Prerequisit branch not merged",
+ },
+ "is_approved": {
+ "passed": False,
+ "error": "Proposal has not been approved",
+ },
+ "CI_checks_passed": {"passed": True},
+ },
+ }
+
+ if not self.git:
+ expected_response["criteria"]["has_no_pending_prerequisite"] = {
+ "passed": None,
+ "error": "Not implemented",
+ }
+
+ response = proposal.getMergeCriteria()
+ self.assertDictEqual(expected_response, response)
class TestBranchMergeProposalMergePermissions(TestCaseWithFactory):
@@ -3881,36 +4037,81 @@ class TestBranchMergeProposalMergePermissions(TestCaseWithFactory):
def setUp(self):
super().setUp()
+ self.person = self.factory.makePerson()
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))
+ self.assertFalse(proposal.personCanMerge(self.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
+ rule=rule, grantee=self.person, can_create=True, can_push=True
)
- self.assertTrue(proposal.personCanMerge(person))
+ self.assertTrue(proposal.personCanMerge(self.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,
+ self.person,
+ )
+
+ def test_canIMerge_no_permission(self):
+ # Check that canIMerge endpoint is exposed and returns False if user
+ # has no merge permissions
+
+ self.webservice = webservice_for_person(
+ self.person, permission=OAuthPermission.WRITE_PUBLIC
+ )
+ with person_logged_in(self.person):
+ proposal_url = api_url(self.proposal)
+
+ response = self.webservice.named_get(
+ proposal_url,
+ "canIMerge",
+ api_version="devel",
+ )
+ self.assertEqual(200, response.status)
+ self.assertFalse(response.jsonBody())
+
+ def test_canIMerge_with_permission(self):
+ # Check that canIMerge endpoint is exposed and returns True if user
+ # has merge permissions
+
+ self.webservice = webservice_for_person(
+ self.person, permission=OAuthPermission.WRITE_PUBLIC
+ )
+ with person_logged_in(self.person):
+ proposal_url = api_url(self.proposal)
+
+ rule = removeSecurityProxy(
+ self.factory.makeGitRule(
+ repository=removeSecurityProxy(
+ self.proposal
+ ).target_git_repository
+ )
+ )
+ self.factory.makeGitRuleGrant(
+ rule=rule, grantee=self.person, can_create=True, can_push=True
+ )
+
+ response = self.webservice.named_get(
+ proposal_url,
+ "canIMerge",
+ api_version="devel",
)
+ self.assertEqual(200, response.status)
+ self.assertTrue(response.jsonBody())
class TestBranchMergeProposalMerge(TestCaseWithFactory):
