Show loading errors inside inline diffs
When diffs encounter loading errors (such as 404s or 500s in the diff
request), the diff would formerly bubble up an event that showed an
error message that blocks the entire page. This works well in the diff
view because the singular diff is the main content of the page.
However, when expanding many files inline in the change view, if only
some of the diffs encounter loading errors, the error message need not
obstruct the rest of the page, which may contain diffs that load fine.
With this change, inline diffs show their own descriptive loading
errors, and won't hide the rest of the content. The structure of the
diff loading request is refactored so that errors can be caught and
handled cleanly in the promise chain and the overall async-loop that
renders many diffs in order will not be halted when one diff fails.
Bug: Issue 9590
Change-Id: I9d984d39dcec90119d05acd94afad938e5fb7c1e
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 8e975f4..cb37d0d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -192,15 +192,20 @@
font-size: var(--font-size, var(--font-size-normal));
padding: 0.5em 0 0.5em 4em;
}
+ #loadingError,
#sizeWarning {
display: none;
margin: 1em auto;
max-width: 60em;
text-align: center;
}
+ #loadingError {
+ color: var(--error-text-color);
+ }
#sizeWarning gr-button {
margin: 1em;
}
+ #loadingError.showError,
#sizeWarning.warn {
display: block;
}
@@ -299,6 +304,9 @@
<div class$="[[_computeNewlineWarningClass(_newlineWarning, _loading)]]">
[[_newlineWarning]]
</div>
+ <div id="loadingError" class$="[[_computeErrorClass(_showError)]]">
+ [[_errorMessage]]
+ </div>
<div id="sizeWarning" class$="[[_computeWarningClass(_showWarning)]]">
<p>
Prevented render because "Whole file" is enabled and this diff is very
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 4b19e67..4032cec 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -123,6 +123,14 @@
*/
lineOfInterest: Object,
+ /**
+ * If the diff fails to load, show the failure message in the diff rather
+ * than bubbling the error up to the whole page. This is useful for when
+ * loading inline diffs because one diff failing need not mark the whole
+ * page with a failure.
+ */
+ showLoadFailure: Boolean,
+
_loading: {
type: Boolean,
value: false,
@@ -162,6 +170,12 @@
_showWarning: Boolean,
+ _showError: {
+ type: Boolean,
+ value: false,
+ },
+ _errorMessage: String,
+
/** @type {?Object} */
_blame: {
type: Object,
@@ -224,16 +238,23 @@
this.clearBlame();
this._safetyBypass = null;
this._showWarning = false;
+ this._showError = false;
this.clearDiffContent();
- const promises = [];
+ const diffRequest = this._getDiff();
- promises.push(this._getDiff().then(diff => {
- this._diff = diff;
+ const assetRequest = diffRequest.then(diff => {
+ // If the diff is null, then it's failed to load.
+ if (!diff) { return null; }
+
return this._loadDiffAssets();
- }));
+ });
- return Promise.all(promises).then(() => {
+ return Promise.all([diffRequest, assetRequest]).then(results => {
+ const diff = results[0];
+ if (!diff) {
+ return Promise.resolve();
+ }
if (this.prefs) {
return this._renderDiffTable();
}
@@ -727,34 +748,59 @@
this.fire('server-error', {response});
return;
}
+
+ if (this.showLoadFailure) {
+ this._errorMessage = [
+ 'Encountered error when loading the diff:',
+ response.status,
+ response.statusText,
+ ].join(' ');
+ this._showError = true;
+ return;
+ }
+
this.fire('page-error', {response});
},
/** @return {!Promise<!Object>} */
_getDiff() {
- return this.$.restAPI.getDiff(
- this.changeNum,
- this.patchRange.basePatchNum,
- this.patchRange.patchNum,
- this.path,
- this._handleGetDiffError.bind(this)).then(diff => {
+ // Wrap the diff request in a new promise so that the error handler
+ // rejects the promise, allowing the error to be handled in the .catch.
+ const request = new Promise((resolve, reject) => {
+ this.$.restAPI.getDiff(
+ this.changeNum,
+ this.patchRange.basePatchNum,
+ this.patchRange.patchNum,
+ this.path,
+ reject)
+ .then(resolve);
+ });
+
+ return request
+ .then(diff => {
+ this.filesWeblinks = this._getFilesWeblinks(diff);
+ this._diff = diff;
this._reportDiff(diff);
- if (!this.commitRange) {
- this.filesWeblinks = {};
- return diff;
- }
- this.filesWeblinks = {
- meta_a: Gerrit.Nav.getFileWebLinks(
- this.projectName, this.commitRange.baseCommit, this.path,
- {weblinks: diff && diff.meta_a && diff.meta_a.web_links}),
- meta_b: Gerrit.Nav.getFileWebLinks(
- this.projectName, this.commitRange.commit, this.path,
- {weblinks: diff && diff.meta_b && diff.meta_b.web_links}),
- };
return diff;
+ })
+ .catch(e => {
+ this._handleGetDiffError(e);
+ return null;
});
},
+ _getFilesWeblinks(diff) {
+ if (!this.commitRange) { return {}; }
+ return {
+ meta_a: Gerrit.Nav.getFileWebLinks(
+ this.projectName, this.commitRange.baseCommit, this.path,
+ {weblinks: diff && diff.meta_a && diff.meta_a.web_links}),
+ meta_b: Gerrit.Nav.getFileWebLinks(
+ this.projectName, this.commitRange.commit, this.path,
+ {weblinks: diff && diff.meta_b && diff.meta_b.web_links}),
+ };
+ },
+
/**
* Report info about the diff response.
*/
@@ -890,6 +936,11 @@
return showWarning ? 'warn' : '';
},
+ /** @return {string} */
+ _computeErrorClass(showError) {
+ return showError ? 'showError' : '';
+ },
+
/**
* @return {number|null}
*/
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index b98602f..2821dde 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -410,7 +410,6 @@
suite('image diffs', () => {
let mockFile1;
let mockFile2;
- const stubs = [];
setup(() => {
mockFile1 = {
body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAE' +
@@ -445,18 +444,18 @@
};
const mockComments = {baseComments: [], comments: []};
- stubs.push(sandbox.stub(element.$.restAPI, 'getCommitInfo',
- () => Promise.resolve(mockCommit)));
- stubs.push(sandbox.stub(element.$.restAPI,
+ sandbox.stub(element.$.restAPI, 'getCommitInfo')
+ .returns(Promise.resolve(mockCommit));
+ sandbox.stub(element.$.restAPI,
'getB64FileContents',
(changeId, patchNum, path, opt_parentIndex) => {
return Promise.resolve(opt_parentIndex === 1 ? mockFile1 :
mockFile2);
- }));
- stubs.push(sandbox.stub(element.$.restAPI, '_getDiffComments',
- () => Promise.resolve(mockComments)));
- stubs.push(sandbox.stub(element.$.restAPI, 'getDiffDrafts',
- () => Promise.resolve(mockComments)));
+ });
+ sandbox.stub(element.$.restAPI, '_getDiffComments')
+ .returns(Promise.resolve(mockComments));
+ sandbox.stub(element.$.restAPI, 'getDiffDrafts')
+ .returns(Promise.resolve(mockComments));
element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
element.comments = {left: [], right: []};
@@ -479,8 +478,8 @@
content: [{skip: 66}],
binary: true,
};
- stubs.push(sandbox.stub(element, '_getDiff',
- () => Promise.resolve(mockDiff)));
+ sandbox.stub(element.$.restAPI, 'getDiff')
+ .returns(Promise.resolve(mockDiff));
const rendered = () => {
// Recognizes that it should be an image diff.
@@ -559,8 +558,8 @@
content: [{skip: 66}],
binary: true,
};
- stubs.push(sandbox.stub(element, '_getDiff',
- () => Promise.resolve(mockDiff)));
+ sandbox.stub(element.$.restAPI, 'getDiff')
+ .returns(Promise.resolve(mockDiff));
const rendered = () => {
// Recognizes that it should be an image diff.
@@ -640,8 +639,8 @@
content: [{skip: 66}],
binary: true,
};
- stubs.push(sandbox.stub(element, '_getDiff',
- () => Promise.resolve(mockDiff)));
+ sandbox.stub(element.$.restAPI, 'getDiff')
+ .returns(Promise.resolve(mockDiff));
element.addEventListener('render', () => {
// Recognizes that it should be an image diff.
@@ -679,8 +678,8 @@
content: [{skip: 66}],
binary: true,
};
- stubs.push(sandbox.stub(element, '_getDiff',
- () => Promise.resolve(mockDiff)));
+ sandbox.stub(element.$.restAPI, 'getDiff')
+ .returns(Promise.resolve(mockDiff));
element.addEventListener('render', () => {
// Recognizes that it should be an image diff.
@@ -720,8 +719,8 @@
};
mockFile1.type = 'image/jpeg-evil';
- stubs.push(sandbox.stub(element, '_getDiff',
- () => Promise.resolve(mockDiff)));
+ sandbox.stub(element.$.restAPI, 'getDiff')
+ .returns(Promise.resolve(mockDiff));
element.addEventListener('render', () => {
// Recognizes that it should be an image diff.
@@ -793,6 +792,55 @@
element._getDiff().then(done);
});
+ test('_getDiff resolves as null on error', () => {
+ const onErrStub = sandbox.stub(element, '_handleGetDiffError');
+ const error = {ok: false, status: 500};
+ sandbox.stub(element.$.restAPI, 'getDiff',
+ (changeNum, basePatchNum, patchNum, path, onErr) => {
+ onErr(error);
+ });
+ return element._getDiff().then(diff => {
+ assert.isNull(diff);
+ assert.isTrue(onErrStub.calledOnce);
+ });
+ });
+
+ suite('_handleGetDiffError', () => {
+ let serverErrorStub;
+ let pageErrorStub;
+
+ setup(() => {
+ serverErrorStub = sinon.stub();
+ element.addEventListener('server-error', serverErrorStub);
+ pageErrorStub = sinon.stub();
+ element.addEventListener('page-error', pageErrorStub);
+ });
+
+ test('page error on HTTP-409', () => {
+ element._handleGetDiffError({status: 409});
+ assert.isTrue(serverErrorStub.calledOnce);
+ assert.isFalse(pageErrorStub.called);
+ assert.isFalse(element._showError);
+ });
+
+ test('server error on non-HTTP-409', () => {
+ element._handleGetDiffError({status: 500});
+ assert.isFalse(serverErrorStub.called);
+ assert.isTrue(pageErrorStub.calledOnce);
+ assert.isFalse(element._showError);
+ });
+
+ test('error message if showLoadFailure', () => {
+ element.showLoadFailure = true;
+ element._handleGetDiffError({status: 500, statusText: 'Failure!'});
+ assert.isFalse(serverErrorStub.called);
+ assert.isFalse(pageErrorStub.called);
+ assert.isTrue(element._showError);
+ assert.equal(element._errorMessage,
+ 'Encountered error when loading the diff: 500 Failure!');
+ });
+ });
+
suite('getCursorStops', () => {
const setupDiff = function() {
const mock = document.createElement('mock-diff-response');
