launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04574
[Merge] lp:~benji/launchpad/bug-823321 into lp:launchpad
Benji York has proposed merging lp:~benji/launchpad/bug-823321 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #823321 in Launchpad itself: "Related branch pre-search clears search activity indicator when complete"
https://bugs.launchpad.net/launchpad/+bug/823321
For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-823321/+merge/71229
This branch fixes bug 823321 which is about the automatic bug number
search function interfering with a user's search.
The implementation is pretty straightforward. The search for
suggestions is flagged as an "automated" search and that detail is
closed over by the success and failure handlers. Then, if the user has
entered their own search by the time the handlers are called, the
results are ignored.
Tests: tests were added to the picker patcher tests
(lib/lp/app/javascript/picker/picker_patcher.html). Related tests that
still pass are the general picker tests
(lib/lp/app/javascript/picker/tests/test_picker.html) and the
"pre-search" tests (lib/lp/bugs/javascript/tests/test_pre_search.html).
Lint: the lint report is clean (after fixing some pre-existing lint).
QA: click the "Link a related branch" link on a bug and quickly enter
your own search term and hit enter. When the original (automatic)
search for branches with the bug number in the name returns, you should
see nothing change and your search results will eventually be displayed.
--
https://code.launchpad.net/~benji/launchpad/bug-823321/+merge/71229
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-823321 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
--- lib/lp/app/javascript/picker/picker_patcher.js 2011-07-22 01:12:20 +0000
+++ lib/lp/app/javascript/picker/picker_patcher.js 2011-08-11 16:06:23 +0000
@@ -328,10 +328,14 @@
// chance to be performed.
picker.publish('save', { defaultFn: function(){} } );
+ // Has the user performed a search yet?
+ var user_has_searched = false;
+
picker.subscribe('save', function (e) {
Y.log('Got save event.');
var picker_result = e.details[Y.lazr.picker.Picker.SAVE_RESULT];
var do_save = function() {
+ user_has_searched = false;
picker.hide();
if (Y.Lang.isFunction(config.save)) {
config.save(picker_result);
@@ -350,6 +354,7 @@
picker.subscribe('cancel', function (e) {
Y.log('Got cancel event.');
reset_form(picker);
+ user_has_searched = false;
});
if (config.extra_no_results_message !== null) {
@@ -370,8 +375,15 @@
Y.log('Got search event:' + Y.dump(e.details));
var search_text = e.details[0];
var selected_batch = e.details[1] || 0;
+ // Was this search initiated automatically, perhaps to load
+ // suggestions?
+ var automated_search = e.details[2] || false;
var start = BATCH_SIZE * selected_batch;
var batch = 0;
+
+ // Record whether or not the user has initiated a search yet.
+ user_has_searched = user_has_searched || !automated_search;
+
var display_vocabulary = function(results, total_size, start) {
var max_size = MAX_BATCHES * BATCH_SIZE;
if (show_search_box && total_size > max_size) {
@@ -409,11 +421,24 @@
var start = entry.start;
var results = entry.entries;
hide_temporary_spinner(config.temp_spinner);
- display_vocabulary(results, total_size, start);
+ // If this was an automated (preemptive) search and the user
+ // has not subsequently initiated their own search, display
+ // the results of the search.
+ if (user_has_searched !== automated_search) {
+ display_vocabulary(results, total_size, start);
+ }
};
var failure_handler = function (ignore, response, args) {
+ Y.log("Loading " + uri + " failed.");
hide_temporary_spinner(config.temp_spinner);
+ // If this was an automated (preemptive) search and the user
+ // has subsequently initiated their own search, don't bother
+ // showing an error message about something the user didn't
+ // initiate and now doesn't care about.
+ if (user_has_searched === automated_search) {
+ return;
+ }
var base_error =
"Sorry, something went wrong with your search.";
if (response.status === 500) {
@@ -430,7 +455,6 @@
}
picker.set('error', base_error);
picker.set('search_mode', false);
- Y.log("Loading " + uri + " failed.");
};
=== modified file 'lib/lp/app/javascript/picker/tests/test_picker_patcher.js'
--- lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2011-07-27 00:01:47 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2011-08-11 16:06:23 +0000
@@ -400,6 +400,77 @@
}
}));
+suite.add(new Y.Test.Case({
+
+ name: 'picker_automated_search',
+
+ create_picker: function(yio) {
+ var config = {yio: yio};
+ return Y.lp.app.picker.addPickerPatcher(
+ "Foo",
+ "foo/bar",
+ "test_link",
+ "picker_id",
+ config);
+ },
+
+ make_response: function(status, oops, responseText) {
+ if (oops === undefined) {
+ oops = null;
+ }
+ return {
+ status: status,
+ responseText: responseText,
+ getResponseHeader: function(header) {
+ if (header === 'X-Lazr-OopsId') {
+ return oops;
+ }
+ }
+ };
+ },
+
+ test_automated_search_results_ignored_if_user_has_searched: function() {
+ // If an automated search (like loading branch suggestions) returns
+ // results and the user has submitted a search, then the results of
+ // the automated search are ignored so as not to confuse the user.
+ var mock_io = new Y.lazr.testing.MockIo();
+ var picker = this.create_picker(mock_io);
+ // First an automated search is run.
+ picker.fire('search', 'guess', undefined, true);
+ // We have to stash away the mock IO's on-success handler because
+ // it'll get clobbered by the second searc if we don't.
+ automated_success = mock_io.cfg.on.success;
+ // Then the user initiates their own search.
+ picker.fire('search', 'test');
+ // Now to get ahold of the user-initiated search's on-success.
+ user_success = mock_io.cfg.on.success;
+ // Now, if the automated search returns...
+ mock_io.cfg.on.success = automated_success;
+ mock_io.simulateXhr(this.make_response(200, null, '{"entries": 1}'));
+ // ... the results are ignored.
+ Assert.areNotEqual(1, picker.get('results'));
+ // But if the user's search returns, the results are kept.
+ mock_io.cfg.on.success = user_success;
+ mock_io.simulateXhr(this.make_response(200, null, '{"entries": 2}'));
+ Assert.areEqual(2, picker.get('results'));
+ cleanup_widget(picker);
+ },
+
+ test_automated_search_error_ignored_if_user_has_searched: function() {
+ // If an automated search (like loading branch suggestions) returns an
+ // error and the user has submitted a search, then the error from the
+ // automated search is ignored so as not to confuse the user.
+ var mock_io = new Y.lazr.testing.MockIo();
+ var picker = this.create_picker(mock_io);
+ picker.fire('search', 'test');
+ picker.fire('search', 'guess', undefined, true);
+ mock_io.simulateXhr(this.make_response(500, 'OOPS'), true);
+ Assert.areEqual(null, picker.get('error'));
+ cleanup_widget(picker);
+ }
+
+}));
+
Y.lp.testing.Runner.run(suite);
});
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-08-09 14:18:02 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-08-11 16:06:23 +0000
@@ -364,6 +364,7 @@
privacy_link.setStyle('display', 'inline');
lp_bug_entry = updated_entry;
+ var notification = Y.one('.global-notification');
if (private_flag) {
Y.one('body').replaceClass('public', 'private');
privacy_div.replaceClass('public', 'private');
@@ -371,7 +372,6 @@
'innerHTML',
'This report is <strong>private</strong> ');
if (privacy_notification_enabled) {
- var notification = Y.one('.global-notification');
if (notification === null) {
Y.lp.app.privacy.setup_privacy_notification();
}
@@ -379,7 +379,6 @@
}
} else {
if (privacy_notification_enabled) {
- var notification = Y.one('.global-notification');
if (notification === null) {
Y.lp.app.privacy.setup_privacy_notification();
}
@@ -430,7 +429,6 @@
lp_bug_entry.lp_save(config);
};
-
/**
* Do a preemptive search for branches that contain the current bug's ID.
*/
@@ -442,7 +440,7 @@
// A very few bugs have small IDs.
var original_min_search_chars = picker.get('min_search_chars');
picker.set('min_search_chars', 0);
- picker.fire('search', bug_id.toString());
+ picker.fire('search', bug_id.toString(), undefined, true);
// Don't disable the search input box or the search button while
// doing our search.
picker.set('search_mode', false);
@@ -494,8 +492,8 @@
config.save = get_branch_and_link_to_bug;
var picker = Y.lp.app.picker.create('Branch', config);
// When the user clicks on "Link a related branch" do a search for
- // branches that contain the bug number.
- link_branch_link.subscribe('click', function (e) {
+ // branches that contain the bug number (but only once).
+ link_branch_link.once('click', function (e) {
do_pre_search(picker);
});
}
