launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-806946 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #806946 in Launchpad itself: "Search by packageset on +localpackagediffs"
  https://bugs.launchpad.net/launchpad/+bug/806946

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-806946/+merge/67302

= Summary =

There's a package-name search box on +localpackagediffs.  When the name matches that of a packageset for the context distroseries, we want the page to show packages from that packageset.


== Proposed fix ==

This is a model change only.  The UI and the web-service API may also need updating once we know that this performs well enough.


== Pre-implementation notes ==

It was wisely decided to honour only exact matches on packageset names.  Ordering of results should not be affected.


== Implementation details ==

I did not widen the main search join, because it may affect ordering and uniqueness to a painful extent.  Instead there's a one-off sub-query that I hope the query optimizer will execute separately, and quickly.

There was a weird failure on the JavaScript test test_distroseries.js, when run from the command line with -t test_distroseries.  It does pass when run from the browser, or with -t javascript as a test selector.  Ian's currently landing JS test cleanup may fix that; I am inclined to believe that it's not related to my changes here.  An EC2 test is underway to verify this.


== Tests ==

{{{
./bin/test -vvc lp.registry.tests.test_distroseriesdifference
}}}


== Demo and Q/A ==

Go to https://dogfood.launchpad.net/ubuntu/oneiric/+localpackagediffs and try entering a packageset name, such as "core" into the package name filter.


= Launchpad lint =

There's one bit of pre-existing lint where somebody else's branch had a problem.  Rather than touching it myself, I notified the author and it's being fixed.

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/interfaces/distroseriesdifference.py
  lib/lp/soyuz/interfaces/packageset.py
  lib/lp/registry/model/distroseriesdifference.py
  lib/lp/registry/model/distroseries.py
  lib/lp/registry/browser/distroseries.py

./lib/lp/soyuz/model/packagecopyjob.py
     487: W291 trailing whitespace
-- 
https://code.launchpad.net/~jtv/launchpad/bug-806946/+merge/67302
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-806946 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-07-07 18:02:34 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-07-08 10:08:21 +0000
@@ -1054,7 +1054,7 @@
         differences = getUtility(
             IDistroSeriesDifferenceSource).getForDistroSeries(
                 self.context, difference_type=self.differences_type,
-                source_package_name_filter=self.specified_name_filter,
+                name_filter=self.specified_name_filter,
                 status=status, child_version_higher=child_version_higher)
         return BatchNavigator(differences, self.request)
 

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2011-07-01 15:53:38 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2011-07-08 10:08:21 +0000
@@ -294,7 +294,7 @@
     def getForDistroSeries(
         distro_series,
         difference_type=None,
-        source_package_name_filter=None,
+        name_filter=None,
         status=None,
         child_version_higher=False,
         parent_series=None,
@@ -308,9 +308,10 @@
         :param difference_type: The type of difference to include in the
             results.
         :type difference_type: `DistroSeriesDifferenceType`.
-        :param source_package_name_filter: Name of a source package.  If
-            given, restricts the search to this package.
-        :type source_package_name_filter: unicode.
+        :param name_filter: Name of either a source package or a package set
+            to look for.  If given, return only packages whose name matches
+            this string, or that are in a `Packageset` those name matches it.
+        :type name_filter: unicode.
         :param status: Only differences matching the status(es) will be
             included.
         :type status: `DistroSeriesDifferenceStatus`.

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-07-07 08:56:35 +0000
+++ lib/lp/registry/model/distroseries.py	2011-07-08 10:08:21 +0000
@@ -1871,7 +1871,7 @@
             IDistroSeriesDifferenceSource).getForDistroSeries(
                 self,
                 difference_type=difference_type,
-                source_package_name_filter=source_package_name_filter,
+                name_filter=source_package_name_filter,
                 status=status,
                 child_version_higher=child_version_higher)
 

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-07-01 15:53:38 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-07-08 10:08:21 +0000
@@ -25,6 +25,7 @@
     And,
     Column,
     Desc,
+    Or,
     Select,
     Table,
     )
