Merge lp:~thumper/launchpad/only-show-latest-bmp-in-revision-mail into lp:launchpad

[Tim Penhey]

Only show the most recent proposal for any given source branch in the revision mail.

[Tim Penhey (thumper)]

The revision mail that goes out includes related merge proposals, which is great as long as one doesn't reuse branches. An example is the pending-db-changes that stub has. This branch only shows the latest merge proposal for any given source branch.

[Michael Hudson-Doyle (mwhudson)]

Hi Tim,

nice branch, just a couple of things to think about.

> === modified file 'lib/lp/code/model/branchjob.py'
> --- lib/lp/code/model/branchjob.py 2010-02-03 19:29:27 +0000
> +++ lib/lp/code/model/branchjob.py 2010-02-16 04:50:58 +0000
> @@ -13,6 +13,7 @@
> ]
>
> import contextlib
> +import operator
> import os
> import shutil
> from StringIO import StringIO
> @@ -600,28 +601,43 @@
> authors.update(revision.get_apparent_authors())
> return authors
>
> - def findRelatedBMP(self, revision_ids, include_superseded=True):
> + def findRelatedBMP(self, revision_ids):
> """Find merge proposals related to the revision-ids and branch.
>
> Only proposals whose source branch last-scanned-id is in the set of
> revision-ids and whose target_branch is the BranchJob branch are
> returned.
>
> + Only return the most recent proposal for any given source branch.
> +
> :param revision_ids: A list of revision-ids to look for.
> :param include_superseded: If true, include merge proposals that are
> superseded in the results.
> """
> store = Store.of(self.branch)
> conditions = [
> + ]

'conditions' is unused now.

> + result = store.find(
> + (BranchMergeProposal, Branch),
> BranchMergeProposal.target_branch == self.branch.id,
> BranchMergeProposal.source_branch == Branch.id,
> - Branch.last_scanned_id.is_in(revision_ids)]
> - if not include_superseded:
> - conditions.append(
> - BranchMergeProposal.queue_status !=
> - BranchMergeProposalStatus.SUPERSEDED)
> - result = store.find(BranchMergeProposal, *conditions)
> - return result
> + Branch.last_scanned_id.is_in(revision_ids),
> + (BranchMergeProposal.queue_status !=
> + BranchMergeProposalStatus.SUPERSEDED))
> +
> + proposals = {}
> + for proposal, source in result:
> + # Only show the must recent proposal for any given source.
> + date_created = proposal.date_created
> + source_id = source.id
> +
> + if (source_id not in proposals or
> + date_created > proposals[source_id][1]):
> + proposals[source_id] = (proposal, date_created)
> +
> + return sorted(
> + [proposal for proposal, date_created in proposals.itervalues()],
> + key=operator.attrgetter('date_created'), reverse=True)

This seems a little complicated, although I admit I can't think of any
obviously clearer ways. Did you think about any ways of doing the
filtering in the database? That seems likely to be complicated too
though...

> def getRevisionMessage(self, revision_id, revno):
> """Return the log message for a revision.
> @@ -654,9 +670,7 @@
> if len(pretty_authors) > 5:
> outf.write('...\n')
> outf.write('\n')
> - bmps = list(
> - self.findRelatedBMP(merged_revisions,
> - include_superseded=False))
> + bmps = self.f...

review: Approve

[Tim Penhey (thumper)]

On Tue, 16 Feb 2010 19:27:34 Michael Hudson wrote:
> Review: Approve
> Hi Tim,
>
> nice branch, just a couple of things to think about.
>
> > === modified file 'lib/lp/code/model/branchjob.py'
> > --- lib/lp/code/model/branchjob.py 2010-02-03 19:29:27 +0000
> > +++ lib/lp/code/model/branchjob.py 2010-02-16 04:50:58 +0000
> > store = Store.of(self.branch)
> > conditions = [
> > + ]
>
> 'conditions' is unused now.

Removed now.

> > + return sorted(
> > + [proposal for proposal, date_created in
> > proposals.itervalues()], +
> > key=operator.attrgetter('date_created'), reverse=True)
>
> This seems a little complicated, although I admit I can't think of any
> obviously clearer ways. Did you think about any ways of doing the
> filtering in the database? That seems likely to be complicated too
> though...

It could possibly be done, but I don't know how to do it in storm.

We want to group by the source branch and choose the merge proposal with the
latest date_created. I just don't know how to do this.

> > def getRevisionMessage(self, revision_id, revno):
> > """Return the log message for a revision.
> > @@ -654,9 +670,7 @@
> > if len(pretty_authors) > 5:
> > outf.write('...\n')
> > outf.write('\n')
> > - bmps = list(
> > - self.findRelatedBMP(merged_revisions,
> > - include_superseded=False))
> > + bmps = self.findRelatedBMP(merged_revisions)
> > if len(bmps) > 0:
> > outf.write('Related merge proposals:\n')
> > for bmp in bmps:
> >
> > === modified file 'lib/lp/code/model/tests/test_branchjob.py'
> > --- lib/lp/code/model/tests/test_branchjob.py 2010-01-26 02:05:23 +0000
> > +++ lib/lp/code/model/tests/test_branchjob.py 2010-02-16 04:50:58 +0000
> > + def test_findRelatedBMP_one_per_source(self):
> > + """Assert that only the lastest for any particular source is
> > returned.
>
> I don't think it adds anything to start a test method
> comment/docstring with "Assert that". Something like "findRelatedBMP
> only returns the latest proposal for any source." would be better
> IMHO.

Fixed.

> And lastest isn't spelt like that :-)

I was tired. Also fixed.

> > self.assertEqual([desired_proposal],
> > list(job.findRelatedBMP(['rev2a-id'])))
>
> It would be nice to reformat this as
>
> self.assertEqual(
> [desired_proposal], job.findRelatedBMP(['rev2a-id']))
>
> as you did for the other test.

Done.

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2010-02-03 19:29:27 +0000
+++ lib/lp/code/model/branchjob.py	2010-02-17 00:50:32 +0000
@@ -13,6 +13,7 @@
 ]
 
 import contextlib
+import operator
 import os
 import shutil
 from StringIO import StringIO
@@ -600,28 +601,41 @@
             authors.update(revision.get_apparent_authors())
         return authors
 
-    def findRelatedBMP(self, revision_ids, include_superseded=True):
+    def findRelatedBMP(self, revision_ids):
         """Find merge proposals related to the revision-ids and branch.
 
         Only proposals whose source branch last-scanned-id is in the set of
         revision-ids and whose target_branch is the BranchJob branch are
         returned.
 
