Merge lp:~rvb/launchpad/bug-869221-ui into lp:launchpad

[Raphaël Badin]

[r=benji][bug=869221] Force the user to select at least one architecture that can be used to build architecture independent packages.

This branch is another branch towards a proper fix for #869221. It changes the ui (+initseries) so that the user will get an error if no architecture suitable to build architecture independent packages is selected.

To achieve that it:
- changes the way FormRowWidget.showError displays error to mimic what non js LaunchpadFormView does;
- add a default validate method to FormRowWidget;
- modify FormActionsWidget to be able to register widgets and call validate on each of these 'encapsulated' widgets;
- add the required methods in ArchitecturesChoiceListWidget to fetch the nominatedarchindep from each parent and use them to validate the architecture list;
- Change lib/lp/registry/javascript/distroseries/initseries.js to register all the widgets and call the validation method before submitting the form.

= Tests =

lib/lp/app/javascript/formwidgets/tests/test_formwidgets.html
lib/lp/registry/javascript/distroseries/tests/test_widgets.html

= QA =

Create a new series and go to ../myseries/+initseries. Inherit from two series and make sure that if none of the arch-indep builders architecture are selected, the initialization is not possible.

[Benji York (benji)]

This branch looks good.

I can't claim it's better but I thought you might like to see another
way to use call chaining in showError:

    this.fieldNode
        .append(Y.Node.create('<div />')
            .addClass('message')
            .set("text", error))
        .ancestor('tr')
            .addClass('error');

To my eyes the above makes the structure of what's happening a little
clearer; not that it was particularly obscure to start with. The
structural subscription JS uses this pattern to good effect.

Line 233 of the diff is indented by one space too many. Oh, and you
don't really need the "check" variable.

    check_and_submit: function() {
        if (this.validate()) {
            this.submit();
        }
    }

The join in testValidateFails surprised me. Is there a reason string
concatenation won't work there?

    Assert.areEqual(
        "The distroseries has no architectures selected to "
        + "build architecture independent binaries.",
        this.widget.fieldNode.one('.message').get('text'));

review: Approve (code)

[Raphaël Badin (rvb)]

> I can't claim it's better but I thought you might like to see another
> way to use call chaining in showError:
>
> this.fieldNode
> .append(Y.Node.create('<div />')
> .addClass('message')
> .set("text", error))
> .ancestor('tr')
> .addClass('error');
>
> To my eyes the above makes the structure of what's happening a little
> clearer; not that it was particularly obscure to start with. The
> structural subscription JS uses this pattern to good effect.

I agree, it's more compact this way.

> Line 233 of the diff is indented by one space too many. Oh, and you
> don't really need the "check" variable.
>
> check_and_submit: function() {
> if (this.validate()) {
> this.submit();
> }
> }

Fixed.

> The join in testValidateFails surprised me. Is there a reason string
> concatenation won't work there?
>
> Assert.areEqual(
> "The distroseries has no architectures selected to "
> + "build architecture independent binaries.",
> this.widget.fieldNode.one('.message').get('text'));

Force of habit I guess ;) I've changed that.

Thanks for the review.

=== modified file 'lib/lp/app/javascript/formwidgets/formwidgets.js'
--- lib/lp/app/javascript/formwidgets/formwidgets.js	2011-08-10 10:23:57 +0000
+++ lib/lp/app/javascript/formwidgets/formwidgets.js	2011-10-18 07:37:29 +0000
@@ -158,9 +158,35 @@
      * @method showError
      */
     showError: function(error) {
-        var message = Y.Node.create('<p />').set("text", error);
-        this.fieldNode.empty().append(message);
-        Y.lp.anim.red_flash({node: message}).run();
+        this.fieldNode
+            .append(Y.Node.create('<div />')
+                .addClass('message')
+                .set("text", error))
+            .ancestor('tr')
+                .addClass('error');
+        Y.lp.anim.red_flash({node: this.fieldNode.one('.message')}).run();
+    },
+
+    /**
+     * Hide any previously displayed error.
+     *
+     * @method hideError
+     */
+    hideError: function(error) {
+        this.fieldNode.all('.message').remove();
+        this.fieldNode.ancestor('tr').removeClass('error');
+    },
+
+    /**
+     * Validate this widget. Subclasses can override this method.
+     * It should return true if the widget validates and false otherwise.
+     * It is also responsible for setting any error on the widget using
+     * this.showError.
+     *
+     * @method hideError
+     */
+    validate: function() {
+        return true;
     }
 
 });
