launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/mp-by-revision-id into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1020961 in Launchpad itself: "API cannot find merge proposal without branch"
  https://bugs.launchpad.net/launchpad/+bug/1020961

For more details, see:
https://code.launchpad.net/~abentley/launchpad/mp-by-revision-id/+merge/114693

= Summary =
Fix bug #1020961: API cannot find merge proposal without branch

== Proposed fix ==
Add merged_revision parameter to GenericBranchCollection.getMergeProposals, add BranchSet.getMergeProposals and export via API.

== Pre-implementation notes ==
None

== LOC Rationale ==
I have a LOC credit of 1983

== Implementation details ==
This approach uses BranchRevision to find the association between a revision_id and a merge proposal.  Another option would be a  schema change to associate the merge proposal with a Revision.id or revision-id directly, but I don't belive that this API has enough traffic to warrant that.

== Tests ==
bin/test -vt TestGetMergeProposals

== Demo and Q/A ==
Using launchpadlib (potentially via lp-shell), use getMergeProposals with revision-id 'launchpad@xxxxxxxxxxxxxxxxx-20120222203340-sn3txswd2ch18ded'.  The sole result should be https://code.qastaging.launchpad.net/~wallyworld/launchpad/sharing-picker-933823-2/+merge/94087


= 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/testing/factory.py
  lib/lp/code/model/branch.py
  lib/lp/_schema_circular_imports.py
  lib/lp/code/model/tests/test_branch.py
-- 
https://code.launchpad.net/~abentley/launchpad/mp-by-revision-id/+merge/114693
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/mp-by-revision-id into lp:launchpad.

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2012-06-19 16:09:38 +0000
+++ lib/lp/_schema_circular_imports.py	2012-07-12 18:00:32 +0000
@@ -59,7 +59,10 @@
     )
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
-from lp.code.interfaces.branch import IBranch
+from lp.code.interfaces.branch import (
+    IBranch,
+    IBranchSet,
+    )
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.branchmergequeue import IBranchMergeQueue
 from lp.code.interfaces.branchsubscription import IBranchSubscription
@@ -263,6 +266,9 @@
 patch_collection_return_type(
     IBranch, 'getMergeProposals', IBranchMergeProposal)
 
+patch_collection_return_type(
+    IBranchSet, 'getMergeProposals', IBranchMergeProposal)
+
 IBranchMergeProposal['getComment'].queryTaggedValue(
     LAZR_WEBSERVICE_EXPORTED)['return_type'].schema = ICodeReviewComment
 IBranchMergeProposal['createComment'].queryTaggedValue(

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2012-07-06 03:33:59 +0000
+++ lib/lp/code/interfaces/branch.py	2012-07-12 18:00:32 +0000
@@ -1405,6 +1405,20 @@
         branches.
         """
 
+    @operation_returns_collection_of(Interface)
+    @call_with(visible_by_user=REQUEST_USER)
+    @operation_parameters(merged_revision=TextLine())
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getMergeProposals(merged_revision, visible_by_user=None):
+        """Return the merge proposals that resulted in this revision.
+
+        :param merged_revision: The revision_id of the revision that resulted
+            from this merge proposal.
+        :param visible_by_user: The user to whom the proposals must be
+            visible.  If None, only public proposals will be returned.
+        """
+
 
 class IBranchListingQueryOptimiser(Interface):
     """Interface for a helper utility to do efficient queries for branches.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-07-11 09:43:04 +0000
+++ lib/lp/code/model/branch.py	2012-07-12 18:00:32 +0000
@@ -1532,6 +1532,11 @@
             'person_name': person.displayname,
             'visible_branches': visible_branches}
 
+    def getMergeProposals(self, merged_revision, visible_by_user=None):
+        """See IBranchSet."""
+        collection = getUtility(IAllBranches).visibleByUser(visible_by_user)
+        return collection.getMergeProposals(merged_revision=merged_revision)
+
 
 def update_trigger_modified_fields(branch):
     """Make the trigger updated fields reload when next accessed."""

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2012-07-09 10:35:00 +0000
+++ lib/lp/code/model/branchcollection.py	2012-07-12 18:00:32 +0000
@@ -54,10 +54,12 @@
     get_branch_privacy_filter,
     )
 from lp.code.model.branchmergeproposal import BranchMergeProposal
