launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/picker-truncate-large-results into lp:launchpad.

Commit message:
Truncate large picker search results rather than refusing to display anything.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #893796 in Launchpad itself: "Package picker: "too many matches" although searching for specific package"
  https://bugs.launchpad.net/launchpad/+bug/893796

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/picker-truncate-large-results/+merge/305528

Truncate large picker search results rather than refusing to display anything.

It really doesn't make much sense to show nothing but an error message when we have at least some perfectly good results, especially now that the package picker is moving towards doing a better job at showing more relevant matches first.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/picker-truncate-large-results into lp:launchpad.

=== modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
--- lib/lp/app/javascript/picker/picker_patcher.js	2016-07-21 15:13:12 +0000
+++ lib/lp/app/javascript/picker/picker_patcher.js	2016-09-12 19:55:47 +0000
@@ -534,29 +534,28 @@
         var display_vocabulary = function(results, total_size, start) {
             var max_size = MAX_BATCHES * BATCH_SIZE;
             if (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
-                // doesn't say that no items matched, which is contradictory.
-                picker.set('results', [{}]);
-                picker.set('batches', []);
-            } else {
-                picker.set('results', results);
+                picker.set(
+                    'error',
+                    'Only displaying first ' + max_size + ' of ' +
+                    total_size + ' matches.');
+                results.length = max_size;
+                total_size = max_size;
+            }
+            picker.set('results', results);
 
-                // Update the batches only if it's a new search.
-                if (e.details[1] === undefined) {
-                    var batches = [];
-                    var stop = Math.ceil(total_size / BATCH_SIZE);
-                    if (stop > 1) {
-                        for (batch = 0; batch < stop; batch++) {
-                            batches.push({
-                                    name: batch+1,
-                                    value: batch
-                                });
-                        }
+            // Update the batches only if it's a new search.
+            if (e.details[1] === undefined) {
+                var batches = [];
+                var stop = Math.ceil(total_size / BATCH_SIZE);
+                if (stop > 1) {
+                    for (batch = 0; batch < stop; batch++) {
+                        batches.push({
+                                name: batch+1,
+                                value: batch
+                            });
                     }
-                    picker.set('batches', batches);
                 }
+                picker.set('batches', batches);
             }
         };
 

=== modified file 'lib/lp/app/javascript/picker/tests/test_picker_patcher.js'
--- lib/lp/app/javascript/picker/tests/test_picker_patcher.js	2016-07-21 15:13:12 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker_patcher.js	2016-09-12 19:55:47 +0000
@@ -460,15 +460,15 @@
                 ['a', 'b', 'c'], this.picker.get('filter_options'));
         },
 
-        test_picker_displays_empty_list: function() {
-            // With too many results, the results will be empty.
+        test_picker_displays_truncated_list: function() {
+            // With too many results, the results will be truncated.
             this.create_picker(true);
             this.picker.render();
             this.picker.set('min_search_chars', 0);
             this.picker.fire('search', '');
-            var result_text = this.picker.get('contentBox')
-                .one('.yui3-picker-results').get('text');
-            Assert.areEqual('', result_text);
+            var results = this.picker.get('contentBox')
+                .one('.yui3-picker-results');
+            Assert.areEqual(120, results.get('children').size());
         },
 
         test_picker_displays_warning: function() {
@@ -478,7 +478,7 @@
             this.picker.set('min_search_chars', 0);
             this.picker.fire('search', '');
             Assert.areEqual(
-                'Too many matches. Please try to narrow your search.',
+                'Only displaying first 120 of 121 matches.',
                 this.picker.get('error'));
         },
 
@@ -490,7 +490,7 @@
             this.picker.set('min_search_chars', 0);
             this.picker.fire('search', '');
             Assert.areEqual(
-                'Too many matches. Please try to narrow your search.',
+                'Only displaying first 120 of 121 matches.',
                 this.picker.get('error'));
         },