Disable diff comments on edit patchsets
Includes a minor refactor of gr-diff comment construction logic.
Bug: Issue 4437
Change-Id: I0428fead7a9d1f1dc5d6aa9efc3d81ecbe6b5c64
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 7135601..7ef5c60 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -15,6 +15,8 @@
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
+
+<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../gr-diff-builder/gr-diff-builder.html">
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 94f60d9..728a766 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -14,6 +14,9 @@
(function() {
'use strict';
+ const ERR_COMMENT_ON_EDIT = 'You cannot comment on an edit.';
+ const ERR_INVALID_LINE = 'Invalid line number: ';
+
const DiffViewMode = {
SIDE_BY_SIDE: 'SIDE_BY_SIDE',
UNIFIED: 'UNIFIED_DIFF',
@@ -79,10 +82,6 @@
},
noRenderOnPrefsChange: Boolean,
comments: Object,
- _loggedIn: {
- type: Boolean,
- value: false,
- },
lineWrapping: {
type: Boolean,
value: false,
@@ -93,6 +92,10 @@
value: DiffViewMode.SIDE_BY_SIDE,
observer: '_viewModeObserver',
},
+ _loggedIn: {
+ type: Boolean,
+ value: false,
+ },
_diff: Object,
_diffHeaderItems: {
type: Array,
@@ -120,6 +123,10 @@
_showWarning: Boolean,
},
+ behaviors: [
+ Gerrit.PatchSetBehavior,
+ ],
+
listeners: {
'thread-discard': '_handleThreadDiscard',
'comment-discard': '_handleCommentDiscard',
@@ -169,27 +176,6 @@
return Polymer.dom(this.root).querySelectorAll('.diff-row');
},
- addDraftAtLine(el) {
- this._selectLine(el);
- this._getLoggedIn().then(loggedIn => {
- if (!loggedIn) {
- this.fire('show-auth-required');
- return;
- }
-
- const value = el.getAttribute('data-value');
- if (value === GrDiffLine.FILE) {
- this._addDraft(el);
- return;
- }
- const lineNum = parseInt(value, 10);
- if (isNaN(lineNum)) {
- throw Error('Invalid line number: ' + value);
- }
- this._addDraft(el, lineNum);
- });
- },
-
isRangeSelected() {
return this.$.highlights.isRangeSelected();
},
@@ -254,34 +240,65 @@
});
},
- _handleCreateComment(e) {
- const range = e.detail.range;
- const diffSide = e.detail.side;
- const line = range.endLine;
- const lineEl = this.$.diffBuilder.getLineElByNumber(line, diffSide);
- const contentText = this.$.diffBuilder.getContentByLineEl(lineEl);
- const contentEl = contentText.parentElement;
- const patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl);
- const isOnParent =
- this._getIsParentCommentByLineAndContent(lineEl, contentEl);
- const threadEl = this._getOrCreateThreadAtLineRange(contentEl, patchNum,
- diffSide, isOnParent, range);
+ addDraftAtLine(el) {
+ this._selectLine(el);
+ this._isValidElForComment(el).then(valid => {
+ if (!valid) { return; }
- threadEl.addOrEditDraft(line, range);
+ 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);
+ });
},
- _addDraft(lineEl, opt_lineNum) {
+ _handleCreateComment(e) {
+ const range = e.detail.range;
+ const side = e.detail.side;
+ const lineNum = range.endLine;
+ const lineEl = this.$.diffBuilder.getLineElByNumber(lineNum, side);
+ this._isValidElForComment(el).then(valid => {
+ if (!valid) { return; }
+
+ this._createComment(lineEl, lineNum, side, range);
+ });
+ },
+
+ _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.patchNumEquals(patchNum, this.EDIT_NAME)) {
+ this.fire('show-alert', {message: ERR_COMMENT_ON_EDIT});
+ return false;
+ }
+ return true;
+ });
+ },
+
+ _createComment(lineEl, opt_lineNum, opt_side, opt_range) {
const contentText = this.$.diffBuilder.getContentByLineEl(lineEl);
const contentEl = contentText.parentElement;
- const patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl);
- const commentSide =
+ const side = opt_side ||
this._getCommentSideByLineAndContent(lineEl, contentEl);
+ const patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl);
const isOnParent =
- this._getIsParentCommentByLineAndContent(lineEl, contentEl);
+ this._getIsParentCommentByLineAndContent(lineEl, contentEl);
const threadEl = this._getOrCreateThreadAtLineRange(contentEl, patchNum,
- commentSide, isOnParent);
-
- threadEl.addOrEditDraft(opt_lineNum);
+ side, isOnParent);
+ threadEl.addOrEditDraft(opt_lineNum, opt_range);
},
_getThreadForRange(threadGroupEl, rangeToCheck) {
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 8077e3a..8e790a0 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
@@ -713,6 +713,7 @@
});
suite('logged in', () => {
+ let fakeLineEl;
setup(() => {
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(true); },
@@ -721,18 +722,43 @@
},
});
element = fixture('basic');
+ element.patchRange = {};
+
+ fakeLineEl = {
+ getAttribute: sandbox.stub().returns(42),
+ classList: {
+ contains: sandbox.stub().returns(true),
+ },
+ };
});
test('addDraftAtLine', done => {
- const fakeLineEl = {getAttribute: sandbox.stub().returns(42)};
sandbox.stub(element, '_selectLine');
- sandbox.stub(element, '_addDraft');
+ sandbox.stub(element, '_createComment');
const loggedInErrorSpy = sandbox.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
element.addDraftAtLine(fakeLineEl);
flush(() => {
assert.isFalse(loggedInErrorSpy.called);
- assert.isTrue(element._addDraft.calledWithExactly(fakeLineEl, 42));
+ assert.isTrue(element._createComment
+ .calledWithExactly(fakeLineEl, 42));
+ done();
+ });
+ });
+
+ test.only('addDraftAtLine on an edit', done => {
+ element.patchRange.basePatchNum = element.EDIT_NAME;
+ sandbox.stub(element, '_selectLine');
+ sandbox.stub(element, '_createComment');
+ const loggedInErrorSpy = sandbox.spy();
+ const alertSpy = sandbox.spy();
+ 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();
});
});
