opendev/gerrit
Code Issues Proposed changes

Merge "Refactor and test sending in gr-reply-dialog"

Browse Source
This commit is contained in:
Wyatt Allen 2017-07-28 21:16:47 +00:00 committed by Gerrit Code Review
commit 68c14fc93b
3 changed files with 166 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

76
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
View File

@ -343,7 +343,7 @@
if (this.knownLatestState === 'not-latest') {
this.fire('show-alert',
{message: 'Cannot reply to non-latest patch.'});
return;
return Promise.resolve({});
}
const labels = this.$.labelScores.getLabelValues();
@ -391,41 +391,63 @@
const errFn = this._handle400Error.bind(this);
return this._saveReview(obj, errFn).then(response => {
if (!response || !response.ok) {
return response;
if (!response) {
// Null or undefined response indicates that an error handler
// took responsibility, so just return.
return {};
}
return this.$.restAPI.getResponseObject(response);
if (!response.ok) {
this.fire('server-error', {response});
return {};
}
// TODO(logan): Remove once the required API changes are live and stable
// on googlesource.com.
return this._maybeSetReady(startReview, response).catch(err => {
// We catch error here because we still want to treat this as a
// successful review.
console.error('error setting ready:', err);
}).then(() => {
this.draft = '';
this._includeComments = true;
this.fire('send', null, {bubbles: false});
return accountAdditions;
});
}).then(result => {
// TODO(logan): Remove this once the required API changes are live and
// stable on googlesource.com.
if (startReview && !result.ready) {
// If we don't see ready in the response, then we're talking to a
// backend that doesn't support moving out of WIP at the same time as
// posting a review. Fall back to sending a second API call to start
// review and block until that returns.
return this.$.restAPI.startReview(this.change._number, null,
response => {
// If we see a 409 response code, then that means the server
// *does* support moving from WIP->ready when posting a review.
// In that case we should just ignore this error.
if (response.status === 409) {
return;
}
this.fire('server-error', {response});
});
}
}).then(() => {
this.disabled = false;
this.draft = '';
this._includeComments = true;
this.fire('send', null, {bubbles: false});
return accountAdditions;
return result;
}).catch(err => {
this.disabled = false;
throw err;
});
},
/**
* Returns a promise resolving to true if review was successfully posted,
* false otherwise.
*
* TODO(logan): Remove this once the required API changes are live and
* stable on googlesource.com.
*/
_maybeSetReady(startReview, response) {
return this.$.restAPI.getResponseObject(response).then(result => {
if (!startReview || result.ready) {
return Promise.resolve();
}
// We don't have confirmation that review was started, so attempt to
// start review explicitly.
return this.$.restAPI.startReview(
this.change._number, null, response => {
// If we see a 409 response code, then that means the server
// *does* support moving from WIP->ready when posting a
// review. Only alert user for non-409 failures.
if (response.status !== 409) {
this.fire('server-error', {response});
}
});
});
},
_focusOn(section) {
if (section === FocusTarget.ANY) {
section = this._chooseFocusTarget();

129
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
View File

@ -114,14 +114,20 @@ limitations under the License.
function stubSaveReview(jsonResponseProducer) {
return sandbox.stub(element, '_saveReview', review => {
const result = jsonResponseProducer(review);
const resultStr =
element.$.restAPI.JSON_PREFIX + JSON.stringify(result);
return Promise.resolve({
ok: true,
text() {
return Promise.resolve(resultStr);
},
return new Promise((resolve, reject) => {
try {
const result = jsonResponseProducer(review) || {};
const resultStr =
element.$.restAPI.JSON_PREFIX + JSON.stringify(result);
resolve({
ok: true,
text() {
return Promise.resolve(resultStr);
},
});
} catch (err) {
reject(err);
}
});
});
}
@ -213,10 +219,13 @@ limitations under the License.
});
element.addEventListener('send', () => {
assert.isFalse(element.disabled,
'Element should be enabled when done sending reply.');
assert.equal(element.draft.length, 0);
done();
// Flush to ensure properties are updated.
flush(() => {
assert.isFalse(element.disabled,
'Element should be enabled when done sending reply.');
assert.equal(element.draft.length, 0);
done();
});
});
// This is needed on non-Blink engines most likely due to the ways in
@ -702,12 +711,14 @@ limitations under the License.
});
test('should not send on enter key', () => {
stubSaveReview(() => undefined);
element.addEventListener('send', () => assert.fail('wrongly called'));
MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
flushAsynchronousOperations();
});
test('emit send on ctrl+enter key', done => {
stubSaveReview(() => undefined);
element.addEventListener('send', () => done());
MockInteractions.pressAndReleaseKeyOn(element, 13, 'ctrl', 'enter');
flushAsynchronousOperations();
@ -795,7 +806,9 @@ limitations under the License.
let startReviewStub;
setup(() => {
startReviewStub = sandbox.stub(element.$.restAPI, 'startReview');
startReviewStub = sandbox.stub(element.$.restAPI, 'startReview', () => {
return Promise.resolve();
});
});
test('ready property in review input on start review', () => {
@ -811,7 +824,6 @@ limitations under the License.
test('no ready property in review input on save review', () => {
stubSaveReview(review => {
assert.isUndefined(review.ready);
return {};
});
return element.send(true, false).then(() => {
assert.isFalse(startReviewStub.called);
@ -864,5 +876,94 @@ limitations under the License.
});
return element.send(true, true);
});
test('buttons disabled until all API calls are resolved', () => {
stubSaveReview(review => {
return {}; // old backend won't set ready: true
});
// Check that element is disabled asynchronously after the setReady
// promise is returned. The element should not be reenabled until
// that promise is resolved.
sandbox.stub(element, '_maybeSetReady', (startReview, response) => {
return new Promise(resolve => {
Polymer.Base.async(() => {
assert.isTrue(element.disabled);
resolve();
});
});
});
return element.send(true, true).then(() => {
assert.isFalse(element.disabled);
});
});
suite('error handling', () => {
const expectedDraft = 'draft';
const expectedError = new Error('test');
setup(() => {
element.draft = expectedDraft;
});
function assertDialogOpenAndEnabled() {
assert.strictEqual(expectedDraft, element.draft);
assert.isFalse(element.disabled);
}
function assertDialogClosed() {
assert.strictEqual('', element.draft);
assert.isFalse(element.disabled);
}
test('error occurs in _saveReview', () => {
stubSaveReview(review => {
throw expectedError;
});
return element.send(true, true).catch(err => {
assert.strictEqual(expectedError, err);
assertDialogOpenAndEnabled();
});
});
test('error occurs during startReview', () => {
stubSaveReview(review => {
return {}; // old backend won't set ready: true
});
const errorStub = sandbox.stub(
console, 'error', (msg, err) => undefined);
sandbox.stub(element.$.restAPI, 'startReview', () => {
throw expectedError;
});
return element.send(true, true).then(() => {
assertDialogClosed();
assert.isTrue(
errorStub.calledWith('error setting ready:', expectedError));
});
});
test('non-ok response received by startReview', () => {
stubSaveReview(review => {
return {}; // old backend won't set ready: true
});
sandbox.stub(element.$.restAPI, 'startReview', (c, b, f) => {
f({status: 500});
});
return element.send(true, true).then(() => {
assertDialogClosed();
});
});
test('409 response received by startReview', () => {
stubSaveReview(review => {
return {}; // old backend won't set ready: true
});
sandbox.stub(element.$.restAPI, 'startReview', (c, b, f) => {
f({status: 409});
});
return element.send(true, true).then(() => {
assertDialogClosed();
});
});
});
});
</script>

5
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@ -889,8 +889,7 @@
return auth.fetch(this.getBaseUrl() + url, options).then(response => {
if (!response.ok) {
if (opt_errFn) {
opt_errFn.call(opt_ctx || null, response);
return undefined;
return opt_errFn.call(opt_ctx || null, response);
}
this.fire('server-error', {response});
}
@ -899,7 +898,7 @@
}).catch(err => {
this.fire('network-error', {error: err});
if (opt_errFn) {
opt_errFn.call(opt_ctx, null, err);
return opt_errFn.call(opt_ctx, null, err);
} else {
throw err;
}