launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07775
[Merge] lp:~wallyworld/launchpad/sharing-failure-error-display-997317 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-failure-error-display-997317 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #997317 in Launchpad itself: "No clear visual indication when deleting someone's sharing fails"
https://bugs.launchpad.net/launchpad/+bug/997317
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-failure-error-display-997317/+merge/105440
== Implementation ==
When I first did the sharing ui I forgot to wire up the error handler to display XHR errors. This branch wires everything up correctly so that any server call which results in an error has the error displayed and the table row (if any) flashes red.
== Tests ==
Add new yui tests for the pillarsharing view, sharee table widget, sharing details view, sharing details widget.
bin/test -vvct test_pillarsharingview -t test_shareetable -t test_sharingdetails_view -t test_sharingdetails
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/javascript/sharing/pillarsharingview.js
lib/lp/registry/javascript/sharing/shareetable.js
lib/lp/registry/javascript/sharing/sharingdetails.js
lib/lp/registry/javascript/sharing/sharingdetailsview.js
lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js
lib/lp/registry/javascript/sharing/tests/test_shareetable.html
lib/lp/registry/javascript/sharing/tests/test_shareetable.js
lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html
lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js
lib/lp/registry/javascript/sharing/tests/test_sharingdetailsview.js
--
https://code.launchpad.net/~wallyworld/launchpad/sharing-failure-error-display-997317/+merge/105440
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-failure-error-display-997317 into lp:launchpad.
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-04-26 07:54:34 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-05-11 06:33:18 +0000
@@ -164,6 +164,24 @@
sharee_table.syncUI();
},
+ // A common error handler for XHR operations.
+ _error_handler: function(person_uri) {
+ var sharee_data = LP.cache.sharee_data;
+ var error_handler = new Y.lp.client.ErrorHandler();
+ var sharee_name = null;
+ Y.Array.some(sharee_data, function(sharee) {
+ if (sharee.self_link === person_uri) {
+ sharee_name = sharee.name;
+ return true;
+ }
+ });
+ var self = this;
+ error_handler.showError = function(error_msg) {
+ self.get('sharee_table').display_error(sharee_name, error_msg);
+ };
+ return error_handler;
+ },
+
/**
* Show a spinner next to the delete icon.
*
@@ -240,7 +258,7 @@
* @param person_uri
*/
perform_remove_sharee: function(delete_link, person_uri) {
- var error_handler = new Y.lp.client.ErrorHandler();
+ var error_handler = this._error_handler(person_uri);
var pillar_uri = LP.cache.context.self_link;
var self = this;
var y_config = {
@@ -316,7 +334,7 @@
* @param permissions
*/
save_sharing_selection: function(person_uri, permissions) {
- var error_handler = new Y.lp.client.ErrorHandler();
+ var error_handler = this._error_handler(person_uri);
var pillar_uri = LP.cache.context.self_link;
person_uri = Y.lp.client.normalize_uri(person_uri);
person_uri = Y.lp.client.get_absolute_uri(person_uri);
=== modified file 'lib/lp/registry/javascript/sharing/shareetable.js'
--- lib/lp/registry/javascript/sharing/shareetable.js 2012-04-11 02:53:26 +0000
+++ lib/lp/registry/javascript/sharing/shareetable.js 2012-05-11 06:33:18 +0000
@@ -526,6 +526,17 @@
delete_rows();
});
anim.run();
+ },
+
+ // An error occurred performing an operation on a sharee.
+ display_error: function(sharee_name, error_msg) {
+ var sharee_table = this.get('sharee_table');
+ var sharee_row = null;
+ if (Y.Lang.isValue(sharee_name)) {
+ sharee_row = sharee_table.one('tr[id=permission-'
+ + sharee_name + ']');
+ }
+ Y.lp.app.errors.display_error(sharee_row, error_msg);
}
});
@@ -537,7 +548,7 @@
}, "0.1", { "requires": [
'node', 'event', 'collection', 'json', 'lazr.choiceedit',
- 'lp.mustache', 'lp.registry.sharing.shareepicker',
+ 'lp.app.errors', 'lp.mustache', 'lp.registry.sharing.shareepicker',
'lp.registry.sharing.shareelisting_navigator'
] });
=== modified file 'lib/lp/registry/javascript/sharing/sharingdetails.js'
--- lib/lp/registry/javascript/sharing/sharingdetails.js 2012-04-23 14:01:43 +0000
+++ lib/lp/registry/javascript/sharing/sharingdetails.js 2012-05-11 06:33:18 +0000
@@ -252,6 +252,17 @@
anim.run();
},
+ // An error occurred performing an operation on an artifact.
+ display_error: function(artifact_id, artifact_type, error_msg) {
+ var details_table_body = this.get('details_table_body');
+ var selector = Y.Lang.substitute(
+ 'tr[id=shared-{artifact_type}-{artifact_id}]', {
+ artifact_type: artifact_type,
+ artifact_id: artifact_id});
+ var artifact_row = details_table_body.one(selector);
+ Y.lp.app.errors.display_error(artifact_row, error_msg);
+ },
+
_table_body_template: function() {
return [
'<tbody id="sharing-table-body">',
@@ -329,5 +340,5 @@
}, "0.1", { "requires": [
'node', 'event', 'json', 'lp.mustache', 'lp.anim',
- 'lp.app.sorttable'
+ 'lp.app.sorttable', 'lp.app.errors'
] });
=== modified file 'lib/lp/registry/javascript/sharing/sharingdetailsview.js'
--- lib/lp/registry/javascript/sharing/sharingdetailsview.js 2012-04-06 17:28:25 +0000
+++ lib/lp/registry/javascript/sharing/sharingdetailsview.js 2012-05-11 06:33:18 +0000
@@ -68,6 +68,33 @@
sharing_details_table.syncUI();
},
+ // A common error handler for XHR operations.
+ _error_handler: function(artifact_uri, artifact_type) {
+ var artifact_id;
+ if (artifact_type === 'bug') {
+ Y.Array.some(LP.cache.bugs, function(bug) {
+ if (bug.self_link === artifact_uri) {
+ artifact_id = bug.bug_id;
+ return true;
+ }
+ });
+ } else {
+ Y.Array.some(LP.cache.branches, function(branch) {
+ if (branch.self_link === artifact_uri) {
+ artifact_id = branch.branch_id;
+ return true;
+ }
+ });
+ }
+ var error_handler = new Y.lp.client.ErrorHandler();
+ var self = this;
+ error_handler.showError = function(error_msg) {
+ self.get('sharing_details_table')
+ .display_error(artifact_id, artifact_type, error_msg);
+ };
+ return error_handler;
+ },
+
/**
* Show a spinner next to the delete icon.
*
@@ -161,7 +188,7 @@
* @param artifact_type
*/
perform_remove_grant: function(delete_link, artifact_uri, artifact_type) {
- var error_handler = new Y.lp.client.ErrorHandler();
+ var error_handler = this._error_handler(artifact_uri, artifact_type);
var bugs = [];
var branches = [];
if (artifact_type === 'bug') {
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-04-26 07:54:34 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-05-11 06:33:18 +0000
@@ -270,6 +270,43 @@
});
},
+ // XHR calls display errors correctly.
+ _assert_error_displayed_on_failure: function(invoke_operation) {
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ var lp_client = new Y.lp.client.Launchpad({
+ io_provider: mockio
+ });
+ this.view = this._create_Widget({
+ lp_client: lp_client
+ });
+ this.view.render();
+ var display_error_called = false;
+ var sharee_table = this.view.get('sharee_table');
+ sharee_table.display_error = function(sharee_name, error_msg) {
+ Y.Assert.areEqual('fred', sharee_name);
+ Y.Assert.areEqual(
+ 'Server error, please contact an administrator.',
+ error_msg);
+ display_error_called = true;
+ };
+ invoke_operation(this.view);
+ mockio.last_request.respond({
+ status: 500,
+ statusText: 'An error occurred'
+ });
+ Y.Assert.isTrue(display_error_called);
+ },
+
+ // The perform_remove_sharee method handles errors correctly.
+ test_perform_remove_sharee_error: function() {
+ var invoke_remove = function(view) {
+ var delete_link =
+ Y.one('#sharee-table span[id=remove-fred] a');
+ view.perform_remove_sharee(delete_link, '~fred');
+ };
+ this._assert_error_displayed_on_failure(invoke_remove);
+ },
+
// When a sharee is added, the expected XHR calls are made.
test_perform_add_sharee: function() {
var mockio = new Y.lp.testing.mockio.MockIo();
@@ -405,6 +442,14 @@
Y.Assert.isTrue(model_updated);
},
+ // The save_sharing_selection method handles errors correctly.
+ test_save_sharing_selection_error: function() {
+ var invoke_save = function(view) {
+ view.save_sharing_selection("~fred", ["P1,All"]);
+ };
+ this._assert_error_displayed_on_failure(invoke_save);
+ },
+
// If the XHR result of a sharePillarInformation call is null, the user
// is to be deleted.
test_save_sharing_selection_null_result: function() {
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareetable.html'
--- lib/lp/registry/javascript/sharing/tests/test_shareetable.html 2012-03-28 03:50:33 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareetable.html 2012-05-11 06:33:18 +0000
@@ -28,6 +28,8 @@
<script type="text/javascript"
src="../../../../../../build/js/lp/app/client.js"></script>
<script type="text/javascript"
+ src="../../../../../../build/js/lp/app/errors.js"></script>
+ <script type="text/javascript"
src="../../../../../../build/js/lp/app/lp.js"></script>
<script type="text/javascript"
src="../../../../../../build/js/lp/app/mustache.js"></script>
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareetable.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareetable.js 2012-04-11 02:53:26 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareetable.js 2012-05-11 06:33:18 +0000
@@ -358,6 +358,35 @@
'permissions': {'P1': 's2'}};
this.sharee_table.navigator.fire('updateContent', [new_sharee]);
this._test_sharee_rendered(new_sharee);
+ },
+
+ test_display_error_named_sharee: function() {
+ // A named sharee operation error is displayed correctly.
+ this.sharee_table = this._create_Widget();
+ this.sharee_table.render();
+ var row_fred = Y.one('#sharee-table tr[id=permission-fred]');
+ var success = false;
+ Y.lp.app.errors.display_error = function(flash_node, msg) {
+ Y.Assert.areEqual(row_fred, flash_node);
+ Y.Assert.areEqual('An error occurred', msg);
+ success = true;
+ };
+ this.sharee_table.display_error('fred', 'An error occurred');
+ Y.Assert.isTrue(success);
+ },
+
+ test_display_error_no_sharee: function() {
+ // An peration error is displayed correctly.
+ this.sharee_table = this._create_Widget();
+ this.sharee_table.render();
+ var success = false;
+ Y.lp.app.errors.display_error = function(flash_node, msg) {
+ Y.Assert.isNull(flash_node);
+ Y.Assert.areEqual('An error occurred', msg);
+ success = true;
+ };
+ this.sharee_table.display_error(null, 'An error occurred');
+ Y.Assert.isTrue(success);
}
}));
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html 2012-04-16 13:03:13 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html 2012-05-11 06:33:18 +0000
@@ -28,6 +28,8 @@
<script type="text/javascript"
src="../../../../../../build/js/lp/app/lp.js"></script>
<script type="text/javascript"
+ src="../../../../../../build/js/lp/app/errors.js"></script>
+ <script type="text/javascript"
src="../../../../../../build/js/lp/app/mustache.js"></script>
<script type="text/javascript"
src="../../../../../../build/js/lp/app/anim/anim.js"></script>
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js 2012-04-23 14:01:43 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js 2012-05-11 06:33:18 +0000
@@ -247,6 +247,36 @@
Y.Assert.areEqual(
'There are no shared bugs or branches.',
Y.one('#sharing-table-body tr').get('text'));
+ },
+
+ test_display_error_bug: function() {
+ // A bug operation error is displayed correctly.
+ this.details_widget = this._create_Widget();
+ this.details_widget.render();
+ var row_bug = Y.one('#sharing-table-body tr[id=shared-bug-2]');
+ var success = false;
+ Y.lp.app.errors.display_error = function(flash_node, msg) {
+ Y.Assert.areEqual(row_bug, flash_node);
+ Y.Assert.areEqual('An error occurred', msg);
+ success = true;
+ };
+ this.details_widget.display_error(2, 'bug', 'An error occurred');
+ Y.Assert.isTrue(success);
+ },
+
+ test_display_error_branch: function() {
+ // A branch operation error is displayed correctly.
+ this.details_widget = this._create_Widget();
+ this.details_widget.render();
+ var row_branch= Y.one('#sharing-table-body tr[id=shared-branch-2]');
+ var success = false;
+ Y.lp.app.errors.display_error = function(flash_node, msg) {
+ Y.Assert.areEqual(row_branch, flash_node);
+ Y.Assert.areEqual('An error occurred', msg);
+ success = true;
+ };
+ this.details_widget.display_error(2, 'branch', 'An error occurred');
+ Y.Assert.isTrue(success);
}
}));
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetailsview.js'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetailsview.js 2012-04-06 17:28:25 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetailsview.js 2012-05-11 06:33:18 +0000
@@ -285,6 +285,60 @@
});
},
+ // XHR calls display errors correctly.
+ _assert_error_displayed_on_failure: function(
+ bug_or_branch, invoke_operation) {
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ var lp_client = new Y.lp.client.Launchpad({
+ io_provider: mockio
+ });
+ this.view = this._create_Widget({
+ lp_client: lp_client
+ });
+ this.view.render();
+ var display_error_called = false;
+ var sharee_table = this.view.get('sharing_details_table');
+ sharee_table.display_error = function(
+ artifact_id, artifact_type, error_msg) {
+ Y.Assert.areEqual(2, artifact_id);
+ Y.Assert.areEqual(bug_or_branch, artifact_type);
+ Y.Assert.areEqual(
+ 'Server error, please contact an administrator.',
+ error_msg);
+ display_error_called = true;
+ };
+ invoke_operation(this.view);
+ mockio.last_request.respond({
+ status: 500,
+ statusText: 'An error occurred'
+ });
+ Y.Assert.isTrue(display_error_called);
+ },
+
+ // The perform_remove_grant method handles errors correctly with bugs.
+ test_perform_remove_bug_error: function() {
+ var invoke_remove = function(view) {
+ var delete_link =
+ Y.one('#sharing-table-body span[id=remove-bug-2] a');
+ view.perform_remove_grant(
+ delete_link, 'api/devel/bugs/2', 'bug');
+ };
+ this._assert_error_displayed_on_failure('bug', invoke_remove);
+ },
+
+ // The perform_remove_grant method handles errors correctly with
+ // branches.
+ test_perform_remove_branch_error: function() {
+ var invoke_remove = function(view) {
+ var delete_link =
+ Y.one('#sharing-table-body span[id=remove-branch-2] a');
+ view.perform_remove_grant(
+ delete_link, 'api/devel/~someone/+junk/somebranch',
+ 'branch');
+ };
+ this._assert_error_displayed_on_failure('branch', invoke_remove);
+ },
+
// Test that syncUI works as expected.
test_syncUI: function() {
this.view = this._create_Widget();
Follow ups
