Merge lp:~rvb/launchpad/dsd-api-bug-766158 into lp:launchpad/db-devel

[Raphaël Badin]

[r=danilo][bug=766158] Add an method IDistroSeries.getDifferencesTo to retrieve differences to a specific parent or all parents of the series. Expose this method on the 'devel' api.

This branch adds a method IDistroSeries.getDifferencesTo to find DistroSeriesDifferences for a DistroSeries (this method is exposed on the 'devel' api).

= Details =
This branch is branched off lp:~stevenk/launchpad/db-use-dsp which removes the usage of IDistroSeries.parent_series in DSDs and DSDJs.
The method getDifferencesTo is a simple wrapper around IDistroSeriesDifferenceSource.getForDistroSeries. This branch fixes IDSDS.getForDistroSeries and the testing factory methods to work in the context of multiple parents.

= Tests =
(added test)
./bin/test -cvv test_distroseriesdifference test_getForDistroSeries_filters_by_parent
./bin/test -cvv test_distroseries_webservice test_getDifferencesTo
(fixed test)
./bin/test -cvv test_distroseriesdifference test_getForDistroSeries_filters_by_type

= QA =
Once lp:~stevenk/launchpad/db-use-dsp is merged and qa-ed, this can be tested by calling the api on a distroseries with multiple parents to fetch differences.

= Drive-by fixes =
- Add missing assert statement in test_getForDistroSeries_filters_by_type (lib/lp/registry/tests/test_distroseriesdifference.py).
- his branch also fixes a small glitch in lib/lp/registry/interfaces/distribution.py.

[Данило Шеган (danilo)]

Several issues as discussed on IRC:

1. There is now a easy-to-fix conflict :)

2. operation_parameters seem to abuse TextLine quite a bit: please use appropriate interface definitions (Choices, Bools, etc) and make sure appropriate enums are exported as well

3. A tiny whitespace issue:

At line 89 of the diff, you've got double space inside a string (at EOL and SOL :):

+ title=_("Only return differences for packages matching this "
+ " name."),

review: Needs Fixing

[Данило Шеган (danilo)]

Oh, also (lest it be forgotten :):

 - assertSameDiffs could use assertContentEqual to avoid sorted() calls, and "ds_diff_ws" should instead be named "derived_ds_ws" or something

[Raphaël Badin (rvb)]

> Oh, also (lest it be forgotten :):
>
> - assertSameDiffs could use assertContentEqual to avoid sorted() calls, and
> "ds_diff_ws" should instead be named "derived_ds_ws" or something

2, 3, 4 done.

I also refactored my test to pass actual parameters to the api method. (I'd like to have your opinion about passing str(Enum) to the api method though).

I still need to merge db-devel to fix the conflict but I guess it's going to be easier to review my changes *before* I merge db-devel and thus diverge from Steven's branch.

[Данило Шеган (danilo)]

Looks good. It seems str(Enum) is the right way to pass the enum value around, though it'd be best to check with someone else more knowledgeable as well (i.e. I looked at https://launchpad.net/+apidoc/devel.html for a few enums and they basically expect a string like that).

(fwiw, merging db-devel should not have introduced any other changes into your diff because it is proposed against db-devel, but would introduce bigger differences between Steve's and your branch)

review: Approve

=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2011-04-12 13:43:36 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2011-05-02 14:04:48 +0000
@@ -112,6 +112,10 @@
     IHWSubmissionDevice,
     IHWVendorID,
     )
+from lp.registry.enum import (
+    DistroSeriesDifferenceStatus,
+    DistroSeriesDifferenceType,
+    )
 from lp.registry.interfaces.commercialsubscription import (
     ICommercialSubscription,
     )
@@ -486,6 +490,16 @@
     IDistroSeries, 'deriveDistroSeries', 'distribution', IDistribution)
 patch_collection_return_type(
     IDistroSeries, 'getDerivedSeries', IDistroSeries)
