launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17170
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-17 13:47:49 +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;
> + if (pkgset === 'none') {
> + var pkgsets = [];
> + } else if (pkgset === 'all') {
> + var pkgsets = null;
> + } 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,16 @@
> form_actions.deriveFromChoices);
> }
>
> + Y.one('.yui3-packagesetpickerwidget').hide();
> + Y.all('input[type=radio]').on('change', function (e) {
> + var val = e.currentTarget.get('value');
> + if (val === 'some') {
> + Y.one('.yui3-packagesetpickerwidget').show();
> + } else {
> + Y.one('.yui3-packagesetpickerwidget').hide();
> + }
> + });
This shouldn't be poking inside other widgets' elements. Use an event to ask the packageset widget to show or hide itself.
Also, that will hook every single radio button on the page.
> +
> // 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-17 13:47:49 +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
> @@ -490,6 +494,13 @@
> if pkgset.distroseries == parent:
> spns += list(pkgset.getSourcesIncluded())
> source_names_by_parent[parent.id] = spns
> + elif self.packagesets == None:
> + for parent in self.derivation_parents:
> + source_names_by_parent[parent.id] = None
> + else:
> + for parent in self.derivation_parents:
> + spns = []
> + source_names_by_parent[parent.id] = spns
The same behaviour can be achieved by just replacing "if packagesets_ids:" with "if packagesets_ids is not None:" in the original code.
> self.source_names_by_parent = source_names_by_parent
>
> def _copy_publishing_records(self, distroarchseries_lists):
> @@ -652,55 +663,56 @@
>
> def _copy_packagesets(self):
> """Copy packagesets from the parent distroseries."""
> - packagesets = self._store.find(
> - Packageset,
> - Packageset.distroseries_id.is_in(self.derivation_parent_ids))
> - parent_to_child = {}
> - # Create the packagesets and any archivepermissions if we're not
> - # copying cross-distribution.
> - parent_distro_ids = [
> - parent.distribution.id for parent in self.derivation_parents]
> - 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
> - str(parent_ps.id) not in self.packagesets_ids):
> - continue
> - packageset_set = getUtility(IPackagesetSet)
> - # First, try to fetch an existing packageset with this name.
> - try:
> - child_ps = packageset_set.getByName(
> - self.distroseries, parent_ps.name)
> - except NoSuchPackageSet:
> - if self.distroseries.distribution.id in parent_distro_ids:
> - new_owner = parent_ps.owner
> - else:
> - new_owner = self.distroseries.owner
> - child_ps = getUtility(IPackagesetSet).new(
> - parent_ps.name, parent_ps.description,
> - new_owner, distroseries=self.distroseries,
> - related_set=parent_ps)
> - parent_to_child[parent_ps] = child_ps
> - # Copy archivepermissions if we're not copying
> - # cross-distribution.
> - if (self.distroseries.distribution ==
> - parent_ps.distroseries.distribution):
> - self._store.execute("""
> - INSERT INTO Archivepermission
> - (person, permission, archive, packageset, explicit)
> - SELECT person, permission, %s, %s, explicit
> - FROM Archivepermission WHERE packageset = %s
> - """ % sqlvalues(
> - self.distroseries.main_archive, child_ps.id,
> - parent_ps.id))
> - # Copy the relations between sets, and the contents.
> - for old_series_ps, new_series_ps in parent_to_child.items():
> - old_series_sets = old_series_ps.setsIncluded(
> - direct_inclusion=True)
> - for old_series_child in old_series_sets:
> - new_series_ps.add(parent_to_child[old_series_child])
> - new_series_ps.add(old_series_ps.sourcesIncluded(
> - direct_inclusion=True))
> + if self.packagesets != []:
> + packagesets = self._store.find(
> + Packageset,
> + Packageset.distroseries_id.is_in(self.derivation_parent_ids))
> + parent_to_child = {}
> + # Create the packagesets and any archivepermissions if we're not
> + # copying cross-distribution.
> + parent_distro_ids = [
> + parent.distribution.id for parent in self.derivation_parents]
> + 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
> + str(parent_ps.id) not in self.packagesets_ids):
> + continue
> + packageset_set = getUtility(IPackagesetSet)
> + # First, try to fetch an existing packageset with this name.
> + try:
> + child_ps = packageset_set.getByName(
> + self.distroseries, parent_ps.name)
> + except NoSuchPackageSet:
> + if self.distroseries.distribution.id in parent_distro_ids:
> + new_owner = parent_ps.owner
> + else:
> + new_owner = self.distroseries.owner
> + child_ps = getUtility(IPackagesetSet).new(
> + parent_ps.name, parent_ps.description,
> + new_owner, distroseries=self.distroseries,
> + related_set=parent_ps)
> + parent_to_child[parent_ps] = child_ps
> + # Copy archivepermissions if we're not copying
> + # cross-distribution.
> + if (self.distroseries.distribution ==
> + parent_ps.distroseries.distribution):
> + self._store.execute("""
> + INSERT INTO Archivepermission
> + (person, permission, archive, packageset, explicit)
> + SELECT person, permission, %s, %s, explicit
> + FROM Archivepermission WHERE packageset = %s
> + """ % sqlvalues(
> + self.distroseries.main_archive, child_ps.id,
> + parent_ps.id))
> + # Copy the relations between sets, and the contents.
> + for old_series_ps, new_series_ps in parent_to_child.items():
> + old_series_sets = old_series_ps.setsIncluded(
> + direct_inclusion=True)
> + for old_series_child in old_series_sets:
> + new_series_ps.add(parent_to_child[old_series_child])
> + new_series_ps.add(old_series_ps.sourcesIncluded(
> + direct_inclusion=True))
This entire hunk can be replaced by changing "if (self.packagesets_ids and" to "if (self.packagesets_ids is not None and".
>
> def _copy_pocket_permissions(self):
> """Copy per-distroseries/pocket permissions from the parent series."""
>
> === 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-17 13:47:49 +0000
> @@ -849,6 +849,58 @@
> 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 want to copy none 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')
We also need something to test that no packages are copied, not just no packagesets.
> +
> + 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)
> + 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=None)
> + child_test1 = getUtility(IPackagesetSet).getByName(child, u'test1')
> + self.assertEqual(test1.description, child_test1.description)
> + self.assertRaises(
> + NoSuchPackageSet, getUtility(IPackagesetSet).getByName,
> + child, u'test2')
> + parent_srcs = test1.getSourcesIncluded(direct_inclusion=True)
> + child_srcs = child_test1.getSourcesIncluded(
> + direct_inclusion=True)
> + self.assertEqual(parent_srcs, child_srcs)
> + child.updatePackageCount()
> + self.assertEqual(child.sourcecount, len(packages))
> + self.assertEqual(child.binarycount, 2) # 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
