launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05202
[Merge] lp:~rvb/launchpad/bug-869221-ui into lp:launchpad
Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/bug-869221-ui into lp:launchpad with lp:~rvb/launchpad/bug-869221-expose-nominatedarchindep as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #869221 in Launchpad itself: "Initializing a distroseries that doesn't use the parent's archindep arch will fail"
https://bugs.launchpad.net/launchpad/+bug/869221
For more details, see:
https://code.launchpad.net/~rvb/launchpad/bug-869221-ui/+merge/78802
This branch is the final branch to fix #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.
--
https://code.launchpad.net/~rvb/launchpad/bug-869221-ui/+merge/78802
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/bug-869221-ui into lp:launchpad.
=== 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-10 08:23:27 +0000
@@ -158,9 +158,34 @@
* @method showError
*/
showError: function(error) {
- var message = Y.Node.create('<p />').set("text", error);
- this.fieldNode.empty().append(message);
+ var message = Y.Node.create('<div />')
+ .addClass('message')
+ .set("text", error);
+ this.fieldNode.append(message);
+ this.fieldNode.ancestor('tr').addClass('error');
Y.lp.anim.red_flash({node: 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 +483,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 +550,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-10 08:23:27 +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-10 08:23:27 +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 (check === true) {
+ 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-10 08:23:27 +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."].join(''),
+ 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-10 08:23:27 +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];