+patch_plain_parameter_type(
+    IDistroSeries, 'getDifferencesTo', 'parent_series', IDistroSeries)
+patch_choice_parameter_type(
+    IDistroSeries, 'getDifferencesTo', 'status', DistroSeriesDifferenceStatus)
+patch_choice_parameter_type(
+    IDistroSeries, 'getDifferencesTo', 'difference_type',
+    DistroSeriesDifferenceType)
+patch_collection_return_type(
+    IDistroSeries, 'getDifferencesTo', IDistroSeriesDifference)
+
 
 # IDistroSeriesDifference
 patch_reference_property(

=== added file 'lib/lp/registry/browser/tests/test_distroseries_webservice.py'
--- lib/lp/registry/browser/tests/test_distroseries_webservice.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_distroseries_webservice.py	2011-05-02 14:04:48 +0000
@@ -0,0 +1,44 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from canonical.testing import AppServerLayer
+from lp.registry.enum import (
+    DistroSeriesDifferenceStatus,
+    DistroSeriesDifferenceType,
+    )
+from lp.testing import (
+    TestCaseWithFactory,
+    ws_object,
+    )
+
+
+class DistroSeriesWebServiceTestCase(TestCaseWithFactory):
+
+    layer = AppServerLayer
+
+    def assertSameDiffs(self, diffs, ws_diffs):
+        self.assertContentEqual(
+            [self._wsFor(diff) for diff in diffs],
+            [ws_diff for ws_diff in ws_diffs])
+
+    def _wsFor(self, obj):
+        return ws_object(
+            self.factory.makeLaunchpadService(version="devel"), obj)
+
+    def test_getDifferencesTo(self):
+        # Distroseries' DistroSeriesDifferences are available
+        # on the web service.
+        # This method is a simple wrapper around getForDistroSeries
+        # that is thoroughly tested in test_distroseriesdifference.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_ws = self._wsFor(ds_diff.derived_series)
+
+        self.assertSameDiffs([ds_diff], ds_ws.getDifferencesTo(
+            status=str(DistroSeriesDifferenceStatus.NEEDS_ATTENTION),
+            difference_type=str(
+                DistroSeriesDifferenceType.DIFFERENT_VERSIONS),
+            source_package_name_filter=ds_diff.source_package_name.name,
+            child_version_higher=False,
+            parent_series=self._wsFor(ds_diff.parent_series)))

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2011-04-14 20:40:03 +0000
+++ lib/lp/registry/interfaces/distribution.py	2011-05-02 14:04:48 +0000
@@ -156,7 +156,7 @@
         Summary(
             title=_("Summary"),
             description=_(
-                "A short paragraph to introduce the the goals and highlights "
+                "A short paragraph to introduce the goals and highlights "
                 "of the distribution."),
             required=True))
     homepage_content = exported(

=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2011-05-02 14:04:47 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2011-05-02 14:04:48 +0000
@@ -24,6 +24,7 @@
     export_write_operation,
     exported,
     LAZR_WEBSERVICE_EXPORTED,
+    operation_for_version,
     operation_parameters,
     operation_returns_collection_of,
     operation_returns_entry,
@@ -893,6 +894,46 @@
     def getDerivedSeries():
         """Get all `DistroSeries` derived from this one."""
 
+    @operation_parameters(
+        parent_series=Reference(
+            schema=Interface, # IDistroSeries
+            title=_("The parent series to consider."),
+            required=False),
+        difference_type=Choice(
+            vocabulary=DBEnumeratedType, # DistroSeriesDifferenceType
+            title=_("Only return differences of this type."), required=False),
+        source_package_name_filter=TextLine(
+            title=_("Only return differences for packages matching this "
+                    "name."),
+            required=False),
+        status=Choice(
+            vocabulary=DBEnumeratedType, # DistroSeriesDifferenceStatus
+            title=_("Only return differences of this status."),
+            required=False),
+        child_version_higher=Bool(
+            title=_("Only return differences for which the child's version "
+                    "is higher than the parent's."),
+            required=False),
+        )
+    @operation_returns_collection_of(Interface)
+    @export_read_operation()
+    @operation_for_version('devel')
+    def getDifferencesTo(parent_series, difference_type,
+                         source_package_name_filter, status,
+                         child_version_higher):
+        """Return the differences between this series and the specified
+        parent_series (or all the parent series if parent_series is None).
+
+        :param parent_series: The parent series for which the differences
+            should be returned. All parents are considered if this is None.
+        :param difference_type: The type of the differences to return.
+        :param source_package_name_filter: A package name to use as a filter
+            for the differences.
+        :param status: The status of the differences to return.
+        :param child_version_higher: Only return differences for which the
+            child's version is higher than the parent's version.
+        """
+
 
 class IDistroSeries(IDistroSeriesEditRestricted, IDistroSeriesPublic,
                     IStructuralSubscriptionTarget):

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2011-05-02 14:04:47 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2011-05-02 14:04:48 +0000
@@ -280,10 +280,11 @@
 
     def getForDistroSeries(
         distro_series,
-        difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
+        difference_type=None,
         source_package_name_filter=None,
         status=None,
-        child_version_higher=False):
+        child_version_higher=False,
+        parent_series=None):
         """Return differences for the derived distro series sorted by
         package name.
 
@@ -301,7 +302,10 @@
         :param child_version_higher: Only differences for which the child's
             version is higher than the parent's version will be included.
         :type child_version_higher: bool.
-        :return: A result set of differences.
+        :param parent_series: The parent series to consider. Consider all
+            parent series if this parameter is None.
+        :type distro_series: `IDistroSeries`.
+        :return: A result set of `IDistroSeriesDifference`.
         """
 
     def getByDistroSeriesAndName(distro_series, source_package_name):

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-05-02 14:04:47 +0000
+++ lib/lp/registry/model/distroseries.py	2011-05-02 14:04:48 +0000
@@ -99,6 +99,9 @@
     IDistroSeries,
     IDistroSeriesSet,
     )
