Move check if we can comment to gr-diff-host
This will make gr-diff oblivious of PatchRanges. This makes it easier to reuse
gr-diff independently of the underlying versioning system.
This also makes the check more precise:
Before this change, we would only check if we could comment on the side
the user had selected - not on the side the comment will actually end up
on.
After this change, we check on the side the comment will end up on -
which may be different in cases of removes, (merge) parents and edit.
Opening inline diff and opening the diff in file view now consistenly
use the patchRange stored in the gr-file-list.
Change-Id: Ia62e9ef20b279aa4e0c5481c9bd5bf7d20f47cab
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 243c500..d41a662 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -1069,19 +1069,13 @@
_openCursorFile() {
const diff = this.$.diffCursor.getTargetDiffElement();
- if (
- !this.change ||
- !diff ||
- !this.patchRange ||
- !diff.path ||
- !diff.patchRange
- ) {
+ if (!this.change || !diff || !this.patchRange || !diff.path) {
throw new Error('change, diff and patchRange must be all set and valid');
}
GerritNav.navigateToDiff(
this.change,
diff.path,
- diff.patchRange.patchNum,
+ this.patchRange.patchNum,
this.patchRange.basePatchNum
);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index eaa9029..57df41c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -49,7 +49,9 @@
BlameInfo,
ChangeInfo,
CommentRange,
+ EditPatchSetNum,
NumericChangeId,
+ ParentPatchSetNum,
PatchRange,
PatchSetNum,
RepoName,
@@ -752,7 +754,7 @@
side === Side.LEFT && isAParent(this.patchRange.basePatchNum)
? CommentSide.PARENT
: CommentSide.REVISION;
-
+ if (!this.canCommentOnPatchSetNum(patchNum)) return;
const threadEl = this._getOrCreateThread(
patchNum,
lineNum,
@@ -766,6 +768,32 @@
this.reporting.recordDraftInteraction();
}
+ private canCommentOnPatchSetNum(patchNum: PatchSetNum) {
+ if (!this._loggedIn) {
+ fireEvent(this, 'show-auth-required');
+ return false;
+ }
+ if (!this.patchRange) {
+ fireAlert(this, 'Cannot create comment. patchRange undefined.');
+ return false;
+ }
+
+ const isEdit = patchNum === EditPatchSetNum;
+ const isEditBase =
+ patchNum === ParentPatchSetNum &&
+ this.patchRange.patchNum === EditPatchSetNum;
+
+ if (isEdit) {
+ fireAlert(this, 'You cannot comment on an edit.');
+ return false;
+ }
+ if (isEditBase) {
+ fireAlert(this, 'You cannot comment on the base patchset of an edit.');
+ return false;
+ }
+ return true;
+ }
+
/**
* Gets or creates a comment thread at a given location.
* May provide a range, to get/create a range comment.
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts
index a7eebf4..9d5555d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts
@@ -21,7 +21,6 @@
id="diff"
change-num="[[changeNum]]"
no-auto-render="[[noAutoRender]]"
- patch-range="[[patchRange]]"
path="[[path]]"
prefs="[[prefs]]"
display-line="[[displayLine]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index db0b65a..99486ad 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -26,6 +26,7 @@
import {CoverageType} from '../../../types/types.js';
import {addListenerForTest, stubRestApi} from '../../../test/test-utils.js';
import {createDefaultDiffPrefs} from '../../../constants/constants.js';
+import {EditPatchSetNum, ParentPatchSetNum} from '../../../types/common.js';
const basicFixture = fixtureFromElement('gr-diff-host');
@@ -659,6 +660,30 @@
});
});
+ test('cannot create comments when not logged in', () => {
+ element.patchRange = {
+ basePatchNum: 'PARENT',
+ patchNum: 2,
+ };
+ const showAuthRequireSpy = sinon.spy();
+ element.addEventListener('show-auth-required', showAuthRequireSpy);
+
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ lineNum: 3,
+ side: Side.LEFT,
+ path: '/p',
+ },
+ }));
+
+ const threads = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread');
+
+ assert.equal(threads.length, 0);
+
+ assert.isTrue(showAuthRequireSpy.called);
+ });
+
test('delegates cancel()', () => {
const stub = sinon.stub(element.$.diff, 'cancel');
element.patchRange = {};
@@ -789,12 +814,6 @@
assert.equal(element.$.diff.noAutoRender, value);
});
- test('passes in patchRange', () => {
- const value = {patchNum: 'foo', basePatchNum: 'bar'};
- element.patchRange = value;
- assert.equal(element.$.diff.patchRange, value);
- });
-
test('passes in path', () => {
const value = 'some/file/path';
element.path = value;
@@ -949,6 +968,12 @@
});
suite('create-comment', () => {
+ setup(async () => {
+ loggedIn = true;
+ element.attached();
+ await flush();
+ });
+
test('creates comments if they do not exist yet', () => {
const diffSide = Side.LEFT;
element.patchRange = {
@@ -1147,6 +1172,50 @@
assert.equal(threads[0].diffSide, diffSide);
assert.equal(threads[0].path, element.file.path);
});
+
+ test('cannot create thread on an edit', () => {
+ const alertSpy = sinon.spy();
+ element.addEventListener('show-alert', alertSpy);
+
+ const diffSide = Side.LEFT;
+ element.patchRange = {
+ basePatchNum: EditPatchSetNum,
+ patchNum: 3,
+ };
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ side: diffSide,
+ path: '/p',
+ },
+ }));
+
+ const threads = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread');
+ assert.equal(threads.length, 0);
+ assert.isTrue(alertSpy.called);
+ });
+
+ test('cannot create thread on an edit base', () => {
+ const alertSpy = sinon.spy();
+ element.addEventListener('show-alert', alertSpy);
+
+ const diffSide = Side.LEFT;
+ element.patchRange = {
+ basePatchNum: ParentPatchSetNum,
+ patchNum: EditPatchSetNum,
+ };
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ side: diffSide,
+ path: '/p',
+ },
+ }));
+
+ const threads = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread');
+ assert.equal(threads.length, 0);
+ assert.isTrue(alertSpy.called);
+ });
});
test('_filterThreadElsForLocation with no threads', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index 4793fe3..e85564a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -41,14 +41,7 @@
} from './gr-diff-utils';
import {getHiddenScroll} from '../../../scripts/hiddenscroll';
import {customElement, observe, property} from '@polymer/decorators';
-import {
- BlameInfo,
- CommentRange,
- EditPatchSetNum,
- ImageInfo,
- ParentPatchSetNum,
- PatchRange,
-} from '../../../types/common';
+import {BlameInfo, CommentRange, ImageInfo} from '../../../types/common';
import {
DiffInfo,
DiffPreferencesInfo,
@@ -152,9 +145,6 @@
@property({type: Boolean})
noAutoRender = false;
- @property({type: Object})
- patchRange?: PatchRange;
-
@property({type: String, observer: '_pathObserver'})
path?: string;
@@ -564,9 +554,6 @@
addDraftAtLine(el: Element) {
this._selectLine(el);
- if (!this._isValidElForComment(el)) {
- return;
- }
const lineNum = getLineNumber(el);
if (lineNum === null) {
@@ -590,7 +577,7 @@
_createCommentForSelection(side: Side, range: CommentRange) {
const lineNum = range.end_line;
const lineEl = this.$.diffBuilder.getLineElByNumber(lineNum, side);
- if (lineEl && this._isValidElForComment(lineEl)) {
+ if (lineEl) {
this._createComment(lineEl, lineNum, side, range);
}
}
@@ -601,35 +588,6 @@
this._createCommentForSelection(side, range);
}
- _isValidElForComment(el: Element) {
- if (!this.loggedIn) {
- fireEvent(this, 'show-auth-required');
- return false;
- }
- if (!this.patchRange) {
- fireAlert(this, 'Cannot create comment. Patch range undefined.');
- return false;
- }
- const patchNum = el.classList.contains(Side.LEFT)
- ? this.patchRange.basePatchNum
- : this.patchRange.patchNum;
-
- const isEdit = patchNum === EditPatchSetNum;
- const isEditBase =
- patchNum === ParentPatchSetNum &&
- this.patchRange.patchNum === EditPatchSetNum;
-
- if (isEdit) {
- fireAlert(this, 'You cannot comment on an edit.');
- return false;
- }
- if (isEditBase) {
- fireAlert(this, 'You cannot comment on the base patchset of an edit.');
- return false;
- }
- return true;
- }
-
_createComment(
lineEl: Element,
lineNum?: LineNumber,
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
index ae5a5d2..d5cee25 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
@@ -24,7 +24,6 @@
import {runA11yAudit} from '../../../test/a11y-test-utils.js';
import '@polymer/paper-button/paper-button.js';
import {stubRestApi} from '../../../test/test-utils.js';
-import {EditPatchSetNum, ParentPatchSetNum} from '../../../types/common.js';
const basicFixture = fixtureFromElement('gr-diff');
@@ -102,14 +101,6 @@
assert.isFalse(element.classList.contains('no-left'));
});
- test('addDraftAtLine', () => {
- sinon.stub(element, '_selectLine');
- const loggedInErrorSpy = sinon.spy();
- element.addEventListener('show-auth-required', loggedInErrorSpy);
- element.addDraftAtLine();
- assert.isTrue(loggedInErrorSpy.called);
- });
-
test('view does not start with displayLine classList', () => {
assert.isFalse(
element.shadowRoot
@@ -567,29 +558,6 @@
.calledWithExactly(fakeLineEl, 42));
});
- test('addDraftAtLine on an edit', () => {
- element.patchRange.basePatchNum = EditPatchSetNum;
- sinon.stub(element, '_selectLine');
- sinon.stub(element, '_createComment');
- const alertSpy = sinon.spy();
- element.addEventListener('show-alert', alertSpy);
- element.addDraftAtLine(fakeLineEl);
- assert.isTrue(alertSpy.called);
- assert.isFalse(element._createComment.called);
- });
-
- test('addDraftAtLine on an edit base', () => {
- element.patchRange.patchNum = EditPatchSetNum;
- element.patchRange.basePatchNum = ParentPatchSetNum;
- sinon.stub(element, '_selectLine');
- sinon.stub(element, '_createComment');
- const alertSpy = sinon.spy();
- element.addEventListener('show-alert', alertSpy);
- element.addDraftAtLine(fakeLineEl);
- assert.isTrue(alertSpy.called);
- assert.isFalse(element._createComment.called);
- });
-
test('adds long range comment chip', async () => {
const range = {
start_line: 1,