@@ -88,7 +89,10 @@
     PackagePublishingStatus,
     )
 from lp.soyuz.interfaces.packagediff import IPackageDiffSet
-from lp.soyuz.interfaces.packageset import IPackagesetSet
+from lp.soyuz.interfaces.packageset import (
+    IPackagesetSet,
+    NoSuchPackageSet,
+    )
 from lp.soyuz.model.archive import Archive
 from lp.soyuz.model.distributionsourcepackagerelease import (
     DistributionSourcePackageRelease,
@@ -333,7 +337,7 @@
     def getForDistroSeries(
         distro_series,
         difference_type=None,
-        source_package_name_filter=None,
+        name_filter=None,
         status=None,
         child_version_higher=False,
         parent_series=None,
@@ -360,9 +364,20 @@
             conditions.extend([
                DistroSeriesDifference.parent_series == parent_series.id])
 
-        if source_package_name_filter:
-            conditions.extend([
-               SourcePackageName.name == source_package_name_filter])
+        if name_filter:
+            name_matches = [SourcePackageName.name == name_filter]
+            try:
+                packageset = getUtility(IPackagesetSet).getByName(
+                    name_filter, distroseries=distro_series)
+            except NoSuchPackageSet:
+                packageset = None
+            if packageset is not None:
+                name_matches.append(
+                    DistroSeriesDifference.source_package_name_id.is_in(
+                        Select(
+                            PackagesetSources.sourcepackagename_id,
+                            PackagesetSources.packageset == packageset)))
+            conditions.extend([Or(*name_matches)])
 
         if child_version_higher:
             conditions.extend([

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-07-01 15:53:38 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-07-08 10:08:21 +0000
@@ -1023,6 +1023,47 @@
 
         self.assertContentEqual(diffs['normal'] + diffs['ignored'], result)
 
+    def test_getForDistroSeries_matches_by_package_name(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        package_name = dsd.source_package_name.name
+        source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            [dsd],
+            source.getForDistroSeries(series, name_filter=package_name))
+
+    def test_getForDistroSeries_matches_by_packageset_name(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        packageset = self.factory.makePackageset(
+            distroseries=series, packages=[dsd.source_package_name])
+        packageset_name = packageset.name
+        source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            [dsd],
+            source.getForDistroSeries(series, name_filter=packageset_name))
+
+    def test_getForDistroSeries_filters_by_package_and_packageset_name(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        other_name = self.factory.getUniqueUnicode()
+        source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            [],
+            source.getForDistroSeries(series, name_filter=other_name))
+
+    def test_getForDistroSeries_ignores_parent_packagesets(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        packageset = self.factory.makePackageset(
+            distroseries=dsd.parent_series,
+            packages=[dsd.source_package_name])
+        packageset_name = packageset.name
+        source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            [],
+            source.getForDistroSeries(series, name_filter=packageset_name))
+
     def test_getForDistroSeries_sorted_by_package_name(self):
         # The differences are sorted by package name.
         derived_series = self.makeDerivedSeries()

=== modified file 'lib/lp/soyuz/interfaces/packageset.py'
--- lib/lp/soyuz/interfaces/packageset.py	2011-06-22 17:06:36 +0000
+++ lib/lp/soyuz/interfaces/packageset.py	2011-07-08 10:08:21 +0000
@@ -411,7 +411,8 @@
         :param distroseries: the distroseries to which the new packageset
             is related. Defaults to the current Ubuntu series.
 
-        :return: An `IPackageset` instance or None.
+        :return: An `IPackageset` instance.
+        :raise NoSuchPackageSet: if no package set is found.
         """
 
     @collection_default_content()

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-07-07 10:31:38 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-07-08 10:08:21 +0000
@@ -496,8 +496,7 @@
         dsd_source = getUtility(IDistroSeriesDifferenceSource)
         target_series = self.target_distroseries
         candidates = dsd_source.getForDistroSeries(
-            distro_series=target_series,
-            source_package_name_filter=self.package_name)
+            distro_series=target_series, name_filter=self.package_name)
 
         # The job doesn't know what distroseries a given package is
         # coming from, and the version number in the DSD may have