launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04144
[Merge] lp:~jtv/launchpad/bug-798297 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-798297 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798297 in Launchpad itself: "No way to get DistroSeriesDifferenceComments via the web service"
https://bugs.launchpad.net/launchpad/+bug/798297
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-798297/+merge/66816
= Summary =
The Ubuntu people need API access to DistroSeriesDifferenceComments. Something called Merge-o-Matic will poll for new comments and pull them into its own database.
We need to provide three kinds of filtering: by distroseries (regardless of exact use), by time the comment was made (to support update polling), and by source package name (to produce per-package timelines).
== Proposed fix ==
Provide all these through the DistroSeries devel API, using a single method.
The combination of filtering by age and distroseries can be awkward, since these two items are at opposite ends of the schema sub-graph that we need for this. Luckily, DistroSeriesDifferenceComments are really just thin immutable wrappers around Message. So instead of filtering by Message.datecreated (the real age of a comment) I find the last Message.id prior to the filter date, and look for DistroSeriesDifferenceComments whose message fields are greater than that.
It looks as if this will require a Message table scan; I'm not sure how costly that will be but expect that we'll probably want an index to support this.
== Pre-implementation notes ==
Julian says security is not a concern for this method as yet; we'll trust the security machinery to keep anyone out who isn't allowed to view the DistroSeries.
The use-case is very similar to what we have with feeds, so this may also become the basis for future feed support.
== Implementation details ==
We could have the client remember the last-polled DSDComment id, but we prefer not to expose ids and use dates instead.
== Tests ==
{{{
./bin/test -vvc lp.registry.tests.test_distroseries -t DifferenceComments
./bin/test -vvc lp.registry.tests.test_distroseriesdifferencecomment -t getForDistroSeries
}}}
== Demo and Q/A ==
We'll be able to browse (and to a limited extent) query DSDComments for a DistroSeries. They should be filtered appropriately and ordered by creation time.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/tests/test_distroseriesdifferencecomment.py
lib/lp/registry/tests/test_distroseries.py
lib/lp/registry/model/distroseriesdifferencecomment.py
lib/lp/registry/interfaces/distroseries.py
lib/lp/registry/interfaces/distroseriesdifferencecomment.py
lib/lp/registry/model/distroseries.py
lib/canonical/launchpad/interfaces/_schema_circular_imports.py
--
https://code.launchpad.net/~jtv/launchpad/bug-798297/+merge/66816
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-798297 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2011-06-23 18:26:26 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2011-07-04 16:11:44 +0000
@@ -516,6 +516,8 @@
DistroSeriesDifferenceType)
patch_collection_return_type(
IDistroSeries, 'getDifferencesTo', IDistroSeriesDifference)
+patch_collection_return_type(
+ IDistroSeries, 'getDifferenceComments', IDistroSeriesDifferenceComment)
# IDistroSeriesDifference
=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py 2011-06-20 16:04:15 +0000
+++ lib/lp/registry/interfaces/distroseries.py 2011-07-04 16:11:44 +0000
@@ -903,6 +903,19 @@
def isInitialized():
"""Has this series been initialized?"""
+ @operation_returns_collection_of(Interface)
+ @export_read_operation()
+ @operation_for_version('devel')
+ def getDifferenceComments(since=None, source_package_name=None):
+ """Get `IDistroSeriesDifferenceComment`s.
+
+ :param since: Ignore comments older than this date.
+ :param source_package_name: Return only comments for a source package
+ with this name.
+ :return: A Storm result set of `IDistroSeriesDifferenceComment`s
+ for this distroseries, ordered from oldest to newest comment.
+ """
+
class IDistroSeriesEditRestricted(Interface):
"""IDistroSeries properties which require launchpad.Edit."""
=== modified file 'lib/lp/registry/interfaces/distroseriesdifferencecomment.py'
--- lib/lp/registry/interfaces/distroseriesdifferencecomment.py 2011-05-12 21:33:10 +0000
+++ lib/lp/registry/interfaces/distroseriesdifferencecomment.py 2011-07-04 16:11:44 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Distribution series difference messages."""
@@ -20,6 +20,7 @@
Datetime,
Int,
Text,
+ TextLine,
)
from canonical.launchpad import _
@@ -56,6 +57,11 @@
comment_date = exported(Datetime(
title=_('Comment date.'), readonly=True))
+ source_package_name = exported(TextLine(
+ title=_("Source package name"), required=True, readonly=True,
+ description=_(
+ "Name of the source package that this comment is for.")))
+
class IDistroSeriesDifferenceCommentSource(Interface):
"""A utility of this interface can be used to create comments."""
@@ -72,3 +78,13 @@
def getForDifference(distro_series_difference, id):
"""Return the `IDistroSeriesDifferenceComment` with the given id."""
+
+ def getForDistroSeries(distroseries, since=None):
+ """Get comments for `distroseries` (since `since` if given).
+
+ :param distroseries: The `DistroSeries` to find comments for.
+ :param since: A date. No comments older than this date will be
+ returned.
+ :return: A result set of `DistroSeriesDifferenceComment`s, ordered
+ from oldest to newest.
+ """
=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2011-06-17 14:23:52 +0000
+++ lib/lp/registry/model/distroseries.py 2011-07-04 16:11:44 +0000
@@ -101,6 +101,9 @@
from lp.registry.interfaces.distroseriesdifference import (
IDistroSeriesDifferenceSource,
)
+from lp.registry.interfaces.distroseriesdifferencecomment import (
+ IDistroSeriesDifferenceCommentSource,
+ )
from lp.registry.interfaces.person import validate_public_person
from lp.registry.interfaces.pocket import (
PackagePublishingPocket,
@@ -1895,6 +1898,12 @@
published = self.main_archive.getPublishedSources(distroseries=self)
return not published.is_empty()
+ def getDifferenceComments(self, since=None, source_package_name=None):
+ """See `IDistroSeries`."""
+ comment_source = getUtility(IDistroSeriesDifferenceCommentSource)
+ return comment_source.getForDistroSeries(
+ self, since=since, source_package_name=source_package_name)
+
class DistroSeriesSet:
implements(IDistroSeriesSet)
=== modified file 'lib/lp/registry/model/distroseriesdifferencecomment.py'
--- lib/lp/registry/model/distroseriesdifferencecomment.py 2011-05-12 21:33:10 +0000
+++ lib/lp/registry/model/distroseriesdifferencecomment.py 2011-07-04 16:11:44 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""A comment/message for a difference between two distribution series."""
@@ -13,6 +13,7 @@
from storm.locals import (
Int,
+ Max,
Reference,
Storm,
)
@@ -30,6 +31,7 @@
IDistroSeriesDifferenceComment,
IDistroSeriesDifferenceCommentSource,
)
+from lp.registry.model.sourcepackagename import SourcePackageName
class DistroSeriesDifferenceComment(Storm):
@@ -63,6 +65,11 @@
"""See `IDistroSeriesDifferenceComment`."""
return self.message.datecreated
+ @property
+ def source_package_name(self):
+ """See `IDistroSeriesDifferenceCommentSource`."""
+ return self.distro_series_difference.source_package_name.name
+
@staticmethod
def new(distro_series_difference, owner, comment):
"""See `IDistroSeriesDifferenceCommentSource`."""
@@ -90,3 +97,34 @@
DSDComment,
DSDComment.distro_series_difference == distro_series_difference,
DSDComment.id == id).one()
+
+ @staticmethod
+ def getForDistroSeries(distroseries, since=None,
+ source_package_name=None):
+ """See `IDistroSeriesDifferenceCommentSource`."""
+ # Avoid circular imports.
+ from lp.registry.model.distroseriesdifference import (
+ DistroSeriesDifference,
+ )
+ store = IStore(DistroSeriesDifferenceComment)
+ DSD = DistroSeriesDifference
+ DSDComment = DistroSeriesDifferenceComment
+ conditions = [
+ DSDComment.distro_series_difference_id == DSD.id,
+ DSD.derived_series_id == distroseries.id,
+ ]
+
+ if source_package_name is not None:
+ conditions += [
+ SourcePackageName.id == DSD.source_package_name_id,
+ SourcePackageName.name == source_package_name,
+ ]
+
+ if since is not None:
+ after_msgid = store.find(
+ Max(Message.id), Message.datecreated < since).one()
+ if after_msgid is not None:
+ conditions.append(DSDComment.message_id > after_msgid)
+
+ return store.find(DSDComment, *conditions).order_by(
+ DSDComment.message_id)
=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py 2011-06-14 19:49:18 +0000
+++ lib/lp/registry/tests/test_distroseries.py 2011-07-04 16:11:44 +0000
@@ -256,6 +256,14 @@
distroseries=distroseries, archive=distroseries.main_archive)
self.assertTrue(distroseries.isInitialized())
+ def test_getDifferenceComments_gets_DistroSeriesDifferenceComments(self):
+ distroseries = self.factory.makeDistroSeries()
+ dsd = self.factory.makeDistroSeriesDifference(
+ derived_series=distroseries)
+ comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+ self.assertContentEqual(
+ [comment], distroseries.getDifferenceComments())
+
class TestDistroSeriesPackaging(TestCaseWithFactory):
=== modified file 'lib/lp/registry/tests/test_distroseriesdifferencecomment.py'
--- lib/lp/registry/tests/test_distroseriesdifferencecomment.py 2010-10-06 18:53:53 +0000
+++ lib/lp/registry/tests/test_distroseriesdifferencecomment.py 2011-07-04 16:11:44 +0000
@@ -1,20 +1,41 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Model tests for the DistroSeriesDifferenceComment class."""
__metaclass__ = type
+from datetime import timedelta
+from random import randint
from storm.store import Store
from zope.component import getUtility
from canonical.launchpad.webapp.testing import verifyObject
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+ DatabaseFunctionalLayer,
+ ZopelessDatabaseLayer,
+ )
from lp.registry.interfaces.distroseriesdifferencecomment import (
IDistroSeriesDifferenceComment,
IDistroSeriesDifferenceCommentSource,
)
from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import Provides
+
+
+def get_comment_source():
+ """Get `IDistroSeriesDifferemtCommentSource` utility."""
+ return getUtility(IDistroSeriesDifferenceCommentSource)
+
+
+def flip_coin(*args):
+ """Random comparison function. Returns -1 or 1 randomly."""
+ return 1 - 2 * randint(0, 1)
+
+
+def randomize_list(original_list):
+ """Sort a list (or other iterable) in random order. Return list."""
+ return sorted(original_list, cmp=flip_coin)
class DistroSeriesDifferenceCommentTestCase(TestCaseWithFactory):
@@ -64,7 +85,90 @@
dsd_comment = self.factory.makeDistroSeriesDifferenceComment()
Store.of(dsd_comment).flush()
- comment_src = getUtility(IDistroSeriesDifferenceCommentSource)
self.assertEqual(
- dsd_comment, comment_src.getForDifference(
+ dsd_comment, get_comment_source().getForDifference(
dsd_comment.distro_series_difference, dsd_comment.id))
+
+ def test_source_package_name_returns_package_name(self):
+ # The comment "knows" the name of the source package it's for.
+ package_name = self.factory.getUniqueUnicode()
+ dsd = self.factory.makeDistroSeriesDifference(
+ source_package_name_str=package_name)
+ comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+ self.assertEqual(package_name, comment.source_package_name)
+
+
+class TestDistroSeriesDifferenceCommentSource(TestCaseWithFactory):
+
+ layer = ZopelessDatabaseLayer
+
+ def test_implements_interface(self):
+ self.assertThat(
+ get_comment_source(),
+ Provides(IDistroSeriesDifferenceCommentSource))
+
+ def test_getForDistroSeries_returns_result_set(self):
+ series = self.factory.makeDistroSeries()
+ source = get_comment_source()
+ self.assertTrue(source.getForDistroSeries(series).is_empty())
+
+ def test_getForDistroSeries_matches_on_distroseries(self):
+ dsd = self.factory.makeDistroSeriesDifference()
+ series = dsd.derived_series
+ comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+ source = get_comment_source()
+ self.assertContentEqual([comment], source.getForDistroSeries(series))
+
+ def test_getForDistroSeries_filters_by_distroseries(self):
+ dsd = self.factory.makeDistroSeriesDifference()
+ other_series = self.factory.makeDistroSeries()
+ self.factory.makeDistroSeriesDifferenceComment(dsd)
+ source = get_comment_source()
+ self.assertContentEqual([], source.getForDistroSeries(other_series))
+
+ def test_getForDistroSeries_matches_on_since(self):
+ dsd = self.factory.makeDistroSeriesDifference()
+ series = dsd.derived_series
+ comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+ source = get_comment_source()
+ yesterday = comment.comment_date - timedelta(1)
+ self.assertContentEqual(
+ [comment], source.getForDistroSeries(series, since=yesterday))
+
+ def test_getForDistroSeries_filters_by_since(self):
+ dsd = self.factory.makeDistroSeriesDifference()
+ series = dsd.derived_series
+ comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+ source = get_comment_source()
+ tomorrow = comment.comment_date + timedelta(1)
+ self.assertContentEqual(
+ [], source.getForDistroSeries(series, since=tomorrow))
+
+ def test_getForDistroSeries_orders_by_age(self):
+ series = self.factory.makeDistroSeries()
+ dsds = randomize_list([
+ self.factory.makeDistroSeriesDifference(derived_series=series)
+ for counter in xrange(5)])
+ comments = [
+ self.factory.makeDistroSeriesDifferenceComment(dsd)
+ for dsd in dsds]
+ source = get_comment_source()
+ self.assertEqual(comments, list(source.getForDistroSeries(series)))
+
+ def test_getForDistroSeries_matches_on_package_name(self):
+ dsd = self.factory.makeDistroSeriesDifference()
+ series = dsd.derived_series
+ package_name = dsd.source_package_name.name
+ comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+ source = get_comment_source()
+ self.assertContentEqual([comment], source.getForDistroSeries(
+ series, source_package_name=package_name))
+
+ def test_getForDistroSeries_filters_by_package_name(self):
+ dsd = self.factory.makeDistroSeriesDifference()
+ series = dsd.derived_series
+ other_package = self.factory.getUniqueUnicode()
+ self.factory.makeDistroSeriesDifferenceComment(dsd)
+ source = get_comment_source()
+ self.assertContentEqual([], source.getForDistroSeries(
+ series, source_package_name=other_package))
