Merge lp:~abentley/launchpad/find-review into lp:launchpad

[Aaron Bentley]

= 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

[j.c.sackett (jcsackett)]

Aaron--

This seems like a really nice feature to have in the API, particularly given recent reviewers conversations.

Also, thanks for explaining some of the pain you had to go through for circular imports on IRC. It was very informative.

> === 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(

I brought the mismatch between this method definition and the interface definition with regards to merged_revnos as an argument up on IRC, and I buy your argument that it's not required b/c there's no security proxy and adding it would break other things.

It's not a blocker for merging, but a comment here to explain the difference or something wouldn't suck. :-)

> === 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)

Thanks for doing that cleanup.

review: Approve (code*)

[Curtis Hovey (sinzui)]

I agree that this is good to land.

review: Approve (code)

=== 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.