launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04268
[Merge] lp:~wgrant/launchpad/bug-419534 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-419534 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #419534 in Launchpad itself: ""loading results failed" in vocabulary search picker"
https://bugs.launchpad.net/launchpad/+bug/419534
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-419534/+merge/67918
This branch replaces the constant "Loading results failed." picker error with something a bit less unhelpful. A condensed version of the usual OOPS page text is shown, along with the OOPS ID if is present.
Screenshots of various cases at <http://people.canonical.com/~wgrant/launchpad/bug-419534/>.
--
https://code.launchpad.net/~wgrant/launchpad/bug-419534/+merge/67918
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-419534 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/error.py'
--- lib/canonical/launchpad/webapp/error.py 2011-06-10 19:36:32 +0000
+++ lib/canonical/launchpad/webapp/error.py 2011-07-14 06:13:24 +0000
@@ -25,10 +25,6 @@
from canonical.config import config
import canonical.launchpad.layers
-from canonical.launchpad.webapp.adapter import (
- clear_request_started,
- set_request_started,
- )
from canonical.launchpad.webapp.interfaces import ILaunchBag
from canonical.launchpad.webapp.publisher import LaunchpadView
from lp.services.propertycache import cachedproperty
@@ -71,6 +67,9 @@
self.request.response.removeAllNotifications()
if self.response_code is not None:
self.request.response.setStatus(self.response_code)
+ if getattr(self.request, 'oopsid') is not None:
+ self.request.response.addHeader(
+ 'X-Lazr-OopsId', self.request.oopsid)
self.computeDebugOutput()
if config.canonical.show_tracebacks:
self.show_tracebacks = True
=== added file 'lib/canonical/launchpad/webapp/tests/test_error.py'
--- lib/canonical/launchpad/webapp/tests/test_error.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_error.py 2011-07-14 06:13:24 +0000
@@ -0,0 +1,31 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test error views."""
+
+from canonical.launchpad.webapp.error import SystemErrorView
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import LaunchpadFunctionalLayer
+from lp.testing import TestCase
+
+
+class TestSystemErrorView(TestCase):
+
+ layer = LaunchpadFunctionalLayer
+
+ def test_without_oops_id(self):
+ request = LaunchpadTestRequest()
+ SystemErrorView(Exception(), request)
+ self.assertEquals(500, request.response.getStatus())
+ self.assertEquals(
+ None,
+ request.response.getHeader('X-Lazr-OopsId', literal=True))
+
+ def test_with_oops_id(self):
+ request = LaunchpadTestRequest()
+ request.oopsid = 'OOPS-1X1'
+ SystemErrorView(Exception(), request)
+ self.assertEquals(500, request.response.getStatus())
+ self.assertEquals(
+ 'OOPS-1X1',
+ request.response.getHeader('X-Lazr-OopsId', literal=True))
=== modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
--- lib/lp/app/javascript/picker/picker_patcher.js 2011-07-08 06:29:36 +0000
+++ lib/lp/app/javascript/picker/picker_patcher.js 2011-07-14 06:13:24 +0000
@@ -466,6 +466,28 @@
display_vocabulary(results, total_size, start);
};
+ var failure_handler = function (ignore, response, args) {
+ hide_temporary_spinner(config.temp_spinner);
+ var base_error =
+ "Sorry, something went wrong with your search.";
+ if (response.status === 500) {
+ base_error +=
+ " We've recorded what happened, and we'll fix it " +
+ "as soon as possible.";
+ } else if (response.status >= 502 && response.status <= 504) {
+ base_error +=
+ " Trying again in a couple of minutes might work.";
+ }
+ var oops_id = response.getResponseHeader('X-Lazr-OopsId');
+ if (oops_id) {
+ base_error += ' (Error ID: ' + oops_id + ')';
+ }
+ picker.set('error', base_error);
+ picker.set('search_mode', false);
+ Y.log("Loading " + uri + " failed.");
+ };
+
+
var qs = '';
qs = Y.lp.client.append_qs(qs, 'name', vocabulary);
qs = Y.lp.client.append_qs(qs, 'search_text', search_text);
@@ -483,17 +505,13 @@
}
uri += '@@+huge-vocabulary?' + qs;
- Y.io(uri, {
+ var yio = (config.yio !== undefined) ? config.yio : Y;
+ yio.io(uri, {
headers: {'Accept': 'application/json'},
timeout: 20000,
on: {
success: success_handler,
- failure: function (arg) {
- hide_temporary_spinner(config.temp_spinner);
- picker.set('error', 'Loading results failed.');
- picker.set('search_mode', false);
- Y.log("Loading " + uri + " failed.");
- }
+ failure: failure_handler
}
});
// Or we can pass in a vocabulary directly.
=== modified file 'lib/lp/app/javascript/picker/tests/test_picker_patcher.html'
--- lib/lp/app/javascript/picker/tests/test_picker_patcher.html 2011-07-08 06:06:15 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker_patcher.html 2011-07-14 06:13:24 +0000
@@ -24,6 +24,9 @@
<script type="text/javascript" src="../picker.js"></script>
<script type="text/javascript" src="../person_picker.js"></script>
+ <!-- Testing helpers -->
+ <script type="text/javascript" src="../../testing/mockio.js"></script>
+
<!-- The test suite -->
<script type="text/javascript" src="test_picker_patcher.js"></script>
</head>
=== modified file 'lib/lp/app/javascript/picker/tests/test_picker_patcher.js'
--- lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2011-07-08 06:29:36 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2011-07-14 06:13:24 +0000
@@ -3,6 +3,7 @@
YUI().use('lp.testing.runner', 'test', 'console', 'node', 'lp', 'lp.client',
'event-focus', 'event-simulate', 'lazr.picker', 'lazr.person-picker',
'lp.app.picker', 'node-event-simulate', 'escape', 'event',
+ 'lazr.testing.mockio',
function(Y) {
var Assert = Y.Assert;
@@ -506,6 +507,74 @@
}));
+suite.add(new Y.Test.Case({
+
+ name: 'picker_error_handling',
+
+ setUp: function() {
+ this.create_picker();
+ this.picker.fire('search', 'foo');
+ },
+
+ tearDown: function() {
+ cleanup_widget(this.picker);
+ },
+
+ create_picker: function() {
+ this.mock_io = new Y.lazr.testing.MockIo();
+ this.picker = Y.lp.app.picker.addPickerPatcher(
+ "Foo",
+ "foo/bar",
+ "test_link",
+ "picker_id",
+ {yio: this.mock_io});
+ },
+
+ make_error_response: function(status, oops) {
+ if (oops === undefined) {
+ oops = null;
+ }
+ return {
+ status: status,
+ getResponseHeader: function(header) {
+ if (header === 'X-Lazr-OopsId') {
+ return oops;
+ }
+ }
+ };
+ },
+
+ test_oops: function() {
+ // A 500 (ISE) with an OOPS ID informs the user that we've
+ // logged it, and gives them the OOPS ID.
+ this.mock_io.simulateXhr(this.make_error_response(500, 'OOPS'), true);
+ Assert.areEqual(
+ "Sorry, something went wrong with your search. We've recorded " +
+ "what happened, and we'll fix it as soon as possible. " +
+ "(Error ID: OOPS)",
+ this.picker.get('error'));
+ },
+
+ test_timeout: function() {
+ // A 503 (timeout) or 502/504 (proxy error) informs the user
+ // that they should retry, and gives them the OOPS ID.
+ this.mock_io.simulateXhr(this.make_error_response(503, 'OOPS'), true);
+ Assert.areEqual(
+ "Sorry, something went wrong with your search. Trying again " +
+ "in a couple of minutes might work. (Error ID: OOPS)",
+ this.picker.get('error'));
+ },
+
+ test_other_error: function() {
+ // Any other type of error just displays a generic failure
+ // message, with no OOPS ID.
+ this.mock_io.simulateXhr(this.make_error_response(400), true);
+ Assert.areEqual(
+ "Sorry, something went wrong with your search.",
+ this.picker.get('error'));
+ }
+}));
+
Y.lp.testing.Runner.run(suite);
});