@@ -458,11 +484,50 @@
         this.error_handler.clearProgressUI = Y.bind(this.hideSpinner, this);
         this.error_handler.showError = Y.bind(this.showError, this);
         this.submitButtonNode = config.submitButtonNode;
+        this._widgets = [];
         this.spinnerNode = Y.Node.create(
             '<img src="/@@/spinner" alt="Loading..." />');
     },
 
     /**
+     * Register a widget as part of this encapsulating widget.
+     *
+     * @method registerWidget
+     */
+    registerWidget: function(widget) {
+        this._widgets.push(widget);
+    },
+
+    /**
+     * Validate all the registered widgets. Display a global error
+     * displaying the number of errors if any of the encapsulated widgets
+     * fails to validate.
+     *
+     * @method validate
+     */
+    validate: function() {
+        this.hideErrors();
+        var i = 0;
+        var error_number = 0;
+        for (i; i<this._widgets.length; i++) {
+            if (this._widgets[i].validate() === false) {
+                error_number = error_number + 1;
+            }
+        }
+        if (error_number !== 0) {
+            if (error_number === 1) {
+                this.showError('There is 1 error.');
+            }
+            else {
+                this.showError('There are ' + error_number + ' errors.');
+            }
+            return false;
+        }
+        return true;
+    },
+
+
+    /**
      * Show the spinner, and hide the submit button.
      *
      * @method showSpinner
@@ -486,6 +551,7 @@
      * @method showError
      */
     showError: function(error) {
+        this.hideErrors();
         Y.Node.create('<p class="error message" />')
             .appendTo(this.get("contentBox"))
             .set("text", error);

=== modified file 'lib/lp/app/javascript/formwidgets/tests/test_formwidgets.js'
--- lib/lp/app/javascript/formwidgets/tests/test_formwidgets.js	2011-08-09 16:31:48 +0000
+++ lib/lp/app/javascript/formwidgets/tests/test_formwidgets.js	2011-10-18 07:37:29 +0000
@@ -159,10 +159,20 @@
         },
 
         testShowError: function() {
+            this.widget.render(this.container);
             this.widget.showError("Unrealistic expectations.");
             Assert.areSame(
                 "Unrealistic expectations.",
-                this.widget.fieldNode.one("p").get("text"));
+                this.widget.fieldNode.one("div.message").get("text"));
+            Assert.isTrue(this.container.one('tr').hasClass('error'));
+        },
+
+        testHideError: function() {
+            this.widget.render(this.container);
+            this.widget.showError("Unrealistic expectations.");
+            this.widget.hideError();
+            Assert.isNull(this.widget.fieldNode.one("div.message"));
+            Assert.isFalse(this.container.one('tr').hasClass('error'));
         }
 
     };
@@ -553,11 +563,75 @@
                 this.actions.one("p.error.message").get("text"));
         },
 
-        testHideErrors: function() {
-            this.widget.showError("The Man From U.N.C.L.E.");
-            this.widget.showError("The Woman From A.U.N.T.I.E.");
-            this.widget.hideErrors();
-            Assert.isNull(this.actions.one("p.error.message"));
+        testRegisterWidget: function() {
+            var fake_widget = 'Fake Widget';
+            Assert.areEqual(0, this.widget._widgets.length);
+            this.widget.registerWidget(fake_widget);
+            Assert.areEqual(1, this.widget._widgets.length);
+            Assert.areSame(
+                "Fake Widget",
+                this.widget._widgets[0]);
+        },
+
+        createMockWidget: function(return_value) {
+            var mockWidget = Y.Mock();
+            Y.Mock.expect(mockWidget, {
+                method: "validate",
+                returns: return_value
+            });
+            return mockWidget;
+        },
+
+        testValidateCallsValidateOnWidget: function() {
+            var widget = this.createMockWidget(true);
+            this.widget.registerWidget(widget);
+            var result = this.widget.validate();
+            Y.Mock.verify(widget);
+            Assert.isTrue(result);
+        },
+
+        testValidateCallsValidateOnWidgets: function() {
+            var widget1 = this.createMockWidget(true);
+            var widget2 = this.createMockWidget(true);
+            this.widget.registerWidget(widget1);
+            this.widget.registerWidget(widget2);
+            var result = this.widget.validate();
+            Y.Mock.verify(widget1);
+            Y.Mock.verify(widget2);
+            Assert.isTrue(result);
+            Assert.isNull(this.widget.get("contentBox").one('.message'));
+        },
+
+        testValidateOneWidgetFails: function() {
+            this.widget.render(this.container);
+            var widget1 = this.createMockWidget(true);
+            var widget2 = this.createMockWidget(false);
+            this.widget.registerWidget(widget1);
+            this.widget.registerWidget(widget2);
+            var result = this.widget.validate();
+            Y.Mock.verify(widget1);
+            Y.Mock.verify(widget2);
+            Assert.isFalse(result);
+            Y.log(this.widget.fieldNode);
+            Assert.areEqual(
+                'There is 1 error.',
+                this.widget.get("contentBox").one('.message').get('text'));
+        },
+
+        testValidateManyWidgetsFail: function() {
+            this.widget.render(this.container);
+            var widget1 = this.createMockWidget(false);
+            var widget2 = this.createMockWidget(false);
+            this.widget.registerWidget(widget1);
+            this.widget.registerWidget(widget2);
+            var result = this.widget.validate();
+            Y.Mock.verify(widget1);
+            Y.Mock.verify(widget2);
+            Assert.isFalse(result);
+            Y.log(this.widget.fieldNode);
+            Assert.areEqual(
+                'There are 2 errors.',
+                this.widget.get("contentBox").one('.message').get('text'));
         }
 
     };

