> === modified file 'lib/lp/registry/interfaces/distribution.py'

>      @operation_parameters(
> +        since=Datetime(
> +            title=_("Time of last change"),
> +            description=_(
> +                "Return branches that have new tips since this timestamp."),
> +            required=False))
> +    @export_operation_as(name="getBranchTips")
> +    @export_read_operation()
> +    @operation_for_version('devel')
> +    def getBranchTips(since):
> +        """Return a collection of branches which have new tips since a date.
> +        """

I'd suggest improving the docstring. Something along the lines of:

    Return a list of branches information which have new tips since a date.

    Each branch information is a tuple of (branch_unique_name, tip_revision,
    (official_series*)).

    So for each branch in the distribution, you'll get the branch unique name,
    the revision id of tip, and if the branch is official for some series, the
    list of series name.

    :param since: If specified, only returns branch modified since that date
        time.

You probably need to make since=None since it's not a required parameter.



> === modified file 'lib/lp/registry/model/distribution.py'

>          """See `IBugTarget`."""
>          return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
>  
> +    def getBranchTips(self, since=None):
> +        """See `IDistribution`."""
> +        query = """
> +            SELECT unique_name, last_scanned_id,
> +                SeriesSourcePackageBranch.distroseries FROM Branch
> +            JOIN DistroSeries ON Branch.distroseries = DistroSeries.id
> +            JOIN Distribution ON DistroSeries.distribution = Distribution.id
> +            JOIN SeriesSourcePackageBranch ON
> +                Branch.id = SeriesSourcePackageBranch.branch
> +            WHERE Distribution.name = %s""" % sqlvalues(self.name)

Not all branches will be official, so you want to use a LEFT OUTER JOIN on
both SeriesSourcePackageBranch and DistroSeries.

Distro series id is pretty meangingless, we should return their name.
DistroSeries.name instead of SeriesSourcePackageBranch.distroseries

> +
> +        if since is not None:
> +            query += (
> +                ' AND branch.last_scanned > %s' % sqlvalues(since))
> +
> +        query += ' ORDER BY unique_name, last_scanned_id;'
> +
> +        data = list(Store.of(self).execute(query))
> +
> +        # Group on location (unique_name) and revision (last_scanned_id).
> +        results = []
> +        for key, group in itertools.groupby(data, itemgetter(0, 1)):
> +            results.append(list(key))
> +            # Pull out all the official series IDs and append them as a list
> +            # to the end of the current record.
> +            results[-1].append(map(itemgetter(-1), group))
> +        return results
> +

That last grouping will need to be modified to handle the NULL case.

It's kind of a shame that we have to materialize the whole results set here,
since for huge size, it will be batched anyway by the web service code. Would
it make sense to use an generator here? Maybe not, since we'd still retrieve
all results for later batches.

> === modified file 'lib/lp/registry/tests/test_distro_webservice.py'

Personally, I would have tested the implementation directly and then just
added one test to show that it's available over the webservice (no need to go
over the overhead of launchpadlib for every test). But heh, since the main
point of this is to be available of the web service, I guess that's fine.

I'd like to see an additional showing what happens with branches that are not
official. (They are not returned with the current implementation.)
 
> +
> +class TestGetBranchTips(TestCaseWithFactory):
> +    """Test the getBranchTips method and it's exposure to the web service."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestGetBranchTips, self).setUp()
> +        self.distro = self.factory.makeDistribution()
> +        series_1 = self.series_1 = self.factory.makeDistroRelease(self.distro)
> +        series_2 = self.series_2 = self.factory.makeDistroRelease(self.distro)
> +        source_package = self.factory.makeSourcePackage(distroseries=series_1)
> +        self.branch = self.factory.makeBranch(sourcepackage=source_package)
> +        registrant = self.factory.makePerson()
> +        now = datetime.now(pytz.UTC)
> +        sourcepackagename = self.factory.makeSourcePackageName()
> +        SeriesSourcePackageBranchSet.new(
> +            series_1, PackagePublishingPocket.RELEASE, sourcepackagename,
> +            self.branch, registrant, now)
> +        SeriesSourcePackageBranchSet.new(
> +            series_2, PackagePublishingPocket.RELEASE, sourcepackagename,
> +            self.branch, registrant, now)

You can use makeRelatedBranches(reference_branch=self.branch) to make it an
official source package. Or use
makeRelatedBranchesForSourcePackage(source_package) to create the branch and
make it official in one call.

> +        self.factory.makeRevisionsForBranch(self.branch)
> +        endInteraction()
> +
> +        self.lp = launchpadlib_for("anonymous-access")
> +        self.lp_distro = [d for d in self.lp.distributions
> +            if d.name == self.distro.name][0]

Couldn't you use self.lp.distributions[d.name] directly?

> +
> +    def test_structure(self):
> +        """The structure of the results is what we expect."""
> +        # The results should be structured as a list of
> +        # (location, tip revision ID, [official series, official series, ...])
> +        item = self.lp_distro.getBranchTips()[0]
> +        self.assertTrue(item[0].startswith('~person-name-'))
Can't you compare with the actual expected value?

self.assertEquals(self.branch_unique_name, item[0])?

> +        self.assertTrue(item[1].startswith('revision-id-'))
> +        self.assertEqual(sorted(item[2]), [14, 15])

Wow, talk about magic numbers! Comparing with the created distro series name
probably makes more sense.