+        Only return the most recent proposal for any given source branch.
+
         :param revision_ids: A list of revision-ids to look for.
         :param include_superseded: If true, include merge proposals that are
             superseded in the results.
         """
         store = Store.of(self.branch)
-        conditions = [
+        result = store.find(
+            (BranchMergeProposal, Branch),
             BranchMergeProposal.target_branch == self.branch.id,
             BranchMergeProposal.source_branch == Branch.id,
-            Branch.last_scanned_id.is_in(revision_ids)]
-        if not include_superseded:
-            conditions.append(
-                BranchMergeProposal.queue_status !=
-                BranchMergeProposalStatus.SUPERSEDED)
-        result = store.find(BranchMergeProposal, *conditions)
-        return result
+            Branch.last_scanned_id.is_in(revision_ids),
+            (BranchMergeProposal.queue_status !=
+             BranchMergeProposalStatus.SUPERSEDED))
+
+        proposals = {}
+        for proposal, source in result:
+            # Only show the must recent proposal for any given source.
+            date_created = proposal.date_created
+            source_id = source.id
+
+            if (source_id not in proposals or
+                date_created > proposals[source_id][1]):
+                proposals[source_id] = (proposal, date_created)
+
+        return sorted(
+            [proposal for proposal, date_created in proposals.itervalues()],
+            key=operator.attrgetter('date_created'), reverse=True)
 
     def getRevisionMessage(self, revision_id, revno):
         """Return the log message for a revision.
@@ -654,9 +668,7 @@
                 if len(pretty_authors) > 5:
                     outf.write('...\n')
                 outf.write('\n')
-            bmps = list(
-                self.findRelatedBMP(merged_revisions,
-                include_superseded=False))
+            bmps = self.findRelatedBMP(merged_revisions)
             if len(bmps) > 0:
                 outf.write('Related merge proposals:\n')
             for bmp in bmps:

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2010-01-26 02:05:23 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2010-02-17 00:50:32 +0000
@@ -7,6 +7,7 @@
 
 import datetime
 import os
+import pytz
 import shutil
 from storm.locals import Store
 import tempfile
@@ -40,8 +41,8 @@
 from lp.services.job.model.job import Job
 from lp.code.bzr import BranchFormat, RepositoryFormat
 from lp.code.enums import (
-    BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
-    CodeReviewNotificationLevel)
+    BranchMergeProposalStatus, BranchSubscriptionDiffSize,
+    BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
 from lp.code.interfaces.branchjob import (
     IBranchDiffJob, IBranchJob, IBranchScanJob, IBranchUpgradeJob,
     IReclaimBranchSpaceJob, IReclaimBranchSpaceJobSource, IRevisionMailJob,
@@ -546,8 +547,27 @@
         wrong_target_proposal.source_branch.last_scanned_id = 'rev2a-id'
         job = RevisionsAddedJob.create(target_branch, 'rev2b-id', 'rev2b-id',
                                        '')
-        self.assertEqual([desired_proposal],
-                         list(job.findRelatedBMP(['rev2a-id'])))
+        self.assertEqual(
+            [desired_proposal], job.findRelatedBMP(['rev2a-id']))
+
+    def test_findRelatedBMP_one_per_source(self):
+        """findRelatedBMP only returns the most recent proposal for any
+        particular source branch.
+        """
+        self.useBzrBranches()
+        target_branch, tree = self.create_branch_and_tree('tree')
+        the_past = datetime.datetime(2009, 1, 1, tzinfo=pytz.UTC)
+        old_proposal = self.factory.makeBranchMergeProposal(
+            target_branch=target_branch, date_created=the_past,
+            set_state=BranchMergeProposalStatus.MERGED)
+        source_branch = old_proposal.source_branch
+        source_branch.last_scanned_id = 'rev2a-id'
+        desired_proposal = source_branch.addLandingTarget(
+            source_branch.owner, target_branch)
+        job = RevisionsAddedJob.create(
+            target_branch, 'rev2b-id', 'rev2b-id', '')
+        self.assertEqual(
+            [desired_proposal], job.findRelatedBMP(['rev2a-id']))
 
     def test_getAuthors(self):
         """Ensure getAuthors returns the authors for the revisions."""