Merge "Fix handling of ported comments (broken after big comment refactor)"
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
index 50399be..a08bf39 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -33,8 +33,6 @@
createCommentThreads,
isInPatchRange,
isDraftThread,
- isInBaseOfPatchRange,
- isInRevisionOfPatchRange,
isPatchsetLevel,
addPath,
} from '../../../utils/comment-util';
@@ -338,20 +336,8 @@
comment => comment.id === portedComment.id
)!;
- if (
- (originalComment.line && !portedComment.line) ||
- (originalComment.range && !portedComment.range)
- ) {
- thread.rangeInfoLost = true;
- }
-
- if (
- isInBaseOfPatchRange(thread.comments[0], patchRange) ||
- isInRevisionOfPatchRange(thread.comments[0], patchRange)
- ) {
- // no need to port this thread as it will be rendered by default
- return false;
- }
+ // Original comment shown anyway? No need to port.
+ if (isInPatchRange(originalComment, patchRange)) return false;
if (thread.commentSide === CommentSide.PARENT) {
// TODO(dhruvsri): Add handling for merge parents
@@ -364,6 +350,17 @@
if (!isUnresolved(thread) && !isDraftThread(thread)) return false;
+ if (
+ (originalComment.line && !portedComment.line) ||
+ (originalComment.range && !portedComment.range)
+ ) {
+ thread.rangeInfoLost = true;
+ }
+ // TODO: It probably makes more sense to set the patch_set in
+ // portedComment either in the backend or in the RestApi layer. Then we
+ // could check `!isInPatchRange(portedComment, patchRange)` and then set
+ // thread.patchNum = portedComment.patch_set;
+ thread.patchNum = patchRange.patchNum;
thread.range = portedComment.range;
thread.line = portedComment.line;
thread.ported = true;
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
index 9770261..2738eb8 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
@@ -131,7 +131,8 @@
{path: 'karma.conf.js'}, {patchNum: 4, basePatchNum: 'PARENT'});
assert.equal(portedThreads.length, 1);
- // check range of thread is from the ported comment and not the original
+ // check that the location of the thread matches the ported comment
+ assert.equal(portedThreads[0].patchNum, 4);
assert.deepEqual(portedThreads[0].range, {
start_line: 136,
start_character: 16,
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 1411c88..bd05edb 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -528,7 +528,7 @@
return html`
<a href="${this.getUrlForComment()}">
<span class="portedMessage" @click="${this.handlePortedMessageClick}">
- From patchset ${this.comment?.patch_set}]]
+ From patchset ${this.comment?.patch_set}
</span>
</a>
`;
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 49e9674..ee26915 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -188,6 +188,10 @@
* thread only contains an unsaved draft.
*/
rootId?: UrlEncodedCommentId;
+ /**
+ * Note that all location information is typically identical to that of the
+ * first comment, but not for ported comments!
+ */
path: string;
commentSide: CommentSide;
/* mergeParentNum is the merge parent number only valid for merge commits
@@ -201,8 +205,17 @@
FILE comments. */
line?: LineNumber;
range?: CommentRange;
- ported?: boolean; // is the comment ported over from a previous patchset
- rangeInfoLost?: boolean; // if BE was unable to determine a range for this
+ /**
+ * Was the thread ported over from its original location to a newer patchset?
+ * If yes, then the location information above contains the ported location,
+ * but the comments still have the original location set.
+ */
+ ported?: boolean;
+ /**
+ * Only relevant when ported:true. Means that no ported range could be
+ * computed. `line` and `range` can be undefined then.
+ */
+ rangeInfoLost?: boolean;
}
export function equalLocation(t1?: CommentThread, t2?: CommentThread) {