Merge lp:~jtv/launchpad/pre-827347 into lp:launchpad

[Jeroen T. Vermeulen]

[r=gmb][bug=827347] Sanitize default/None status arg for IDistroSeriesDifferenceSource.getForDistroSeries.

= Summary =

IDistroSeriesDifferenceSource.getForDistroSeries accepts a filter argument for status. It defaults to None. But surprisingly, it does not interpret None as "any status." Instead, None is read as DistroSeriesDifferenceStatus.NEEDS_ATTENTION. This causes no end of confusion, and also makes it harder to search for DSDs without regard for status. The weirdness was ossified into a test, but what documentation there is suggests that None is supposed to mean any status.

== Proposed fix ==

Make None mean "any status." No documentation changes needed. Some test cleanups though.

== Pre-implementation notes ==

DistroSeriesDifferencesVocabulary did not pass a status, but its documentation suggested that there would be no filtering by status. Gavin worked on this recently, and so was able to tell me that it was fooled by the unclear interface. The change in this branch should make it work as was always intended.

== Implementation details ==

The actual code change is tiny and appears at the top of the diff.

Only in packagecopyjob did I see a case where the status argument was left out but the caller clearly meant to get NEEDS_ATTENTION DSDs. There are a few tests elsewhere that do not pass status but are just as valid with "any status" as they are with the previous default. I kept those unchanged.

I also rewrote a whole bunch of getForDistroSeries tests to get rid of the complex test data that enabled or compounded this situation.

I'm still running tests to find out whether I missed anything. Since DSDs are a new feature, I'll count on the test suite to expose any problems with the change.

== Tests ==

{{{
./bin/test -vvc lp.registry.tests.test_distroseriesdifference
./bin/test -vvc lp.registry -t DistroSeriesDifferencesVocabulary
./bin/test -vvc lp.registry.browser.tests -t distroseries
}}}

== Demo and Q/A ==

The DistroSeries:+localpackagediffs page is a good place to exercise the effects on the DSD views. Go through the various statuses available in the search radio buttons, searching for each, and see that they all still select the appropriate differences.

Also verify the links from the page for a derived distroseries to the missing / changed / unique packages. Their counts may be affected. I'll be changing these links in my next branch for the same bug, since we want them to behave differently.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/tests/test_distroseriesdifference.py
  lib/lp/soyuz/model/packagecopyjob.py
  lib/lp/registry/model/distroseriesdifference.py

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-08-15 14:53:26 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-08-16 15:51:26 +0000
@@ -443,10 +443,8 @@
                            child_version_higher=False, parent_series=None,
                            packagesets=None, changed_by=None):
         """See `IDistroSeriesDifferenceSource`."""
-        if status is None:
-            status = (DistroSeriesDifferenceStatus.NEEDS_ATTENTION,)
-        elif isinstance(status, DBItem):
-            status = (status, )
+        if isinstance(status, DBItem):
+            status = (status,)
         if IPerson.providedBy(changed_by):
             changed_by = (changed_by,)
 
@@ -461,10 +459,11 @@
         conditions = [
             DSD.derived_series == distro_series,
             DSD.source_package_name == SPN.id,  # For ordering.
-            DSD.status.is_in(status),
             ]
         if difference_type is not None:
             conditions.append(DSD.difference_type == difference_type)
+        if status is not None:
+            conditions.append(DSD.status.is_in(tuple(status)))
 
         if child_version_higher:
             conditions.append(DSD.source_version > DSD.parent_source_version)

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-08-16 08:55:31 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-08-16 15:51:26 +0000
@@ -955,45 +955,22 @@
 
     layer = DatabaseFunctionalLayer
 
