launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/initseries-no-arch-bug-795396 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #795396 in Launchpad itself: "it's possible to choose as parents distros that shouldn't be there"
  https://bugs.launchpad.net/launchpad/+bug/795396

For more details, see:
https://code.launchpad.net/~rvb/launchpad/initseries-no-arch-bug-795396/+merge/64843

This branch fixes the DistroSeriesDerivationVocabulary (Vocabulary used by the picker of potential parents the +initseries page) so that only the series with architectures configured inside LP are returned.

= Note =

I had to relax the number of queries to fetch the parents from 1 to 3 because the clever trick that was used to fetch the parents all in one query cannot be applied anymore ... I think.

= Tests =

./bin/test -cvv test_distroseries_vocabularies test_distribution_with_derived_series_no_arch

= Q/A =

On DF: 
- Go to https://dogfood.launchpad.net/explorer/theseries/+initseries;
- In th parent picker search for 'Deb';
- Debian testing should not be there.
-- 
https://code.launchpad.net/~rvb/launchpad/initseries-no-arch-bug-795396/+merge/64843
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/initseries-no-arch-bug-795396 into lp:launchpad.

=== modified file 'lib/lp/registry/tests/test_distroseries_vocabularies.py'
--- lib/lp/registry/tests/test_distroseries_vocabularies.py	2011-05-31 16:12:15 +0000
+++ lib/lp/registry/tests/test_distroseries_vocabularies.py	2011-06-16 14:22:37 +0000
@@ -36,6 +36,9 @@
     def setUp(self):
         super(TestDistroSeriesDerivationVocabulary, self).setUp()
         self.all_distroseries = getUtility(IDistroSeriesSet).search()
+        self.all_series_with_arch = [
+            series for series in self.all_distroseries
+            if series.architecturecount != 0]
 
     def test_registration(self):
         # DistroSeriesDerivationVocabulary is registered as a named
