launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/canonical/launchpad/icing/css/components/yui_picker.css'
> --- lib/canonical/launchpad/icing/css/components/yui_picker.css	2012-08-09 04:56:41 +0000
> +++ lib/canonical/launchpad/icing/css/components/yui_picker.css	2014-07-21 01:02:30 +0000
> @@ -25,3 +25,6 @@
>  .yui3-picker-filter div {
>      padding-bottom: 1em;
>      }
> +.yui3-packagesetpickerwidget-hidden {
> +    display:none;

Missing space.

> +}
> 
> === modified file 'lib/lp/app/javascript/formwidgets/formwidgets.js'
> --- lib/lp/app/javascript/formwidgets/formwidgets.js	2012-07-07 14:00:30 +0000
> +++ lib/lp/app/javascript/formwidgets/formwidgets.js	2014-07-21 01:02:30 +0000
> @@ -131,6 +131,16 @@
>              .append(this.descriptionNode);
>      },
>  
> +    bindUI: function() {
> +        Y.on('togglevisibility', function(val) {
> +            if (val === 'show') {

Why not a boolean?

> +                this.set('visible', true);
> +            } else {
> +                this.set('visible', false);
> +            }
> +        }, this);
> +    },
> +
>      /**
>       * Show the spinner.
>       *
> 
> === 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 01:02:30 +0000
> @@ -47,6 +47,8 @@
>          this.registerWidget(this.architectureChoice);
>          this.architectureIndepChoice = config.architectureIndepChoice;
>          this.registerWidget(this.architectureIndepChoice);
> +        this.packageImportOptions = config.packageImportOptions;
> +        this.registerWidget(this.packageImportOptions);
>          this.packagesetChoice = config.packagesetChoice;
>          this.registerWidget(this.packagesetChoice);
>          this.packageCopyOptions = config.packageCopyOptions;
> @@ -83,7 +85,7 @@
>       * @method validate
>       */
>      validate: function() {
> -        this.hideErrors();
> +        //this.hideErrors();

Can this be uncommented now?

>          var arch_indep_choice = this.architectureIndepChoice.get(
>              'choice').value;
>          if (arch_indep_choice === this.architectureIndepChoice.AUTOSELECT) {
> @@ -139,10 +141,18 @@
>          var values = attrselect("value");
>          var arch_indep = values(
>              this.architectureIndepChoice.get("choice"))[0];
> +        var copy_package = this.packageImportOptions.get("choice").value;
> +        if (copy_package === 'none') {
> +            var packagess = [];

Spelling.

Also, the variable should only be declared once, and "packages" is a misleading name for what is an array of packageset ID.

> +        } else if (copy_package === 'all') {
> +            var packages = null; 
> +        } else {
> +            var packages = values(this.packagesetChoice.get("choice"));
> +        }
>          var config = {
>              on: {
>                  start: function() {
> -                    self.hideErrors();
> +                    //self.hideErrors();

Also needs uncommenting.

>                      self.showSpinner();
>                  },
>                  success: function() {
> @@ -160,8 +170,7 @@
>                  archindep_archtag:
>                      arch_indep === this.architectureIndepChoice.AUTOSELECT ?
>                          null : arch_indep,
> -                packagesets: this.packagesetChoice !== null ?
> -                    values(this.packagesetChoice.get("choice")) : [],
> +                packagesets: packages,
>                  rebuild:
>                      this.packageCopyOptions.get("choice").value === "rebuild",
>                  overlays: this.deriveFromChoices.get("overlays"),
> @@ -267,6 +276,20 @@
>                       "Choose the architecture tag that should be " +
>                       "used to build architecture independent binaries.")
>              .render(form_table_body);
> +    var package_import_options =
> +        new formwidgets.ChoiceListWidget()
> +            .set("name", "field.package_import_options")
> +            .set("multiple", false)
> +            .set("label", "Package set import options:")
> +            .set("description", (
> +                     "Choose whether to import all package sets, some " +
> +                     "package sets or no package sets."))
> +            .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);

packagesetChoice is only created when is_first_derivation is true. You need to do the same with your new packageImport widget, which should still be renamed to something more accurate (eg. "packageCopyChoice").

>      var packageset_choice = null;
>      if (cache.is_first_derivation) {
>          packageset_choice =
> @@ -278,10 +301,10 @@
>                  .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 which package sets to be imported. Will " +
> +                         "only be considered if 'Choose which package sets " +
> +                         "to import was selected above")

The field is only shown when the relevant option is selected.

> +                .set("visible", false)
>                  .render(form_table_body);
>      }
>      var package_copy_options =
> @@ -304,6 +327,7 @@
>              deriveFromChoices: parents_selection,
>              architectureChoice: architecture_choice,
>              architectureIndepChoice: arch_indep_choice,
> +            packageImportOptions: package_import_options,
>              packagesetChoice: packageset_choice,
>              packageCopyOptions: package_copy_options,
>              form_container: form_container
> @@ -387,6 +411,15 @@
>              form_actions.deriveFromChoices);
>      }
>  
> +    form_actions.packageImportOptions.on('change', function (e) {
> +        var val = e.currentTarget.get('choice').value;
> +        if (val === 'some') {
> +            Y.fire('togglevisibility', 'show');
> +        } else {
> +            Y.fire('togglevisibility', 'hide');
> +        }
> +    });
> +

