Merge lp:~cjohnston/launchpad/dd-initialization into lp:launchpad

[Chris Johnston]

Allow a user to copy all, some or no packages when initializing a new distro series.

[William Grant (wgrant)]

The packageset_import widget isn't exactly choosing which packagesets to import; not every package is in a packageset, so choosing to copy ("import" isn't a term that Launchpad ever uses for copying packages internally) all packages does more than copy all the packagesets. I'd say it's the widget that chooses which packages to copy.

Consider "Don't copy any packages", "Copy all packages", "Copy some packagesets". It probably also makes sense to only show the packageset selection widget in the "some" case.

[William Grant (wgrant)]

review: Needs Fixing (code)

[William Grant (wgrant)]

review: Needs Fixing (code)

[William Grant (wgrant)]

For tests, packagesets=None seems like a more sensible default than packagesets=[]. Would it make more sense to change the tests that need [] to use [] explicitly?

[William Grant (wgrant)]

review: Needs Fixing (code)

[William Grant (wgrant)]

review: Needs Fixing (code)

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2014-05-29 07:27:25 +0000
+++ lib/canonical/launchpad/icing/style.css	2014-07-22 20:32:02 +0000
@@ -876,6 +876,11 @@
   display: none;
 }
 
+/* Hide the packageset picker widget on the Initialize series page */
+.yui3-packagesetpickerwidget-hidden {
+    display: none;
+}
+
 /* Search results*/
 
 #search-results {

=== modified file 'lib/lp/registry/javascript/distroseries/initseries.js'
--- lib/lp/registry/javascript/distroseries/initseries.js	2012-07-07 14:00:30 +0000
+++ lib/lp/registry/javascript/distroseries/initseries.js	2014-07-22 20:32:02 +0000
@@ -47,6 +47,8 @@
         this.registerWidget(this.architectureChoice);
         this.architectureIndepChoice = config.architectureIndepChoice;
         this.registerWidget(this.architectureIndepChoice);
+        this.packageCopyChoice = config.packageCopyChoice;
+        this.registerWidget(this.packageCopyChoice);
         this.packagesetChoice = config.packagesetChoice;
         this.registerWidget(this.packagesetChoice);
         this.packageCopyOptions = config.packageCopyOptions;
@@ -139,6 +141,16 @@
         var values = attrselect("value");
         var arch_indep = values(
             this.architectureIndepChoice.get("choice"))[0];
+        var copy_package = this.packageCopyChoice !== null ?
+            this.packageCopyChoice.get("choice").value : 'all';
+        var packageset_ids = [];
+        if (copy_package === 'none') {
+            packageset_ids = [];
+        } else if (copy_package === 'all') {
+            packageset_ids = null; 
+        } else {
+            packageset_ids = values(this.packagesetChoice.get("choice"));
+        }
         var config = {
             on: {
                 start: function() {
@@ -160,8 +172,7 @@
                 archindep_archtag:
                     arch_indep === this.architectureIndepChoice.AUTOSELECT ?
                         null : arch_indep,
-                packagesets: this.packagesetChoice !== null ?
-                    values(this.packagesetChoice.get("choice")) : [],
+                packagesets: packageset_ids,
                 rebuild:
                     this.packageCopyOptions.get("choice").value === "rebuild",
                 overlays: this.deriveFromChoices.get("overlays"),
@@ -267,8 +278,22 @@
                      "Choose the architecture tag that should be " +
                      "used to build architecture independent binaries.")
             .render(form_table_body);
+    var package_copy_choice = null;
     var packageset_choice = null;
     if (cache.is_first_derivation) {
+        package_copy_choice =
+        new formwidgets.ChoiceListWidget()
+            .set("name", "field.package_copy_choice")
+            .set("multiple", false)
+            .set("label", "Packages to copy:")
+            .set("description", 
+                "Choose which packages to copy from the parent series ")
+            .set("choices", [
+                {text: "Copy all packages", value: "all"},
+                {text: "Copy some packagesets", value: "some"},
+                {text: "Don't copy any packages", value: "none"}])
+            .set("choice", "all")
+            .render(form_table_body);
         packageset_choice =
             new widgets.PackagesetPickerWidget()
                 .set("name", "field.packagesets")
@@ -278,10 +303,7 @@
                 .set("multiple", true)
                 .set("label", "Package sets to copy from parent:")
                 .set("description",
-                         "The package sets that will be imported " +
-                         "into the derived distroseries (select none " +
-                         "if you want to import all the available " +
-                         "package sets).")
+                         "Select the package sets to copy.")
                 .render(form_table_body);
     }
     var package_copy_options =
@@ -304,6 +326,7 @@
             deriveFromChoices: parents_selection,
             architectureChoice: architecture_choice,
             architectureIndepChoice: arch_indep_choice,
+            packageCopyChoice: package_copy_choice,
             packagesetChoice: packageset_choice,
             packageCopyOptions: package_copy_options,
             form_container: form_container
@@ -363,6 +386,20 @@
             form_actions.architectureChoice.remove_distroseries(parent_id);
             form_actions.packagesetChoice.remove_distroseries(parent_id);
         });
