Merge lp:~bac/launchpad/bug-590813 into lp:launchpad

[Brad Crittenden]

Don't snapshot collections in IProduct and IDistribution.

= Summary =

Some products have lots of series...sometimes over 1000, which breaks
ShortList. Snapshotting those series is a huge waste of time.

== Proposed fix ==

Add 'doNotSnapshot' to the interface definition of CollectionFields in
IProduct and IDistribution.

Also did some drive-by lint, spelling, and grammar fixes.

== Pre-implementation notes ==

Talk with Curtis.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt test_snapshot

== Demo and Q/A ==

Nope

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/browser/stringformatter.py
  lib/lp/registry/interfaces/product.py
  lib/lp/registry/interfaces/distribution.py
  lib/lp/registry/tests/test_distribution.py
  lib/lp/registry/tests/test_person.py
  lib/lp/registry/tests/test_product.py
  lib/lp/app/browser/tests/root-views.txt

./lib/lp/app/browser/stringformatter.py
     405: E501 line too long (90 characters)
     409: E501 line too long (88 characters)
     405: Line exceeds 78 characters.
     409: Line exceeds 78 characters.
./lib/lp/registry/interfaces/product.py
     903: E301 expected 1 blank line, found 2

[Jelmer Vernooij (jelmer)]

review: Approve

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2010-06-14 04:29:01 +0000
+++ lib/lp/app/browser/stringformatter.py	2010-08-12 19:37:44 +0000
@@ -156,6 +156,7 @@
 
     The text may contain entity references or HTML tags.
     """
+
     def replace(match):
         if match.group('tag'):
             return match.group()
@@ -176,7 +177,7 @@
 
     def nl_to_br(self):
         """Quote HTML characters, then replace newlines with <br /> tags."""
-        return cgi.escape(self._stringtoformat).replace('\n','<br />\n')
+        return cgi.escape(self._stringtoformat).replace('\n', '<br />\n')
 
     def escape(self):
         return escape(self._stringtoformat)
@@ -580,7 +581,7 @@
             if in_fold and line.endswith('</p>') and in_false_paragraph:
                 # The line ends with a false paragraph in a PGP signature.
                 # Restore the line break to join with the next paragraph.
-                line = '%s<br />\n<br />' %  strip_trailing_p_tag(line)
+                line = '%s<br />\n<br />' % strip_trailing_p_tag(line)
             elif (in_quoted and self._re_quoted.match(line) is None):
                 # The line is not quoted like the previous line.
                 # End fold before we append this line.
@@ -684,7 +685,8 @@
             if person is not None and not person.hide_email_addresses:
                 # Circular dependancies now. Should be resolved by moving the
                 # object image display api.
-                from canonical.launchpad.webapp.tales import ObjectImageDisplayAPI
+                from canonical.launchpad.webapp.tales import (
+                    ObjectImageDisplayAPI)
                 css_sprite = ObjectImageDisplayAPI(person).sprite_css()
                 text = text.replace(
                     address, '<a href="%s" class="%s">%s</a>' % (
@@ -770,7 +772,7 @@
         letter.
 
         :param prefix: an optional string to prefix to the id. It can be
-            used to ensure that the start of the id is predicable.
+            used to ensure that the start of the id is predictable.
         """
         if prefix is not None:
             raw_text = prefix + self._stringtoformat

=== modified file 'lib/lp/app/browser/tests/root-views.txt'
--- lib/lp/app/browser/tests/root-views.txt	2010-01-08 21:23:15 +0000
+++ lib/lp/app/browser/tests/root-views.txt	2010-08-12 19:37:44 +0000
@@ -4,7 +4,7 @@
 The launchpad front page uses the LaunchpadRootIndexView to provide the
 special data needed for the layout.
 
