launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04297
[Merge] lp:~wallyworld/launchpad/picker-displays-null-812253 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/picker-displays-null-812253 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #812253 in Launchpad itself: "Person picker displays "null" on second use"
https://bugs.launchpad.net/launchpad/+bug/812253
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/picker-displays-null-812253/+merge/68318
The picker popup as created by the Choose... link was being given configuration which was sent through json.dumps() twice, messing up some logic in the Javascript. The result was the "null" was being displayed in the picker if it was opened up again after initially selecting an item.
== Implementation ==
A fair bit of yak shaving. The form-picker-macros.pt template had embedded javascript to hook up the associated text field for the Choose... link. This was moved to picker_patcher.js (Javascript should live in js files where possible). Plus, there was code to hook up a "beforeResults" handler, used to display the "extra_no_results_message"; again why have it in a tales file when it should be in Javascript. This change also means than inline pickers (like used for bug assignee, recipe owner etc) can now benefit from having this message available for use.
There was also a driveby where a ".yui3-picker-footer-slot" selector was being used to insert a node into the person picker when the _footer_slot node should have been used instead.
The net result is the bug is fixed and there's less Javascript in tales files and the picker gets to use some functionality that was previously only available under one use case.
== Tests ==
Run existing yui tests, plus rework the test_widget_template_properties test in test_popup.py
Also test the set branch reviewer and bug task assignee functionality.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/picker/person_picker.js
lib/lp/app/javascript/picker/picker_patcher.js
lib/lp/app/widgets/popup.py
lib/lp/app/widgets/templates/form-picker-macros.pt
lib/lp/app/widgets/tests/test_popup.py
./lib/lp/app/javascript/picker/person_picker.js
13: 'PersonPicker' has not been fully defined yet.
./lib/lp/app/javascript/picker/picker_patcher.js
103: Expected '===' and instead saw '=='.
405: Expected '!==' and instead saw '!='.
438: Expected '!==' and instead saw '!='.
The !==, === warnings above are bogus imho since the implicit conversion is valid in those cases.
--
https://code.launchpad.net/~wallyworld/launchpad/picker-displays-null-812253/+merge/68318
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/picker-displays-null-812253 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/picker/person_picker.js'
--- lib/lp/app/javascript/picker/person_picker.js 2011-07-14 12:46:31 +0000
+++ lib/lp/app/javascript/picker/person_picker.js 2011-07-19 03:43:29 +0000
@@ -9,8 +9,6 @@
/*
* Extend the picker into the PersonPicker
*/
-var footer_label = ".yui3-picker-footer-slot";
-
var PersonPicker = function() {
PersonPicker.superclass.constructor.apply(this, arguments);
this._extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
@@ -99,8 +97,7 @@
syncUI: function() {
// call Picker's sync
this.constructor.superclass.syncUI.apply(this, arguments);
- footer_slot = Y.one(footer_label);
- footer_slot.appendChild(this._extra_buttons);
+ this._footer_slot_box.appendChild(this._extra_buttons);
},
bindUI: function() {
=== modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
--- lib/lp/app/javascript/picker/picker_patcher.js 2011-07-18 05:32:40 +0000
+++ lib/lp/app/javascript/picker/picker_patcher.js 2011-07-19 03:43:29 +0000
@@ -344,8 +344,11 @@
* a single string argument.
* config.show_search_box: Should the search box be
* shown.
+ * @param {String} associated_field_id Optional Id of the text field to
+ * to be updated with the value selected by the
+ * picker.
*/
-namespace.create = function (vocabulary, config) {
+namespace.create = function (vocabulary, config, associated_field_id) {
if (Y.UA.ie) {
return;
}
@@ -370,6 +373,8 @@
if (config.picker_type !== undefined) {
picker_type = config.picker_type;
}
+ } else {
+ config = {};
}
if (typeof vocabulary !== 'string' && typeof vocabulary !== 'object') {
@@ -397,6 +402,10 @@
} else {
picker = new Y.lazr.picker.Picker(new_config);
}
+ if (associated_field_id != null) {
+ picker.plug(Y.lazr.picker.TextFieldPickerPlugin,
+ {input_element: '[id="'+associated_field_id+'"]'});
+ }
// We don't want the default save to fire since this hides
// the form. We want to do this ourselves after any validation has had a
@@ -426,6 +435,18 @@
reset_form(picker);
});
+ if (config.extra_no_results_message != null) {
+ picker.before('resultsChange', function (e) {
+ var new_results = e.details[0].newVal;
+ if (new_results.length === 0) {
+ picker.set('footer_slot',
+ Y.Node.create(config.extra_no_results_message));
+ } else {
+ picker.set('footer_slot', null);
+ }
+ });
+ }
+
// Search for results, create batches and update the display.
// in the widget.
var search_handler = function (e) {
=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py 2011-07-18 00:45:28 +0000
+++ lib/lp/app/widgets/popup.py 2011-07-19 03:43:29 +0000
@@ -138,7 +138,7 @@
:return: A string that will be passed to Y.Node.create()
so it needs to be contained in a single HTML element.
"""
- return simplejson.dumps(None)
+ return None
@property
def vocabulary_name(self):
@@ -157,11 +157,11 @@
@property
def header_text(self):
- return simplejson.dumps(self.header or self.vocabulary.displayname)
+ return self.header or self.vocabulary.displayname
@property
def step_title_text(self):
- return simplejson.dumps(self.step_title or self.vocabulary.step_title)
+ return self.step_title or self.vocabulary.step_title
@property
def input_id(self):
@@ -248,7 +248,7 @@
@property
def extra_no_results_message(self):
- return simplejson.dumps("<strong>Didn't find the project you were "
+ return ("<strong>Didn't find the project you were "
"looking for? "
'<a href="%s/+affects-new-product">Register it</a>.</strong>'
% canonical_url(self.context.context))
=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt 2011-07-14 12:44:48 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt 2011-07-19 03:43:29 +0000
@@ -30,31 +30,10 @@
<tal:chooseLink replace="structure view/chooseLink" />
<script tal:content="structure string:
- LPS.use('node', 'lp.app.picker', 'plugin', function(Y) {
+ LPS.use('node', 'lp.app.picker', function(Y) {
if (Y.UA.ie) {
return;
}
-
- // The vocabulary picker, created when used for the first time.
- function make_picker() {
- var config = ${view/json_config};
- var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
- if (config.extra_no_results_message !== null) {
- picker.before('resultsChange', function (e) {
- var new_results = e.details[0].newVal;
- if (new_results.length === 0) {
- picker.set('footer_slot',
- Y.Node.create(config.extra_no_results_message));
- }
- else {
- picker.set('footer_slot', null);
- }
- });
- }
- picker.plug(Y.lazr.picker.TextFieldPickerPlugin,
- {input_element: '[id=\x22${view/input_id}\x22]'});
- return picker;
- }
var picker = null;
Y.on('domready', function(e) {
// Sort out the Choose... link.
@@ -65,7 +44,10 @@
show_widget_node.get('parentNode').removeClass('unseen');
show_widget_node.on('click', function (e) {
if (picker === null) {
- picker = make_picker();
+ picker = Y.lp.app.picker.create(
+ '${view/vocabulary_name}',
+ ${view/json_config},
+ '${view/input_id}');
}
picker.show();
e.preventDefault();
=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py 2011-07-18 00:45:28 +0000
+++ lib/lp/app/widgets/tests/test_popup.py 2011-07-19 03:43:29 +0000
@@ -62,21 +62,32 @@
widget_config = simplejson.loads(picker_widget.json_config)
self.assertEqual(
'ValidTeamOwner', picker_widget.vocabulary_name)
- self.assertEqual(
- simplejson.dumps(self.vocabulary.displayname),
- widget_config['header'])
- self.assertEqual(
- simplejson.dumps(self.vocabulary.step_title),
+ self.assertEqual(self.vocabulary.displayname, widget_config['header'])
+ self.assertEqual(self.vocabulary.step_title,
widget_config['step_title'])
self.assertEqual(
'show-widget-field-test_valid-item', picker_widget.show_widget_id)
self.assertEqual(
'field.test_valid.item', picker_widget.input_id)
- self.assertEqual(
- simplejson.dumps(None), picker_widget.extra_no_results_message)
+ self.assertIsNone(picker_widget.extra_no_results_message)
markup = picker_widget()
- self.assertIn(
- "Y.lp.app.picker.create('ValidTeamOwner', config);", markup)
+ self.assertTextMatchesExpressionIgnoreWhitespace("""\
+ .*
+ picker = Y\\.lp\\.app\\.picker\\.create\\(
+ 'ValidTeamOwner',
+ {"show_remove_button": "false",
+ "show_assign_me_button": "false",
+ "assign_me_text": "Pick me",
+ "remove_team_text": "Remove team",
+ "extra_no_results_message": null,
+ "remove_person_text": "Remove person",
+ "header": "Select a Team or Person",
+ "selected_value_metadata": null,
+ "picker_type": "default",
+ "step_title": "Search"},
+ 'field\\.test_valid.item'\\);
+ .*
+ """, markup)
def test_widget_fieldname_with_invalid_html_chars(self):
# Check the picker widget is correctly set up for a field which has a