+
+        // Hide packagesetChoice by default
+        form_actions.packagesetChoice.hide();
+
+        // Unhide packagesetChoice if user wants to specify which packagesets
+        // to copy.
+        form_actions.packageCopyChoice.on('change', function (e) {
+            var val = e.currentTarget.get('choice').value;
+            if (val === 'some') {
+                form_actions.packagesetChoice.show();
+            } else {
+                form_actions.packagesetChoice.hide();
+            }
+        });
     }
     else {
         // Disable rebuilding if cache.is_first_derivation is false.

=== modified file 'lib/lp/registry/javascript/distroseries/tests/test_initseries.js'
--- lib/lp/registry/javascript/distroseries/tests/test_initseries.js	2012-10-26 10:00:20 +0000
+++ lib/lp/registry/javascript/distroseries/tests/test_initseries.js	2014-07-22 20:32:02 +0000
@@ -69,6 +69,15 @@
                         return {value: "sparc", text: "sparc"};
                     }
                 },
+                packageCopyChoice: {
+                    get: function(name) {
+                        Assert.areEqual("choice", name);
+                        return {
+                            value: "some",
+                            text: "Copy some packagesets"
+                        };
+                    }
+                },
                 packagesetChoice: {
                     get: function(name) {
                         Assert.areEqual("choice", name);
@@ -266,6 +275,14 @@
                 form_actions.deriveFromChoices.get("parents"));
         },
 
