Skip to content

Commit f429092

Browse files
joeldicksondicko2
joeldickson
and
dicko2
authored
Add check before delete comment (#165)
* Add check before delete comment * revert lint change * fix lint --------- Co-authored-by: jdickson <[email protected]>
1 parent c5b338f commit f429092

File tree

3 files changed

+122
-61
lines changed

3 files changed

+122
-61
lines changed

‎src/Provider/GitLab/GitLab.spec.ts‎

Lines changed: 66 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,24 @@ import { GitLabAdapter } from './GitLabAdapter';
1313
import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot';
1414

1515
const mockCurrentUserId = 123456;
16-
const mockNoteIdToBeDeleted = 6544321;
16+
17+
// Create helper function to generate consistent comment keys
18+
const generateCommentKey = (file: string, line: number | undefined, text: string) =>
19+
`${file}:${line}:${text}`;
1720

1821
const mockNotes = [
19-
{ id: 1, author: { id: mockCurrentUserId }, system: true },
20-
{ id: mockNoteIdToBeDeleted, author: { id: mockCurrentUserId }, system: false }, // only one to be deleted
21-
{ id: 3, author: { id: 99 }, system: false },
22+
{
23+
id: 1,
24+
author: { id: mockCurrentUserId },
25+
system: true,
26+
body: 'system message',
27+
},
28+
{
29+
id: 2,
30+
author: { id: mockCurrentUserId },
31+
system: false,
32+
body: 'existing comment',
33+
},
2234
] as MergeRequestNoteSchema[];
2335

2436
const mockVersionId = 3425234;
@@ -47,7 +59,11 @@ function createGitLab(service: IGitLabMRService, configs: ConfigArgument) {
4759
}
4860

4961
describe('VCS: GitLab', () => {
50-
it('should returns true when there is no error', async () => {
62+
beforeEach(() => {
63+
jest.clearAllMocks();
64+
});
65+
66+
it('should return true when there is no error', async () => {
5167
const service = new MrServiceMock();
5268
const gitLab = createGitLab(service, configs);
5369

@@ -60,7 +76,7 @@ describe('VCS: GitLab', () => {
6076
expect(result).toBe(true);
6177
});
6278

63-
it('should returns false when there is some error', async () => {
79+
it('should return false when there is some error', async () => {
6480
const service = new MrServiceMock();
6581
const gitLab = createGitLab(service, configs);
6682

@@ -74,40 +90,32 @@ describe('VCS: GitLab', () => {
7490
expect(result).toBe(false);
7591
});
7692

77-
it('should remove old self comments and reviews and post new ones', async () => {
93+
it('should not create duplicate comments and should create new unique comments', async () => {
7894
const service = new MrServiceMock();
79-
const gitLab = createGitLab(service, { ...configs, removeOldComment: true });
8095

81-
await gitLab.report([
82-
touchFileError,
83-
touchFileWarning,
84-
untouchedError,
85-
untouchedWarning,
96+
// Pre-populate with an existing comment for the error
97+
const existingErrorText = `${touchFileError.source}:${touchFileError.line}::rotating_light: ${touchFileError.msg} \n`;
98+
99+
service.listAllNotes.mockResolvedValue([
100+
...mockNotes,
101+
{
102+
id: 4,
103+
author: { id: mockCurrentUserId },
104+
system: false,
105+
body: existingErrorText,
106+
},
86107
]);
87108

88-
expect(service.listAllNotes).toHaveBeenCalledTimes(1);
89-
expect(service.getCurrentUserId).toHaveBeenCalledTimes(1);
90-
91-
expect(service.deleteNote).toHaveBeenCalledTimes(1);
92-
expect(service.deleteNote).toHaveBeenCalledWith(mockNoteIdToBeDeleted);
93-
94-
expect(service.createMRDiscussion).toHaveBeenNthCalledWith(
95-
1,
96-
mockVersion,
97-
touchFileError.source,
98-
touchFileError.line,
99-
expect.any(String),
100-
);
109+
const gitLab = createGitLab(service, configs);
110+
await gitLab.report([touchFileError, touchFileWarning]);
101111

102-
expect(service.createMRDiscussion).toHaveBeenNthCalledWith(
103-
2,
112+
expect(service.createMRDiscussion).toHaveBeenCalledTimes(1);
113+
expect(service.createMRDiscussion).toHaveBeenCalledWith(
104114
mockVersion,
105115
touchFileWarning.source,
106116
touchFileWarning.line,
107117
expect.any(String),
108118
);
109-
110-
expect(service.createNote).toHaveBeenCalledTimes(1);
111119
});
112120

113121
it('should not comment if there is no relevant lint issue', async () => {
@@ -120,39 +128,58 @@ describe('VCS: GitLab', () => {
120128
expect(service.createNote).not.toHaveBeenCalled();
121129
});
122130

131+
it('should not create comments that already exist', async () => {
132+
const service = new MrServiceMock();
133+
const gitLab = createGitLab(service, configs);
134+
135+
// Set up mock with existing summary comment
136+
const existingSummaryComment =
137+
'## CodeCoach reports 1 issue\n:rotating_light: 0 error\n:warning: 1 warning';
138+
service.listAllNotes.mockResolvedValue([
139+
{
140+
id: 1,
141+
author: { id: mockCurrentUserId },
142+
system: false,
143+
body: existingSummaryComment,
144+
},
145+
]);
146+
147+
await gitLab.report([touchFileWarning]);
148+
expect(service.createNote).not.toHaveBeenCalled();
149+
});
150+
123151
describe('when failOnWarnings is true', () => {
124-
it('should returns true when there is no error or warning', async () => {
152+
const warningConfigs = { ...configs, failOnWarnings: true };
153+
154+
it('should return true when there is no error or warning', async () => {
125155
const service = new MrServiceMock();
126-
const gitLab = createGitLab(service, { ...configs, failOnWarnings: true });
156+
const gitLab = createGitLab(service, warningConfigs);
127157

128158
const result = await gitLab.report([untouchedError, untouchedWarning]);
129-
130159
expect(result).toBe(true);
131160
});
132161

133-
it('should returns false when there is some error', async () => {
162+
it('should return false when there is some error', async () => {
134163
const service = new MrServiceMock();
135-
const gitLab = createGitLab(service, { ...configs, failOnWarnings: true });
164+
const gitLab = createGitLab(service, warningConfigs);
136165

137166
const result = await gitLab.report([
138167
touchFileError,
139168
untouchedError,
140169
untouchedWarning,
141170
]);
142-
143171
expect(result).toBe(false);
144172
});
145173

146-
it('should returns false when there is some warnings', async () => {
174+
it('should return false when there is some warnings', async () => {
147175
const service = new MrServiceMock();
148-
const gitLab = createGitLab(service, { ...configs, failOnWarnings: true });
176+
const gitLab = createGitLab(service, warningConfigs);
149177

150178
const result = await gitLab.report([
151179
touchFileWarning,
152180
untouchedError,
153181
untouchedWarning,
154182
]);
155-
156183
expect(result).toBe(false);
157184
});
158185
});

‎src/Provider/GitLab/GitLabAdapter.ts‎

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,68 @@ import { VCSAdapter } from '../@interfaces/VCSAdapter';
22
import { Diff } from '../../Git/@types/PatchTypes';
33
import { Log } from '../../Logger';
44
import { IGitLabMRService } from './IGitLabMRService';
5-
import { MergeRequestDiffVersionsSchema } from '@gitbeaker/core';
5+
import { MergeRequestDiffVersionsSchema, MergeRequestNoteSchema } from '@gitbeaker/core';
66
import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot';
77

88
export class GitLabAdapter implements VCSAdapter {
99
private latestMrVersion: MergeRequestDiffVersionsSchema;
10+
private existingComments: Set<string> = new Set();
11+
1012
constructor(private readonly mrService: IGitLabMRService) {}
1113

1214
async init(): Promise<void> {
13-
this.latestMrVersion = await this.mrService.getLatestVersion();
15+
const [latestVersion, userId, notes] = await Promise.all([
16+
this.mrService.getLatestVersion(),
17+
this.mrService.getCurrentUserId(),
18+
this.mrService.listAllNotes(),
19+
]);
20+
21+
this.latestMrVersion = latestVersion;
22+
23+
// Store existing bot comments
24+
notes
25+
.filter(
26+
(note: { author: { id: any }; system: any }) =>
27+
note.author.id === userId && !note.system,
28+
)
29+
.forEach((note: { body: string }) => this.existingComments.add(note.body));
30+
31+
Log.debug(`Found ${this.existingComments.size} existing CodeCoach comments`);
32+
}
33+
34+
private generateCommentKey(
35+
file: string,
36+
line: number | undefined,
37+
text: string,
38+
): string {
39+
return `${file}:${line}:${text}`;
40+
}
41+
42+
async createComment(comment: string): Promise<void> {
43+
if (!this.existingComments.has(comment)) {
44+
await this.mrService.createNote(comment);
45+
this.existingComments.add(comment);
46+
Log.debug('Created new comment');
47+
} else {
48+
Log.debug('Skipped creating duplicate comment');
49+
}
50+
}
51+
52+
async createReviewComment(
53+
text: string,
54+
file: string,
55+
line: number,
56+
nLines?: number,
57+
): Promise<void> {
58+
const commentKey = this.generateCommentKey(file, line, text);
59+
60+
if (!this.existingComments.has(commentKey)) {
61+
await this.mrService.createMRDiscussion(this.latestMrVersion, file, line, text);
62+
this.existingComments.add(commentKey);
63+
Log.debug('Created new review comment');
64+
} else {
65+
Log.debug('Skipped creating duplicate review comment');
66+
}
1467
}
1568

1669
async wrapUp(analyzer: IAnalyzerBot): Promise<boolean> {
@@ -28,26 +81,8 @@ export class GitLabAdapter implements VCSAdapter {
2881
diff(): Promise<Diff[]> {
2982
return this.mrService.diff();
3083
}
31-
createComment(comment: string): Promise<void> {
32-
return this.mrService.createNote(comment);
33-
}
34-
35-
createReviewComment(text: string, file: string, line: number): Promise<void> {
36-
return this.mrService.createMRDiscussion(this.latestMrVersion, file, line, text);
37-
}
3884

3985
async removeExistingComments(): Promise<void> {
40-
const [userId, notes] = await Promise.all([
41-
this.mrService.getCurrentUserId(),
42-
this.mrService.listAllNotes(),
43-
]);
44-
Log.debug('Get existing CodeCoach comments completed');
45-
46-
const deleteNotes = notes
47-
.filter((note) => note.author.id === userId && !note.system)
48-
.map((note) => this.mrService.deleteNote(note.id));
49-
50-
await Promise.all([...deleteNotes]);
51-
Log.debug('Delete CodeCoach comments completed');
86+
Log.debug('Skipping comment removal as we now handle duplicates on creation');
5287
}
5388
}

‎src/Provider/GitLab/IGitLabMRService.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ export interface IGitLabMRService {
77
deleteNote(noteId: number): Promise<void>;
88
getCurrentUserId(): Promise<number>;
99
diff(): Promise<Diff[]>;
10-
1110
getLatestVersion(): Promise<MergeRequestDiffVersionsSchema>;
1211
createMRDiscussion(
1312
latestVersion: MergeRequestDiffVersionsSchema,

0 commit comments

Comments
 (0)