Merge lp:~allenap/launchpad/selected-differences-vocab-bug-817408 into lp:launchpad

[Gavin Panella]

[r=benji][bug=817408] Use a new DistroSeriesDifferences vocabulary in place of the hand-crafted one in IDifferencesFormSchema.

This adds a new vocabulary called "DistroSeriesDifferences" which represents all DistroSeriesDifference records for a given derived series. This is then used in place of the hand-crafted vocabulary in IDifferencesFormSchema.

[Benji York (benji)]

This looks good. Definitely an improvement.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-07-29 20:05:40 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-08-01 16:44:42 +0000
@@ -792,7 +792,7 @@
 
     selected_differences = List(
         title=_('Selected differences'),
-        value_type=Choice(vocabulary=SimpleVocabulary([])),
+        value_type=Choice(vocabulary="DistroSeriesDifferences"),
         description=_("Select the differences for syncing."),
         required=True)
 
@@ -859,15 +859,6 @@
         self.form_fields = (
             self.setupPackageFilterRadio() +
             self.form_fields)
-        check_permission('launchpad.Edit', self.context)
-        terms = [
-            SimpleTerm(diff, diff.id)
-                    for diff in self.cached_differences.batch]
-        diffs_vocabulary = SimpleVocabulary(terms)
-        # XXX: GavinPanella 2011-07-13 bug=809985: Good thing the app servers
-        # are running single threaded...
-        choice = self.form_fields['selected_differences'].field.value_type
-        choice.vocabulary = diffs_vocabulary
 
     def _sync_sources(self, action, data):
         """Synchronise packages from the parent series to this one."""

=== modified file 'lib/lp/registry/tests/test_distroseries_vocabularies.py'
--- lib/lp/registry/tests/test_distroseries_vocabularies.py	2011-07-08 14:09:11 +0000
+++ lib/lp/registry/tests/test_distroseries_vocabularies.py	2011-08-01 16:44:42 +0000
@@ -11,21 +11,34 @@
     )
 
 from pytz import utc
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    Not,
+    )
 from zope.component import getUtility
-from zope.schema.interfaces import IVocabularyFactory
+from zope.schema.interfaces import (
+    ITokenizedTerm,
+    IVocabularyFactory,
+    )
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.database.sqlbase import flush_database_caches
 from canonical.launchpad.webapp.vocabulary import IHugeVocabulary
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
-from lp.registry.vocabularies import DistroSeriesDerivationVocabulary
+from lp.registry.vocabularies import (
+    DistroSeriesDerivationVocabulary,
+    DistroSeriesDifferencesVocabulary,
+    )
 from lp.testing import (
     StormStatementRecorder,
     TestCaseWithFactory,
     )
-from lp.testing.matchers import HasQueryCount
+from lp.testing.matchers import (
+    Contains,
+    HasQueryCount,
+    Provides,
+    )
 
 
 class TestDistroSeriesDerivationVocabulary(TestCaseWithFactory):
@@ -47,9 +60,8 @@
             getUtility(IVocabularyFactory, name="DistroSeriesDerivation"),
             DistroSeriesDerivationVocabulary)
 
-    def test_interfaces(self):
-        # DistroSeriesDerivationVocabulary instances provide
-        # IVocabulary and IVocabularyTokenized.
+    def test_interface(self):
+        # DistroSeriesDerivationVocabulary instances provide IHugeVocabulary.
         distroseries = self.factory.makeDistroSeries()
         vocabulary = DistroSeriesDerivationVocabulary(distroseries)
         self.assertProvides(vocabulary, IHugeVocabulary)
@@ -248,3 +260,122 @@
         self.assertContentEqual(
             expected_distroseries,
             observed_distroseries)