+from lp.registry.interfaces.distroseriesdifference import (
+    IDistroSeriesDifferenceSource,
+    )
 from lp.registry.interfaces.person import validate_public_person
 from lp.registry.interfaces.pocket import (
     PackagePublishingPocket,
@@ -2021,6 +2024,18 @@
                 return OrderedBugTask(3, bugtask.id, bugtask)
         return weight_function
 
+    def getDifferencesTo(self, parent_series=None, difference_type=None,
+                         source_package_name_filter=None, status=None,
+                         child_version_higher=False):
+        """See `IDistroSeries`."""
+        return getUtility(
+            IDistroSeriesDifferenceSource).getForDistroSeries(
+                self,
+                difference_type = difference_type,
+                source_package_name_filter=source_package_name_filter,
+                status=status,
+                child_version_higher=child_version_higher)
+
 
 class DistroSeriesSet:
     implements(IDistroSeriesSet)

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-05-02 14:04:47 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-05-02 14:04:48 +0000
@@ -258,11 +258,14 @@
     @staticmethod
     def getForDistroSeries(
         distro_series,
-        difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
+        difference_type=None,
         source_package_name_filter=None,
         status=None,
-        child_version_higher=False):
+        child_version_higher=False,
+        parent_series=None):
         """See `IDistroSeriesDifferenceSource`."""
