Handle reviewer errors when sending a reply

The server may respond with a 400 Bad Request when we send a review
that adds any invalid reviewers (e.g. an unresolvable account, an
oversized group). We need to not discard the draft review in this
case, and apply special error handling to extract human-friendly
error details from the server response.

Change-Id: I46365c3261f383e9a7a210a622cbb59b1d93d107
This commit is contained in:
Logan Hanks
2016-07-22 16:05:04 -07:00
parent 9d8447b470
commit f1063a2690
2 changed files with 74 additions and 5 deletions

View File

@@ -148,10 +148,13 @@
});
this.disabled = true;
return this._saveReview(obj).then(function(response) {
this.disabled = false;
if (!response.ok) { return response; }
var errFn = this._handle400Error.bind(this);
return this._saveReview(obj, errFn).then(function(response) {
if (!response.ok) {
return response;
}
this.disabled = false;
this.draft = '';
this.fire('send', null, {bubbles: false});
}.bind(this)).catch(function(err) {
@@ -170,6 +173,48 @@
}
},
_handle400Error: function(response) {
// A call to _saveReview could fail with a server error if erroneous
// reviewers were requested. This is signalled with a 400 Bad Request
// status. The default gr-rest-api-interface error handling would
// result in a large JSON response body being displayed to the user in
// the gr-error-manager toast.
//
// We can modify the error handling behavior by passing this function
// through to restAPI as a custom error handling function. Since we're
// short-circuiting restAPI we can do our own response parsing and fire
// the server-error ourselves.
//
this.disabled = false;
if (response.status !== 400) {
// This is all restAPI does when there is no custom error handling.
this.fire('server-error', {response: response});
return;
}
// Process the response body, format a better error message, and fire
// an event for gr-event-manager to display.
var jsonPromise = this.$.restAPI.getResponseObject(response);
return jsonPromise.then(function(result) {
var errors = [];
['reviewers', 'ccs'].forEach(function(state) {
for (var input in result[state]) {
var reviewer = result[state][input];
if (!!reviewer.error) {
errors.push(reviewer.error);
}
}
});
response = {
ok: false,
status: response.status,
text: function() { return Promise.resolve(errors.join(', ')); },
};
this.fire('server-error', {response: response});
}.bind(this));
},
_computeShowLabels: function(patchNum, revisions) {
var num = parseInt(patchNum, 10);
for (var rev in revisions) {
@@ -274,9 +319,9 @@
this.send();
},
_saveReview: function(review) {
_saveReview: function(review, opt_errFn) {
return this.$.restAPI.saveChangeReview(this.change._number, this.patchNum,
review);
review, opt_errFn);
},
_reviewerPendingConfirmationUpdated: function(reviewer) {

View File

@@ -290,5 +290,29 @@ limitations under the License.
assert.isTrue(eraseDraftCommentStub.calledWith(location));
});
test('400 converts to human-readable server-error', function(done) {
sandbox.stub(window, 'fetch', function() {
var text = '....{"reviewers":{"id1":{"error":"first error"}},' +
'"ccs":{"id2":{"error":"second error"}}}';
return Promise.resolve({
ok: false,
status: 400,
text: function() { return Promise.resolve(text); },
});
});
element.addEventListener('server-error', function(event) {
if (event.target !== element) {
return;
}
event.detail.response.text().then(function(body) {
assert.equal(body, 'first error, second error');
done();
});
});
element.send();
});
});
</script>