Merge "Show loading errors inside inline diffs"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index a4679a4..bc1b6be 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -390,6 +390,7 @@
if="[[_isFileExpanded(file.__path, _expandedFilePaths.*)]]">
<gr-diff
no-auto-render
+ show-load-failure
display-line="[[_displayLine]]"
inline-index=[[index]]
hidden="[[!_isFileExpanded(file.__path, _expandedFilePaths.*)]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
index 8a89937..2a00425 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
@@ -50,6 +50,9 @@
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(false); },
+ getDiff() {
+ return Promise.resolve(mockDiffResponse.diffResponse);
+ },
});
const fixtureElems = fixture('basic');
@@ -60,15 +63,12 @@
// Register the diff with the cursor.
cursorElement.push('diffs', diffElement);
+ diffElement.patchRange = {basePatchNum: 1, patchNum: 2};
diffElement.comments = {left: [], right: []};
diffElement.$.restAPI.getDiffPreferences().then(prefs => {
diffElement.prefs = prefs;
});
- sandbox.stub(diffElement, '_getDiff', () => {
- return Promise.resolve(mockDiffResponse.diffResponse);
- });
-
const setupDone = () => {
cursorElement._updateStops();
cursorElement.moveToFirstChunk();
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');
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index d014758..8d714eb 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -61,7 +61,6 @@
* endpoint: string,
* patchNum: (string|number|null|undefined),
* errFn: (function(?Response, string=)|null|undefined),
- * cancelCondition: (function()|null|undefined),
* params: (Object|null|undefined),
* fetchOptions: (Object|null|undefined),
* anonymizedEndpoint: (string|undefined),
@@ -2112,10 +2111,8 @@
* @param {number|string} patchNum
* @param {string} path
* @param {function(?Response, string=)=} opt_errFn
- * @param {function()=} opt_cancelCondition
*/
- getDiff(changeNum, basePatchNum, patchNum, path,
- opt_errFn, opt_cancelCondition) {
+ getDiff(changeNum, basePatchNum, patchNum, path, opt_errFn) {
const params = {
context: 'ALL',
intraline: null,
@@ -2133,7 +2130,6 @@
endpoint,
patchNum,
errFn: opt_errFn,
- cancelCondition: opt_cancelCondition,
params,
anonymizedEndpoint: '/files/*/diff',
});
@@ -2796,7 +2792,6 @@
return this._fetchJSON({
url: url + req.endpoint,
errFn: req.errFn,
- cancelCondition: req.cancelCondition,
params: req.params,
fetchOptions: req.fetchOptions,
anonymizedUrl: anonymizedEndpoint ?
