Merge "Lower the frequency of the respectful tips"
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index b60cce5..566308d 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -48,6 +48,7 @@
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
@@ -85,6 +86,7 @@
@Inject private Provider<ChangesCollection> changes;
@Inject private Provider<PostReview> postReview;
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private CommentsUtil commentsUtil;
private final Integer[] lines = {0, 1};
@@ -446,6 +448,82 @@
}
@Test
+ public void putDraft_idMismatch() throws Exception {
+ String file = "file";
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ DraftInput comment = newDraft(file, Side.REVISION, 0, "foo");
+ CommentInfo commentInfo = addDraft(changeId, revId, comment);
+ DraftInput draftInput = newDraft(file, Side.REVISION, 0, "bar");
+ draftInput.id = "anything_but_" + commentInfo.id;
+ BadRequestException e =
+ assertThrows(
+ BadRequestException.class,
+ () -> updateDraft(changeId, revId, draftInput, commentInfo.id));
+ assertThat(e).hasMessageThat().contains("id must match URL");
+ }
+
+ @Test
+ public void putDraft_negativeLine() throws Exception {
+ String file = "file";
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ DraftInput comment = newDraft(file, Side.REVISION, -666, "foo");
+ BadRequestException e =
+ assertThrows(BadRequestException.class, () -> addDraft(changeId, revId, comment));
+ assertThat(e).hasMessageThat().contains("line must be >= 0");
+ }
+
+ @Test
+ public void putDraft_invalidRange() throws Exception {
+ String file = "file";
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ DraftInput draftInput = newDraft(file, Side.REVISION, createLineRange(2, 3), "bar");
+ draftInput.line = 666;
+ BadRequestException e =
+ assertThrows(BadRequestException.class, () -> addDraft(changeId, revId, draftInput));
+ assertThat(e)
+ .hasMessageThat()
+ .contains("range endLine must be on the same line as the comment");
+ }
+
+ @Test
+ public void putDraft_updatePath() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ DraftInput comment = newDraft("file_foo", Side.REVISION, 0, "foo");
+ CommentInfo commentInfo = addDraft(changeId, revId, comment);
+ assertThat(getDraftComments(changeId, revId).keySet()).containsExactly("file_foo");
+ DraftInput draftInput = newDraft("file_bar", Side.REVISION, 0, "bar");
+ updateDraft(changeId, revId, draftInput, commentInfo.id);
+ assertThat(getDraftComments(changeId, revId).keySet()).containsExactly("file_bar");
+ }
+
+ @Test
+ public void putDraft_updateInReplyToAndTag() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ DraftInput draftInput1 = newDraft(FILE_NAME, Side.REVISION, 0, "foo");
+ CommentInfo commentInfo = addDraft(changeId, revId, draftInput1);
+ DraftInput draftInput2 = newDraft(FILE_NAME, Side.REVISION, 0, "bar");
+ String inReplyTo = "in_reply_to";
+ String tag = "täg";
+ draftInput2.inReplyTo = inReplyTo;
+ draftInput2.tag = tag;
+ updateDraft(changeId, revId, draftInput2, commentInfo.id);
+ com.google.gerrit.entities.Comment comment =
+ Iterables.getOnlyElement(commentsUtil.draftByChange(r.getChange().notes()));
+ assertThat(comment.parentUuid).isEqualTo(inReplyTo);
+ assertThat(comment.tag).isEqualTo(tag);
+ }
+
+ @Test
public void listDrafts() throws Exception {
String file = "file";
PushOneCommit.Result r = createChange();
@@ -677,15 +755,15 @@
addDraft(
r1.getChangeId(),
r1.getCommit().getName(),
- newDraft(FILE_NAME, Side.REVISION, createLineRange(1, 4, 10), "Is it that bad?"));
+ newDraft(FILE_NAME, Side.REVISION, createLineRange(4, 10), "Is it that bad?"));
addDraft(
r1.getChangeId(),
r1.getCommit().getName(),
- newDraft(FILE_NAME, Side.PARENT, createLineRange(1, 0, 7), "what happened to this?"));
+ newDraft(FILE_NAME, Side.PARENT, createLineRange(0, 7), "what happened to this?"));
addDraft(
r2.getChangeId(),
r2.getCommit().getName(),
- newDraft(FILE_NAME, Side.REVISION, createLineRange(1, 4, 15), "better now"));
+ newDraft(FILE_NAME, Side.REVISION, createLineRange(4, 15), "better now"));
addDraft(
r2.getChangeId(),
r2.getCommit().getName(),
@@ -905,7 +983,7 @@
@Test
public void deleteCommentCannotBeAppliedByUser() throws Exception {
PushOneCommit.Result result = createChange();
- CommentInput targetComment = addComment(result.getChangeId(), "My password: abc123");
+ CommentInput targetComment = addComment(result.getChangeId());
Map<String, List<CommentInfo>> commentsMap =
getPublishedComments(result.getChangeId(), result.getCommit().name());
@@ -1083,9 +1161,9 @@
.collect(toList());
}
- private CommentInput addComment(String changeId, String message) throws Exception {
+ private CommentInput addComment(String changeId) throws Exception {
ReviewInput input = new ReviewInput();
- CommentInput comment = newComment(FILE_NAME, Side.REVISION, 0, message, false);
+ CommentInput comment = newComment(FILE_NAME, Side.REVISION, 0, "a message", false);
input.comments = ImmutableMap.of(comment.path, Lists.newArrayList(comment));
gApi.changes().id(changeId).current().review(input);
return comment;
@@ -1240,7 +1318,7 @@
private static CommentInput newCommentOnParent(
String path, int parent, int line, String message) {
CommentInput c = new CommentInput();
- return populate(c, path, Side.PARENT, Integer.valueOf(parent), line, message, false);
+ return populate(c, path, Side.PARENT, parent, line, message, false);
}
private DraftInput newDraft(String path, Side side, int line, String message) {
@@ -1255,7 +1333,7 @@
private DraftInput newDraftOnParent(String path, int parent, int line, String message) {
DraftInput d = new DraftInput();
- return populate(d, path, Side.PARENT, Integer.valueOf(parent), line, message, false);
+ return populate(d, path, Side.PARENT, parent, line, message, false);
}
private static <C extends Comment> C populate(
@@ -1284,11 +1362,11 @@
return populate(c, path, side, parent, line, null, message, unresolved);
}
- private static Comment.Range createLineRange(int line, int startChar, int endChar) {
+ private static Comment.Range createLineRange(int startChar, int endChar) {
Comment.Range range = new Comment.Range();
- range.startLine = line;
+ range.startLine = 1;
range.startCharacter = startChar;
- range.endLine = line;
+ range.endLine = 1;
range.endCharacter = endChar;
return range;
}
diff --git a/plugins/download-commands b/plugins/download-commands
index 5c50f9b..3f5a024 160000
--- a/plugins/download-commands
+++ b/plugins/download-commands
@@ -1 +1 @@
-Subproject commit 5c50f9b17e616fd84d2c561822161fff46bbf902
+Subproject commit 3f5a024fd46f30f4646bfceb285763e44fda15a7
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 5bca7be..a3076f0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -737,14 +737,18 @@
this._prefsChanged(this.prefs);
}
+ _cleanup() {
+ this.cancel();
+ this._blame = null;
+ this._safetyBypass = null;
+ this._showWarning = false;
+ this.clearDiffContent();
+ }
+
/** @param {boolean} newValue */
_loadingChanged(newValue) {
if (newValue) {
- this.cancel();
- this._blame = null;
- this._safetyBypass = null;
- this._showWarning = false;
- this.clearDiffContent();
+ this._cleanup();
}
}
@@ -785,6 +789,7 @@
_diffChanged(newValue) {
if (newValue) {
+ this._cleanup();
this._diffLength = this.getDiffLength(newValue);
this._debounceRenderDiffTable();
}
@@ -806,7 +811,6 @@
}
_renderDiffTable() {
- this._unobserveIncrementalNodes();
if (!this.prefs) {
this.dispatchEvent(
new CustomEvent('render', {bubbles: true, composed: true}));
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 18b4e09..08576a2 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
@@ -952,47 +952,68 @@
});
});
});
+ const setupSampleDiff = function(params) {
+ const {ignore_whitespace, content} = params;
+ element = fixture('basic');
+ element.prefs = {
+ ignore_whitespace: ignore_whitespace || 'IGNORE_ALL',
+ auto_hide_diff_table_header: true,
+ context: 10,
+ cursor_blink_rate: 0,
+ font_size: 12,
+ intraline_difference: true,
+ line_length: 100,
+ line_wrapping: false,
+ show_line_endings: true,
+ show_tabs: true,
+ show_whitespace_errors: true,
+ syntax_highlighting: true,
+ tab_size: 8,
+ theme: 'DEFAULT',
+ };
+ element.diff = {
+ intraline_status: 'OK',
+ change_type: 'MODIFIED',
+ diff_header: [
+ 'diff --git a/carrot.js b/carrot.js',
+ 'index 2adc47d..f9c2f2c 100644',
+ '--- a/carrot.js',
+ '+++ b/carrot.jjs',
+ 'file differ',
+ ],
+ content,
+ binary: false,
+ };
+ element._renderDiffTable();
+ flushAsynchronousOperations();
+ };
+
+ test('clear diff table content as soon as diff changes', () => {
+ const content = [{
+ a: ['all work and no play make andybons a dull boy'],
+ }, {
+ b: [
+ 'Non eram nescius, Brute, cum, quae summis ingeniis ',
+ ],
+ }];
+ function assertDiffTableWithContent() {
+ assert.isTrue(element.$.diffTable.innerText.includes(content[0].a));
+ }
+ setupSampleDiff({content});
+ assertDiffTableWithContent();
+ const diffCopy = Object.assign({}, element.diff);
+ element.diff = diffCopy;
+ // immediatelly cleaned up
+ assert.equal(element.$.diffTable.innerHTML, '');
+ element._renderDiffTable();
+ flushAsynchronousOperations();
+ // rendered again
+ assertDiffTableWithContent();
+ });
suite('whitespace changes only message', () => {
- const setupDiff = function(ignore_whitespace, diffContent) {
- element = fixture('basic');
- element.prefs = {
- ignore_whitespace,
- auto_hide_diff_table_header: true,
- context: 10,
- cursor_blink_rate: 0,
- font_size: 12,
- intraline_difference: true,
- line_length: 100,
- line_wrapping: false,
- show_line_endings: true,
- show_tabs: true,
- show_whitespace_errors: true,
- syntax_highlighting: true,
- tab_size: 8,
- theme: 'DEFAULT',
- };
-
- element.diff = {
- intraline_status: 'OK',
- change_type: 'MODIFIED',
- diff_header: [
- 'diff --git a/carrot.js b/carrot.js',
- 'index 2adc47d..f9c2f2c 100644',
- '--- a/carrot.js',
- '+++ b/carrot.jjs',
- 'file differ',
- ],
- content: diffContent,
- binary: true,
- };
-
- element._renderDiffTable();
- flushAsynchronousOperations();
- };
-
test('show the message if ignore_whitespace is criteria matches', () => {
- setupDiff('IGNORE_ALL', [{skip: 100}]);
+ setupSampleDiff({content: [{skip: 100}]});
assert.isTrue(element.showNoChangeMessage(
/* loading= */ false,
element.prefs,
@@ -1001,7 +1022,7 @@
});
test('do not show the message if still loading', () => {
- setupDiff('IGNORE_ALL', [{skip: 100}]);
+ setupSampleDiff({content: [{skip: 100}]});
assert.isFalse(element.showNoChangeMessage(
/* loading= */ true,
element.prefs,
@@ -1019,7 +1040,7 @@
'exquisitaque doctrina philosophi Graeco sermone tractavissent',
],
}];
- setupDiff('IGNORE_ALL', content);
+ setupSampleDiff({content});
assert.equal(element._diffLength, 3);
assert.isFalse(element.showNoChangeMessage(
/* loading= */ false,
@@ -1038,7 +1059,7 @@
'exquisitaque doctrina philosophi Graeco sermone tractavissent',
],
}];
- setupDiff('IGNORE_NONE', content);
+ setupSampleDiff({ignore_whitespace: 'IGNORE_NONE', content});
assert.isFalse(element.showNoChangeMessage(
/* loading= */ false,
element.prefs,
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.html b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.html
index 7a5f4c6..be0f2b2 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.html
@@ -24,20 +24,8 @@
<script src="/node_modules/@webcomponents/webcomponentsjs/webcomponents-lite.js"></script>
<script src="/components/wct-browser-legacy/browser.js"></script>
-<script type="module" src="../../../test/test-pre-setup.js"></script>
-<script type="module" src="../../../test/common-test-setup.js"></script>
<script src="../../../node_modules/iron-test-helpers/mock-interactions.js" type="module"></script>
-<script type="module" src="./gr-hovercard-account.js"></script>
-
-<script type="module">
- import '../../../test/test-pre-setup.js';
- import '../../../test/common-test-setup.js';
- import './gr-hovercard-account.js';
-
- void (0);
-</script>
-
<test-fixture id="basic">
<template>
<gr-hovercard-account class="hovered"></gr-hovercard-account>
@@ -46,7 +34,6 @@
<script type="module">
- import '../../../test/test-pre-setup.js';
import '../../../test/common-test-setup.js';
import './gr-hovercard-account.js';
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.html
index df3b9a5..015d71e 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.html
@@ -16,7 +16,6 @@
-->
<link rel="import" href="/bower_components/polymer/polymer.html">
-<script src="../../../test/test-pre-setup.js"></script>
<link rel="import" href="../../../test/common-test-setup.html"/>
<dom-module id="mock-diff-response">
<template></template>
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.js
index e29d300..e7bc662 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/mock-diff-response_test.js
@@ -16,7 +16,6 @@
*/
import '../../../scripts/bundled-polymer.js';
-import '../../../test/test-pre-setup.js';
import '../../../test/common-test-setup.js';
import {Polymer} from '@polymer/polymer/lib/legacy/polymer-fn.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
