launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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-15 12:44:06 +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 = 'None'; 
> +        } 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
> 
> === 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-15 12:44:06 +0000
> @@ -116,8 +116,11 @@
>          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 == [u'None']:

This is weird. Why isn't it just None?

> +            self.packagesets = None
> +        else:
> +            self.packagesets = bulk.load(
> +                Packageset, [int(packageset) for packageset in packagesets])
>          self.rebuild = rebuild
>          self.overlays = overlays
>          self.overlay_pockets = overlay_pockets
> @@ -216,7 +219,8 @@
>          Restrict the check to the selected packages if a limited set of
>          packagesets is used by the initialization.
>          """
> -        spns = self.source_names_by_parent.get(parent.id, None)
> +        if self.source_names_by_parent is not None:
> +            spns = self.source_names_by_parent.get(parent.id, None)

Why is source_names_by_parent sometimes None? What does that mean, and where is spns defined in that case?

>          if spns is not None and len(spns) == 0:
>              # If no sources are selected in this parent, skip the check.
>              return
> @@ -246,7 +250,8 @@
>              PackageUploadStatus.ACCEPTED,
>              PackageUploadStatus.UNAPPROVED,
>              ]
> -        spns = self.source_names_by_parent.get(parent.id, None)
> +        if self.source_names_by_parent is not None:
> +            spns = self.source_names_by_parent.get(parent.id, None)
>          if spns is not None and len(spns) == 0:
>              # If no sources are selected in this parent, skip the check.
>              return
> @@ -483,13 +488,18 @@
>          parent so the list of packages to consider in not empty.
>          """
>          source_names_by_parent = {}
> -        if self.packagesets_ids:
> -            for parent in self.derivation_parents:
> -                spns = []
> -                for pkgset in self.packagesets:
> -                    if pkgset.distroseries == parent:
> -                        spns += list(pkgset.getSourcesIncluded())
> -                source_names_by_parent[parent.id] = spns
> +        if self.packagesets is None:
> +            source_names_by_parent = None

Why None?

> +        elif self.packagesets is []:
> +            self.source_names_by_parent = []

source_names_by_parent is a dict, not a list. One also doesn't usually use "is" to compare lists.

> +        else:
> +            if self.packagesets_ids:
> +                for parent in self.derivation_parents:
> +                    spns = []
> +                    for pkgset in self.packagesets:
> +                        if pkgset.distroseries == parent:
> +                            spns += list(pkgset.getSourcesIncluded())
> +                    source_names_by_parent[parent.id] = spns
>          self.source_names_by_parent = source_names_by_parent
>  
>      def _copy_publishing_records(self, distroarchseries_lists):
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/dd-initialization/+merge/226796
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjohnston/launchpad/dd-initialization into lp:launchpad.