launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/find-review into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #677077 It should be possible to find the merge proposal that approved a revision via the API
  https://bugs.launchpad.net/bugs/677077


= Summary =
Fix bug #677077: It should be possible to find the merge proposal that approved
a revision via the API

== Proposed fix ==
IBranch.getMergeProposals and add merged_revnos parameter.

== Pre-implementation notes ==
None

== Implementation details ==
IBranch derives from IHasMergeProposals, so it already provides
getMergeProposals.  merged_revnos only makes sense in the context of a single
branch, because a revnos only refers to a specific revision in the context of a
branch.  Therefore, merged_revnos could not be provided on IHasMergeProposals.
Therefore, getMergeProposals was exposed on IBranch.

merge_revnos is plural (not merged_revno) in order to support efficient access
to multiple merge proposals, should that be desired.

== Tests ==
bin/test -t test_getMergeProposals_with_merged_revnos -t test_merge_proposals_merging_revno

== Demo and Q/A ==
Write a commandline script that uses merged_revnos to get a merge proposal over
the API.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/branchcollection.py
  lib/lp/code/interfaces/branch.py
  lib/lp/code/model/branch.py
  lib/lp/code/model/tests/test_branchcollection.py
  lib/canonical/launchpad/interfaces/_schema_circular_imports.py
  lib/lp/code/model/tests/test_branch.py
-- 
https://code.launchpad.net/~abentley/launchpad/find-review/+merge/41195
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/find-review into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-11-09 15:07:38 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-11-18 16:50:14 +0000
@@ -169,6 +169,8 @@
     IBranch, '_createMergeProposal', 'target_branch', IBranch)
 patch_plain_parameter_type(
     IBranch, '_createMergeProposal', 'prerequisite_branch', IBranch)
+patch_collection_return_type(
+    IBranch, 'getMergeProposals', IBranchMergeProposal)
 
 IBranchMergeProposal['getComment'].queryTaggedValue(
     LAZR_WEBSERVICE_EXPORTED)['return_type'].schema = ICodeReviewComment

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2010-11-04 19:59:02 +0000
+++ lib/lp/code/interfaces/branch.py	2010-11-18 16:50:14 +0000
@@ -39,6 +39,7 @@
     exported,
     mutator_for,
     operation_parameters,
+    operation_returns_collection_of,
     operation_returns_entry,
     REQUEST_USER,
     )
