launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17172
Re: [Merge] lp:~cjohnston/launchpad/dd-initialization into lp:launchpad
Review: Needs Fixing code
Diff comments:
> === 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-18 00:01:02 +0000
> @@ -47,6 +47,8 @@
> this.registerWidget(this.architectureChoice);
> this.architectureIndepChoice = config.architectureIndepChoice;
> this.registerWidget(this.architectureIndepChoice);
> + this.packagesetImportOptions = config.packagesetImportOptions;
> + this.registerWidget(this.packagesetImportOptions);
> this.packagesetChoice = config.packagesetChoice;
> this.registerWidget(this.packagesetChoice);
> this.packageCopyOptions = config.packageCopyOptions;
> @@ -83,7 +85,7 @@
> * @method validate
> */
> validate: function() {
> - this.hideErrors();
> + //this.hideErrors();
> var arch_indep_choice = this.architectureIndepChoice.get(
> 'choice').value;
> if (arch_indep_choice === this.architectureIndepChoice.AUTOSELECT) {
> @@ -139,10 +141,19 @@
> var values = attrselect("value");
> var arch_indep = values(
> this.architectureIndepChoice.get("choice"))[0];
> + var pkgset = this.packagesetImportOptions.get("choice").value;
Please revise the 'pkgset' and 'packagesetImportOptions' names. They're not a packageset, or a choice of packagesets; rather they're a choice of packages to copy. There's also no need for 'pkgsets' to be abbreviated from its normal spelling.
> + if (pkgset === 'none') {
> + var pkgsets = [];
> + } else if (pkgset === 'all') {
> + var pkgsets = null;
Trailing whitespace.
> + } else {
> + var pkgsets = values(this.packagesetChoice.get("choice"));
> + }
> + console.log(pkgsets);
> var config = {
> on: {
> start: function() {
> - self.hideErrors();
> + //self.hideErrors();
> self.showSpinner();
> },
> success: function() {
> @@ -160,8 +171,7 @@
> archindep_archtag:
> arch_indep === this.architectureIndepChoice.AUTOSELECT ?
> null : arch_indep,
> - packagesets: this.packagesetChoice !== null ?
> - values(this.packagesetChoice.get("choice")) : [],
> + packagesets: pkgsets,
> rebuild:
> this.packageCopyOptions.get("choice").value === "rebuild",
> overlays: this.deriveFromChoices.get("overlays"),
> @@ -267,6 +277,20 @@
> "Choose the architecture tag that should be " +
> "used to build architecture independent binaries.")
> .render(form_table_body);
> + var packageset_import_options =
> + new formwidgets.ChoiceListWidget()
> + .set("name", "field.packageset_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);
> var packageset_choice = null;
> if (cache.is_first_derivation) {
> packageset_choice =
> @@ -278,10 +302,9 @@
> .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")
> .render(form_table_body);
> }
> var package_copy_options =
> @@ -304,6 +327,7 @@
> deriveFromChoices: parents_selection,
> architectureChoice: architecture_choice,
> architectureIndepChoice: arch_indep_choice,
> + packagesetImportOptions: packageset_import_options,
> packagesetChoice: packageset_choice,
> packageCopyOptions: package_copy_options,
> form_container: form_container
> @@ -387,6 +411,21 @@
> form_actions.deriveFromChoices);
> }
>
> + Y.one('.yui3-packagesetpickerwidget').hide();
> + form_actions.packagesetImportOptions.on('change', function (e) {
> + var val = e.currentTarget.get('choice').value;
> + Y.fire('import_option_change', val)
> + });
> +
> + Y.on('import_option_change', function(val) {
> + var pickerwidget = Y.one('.yui3-packagesetpickerwidget')
> + if (val === 'some') {
> + pickerwidget.show();
> + } else {
> + pickerwidget.hide();
> + }
> + });
> +
Avoid hardcoding the class here; consider form_actions.packagesetChoice.get("contentBox").hide().
Also, there's not much point defining a new event if its only listener is in the same function. If you're going to have both here, I'd just inline the show/hide into the 'change' event handler.
> // Wire up the form check and submission.
> form_actions.form_container.one("form").on(
> "submit", function(event) {
>
> === 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-18 00:01:02 +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-18 00:01: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(
> @@ -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) # 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()
>
--
https://code.launchpad.net/~cjohnston/launchpad/dd-initialization/+merge/226796
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
