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

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.

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

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.