This uses a global 'togglevisibility' event. That will hide or show every widget on the page that has -hidden CSS defined, not just the packageset widget. Consider packagesetChoice.hide().

>      // Wire up the form check and submission.
>      form_actions.form_container.one("form").on(
>          "submit", function(event) {
> 
> === 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 01:02:30 +0000
> @@ -69,6 +69,15 @@
>                          return {value: "sparc", text: "sparc"};
>                      }
>                  },
> +                packageImportOptions: {
> +                    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"));
>          },
>  
> +        testDefaultImportChoice: function() {
> +            var cache = {is_first_derivation: true};
> +            var form_actions = initseries.setupWidgets(cache);
> +            Assert.areEqual(
> +                "all",
> +                form_actions.packageImportOptions.get('choice').value);
> +        },
> +
>          testDefaultRebuildChoice: function() {
>              var cache = {is_first_derivation: true};
>              var form_actions = initseries.setupWidgets(cache);
> 
> === 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 01:02:30 +0000
> @@ -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
> @@ -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 01:02:30 +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,

InitializeDistroSeries' packagesets argument has classically defaulted to (), which means to copy everything. A lot of test changes probably disappear if you change the default to None, preserving the old default behaviour of copying all packages.

>                          rebuild=False, distribution=None, overlays=(),
>                          overlay_pockets=(), overlay_components=()):
>          if child is None:
> @@ -201,7 +201,7 @@
>          self.factory.makeSourcePackagePublishingHistory(
>               distroseries=another_distroseries)
>          self.factory.makeDistroArchSeries(distroseries=child)
> -        ids = InitializeDistroSeries(child, [self.parent.id])
> +        ids = InitializeDistroSeries(child, [self.parent.id], packagesets=None)
>          self.assertRaisesWithContent(
>              InitializationError,
>              ("Series series has no previous series and the "
> @@ -225,7 +225,8 @@
>                  pocket=pocket)
>              source.createMissingBuilds()
>              child = self.factory.makeDistroSeries()
> -            ids = InitializeDistroSeries(child, [self.parent.id])
> +            ids = InitializeDistroSeries(
> +                child, [self.parent.id], packagesets=None)
>              self.assertRaisesWithContent(
>                  InitializationError,
>                  ("The parent series has pending builds for "
> @@ -261,7 +262,8 @@
>          # Initialize only with parent_das's architecture.
>          child = self.factory.makeDistroSeries()
>          ids = InitializeDistroSeries(
> -            child, [parent.id], arches=[parent_das2.architecturetag])
> +            child, [parent.id], arches=[parent_das2.architecturetag],
> +            packagesets=None)
>  
>          self.assertRaisesWithContent(
>              InitializationError,
> @@ -423,7 +425,7 @@
>              # Create a binary package upload for this upload.
>              upload.addBuild(self.factory.makeBinaryPackageBuild())
>              child = self.factory.makeDistroSeries()
> -            ids = InitializeDistroSeries(child, [parent.id])
> +            ids = InitializeDistroSeries(child, [parent.id], packagesets=None)
>  
>              self.assertRaisesWithContent(
>                  InitializationError,
> @@ -448,7 +450,7 @@
>              # Create a binary package upload for this upload.
>              upload.addBuild(self.factory.makeBinaryPackageBuild())
>              child = self.factory.makeDistroSeries()
> -            ids = InitializeDistroSeries(child, [parent.id])
> +            ids = InitializeDistroSeries(child, [parent.id], packagesets=None)
>  
>              self.assertRaisesWithContent(
>                  InitializationError,
> @@ -849,6 +851,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)  # Chromium is FTBFS
> +
> +    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()
> @@ -1212,7 +1277,8 @@
>          parent = self.factory.makeDistroSeries()
>          target_archive = distroseries.main_archive
>  
> -        ids = InitializeDistroSeries(distroseries, [parent.id])
> +        ids = InitializeDistroSeries(
> +            distroseries, [parent.id], packagesets=None)
>          self.assertTrue(
>              ids._use_cloner(
>                  target_archive, parent.main_archive))
> @@ -1238,7 +1304,8 @@
>          self.factory.makeSourcePackagePublishingHistory(
>              distroseries=other_distroseries)
>          target_archive = distroseries.main_archive
> -        ids = InitializeDistroSeries(distroseries, [parent.id])
> +        ids = InitializeDistroSeries(
> +            distroseries, [parent.id], packagesets=None)
>  
>          self.assertTrue(
>              ids._use_cloner(target_archive, target_archive))
> @@ -1253,7 +1320,8 @@
>              distribution=distroseries.distribution)
>          self.factory.makeSourcePackagePublishingHistory(
>              distroseries=other_distroseries)
> -        ids = InitializeDistroSeries(distroseries, [parent.id])
> +        ids = InitializeDistroSeries(
> +            distroseries, [parent.id], packagesets=None)
>  
>          self.assertFalse(
>              ids._use_cloner(target_archive, parent.main_archive))
> @@ -1267,7 +1335,8 @@
>              distroseries=distroseries)
>          target_archive = distroseries.main_archive
>  
> -        ids = InitializeDistroSeries(distroseries, [parent.id])
> +        ids = InitializeDistroSeries(
> +            distroseries, [parent.id], packagesets=None)
>          self.assertFalse(
>              ids._use_cloner(target_archive, target_archive))
>  
> 


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