-    # The _get_day_of_year() method must be hacked to return a predicable day
+    # The _get_day_of_year() method must be hacked to return a predictable day
     # to testing the view.
     >>> from lp.app.browser.root import LaunchpadRootIndexView
     >>> def day():

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2010-08-06 11:52:05 +0000
+++ lib/lp/registry/interfaces/distribution.py	2010-08-12 19:37:44 +0000
@@ -23,6 +23,7 @@
 from zope.schema import Bool, Choice, Datetime, List, Object, Text, TextLine
 from zope.interface import Attribute, Interface
 
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.fields import CollectionField, Reference
 from lazr.restful.declarations import (
    collection_default_content, export_as_webservice_collection,
@@ -200,25 +201,27 @@
     lucilleconfig = TextLine(
         title=_("Lucille Config"),
         description=_("The Lucille Config."), required=False)
-    archive_mirrors = exported(CollectionField(
-        description=_("All enabled and official ARCHIVE mirrors of this "
-                      "Distribution."),
-        readonly=True, value_type=Object(schema=IDistributionMirror)))
-    cdimage_mirrors = exported(CollectionField(
-        description=_("All enabled and official RELEASE mirrors of this "
-                      "Distribution."),
-        readonly=True, value_type=Object(schema=IDistributionMirror)))
+    archive_mirrors = exported(doNotSnapshot(
+        CollectionField(
+            description=_("All enabled and official ARCHIVE mirrors "
+                          "of this Distribution."),
+            readonly=True, value_type=Object(schema=IDistributionMirror))))
+    cdimage_mirrors = exported(doNotSnapshot(
+        CollectionField(
+            description=_("All enabled and official RELEASE mirrors "
+                          "of this Distribution."),
+            readonly=True, value_type=Object(schema=IDistributionMirror))))
     disabled_mirrors = Attribute(
         "All disabled and official mirrors of this Distribution.")
     unofficial_mirrors = Attribute(
         "All unofficial mirrors of this Distribution.")
     pending_review_mirrors = Attribute(
         "All mirrors of this Distribution that haven't been reviewed yet.")
-    series = exported(
+    series = exported(doNotSnapshot(
         CollectionField(
             title=_("DistroSeries inside this Distribution"),
             # Really IDistroSeries, see _schema_circular_imports.py.
-            value_type=Reference(schema=Interface)))
+            value_type=Reference(schema=Interface))))
     architectures = List(
         title=_("DistroArchSeries inside this Distribution"))
     uploaders = Attribute(_(
@@ -262,11 +265,11 @@
             # Really IArchive, see _schema_circular_imports.py.
             schema=Interface))
 