+        testDefaultCopyChoice: function() {
+            var cache = {is_first_derivation: true};
+            var form_actions = initseries.setupWidgets(cache);
+            Assert.areEqual(
+                "all",
+                form_actions.packageCopyChoice.get('choice').value);
+        },
+
         testDefaultRebuildChoice: function() {
             var cache = {is_first_derivation: true};
             var form_actions = initseries.setupWidgets(cache);
@@ -328,7 +345,8 @@
             ArrayAssert.itemsAreEqual(
                 ['4', '5'],
                 form_actions.deriveFromChoices.get("parents"));
-            // No packageset choice widget.
+            // No packageCopyChoice widget or packageset choice widget.
+            Assert.isNull(form_actions.packageCopyChoice);
             Assert.isNull(form_actions.packagesetChoice);
             // The architecture picker features the architectures
             // from the previous series.

=== modified file 'lib/lp/soyuz/model/initializedistroseriesjob.py'
--- lib/lp/soyuz/model/initializedistroseriesjob.py	2014-05-28 12:23:10 +0000
+++ lib/lp/soyuz/model/initializedistroseriesjob.py	2014-07-22 20:32:02 +0000
@@ -51,7 +51,7 @@
 
     @classmethod
     def create(cls, child, parents, arches=(), archindep_archtag=None,
-               packagesets=(), rebuild=False, overlays=(),
+               packagesets=None, rebuild=False, overlays=(),
                overlay_pockets=(), overlay_components=()):
         """Create a new `InitializeDistroSeriesJob`.
 
@@ -139,9 +139,12 @@
                 self.overlay_pockets[i],
                 self.overlay_components[i]))
         parts += ",".join(parents)
-        pkgsets = [
-            IStore(Packageset).get(Packageset, int(pkgsetid)).name
-            for pkgsetid in self.packagesets]
+        if self.packagesets is None:
+            pkgsets = None
+        else:
+            pkgsets = [
+                IStore(Packageset).get(Packageset, int(pkgsetid)).name
+                for pkgsetid in self.packagesets]
         parts += ", architectures: %s" % (self.arches,)
         parts += ", archindep_archtag: %s" % self.archindep_archtag
         parts += ", packagesets: %s" % pkgsets
@@ -187,7 +190,7 @@
     @property
     def packagesets(self):
         if self.metadata['packagesets'] is None:
-            return ()
+            return None
         else:
             return tuple(self.metadata['packagesets'])
 

=== modified file 'lib/lp/soyuz/scripts/initialize_distroseries.py'
--- lib/lp/soyuz/scripts/initialize_distroseries.py	2014-07-04 11:21:54 +0000
+++ lib/lp/soyuz/scripts/initialize_distroseries.py	2014-07-22 20:32:02 +0000
@@ -102,7 +102,7 @@
 
     def __init__(
         self, distroseries, parents=(), arches=(), archindep_archtag=None,
-        packagesets=(), rebuild=False, overlays=(), overlay_pockets=(),
+        packagesets=None, rebuild=False, overlays=(), overlay_pockets=(),
         overlay_components=()):
         self.distroseries = distroseries
         self.parent_ids = [int(id) for id in parents]
@@ -114,10 +114,14 @@
             key=lambda parent: self.parent_ids.index(parent.id))
         self.arches = arches
         self.archindep_archtag = archindep_archtag
-        self.packagesets_ids = [
-            ensure_unicode(packageset) for packageset in packagesets]
-        self.packagesets = bulk.load(
-            Packageset, [int(packageset) for packageset in packagesets])
+        if packagesets is None:
+            self.packagesets_ids = None
+            self.packagesets = None
+        else:
+            self.packagesets_ids = [
+                ensure_unicode(packageset) for packageset in packagesets]
+            self.packagesets = bulk.load(
+                Packageset, [int(packageset) for packageset in packagesets])
         self.rebuild = rebuild
         self.overlays = overlays
         self.overlay_pockets = overlay_pockets
@@ -326,7 +330,7 @@
     def _create_dsds(self):
         if not self.first_derivation:
             if (self._has_same_parents_as_previous_series() and
-                not self.packagesets_ids):
+                self.packagesets_ids is None):
                 # If the parents are the same as previous_series's
                 # parents and all the packagesets are being copied,
                 # then we simply copy the DSDs from previous_series
@@ -483,7 +487,7 @@
         parent so the list of packages to consider in not empty.
         """
         source_names_by_parent = {}
-        if self.packagesets_ids:
+        if self.packagesets_ids is not None:
             for parent in self.derivation_parents:
                 spns = []
                 for pkgset in self.packagesets:
@@ -663,7 +667,7 @@
         for parent_ps in packagesets:
             # Cross-distro initializations get packagesets owned by the
             # distro owner, otherwise the old owner is preserved.
-            if (self.packagesets_ids and
+            if (self.packagesets_ids is not None and
                 str(parent_ps.id) not in self.packagesets_ids):
                 continue
             packageset_set = getUtility(IPackagesetSet)

=== modified file 'lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2014-07-16 00:57:21 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2014-07-22 20:32:02 +0000
@@ -89,7 +89,7 @@
                          pocket=PackagePublishingPocket.RELEASE):
         if packages is None:
             packages = {'udev': '0.1-1', 'libc6': '2.8-1',
-                'postgresql': '9.0-1', 'chromium': '3.6'}
+                'postgresql': '9.0-1', 'chromium': '3.6', 'vim': '7.4'}
         for package in packages.keys():
             spn = self.factory.getOrMakeSourcePackageName(package)
             spph = self.factory.makeSourcePackagePublishingHistory(
@@ -115,7 +115,7 @@
                 self.factory.makeBinaryPackageFile(binarypackagerelease=bpr)
 
     def _fullInitialize(self, parents, child=None, previous_series=None,
-                        arches=(), archindep_archtag=None, packagesets=(),
+                        arches=(), archindep_archtag=None, packagesets=None,
                         rebuild=False, distribution=None, overlays=(),
                         overlay_pockets=(), overlay_components=()):
         if child is None:
@@ -847,6 +847,69 @@
         self.assertEqual(child.sourcecount, len(packages))
         self.assertEqual(child.binarycount, 2)  # Chromium is FTBFS
 
+    def test_copy_limit_packagesets_empty(self):
+        # If a parent series has packagesets, we don't want to copy any of them
+        self.parent, self.parent_das = self.setupParent()
+        test1 = getUtility(IPackagesetSet).new(
+            u'test1', u'test 1 packageset', self.parent.owner,
+            distroseries=self.parent)
+        getUtility(IPackagesetSet).new(
+            u'test2', u'test 2 packageset', self.parent.owner,
+            distroseries=self.parent)
+        packages = ('udev', 'chromium', 'libc6')
+        for pkg in packages:
+            test1.addSources(pkg)
+        packageset1 = getUtility(IPackagesetSet).getByName(
+            self.parent, u'test1')
+        child = self._fullInitialize(
+            [self.parent], packagesets=[])
+        self.assertRaises(
+            NoSuchPackageSet, getUtility(IPackagesetSet).getByName,
+            child, u'test1')
+        self.assertRaises(
+            NoSuchPackageSet, getUtility(IPackagesetSet).getByName,
+            child, u'test2')
+        self.assertEqual(child.sourcecount, 0)
+        self.assertEqual(child.binarycount, 0)
+
+    def test_copy_limit_packagesets_none(self):
+        # If a parent series has packagesets, we want to copy all of them
+        self.parent, self.parent_das = self.setupParent()
+        test1 = getUtility(IPackagesetSet).new(
+            u'test1', u'test 1 packageset', self.parent.owner,
+            distroseries=self.parent)
+        test2 = getUtility(IPackagesetSet).new(
+            u'test2', u'test 2 packageset', self.parent.owner,
+            distroseries=self.parent)
+        packages_test1 = ('udev', 'chromium', 'libc6')
+        packages_test2 = ('postgresql', 'vim')
+        for pkg in packages_test1:
+            test1.addSources(pkg)
+        for pkg in packages_test2:
+            test2.addSources(pkg)
+        packageset1 = getUtility(IPackagesetSet).getByName(
+            self.parent, u'test1')
+        packageset2 = getUtility(IPackagesetSet).getByName(
+            self.parent, u'test2')
+        child = self._fullInitialize(
+            [self.parent], packagesets=None)
+        child_test1 = getUtility(IPackagesetSet).getByName(child, u'test1')
+        child_test2 = getUtility(IPackagesetSet).getByName(child, u'test2')
+        self.assertEqual(test1.description, child_test1.description)
+        self.assertEqual(test2.description, child_test2.description)
+        parent_srcs_test1 = test1.getSourcesIncluded(direct_inclusion=True)
+        child_srcs_test1 = child_test1.getSourcesIncluded(
+            direct_inclusion=True)
+        self.assertEqual(parent_srcs_test1, child_srcs_test1)
+        parent_srcs_test2 = test2.getSourcesIncluded(direct_inclusion=True)
+        child_srcs_test2 = child_test2.getSourcesIncluded(
+            direct_inclusion=True)
+        self.assertEqual(parent_srcs_test2, child_srcs_test2)
+        child.updatePackageCount()
+        self.assertEqual(child.sourcecount,
+            len(packages_test1) + len(packages_test2))
+        self.assertEqual(child.binarycount, 4)  # Chromium is FTBFS
+
     def test_rebuild_flag(self):
         # No binaries will get copied if we specify rebuild=True.
         self.parent, self.parent_das = self.setupParent()