Treat unsaved comment as a DraftInfo entity
This means removing the `UnsavedInfo` type and moving control of unsaved
drafts into the comment model. Before this change unsaved drafts were
just a local state of comment components.
The motivation for this change comes from implementing optimistic
updates. For that we need to be able to go back from SAVING state to
UNSAVED in case of an error. With the current implementation that was
not really possible, or would at least have created a great mess.
The simplification in `gr-comment-thread` and `gr-diff-host` proves that
this change is also desirable for other reasons. It is much easier for
these two components to just consume everything from the model instead
of treating unsaved and saved drafts separately.
One challenge was how to deal with the `id` of a draft. The server sets
the `id` once it was saved, but until then we still need some kind of
identification, so for unsaved drafts we create a `client_id` and use
that for identifying the draft, even after it has been saved.
The type system for `CommentInfo` and `DraftInfo` has also cleaned up a
bit. Sorry for the noise of replacing some `PathToCommentsInfoMap` by
`{[path: string]: CommentInfo[]}` etc.
Release-Notes: skip
Google-Bug-Id: b/262228572
Change-Id: I1bd03ad58779d108bca0a22c517d27557aa62bbf
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index edd7f3d..0b96ce7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -103,7 +103,7 @@
import {LitElement, PropertyValues, css, html, nothing} from 'lit';
import {customElement, property, query, state} from 'lit/decorators.js';
import {ifDefined} from 'lit/directives/if-defined.js';
-import {assertIsDefined, queryAll} from '../../../utils/common-util';
+import {assertIsDefined, queryAll, uuid} from '../../../utils/common-util';
import {Interaction} from '../../../constants/reporting';
import {rootUrl} from '../../../utils/url-util';
import {createSearchUrl} from '../../../models/views/search';
@@ -909,8 +909,7 @@
enabled: true,
label,
__type: type,
- __key:
- ADDITIONAL_ACTION_KEY_PREFIX + Math.random().toString(36).substr(2),
+ __key: ADDITIONAL_ACTION_KEY_PREFIX + uuid(),
};
this.additionalActions.push(action);
this.requestUpdate('additionalActions');
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index 4d2602e..891209e 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -39,6 +39,7 @@
import {resolve} from '../../../models/dependency';
import {createSearchUrl} from '../../../models/views/search';
import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {uuid} from '../../../utils/common-util';
const SUGGESTIONS_LIMIT = 15;
const CHANGE_SUBJECT_LIMIT = 50;
@@ -532,8 +533,7 @@
}
private generateRandomCherryPickTopic(change: ChangeInfo) {
- const randomString = Math.random().toString(36).substr(2, 10);
- const message = `cherrypick-${change.topic}-${randomString}`;
+ const message = `cherrypick-${change.topic}-${uuid()}`;
return message;
}
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
index ec43aea..0217aa8 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
@@ -27,7 +27,7 @@
Timestamp,
UrlEncodedCommentId,
} from '../../../types/common';
-import {assertIsDefined} from '../../../utils/common-util';
+import {assertIsDefined, uuid} from '../../../utils/common-util';
import {html} from 'lit';
import {fixture, assert} from '@open-wc/testing';
import {GrButton} from '../../shared/gr-button/gr-button';
@@ -61,9 +61,9 @@
email: 'andybons@chromium.org' as EmailAddress,
};
return {
- id: (params.id || Math.random().toString()) as ChangeMessageId,
+ id: (params.id || uuid()) as ChangeMessageId,
date: (params.date || '2016-01-12 20:28:33.038000') as Timestamp,
- message: params.message || Math.random().toString(),
+ message: params.message || uuid(),
_revision_number: (params._revision_number || 1) as PatchSetNum,
author: params.author || author1,
tag: params.tag,
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 7c85bb6..bd1b193 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -58,7 +58,6 @@
SuggestedReviewerGroupInfo,
Suggestion,
UserId,
- UnsavedInfo,
isDraft,
} from '../../../types/common';
import {GrButton} from '../../shared/gr-button/gr-button';
@@ -72,7 +71,7 @@
queryAndAssert,
} from '../../../utils/common-util';
import {
- createPatchsetLevelUnsavedDraft,
+ createUnsavedPatchsetLevel,
getFirstComment,
isPatchsetLevel,
isUnresolved,
@@ -335,7 +334,7 @@
patchsetLevelDraftIsResolved = true;
@state()
- patchsetLevelComment?: UnsavedInfo | DraftInfo;
+ patchsetLevelComment?: DraftInfo;
@state()
isOwner = false;
@@ -700,6 +699,20 @@
});
}
+ override updated() {
+ if (!this.patchsetLevelComment && this.latestPatchNum) {
+ // TODO: This should rather be done in the comments model. It should
+ // ensure that a patchset level draft is always present.
+ this.getCommentsModel().addUnsavedDraft(
+ createUnsavedPatchsetLevel(
+ this.latestPatchNum,
+ this.patchsetLevelDraftMessage,
+ !this.patchsetLevelDraftIsResolved
+ )
+ );
+ }
+ }
+
override willUpdate(changedProperties: PropertyValues) {
if (changedProperties.has('ccPendingConfirmation')) {
this.pendingConfirmationUpdated(this.ccPendingConfirmation);
@@ -894,12 +907,7 @@
}
private renderPatchsetLevelComment() {
- if (!this.patchsetLevelComment)
- this.patchsetLevelComment = createPatchsetLevelUnsavedDraft(
- this.latestPatchNum,
- this.patchsetLevelDraftMessage,
- !this.patchsetLevelDraftIsResolved
- );
+ if (!this.patchsetLevelComment) return nothing;
return html`
<gr-comment
id="patchsetLevelComment"
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 6567e33..00e013b 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
@@ -7,14 +7,13 @@
PatchRange,
PatchSetNum,
RobotCommentInfo,
- PathToCommentsInfoMap,
FileInfo,
PARENT,
- CommentInfo,
CommentThread,
Comment,
CommentMap,
DraftInfo,
+ CommentInfo,
} from '../../../types/common';
import {
isUnresolved,
@@ -23,6 +22,7 @@
isDraftThread,
isPatchsetLevel,
addPath,
+ id,
} from '../../../utils/comment-util';
import {PatchSetFile, PatchNumOnly, isPatchSetFile} from '../../../types/types';
import {CommentSide} from '../../../constants/constants';
@@ -31,22 +31,22 @@
// TODO: Move file out of elements/ directory
export class ChangeComments {
- private readonly _comments: PathToCommentsInfoMap;
+ private readonly _comments: {[path: string]: CommentInfo[]};
private readonly _robotComments: {[path: string]: RobotCommentInfo[]};
private readonly _drafts: {[path: string]: DraftInfo[]};
- private readonly _portedComments: PathToCommentsInfoMap;
+ private readonly _portedComments: {[path: string]: CommentInfo[]};
- private readonly _portedDrafts: PathToCommentsInfoMap;
+ private readonly _portedDrafts: {[path: string]: DraftInfo[]};
constructor(
- comments?: PathToCommentsInfoMap,
+ comments?: {[path: string]: CommentInfo[]},
robotComments?: {[path: string]: RobotCommentInfo[]},
drafts?: {[path: string]: DraftInfo[]},
- portedComments?: PathToCommentsInfoMap,
- portedDrafts?: PathToCommentsInfoMap
+ portedComments?: {[path: string]: CommentInfo[]},
+ portedDrafts?: {[path: string]: DraftInfo[]}
) {
this._comments = addPath(comments);
this._robotComments = addPath(robotComments);
@@ -102,7 +102,7 @@
*/
getAllComments(includeDrafts?: boolean, patchNum?: PatchSetNum) {
const paths = this.getPaths();
- const publishedComments: {[path: string]: CommentInfo[]} = {};
+ const publishedComments: {[path: string]: Comment[]} = {};
for (const path of Object.keys(paths)) {
publishedComments[path] = this.getAllCommentsForPath(
path,
@@ -136,8 +136,8 @@
path: string,
patchNum?: PatchSetNum,
includeDrafts?: boolean
- ): CommentInfo[] {
- const comments: CommentInfo[] = this._comments[path] || [];
+ ): Comment[] {
+ const comments: Comment[] = this._comments[path] || [];
const robotComments = this._robotComments[path] || [];
let allComments = comments.concat(robotComments);
if (includeDrafts) {
@@ -192,7 +192,7 @@
*
* // TODO(taoalpha): maybe merge in *ForPath
*/
- getAllDraftsForFile(file: PatchSetFile): CommentInfo[] {
+ getAllDraftsForFile(file: PatchSetFile): DraftInfo[] {
let allDrafts = this.getAllDraftsForPath(file.path, file.patchNum);
if (file.basePath) {
allDrafts = allDrafts.concat(
@@ -210,8 +210,8 @@
* @param patchRange The patch-range object containing patchNum
* and basePatchNum properties to represent the range.
*/
- getCommentsForPath(path: string, patchRange: PatchRange): CommentInfo[] {
- let comments: CommentInfo[] = [];
+ getCommentsForPath(path: string, patchRange: PatchRange): Comment[] {
+ let comments: Comment[] = [];
let drafts: DraftInfo[] = [];
let robotComments: RobotCommentInfo[] = [];
if (this._comments && this._comments[path]) {
@@ -269,7 +269,7 @@
file: PatchSetFile,
patchRange: PatchRange
): CommentThread[] {
- const portedComments = this._portedComments[file.path] || [];
+ const portedComments: Comment[] = this._portedComments[file.path] || [];
portedComments.push(...(this._portedDrafts[file.path] || []));
if (file.basePath) {
portedComments.push(...(this._portedComments[file.basePath] || []));
@@ -281,7 +281,7 @@
// ported comments will involve comments that may not belong to the
// current patchrange, so we need to form threads for them using all
// comments
- const allComments: CommentInfo[] = this.getAllCommentsForFile(file, true);
+ const allComments: Comment[] = this.getAllCommentsForFile(file, true);
return createCommentThreads(allComments).filter(thread => {
// Robot comments and drafts are not ported over. A human reply to
@@ -289,12 +289,12 @@
// have the root comment of the thread not be ported, hence loop over
// entire thread
const portedComment = portedComments.find(portedComment =>
- thread.comments.some(c => portedComment.id === c.id)
+ thread.comments.some(c => id(portedComment) === id(c))
);
if (!portedComment) return false;
const originalComment = thread.comments.find(
- comment => comment.id === portedComment.id
+ comment => id(comment) === id(portedComment)
)!;
// Original comment shown anyway? No need to port.
@@ -347,10 +347,7 @@
* @param patchRange The patch-range object containing patchNum
* and basePatchNum properties to represent the range.
*/
- getCommentsForFile(
- file: PatchSetFile,
- patchRange: PatchRange
- ): CommentInfo[] {
+ getCommentsForFile(file: PatchSetFile, patchRange: PatchRange): Comment[] {
const comments = this.getCommentsForPath(file.path, patchRange);
if (file.basePath) {
comments.push(...this.getCommentsForPath(file.basePath, patchRange));
@@ -372,11 +369,11 @@
file: PatchSetFile | PatchNumOnly,
ignorePatchsetLevelComments?: boolean
) {
- let comments: CommentInfo[] = [];
+ let comments: Comment[] = [];
if (isPatchSetFile(file)) {
comments = this.getAllCommentsForFile(file);
} else {
- comments = this._commentObjToArray<CommentInfo>(
+ comments = this._commentObjToArray<Comment>(
this.getAllPublishedComments(file.patchNum)
);
}
@@ -500,8 +497,8 @@
file: PatchSetFile | PatchNumOnly,
ignorePatchsetLevelComments?: boolean
) {
- let comments: CommentInfo[] = [];
- let drafts: CommentInfo[] = [];
+ let comments: Comment[] = [];
+ let drafts: Comment[] = [];
if (isPatchSetFile(file)) {
comments = this.getAllCommentsForFile(file);
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.ts
index d337521..3e36ab5 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.ts
@@ -29,7 +29,6 @@
PARENT,
PatchRange,
PatchSetNum,
- PathToCommentsInfoMap,
RevisionPatchSetNum,
RobotCommentInfo,
Timestamp,
@@ -49,7 +48,7 @@
});
suite('ported comments', () => {
- let portedComments: PathToCommentsInfoMap;
+ let portedComments: {[path: string]: CommentInfo[]};
const comment1: CommentInfo = {
...createComment(),
unresolved: true,
@@ -593,7 +592,7 @@
const robotComments: {[path: string]: RobotCommentInfo[]} = {
'file/one': [comments[0], comments[1]],
};
- const commentsByFile: PathToCommentsInfoMap = {
+ const commentsByFile: {[path: string]: CommentInfo[]} = {
'file/one': [comments[2], comments[3]],
'file/two': [comments[4], comments[5]],
'file/three': [comments[6], comments[7], comments[8]],
@@ -741,7 +740,7 @@
});
test('computeUnresolvedNum w/ non-linear thread', () => {
- const comments: PathToCommentsInfoMap = {
+ const comments: {[path: string]: CommentInfo[]} = {
path: [
{
id: '9c6ba3c6_28b7d467' as UrlEncodedCommentId,
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 9bead95..511c9ab 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
@@ -21,7 +21,7 @@
isNumber,
} from '../../../utils/patch-set-util';
import {
- equalLocation,
+ createUnsaved,
isInBaseOfPatchRange,
isInRevisionOfPatchRange,
} from '../../../utils/comment-util';
@@ -31,6 +31,7 @@
BlameInfo,
ChangeInfo,
CommentThread,
+ DraftInfo,
EDIT,
NumericChangeId,
PARENT,
@@ -1044,15 +1045,10 @@
private threadsChanged(threads: CommentThread[]) {
const rootIdToThreadEl = new Map<UrlEncodedCommentId, GrCommentThread>();
- const unsavedThreadEls: GrCommentThread[] = [];
const threadEls = this.getThreadEls();
for (const threadEl of threadEls) {
- if (threadEl.rootId) {
- rootIdToThreadEl.set(threadEl.rootId, threadEl);
- } else {
- // Unsaved thread els must have editing:true, just being defensive here.
- if (threadEl.editing) unsavedThreadEls.push(threadEl);
- }
+ assertIsDefined(threadEl.rootId, 'threadEl.rootId');
+ rootIdToThreadEl.set(threadEl.rootId, threadEl);
}
const dontRemove = new Set<GrCommentThread>();
const threadCount = threads.length;
@@ -1062,23 +1058,8 @@
for (const thread of threads) {
// Let's find an existing DOM element matching the thread. Normally this
// is as simple as matching the rootIds.
- let existingThreadEl =
+ const existingThreadEl =
thread.rootId && rootIdToThreadEl.get(thread.rootId);
- // But unsaved threads don't have rootIds. The incoming thread might be
- // the saved version of the unsaved thread element. To verify that we
- // check that the thread only has one comment and that their location is
- // identical.
- // TODO(brohlfs): This matching is not perfect. You could quickly create
- // two new threads on the same line/range. Then this code just makes a
- // random guess.
- if (!existingThreadEl && thread.comments?.length === 1) {
- for (const unsavedThreadEl of unsavedThreadEls) {
- if (equalLocation(unsavedThreadEl.thread, thread)) {
- existingThreadEl = unsavedThreadEl;
- break;
- }
- }
- }
// There is a case possible where the rootIds match but the locations
// are different. Such as when a thread was originally attached on the
// right side of the diff but now should be attached on the left side of
@@ -1105,10 +1086,6 @@
// Remove all threads that are no longer existing.
for (const threadEl of this.getThreadEls()) {
if (dontRemove.has(threadEl)) continue;
- // The user may have opened a couple of comment boxes for editing. They
- // might be unsaved and thus not be reflected in `threads` yet, so let's
- // keep them open.
- if (threadEl.editing && threadEl.thread?.comments.length === 0) continue;
threadEl.remove();
}
const portedThreadsCount = threads.filter(thread => thread.ported).length;
@@ -1163,19 +1140,16 @@
assertIsDefined(path, 'path');
const parentIndex = this.computeParentIndex();
- const newThread: CommentThread = {
- rootId: undefined,
- comments: [],
- patchNum: patchNum as RevisionPatchSetNum,
- commentSide,
- // TODO: Maybe just compute from patchRange.base on the fly?
- mergeParentNum: parentIndex ?? undefined,
+ const draft: DraftInfo = {
+ ...createUnsaved('', true),
+ patch_set: patchNum as RevisionPatchSetNum,
+ side: commentSide,
+ parent: parentIndex ?? undefined,
path,
- line: lineNum,
+ line: typeof lineNum === 'number' ? lineNum : undefined,
range,
};
- const el = this.createThreadElement(newThread);
- this.attachThreadElement(el);
+ this.getCommentsModel().addUnsavedDraft(draft);
}
private canCommentOnPatchSetNum(patchNum: PatchSetNum) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
index 31991d4..5de86aa 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
@@ -33,7 +33,7 @@
BasePatchSetNum,
BlameInfo,
CommentRange,
- DraftState,
+ DraftInfo,
EDIT,
ImageInfo,
NumericChangeId,
@@ -57,6 +57,10 @@
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
+import {
+ CommentsModel,
+ commentsModelToken,
+} from '../../../models/comments/comments-model';
suite('gr-diff-host tests', () => {
let element: GrDiffHost;
@@ -1000,7 +1004,12 @@
});
suite('create-comment', () => {
+ let addDraftSpy: sinon.SinonSpy;
+
setup(async () => {
+ const commentsModel: CommentsModel = testResolver(commentsModelToken);
+ addDraftSpy = sinon.spy(commentsModel, 'addUnsavedDraft');
+
account = createAccountDetailWithId(1);
element.disconnectedCallback();
element.connectedCallback();
@@ -1018,17 +1027,12 @@
},
})
);
- assertIsDefined(element.diffElement);
- let threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
- assert.equal(threads.length, 1);
- assert.equal(threads[0].thread?.commentSide, CommentSide.PARENT);
- assert.equal(threads[0].getAttribute('diff-side'), Side.LEFT);
- assert.equal(threads[0].thread?.range, undefined);
- assert.equal(threads[0].thread?.patchNum, 1 as RevisionPatchSetNum);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.PARENT);
+ assert.equal(draft1.range, undefined);
+ assert.equal(draft1.patch_set, 1 as RevisionPatchSetNum);
// Try to fetch a thread with a different range.
const range = {
@@ -1050,17 +1054,11 @@
})
);
- assertIsDefined(element.diffElement);
- threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
-
- assert.equal(threads.length, 2);
- assert.equal(threads[0].thread?.commentSide, CommentSide.PARENT);
- assert.equal(threads[0].getAttribute('diff-side'), Side.LEFT);
- assert.equal(threads[1].thread?.range, range);
- assert.equal(threads[1].thread?.patchNum, 1 as RevisionPatchSetNum);
+ assert.equal(addDraftSpy.callCount, 2);
+ const draft2: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft2.side, CommentSide.PARENT);
+ assert.equal(draft2.range, range);
+ assert.equal(draft2.patch_set, 1 as RevisionPatchSetNum);
});
test('should not be on parent if on the right', async () => {
@@ -1075,16 +1073,10 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
- assert.equal(threads.length, 1);
- const threadEl = threads[0];
-
- assert.equal(threadEl.thread?.commentSide, CommentSide.REVISION);
- assert.equal(threadEl.getAttribute('diff-side'), Side.RIGHT);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.REVISION);
+ assert.equal(draft1.patch_set, 3 as RevisionPatchSetNum);
});
test('should be on parent if right and base is PARENT', () => {
@@ -1098,15 +1090,10 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
- const threadEl = threads[0];
-
- assert.equal(threadEl.thread?.commentSide, CommentSide.PARENT);
- assert.equal(threadEl.getAttribute('diff-side'), Side.LEFT);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.PARENT);
+ assert.equal(draft1.patch_set, 1 as RevisionPatchSetNum);
});
test('should be on parent if right and base negative', () => {
@@ -1120,15 +1107,11 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
- const threadEl = threads[0];
-
- assert.equal(threadEl.thread?.commentSide, CommentSide.PARENT);
- assert.equal(threadEl.getAttribute('diff-side'), Side.LEFT);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.PARENT);
+ assert.equal(draft1.patch_set, 3 as RevisionPatchSetNum);
+ assert.equal(draft1.parent, 2);
});
test('should not be on parent otherwise', () => {
@@ -1141,15 +1124,10 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
- const threadEl = threads[0];
-
- assert.equal(threadEl.thread?.commentSide, CommentSide.REVISION);
- assert.equal(threadEl.getAttribute('diff-side'), Side.LEFT);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.REVISION);
+ assert.equal(draft1.patch_set, 2 as RevisionPatchSetNum);
});
test(
@@ -1169,14 +1147,11 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
- assert.equal(threads.length, 1);
- assert.equal(threads[0].getAttribute('diff-side'), Side.LEFT);
- assert.equal(threads[0].thread?.path, element.file.basePath);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.REVISION);
+ assert.equal(draft1.patch_set, 2 as RevisionPatchSetNum);
+ assert.equal(draft1.path, element.file.basePath);
}
);
@@ -1197,15 +1172,11 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
-
- assert.equal(threads.length, 1);
- assert.equal(threads[0].getAttribute('diff-side'), Side.RIGHT);
- assert.equal(threads[0].thread?.path, element.file.path);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.REVISION);
+ assert.equal(draft1.patch_set, 3 as RevisionPatchSetNum);
+ assert.equal(draft1.path, element.file.path);
}
);
@@ -1258,48 +1229,6 @@
assert.equal(threads.length, 2);
});
- test('unsaved thread changes to draft', async () => {
- element.patchRange = createPatchRange(2, 3);
- element.file = {basePath: 'file_renamed.txt', path: element.path ?? ''};
- element.threads = [];
- await element.updateComplete;
-
- element.dispatchEvent(
- new CustomEvent('create-comment', {
- detail: {
- side: Side.RIGHT,
- path: element.path,
- lineNum: 13,
- },
- })
- );
- await element.updateComplete;
- assert.equal(element.getThreadEls().length, 1);
- const threadEl = element.getThreadEls()[0];
- assert.equal(threadEl.thread?.line, 13);
- assert.isDefined(threadEl.unsavedComment);
- assert.equal(threadEl.thread?.comments.length, 0);
-
- const draftThread = createCommentThread([
- {
- path: element.path,
- patch_set: 3 as RevisionPatchSetNum,
- line: 13,
- __draft: DraftState.SAVED,
- },
- ]);
- element.threads = [draftThread];
- await element.updateComplete;
-
- // We expect that no additional thread element was created.
- assert.equal(element.getThreadEls().length, 1);
- // In fact the thread element must still be the same.
- assert.equal(element.getThreadEls()[0], threadEl);
- // But it must have been updated from unsaved to draft:
- assert.isUndefined(threadEl.unsavedComment);
- assert.equal(threadEl.thread?.comments.length, 1);
- });
-
test(
'thread should use new file path if first created ' +
'on patch set (left) but is base',
@@ -1317,15 +1246,11 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
-
- assert.equal(threads.length, 1);
- assert.equal(threads[0].getAttribute('diff-side'), Side.LEFT);
- assert.equal(threads[0].thread?.path, element.file.path);
+ assert.equal(addDraftSpy.callCount, 1);
+ const draft1: DraftInfo = addDraftSpy.lastCall.firstArg;
+ assert.equal(draft1.side, CommentSide.PARENT);
+ assert.equal(draft1.patch_set, 1 as RevisionPatchSetNum);
+ assert.equal(draft1.path, element.file.path);
}
);
@@ -1347,13 +1272,7 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
-
- assert.equal(threads.length, 0);
+ assert.isFalse(addDraftSpy.called);
assert.isTrue(alertSpy.called);
});
@@ -1375,12 +1294,7 @@
})
);
- assertIsDefined(element.diffElement);
- const threads =
- element.diffElement.querySelectorAll<GrCommentThread>(
- 'gr-comment-thread'
- );
- assert.equal(threads.length, 0);
+ assert.isFalse(addDraftSpy.called);
assert.isTrue(alertSpy.called);
});
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index ed7ce1b..e527633 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -42,7 +42,6 @@
PARENT,
PatchSetNum,
PatchSetNumber,
- PathToCommentsInfoMap,
RepoName,
RevisionPatchSetNum,
UrlEncodedCommentId,
@@ -487,7 +486,7 @@
assertIsDefined(element.cursor);
sinon.stub(element.cursor, 'isAtEnd').returns(true);
element.changeNum = 42 as NumericChangeId;
- const comment: PathToCommentsInfoMap = {
+ const comment: {[path: string]: CommentInfo[]} = {
'wheatley.md': [createComment('c2', 21, 10, 'wheatley.md')],
};
element.changeComments = new ChangeComments(comment);
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
index 20d2549..6051131 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
@@ -20,7 +20,7 @@
RevisionInfo,
Timestamp,
UrlEncodedCommentId,
- PathToCommentsInfoMap,
+ CommentInfo,
} from '../../../types/common';
import {EditRevisionInfo, ParsedChangeInfo} from '../../../types/types';
import {SpecialFilePath} from '../../../constants/constants';
@@ -331,7 +331,7 @@
test('computePatchSetCommentsString', () => {
// Test string with unresolved comments.
- const comments: PathToCommentsInfoMap = {
+ const comments: {[path: string]: CommentInfo[]} = {
foo: [
{
id: '27dcee4d_f7b77cfa' as UrlEncodedCommentId,
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 49a3b06..3ec0764 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -20,10 +20,10 @@
import {
computeDiffFromContext,
getLastComment,
- createUnsavedComment,
getFirstComment,
createUnsavedReply,
NEWLINE_PATTERN,
+ id,
} from '../../../utils/comment-util';
import {ChangeMessageId} from '../../../api/rest-api';
import {getAppContext} from '../../../services/app-context';
@@ -43,7 +43,6 @@
isUnsaved,
NumericChangeId,
RepoName,
- UnsavedInfo,
UrlEncodedCommentId,
} from '../../../types/common';
import {CommentEditingChangedDetail, GrComment} from '../gr-comment/gr-comment';
@@ -128,13 +127,14 @@
thread?: CommentThread;
/**
- * Id of the first comment and thus must not change. Will be derived from
+ * Id of the first comment, must not change. Will be derived from
* the `thread` property in the first willUpdate() cycle.
*
* The `rootId` property is also used in gr-diff for maintaining lists and
* maps of threads and their associated elements.
*
- * Only stays `undefined` for new threads that only have an unsaved comment.
+ * For newly created threads in this session the `client_id` property of the
+ * first comment will be used instead of the `id` property.
*/
@property({type: String})
rootId?: UrlEncodedCommentId;
@@ -190,15 +190,6 @@
@property({type: Boolean, attribute: 'false'})
editing = false;
- /**
- * This can either be an unsaved reply to the last comment or the unsaved
- * content of a brand new comment thread (then `comments` is empty).
- * If set, then `thread.comments` must not contain a draft. A thread can only
- * contain *either* an unsaved comment *or* a draft, not both.
- */
- @state()
- unsavedComment?: UnsavedInfo;
-
@state()
changeNum?: NumericChangeId;
@@ -484,6 +475,9 @@
// We are deliberately not including the draft in the repeat directive,
// because we ran into spurious issues with <gr-comment> being destroyed
// and re-created when an unsaved draft transitions to 'saved' state.
+ // TODO: Revisit this, because UNSAVED and DRAFT is not so different
+ // anymore. Just put the draft into the `repeat` directive above and
+ // then use `id()` instead of`.id` above.
const draftComment = this.renderComment(this.getDraftOrUnsaved());
return html`${publishedComments}${draftComment}`;
}
@@ -491,6 +485,7 @@
private renderComment(comment?: Comment) {
if (!comment) return nothing;
const robotButtonDisabled = !this.account || this.isDraftOrUnsaved();
+ const isFirstComment = this.getFirstComment() === comment;
const initiallyCollapsed =
!isDraftOrUnsaved(comment) &&
(this.messageId
@@ -503,8 +498,7 @@
?initially-collapsed=${initiallyCollapsed}
?robot-button-disabled=${robotButtonDisabled}
?show-patchset=${this.showPatchset}
- ?show-ported-comment=${this.showPortedComment &&
- comment.id === this.rootId}
+ ?show-ported-comment=${this.showPortedComment && isFirstComment}
@reply-to-comment=${this.handleReplyToComment}
@copy-comment-link=${this.handleCopyLink}
@comment-editing-changed=${(
@@ -613,9 +607,6 @@
if (this.firstWillUpdateDone) return;
this.firstWillUpdateDone = true;
- if (this.getFirstComment() === undefined) {
- this.unsavedComment = createUnsavedComment(this.thread);
- }
this.unresolved = this.getLastComment()?.unresolved ?? true;
this.diff = this.computeDiff();
this.highlightRange = this.computeHighlightRange();
@@ -624,6 +615,8 @@
override willUpdate(changed: PropertyValues) {
this.firstWillUpdate();
if (changed.has('thread')) {
+ assertIsDefined(this.thread, 'thread');
+ assertIsDefined(this.getFirstComment(), 'first comment');
if (!this.isDraftOrUnsaved()) {
// We can only do this for threads without draft, because otherwise we
// are relying on the <gr-comment> component for the draft to fire
@@ -631,21 +624,9 @@
this.unresolved = this.getLastComment()?.unresolved ?? true;
}
this.hasDraft = this.isDraftOrUnsaved();
- this.rootId = this.getFirstComment()?.id;
- if (this.isDraft()) {
- this.unsavedComment = undefined;
- }
+ this.rootId = id(this.getFirstComment()!);
}
if (changed.has('editing')) {
- // changed.get('editing') contains the old value. We only want to trigger
- // when changing from editing to non-editing (user has cancelled/saved).
- // We do *not* want to trigger on first render (old value is `null`)
- if (!this.editing && changed.get('editing') === true) {
- this.unsavedComment = undefined;
- if (this.thread?.comments.length === 0) {
- this.remove();
- }
- }
fire(this, 'comment-thread-editing-changed', {value: this.editing});
}
}
@@ -675,17 +656,12 @@
}
private getDraftOrUnsaved(): Comment | undefined {
- if (this.unsavedComment) return this.unsavedComment;
- if (this.isDraft()) return this.getLastComment();
+ if (this.isDraftOrUnsaved()) return this.getLastComment();
return undefined;
}
- private isNewThread(): boolean {
- return this.thread?.comments.length === 0;
- }
-
private isUnsaved(): boolean {
- return !!this.unsavedComment || this.thread?.comments.length === 0;
+ return isUnsaved(this.getLastComment());
}
private isPatchsetLevel() {
@@ -717,7 +693,6 @@
if (!this.changeNum || !this.repoName || !this.thread?.path) {
return undefined;
}
- if (this.isNewThread()) return undefined;
return createDiffUrl({
changeNum: this.changeNum,
repo: this.repoName,
@@ -743,14 +718,12 @@
// Does not work for patchset level comments
private getUrlForFileComment() {
- if (!this.repoName || !this.changeNum || this.isNewThread()) {
- return undefined;
- }
- assertIsDefined(this.rootId, 'rootId of comment thread');
+ const id = this.getFirstComment()?.id;
+ if (!id || !this.repoName || !this.changeNum) return undefined;
return createDiffUrl({
changeNum: this.changeNum,
repo: this.repoName,
- commentId: this.rootId,
+ commentId: id,
});
}
@@ -835,7 +808,7 @@
}
const unsaved = createUnsavedReply(replyingTo, content, unresolved);
if (userWantsToEdit) {
- this.unsavedComment = unsaved;
+ this.getCommentsModel().addUnsavedDraft(unsaved);
} else {
try {
this.saving = true;
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index 0e144de..6d52902 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -26,9 +26,10 @@
import {
createAccountDetailWithId,
createThread,
+ createUnsaved,
} from '../../../test/test-data-generators';
import {SinonStubbedMember} from 'sinon';
-import {fixture, html, waitUntil, assert} from '@open-wc/testing';
+import {fixture, html, assert} from '@open-wc/testing';
import {GrButton} from '../gr-button/gr-button';
import {SpecialFilePath} from '../../../constants/constants';
import {GrIcon} from '../gr-icon/gr-icon';
@@ -127,23 +128,23 @@
});
test('renders unsaved', async () => {
- element.thread = createThread();
+ element.thread = createThread(createUnsaved());
await element.updateComplete;
assert.shadowDom.equal(
element,
/* HTML */ `
<div class="fileName">
- <span>test-path-comment-thread</span>
+ <a href="/c/test-repo-name/+/1/1/test-path-comment-thread">
+ test-path-comment-thread
+ </a>
<gr-copy-clipboard hideinput=""></gr-copy-clipboard>
</div>
<div class="pathInfo">
<span>#314</span>
</div>
<div id="container">
- <h3 class="assistive-tech-only">
- Unresolved Draft Comment thread by Yoda
- </h3>
- <div class="comment-box unresolved" tabindex="0">
+ <h3 class="assistive-tech-only">Draft Comment thread by Yoda</h3>
+ <div class="comment-box" tabindex="0">
<gr-comment robot-button-disabled="" show-patchset=""></gr-comment>
</div>
</div>
@@ -314,13 +315,15 @@
suite('action button clicks', () => {
let savePromise: MockPromise<DraftInfo>;
- let stub: SinonStubbedMember<CommentsModel['saveDraft']>;
+ let stubSave: SinonStubbedMember<CommentsModel['saveDraft']>;
+ let stubAdd: SinonStubbedMember<CommentsModel['addUnsavedDraft']>;
setup(async () => {
savePromise = mockPromise<DraftInfo>();
- stub = sinon
+ stubSave = sinon
.stub(testResolver(commentsModelToken), 'saveDraft')
.returns(savePromise);
+ stubAdd = sinon.stub(testResolver(commentsModelToken), 'addUnsavedDraft');
element.thread = createThread(c1, {...c2, unresolved: true});
await element.updateComplete;
@@ -328,9 +331,9 @@
test('handle Acknowledge', async () => {
queryAndAssert<GrButton>(element, '#ackBtn').click();
- waitUntilCalled(stub, 'saveDraft()');
- assert.equal(stub.lastCall.firstArg.message, 'Acknowledged');
- assert.equal(stub.lastCall.firstArg.unresolved, false);
+ waitUntilCalled(stubSave, 'saveDraft()');
+ assert.equal(stubSave.lastCall.firstArg.message, 'Acknowledged');
+ assert.equal(stubSave.lastCall.firstArg.unresolved, false);
assert.isTrue(element.saving);
savePromise.resolve();
@@ -340,47 +343,23 @@
test('handle Done', async () => {
queryAndAssert<GrButton>(element, '#doneBtn').click();
- waitUntilCalled(stub, 'saveDraft()');
- assert.equal(stub.lastCall.firstArg.message, 'Done');
- assert.equal(stub.lastCall.firstArg.unresolved, false);
+ waitUntilCalled(stubSave, 'saveDraft()');
+ assert.equal(stubSave.lastCall.firstArg.message, 'Done');
+ assert.equal(stubSave.lastCall.firstArg.unresolved, false);
});
test('handle Reply', async () => {
- assert.isUndefined(element.unsavedComment);
+ assert.equal(element.thread?.comments.length, 2);
queryAndAssert<GrButton>(element, '#replyBtn').click();
- assert.equal(element.unsavedComment?.message, '');
+ assert.isTrue(stubAdd.called);
+ assert.equal(stubAdd.lastCall.firstArg.message, '');
});
test('handle Quote', async () => {
- assert.isUndefined(element.unsavedComment);
+ assert.equal(element.thread?.comments.length, 2);
queryAndAssert<GrButton>(element, '#quoteBtn').click();
- assert.equal(element.unsavedComment?.message?.trim(), `> ${c2.message}`);
- });
- });
-
- suite('self removal when empty thread changed to editing:false', () => {
- let threadEl: GrCommentThread;
-
- setup(async () => {
- threadEl = await fixture(html`<gr-comment-thread></gr-comment-thread>`);
- threadEl.thread = createThread();
- });
-
- test('new thread el normally has a parent and an unsaved comment', async () => {
- await waitUntil(() => threadEl.editing);
- assert.isOk(threadEl.unsavedComment);
- assert.isOk(threadEl.parentElement);
- });
-
- test('thread el removed after clicking CANCEL', async () => {
- await waitUntil(() => threadEl.editing);
-
- const commentEl = queryAndAssert(threadEl, 'gr-comment');
- const buttonEl = queryAndAssert<GrButton>(commentEl, 'gr-button.cancel');
- buttonEl.click();
-
- await waitUntil(() => !threadEl.editing);
- assert.isNotOk(threadEl.parentElement);
+ assert.isTrue(stubAdd.called);
+ assert.equal(stubAdd.lastCall.firstArg.message.trim(), `> ${c2.message}`);
});
});
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 de6a39c9..beaee8b 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -36,6 +36,7 @@
getContentInCommentRange,
getUserSuggestion,
hasUserSuggestion,
+ id,
NEWLINE_PATTERN,
USER_SUGGESTION_START_PATTERN,
} from '../../../utils/comment-util';
@@ -923,13 +924,11 @@
}
private getUrlForComment() {
- const comment = this.comment;
- if (!comment || !this.changeNum || !this.repoName) return '';
- if (!comment.id) throw new Error('comment must have an id');
+ if (!this.changeNum || !this.repoName || !this.comment?.id) return '';
return createDiffUrl({
changeNum: this.changeNum,
repo: this.repoName,
- commentId: comment.id,
+ commentId: this.comment.id,
});
}
@@ -1199,8 +1198,8 @@
const messageToSave = this.messageText.trimEnd();
if (messageToSave === '') {
// Don't try to discard UnsavedInfo. Nothing to do then.
- if (this.comment.id) {
- await this.getCommentsModel().discardDraft(this.comment.id);
+ if (this.comment) {
+ await this.getCommentsModel().discardDraft(id(this.comment));
}
} else {
// No need to make a backend call when nothing has changed.
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index e8d715e..66d78a6 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -19,7 +19,8 @@
import {
FixSuggestionInfo,
FixReplacementInfo,
- UnsavedInfo,
+ DraftInfo,
+ DraftState,
} from '../../types/common';
import {OpenFixPreviewEventDetail} from '../../types/events';
import {isDefined} from '../../types/types';
@@ -97,11 +98,11 @@
${result.message}`;
}
-export function createPleaseFixComment(result: RunResult): UnsavedInfo {
+export function createPleaseFixComment(result: RunResult): DraftInfo {
const pointer = result.codePointers?.[0];
assertIsDefined(pointer, 'codePointer');
return {
- __unsaved: true,
+ __draft: DraftState.UNSAVED,
path: pointer.path,
patch_set: result.patchset as RevisionPatchSetNum,
side: CommentSide.REVISION,
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index ba7aaa1..48f8319 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -5,26 +5,23 @@
*/
import {ChangeComments} from '../../elements/diff/gr-comment-api/gr-comment-api';
import {
- CommentBasics,
CommentInfo,
NumericChangeId,
PatchSetNum,
RevisionId,
UrlEncodedCommentId,
- PathToCommentsInfoMap,
RobotCommentInfo,
PathToRobotCommentsInfoMap,
AccountInfo,
DraftInfo,
- UnsavedInfo,
Comment,
- isDraft,
isDraftOrUnsaved,
isUnsaved,
DraftState,
} from '../../types/common';
import {
addPath,
+ id,
isDraftThread,
reportingDetails,
} from '../../utils/comment-util';
@@ -37,7 +34,7 @@
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
import {ChangeModel} from '../change/change-model';
import {Interaction, Timing} from '../../constants/reporting';
-import {assertIsDefined} from '../../utils/common-util';
+import {assert, assertIsDefined} from '../../utils/common-util';
import {debounce, DelayedTask} from '../../utils/async-util';
import {pluralize} from '../../utils/string-util';
import {ReportingService} from '../../services/gr-reporting/gr-reporting';
@@ -57,20 +54,18 @@
export interface CommentState {
/** undefined means 'still loading' */
- comments?: PathToCommentsInfoMap;
+ comments?: {[path: string]: CommentInfo[]};
/** undefined means 'still loading' */
robotComments?: {[path: string]: RobotCommentInfo[]};
// All drafts are DraftInfo objects and have `__draft` state set.
- // Draft state is either SAVED (known to the backend) or still SAVING.
- // Unsaved drafts (see UnsavedInfo) do NOT belong in the application model.
/** undefined means 'still loading' */
drafts?: {[path: string]: DraftInfo[]};
// Ported comments only affect `CommentThread` properties, not individual
// comments.
/** undefined means 'still loading' */
- portedComments?: PathToCommentsInfoMap;
+ portedComments?: {[path: string]: CommentInfo[]};
/** undefined means 'still loading' */
- portedDrafts?: PathToCommentsInfoMap;
+ portedDrafts?: {[path: string]: DraftInfo[]};
/**
* If a draft is discarded by the user, then we temporarily keep it in this
* array in case the user decides to Undo the discard operation and bring the
@@ -169,7 +164,7 @@
// Private but used in tests.
export function setPortedComments(
state: CommentState,
- portedComments?: PathToCommentsInfoMap
+ portedComments?: {[path: string]: CommentInfo[]}
): CommentState {
if (deepEqual(portedComments, state.portedComments)) return state;
const nextState = {...state};
@@ -180,7 +175,7 @@
// Private but used in tests.
export function setPortedDrafts(
state: CommentState,
- portedDrafts?: PathToCommentsInfoMap
+ portedDrafts?: {[path: string]: DraftInfo[]}
): CommentState {
if (deepEqual(portedDrafts, state.portedDrafts)) return state;
const nextState = {...state};
@@ -205,7 +200,7 @@
): CommentState {
const nextState = {...state};
const drafts = [...nextState.discardedDrafts];
- const index = drafts.findIndex(d => d.id === draftID);
+ const index = drafts.findIndex(draft => id(draft) === draftID);
if (index === -1) {
throw new Error('discarded draft not found');
}
@@ -218,14 +213,13 @@
export function setDraft(state: CommentState, draft: DraftInfo): CommentState {
const nextState = {...state};
if (!draft.path) throw new Error('draft path undefined');
- if (!isDraft(draft)) throw new Error('draft is not a draft');
- if (isUnsaved(draft)) throw new Error('unsaved drafts dont belong to model');
+ if (!isDraftOrUnsaved(draft)) throw new Error('draft is not a draft');
nextState.drafts = {...nextState.drafts};
const drafts = nextState.drafts;
if (!drafts[draft.path]) drafts[draft.path] = [] as DraftInfo[];
else drafts[draft.path] = [...drafts[draft.path]];
- const index = drafts[draft.path].findIndex(d => d.id && d.id === draft.id);
+ const index = drafts[draft.path].findIndex(d => id(d) === id(draft));
if (index !== -1) {
drafts[draft.path][index] = draft;
} else {
@@ -240,13 +234,11 @@
): CommentState {
const nextState = {...state};
if (!draft.path) throw new Error('draft path undefined');
- if (!isDraft(draft)) throw new Error('draft is not a draft');
- if (isUnsaved(draft)) throw new Error('unsaved drafts dont belong to model');
+ if (!isDraftOrUnsaved(draft)) throw new Error('draft is not a draft');
+
nextState.drafts = {...nextState.drafts};
const drafts = nextState.drafts;
- const index = (drafts[draft.path] || []).findIndex(
- d => d.id && d.id === draft.id
- );
+ const index = (drafts[draft.path] || []).findIndex(d => id(d) === id(draft));
if (index === -1) return state;
const discardedDraft = drafts[draft.path][index];
drafts[draft.path] = [...drafts[draft.path]];
@@ -552,32 +544,37 @@
this.modifyState(s => setPortedDrafts(s, portedDrafts));
}
- async restoreDraft(id: UrlEncodedCommentId) {
- const found = this.discardedDrafts?.find(d => d.id === id);
+ async restoreDraft(draftId: UrlEncodedCommentId) {
+ const found = this.discardedDrafts?.find(d => id(d) === draftId);
if (!found) throw new Error('discarded draft not found');
const newDraft = {
...found,
id: undefined,
updated: undefined,
- // Converting from DraftInfo to UnsavedInfo here.
- __draft: undefined,
- __unsaved: true,
+ __draft: DraftState.UNSAVED,
};
await this.saveDraft(newDraft);
- this.modifyState(s => deleteDiscardedDraft(s, id));
+ this.modifyState(s => deleteDiscardedDraft(s, draftId));
+ }
+
+ /**
+ * Adds a new unsaved draft.
+ */
+ addUnsavedDraft(draft: DraftInfo) {
+ assert(isUnsaved(draft), 'draft must be unsaved');
+ this.modifyState(s => setDraft(s, draft));
}
/**
* Saves a new or updates an existing draft.
* The model will only be updated when a successful response comes back.
*/
- async saveDraft(
- draft: DraftInfo | UnsavedInfo,
- showToast = true
- ): Promise<DraftInfo> {
+ async saveDraft(draft: DraftInfo, showToast = true): Promise<DraftInfo> {
assertIsDefined(this.changeNum, 'change number');
assertIsDefined(draft.patch_set, 'patchset number of comment draft');
- if (!draft.message?.trim()) throw new Error('Cannot save empty draft.');
+ if (!draft.message?.trim()) {
+ throw new Error('Cannot save empty draft.');
+ }
// Saving the change number as to make sure that the response is still
// relevant when it comes back. The user maybe have navigated away.
@@ -605,7 +602,6 @@
id: savedComment.id,
updated: savedComment.updated,
__draft: DraftState.SAVED,
- __unsaved: undefined,
};
timer.end({id: updatedDraft.id});
if (showToast) this.showEndRequest();
@@ -616,38 +612,40 @@
async discardDraft(draftId: UrlEncodedCommentId) {
const draft = this.lookupDraft(draftId);
- assertIsDefined(this.changeNum, 'change number');
assertIsDefined(draft, `draft not found by id ${draftId}`);
assertIsDefined(draft.patch_set, 'patchset number of comment draft');
- if (!draft.message?.trim()) throw new Error('saved draft cant be empty');
- // Saving the change number as to make sure that the response is still
- // relevant when it comes back. The user maybe have navigated away.
- const changeNum = this.changeNum;
- this.report(Interaction.DISCARD_COMMENT, draft);
- this.showStartRequest();
- const timer = this.reporting.getTimer(Timing.DRAFT_DISCARD);
- const result = await this.restApiService.deleteDiffDraft(
- changeNum,
- draft.patch_set,
- {id: draft.id}
- );
- timer.end({id: draft.id});
- if (changeNum !== this.changeNum) throw new Error('change changed');
- if (!result.ok) {
- this.handleFailedDraftRequest();
- throw new Error(
- `Failed to discard draft comment: ${JSON.stringify(result)}`
+ if (draft.id) {
+ if (!draft.message?.trim()) throw new Error('empty draft');
+ // Saving the change number as to make sure that the response is still
+ // relevant when it comes back. The user maybe have navigated away.
+ assertIsDefined(this.changeNum, 'change number');
+ const changeNum = this.changeNum;
+ this.report(Interaction.DISCARD_COMMENT, draft);
+ this.showStartRequest();
+ const timer = this.reporting.getTimer(Timing.DRAFT_DISCARD);
+ const result = await this.restApiService.deleteDiffDraft(
+ changeNum,
+ draft.patch_set,
+ {id: draft.id}
);
+ timer.end({id: draft.id});
+ if (changeNum !== this.changeNum) throw new Error('change changed');
+ if (!result.ok) {
+ this.handleFailedDraftRequest();
+ throw new Error(
+ `Failed to discard draft comment: ${JSON.stringify(result)}`
+ );
+ }
+ this.showEndRequest();
}
- this.showEndRequest();
this.modifyState(s => deleteDraft(s, draft));
// We don't store empty discarded drafts and don't need an UNDO then.
if (draft.message?.trim()) {
fire(document, 'show-alert', {
message: 'Draft Discarded',
action: 'Undo',
- callback: () => this.restoreDraft(draft.id),
+ callback: () => this.restoreDraft(draftId),
});
}
this.report(Interaction.COMMENT_DISCARDED, draft);
@@ -672,7 +670,7 @@
this.modifyState(s => updateComment(s, newComment));
}
- private report(interaction: Interaction, comment: CommentBasics) {
+ private report(interaction: Interaction, comment: Comment) {
const details = reportingDetails(comment);
this.reporting.reportInteraction(interaction, details);
}
@@ -713,9 +711,9 @@
);
}
- private lookupDraft(id: UrlEncodedCommentId): DraftInfo | undefined {
+ private lookupDraft(commentId: UrlEncodedCommentId): DraftInfo | undefined {
return Object.values(this.drafts)
.flat()
- .find(d => d.id === id);
+ .find(draft => id(draft) === commentId);
}
}
diff --git a/polygerrit-ui/app/models/comments/comments-model_test.ts b/polygerrit-ui/app/models/comments/comments-model_test.ts
index a689e42..20a3b49 100644
--- a/polygerrit-ui/app/models/comments/comments-model_test.ts
+++ b/polygerrit-ui/app/models/comments/comments-model_test.ts
@@ -11,6 +11,7 @@
} from '../../test/test-data-generators';
import {
AccountInfo,
+ CommentInfo,
EmailAddress,
NumericChangeId,
Timestamp,
@@ -24,7 +25,6 @@
} from '../../test/test-data-generators';
import {stubRestApi, waitUntil, waitUntilCalled} from '../../test/test-utils';
import {getAppContext} from '../../services/app-context';
-import {PathToCommentsInfoMap} from '../../types/common';
import {changeModelToken} from '../change/change-model';
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
@@ -93,9 +93,9 @@
const portedDraftsSpy = stubRestApi('getPortedDrafts').returns(
Promise.resolve({})
);
- let comments: PathToCommentsInfoMap = {};
+ let comments: {[path: string]: CommentInfo[]} = {};
subscriptions.push(model.comments$.subscribe(c => (comments = c ?? {})));
- let portedComments: PathToCommentsInfoMap = {};
+ let portedComments: {[path: string]: CommentInfo[]} = {};
subscriptions.push(
model.portedComments$.subscribe(c => (portedComments = c ?? {}))
);
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
index eed7675..b1f9646 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -93,7 +93,6 @@
Password,
PatchRange,
PatchSetNum,
- PathToCommentsInfoMap,
PathToRobotCommentsInfoMap,
PluginInfo,
PreferencesInfo,
@@ -2338,7 +2337,7 @@
getDiffComments(
changeNum: NumericChangeId
- ): Promise<PathToCommentsInfoMap | undefined>;
+ ): Promise<{[path: string]: CommentInfo[]} | undefined>;
getDiffComments(
changeNum: NumericChangeId,
@@ -2439,7 +2438,7 @@
changeNum: NumericChangeId,
endpoint: '/comments' | '/drafts',
params?: FetchParams
- ): Promise<PathToCommentsInfoMap | undefined>;
+ ): Promise<{[path: string]: CommentInfo[]} | undefined>;
_getDiffComments(
changeNum: NumericChangeId,
@@ -2474,7 +2473,7 @@
): Promise<
| GetDiffCommentsOutput
| GetDiffRobotCommentsOutput
- | PathToCommentsInfoMap
+ | {[path: string]: CommentInfo[]}
| PathToRobotCommentsInfoMap
| undefined
> {
@@ -2496,7 +2495,7 @@
},
noAcceptHeader
) as Promise<
- PathToCommentsInfoMap | PathToRobotCommentsInfoMap | undefined
+ {[path: string]: CommentInfo[]} | PathToRobotCommentsInfoMap | undefined
>;
if (!basePatchNum && !patchNum && !path) {
@@ -2564,7 +2563,7 @@
getPortedComments(
changeNum: NumericChangeId,
revision: RevisionId
- ): Promise<PathToCommentsInfoMap | undefined> {
+ ): Promise<{[path: string]: CommentInfo[]} | undefined> {
// maintaining a custom error function so that errors do not surface in UI
const errFn: ErrorCallback = (response?: Response | null) => {
if (response)
@@ -2578,24 +2577,24 @@
});
}
- getPortedDrafts(
+ async getPortedDrafts(
changeNum: NumericChangeId,
revision: RevisionId
- ): Promise<PathToCommentsInfoMap | undefined> {
+ ): Promise<{[path: string]: DraftInfo[]} | undefined> {
// maintaining a custom error function so that errors do not surface in UI
const errFn: ErrorCallback = (response?: Response | null) => {
if (response)
console.info(`Fetching ported drafts failed, ${response.status}`);
};
- return this.getLoggedIn().then(loggedIn => {
- if (!loggedIn) return {};
- return this._getChangeURLAndFetch({
- changeNum,
- endpoint: '/ported_drafts/',
- revision,
- errFn,
- });
- });
+ const loggedIn = await this.getLoggedIn();
+ if (!loggedIn) return {};
+ const comments = (await this._getChangeURLAndFetch({
+ changeNum,
+ endpoint: '/ported_drafts/',
+ revision,
+ errFn,
+ })) as {[path: string]: CommentInfo[]} | undefined;
+ return addDraftProp(comments);
}
saveDiffDraft(
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index b1f1884..24ba0e5 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -64,7 +64,6 @@
Password,
PatchRange,
PatchSetNum,
- PathToCommentsInfoMap,
PathToRobotCommentsInfoMap,
PluginInfo,
PreferencesInfo,
@@ -412,16 +411,16 @@
getPortedComments(
changeNum: NumericChangeId,
revision: RevisionId
- ): Promise<PathToCommentsInfoMap | undefined>;
+ ): Promise<{[path: string]: CommentInfo[]} | undefined>;
getPortedDrafts(
changeNum: NumericChangeId,
revision: RevisionId
- ): Promise<PathToCommentsInfoMap | undefined>;
+ ): Promise<{[path: string]: DraftInfo[]} | undefined>;
getDiffComments(
changeNum: NumericChangeId
- ): Promise<PathToCommentsInfoMap | undefined>;
+ ): Promise<{[path: string]: CommentInfo[]} | undefined>;
getDiffComments(
changeNum: NumericChangeId,
basePatchNum: PatchSetNum,
@@ -434,7 +433,7 @@
patchNum?: PatchSetNum,
path?: string
):
- | Promise<PathToCommentsInfoMap | undefined>
+ | Promise<{[path: string]: CommentInfo[]} | undefined>
| Promise<GetDiffCommentsOutput>;
getDiffRobotComments(
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index 6e363d3..b60ace5 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -33,7 +33,6 @@
RepoAccessInfoMap,
IncludedInInfo,
CommentInfo,
- PathToCommentsInfoMap,
PluginInfo,
DocResult,
ContributorAgreementInfo,
@@ -59,6 +58,7 @@
UrlEncodedRepoName,
NumericChangeId,
PreferencesInput,
+ DraftInfo,
} from '../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../types/diff';
import {readResponsePayload} from '../../elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
@@ -373,10 +373,10 @@
getPlugins(): Promise<{[p: string]: PluginInfo} | undefined> {
return Promise.resolve({});
},
- getPortedComments(): Promise<PathToCommentsInfoMap | undefined> {
+ getPortedComments(): Promise<{[path: string]: CommentInfo[]} | undefined> {
return Promise.resolve({});
},
- getPortedDrafts(): Promise<PathToCommentsInfoMap | undefined> {
+ getPortedDrafts(): Promise<{[path: string]: DraftInfo[]} | undefined> {
return Promise.resolve({});
},
getPreferences(): Promise<PreferencesInfo | undefined> {
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index 32babc6..a06bffd 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -72,7 +72,6 @@
CommentThread,
DraftInfo,
ChangeMessage,
- UnsavedInfo,
DraftState,
} from '../types/common';
import {
@@ -115,6 +114,7 @@
import {RepoDetailView, RepoViewState} from '../models/views/repo';
import {AdminChildView, AdminViewState} from '../models/views/admin';
import {DashboardViewState} from '../models/views/dashboard';
+import {uuid} from '../utils/common-util';
const TEST_DEFAULT_EXPRESSION = 'label:Verified=MAX -label:Verified=MIN';
export const TEST_PROJECT_NAME: RepoName = 'test-project' as RepoName;
@@ -850,10 +850,11 @@
};
}
-export function createUnsaved(extra: Partial<CommentInfo> = {}): UnsavedInfo {
+export function createUnsaved(extra: Partial<CommentInfo> = {}): DraftInfo {
return {
...createComment(),
- __unsaved: true,
+ __draft: DraftState.UNSAVED,
+ client_id: uuid() as UrlEncodedCommentId,
id: undefined,
updated: undefined,
...extra,
@@ -996,6 +997,9 @@
export function createThread(
...comments: Partial<CommentInfo | DraftInfo>[]
): CommentThread {
+ if (comments.length === 0) {
+ comments = [createComment()];
+ }
return {
comments: comments.map(c => createComment(c)),
rootId: 'test-root-id-comment-thread' as UrlEncodedCommentId,
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index ef42397..c7331b7 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -698,55 +698,89 @@
}
export enum DraftState {
- SAVING = 'SAVING',
+ /**
+ * New draft created in this session and not yet saved.
+ * Must have a `client_id` set.
+ */
+ // Possible prior states: -
+ // Possible subsequent states: SAVING
+ UNSAVED = 'UNSAVED',
+ /**
+ * Currently saving to the backend.
+ */
+ // Possible prior states: UNSAVED, SAVED, ERROR
+ // Possible subsequent states: SAVED, ERROR
+ // TODO: Implement support of SAVING state.
+ // SAVING = 'SAVING',
+ /**
+ * Comment saved to the backend.
+ * Must have `id` and `updated` set.
+ */
+ // Possible prior states: SAVING
+ // Possible subsequent states: SAVING
SAVED = 'SAVED',
+ /**
+ * Latest saving attempt failed with an error.
+ */
+ // Possible prior states: SAVING
+ // Possible subsequent states: SAVING
+ // TODO: Implement support of ERROR state.
+ // ERROR = 'ERROR',
}
-export interface DraftCommentProps {
- // This must be set for all drafts. Drafts received from the backend will be
- // modified immediately with __draft:SAVED before allowing them to get into
- // the application state.
+export type DraftInfo = Omit<CommentInfo, 'id' | 'updated'> & {
+ // Must be set for all drafts.
+ // Drafts received from the backend will be modified immediately with
+ // `__draft: SAVED` before allowing them to get into the model.
__draft: DraftState;
-}
+ // Must be set for UNSAVED drafts. Can be set for other drafts.
+ // Use the id() utility function for uniquely identifying drafts.
+ client_id?: UrlEncodedCommentId;
+ // Must be set for SAVED drafts. Must not be set for UNSAVED drafts.
+ // Use the id() utility function for uniquely identifying drafts.
+ id?: UrlEncodedCommentId;
+ // Must be set for SAVED drafts. Can be set for other drafts. Will then just
+ // contain the last value of then the draft was previously SAVED.
+ updated?: Timestamp;
+};
-export interface UnsavedCommentProps {
- // This must be true for all unsaved comment drafts. An unsaved draft is
- // always just local to a comment component like <gr-comment> or
- // <gr-comment-thread>. Unsaved drafts will never appear in the application
- // state.
- __unsaved: boolean;
-}
-
-export type DraftInfo = CommentInfo & DraftCommentProps;
-
-export type UnsavedInfo = CommentBasics & UnsavedCommentProps;
-
-export type Comment = UnsavedInfo | DraftInfo | CommentInfo | RobotCommentInfo;
+/**
+ * This is what human, robot and draft comments can agree upon.
+ *
+ * Note that `id` and `updated` must be considered optional, because we might
+ * be dealing with unsaved draft comments.
+ */
+export type Comment = DraftInfo | CommentInfo | RobotCommentInfo;
// TODO: Replace the CommentMap type with just an array of paths.
export type CommentMap = {[path: string]: boolean};
-export function isRobot<T extends CommentBasics>(
- x: T | DraftInfo | RobotCommentInfo | undefined
+export function isRobot<T extends Comment>(
+ x: T | RobotCommentInfo | undefined
): x is RobotCommentInfo {
return !!x && !!(x as RobotCommentInfo).robot_id;
}
-export function isDraft<T extends CommentBasics>(
+export function isDraft<T extends Comment>(
x: T | DraftInfo | undefined
): x is DraftInfo {
- return !!x && !!(x as DraftInfo).__draft;
+ return (
+ !!x &&
+ (x as DraftInfo).__draft !== undefined &&
+ (x as DraftInfo).__draft !== DraftState.UNSAVED
+ );
}
-export function isUnsaved<T extends CommentBasics>(
- x: T | UnsavedInfo | undefined
-): x is UnsavedInfo {
- return !!x && !!(x as UnsavedInfo).__unsaved;
+export function isUnsaved<T extends Comment>(
+ x: T | DraftInfo | undefined
+): x is DraftInfo {
+ return !!x && (x as DraftInfo).__draft === DraftState.UNSAVED;
}
-export function isDraftOrUnsaved<T extends CommentBasics>(
- x: T | DraftInfo | UnsavedInfo | undefined
-): x is UnsavedInfo | DraftInfo {
+// TODO: Find a better name for this. Maybe this can become just `isDraft()`.
+export function isDraftOrUnsaved<T extends Comment>(
+ x: T | DraftInfo | undefined
+): x is DraftInfo {
return isDraft(x) || isUnsaved(x);
}
@@ -755,7 +789,7 @@
* This can only contain at most one draft. And if so, then it is the last
* comment in this list. This must not contain unsaved drafts.
*/
- comments: Array<CommentInfo | DraftInfo | RobotCommentInfo>;
+ comments: Array<Comment>;
/**
* Identical to the id of the first comment. If this is undefined, then the
* thread only contains an unsaved draft.
@@ -827,8 +861,6 @@
source_content_type?: string;
}
-export type PathToCommentsInfoMap = {[path: string]: CommentInfo[]};
-
/**
* The ContextLine entity contains the line number and line text of a single line of the source file content..
* https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#context-line
@@ -1326,18 +1358,6 @@
export type RobotCommentInput = RobotCommentInfo;
/**
- * This is what human, robot and draft comments can agree upon.
- *
- * Human, robot and saved draft comments all have a required id, but unsaved
- * drafts do not. That is why the id is omitted from CommentInfo, such that it
- * can be optional in Draft, but required in CommentInfo and RobotCommentInfo.
- */
-export interface CommentBasics extends Omit<CommentInfo, 'id' | 'updated'> {
- id?: UrlEncodedCommentId;
- updated?: Timestamp;
-}
-
-/**
* The RobotCommentInfo entity contains information about a robot inline comment
* https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#robot-comment-info
*/
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index f128cb5..2570708 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -4,10 +4,8 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {
- CommentBasics,
CommentInfo,
PatchSetNum,
- Timestamp,
UrlEncodedCommentId,
PatchRange,
PARENT,
@@ -23,7 +21,6 @@
CommentThread,
DraftInfo,
ChangeMessage,
- UnsavedInfo,
isRobot,
isDraft,
isDraftOrUnsaved,
@@ -38,11 +35,7 @@
import {DiffInfo} from '../types/diff';
import {FormattedReviewerUpdateInfo} from '../types/types';
import {extractMentionedUsers} from './account-util';
-
-interface SortableComment {
- updated: Timestamp;
- id: UrlEncodedCommentId;
-}
+import {assertIsDefined, uuid} from './common-util';
export function isFormattedReviewerUpdate(
message: ChangeMessage
@@ -57,48 +50,71 @@
export const PATCH_SET_PREFIX_PATTERN =
/^(?:Uploaded\s*)?[Pp]atch [Ss]et \d+:\s*(.*)/;
-export function sortComments<T extends SortableComment>(comments: T[]): T[] {
+/**
+ * We need a way to uniquely identify drafts. That is easy for all drafts that
+ * were already known to the backend at the time of change page load: They will
+ * have an `id` that we can use.
+ *
+ * For newly created drafts we start by setting a `client_id`, so that we can
+ * identify the draft even, if no `id` is available yet.
+ *
+ * If a comment with a `client_id` gets saved, then id gets an `id`, but we have
+ * to keep using the `client_id`, because that is what the UI is already using,
+ * e.g. in `repeat()` directives.
+ */
+export function id(comment: Comment): UrlEncodedCommentId {
+ if (isUnsaved(comment)) {
+ assertIsDefined(comment.client_id);
+ return comment.client_id;
+ }
+ if (isDraft(comment) && comment.client_id) {
+ return comment.client_id;
+ }
+ assertIsDefined(comment.id);
+ return comment.id;
+}
+
+export function sortComments<T extends Comment>(comments: T[]): T[] {
return comments.slice(0).sort((c1, c2) => {
+ const u1 = isUnsaved(c1);
+ const u2 = isUnsaved(c2);
+ if (u1 !== u2) return u1 ? 1 : -1;
+
const d1 = isDraft(c1);
const d2 = isDraft(c2);
if (d1 !== d2) return d1 ? 1 : -1;
- const date1 = parseDate(c1.updated);
- const date2 = parseDate(c2.updated);
- const dateDiff = date1.valueOf() - date2.valueOf();
- if (dateDiff !== 0) return dateDiff;
+ if (c1.updated && c2.updated) {
+ const date1 = parseDate(c1.updated);
+ const date2 = parseDate(c2.updated);
+ const dateDiff = date1.valueOf() - date2.valueOf();
+ if (dateDiff !== 0) return dateDiff;
+ }
- const id1 = c1.id;
- const id2 = c2.id;
+ const id1 = id(c1);
+ const id2 = id(c2);
return id1.localeCompare(id2);
});
}
-export function createUnsavedComment(thread: CommentThread): UnsavedInfo {
+export function createUnsaved(message?: string, unresolved?: boolean) {
return {
- path: thread.path,
- patch_set: thread.patchNum,
- side: thread.commentSide ?? CommentSide.REVISION,
- line: typeof thread.line === 'number' ? thread.line : undefined,
- range: thread.range,
- parent: thread.mergeParentNum,
- message: '',
- unresolved: true,
- __unsaved: true,
+ message,
+ unresolved,
+ __draft: DraftState.UNSAVED,
+ client_id: uuid() as UrlEncodedCommentId,
};
}
-export function createPatchsetLevelUnsavedDraft(
+export function createUnsavedPatchsetLevel(
patchNum?: PatchSetNumber,
message?: string,
unresolved?: boolean
-): UnsavedInfo {
+): DraftInfo {
return {
+ ...createUnsaved(message, unresolved),
patch_set: patchNum,
- message,
- unresolved,
path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
- __unsaved: true,
};
}
@@ -106,8 +122,9 @@
replyingTo: CommentInfo,
message: string,
unresolved: boolean
-): UnsavedInfo {
+): DraftInfo {
return {
+ ...createUnsaved(message, unresolved),
path: replyingTo.path,
patch_set: replyingTo.patch_set,
side: replyingTo.side,
@@ -115,13 +132,10 @@
range: replyingTo.range,
parent: replyingTo.parent,
in_reply_to: replyingTo.id,
- message,
- unresolved,
- __unsaved: true,
};
}
-export function createCommentThreads(comments: CommentInfo[]) {
+export function createCommentThreads(comments: Comment[]) {
const sortedComments = sortComments(comments);
const threads: CommentThread[] = [];
const idThreadMap: CommentIdToCommentThreadMap = {};
@@ -131,7 +145,7 @@
const thread = idThreadMap[comment.in_reply_to];
if (thread) {
thread.comments.push(comment);
- if (comment.id) idThreadMap[comment.id] = thread;
+ if (id(comment)) idThreadMap[id(comment)] = thread;
continue;
}
}
@@ -148,13 +162,13 @@
path: comment.path,
line: comment.line,
range: comment.range,
- rootId: comment.id,
+ rootId: id(comment),
};
if (!comment.line && !comment.range) {
newThread.line = 'FILE';
}
threads.push(newThread);
- if (comment.id) idThreadMap[comment.id] = newThread;
+ if (id(comment)) idThreadMap[id(comment)] = newThread;
}
return threads;
}
@@ -174,14 +188,16 @@
);
}
-export function getLastComment(thread: CommentThread): CommentInfo | undefined {
+export function getLastComment(
+ thread: CommentThread
+): CommentInfo | DraftInfo | undefined {
const len = thread.comments.length;
return thread.comments[len - 1];
}
export function getLastPublishedComment(
thread: CommentThread
-): CommentInfo | undefined {
+): CommentInfo | DraftInfo | undefined {
const publishedComments = thread.comments.filter(c => !isDraftOrUnsaved(c));
const len = publishedComments.length;
return publishedComments[len - 1];
@@ -189,7 +205,7 @@
export function getFirstComment(
thread: CommentThread
-): CommentInfo | undefined {
+): CommentInfo | DraftInfo | undefined {
return thread.comments[0];
}
@@ -296,10 +312,7 @@
/**
* Whether the given comment should be included in the given patch range.
*/
-export function isInPatchRange(
- comment: CommentBasics,
- range: PatchRange
-): boolean {
+export function isInPatchRange(comment: Comment, range: PatchRange): boolean {
return (
isInBaseOfPatchRange(comment, range) ||
isInRevisionOfPatchRange(comment, range)
@@ -421,7 +434,7 @@
return updated;
}
-export function reportingDetails(comment: CommentBasics) {
+export function reportingDetails(comment: Comment) {
return {
id: comment?.id,
message_length: comment?.message?.trim().length,
diff --git a/polygerrit-ui/app/utils/comment-util_test.ts b/polygerrit-ui/app/utils/comment-util_test.ts
index 4797aca..8a37a95 100644
--- a/polygerrit-ui/app/utils/comment-util_test.ts
+++ b/polygerrit-ui/app/utils/comment-util_test.ts
@@ -24,6 +24,8 @@
} from '../test/test-data-generators';
import {CommentSide} from '../constants/constants';
import {
+ Comment,
+ DraftInfo,
DraftState,
PARENT,
RevisionPatchSetNum,
@@ -128,13 +130,13 @@
});
test('comments sorting', () => {
- const comments = [
+ const comments: Comment[] = [
{
id: 'new_draft' as UrlEncodedCommentId,
message: 'i do not like either of you',
__draft: DraftState.SAVED,
updated: '2015-12-20 15:01:20.396000000' as Timestamp,
- },
+ } as DraftInfo,
{
id: 'sallys_confession' as UrlEncodedCommentId,
message: 'i like you, jack',
@@ -146,7 +148,7 @@
message: 'i like you, too',
updated: '2015-12-24 15:01:20.396000000' as Timestamp,
line: 1,
- in_reply_to: 'sallys_confession',
+ in_reply_to: 'sallys_confession' as UrlEncodedCommentId,
},
];
const sortedComments = sortComments(comments);
diff --git a/polygerrit-ui/app/utils/common-util.ts b/polygerrit-ui/app/utils/common-util.ts
index 183d167..56fe9b0 100644
--- a/polygerrit-ui/app/utils/common-util.ts
+++ b/polygerrit-ui/app/utils/common-util.ts
@@ -175,3 +175,7 @@
await navigator.clipboard.writeText(text);
fireAlert(document, `${copyTargetName ?? text} was copied to clipboard`);
}
+
+export function uuid() {
+ return Math.random().toString(36).substring(2);
+}