+
+
+class TestDistroSeriesDifferencesVocabulary(TestCaseWithFactory):
+    """Tests for `DistroSeriesDifferencesVocabulary`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_registration(self):
+        # DistroSeriesDifferencesVocabulary is registered as a named utility
+        # for IVocabularyFactory.
+        self.assertEqual(
+            getUtility(IVocabularyFactory, name="DistroSeriesDifferences"),
+            DistroSeriesDifferencesVocabulary)
+
+    def test_interface(self):
+        # DistroSeriesDifferencesVocabulary instances provide IHugeVocabulary.
+        distroseries = self.factory.makeDistroSeries()
+        vocabulary = DistroSeriesDifferencesVocabulary(distroseries)
+        self.assertProvides(vocabulary, IHugeVocabulary)
+
+    def test_non_derived_distroseries(self):
+        # The vocabulary is empty for a non-derived series.
+        distroseries = self.factory.makeDistroSeries()
+        vocabulary = DistroSeriesDifferencesVocabulary(distroseries)
+        self.assertContentEqual([], vocabulary)
+
+    def test_derived_distroseries(self):
+        # The vocabulary contains all DSDs for a derived series.
+        distroseries = self.factory.makeDistroSeries()
+        dsds = [
+            self.factory.makeDistroSeriesDifference(
+                derived_series=distroseries),
+            self.factory.makeDistroSeriesDifference(
+                derived_series=distroseries),
+            ]
+        vocabulary = DistroSeriesDifferencesVocabulary(distroseries)
+        self.assertContentEqual(
+            dsds, (term.value for term in vocabulary))
+
+    def test_derived_distroseries_not_other_distroseries(self):
+        # The vocabulary contains all DSDs for a derived series and not for
+        # another series.
+        distroseries1 = self.factory.makeDistroSeries()
+        distroseries2 = self.factory.makeDistroSeries()
+        dsds = [
+            self.factory.makeDistroSeriesDifference(
+                derived_series=distroseries1),
+            self.factory.makeDistroSeriesDifference(
+                derived_series=distroseries1),
+            self.factory.makeDistroSeriesDifference(
+                derived_series=distroseries2),
+            self.factory.makeDistroSeriesDifference(
+                derived_series=distroseries2),
+            ]
+        vocabulary = DistroSeriesDifferencesVocabulary(distroseries1)
+        self.assertContentEqual(
+            (dsd for dsd in dsds if dsd.derived_series == distroseries1),
+            (term.value for term in vocabulary))
+
+    def test_contains_difference(self):
+        # The vocabulary can be tested for membership.
+        difference = self.factory.makeDistroSeriesDifference()
+        vocabulary = DistroSeriesDifferencesVocabulary(
+            difference.derived_series)
+        self.assertThat(vocabulary, Contains(difference))
+
+    def test_does_not_contain_difference(self):
+        # The vocabulary can be tested for non-membership.
+        difference = self.factory.makeDistroSeriesDifference()
+        vocabulary = DistroSeriesDifferencesVocabulary(
+            self.factory.makeDistroSeries())
+        self.assertThat(vocabulary, Not(Contains(difference)))
+
+    def test_does_not_contain_something_else(self):
+        # The vocabulary can be tested for non-membership of something that's
+        # not a DistroSeriesDifference.
+        distroseries = self.factory.makeDistroSeries()
+        vocabulary = DistroSeriesDifferencesVocabulary(distroseries)
+        self.assertThat(vocabulary, Not(Contains("foobar")))
+
+    def test_size(self):
+        # The vocabulary can report its size.
+        difference = self.factory.makeDistroSeriesDifference()
+        vocabulary = DistroSeriesDifferencesVocabulary(
+            difference.derived_series)
+        self.assertEqual(1, len(vocabulary))
+
+    def test_getTerm(self):
+        # A term can be obtained from a given value.
+        difference = self.factory.makeDistroSeriesDifference()
+        vocabulary = DistroSeriesDifferencesVocabulary(
+            difference.derived_series)
+        term = vocabulary.getTerm(difference)
+        self.assertThat(term, Provides(ITokenizedTerm))
+        self.assertEqual(difference, term.value)
+        self.assertEqual(str(difference.id), term.token)
+
+    def test_getTermByToken(self):
+        # A term can be obtained from a given token.
+        difference = self.factory.makeDistroSeriesDifference()
+        vocabulary = DistroSeriesDifferencesVocabulary(
+            difference.derived_series)
+        term = vocabulary.getTermByToken(str(difference.id))
+        self.assertEqual(difference, term.value)
+
+    def test_getTermByToken_not_found(self):
+        # LookupError is raised when the token cannot be found.
+        distroseries = self.factory.makeDistroSeries()
+        difference = self.factory.makeDistroSeriesDifference()
+        vocabulary = DistroSeriesDifferencesVocabulary(distroseries)
+        self.assertRaises(
+            LookupError, vocabulary.getTermByToken, str(difference.id))
+
+    def test_getTermByToken_invalid(self):
+        # LookupError is raised when the token is not valid (i.e. a string
+        # containing only digits).
+        distroseries = self.factory.makeDistroSeries()
+        vocabulary = DistroSeriesDifferencesVocabulary(distroseries)
+        self.assertRaises(LookupError, vocabulary.getTermByToken, "foobar")

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2011-07-26 19:09:48 +0000
+++ lib/lp/registry/vocabularies.py	2011-08-01 16:44:42 +0000
@@ -33,6 +33,7 @@
     'DistributionSourcePackageVocabulary',
     'DistributionVocabulary',
     'DistroSeriesDerivationVocabulary',
+    'DistroSeriesDifferencesVocabulary',
     'DistroSeriesVocabulary',
     'FeaturedProjectVocabulary',
     'FilteredDistroSeriesVocabulary',
@@ -138,6 +139,9 @@
     IDistributionSourcePackage,
     )
 from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.distroseriesdifference import (
+    IDistroSeriesDifference,
+    )
 from lp.registry.interfaces.mailinglist import (
     IMailingListSet,
     MailingListStatus,
@@ -167,6 +171,7 @@
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
+from lp.registry.model.distroseriesdifference import DistroSeriesDifference
 from lp.registry.model.distroseriesparent import DistroSeriesParent
 from lp.registry.model.featuredproject import FeaturedProject
 from lp.registry.model.karma import KarmaCategory
@@ -194,9 +199,7 @@
 from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
 from lp.soyuz.model.distroarchseries import DistroArchSeries
-from lp.soyuz.model.publishing import (
-    SourcePackagePublishingHistory,
-    )
+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
 
@@ -1866,6 +1869,77 @@
             return self.find_terms(where)
 
 
+class DistroSeriesDifferencesVocabulary:
+    """A vocabulary source for differences relating to a series.
+
+    Specifically, all `DistroSeriesDifference`s relating to a derived series.
+    """
+
+    implements(IHugeVocabulary)
+
+    displayname = "Choose a difference"
+    step_title = 'Search'
+
+    def __init__(self, context):
+        """Create a new vocabulary for the context.
+
+        :type context: `IDistroSeries`.
+        """
+        assert IDistroSeries.providedBy(context)
+        self.distroseries = context
+
+    def __len__(self):
+        """See `IIterableVocabulary`."""
+        return self.searchForDifferences().count()
+
+    def __iter__(self):
+        """See `IIterableVocabulary`."""
+        for difference in self.searchForDifferences():
+            yield self.toTerm(difference)
+
+    def __contains__(self, value):
+        """See `IVocabulary`."""
+        return (
+            IDistroSeriesDifference.providedBy(value) and
+            value.derived_series == self.distroseries)
+
+    def getTerm(self, value):
+        """See `IVocabulary`."""
+        if value not in self:
+            raise LookupError(value)
+        return self.toTerm(value)
+
+    def getTermByToken(self, token):
+        """See `IVocabularyTokenized`."""
+        if not token.isdigit():
+            raise LookupError(token)
+        difference = IStore(DistroSeriesDifference).get(
+            DistroSeriesDifference, int(token))
+        if difference is None:
+            raise LookupError(token)
+        elif difference.derived_series != self.distroseries:
+            raise LookupError(token)
+        else:
+            return self.toTerm(difference)
+
+    @staticmethod
+    def toTerm(dsd):
+        """Return the term for a `DistroSeriesDifference`."""
+        return SimpleTerm(dsd, dsd.id)
+
+    def searchForTerms(self, query=None):
+        """See `IHugeVocabulary`."""
+        results = self.searchForDifferences()
+        return CountableIterator(results.count(), results, self.toTerm)
+
+    def searchForDifferences(self):
+        """The set of `DistroSeriesDifference`s related to the context.
+
+        :return: `IResultSet` yielding `IDistroSeriesDifference`.
+        """
+        return DistroSeriesDifference.getForDistroSeries(self.distroseries)
+
+
 class PillarVocabularyBase(NamedSQLObjectHugeVocabulary):
     """Active `IPillar` objects vocabulary."""
     displayname = 'Needs to be overridden'

=== modified file 'lib/lp/registry/vocabularies.zcml'
--- lib/lp/registry/vocabularies.zcml	2011-07-13 01:49:53 +0000
+++ lib/lp/registry/vocabularies.zcml	2011-08-01 16:44:42 +0000
@@ -42,6 +42,18 @@
 
 
   <securedutility
+      name="DistroSeriesDifferences"
+      component=".vocabularies.DistroSeriesDifferencesVocabulary"
+      provides="zope.schema.interfaces.IVocabularyFactory">
+    <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+  </securedutility>
+
+  <class class=".vocabularies.DistroSeriesDifferencesVocabulary">
+    <allow interface="canonical.launchpad.webapp.vocabulary.IHugeVocabulary"/>
+  </class>
+
+
+  <securedutility
     name="FeaturedProject"
     component="lp.registry.vocabularies.FeaturedProjectVocabulary"
     provides="zope.schema.interfaces.IVocabularyFactory"