+from lp.code.model.branchrevision import BranchRevision
 from lp.code.model.branchsubscription import BranchSubscription
 from lp.code.model.codeimport import CodeImport
 from lp.code.model.codereviewcomment import CodeReviewComment
 from lp.code.model.codereviewvote import CodeReviewVoteReference
+from lp.code.model.revision import Revision
 from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
 from lp.registry.enums import PUBLIC_INFORMATION_TYPES
 from lp.registry.model.distribution import Distribution
@@ -313,7 +315,7 @@
 
     def getMergeProposals(self, statuses=None, for_branches=None,
                           target_branch=None, merged_revnos=None,
-                          eager_load=False):
+                          merged_revision=None, eager_load=False):
         """See `IBranchCollection`."""
         if for_branches is not None and not for_branches:
             # We have an empty branches list, so we can shortcut.
@@ -324,10 +326,11 @@
         elif (self._asymmetric_filter_expressions or
             for_branches is not None or
             target_branch is not None or
-            merged_revnos is not None):
+            merged_revnos is not None or
+            merged_revision is not None):
             return self._naiveGetMergeProposals(
                 statuses, for_branches, target_branch, merged_revnos,
-                eager_load=eager_load)
+                merged_revision, eager_load=eager_load)
         else:
             # When examining merge proposals in a scope, this is a moderately
             # effective set of constrained queries. It is not effective when
@@ -337,7 +340,7 @@
 
     def _naiveGetMergeProposals(self, statuses=None, for_branches=None,
                                 target_branch=None, merged_revnos=None,
-                                eager_load=False):
+                                merged_revision=None, eager_load=False):
         Target = ClassAlias(Branch, "target")
         extra_tables = list(set(
             self._tables.values() + self._asymmetric_tables.values()))
@@ -360,6 +363,15 @@
         if merged_revnos is not None:
             expressions.append(
                 BranchMergeProposal.merged_revno.is_in(merged_revnos))
+        if merged_revision is not None:
+            expressions.extend([
+                BranchMergeProposal.merged_revno == BranchRevision.sequence,
+                BranchRevision.revision_id == Revision.id,
+                BranchRevision.branch_id ==
+                    BranchMergeProposal.target_branchID,
+                Revision.revision_id == merged_revision
+            ])
+            tables.extend([BranchRevision, Revision])
         if statuses is not None:
             expressions.append(
                 BranchMergeProposal.queue_status.is_in(statuses))

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-07-11 09:36:13 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-07-12 18:00:32 +0000
@@ -84,6 +84,7 @@
     IFindOfficialBranchLinks,
     )
 from lp.code.model.branch import (
+    BranchSet,
     ClearDependentBranch,
     ClearOfficialPackageBranch,
     ClearSeriesBranch,
@@ -150,6 +151,7 @@
     TestCaseWithFactory,
     time_counter,
     ws_object,
+    WebServiceTestCase,
     )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.layers import (
@@ -2947,6 +2949,86 @@
             new_product, get_policies_for_artifact(branch)[0].pillar)
 
 