-    all_distro_archives = exported(
+    all_distro_archives = exported(doNotSnapshot(
         CollectionField(
             title=_("A sequence of the distribution's non-PPA Archives."),
             readonly=True, required=False,
-            value_type=Reference(schema=Interface)),
+            value_type=Reference(schema=Interface))),
                 # Really IArchive, see _schema_circular_imports.py.
         exported_as='archives')
 

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2010-08-06 14:49:35 +0000
+++ lib/lp/registry/interfaces/product.py	2010-08-12 19:37:44 +0000
@@ -74,6 +74,7 @@
     ITranslationPolicy)
 from canonical.launchpad.validators import LaunchpadValidationError
 from canonical.launchpad.validators.name import name_validator
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.fields import CollectionField, Reference, ReferenceChoice
 from lazr.restful.declarations import (
     REQUEST_USER, call_with, collection_default_content,
@@ -585,7 +586,8 @@
         "this product"))
 
     series = exported(
-        CollectionField(value_type=Object(schema=IProductSeries)))
+        doNotSnapshot(
+            CollectionField(value_type=Object(schema=IProductSeries))))
 
     development_focus = exported(
         ReferenceChoice(
@@ -602,10 +604,12 @@
         "product; otherwise, simply returns the product name."))
 
     releases = exported(
-        CollectionField(
-            title=_("An iterator over the ProductReleases for this product."),
-            readonly=True,
-            value_type=Reference(schema=IProductRelease)))
+        doNotSnapshot(
+            CollectionField(
+                title=_("An iterator over the ProductReleases for "
+                        "this product."),
+                readonly=True,
+                value_type=Reference(schema=IProductRelease))))
 
     translation_focus = exported(
         ReferenceChoice(

=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2010-08-03 15:15:16 +0000
+++ lib/lp/registry/tests/test_distribution.py	2010-08-12 19:37:44 +0000
@@ -7,10 +7,13 @@
 
 from zope.security.proxy import removeSecurityProxy
 
+from lazr.lifecycle.snapshot import Snapshot
 from lp.registry.tests.test_distroseries import (
     TestDistroSeriesCurrentSourceReleases)
 from lp.registry.interfaces.distroseries import NoSuchDistroSeries
 from lp.registry.interfaces.series import SeriesStatus
+from lp.registry.interfaces.distribution import IDistribution
+
 from lp.soyuz.interfaces.distributionsourcepackagerelease import (
     IDistributionSourcePackageRelease)
 from lp.testing import TestCaseWithFactory
@@ -140,3 +143,31 @@
         series = self.factory.makeDistroSeries(distribution=distro,
             name="dappere", version="42.6")
         self.assertEquals(series, distro.getSeries("42.6"))
+
+
+class DistroSnapshotTestCase(TestCaseWithFactory):
+    """A TestCase for distribution snapshots."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(DistroSnapshotTestCase, self).setUp()
+        self.distribution = self.factory.makeDistribution(name="boobuntu")
+
+    def test_snapshot(self):
+        """Snapshots of products should not include marked attribues.
+
+        Wrap an export with 'doNotSnapshot' to force the snapshot to not
+        include that attribute.
+        """
+        snapshot = Snapshot(self.distribution, providing=IDistribution)
+        omitted = [
+            'archive_mirrors',
+            'cdimage_mirrors',
+            'series',
+            'all_distro_archives',
+            ]
+        for attribute in omitted:
+            self.assertFalse(
+                hasattr(snapshot, attribute),
+                "Snapshot should not include %s." % attribute)

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-08-05 13:23:52 +0000
+++ lib/lp/registry/tests/test_person.py	2010-08-12 19:37:44 +0000
@@ -249,8 +249,8 @@
         transaction.commit()
         logout()
 
-    def _get_testible_account(self, person, date_created, openid_identifier):
-        # Return a naked account with predicable attributes.
+    def _get_testable_account(self, person, date_created, openid_identifier):
+        # Return a naked account with predictable attributes.
         account = removeSecurityProxy(person.account)
         account.date_created = date_created
         account.openid_identifier = openid_identifier
@@ -264,7 +264,7 @@
             2010, 04, 01, 0, 0, 0, 0, tzinfo=pytz.timezone('UTC'))
         # Free an OpenID identifier using merge.
         first_duplicate = self.factory.makePerson()
-        first_account = self._get_testible_account(
+        first_account = self._get_testable_account(
             first_duplicate, test_date, test_identifier)
         first_person = self.factory.makePerson()
         self._do_premerge(first_duplicate, first_person)
@@ -276,7 +276,7 @@
         # Create an account that reuses the freed OpenID_identifier.
         test_date = test_date.replace(2010, 05)
         second_duplicate = self.factory.makePerson()
-        second_account = self._get_testible_account(
+        second_account = self._get_testable_account(
             second_duplicate, test_date, test_identifier)
         second_person = self.factory.makePerson()
         self._do_premerge(second_duplicate, second_person)

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2010-08-02 20:24:24 +0000
+++ lib/lp/registry/tests/test_product.py	2010-08-12 19:37:44 +0000
@@ -18,14 +18,16 @@
 
 from canonical.launchpad.ftests import syncUpdate
 
+from lazr.lifecycle.snapshot import Snapshot
 from lp.registry.interfaces.person import IPersonSet
-from lp.registry.interfaces.product import License
+from lp.registry.interfaces.product import IProduct, License
 from lp.registry.model.product import Product
 from lp.registry.model.productlicense import ProductLicense
 from lp.registry.model.commercialsubscription import (
     CommercialSubscription)
 from lp.testing import TestCaseWithFactory
 
+
 class TestProduct(TestCaseWithFactory):
     """Tests product object."""
 
@@ -178,6 +180,32 @@
                          'new')
 
 
+class ProductSnapshotTestCase(TestCaseWithFactory):
+    """A TestCase for product snapshots."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(ProductSnapshotTestCase, self).setUp()
+        self.product = self.factory.makeProduct(name="shamwow")
+
+    def test_snapshot(self):
+        """Snapshots of products should not include marked attribues.
+
+        Wrap an export with 'doNotSnapshot' to force the snapshot to not
+        include that attribute.
+        """
+        snapshot = Snapshot(self.product, providing=IProduct)
+        omitted = [
+            'series',
+            'releases',
+            ]
+        for attribute in omitted:
+            self.assertFalse(
+                hasattr(snapshot, attribute),
+                "Snapshot should not include %s." % attribute)
+
+
 class BugSupervisorTestCase(TestCaseWithFactory):
     """A TestCase for bug supervisor management."""