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/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', () => {