+        if difference_type is None:
+            difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS
         if status is None:
             status = (
                 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
@@ -276,7 +279,11 @@
             DistroSeriesDifference.status.is_in(status),
             DistroSeriesDifference.source_package_name ==
                 SourcePackageName.id,
-         ]
+        ]
+
+        if parent_series:
+            conditions.extend([
+               DistroSeriesDifference.parent_series == parent_series.id])
 
         if source_package_name_filter:
             conditions.extend([

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-05-02 14:04:47 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-05-02 14:04:48 +0000
@@ -871,7 +871,7 @@
 
         verifyObject(IDistroSeriesDifferenceSource, dsd_source)
 
-    def makeDiffsForDistroSeries(self, derived_series):
+    def makeDiffsForDistroSeries(self, derived_series, parent_series=None):
         # Helper that creates a range of differences for a derived
         # series.
         diffs = {
@@ -881,21 +881,24 @@
             }
         diffs['normal'].append(
             self.factory.makeDistroSeriesDifference(
-                derived_series=derived_series))
+                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['ignored'].append(
             self.factory.makeDistroSeriesDifference(
                 derived_series=derived_series,
+                parent_series=parent_series,
                 status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT))
         return diffs
 
-    def makeDerivedSeries(self):
+    def makeDerivedSeries(self, derived_series=None):
         # Keep tests DRY.
-        dsp = self.factory.makeDistroSeriesParent()
+        dsp = self.factory.makeDistroSeriesParent(
+            derived_series=derived_series)
         return dsp.derived_series
 
     def test_getForDistroSeries_default(self):
@@ -930,6 +933,8 @@
             derived_series,
             DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES)
 
+        self.assertContentEqual(diffs['unique'], result)
+
     def test_getForDistroSeries_filters_by_status(self):
         # A single status can be used to filter results.
         derived_series = self.makeDerivedSeries()
@@ -971,6 +976,31 @@
             sorted(names),
             [result.source_package_name.name for result in results])
 
+    def test_getForDistroSeries_filters_by_parent(self):
+        # The differences can be filtered by parent series.
+        dsp = self.factory.makeDistroSeriesParent()
+        derived_series = dsp.derived_series
+        parent_series = dsp.parent_series
+
+        # Add another parent to this series.
+        parent_series2 = self.factory.makeDistroSeriesParent(
+            derived_series=derived_series).parent_series
+
+        diffs = self.makeDiffsForDistroSeries(
+            derived_series, parent_series=parent_series)
+        diffs2 = self.makeDiffsForDistroSeries(
+            derived_series, parent_series=parent_series2)
+
+        results = getUtility(
+            IDistroSeriesDifferenceSource).getForDistroSeries(
+                derived_series, parent_series=parent_series)
+        results2 = getUtility(
+            IDistroSeriesDifferenceSource).getForDistroSeries(
+                derived_series, parent_series=parent_series2)
+
+        self.assertContentEqual(diffs['normal'], results)
+        self.assertContentEqual(diffs2['normal'], results2)
+
     def test_getByDistroSeriesAndName(self):
         # An individual difference is obtained using the name.
         ds_diff = self.factory.makeDistroSeriesDifference(

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-05-02 14:04:47 +0000
+++ lib/lp/testing/factory.py	2011-05-02 14:04:48 +0000
@@ -2314,21 +2314,24 @@
         versions=None,
         difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
         status=DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
-        changelogs=None, set_base_version=False):
+        changelogs=None, set_base_version=False, parent_series=None):
         """Create a new distro series source package difference."""
         if derived_series is None:
-            dsp = self.makeDistroSeriesParent()
+            dsp = self.makeDistroSeriesParent(
+                parent_series=parent_series)
             derived_series = dsp.derived_series
             parent_series = dsp.parent_series
         else:
-            dsp = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
-                derived_series)
-            if dsp.count() == 0:
-                new_dsp = self.makeDistroSeriesParent(
-                    derived_series=derived_series)
-                parent_series = new_dsp.parent_series
-            else:
-                parent_series = dsp[0].parent_series
+            if parent_series is None:
+                dsp = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
+                    derived_series)
+                if dsp.count() == 0:
+                    new_dsp = self.makeDistroSeriesParent(
+                        derived_series=derived_series,
+                        parent_series=parent_series)
+                    parent_series = new_dsp.parent_series
+                else:
+                    parent_series = dsp[0].parent_series
 
         if source_package_name_str is None:
             source_package_name_str = self.getUniqueString('src-name')
@@ -2373,7 +2376,7 @@
                 status = PackagePublishingStatus.PUBLISHED)
 
         diff = getUtility(IDistroSeriesDifferenceSource).new(
-            derived_series, source_package_name)
+            derived_series, source_package_name, parent_series)
 
         removeSecurityProxy(diff).status = status