opendev/gerrit
Code Issues Proposed changes

Merge changes Id7e7b284,Ic4f8706e

Browse Source 
* changes:
  Make log-in check when adding comment synchronous
  Group rendering and weblinking in gr-diff.reload()
This commit is contained in:
Ole Rehmsen
2018-08-24 08:26:02 +00:00
committed by Gerrit Code Review
parent fa9c6e2f26 a1fdc504cb
commit 62bea21ab1
2 changed files with 97 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -237,13 +237,21 @@
this._loading = true;
this._errorMessage = null;
const diffRequest = this._getDiff();
const diffRequest = this._getDiff()
.then(diff => {
this._reportDiff(diff);
return diff;
})
.catch(e => {
this._handleGetDiffError(e);
return null;
});
const assetRequest = diffRequest.then(diff => {
// If the diff is null, then it's failed to load.
if (!diff) { return null; }
return this._loadDiffAssets();
return this._loadDiffAssets(diff);
});
return Promise.all([diffRequest, assetRequest]).then(results => {
@@ -251,11 +259,10 @@
if (!diff) {
return Promise.resolve();
}
if (this.prefs) {
return this._renderDiffTable();
}
return Promise.resolve();
}).then(() => {
this.filesWeblinks = this._getFilesWeblinks(diff);
this._diff = diff;
return this._renderDiffTable();
}).finally(() => {
this._loading = false;
});
},
@@ -388,20 +395,18 @@
addDraftAtLine(el) {
this._selectLine(el);
this._isValidElForComment(el).then(valid => {
if (!valid) { return; }
if (!this._isValidElForComment(el)) { return; }
const value = el.getAttribute('data-value');
let lineNum;
if (value !== GrDiffLine.FILE) {
lineNum = parseInt(value, 10);
if (isNaN(lineNum)) {
this.fire('show-alert', {message: ERR_INVALID_LINE + value});
return;
}
const value = el.getAttribute('data-value');
let lineNum;
if (value !== GrDiffLine.FILE) {
lineNum = parseInt(value, 10);
if (isNaN(lineNum)) {
this.fire('show-alert', {message: ERR_INVALID_LINE + value});
return;
}
this._createComment(el, lineNum);
});
}
this._createComment(el, lineNum);
},
_handleCreateComment(e) {
@@ -409,36 +414,34 @@
const side = e.detail.side;
const lineNum = range.endLine;
const lineEl = this.$.diffBuilder.getLineElByNumber(lineNum, side);
this._isValidElForComment(lineEl).then(valid => {
if (!valid) { return; }
if (this._isValidElForComment(lineEl)) {
this._createComment(lineEl, lineNum, side, range);
});
}
},
/** @return {boolean} */
_isValidElForComment(el) {
return this._getLoggedIn().then(loggedIn => {
if (!loggedIn) {
this.fire('show-auth-required');
return false;
}
const patchNum = el.classList.contains(DiffSide.LEFT) ?
this.patchRange.basePatchNum :
this.patchRange.patchNum;
if (!this._loggedIn) {
this.fire('show-auth-required');
return false;
}
const patchNum = el.classList.contains(DiffSide.LEFT) ?
this.patchRange.basePatchNum :
this.patchRange.patchNum;
const isEdit = this.patchNumEquals(patchNum, this.EDIT_NAME);
const isEditBase = this.patchNumEquals(patchNum, this.PARENT_NAME) &&
this.patchNumEquals(this.patchRange.patchNum, this.EDIT_NAME);
const isEdit = this.patchNumEquals(patchNum, this.EDIT_NAME);
const isEditBase = this.patchNumEquals(patchNum, this.PARENT_NAME) &&
this.patchNumEquals(this.patchRange.patchNum, this.EDIT_NAME);
if (isEdit) {
this.fire('show-alert', {message: ERR_COMMENT_ON_EDIT});
return false;
} else if (isEditBase) {
this.fire('show-alert', {message: ERR_COMMENT_ON_EDIT_BASE});
return false;
}
return true;
});
if (isEdit) {
this.fire('show-alert', {message: ERR_COMMENT_ON_EDIT});
return false;
} else if (isEditBase) {
this.fire('show-alert', {message: ERR_COMMENT_ON_EDIT_BASE});
return false;
}
return true;
},
/**
@@ -709,6 +712,7 @@
},
_renderDiffTable() {
if (!this.prefs) { return Promise.resolve(); }
if (this.prefs.context === -1 &&
this._diffLength(this._diff) >= LARGE_DIFF_THRESHOLD_LINES &&
this._safetyBypass === null) {
@@ -758,7 +762,7 @@
_getDiff() {
// 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) => {
return new Promise((resolve, reject) => {
this.$.restAPI.getDiff(
this.changeNum,
this.patchRange.basePatchNum,
@@ -767,18 +771,6 @@
reject)
.then(resolve);
});
return request
.then(diff => {
this.filesWeblinks = this._getFilesWeblinks(diff);
this._diff = diff;
this._reportDiff(diff);
return diff;
})
.catch(e => {
this._handleGetDiffError(e);
return null;
});
},
_getFilesWeblinks(diff) {
@@ -837,22 +829,28 @@
return this.$.restAPI.getLoggedIn();
},
/** @return {boolean} */
_computeIsImageDiff() {
if (!this._diff) { return false; }
/**
* @param {Object} diff
* @return {boolean}
*/
_computeIsImageDiff(diff) {
if (!diff) { return false; }
const isA = this._diff.meta_a &&
this._diff.meta_a.content_type.startsWith('image/');
const isB = this._diff.meta_b &&
this._diff.meta_b.content_type.startsWith('image/');
const isA = diff.meta_a &&
diff.meta_a.content_type.startsWith('image/');
const isB = diff.meta_b &&
diff.meta_b.content_type.startsWith('image/');
return !!(this._diff.binary && (isA || isB));
return !!(diff.binary && (isA || isB));
},
/** @return {!Promise} */
_loadDiffAssets() {
if (this.isImageDiff) {
return this._getImages().then(images => {
/**
* @param {Object} diff
* @return {!Promise}
*/
_loadDiffAssets(diff) {
if (this._computeIsImageDiff(diff)) {
return this._getImages(diff).then(images => {
this._baseImage = images.baseImage;
this._revisionImage = images.revisionImage;
});
@@ -863,9 +861,12 @@
}
},
/** @return {!Promise} */
_getImages() {
return this.$.restAPI.getImagesForDiff(this.changeNum, this._diff,
/**
* @param {Object} diff
* @return {!Promise}
*/
_getImages(diff) {
return this.$.restAPI.getImagesForDiff(this.changeNum, diff,
this.patchRange);
},

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

@@ -172,10 +172,12 @@ limitations under the License.
suite('not logged in', () => {
setup(() => {
const getLoggedInPromise = Promise.resolve(false);
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(false); },
getLoggedIn() { return getLoggedInPromise; },
});
element = fixture('basic');
return getLoggedInPromise;
});
test('toggleLeftDiff', () => {
@@ -185,15 +187,12 @@ limitations under the License.
assert.isFalse(element.classList.contains('no-left'));
});
test('addDraftAtLine', done => {
test('addDraftAtLine', () => {
sandbox.stub(element, '_selectLine');
const loggedInErrorSpy = sandbox.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
element.addDraftAtLine();
flush(() => {
assert.isTrue(loggedInErrorSpy.called);
done();
});
assert.isTrue(loggedInErrorSpy.called);
});
test('view does not start with displayLine classList', () => {
@@ -212,12 +211,14 @@ limitations under the License.
test('loads files weblinks', () => {
const weblinksStub = sandbox.stub(Gerrit.Nav, '_generateWeblinks')
.returns({name: 'stubb', url: '#s'});
sandbox.stub(element.$.restAPI, 'getDiff').returns(Promise.resolve({}));
sandbox.stub(element.$.restAPI, 'getDiff').returns(Promise.resolve({
content: [],
}));
element.projectName = 'test-project';
element.path = 'test-path';
element.commitRange = {baseCommit: 'test-base', commit: 'test-commit'};
element.patchRange = {};
return element._getDiff().then(() => {
return element.reload().then(() => {
assert.isTrue(weblinksStub.calledTwice);
assert.isTrue(weblinksStub.firstCall.calledWith({
commit: 'test-base',
@@ -778,15 +779,14 @@ limitations under the License.
element._getDiff().then(done);
});
test('_getDiff resolves as null on error', () => {
test('reload resolves 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);
return element.reload().then(() => {
assert.isTrue(onErrStub.calledOnce);
});
});
@@ -878,8 +878,9 @@ limitations under the License.
suite('logged in', () => {
let fakeLineEl;
setup(() => {
const getLoggedInPromise = Promise.resolve(true);
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(true); },
getLoggedIn() { return getLoggedInPromise; },
getPreferences() {
return Promise.resolve({time_format: 'HHMM_12'});
},
@@ -894,23 +895,21 @@ limitations under the License.
contains: sandbox.stub().returns(true),
},
};
return getLoggedInPromise;
});
test('addDraftAtLine', done => {
test('addDraftAtLine', () => {
sandbox.stub(element, '_selectLine');
sandbox.stub(element, '_createComment');
const loggedInErrorSpy = sandbox.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
assert.isFalse(loggedInErrorSpy.called);
element.addDraftAtLine(fakeLineEl);
flush(() => {
assert.isFalse(loggedInErrorSpy.called);
assert.isTrue(element._createComment
.calledWithExactly(fakeLineEl, 42));
done();
});
assert.isTrue(element._createComment
.calledWithExactly(fakeLineEl, 42));
});
test('addDraftAtLine on an edit', done => {
test('addDraftAtLine on an edit', () => {
element.patchRange.basePatchNum = element.EDIT_NAME;
sandbox.stub(element, '_selectLine');
sandbox.stub(element, '_createComment');
@@ -919,15 +918,12 @@ limitations under the License.
element.addEventListener('show-auth-required', loggedInErrorSpy);
element.addEventListener('show-alert', alertSpy);
element.addDraftAtLine(fakeLineEl);
flush(() => {
assert.isFalse(loggedInErrorSpy.called);
assert.isTrue(alertSpy.called);
assert.isFalse(element._createComment.called);
done();
});
assert.isFalse(loggedInErrorSpy.called);
assert.isTrue(alertSpy.called);
assert.isFalse(element._createComment.called);
});
test('addDraftAtLine on an edit base', done => {
test('addDraftAtLine on an edit base', () => {
element.patchRange.patchNum = element.EDIT_NAME;
element.patchRange.basePatchNum = element.PARENT_NAME;
sandbox.stub(element, '_selectLine');
@@ -937,12 +933,9 @@ limitations under the License.
element.addEventListener('show-auth-required', loggedInErrorSpy);
element.addEventListener('show-alert', alertSpy);
element.addDraftAtLine(fakeLineEl);
flush(() => {
assert.isFalse(loggedInErrorSpy.called);
assert.isTrue(alertSpy.called);
assert.isFalse(element._createComment.called);
done();
});
assert.isFalse(loggedInErrorSpy.called);
assert.isTrue(alertSpy.called);
assert.isFalse(element._createComment.called);
});
suite('change in preferences', () => {