opendev/gerrit
Code Issues Proposed changes

Show loading errors inside inline diffs

Browse Source 
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
This commit is contained in:
Wyatt Allen 2018-08-15 14:13:01 -07:00
parent 55c6c0e684
commit eb04344f67
6 changed files with 155 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
View File

@ -390,6 +390,7 @@ limitations under the License.
if="[[_isFileExpanded(file.__path, _expandedFilePaths.*)]]">
<gr-diff
no-auto-render
show-load-failure
display-line="[[_displayLine]]"
inline-index=[[index]]
hidden="[[!_isFileExpanded(file.__path, _expandedFilePaths.*)]]"

8
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
View File

@ -50,6 +50,9 @@ limitations under the License.
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(false); },
getDiff() {
return Promise.resolve(mockDiffResponse.diffResponse);
},
});
const fixtureElems = fixture('basic');
@ -60,15 +63,12 @@ limitations under the License.
// 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();

8
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
View File

@ -192,15 +192,20 @@ limitations under the License.
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 @@ limitations under the License.
<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

97
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
View File

@ -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();
const assetRequest = diffRequest.then(diff => {
// If the diff is null, then it's failed to load.
if (!diff) { return null; }
promises.push(this._getDiff().then(diff => {
this._diff = diff;
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}
*/

86
polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
View File

@ -410,7 +410,6 @@ limitations under the License.
suite('image diffs', () => {
let mockFile1;
let mockFile2;
const stubs = [];
setup(() => {
mockFile1 = {
body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAE' +
@ -445,18 +444,18 @@ limitations under the License.
};
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 @@ limitations under the License.
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 @@ limitations under the License.
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 @@ limitations under the License.
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 @@ limitations under the License.
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 @@ limitations under the License.
};
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 @@ limitations under the License.
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');

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

@ -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 ?