=== modified file 'lib/lp/registry/javascript/distroseries/initseries.js'
--- lib/lp/registry/javascript/distroseries/initseries.js	2011-08-22 14:03:02 +0000
+++ lib/lp/registry/javascript/distroseries/initseries.js	2011-10-18 07:37:29 +0000
@@ -42,9 +42,13 @@
     initializer: function(config) {
         this.context = config.context;
         this.deriveFromChoices = config.deriveFromChoices;
+        this.registerWidget(this.deriveFromChoices);
         this.architectureChoice = config.architectureChoice;
+        this.registerWidget(this.architectureChoice);
         this.packagesetChoice = config.packagesetChoice;
+        this.registerWidget(this.packagesetChoice);
         this.packageCopyOptions = config.packageCopyOptions;
+        this.registerWidget(this.packageCopyOptions);
         this.form_container = config.form_container;
     },
 
@@ -70,6 +74,18 @@
     },
 
     /**
+     * Validate all the widgets and call submit as appropriate.
+     *
+     * @method submit
+     */
+    check_and_submit: function() {
+        var check = this.validate();
+        if (this.validate()) {
+            this.submit();
+        }
+    },
+
+    /**
      * Call deriveDistroSeries via the API.
      *
      * @method submit
@@ -299,10 +315,10 @@
             form_actions.deriveFromChoices);
     }
 
-    // Wire up the form submission.
+    // Wire up the form check and submission.
     form_actions.form_container.one("form").on(
         "submit", function(event) {
-            event.halt(); form_actions.submit(); });
+            event.halt(); form_actions.check_and_submit(); });
 
     // Show the form.
     form_actions.form_container.removeClass("unseen");

=== modified file 'lib/lp/registry/javascript/distroseries/tests/test_widgets.js'
--- lib/lp/registry/javascript/distroseries/tests/test_widgets.js	2011-08-09 14:32:42 +0000
+++ lib/lp/registry/javascript/distroseries/tests/test_widgets.js	2011-10-18 07:37:29 +0000
@@ -190,6 +190,72 @@
                 attrselect("value")(this.widget.get("choices")));
         },
 
+        test_populate_archindep_tags_InitiatesIO: function() {
+            var distroseries = {api_uri: "ubuntu/hoary", value: 3};
+            var io = false;
+            this.widget.client = {
+                get: function(path, config) {
+                    io = true;
+                    Assert.areEqual(
+                        "/api/devel/ubuntu/hoary/nominatedarchindep",
+                        path);
+                    Assert.isObject(config.on);
+                    Assert.isFunction(config.on.success);
+                    Assert.isFunction(config.on.failure);
+                }
+            };
+            this.widget._populate_archindep_tags(distroseries);
+            Assert.isTrue(io, "No IO initiated.");
+        },
+
+        test_populate_archindep_tags: function() {
+            var distroseries = {api_uri: "ubuntu/hoary", value: "3"};
+            this.widget.client = {
+                get: function(path, config) {
+                    var nominatedarchindep = {
+                        get: function(key) {
+                            Assert.areEqual("architecture_tag", key);
+                            return 'i386';
+                        }
+                    };
+                    config.on.success(nominatedarchindep);
+                }
+            };
+            this.widget._populate_archindep_tags(distroseries);
+            Assert.areEqual("i386", this.widget._archindep_tags["3"]);
+        },
+
+        testValidateOk: function() {
+            this.widget.render(this.container);
+            this.widget.add_choices(['i386', 'hppa']);
+            this.widget.set('choice', ['i386']);
+            this.widget._archindep_tags['3'] = 'i386';
+            var res = this.widget.validate();
+            Assert.isTrue(res);
+        },
+
+        testValidateOkEmpty: function() {
+            this.widget.render(this.container);
+            this.widget.add_choices(['i386', 'hppa']);
+            this.widget.set('choice', []);
+            this.widget._archindep_tags['3'] = 'i386';
+            var res = this.widget.validate();
+            Assert.isTrue(res);
+        },
+
+        testValidateFails: function() {
+            this.widget.render(this.container);
+            this.widget.add_choices(['i386', 'hppa']);
+            this.widget.set('choice', ['i386']);
+            this.widget._archindep_tags['3'] = 'hppa';
+            var res = this.widget.validate();
+            Assert.isFalse(res);
+            Assert.areEqual(
+                "The distroseries has no architectures selected to "
+                + "build architecture independent binaries.",
+                this.widget.fieldNode.one('.message').get('text'));
+        },
+
         testAddDistroSeriesInitiatesIO: function() {
             var io = false;
             this.widget.client = {
@@ -240,6 +306,7 @@
             ArrayAssert.itemsAreEqual(
                 ["amd64", "i386"],
                 values(this.widget.get("choices")));
+            Assert.isUndefined(this.widget._archindep_tags["4"]);
         },
 
         testSetDistroSeriesError: function() {

=== modified file 'lib/lp/registry/javascript/distroseries/widgets.js'
--- lib/lp/registry/javascript/distroseries/widgets.js	2011-08-09 16:19:19 +0000
+++ lib/lp/registry/javascript/distroseries/widgets.js	2011-10-18 07:37:29 +0000
@@ -347,6 +347,7 @@
         this.error_handler.clearProgressUI = Y.bind(this.hideSpinner, this);
         this.error_handler.showError = Y.bind(this.showError, this);
         this._distroseries = {};
+        this._archindep_tags = {};
         this.clean_display();
     },
 
@@ -377,6 +378,7 @@
         var on = {
             success: function (results) {
                 self.add_distroarchseries(distroseries_id, results);
+                self._populate_archindep_tags(distroseries);
             },
             failure: this.error_handler.getFailureHandler()
         };
@@ -384,12 +386,69 @@
      },
 
     /**
+     * Given a newly added distroseries object, populate the
+     * _archindep_tags field with it's architecture independent arch_tag.
+     *
+     * @method _populate_archindep_tags
+     * @param {Object} distroseries The distroseries object
+     *     ({value:distroseries_id, api_uri:distroseries_uri}).
+     */
+    _populate_archindep_tags: function(distroseries) {
+        var path = Y.lp.client.get_field_uri(
+            distroseries.api_uri, 'nominatedarchindep');
+        var self = this;
+        var on = {
+            success: function (nominatedarchindep) {
+                var arch_tag = nominatedarchindep.get('architecture_tag');
+                self._archindep_tags[distroseries.value] = arch_tag;
+            },
+            failure: this.error_handler.getFailureHandler()
+        };
+        this.client.get(path, {on: on});
+    },
+
+    /**
+     * Validate the architectures choice: make sure that at least one
+     * architecture is suitable to build architecture independent packages.
+     *
+     * @method validate
+     */
+    validate: function() {
+        this.hideError();
+        var choice_objects = this.get('choice');
+        var choices = choice_objects.map(
+            function(choice_object) { return choice_object.value; });
+        if (choice_objects.length === 0) {
+            // None architectures selected implies that all the architectures
+            // are copied over so we're sure that the arch-indep arch from one
+            // of the parents is among them.
+            return true;
+        }
+        var ds, choice;
+        for (ds in this._archindep_tags) {
+            var arch_tag = this._archindep_tags[ds];
+            var i = 0;
+            for (i; i<choices.length; i++) {
+                if (Y.Lang.isValue(choices[i]) && choices[i] === arch_tag) {
+                    return true;
+                }
+            }
+        }
+        this.showError(
+            ["The distroseries has no architectures selected to ",
+             "build architecture independent binaries."].join(''));
+        return false;
+    },
+
+    /**
      * Remove a parent distroseries, remove the architectures only
      * present in this parent series from the possible choices.
      *
      * @method remove_distroseries
      */
     remove_distroseries: function(distroseries_id) {
+        // Cleanup the corresponding _archindep_tags' entry.
+        delete this._archindep_tags[distroseries_id];
         // Compute which das is only in the distroseries to be removed.
         var arch_to_remove = [];
         var das = this._distroseries[distroseries_id];