+def make_proposal_and_branch_revision(factory, revno, revision_id,
+                                      userdata_target=False):
+    revision = factory.makeBranchRevision(revision_id=revision_id,
+                                          sequence=revno)
+    bmp = factory.makeBranchMergeProposal(merged_revno=revno,
+                                          target_branch=revision.branch)
+    if userdata_target:
+        naked_target = removeSecurityProxy(bmp.target_branch)
+        naked_target.information_type = InformationType.USERDATA
+    return bmp
+
+
+class TestGetMergeProposalsWS(WebServiceTestCase):
+
+    def test_getMergeProposals(self):
+        """getMergeProposals works as expected over the API."""
+        bmp = make_proposal_and_branch_revision(self.factory, 5, 'rev-id',
+                                                userdata_target=True)
+        transaction.commit()
+        user = removeSecurityProxy(bmp).target_branch.owner
+        service = self.factory.makeLaunchpadService(
+            user, version=self.ws_version)
+        result = service.branches.getMergeProposals(merged_revision='rev-id')
+        self.assertEqual([self.wsObject(bmp, user)], list(result))
+
+
+class TestGetMergeProposals(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGetMergeProposals, self).setUp()
+        self.branch_set = BranchSet()
+
+    def test_getMergeProposals_with_no_merged_revno(self):
+        """Merge proposals with no merged revno are not found."""
+        make_proposal_and_branch_revision(self.factory, None, 'rev-id')
+        result = self.branch_set.getMergeProposals(merged_revision='rev-id')
+        self.assertEqual([], list(result))
+
+    def test_getMergeProposals_with_any_merged_revno(self):
+        """Any arbitrary revno will connect a revid to a proposal."""
+        bmp = make_proposal_and_branch_revision(
+            self.factory, self.factory.getUniqueInteger(), 'rev-id')
+        result = self.branch_set.getMergeProposals(merged_revision='rev-id')
+        self.assertEqual([bmp], list(result))
+
+    def test_getMergeProposals_correct_merged_revno(self):
+        """Only proposals with the correct merged_revno match."""
+        bmp1 = make_proposal_and_branch_revision(self.factory, 4, 'rev-id')
+        bmp2 = make_proposal_and_branch_revision(self.factory, 5, 'other')
+        result = self.branch_set.getMergeProposals(merged_revision='rev-id')
+        self.assertEqual([bmp1], list(result))
+        result = self.branch_set.getMergeProposals(merged_revision='other')
+        self.assertEqual([bmp2], list(result))
+
+    def test_getMergeProposals_correct_branch(self):
+        """Only proposals with the correct branch match."""
+        bmp1 = make_proposal_and_branch_revision(self.factory, 5, 'rev-id')
+        make_proposal_and_branch_revision(self.factory, 5, 'other')
+        result = self.branch_set.getMergeProposals(merged_revision='rev-id')
+        self.assertEqual([bmp1], list(result))
+
+    def test_getMergeProposals_skips_hidden(self):
+        """Proposals not visible to the user are skipped."""
+        make_proposal_and_branch_revision(
+            self.factory, 5, 'rev-id', userdata_target=True)
+        result = self.branch_set.getMergeProposals(merged_revision='rev-id',
+            visible_by_user=self.factory.makePerson())
+        self.assertEqual([], list(result))
+
+    def test_getMergeProposals_shows_visible_userdata(self):
+        """Proposals visible to the user are listed."""
+        bmp = make_proposal_and_branch_revision(
+            self.factory, 5, 'rev-id', userdata_target=True)
+        result = self.branch_set.getMergeProposals(merged_revision='rev-id',
+            visible_by_user=bmp.target_branch.owner)
+        self.assertEqual([bmp], list(result))
+
+
 class TestScheduleDiffUpdates(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-07-03 10:29:53 +0000
+++ lib/lp/testing/factory.py	2012-07-12 18:00:32 +0000
@@ -1441,7 +1441,7 @@
                                 product=None, initial_comment=None,
                                 source_branch=None, preview_diff=None,
                                 date_created=None, description=None,
-                                reviewer=None):
+                                reviewer=None, merged_revno=None):
         """Create a proposal to merge based on anonymous branches."""
         if target_branch is not None:
             target = target_branch.target
@@ -1475,6 +1475,7 @@
             date_created=date_created)
 
         unsafe_proposal = removeSecurityProxy(proposal)
+        unsafe_proposal.merged_revno = merged_revno
         if preview_diff is not None:
             unsafe_proposal.preview_diff = preview_diff
         if (set_state is None or
@@ -1638,8 +1639,10 @@
             '', parent.revision_id, None, None, None)
         branch.updateScannedDetails(parent, sequence)
 
-    def makeBranchRevision(self, branch, revision_id=None, sequence=None,
+    def makeBranchRevision(self, branch=None, revision_id=None, sequence=None,
                            parent_ids=None, revision_date=None):
+        if branch is None:
+            branch = self.makeBranch()
         revision = self.makeRevision(
             rev_id=revision_id, parent_ids=parent_ids,
             revision_date=revision_date)