launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03330
lp:~allenap/launchpad/localpackagediffs-performance-bug-751321-queries into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/localpackagediffs-performance-bug-751321-queries into lp:launchpad with lp:~allenap/launchpad/localpackagediffs-performance-bug-751321-refactor as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-performance-bug-751321-queries/+merge/57778
This attempts to eager-load a ton of useful stuff for the +localpackagediffs page. It does this in several different ways, but the core test to look for is test_queries. This attempts to show that the query count is flat even after adding more differences to render. At the moment that is not true - bug 760733 has been filed to follow up on that - but this branch represents a big move in the right direction.
--
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-performance-bug-751321-queries/+merge/57778
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/localpackagediffs-performance-bug-751321-queries into lp:launchpad.
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2011-04-08 07:43:39 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2011-04-14 21:12:40 +0000
@@ -487,6 +487,9 @@
patch_collection_return_type(
IDistroSeries, 'getDerivedSeries', IDistroSeries)
+# IDistroSeriesDifference
+patch_reference_property(
+ IDistroSeriesDifference, 'latest_comment', IDistroSeriesDifferenceComment)
# IDistroSeriesDifferenceComment
IDistroSeriesDifferenceComment['comment_author'].schema = IPerson
=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py 2011-04-12 12:50:34 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py 2011-04-14 21:12:40 +0000
@@ -5,16 +5,25 @@
__metaclass__ = type
+import difflib
import re
+from textwrap import TextWrapper
from BeautifulSoup import BeautifulSoup
from lxml import html
import soupmatchers
from storm.zope.interfaces import IResultSet
-from testtools.matchers import EndsWith
+from testtools.content import Content
+from testtools.content_type import UTF8_TEXT
+from testtools.matchers import (
+ EndsWith,
+ LessThan,
+ )
from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from canonical.config import config
+from canonical.database.sqlbase import flush_database_caches
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.testing.pages import find_tag_by_id
from canonical.launchpad.webapp.batching import BatchNavigator
@@ -56,8 +65,10 @@
login_person,
person_logged_in,
set_feature_flag,
+ StormStatementRecorder,
TestCaseWithFactory,
)
+from lp.testing.matchers import HasQueryCount
from lp.testing.views import create_initialized_view
@@ -319,6 +330,100 @@
self._test_packagesets(
html, packageset_text, 'parent-packagesets', 'Parent packagesets')
+ def test_queries(self):
+ # With no DistroSeriesDifferences the query count should be low and
+ # fairly static. However, with some DistroSeriesDifferences the query
+ # count will be higher, but it should remain the same no matter how
+ # many differences there are.
+ login_person(self.simple_user)
+ derived_series = self.factory.makeDistroSeries(
+ parent_series=self.factory.makeDistroSeries())
+
+ def add_differences(num):
+ for index in xrange(num):
+ version = self.factory.getUniqueInteger()
+ versions = {
+ 'base': u'1.%d' % version,
+ 'derived': u'1.%dderived1' % version,
+ 'parent': u'1.%d-1' % version,
+ }
+ dsd = self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series,
+ versions=versions)
+
+ # Push a base_version in... not sure how better to do it.
+ removeSecurityProxy(dsd).base_version = versions["base"]
+
+ # Add a couple of comments.
+ self.factory.makeDistroSeriesDifferenceComment(dsd)
+ self.factory.makeDistroSeriesDifferenceComment(dsd)
+
+ # Update the spr, some with recipes, some with signing keys.
+ # SPR.uploader references both, and the uploader is referenced
+ # in the page.
+ spr = dsd.source_pub.sourcepackagerelease
+ if index % 2 == 0:
+ removeSecurityProxy(spr).source_package_recipe_build = (
+ self.factory.makeSourcePackageRecipeBuild(
+ sourcename=spr.sourcepackagename.name,
+ distroseries=derived_series))
+ else:
+ removeSecurityProxy(spr).dscsigningkey = (
+ self.factory.makeGPGKey(owner=spr.creator))
+
+ def flush_and_render():
+ flush_database_caches()
+ # Pull in the calling user's location so that it isn't recorded in
+ # the query count; it causes the total to be fragile for no
+ # readily apparent reason.
+ self.simple_user.location
+ with StormStatementRecorder() as recorder:
+ create_initialized_view(
+ derived_series, '+localpackagediffs',
+ principal=self.simple_user)()
+ return recorder
+
+ def statement_differ(rec1, rec2):
+ wrapper = TextWrapper(break_long_words=False)
+
+ def prepare_statements(rec):
+ for statement in rec.statements:
+ for line in wrapper.wrap(statement):
+ yield line
+ yield "-" * wrapper.width
+
+ def statement_diff():
+ diff = difflib.ndiff(
+ list(prepare_statements(rec1)),
+ list(prepare_statements(rec2)))
+ for line in diff:
+ yield "%s\n" % line
+
+ return statement_diff
+
+ # Render without differences and check the query count isn't silly.
+ recorder1 = flush_and_render()
+ self.assertThat(recorder1, HasQueryCount(LessThan(30)))
+ # Add some differences and render.
+ add_differences(2)
+ recorder2 = flush_and_render()
+ # Add more differences and render again.
+ add_differences(2)
+ recorder3 = flush_and_render()
+ # The last render should not need more queries than the previous.
+ self.addDetail(
+ "statement-diff", Content(
+ UTF8_TEXT, statement_differ(recorder2, recorder3)))
+ # XXX: GavinPanella 2011-04-12 bug=760733: Reducing the query count
+ # further needs work. Ideally this test would be along the lines of
+ # recorder3.count == recorder2.count. 8 queries above the recorder2
+ # count is 4 queries per difference which is not acceptable, but is
+ # *far* better than without the changes introduced by landing this.
+ compromise_statement_count = recorder2.count + 8
+ self.assertThat(
+ recorder3, HasQueryCount(
+ LessThan(compromise_statement_count + 1)))
+
class TestDistroSeriesLocalDifferencesZopeless(TestCaseWithFactory):
"""Test the distroseries +localpackagediffs view."""
@@ -519,6 +624,8 @@
# Delete the publications.
difference.source_pub.status = PackagePublishingStatus.DELETED
difference.parent_source_pub.status = PackagePublishingStatus.DELETED
+ # Flush out the changes and invalidate caches (esp. property caches).
+ flush_database_caches()
set_derived_series_ui_feature_flag(self)
view = create_initialized_view(
=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py 2011-04-14 20:06:33 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py 2011-04-14 21:12:40 +0000
@@ -188,6 +188,11 @@
:return: True if the record was updated, False otherwise.
"""
+ latest_comment = Reference(
+ Interface, # IDistroSeriesDifferenceComment
+ title=_("The latest comment"),
+ readonly=True)
+
def getComments():
"""Return a result set of the comments for this difference."""
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2011-04-13 14:44:31 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2011-04-14 21:12:40 +0000
@@ -9,18 +9,25 @@
'DistroSeriesDifference',
]
+from itertools import chain
+from operator import itemgetter
+
from debian.changelog import (
Changelog,
Version,
)
from lazr.enum import DBItem
from sqlobject import StringCol
-from storm.expr import Desc
+from storm.expr import (
+ And,
+ compile as storm_compile,
+ Desc,
+ SQL,
+ )
+from storm.info import ClassAlias
from storm.locals import (
- And,
Int,
Reference,
- Storm,
)
from storm.zope.interfaces import IResultSet
from zope.component import getUtility
@@ -30,10 +37,15 @@
)
from canonical.database.enumcol import DBEnum
+from canonical.launchpad.components.decoratedresultset import (
+ DecoratedResultSet,
+ )
+from canonical.launchpad.database.message import Message
from canonical.launchpad.interfaces.lpstorm import (
IMasterStore,
IStore,
)
+from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
from lp.registry.enum import (
DistroSeriesDifferenceStatus,
DistroSeriesDifferenceType,
@@ -49,25 +61,130 @@
from lp.registry.interfaces.distroseriesdifferencecomment import (
IDistroSeriesDifferenceCommentSource,
)
+from lp.registry.interfaces.person import IPersonSet
+from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.distroseriesdifferencecomment import (
DistroSeriesDifferenceComment,
)
+from lp.registry.model.gpgkey import GPGKey
from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.services.database import bulk
+from lp.services.database.stormbase import StormBase
from lp.services.propertycache import (
cachedproperty,
clear_property_cache,
+ get_property_cache,
)
from lp.soyuz.enums import (
+ ArchivePurpose,
PackageDiffStatus,
PackagePublishingStatus,
)
from lp.soyuz.interfaces.packageset import IPackagesetSet
+from lp.soyuz.model.archive import Archive
from lp.soyuz.model.distroseriessourcepackagerelease import (
DistroSeriesSourcePackageRelease,
)
-
-
-class DistroSeriesDifference(Storm):
+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+
+
+def most_recent_publications(dsds, in_parent, statuses, match_version=False):
+ """The most recent publications for the given `DistroSeriesDifference`s.
+
+ Returns an `IResultSet` that yields two columns: `SourcePackageName.id`
+ and `SourcePackagePublishingHistory`.
+
+ :param dsds: An iterable of `DistroSeriesDifference` instances.
+ :param in_parent: A boolean indicating if we should look in the parent
+ series' archive instead of the derived series' archive.
+ """
+ distinct_on = "DistroSeriesDifference.source_package_name"
+ columns = (
+ # XXX: GavinPanella 2010-04-06 bug=374777: This SQL(...) is a hack; it
+ # does not seem to be possible to express DISTINCT ON with Storm.
+ SQL("DISTINCT ON (%s) 0 AS ignore" % distinct_on),
+ DistroSeriesDifference.source_package_name_id,
+ SourcePackagePublishingHistory,
+ )
+ conditions = And(
+ DistroSeriesDifference.id.is_in(dsd.id for dsd in dsds),
+ DistroSeries.id == DistroSeriesDifference.derived_series_id,
+ SourcePackagePublishingHistory.archiveID == Archive.id,
+ SourcePackagePublishingHistory.sourcepackagereleaseID == (
+ SourcePackageRelease.id),
+ SourcePackagePublishingHistory.status.is_in(statuses),
+ SourcePackageRelease.sourcepackagenameID == (
+ DistroSeriesDifference.source_package_name_id),
+ )
+ # Check in the parent archive or the child?
+ if in_parent:
+ ParentDistroSeries = ClassAlias(DistroSeries)
+ conditions = And(
+ conditions,
+ ParentDistroSeries.id == DistroSeries.parent_seriesID,
+ Archive.distributionID == ParentDistroSeries.distributionID,
+ Archive.purpose == ArchivePurpose.PRIMARY,
+ )
+ else:
+ conditions = And(
+ conditions,
+ Archive.distributionID == DistroSeries.distributionID,
+ Archive.purpose == ArchivePurpose.PRIMARY,
+ )
+ # Do we match on DistroSeriesDifference.(parent_)source_version?
+ if match_version:
+ if in_parent:
+ version_column = DistroSeriesDifference.parent_source_version
+ else:
+ version_column = DistroSeriesDifference.source_version
+ conditions = And(
+ conditions,
+ SourcePackageRelease.version == version_column,
+ )
+ # The sort order is critical so that the DISTINCT ON clause selects the
+ # most recent publication (i.e. the one with the highest id).
+ order_by = (
+ DistroSeriesDifference.source_package_name_id,
+ Desc(SourcePackagePublishingHistory.id),
+ )
+ store = IStore(SourcePackagePublishingHistory)
+ results = store.find(columns, conditions).order_by(*order_by)
+ return DecoratedResultSet(results, itemgetter(1, 2))
+
+
+def most_recent_comments(dsds):
+ """The most recent comments for the given `DistroSeriesDifference`s.
+
+ Returns an `IResultSet` that yields a single column of
+ `DistroSeriesDifferenceComment`.
+
+ :param dsds: An iterable of `DistroSeriesDifference` instances.
+ """
+ distinct_on = storm_compile(
+ DistroSeriesDifferenceComment.distro_series_difference_id)
+ columns = (
+ # XXX: GavinPanella 2010-04-06 bug=374777: This SQL(...) is a
+ # hack; it does not seem to be possible to express DISTINCT ON
+ # with Storm.
+ SQL("DISTINCT ON (%s) 0 AS ignore" % distinct_on),
+ DistroSeriesDifferenceComment,
+ Message,
+ )
+ conditions = And(
+ DistroSeriesDifferenceComment
+ .distro_series_difference_id.is_in(dsd.id for dsd in dsds),
+ Message.id == DistroSeriesDifferenceComment.message_id)
+ order_by = (
+ DistroSeriesDifferenceComment.distro_series_difference_id,
+ Desc(DistroSeriesDifferenceComment.id),
+ )
+ store = IStore(DistroSeriesDifferenceComment)
+ comments = store.find(columns, conditions).order_by(*order_by)
+ return DecoratedResultSet(comments, itemgetter(1))
+
+
+class DistroSeriesDifference(StormBase):
"""See `DistroSeriesDifference`."""
implements(IDistroSeriesDifference)
classProvides(IDistroSeriesDifferenceSource)
@@ -154,10 +271,90 @@
DistroSeriesDifference.source_version >
DistroSeriesDifference.parent_source_version])
- return IStore(DistroSeriesDifference).find(
+ differences = IStore(DistroSeriesDifference).find(
DistroSeriesDifference,
And(*conditions)).order_by(SourcePackageName.name)
+ def eager_load(dsds):
+ source_pubs = dict(
+ most_recent_publications(
+ dsds, in_parent=False, statuses=(
+ PackagePublishingStatus.PUBLISHED,
+ PackagePublishingStatus.PENDING)))
+ parent_source_pubs = dict(
+ most_recent_publications(
+ dsds, in_parent=True, statuses=(
+ PackagePublishingStatus.PUBLISHED,
+ PackagePublishingStatus.PENDING)))
+
+ source_pubs_for_release = dict(
+ most_recent_publications(
+ dsds, in_parent=False, statuses=(
+ PackagePublishingStatus.PUBLISHED,),
+ match_version=True))
+ parent_source_pubs_for_release = dict(
+ most_recent_publications(
+ dsds, in_parent=True, statuses=(
+ PackagePublishingStatus.PUBLISHED,),
+ match_version=True))
+
+ latest_comment_by_dsd_id = dict(
+ (comment.distro_series_difference_id, comment)
+ for comment in most_recent_comments(dsds))
+ latest_comments = latest_comment_by_dsd_id.values()
+
+ for dsd in dsds:
+ spn_id = dsd.source_package_name_id
+ cache = get_property_cache(dsd)
+ cache.source_pub = source_pubs.get(spn_id)
+ cache.parent_source_pub = parent_source_pubs.get(spn_id)
+ if dsd.id in source_pubs_for_release:
+ cache.source_package_release = (
+ DistroSeriesSourcePackageRelease(
+ dsd.derived_series,
+ source_pubs_for_release[dsd.id]))
+ if dsd.id in parent_source_pubs_for_release:
+ cache.parent_source_package_release = (
+ DistroSeriesSourcePackageRelease(
+ dsd.derived_series.parent_series,
+ parent_source_pubs_for_release[dsd.id]))
+ cache.latest_comment = latest_comment_by_dsd_id.get(dsd.id)
+
+ # SourcePackageReleases of the parent and source pubs are often
+ # referred to.
+ sprs = bulk.load_related(
+ SourcePackageRelease, chain(
+ source_pubs.itervalues(),
+ parent_source_pubs.itervalues()),
+ ("sourcepackagereleaseID",))
+
+ # SourcePackageRelease.uploader can end up getting the requester
+ # for a source package recipe build.
+ sprbs = bulk.load_related(
+ SourcePackageRecipeBuild, sprs,
+ ("source_package_recipe_build_id",))
+
+ # SourcePackageRelease.uploader can end up getting the owner of
+ # the DSC signing key.
+ gpgkeys = bulk.load_related(GPGKey, sprs, ("dscsigningkeyID",))
+
+ # Load DistroSeriesDifferenceComment owners,
+ # SourcePackageRecipeBuild requesters and GPGKey owners.
+ person_ids = set().union(
+ (dsdc.message.ownerID for dsdc in latest_comments),
+ (sprb.requester_id for sprb in sprbs),
+ (gpgkey.ownerID for gpgkey in gpgkeys))
+ uploaders = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+ person_ids, need_validity=True)
+ list(uploaders)
+
+ # Load SourcePackageNames.
+ bulk.load_related(
+ SourcePackageName, dsds, ("source_package_name_id",))
+
+ return DecoratedResultSet(
+ differences, pre_iter_hook=eager_load)
+
@staticmethod
def getByDistroSeriesAndName(distro_series, source_package_name):
"""See `IDistroSeriesDifferenceSource`."""
@@ -178,6 +375,21 @@
"""See `IDistroSeriesDifference`."""
return self._getLatestSourcePub(for_parent=True)
+ def _getLatestSourcePub(self, for_parent=False):
+ """Helper to keep source_pub/parent_source_pub DRY."""
+ distro_series = self.derived_series
+ if for_parent:
+ distro_series = self.derived_series.parent_series
+
+ pubs = distro_series.getPublishedSources(
+ self.source_package_name, include_pending=True)
+
+ # The most recent published source is the first one.
+ try:
+ return pubs[0]
+ except IndexError:
+ return None
+
@cachedproperty
def base_source_pub(self):
"""See `IDistroSeriesDifference`."""
@@ -284,28 +496,13 @@
else:
return self.parent_package_diff.status
- def _getLatestSourcePub(self, for_parent=False):
- """Helper to keep source_pub/parent_source_pub DRY."""
- distro_series = self.derived_series
- if for_parent:
- distro_series = self.derived_series.parent_series
-
- pubs = distro_series.getPublishedSources(
- self.source_package_name, include_pending=True)
-
- # The most recent published source is the first one.
- try:
- return pubs[0]
- except IndexError:
- return None
-
- @property
+ @cachedproperty
def parent_source_package_release(self):
return self._package_release(
self.derived_series.parent_series,
self.parent_source_version)
- @property
+ @cachedproperty
def source_package_release(self):
return self._package_release(
self.derived_series,
@@ -438,6 +635,11 @@
return getUtility(IDistroSeriesDifferenceCommentSource).new(
self, commenter, comment)
+ @cachedproperty
+ def latest_comment(self):
+ """See `IDistroSeriesDifference`."""
+ return self.getComments().first()
+
def getComments(self):
"""See `IDistroSeriesDifference`."""
DSDComment = DistroSeriesDifferenceComment
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt 2011-04-06 07:52:33 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2011-04-14 21:12:40 +0000
@@ -62,6 +62,12 @@
source_pub difference/source_pub;
src_name difference/source_package_name/name;"
tal:attributes="class src_name">
+
+ <tal:comment replace="nothing">
+ XXX: GavinPanella 2011-04-14 bug=760733: In
+ TestDistroSeriesLocalDifferences.test_queries, this
+ cell needs 0 QUERIES per row.
+ </tal:comment>
<td>
<input tal:condition="view/canPerformSync"
name="field.selected_differences" type="checkbox"
@@ -72,6 +78,13 @@
<a tal:attributes="href difference/fmt:url" class="toggle-extra"
tal:content="src_name">Foo</a>
</td>
+
+ <tal:comment replace="nothing">
+ XXX: GavinPanella 2011-04-14 bug=760733: In
+ TestDistroSeriesLocalDifferences.test_queries, this
+ cell needs 1 QUERY per row:
+ SourcePackagePublishingHistory
+ </tal:comment>
<td tal:condition="view/show_parent_version">
<a tal:condition="difference/parent_source_package_release"
tal:attributes="href difference/parent_source_package_release/fmt:url"
@@ -83,6 +96,12 @@
tal:content="difference/parent_source_version">
</span>
</td>
+
+ <tal:comment replace="nothing">
+ XXX: GavinPanella 2011-04-14 bug=760733: In
+ TestDistroSeriesLocalDifferences.test_queries, this
+ cell needs 1 QUERY per row: SourcePackagePublishingHistory
+ </tal:comment>
<td tal:condition="view/show_derived_version">
<a tal:condition="difference/source_package_release"
tal:attributes="href difference/source_package_release/fmt:url"
@@ -93,33 +112,67 @@
class="derived-version"
tal:content="difference/source_version">
</span>
- </td>
+ </td>
+
+ <tal:comment replace="nothing">
+ XXX: GavinPanella 2011-04-14 bug=760733: In
+ TestDistroSeriesLocalDifferences.test_queries, this
+ cell needs 1 QUERY per row: PackageSet
+ </tal:comment>
<td tal:condition="view/show_parent_packagesets"
class="parent-packagesets">
<tal:replace replace="difference/@@/parent_packagesets_names"/>
</td>
+
+ <tal:comment replace="nothing">
+ XXX: GavinPanella 2011-04-14 bug=760733: In
+ TestDistroSeriesLocalDifferences.test_queries, this
+ cell needs 0 QUERIES per row.
+ </tal:comment>
<td tal:condition="view/show_packagesets"
class="packagesets">
<tal:replace replace="difference/@@/packagesets_names" />
</td>
+
+ <tal:comment replace="nothing">
+ XXX: GavinPanella 2011-04-14 bug=760733: In
+ TestDistroSeriesLocalDifferences.test_queries, this
+ cell needs 1 QUERY per row.
+ </tal:comment>
<td>
<tal:parent condition="not: view/show_derived_version">
- <span tal:attributes="title difference/parent_source_pub/datepublished/fmt:datetime"
- tal:content="difference/parent_source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
- <tal:signer condition="parent_source_pub/sourcepackagerelease/uploader">
- by <a tal:replace="structure parent_source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
- </tal:signer>
+ <tal:published condition="parent_source_pub">
+ <span tal:attributes="title parent_source_pub/datepublished/fmt:datetime"
+ tal:content="parent_source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
+ <tal:signer condition="parent_source_pub/sourcepackagerelease/uploader">
+ by <a tal:replace="structure parent_source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
+ </tal:signer>
+ </tal:published>
+ <tal:not-published condition="not:parent_source_pub">
+ <span tal:content="difference/parent_source_version" />
+ </tal:not-published>
</tal:parent>
<tal:derived condition="view/show_derived_version">
- <span tal:attributes="title difference/source_pub/datepublished/fmt:datetime"
- tal:content="difference/source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
- <tal:signer condition="source_pub/sourcepackagerelease/uploader">
- by <a tal:replace="structure source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
- </tal:signer>
+ <tal:published condition="parent_source_pub">
+ <span tal:attributes="title source_pub/datepublished/fmt:datetime"
+ tal:content="source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
+ <tal:signer condition="source_pub/sourcepackagerelease/uploader">
+ by <a tal:replace="structure source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
+ </tal:signer>
+ </tal:published>
+ <tal:not-published condition="not:source_pub">
+ <span tal:content="difference/source_version" />
+ </tal:not-published>
</tal:derived>
</td>
+
+ <tal:comment replace="nothing">
+ XXX: GavinPanella 2011-04-14 bug=760733: In
+ TestDistroSeriesLocalDifferences.test_queries, this
+ cell needs 1 QUERY per row: MessageChunk
+ </tal:comment>
<td>
- <tal:comment tal:define="comment python:difference.getComments().first();"
+ <tal:comment tal:define="comment difference/latest_comment"
tal:condition="comment">
<span tal:replace="comment/body_text/fmt:shorten/50">I'm on this.</span>
<br /><span class="greyed-out greylink"><span
@@ -128,6 +181,7 @@
comment/comment_author/fmt:link">joesmith</a></span>
</tal:comment>
</td>
+
</tr>
</tal:difference>
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2011-04-14 20:06:33 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-04-14 21:12:40 +0000
@@ -29,6 +29,10 @@
IDistroSeriesDifference,
IDistroSeriesDifferenceSource,
)
+from lp.registry.model.distroseriesdifference import (
+ most_recent_comments,
+ most_recent_publications,
+ )
from lp.services.propertycache import get_property_cache
from lp.soyuz.enums import PackageDiffStatus
from lp.soyuz.interfaces.publishing import PackagePublishingStatus
@@ -322,6 +326,33 @@
self.assertEqual(
[dsd_comment_2, dsd_comment], list(ds_diff.getComments()))
+<<<<<<< TREE
+=======
+ def test_latest_comment(self):
+ # latest_comment is a property containing the most recent comment.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+
+ with person_logged_in(ds_diff.owner):
+ comments = [
+ ds_diff.addComment(
+ ds_diff.owner, "Wait until version 2.1"),
+ ds_diff.addComment(
+ ds_diff.owner, "Wait until version 2.1"),
+ ]
+
+ self.assertEqual(comments[-1], ds_diff.latest_comment)
+
+ def test_addComment_not_public(self):
+ # Comments cannot be added with launchpad.View.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ person = self.factory.makePerson()
+
+ with person_logged_in(person):
+ self.assertTrue(check_permission('launchpad.View', ds_diff))
+ self.assertFalse(check_permission('launchpad.Edit', ds_diff))
+ self.assertRaises(Unauthorized, getattr, ds_diff, 'addComment')
+
+>>>>>>> MERGE-SOURCE
def test_addComment_for_owners(self):
# Comments can be added by any of the owners of the derived
# series.
@@ -844,3 +875,114 @@
ds_diff.derived_series, 'fooname')
self.assertEqual(ds_diff, result)
+
+
+class TestMostRecentComments(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_most_recent_comments(self):
+ derived_series = self.factory.makeDistroSeries(
+ parent_series=self.factory.makeDistroSeries())
+ dsds = set(
+ self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series) for index in xrange(5))
+ expected_comments = set()
+ for dsd in dsds:
+ # Add a couple of comments.
+ self.factory.makeDistroSeriesDifferenceComment(dsd)
+ expected_comments.add(
+ self.factory.makeDistroSeriesDifferenceComment(dsd))
+ self.assertContentEqual(
+ expected_comments, most_recent_comments(dsds))
+
+
+class TestMostRecentPublications(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def create_difference(self, derived_series):
+ # Create a new DistroSeriesDifference
+ version = self.factory.getUniqueInteger()
+ versions = {
+ 'base': u'1.%d' % version,
+ 'derived': u'1.%dderived1' % version,
+ 'parent': u'1.%d-1' % version,
+ }
+ dsd = self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series,
+ versions=versions)
+ # Push a base_version in... not sure how better to do it.
+ removeSecurityProxy(dsd).base_version = versions["base"]
+ return dsd
+
+ def test_simple(self):
+ derived_series = self.factory.makeDistroSeries(
+ parent_series=self.factory.makeDistroSeries())
+ dsds = [
+ self.create_difference(derived_series),
+ self.create_difference(derived_series),
+ ]
+ # Derived publication.
+ source_pubs_by_spn_id_expected = set(
+ (dsd.source_package_name.id, dsd.source_pub)
+ for dsd in dsds)
+ source_pubs_by_spn_id_found = most_recent_publications(
+ dsds, in_parent=False, statuses=(
+ PackagePublishingStatus.PUBLISHED,
+ PackagePublishingStatus.PENDING))
+ self.assertContentEqual(
+ source_pubs_by_spn_id_expected,
+ source_pubs_by_spn_id_found)
+ # Parent publication
+ parent_source_pubs_by_spn_id_expected = set(
+ (dsd.source_package_name.id, dsd.parent_source_pub)
+ for dsd in dsds)
+ parent_source_pubs_by_spn_id_found = most_recent_publications(
+ dsds, in_parent=True, statuses=(
+ PackagePublishingStatus.PUBLISHED,
+ PackagePublishingStatus.PENDING))
+ self.assertContentEqual(
+ parent_source_pubs_by_spn_id_expected,
+ parent_source_pubs_by_spn_id_found)
+
+ def test_statuses(self):
+ derived_series = self.factory.makeDistroSeries(
+ parent_series=self.factory.makeDistroSeries())
+ dsd = self.create_difference(derived_series)
+ # Change the derived source publication to DELETED.
+ removeSecurityProxy(dsd.source_pub).status = (
+ PackagePublishingStatus.DELETED)
+ # Searching for DELETED will find the source publication.
+ self.assertContentEqual(
+ [(dsd.source_package_name.id, dsd.source_pub)],
+ most_recent_publications(
+ [dsd], in_parent=False, statuses=(
+ PackagePublishingStatus.DELETED,)))
+ # Searched for DELETED will *not* find the parent publication.
+ self.assertContentEqual(
+ [], most_recent_publications(
+ [dsd], in_parent=True, statuses=(
+ PackagePublishingStatus.DELETED,)))
+
+ def test_match_version(self):
+ # When match_version is True, the version of the publications (well,
+ # the release) must exactly match those recorded on the
+ # DistroSeriesDifference.
+ derived_series = self.factory.makeDistroSeries(
+ parent_series=self.factory.makeDistroSeries())
+ dsd = self.create_difference(derived_series)
+ # Modify the release version.
+ removeSecurityProxy(
+ dsd.source_package_release.sourcepackagerelease).version += u"2"
+ # Searching with match_version=False finds the publication.
+ self.assertContentEqual(
+ [(dsd.source_package_name.id, dsd.source_pub)],
+ most_recent_publications(
+ [dsd], in_parent=False, match_version=False,
+ statuses=(PackagePublishingStatus.PUBLISHED,)))
+ # Searching with match_version=True does not find the publication.
+ self.assertContentEqual(
+ [], most_recent_publications(
+ [dsd], in_parent=False, match_version=True,
+ statuses=(PackagePublishingStatus.PUBLISHED,)))
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2011-04-07 01:08:21 +0000
+++ lib/lp/testing/__init__.py 2011-04-14 21:12:40 +0000
@@ -6,19 +6,20 @@
from __future__ import absolute_import
from lp.testing.windmill.lpuser import LaunchpadUser
+
__metaclass__ = type
__all__ = [
'ANONYMOUS',
'anonymous_logged_in',
'api_url',
+ 'BrowserTestCase',
'build_yui_unittest_suite',
- 'BrowserTestCase',
'celebrity_logged_in',
'FakeTime',
'get_lsb_information',
'is_logged_in',
+ 'launchpadlib_credentials_for',
'launchpadlib_for',
- 'launchpadlib_credentials_for',
'login',
'login_as',
'login_celebrity',
@@ -31,13 +32,13 @@
'person_logged_in',
'quote_jquery_expression',
'record_statements',
+ 'run_script',
'run_with_login',
'run_with_storm_debug',
- 'run_script',
'StormStatementRecorder',
+ 'test_tales',
'TestCase',
'TestCaseWithFactory',
- 'test_tales',
'time_counter',
'unlink_source_packages',
'validate_mock_class',
@@ -166,6 +167,26 @@
lpuser,
)
+# The following names have been imported for the purpose of being
+# exported. They are referred to here to silence lint warnings.
+anonymous_logged_in
+api_url
+celebrity_logged_in
+is_logged_in
+launchpadlib_credentials_for
+launchpadlib_for
+login_as
+login_celebrity
+login_person
+login_team
+oauth_access_token_for
+person_logged_in
+run_with_login
+test_tales
+with_anonymous_login
+with_celebrity_logged_in
+with_person_logged_in
+
class FakeTime:
"""Provides a controllable implementation of time.time().
@@ -809,7 +830,6 @@
person.displayname, naked_person.preferredemail.email, password)
return self.getClientFor(url, user=user)
-
def getClientForAnomymous(self, obj, view_name=None):
"""Return a new client, and the url that it has loaded."""
client = WindmillTestClient(self.suite_name)
Follow ups