@@ -75,6 +76,7 @@
     )
 from lp.code.enums import (
     BranchLifecycleStatus,
+    BranchMergeProposalStatus,
     BranchSubscriptionDiffSize,
     BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel,
@@ -585,6 +587,19 @@
         :param review_requests: An optional list of (`Person`, review_type).
         """
 
+    @operation_parameters(
+        status=List(
+            title=_("A list of merge proposal statuses to filter by."),
+            value_type=Choice(vocabulary=BranchMergeProposalStatus)),
+        merged_revnos=List(Int(
+            title=_('The target-branch revno of the merge.'))))
+    @call_with(visible_by_user=REQUEST_USER)
+    @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
+    @export_read_operation()
+    def getMergeProposals(status=None, visible_by_user=None,
+                          merged_revnos=None):
+        """Return matching BranchMergeProposals."""
+
     def scheduleDiffUpdates():
         """Create UpdatePreviewDiffJobs for this branch's targets."""
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-11-09 08:43:34 +0000
+++ lib/lp/code/model/branch.py	2010-11-18 16:50:14 +0000
@@ -360,8 +360,9 @@
             BranchMergeProposal.queue_status NOT IN %s
             """ % sqlvalues(self, BRANCH_MERGE_PROPOSAL_FINAL_STATES))
 
-    def getMergeProposals(self, status=None, visible_by_user=None):
-        """See `IHasMergeProposals`."""
+    def getMergeProposals(self, status=None, visible_by_user=None,
+                          merged_revnos=None):
+        """See `IBranch`."""
         if not status:
             status = (
                 BranchMergeProposalStatus.CODE_APPROVED,
@@ -369,7 +370,8 @@
                 BranchMergeProposalStatus.WORK_IN_PROGRESS)
 
         collection = getUtility(IAllBranches).visibleByUser(visible_by_user)
-        return collection.getMergeProposals(status, target_branch=self)
+        return collection.getMergeProposals(
+            status, target_branch=self, merged_revnos=merged_revnos)
 
     def isBranchMergeable(self, target_branch):
         """See `IBranch`."""

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/model/branchcollection.py	2010-11-18 16:50:14 +0000
@@ -145,7 +145,7 @@
         return self.store.using(*tables).find(Branch, *expressions)
 
     def getMergeProposals(self, statuses=None, for_branches=None,
-                          target_branch=None):
+                          target_branch=None, merged_revnos=None):
         """See `IBranchCollection`."""
         expressions = [
             BranchMergeProposal.source_branchID.is_in(
@@ -158,6 +158,9 @@
         if target_branch is not None:
             expressions.append(
                 BranchMergeProposal.target_branch == target_branch)
+        if merged_revnos is not None:
+            expressions.append(
+                BranchMergeProposal.merged_revno.is_in(merged_revnos))
         expressions.extend(self._getExtraMergeProposalExpressions())
         if statuses is not None:
             expressions.append(

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2010-11-04 19:59:02 +0000
+++ lib/lp/code/model/tests/test_branch.py	2010-11-18 16:50:14 +0000
@@ -66,7 +66,6 @@
     InvalidBranchMergeProposal,
     InvalidMergeQueueConfig,
     )
-from lp.code.event.branchmergeproposal import NewBranchMergeProposalEvent
 from lp.code.interfaces.branch import (
     DEFAULT_BRANCH_STATUS_IN_LISTING,
     IBranch,
@@ -2757,7 +2756,8 @@
         branch = self.factory.makeBranch()
         config = simplejson.dumps({
             'path': '/',
-            'test': 'make test',})
+            'test': 'make test',
+            })
 
         with person_logged_in(branch.owner):
             branch.setMergeQueueConfig(config)
@@ -2815,6 +2815,19 @@
         branch2 = ws_object(launchpad, db_branch)
         self.assertEqual(branch2.merge_queue_config, configuration)
 
+    def test_getMergeProposals_with_merged_revnos(self):
+        """Specifying merged revnos selects the correct merge proposal."""
+        mp = self.factory.makeBranchMergeProposal()
+        launchpad = launchpadlib_for('test', mp.registrant,
+            service_root="http://api.launchpad.dev:8085";)
+        with person_logged_in(mp.registrant):
+            mp.markAsMerged(merged_revno=123)
+            transaction.commit()
+            target = ws_object(launchpad, mp.target_branch)
+            mp = ws_object(launchpad, mp)
+        self.assertEqual([mp], list(target.getMergeProposals(
+            status=['Merged'], merged_revnos=[123])))
+
 
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2010-11-18 16:50:14 +0000
@@ -40,6 +40,7 @@
 from lp.code.model.branchcollection import GenericBranchCollection
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.testing import (
+    person_logged_in,
     run_with_login,
     TestCaseWithFactory,
     )
@@ -638,6 +639,24 @@
         proposals = collection.getMergeProposals()
         self.assertEqual([mp1], list(proposals))
 
+    def test_merge_proposals_merging_revno(self):
+        """Specifying merged_revnos selects the correct merge proposals."""
+        target = self.factory.makeBranch()
+        mp1 = self.factory.makeBranchMergeProposal(target_branch=target)
+        mp2 = self.factory.makeBranchMergeProposal(target_branch=target)
+        mp3 = self.factory.makeBranchMergeProposal(target_branch=target)
+        with person_logged_in(target.owner):
+            mp1.markAsMerged(123)
+            mp2.markAsMerged(123)
+            mp3.markAsMerged(321)
+        collection = self.all_branches
+        result = collection.getMergeProposals(
+            target_branch=target, merged_revnos=[123])
+        self.assertEqual(sorted([mp1, mp2]), sorted(result))
+        result = collection.getMergeProposals(
+            target_branch=target, merged_revnos=[123, 321])
+        self.assertEqual(sorted([mp1, mp2, mp3]), sorted(result))
+
     def test_target_branch_private(self):
         # The target branch must be in the branch collection, as must the
         # source branch.