opendev/gerrit
Code Issues Proposed changes

Make log-in check when adding comment synchronous

Browse Source 
The log-in state is fetched on attach and very fast, making it
basically impossible in practice to add a comment before it returns.

Making it synchronous simplifies the code, and makes it easier to later
pass in the log-in state as a property rather than fetching it here
when we split all the restAPI calls into a wrapper component.

Bug: Issue 9623
Change-Id: Id7e7b284eb73185371805315402cbb96aa31ac6b
This commit is contained in:
Ole Rehmsen
2018-08-23 13:12:37 +02:00
parent 9e98b0b03e
commit a1fdc504cb
2 changed files with 51 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -395,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) {
@@ -416,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;
},
/**

48
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', () => {
@@ -879,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'});
},
@@ -895,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');
@@ -920,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');
@@ -938,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', () => {