launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === 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-21 22:48:22 +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-21 22:48:22 +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,17 @@
>          var values = attrselect("value");
>          var arch_indep = values(
>              this.architectureIndepChoice.get("choice"))[0];
> +        var copy_package = this.packageCopyChoice !== null ?
> +            this.packageCopyChoice.get("choice").value : [];

This means that copy_package is sometimes a string, sometimes an empty array. That's a bit weird.

> +        var packageset_ids = [];
> +        if (copy_package === 'none') {
> +            packageset_ids = [];
> +        } else if (copy_package === 'all') {
> +            packageset_ids = null; 
> +        } else {
> +            packageset_ids = this.packagesetChoice !== null ?
> +                values(this.packagesetChoice.get("choice")) : [];

is_first_derivation=False causes InitializeDistroSeries to be invoked with packagesets=[], so it used to copy all packages. But [] now means *no* packages. That probably means Vicarious Velociraptor will end up empty!

> +        }
>          var config = {
>              on: {
>                  start: function() {
> @@ -160,8 +173,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 +279,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 +304,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 +327,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 +387,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-21 22:48:22 +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/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-21 22:48:22 +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-04 11:21:54 +0000
> +++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2014-07-21 22:48:22 +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:
> @@ -849,6 +849,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()
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/dd-initialization/+merge/226796
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.