launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06165
[Merge] lp:~wallyworld/launchpad/chained-personpicker-validators into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/chained-personpicker-validators into lp:launchpad with lp:~wallyworld/launchpad/better-review-request-330290 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/chained-personpicker-validators/+merge/89861
== Implementation ==
Part 2 of work to support making source and target branches visible to reviewers.
This branches add infrastructure. The next branch will add the specific code to assigning mp reviewers to use the infrastructure.
We need to warn the user and confirm that they really want to grant visibility to private branches when a reviewer is picked. We already have a picker validation/confirmation callback, but it's only the one and is set in the picker config. We need to allow more than one confirmation prompt now, since we are adding a new potential question to ask the user. The current implementation also requires access to the javascript used to setup the picker and this is problematic.
This branch does two things:
1. Allows more than one confirmation prompt to be specified for a picker. The prompts are asked one after the other, and if No is answered at any time, the user is returned to the picker selection.
2. Uses a YUI namespace for specifying the confirmation callbacks to be used for a given picker widget id. These can be set up separately to the widget itself, which is done as part of the lazr widget stuff.
Core work includes:
- add chained_validator method to picker_patcher
- get all available confirmation/validation callbacks from the Y.app.picker.validation namespace and also picker config and pass these to the chained_validator method
A bug was found with the existing implementation - when a picker is created and associated with a text field, the selected value is put into the text field as soon as it is picked rather than waiting for any validators to be used. This is because the 'save' event is used. So a new 'value_confirmed' event is fired when the picker save is run after all validation passes and this event is used to update the text field.
== Tests ==
Rework the picker_patcher tests to test that the chained validation stuff works. Add tests specifically for multiple validators where all pass and only one passes.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/picker/picker.js
lib/lp/app/javascript/picker/picker_patcher.js
lib/lp/app/javascript/picker/tests/test_picker_patcher.js
lib/lp/app/widgets/popup.py
lib/lp/app/widgets/templates/form-picker-macros.pt
--
https://code.launchpad.net/~wallyworld/launchpad/chained-personpicker-validators/+merge/89861
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/chained-personpicker-validators into lp:launchpad.
=== modified file 'lib/lp/app/javascript/picker/picker.js'
--- lib/lp/app/javascript/picker/picker.js 2011-09-26 06:30:07 +0000
+++ lib/lp/app/javascript/picker/picker.js 2012-01-24 12:00:33 +0000
@@ -977,12 +977,15 @@
* have it not cleared by setting clear_on_save to 'false'. The search
* entered by the user is passed in the first details attribute of the
* event.
+ * We also fire a 'value_confirmed' event which means that the selected
+ * value has been accepted as valid.
*
* @method _defaultSave
* @param e {Event.Facade} An Event Facade object.
* @protected
*/
_defaultSave : function(e) {
+ this.fire('value_confirmed', e);
this.hide();
if ( this.get('clear_on_save') ) {
this._clear();
@@ -1260,7 +1263,7 @@
Y.extend(TextFieldPickerPlugin, Y.Plugin.Base, {
initializer: function(config) {
var input = Y.one(config.input_element);
- this.doAfter('save', function (e) {
+ this.doAfter('value_confirmed', function (e) {
var result = e.details[Picker.SAVE_RESULT];
this.get('host').set(SELECTED_VALUE_METADATA, result.metadata);
this.get('host').set(SELECTED_VALUE, result.value);
=== modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
--- lib/lp/app/javascript/picker/picker_patcher.js 2011-12-21 07:54:40 +0000
+++ lib/lp/app/javascript/picker/picker_patcher.js 2012-01-24 12:00:33 +0000
@@ -164,6 +164,13 @@
namespace.yesno_save_confirmation = function(
picker, content, yes_label, no_label, yes_fn, no_fn) {
+ // If this is being called as part of a chain of validators, we need to
+ // first remove any existing validation node.
+ var existing_validation = picker.get('contentBox').one('.validation-node');
+ if (existing_validation !== null) {
+ existing_validation.remove(true);
+ }
+
var node = Y.Node.create(
['<div class="validation-node">',
'<div class="step-on" style="width: 100%;"></div>',
@@ -181,7 +188,6 @@
if (Y.Lang.isFunction(callback_fn) ) {
callback_fn();
}
- reset_form(picker);
};
node.one(".yes_button")
.set('text', yes_label)
@@ -204,7 +210,7 @@
picker, picker_result, do_save, do_cancel) {
var client = new Y.lp.client.Launchpad();
- config = {
+ var config = {
on: {
success: function (person) {
var private_person = person.get('private');
@@ -285,6 +291,33 @@
}
};
+
+/**
+ * Run the validators in order and perform the do_save callback only if all
+ * validators are successful.
+ * @param validators the array of validators to run
+ * @param picker the picker
+ * @param picker_result the selected picker value
+ * @param do_save the save function if all validators pass
+ * @param do_cancel the cancel function if any validator fails
+ */
+namespace._chained_validator = function(
+ validators, picker, picker_result, do_save, do_cancel) {
+ if (validators.length === 0) {
+ do_save();
+ reset_form(picker);
+ return;
+ }
+ var validator_callback = validators.pop();
+ validator_callback(picker, picker_result,
+ function() {
+ namespace._chained_validator(validators, picker, picker_result,
+ do_save, do_cancel);
+ },
+ do_cancel);
+};
+
+
/**
* Creates a picker widget that has already been rendered and hidden.
*
@@ -304,7 +337,7 @@
* picker.
* @param {Object} vocabulary_filters Optional List of filters which are
* supported by the vocabulary. Filter objects are a
- * dict of name, title, description values.
+ * dict of name, title, description values.
*
*/
namespace.create = function (vocabulary, config, associated_field_id,
@@ -384,13 +417,34 @@
}
picker._defaultSave(e);
};
+ // We now run any validators. There may be more than one specified, in
+ // which case they are chained. The picker result is only saved if all
+ // validators pass. Validators are run in this order (if defined):
+ // - Y.lp.app.picker.validation[widget_id], either a single or an
+ // array of validators for widget_id
+ // - config.validate_callback
+ var validation_callbacks = [];
var validate_callback = config.validate_callback;
if (Y.Lang.isFunction(validate_callback)) {
- validate_callback(
- picker, picker_result, do_save, undefined);
- } else {
- do_save();
- }
+ validation_callbacks.push(validate_callback);
+ }
+ var picker_validation = Y.namespace('lp.app.picker.validation');
+ var other_validate_callbacks =
+ picker_validation[config.show_widget_id];
+ if (Y.Lang.isArray(other_validate_callbacks)) {
+ other_validate_callbacks.forEach(function(callback) {
+ validation_callbacks.push(callback)
+ });
+ } else if (Y.Lang.isFunction(other_validate_callbacks)) {
+ validation_callbacks.push(other_validate_callbacks);
+ }
+ namespace._chained_validator(
+ validation_callbacks, picker, picker_result,
+ do_save,
+ function() {
+ reset_form(picker);
+ }
+ );
});
picker.subscribe('cancel', function (e) {
=== modified file 'lib/lp/app/javascript/picker/tests/test_picker_patcher.js'
--- lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2011-12-15 18:57:05 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2012-01-24 12:00:33 +0000
@@ -54,6 +54,9 @@
this.picker = null;
this.text_input = null;
this.select_menu = null;
+ this.saved_picker_value = null;
+ this.validation_namespace
+ = Y.namespace('lp.app.picker.validation');
},
tearDown: function() {
@@ -66,6 +69,7 @@
if (this.picker !== null) {
cleanup_widget(this.picker);
}
+ this.validation_namespace['show_picker_id'] = undefined;
},
create_picker: function(validate_callback, extra_config) {
@@ -73,6 +77,7 @@
"step_title": "Choose someone",
"header": "Pick Someone",
"null_display_value": "Noone",
+ "show_widget_id": "show_picker_id",
"validate_callback": validate_callback
};
if (extra_config !== undefined) {
@@ -84,6 +89,11 @@
"test_link",
"picker_id",
config);
+ var self = this;
+ this.picker.subscribe('value_confirmed', function(e) {
+ self.saved_picker_value =
+ e.details[Y.lazr.picker.Picker.SAVE_RESULT].api_uri;
+ });
},
create_picker_direct: function(associated_field) {
@@ -91,6 +101,11 @@
this.vocabulary,
undefined,
associated_field);
+ var self = this;
+ this.picker.subscribe('value_confirmed', function(e) {
+ self.saved_picker_value =
+ e.details[Y.lazr.picker.Picker.SAVE_RESULT].api_uri;
+ });
},
test_picker_can_be_instantiated: function() {
@@ -101,18 +116,10 @@
"Picker failed to be instantiated");
},
- // Called when the picker saves it's data. Sets a flag for checking.
- picker_save_callback: function(save_flag) {
- return function(e) {
- save_flag.event_has_fired = true;
- };
- },
-
// A validation callback stub. Instead of going to the server to see if
// a picker value requires confirmation, we compare it to a known value.
- yesno_validate_callback: function(save_flag, expected_value) {
- var save_fn = this.picker_save_callback(save_flag);
- return function(picker, value, ignore, cancel_fn) {
+ yesno_validate_callback: function(expected_value) {
+ return function(picker, value, save_fn, cancel_fn) {
Assert.areEqual(
expected_value, value.api_uri, "unexpected picker value");
if (value === null) {
@@ -133,15 +140,14 @@
test_no_confirmation_required: function() {
// The picker saves the selected value if no confirmation
// is required.
- var save_flag = {};
- this.create_picker(this.yesno_validate_callback(save_flag, "~/fred"));
+ this.create_picker(this.yesno_validate_callback("~/fred"));
this.picker.set('results', this.vocabulary);
this.picker.render();
simulate(
this.picker.get('boundingBox').one('.yui3-picker-results'),
'li:nth-child(1)', 'click');
- Assert.isTrue(save_flag.event_has_fired, "save event wasn't fired.");
+ Assert.areEqual('~/fred', this.saved_picker_value);
},
test_TextFieldPickerPlugin_selected_item_is_saved: function () {
@@ -149,7 +155,7 @@
// textfield if one is defined.
this.text_input = Y.Node.create(
'<input id="field.testfield" value="foo" />');
- node = Y.one(document.body).appendChild(this.text_input);
+ Y.one(document.body).appendChild(this.text_input);
this.create_picker_direct('field.testfield');
this.picker.set('results', this.vocabulary);
this.picker.render();
@@ -200,41 +206,103 @@
test_confirmation_yes: function() {
// The picker saves the selected value if the user answers
// "Yes" to a confirmation request.
- var save_flag = {};
- this.create_picker(
- this.yesno_validate_callback(save_flag, "~/frieda"));
- this.picker.set('results', this.vocabulary);
- this.picker.render();
-
- simulate(
- this.picker.get('boundingBox').one('.yui3-picker-results'),
- 'li:nth-child(2)', 'click');
- var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
-
- simulate(
- yesno, 'button:nth-child(1)', 'click');
- Assert.isTrue(
- save_flag.event_has_fired, "save event wasn't fired.");
+ // The validator is specified using the picker's config object.
+ this.create_picker(this.yesno_validate_callback("~/frieda"));
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+
+ simulate(
+ this.picker.get('boundingBox').one('.yui3-picker-results'),
+ 'li:nth-child(2)', 'click');
+ var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+
+ simulate(
+ yesno, 'button:nth-child(1)', 'click');
+ Assert.areEqual('~/frieda', this.saved_picker_value);
+ },
+
+ test_confirmation_yes_namespace_validator: function() {
+ // The picker saves the selected value if the user answers
+ // "Yes" to a confirmation request.
+ // The validator is specified using the validation namespace.
+ this.validation_namespace['show_picker_id'] =
+ [this.yesno_validate_callback("~/frieda")];
+
+ this.create_picker();
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+
+ simulate(
+ this.picker.get('boundingBox').one('.yui3-picker-results'),
+ 'li:nth-child(2)', 'click');
+ var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+
+ simulate(
+ yesno, 'button:nth-child(1)', 'click');
+ Assert.areEqual('~/frieda', this.saved_picker_value);
},
test_confirmation_no: function() {
- // The picker doesn't save the selected value if the answers
+ // The picker doesn't save the selected value if the user answers
// "No" to a confirmation request.
- var save_flag = {};
- this.create_picker(
- this.yesno_validate_callback(save_flag, "~/frieda"));
- this.picker.set('results', this.vocabulary);
- this.picker.render();
-
- simulate(
- this.picker.get('boundingBox').one('.yui3-picker-results'),
- 'li:nth-child(2)', 'click');
- var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
- simulate(
- yesno, 'button:nth-child(2)', 'click');
- Assert.areEqual(
- undefined, save_flag.event_has_fired,
- "save event wasn't fired.");
+ this.create_picker(this.yesno_validate_callback("~/frieda"));
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+
+ simulate(
+ this.picker.get('boundingBox').one('.yui3-picker-results'),
+ 'li:nth-child(2)', 'click');
+ var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+ simulate(
+ yesno, 'button:nth-child(2)', 'click');
+ Assert.isNull(this.saved_picker_value);
+ },
+
+ test_chained_validators_all_yes: function() {
+ // The picker saves the selected value if the user answers
+ // "Yes" to all confirmation request.
+
+ // Set up a picker with 2 validators, one via the config and one via
+ // the namespace.
+ this.validation_namespace['show_picker_id'] =
+ this.yesno_validate_callback("~/frieda");
+ this.create_picker(this.yesno_validate_callback("~/frieda"));
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+
+ simulate(
+ this.picker.get('boundingBox').one('.yui3-picker-results'),
+ 'li:nth-child(2)', 'click');
+ var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+ // Click the Yes button.
+ simulate(yesno, 'button:nth-child(1)', 'click');
+ yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+ // Click the Yes button.
+ simulate(yesno, 'button:nth-child(1)', 'click');
+ Assert.areEqual('~/frieda', this.saved_picker_value);
+ },
+
+ test_chained_validators_one_no: function() {
+ // The picker doesn't save the selected value if the user answers
+ // "No" to any one of the confirmation requests.
+ this.validation_namespace['show_picker_id'] = [
+ this.yesno_validate_callback("~/frieda"),
+ this.yesno_validate_callback("~/frieda")];
+
+ this.create_picker();
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+
+ simulate(
+ this.picker.get('boundingBox').one('.yui3-picker-results'),
+ 'li:nth-child(2)', 'click');
+ var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+ // Click the Yes button.
+ simulate(yesno, 'button:nth-child(1)', 'click');
+ yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+ // Click the No button.
+ simulate(yesno, 'button:nth-child(2)', 'click');
+ Assert.isNull(this.saved_picker_value);
},
test_connect_select_menu: function() {
@@ -286,7 +354,7 @@
simulate(
this.picker.get('boundingBox').one('.yui3-picker-results'),
- 'li:nth-child(1)', 'click');
+ 'li:nth-child(2)', 'click');
// expected_text is a little weird since breaks between p tags and
// buttons are lost.
var expected_text = 'This action will reveal this team\'s name to ' +
=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py 2012-01-01 02:58:52 +0000
+++ lib/lp/app/widgets/popup.py 2012-01-24 12:00:33 +0000
@@ -140,7 +140,8 @@
show_assign_me_button=self.show_assign_me_button,
vocabulary_name=self.vocabulary_name,
vocabulary_filters=self.vocabulary_filters,
- input_element=self.input_id)
+ input_element=self.input_id,
+ show_widget_id=self.show_widget_id)
@property
def json_config(self):
=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt 2011-10-31 08:09:56 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt 2012-01-24 12:00:33 +0000
@@ -40,7 +40,7 @@
var vocabulary = config.vocabulary_name;
var vocabulary_filters = config.vocabulary_filters;
var input_element = config.input_element;
- var show_widget_id = '${view/show_widget_id}';
+ var show_widget_id =config.show_widget_id;
var namespace = Y.namespace('lp.app.picker.connect');
namespace[show_widget_id] = function() {
// Sort out the Choose... link.
