launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03781
[Merge] lp:~henninge/launchpad/bug-778129-person-picker into lp:launchpad
Henning Eggers has proposed merging lp:~henninge/launchpad/bug-778129-person-picker into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #778129 in Launchpad itself: "cannot change owner for recipe - picker shows 'undefined'"
https://bugs.launchpad.net/launchpad/+bug/778129
For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-778129-person-picker/+merge/62894
= Summary =
Lifeless could not use the owner picker for a SourcePackageRecipe
because it contained more than 120 choices. The picker is restricted
to 20 batches of 6 entries and displays a warning to "narrow down the
search" if more are requested to be displayed. Unfortunately that is
only possible for vocabularies that implement "IHugeVocabulary" which
is not the case for the "UserTeamsParticipationPlusSelf" vocabulary
used here.
== Proposed fix ==
The real fix is to implement IHugeVocabulary on this vocabulary
(bug 790253) while at the same time fixing the widget to not show the
search box for vocabularies under a certain size (bug 790255). I tried
to do that at first but found out that that is beyond the scope of this
bug so I went with the short-term solution and simply turned the
batch limit off if no search box is available. This may cause disorder
in the UI for large batch numbers but will still be usable.
== Pre-implementation notes ==
I talked to Robert about this and we agreed on the short-term/long-term
split for the fix.
== Implementation details ==
Quite straight forward. I only wish the batch limit could be configured
somewhere more obvious.
== Tests ==
lib/lp/app/javascript/tests/test_picker.html
== Demo and Q/A ==
Ask Robert to open the owner picker on a SourcePackageRecipe. He should
see 21 batches. (Alternatively make sure you become a member of 120
teams .. ;-)
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/picker.js
lib/lp/app/javascript/tests/test_picker.js
./lib/lp/app/javascript/picker.js
91: Expected '!==' and instead saw '!='.
220: Expected '!==' and instead saw '!='.
294: Expected '!==' and instead saw '!='.
342: Expected '!==' and instead saw '!='.
342: Expected '!==' and instead saw '!='.
414: Move 'var' declarations to the top of the function.
414: Stopping. (85% scanned).
-1: JSLINT had a fatal error.
--
https://code.launchpad.net/~henninge/launchpad/bug-778129-person-picker/+merge/62894
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-778129-person-picker into lp:launchpad.
=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js 2011-04-20 06:29:11 +0000
+++ lib/lp/app/javascript/picker.js 2011-05-30 15:26:31 +0000
@@ -188,7 +188,8 @@
config.temp_spinner.removeClass('unseen');
picker.set('min_search_chars', 0);
picker.fire('search', '');
- picker.get('contentBox').one('.yui3-picker-search-box').addClass('unseen');
+ picker.get('contentBox').one(
+ '.yui3-picker-search-box').addClass('unseen');
}
picker.show();
});
@@ -362,7 +363,7 @@
// the form. We want to do this ourselves after any validation has had a
// chance to be performed.
picker.publish('save', { defaultFn: function(){} } );
-
+
picker.subscribe('save', function (e) {
Y.log('Got save event.');
var picker_result = e.details[Y.lazr.Picker.SAVE_RESULT];
@@ -394,7 +395,8 @@
var selected_batch = e.details[1] || 0;
var start = BATCH_SIZE * selected_batch;
var display_vocabulary = function(results, total_size, start) {
- if (total_size > (MAX_BATCHES * BATCH_SIZE)) {
+ var max_size = MAX_BATCHES * BATCH_SIZE;
+ if (config.show_search_box && total_size > max_size) {
picker.set('error',
'Too many matches. Please try to narrow your search.');
// Display a single empty result item so that the picker
@@ -444,8 +446,9 @@
var uri = '';
if (Y.Lang.isValue(config['context'])){
- uri += Y.lp.get_url_path(config['context'].get('web_link')) + '/';
- }
+ uri += Y.lp.get_url_path(
+ config['context'].get('web_link')) + '/';
+ }
uri += '@@+huge-vocabulary?' + qs;
Y.io(uri, {
=== modified file 'lib/lp/app/javascript/tests/test_picker.js'
--- lib/lp/app/javascript/tests/test_picker.js 2011-04-15 08:16:32 +0000
+++ lib/lp/app/javascript/tests/test_picker.js 2011-05-30 15:26:31 +0000
@@ -69,7 +69,7 @@
},
test_picker_can_be_instantiated: function() {
- // Ensure the picker can be instantiated.
+ // The picker can be instantiated.
this.create_picker();
Assert.isInstanceOf(
Y.lazr.Picker, this.picker, "Picker failed to be instantiated");
@@ -105,7 +105,7 @@
},
test_no_confirmation_required: function() {
- // Test that the picker saves the selected value if no confirmation
+ // The picker saves the selected value if no confirmation
// is required.
var save_flag = {};
this.create_picker(this.yesno_validate_callback(save_flag, "~/fred"));
@@ -119,7 +119,7 @@
},
test_TextFieldPickerPlugin_selected_item_is_saved: function () {
- // Test that the picker saves the selected value to its associated
+ // The picker saves the selected value to its associated
// textfield if one is defined.
var search_input = Y.Node.create(
'<input id="field.initval" value="foo" />');
@@ -142,7 +142,7 @@
},
test_confirmation_yes: function() {
- // Test that the picker saves the selected value if the user answers
+ // The picker saves the selected value if the user answers
// "Yes" to a confirmation request.
var save_flag = {};
this.create_picker(
@@ -162,7 +162,7 @@
},
test_confirmation_no: function() {
- // Test that the picker doesn't save the selected value if the answers
+ // The picker doesn't save the selected value if the answers
// "No" to a confirmation request.
var save_flag = {};
this.create_picker(
@@ -182,6 +182,65 @@
}
}));
+/*
+ * Test cases for a picker with a large vocabulary.
+ */
+suite.add(new Y.Test.Case({
+
+ name: 'picker_large_vocabulary',
+
+ setUp: function() {
+ var i;
+ this.vocabulary = new Array(121);
+ for (i = 0; i < 121; i++) {
+ this.vocabulary[i] = {
+ "value": "value-" + i,
+ "title": "title-" + i,
+ "css": "sprite-person",
+ "description": "description-" + i,
+ "api_uri": "~/fred-" + i};
+ }
+ },
+
+ tearDown: function() {
+ cleanup_widget(this.picker);
+ },
+
+ create_picker: function(show_search_box) {
+ this.picker = Y.lp.app.picker.addPickerPatcher(
+ this.vocabulary,
+ "foo/bar",
+ "test_link",
+ "picker_id",
+ {"step_title": "Choose someone",
+ "header": "Pick Someone",
+ "show_search_box": show_search_box,
+ "validate_callback": null});
+ },
+
+ test_picker_displays_warning: function() {
+ // With a search box the picker will refuse to display more than
+ // 120 values.
+ this.create_picker(true);
+ this.picker.set('min_search_chars', 0);
+ this.picker.fire('search', '');
+ Assert.areEqual(
+ 'Too many matches. Please try to narrow your search.',
+ this.picker.get('error'));
+ },
+
+ test_picker_no_warning: function() {
+ // Without a search box the picker will also display more than
+ // 120 values.
+ this.create_picker(false);
+ this.picker.set('min_search_chars', 0);
+ this.picker.fire('search', '');
+ Assert.areEqual(null, this.picker.get('error'));
+ }
+
+}));
+
+
// Lock, stock, and two smoking barrels.
var handle_complete = function(data) {
status_node = Y.Node.create(