-    def test_implements_interface(self):
-        # The implementation implements the interface correctly.
-        dsd_source = getUtility(IDistroSeriesDifferenceSource)
-
-        verifyObject(IDistroSeriesDifferenceSource, dsd_source)
-
-    def makeDiffsForDistroSeries(self, derived_series, parent_series=None):
-        # Helper that creates a range of differences for a derived
-        # series.
-        diffs = {
-            'normal': [],
-            'unique': [],
-            'missing': [],
-            'ignored': [],
-            }
-        diffs['normal'].append(
-            self.factory.makeDistroSeriesDifference(
-                derived_series=derived_series, parent_series=parent_series))
-        diffs['unique'].append(
-            self.factory.makeDistroSeriesDifference(
-                derived_series=derived_series,
-                parent_series=parent_series,
-                difference_type=(
-                    DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES)))
-        diffs['missing'].append(
-            self.factory.makeDistroSeriesDifference(
-                derived_series=derived_series,
-                parent_series=parent_series,
-                difference_type=(
-                    DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES)))
-        diffs['ignored'].append(
-            self.factory.makeDistroSeriesDifference(
-                derived_series=derived_series,
-                parent_series=parent_series,
-                status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT))
-        return diffs
+    def makeDifferencesForAllDifferenceTypes(self, derived_series):
+        """Create DSDs of all types for `derived_series`."""
+        return dict(
+            (diff_type, self.factory.makeDistroSeriesDifference(
+                derived_series, difference_type=diff_type))
+            for diff_type in DistroSeriesDifferenceType.items)
+
+    def makeDifferencesForAllStatuses(self, derived_series):
+        """Create DSDs of all statuses for `derived_series`."""
+        return dict(
+            (status, self.factory.makeDistroSeriesDifference(
+                derived_series, status=status))
+            for status in DistroSeriesDifferenceStatus.items)
 
     def makeDerivedSeries(self, derived_series=None):
-        # Keep tests DRY.
+        """Create a derived `DistroSeries`."""
         dsp = self.factory.makeDistroSeriesParent(
             derived_series=derived_series)
         return dsp.derived_series
@@ -1026,82 +1003,78 @@
             derived_series=derived_series, versions=versions, status=status,
             set_base_version=True)
 
+    def test_implements_interface(self):
+        self.assertProvides(
+            getUtility(IDistroSeriesDifferenceSource),
+            IDistroSeriesDifferenceSource),
+
     def test_getForDistroSeries_default(self):
-        # By default all differences needing attention for the given
-        # series are returned.
-        derived_series = self.makeDerivedSeries()
-        diffs = self.makeDiffsForDistroSeries(derived_series)
-
-        result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
-            derived_series)
-
-        self.assertContentEqual(
-            diffs['normal'] + diffs['unique'] + diffs['missing'], result)
+        # By default all differences for the given series are returned.
+        series = self.makeDerivedSeries()
+        dsd = self.factory.makeDistroSeriesDifference(series)
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual([dsd], dsd_source.getForDistroSeries(series))
 
     def test_getForDistroSeries_filters_by_distroseries(self):
         # Differences for other series are not included.
-        derived_series = self.makeDerivedSeries()
-        self.makeDiffsForDistroSeries(derived_series)
-        diff_for_other_series = self.factory.makeDistroSeriesDifference()
-
+        self.factory.makeDistroSeriesDifference()
+        other_series = self.makeDerivedSeries()
         dsd_source = getUtility(IDistroSeriesDifferenceSource)
-        results = set(dsd_source.getForDistroSeries(derived_series))
-
-        self.assertFalse(diff_for_other_series in results)
+        self.assertContentEqual(
+            [], dsd_source.getForDistroSeries(other_series))
 
     def test_getForDistroSeries_does_not_filter_dsd_type_by_default(self):
         # If no difference_type is given, getForDistroSeries returns
         # DSDs of all types (missing in derived series, different
         # versions, or unique to derived series).
-        derived_series = self.makeDerivedSeries()
-        dsds = [
-            self.factory.makeDistroSeriesDifference(
-                derived_series, difference_type=diff_type)
-            for diff_type in DistroSeriesDifferenceType.items]
+        series = self.makeDerivedSeries()
+        dsds = self.makeDifferencesForAllDifferenceTypes(series)
         dsd_source = getUtility(IDistroSeriesDifferenceSource)
         self.assertContentEqual(
-            dsds, dsd_source.getForDistroSeries(derived_series))
+            dsds.values(), dsd_source.getForDistroSeries(series))
 
     def test_getForDistroSeries_filters_by_type(self):
         # Only differences for the specified types are returned.