@@ -58,34 +61,57 @@
         distroseries = self.factory.makeDistroSeries()
         vocabulary = DistroSeriesDerivationVocabulary(distroseries)
         expected_distroseries = (
-            set(self.all_distroseries).difference(
+            set(self.all_series_with_arch).difference(
                 distroseries.distribution.series))
         observed_distroseries = set(term.value for term in vocabulary)
         self.assertEqual(expected_distroseries, observed_distroseries)
 
+    def makeDistroSeriesWithDistroArch(self, *args, **kwargs):
+        # Helper method to create a distroseries with a
+        # DistroArchSeries.
+        distroseries = self.factory.makeDistroSeries(*args, **kwargs)
+        self.factory.makeDistroArchSeries(distroseries=distroseries)
+        return distroseries
+
     def test_distribution_with_derived_series(self):
         # Given a distribution with series, one or more of which are derived,
         # the vocabulary factory returns a vocabulary for all distroseries of
-        # the distribution from which the derived series have been derived.
-        parent_distroseries = self.factory.makeDistroSeries()
-        self.factory.makeDistroSeries(
+        # the distribution from which the derived series have been
+        # derived with distroarchseries setup in LP.
+        parent_distroseries = self.makeDistroSeriesWithDistroArch()
+        self.makeDistroSeriesWithDistroArch()
+        distroseries = self.factory.makeDistroSeries()
+        self.factory.makeDistroSeriesParent(
+            derived_series=distroseries, parent_series=parent_distroseries)
+        vocabulary = DistroSeriesDerivationVocabulary(distroseries)
+        expected_distroseries = set(parent_distroseries.distribution.series)
+        observed_distroseries = set(term.value for term in vocabulary)
+        self.assertContentEqual(expected_distroseries, observed_distroseries)
+
+    def test_distribution_with_derived_series_no_arch(self):
+        # Only the parents with DistroArcheSeries configured in LP are
+        # returned.
+        parent_distroseries = self.makeDistroSeriesWithDistroArch()
+        another_parent_no_arch = self.factory.makeDistroSeries(
             distribution=parent_distroseries.distribution)
         distroseries = self.factory.makeDistroSeries()
         self.factory.makeDistroSeriesParent(
             derived_series=distroseries, parent_series=parent_distroseries)
         vocabulary = DistroSeriesDerivationVocabulary(distroseries)
         expected_distroseries = set(parent_distroseries.distribution.series)
+        expected_distroseries.remove(another_parent_no_arch)
         observed_distroseries = set(term.value for term in vocabulary)
-        self.assertEqual(expected_distroseries, observed_distroseries)
+
+        self.assertContentEqual(expected_distroseries, observed_distroseries)
 
     def test_distribution_with_derived_series_from_multiple_parents(self):
         # Given a distribution with series, one or more of which are derived
         # from multiple parents, the vocabulary factory returns a vocabulary
         # for all distroseries of the distribution*s* from which the derived
         # series have been derived.
-        parent_distroseries = self.factory.makeDistroSeries()
-        another_parent_distroseries = self.factory.makeDistroSeries()
-        self.factory.makeDistroSeries(
+        parent_distroseries = self.makeDistroSeriesWithDistroArch()
+        another_parent_distroseries = self.makeDistroSeriesWithDistroArch()
+        self.makeDistroSeriesWithDistroArch(
             distribution=parent_distroseries.distribution)
         distroseries = self.factory.makeDistroSeries()
         self.factory.makeDistroSeriesParent(
@@ -105,14 +131,14 @@
         # (which shouldn't happen), the vocabulary factory returns a
         # vocabulary for all distroseries in all distributions *except* the
         # given distribution.
-        parent_distroseries = self.factory.makeDistroSeries()
+        parent_distroseries = self.makeDistroSeriesWithDistroArch()
         distroseries = self.factory.makeDistroSeries(
             distribution=parent_distroseries.distribution)
         self.factory.makeDistroSeriesParent(
             derived_series=distroseries, parent_series=parent_distroseries)
         vocabulary = DistroSeriesDerivationVocabulary(distroseries)
         expected_distroseries = (
-            set(self.all_distroseries).difference(
+            set(self.all_series_with_arch).difference(
                 distroseries.distribution.series))
         observed_distroseries = set(term.value for term in vocabulary)
         self.assertEqual(expected_distroseries, observed_distroseries)
@@ -120,10 +146,10 @@
     def test_distroseries(self):
         # Given a distroseries, the vocabulary factory returns the vocabulary
         # the same as for its distribution.
-        distroseries = self.factory.makeDistroSeries()
+        distroseries = self.makeDistroSeriesWithDistroArch()
         vocabulary = DistroSeriesDerivationVocabulary(distroseries)
         expected_distroseries = (
-            set(self.all_distroseries).difference(
+            set(self.all_series_with_arch).difference(
                 distroseries.distribution.series))
         observed_distroseries = set(term.value for term in vocabulary)
         self.assertEqual(expected_distroseries, observed_distroseries)
@@ -136,23 +162,23 @@
         six_days_ago = now - timedelta(7)
 
         aaa = self.factory.makeDistribution(displayname="aaa")
-        aaa_series_older = self.factory.makeDistroSeries(
+        aaa_series_older = self.makeDistroSeriesWithDistroArch(
             name="aaa-series-older", distribution=aaa)
         removeSecurityProxy(aaa_series_older).date_created = six_days_ago
-        aaa_series_newer = self.factory.makeDistroSeries(
+        aaa_series_newer = self.makeDistroSeriesWithDistroArch(
             name="aaa-series-newer", distribution=aaa)
         removeSecurityProxy(aaa_series_newer).date_created = two_days_ago
 
         bbb = self.factory.makeDistribution(displayname="bbb")
-        bbb_series_older = self.factory.makeDistroSeries(
+        bbb_series_older = self.makeDistroSeriesWithDistroArch(
             name="bbb-series-older", distribution=bbb)
         removeSecurityProxy(bbb_series_older).date_created = six_days_ago
-        bbb_series_newer = self.factory.makeDistroSeries(
+        bbb_series_newer = self.makeDistroSeriesWithDistroArch(
             name="bbb-series-newer", distribution=bbb)
         removeSecurityProxy(bbb_series_newer).date_created = two_days_ago
 
         ccc = self.factory.makeDistribution(displayname="ccc")
-        ccc_series = self.factory.makeDistroSeries(distribution=ccc)
+        ccc_series = self.makeDistroSeriesWithDistroArch(distribution=ccc)
 
         vocabulary = DistroSeriesDerivationVocabulary(ccc_series)
         expected_distroseries = [
@@ -196,7 +222,7 @@
         # Reload distroseries and distribution; these will reasonably already
         # be loaded before using the vocabulary.
         distroseries.distribution
-        # Getting terms issues one query to find parent serieses.
+        # Getting terms issues 3 queries to find parent serieses.
         with StormStatementRecorder() as recorder:
             DistroSeriesDerivationVocabulary(distroseries).terms
-            self.assertThat(recorder, HasQueryCount(Equals(1)))
+            self.assertThat(recorder, HasQueryCount(Equals(3)))

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2011-06-14 06:06:33 +0000
+++ lib/lp/registry/vocabularies.py	2011-06-16 14:22:37 +0000
@@ -188,8 +188,9 @@
 from lp.services.features import getFeatureFlag
 from lp.services.propertycache import (
     cachedproperty,
-    get_property_cache
+    get_property_cache,
     )
+from lp.soyuz.model.distroarchseries import DistroArchSeries
 
 
 class BasePersonVocabulary:
@@ -1728,6 +1729,9 @@
     derived at a later date, but as soon as this happens, the above rule
     applies.
 
+    Also, a series must have architectures setup in LP to be a potential
+    parent.
+
     It is permissible for a distribution to have both derived and non-derived
     series at the same time.
     """
@@ -1809,7 +1813,8 @@
         """See `IHugeVocabulary`."""
         parent = ClassAlias(DistroSeries, "parent")
         child = ClassAlias(DistroSeries, "child")
-        where = []
+        # Select only the series with architectures setup in LP.
+        where = [DistroSeries.id == DistroArchSeries.distroseriesID]
         if query is not None:
             term = '%' + query.lower() + '%'
             search = Or(
@@ -1817,22 +1822,20 @@
                     DistroSeries.description.lower().like(term),
                     DistroSeries.summary.lower().like(term))
             where.append(search)
-        parent_distributions = Select(
+        parent_distributions = IStore(DistroSeries).find(
             parent.distributionID, And(
                 parent.distributionID != self.distribution.id,
                 child.distributionID == self.distribution.id,
                 child.id == DistroSeriesParent.derived_series_id,
                 parent.id == DistroSeriesParent.parent_series_id))
-        where.append(
-            DistroSeries.distributionID.is_in(parent_distributions))
-        terms = self.find_terms(where)
-        if len(terms) == 0:
-            where = []
-            if query is not None:
-                where.append(search)
-            where.append(DistroSeries.distribution != self.distribution)
-            terms = self.find_terms(where)
-        return terms
+        if not parent_distributions.is_empty():
+            where.append(
+                DistroSeries.distributionID.is_in(parent_distributions))
+            return self.find_terms(where)
+        else:
+            where.append(
+                DistroSeries.distribution != self.distribution)
+            return self.find_terms(where)
 
 
 class PillarVocabularyBase(NamedSQLObjectHugeVocabulary):