-        derived_series = self.makeDerivedSeries()
-        diffs = self.makeDiffsForDistroSeries(derived_series)
-
-        result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
-            derived_series,
-            DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES)
-
-        self.assertContentEqual(diffs['unique'], result)
+        series = self.makeDerivedSeries()
+        dsds = self.makeDifferencesForAllDifferenceTypes(series)
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        wanted_type = DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES
+        self.assertContentEqual(
+            [dsds[wanted_type]],
+            dsd_source.getForDistroSeries(
+                series, difference_type=wanted_type))
+
+    def test_getForDistroSeries_includes_all_statuses_by_default(self):
+        # If no status is given, getForDistroSeries returns DSDs of all
+        # statuses.
+        series = self.makeDerivedSeries()
+        dsds = self.makeDifferencesForAllStatuses(series)
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            dsds.values(), dsd_source.getForDistroSeries(series))
 
     def test_getForDistroSeries_filters_by_status(self):
         # A single status can be used to filter results.
-        derived_series = self.makeDerivedSeries()
-        diffs = self.makeDiffsForDistroSeries(derived_series)
-
-        result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
-            derived_series,
-            status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
-
-        self.assertContentEqual(diffs['ignored'], result)
+        series = self.makeDerivedSeries()
+        dsds = self.makeDifferencesForAllStatuses(series)
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        wanted_status = DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT
+        self.assertContentEqual(
+            [dsds[wanted_status]],
+            dsd_source.getForDistroSeries(series, status=wanted_status))
 
     def test_getForDistroSeries_filters_by_multiple_statuses(self):
         # Multiple statuses can be passed for filtering.
-        derived_series = self.makeDerivedSeries()
-        for status in DistroSeriesDifferenceStatus.items:
-            self.factory.makeDistroSeriesDifference(
-                derived_series, status=status)
-
-        statuses = (
+        series = self.makeDerivedSeries()
+        dsds = self.makeDifferencesForAllStatuses(series)
+        wanted_statuses = (
             DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
             DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
             )
-
         dsd_source = getUtility(IDistroSeriesDifferenceSource)
         self.assertContentEqual(
-            statuses, [
-                dsd.status
-                for dsd in dsd_source.getForDistroSeries(
-                    derived_series, status=statuses)])
+            [dsds[status] for status in wanted_statuses],
+            dsd_source.getForDistroSeries(series, status=wanted_statuses))
 
     def test_getForDistroSeries_matches_by_package_name(self):
         dsd = self.factory.makeDistroSeriesDifference()
@@ -1146,15 +1119,14 @@
 
     def test_getForDistroSeries_sorted_by_package_name(self):
         # The differences are sorted by package name.
-        derived_series = self.makeDerivedSeries()
-        names = []
-        for i in range(10):
-            diff = self.factory.makeDistroSeriesDifference(
-                derived_series=derived_series)
-            names.append(diff.source_package_name.name)
+        series = self.makeDerivedSeries()
+        names = [
+            self.factory.makeDistroSeriesDifference(
+                series).source_package_name.name
+            for counter in xrange(10)]
 
         results = getUtility(
-            IDistroSeriesDifferenceSource).getForDistroSeries(derived_series)
+            IDistroSeriesDifferenceSource).getForDistroSeries(series)
 
         self.assertContentEqual(
             sorted(names),

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-07-28 14:26:49 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-08-16 15:51:26 +0000
@@ -8,8 +8,9 @@
     "PlainPackageCopyJob",
     ]
 
+import logging
+
 from lazr.delegates import delegates
-import logging
 import simplejson
 from storm.locals import (
     And,
@@ -36,6 +37,7 @@
     )
 from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.registry.enum import DistroSeriesDifferenceStatus
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifferenceSource,
     )
@@ -528,7 +530,8 @@
         dsd_source = getUtility(IDistroSeriesDifferenceSource)
         target_series = self.target_distroseries
         candidates = dsd_source.getForDistroSeries(
-            distro_series=target_series, name_filter=self.package_name)
+            distro_series=target_series, name_filter=self.package_name,
+            status=DistroSeriesDifferenceStatus.NEEDS_ATTENTION)
 
         # The job doesn't know what distroseries a given package is
         # coming from, and the version number in the DSD may have