diff --git a/changelog.d/1756.bugfix b/changelog.d/1756.bugfix new file mode 100644 index 00000000..bf6cf434 --- /dev/null +++ b/changelog.d/1756.bugfix @@ -0,0 +1 @@ +GitLab merge request comments are now correctly filtered based on label include / exclude configuration. \ No newline at end of file diff --git a/src/Bridge.ts b/src/Bridge.ts index 36bd695e..251375e7 100644 --- a/src/Bridge.ts +++ b/src/Bridge.ts @@ -463,7 +463,7 @@ export class Bridge { ...( iid ? connManager.getConnectionsForGitLabIssueWebhook(data.repository.homepage, iid) : []), ...connManager.getConnectionsForGitLabRepo(data.project.path_with_namespace), ]}, - (c, data) => c.onCommentCreated(data), + (c, data) => c instanceof GitLabRepoConnection ? c.onMergeRequestCommentCreated(data) : c.onCommentCreated(data), ); this.bindHandlerToQueue( @@ -733,13 +733,13 @@ export class Bridge { notifContent = await botUser.intent.underlyingClient.getRoomStateEvent( roomId, NotifFilter.StateType, "", ); - } catch (ex) { + } catch { try { notifContent = await botUser.intent.underlyingClient.getRoomStateEvent( roomId, NotifFilter.LegacyStateType, "", ); } - catch (ex) { + catch { // No state yet } } @@ -942,13 +942,13 @@ export class Bridge { } log.info(`Got message roomId=${roomId} type=${event.type} from=${event.sender}`); log.debug("Content:", JSON.stringify(event)); - let processedReply: any; + let processedReply; let processedReplyMetadata: IRichReplyMetadata|undefined = undefined; try { processedReply = await this.replyProcessor.processEvent(event, this.as.botClient, EventKind.RoomEvent); processedReplyMetadata = processedReply?.mx_richreply; } catch (ex) { - log.warn(`Event ${event.event_id} could not be processed by the reply processor, possibly a faulty event`); + log.warn(`Event ${event.event_id} could not be processed by the reply processor, possibly a faulty event`, ex); } const adminRoom = this.adminRooms.get(roomId); const checkPermission = (service: string, level: BridgePermissionLevel) => this.config.checkPermission(event.sender, service, level); diff --git a/src/CommentProcessor.ts b/src/CommentProcessor.ts index b2d0fb6e..19e60bc1 100644 --- a/src/CommentProcessor.ts +++ b/src/CommentProcessor.ts @@ -77,7 +77,7 @@ export class CommentProcessor { } public async getEventBodyForGitLabNote(comment: IGitLabWebhookNoteEvent): Promise { - let body = comment.object_attributes.description; + let body = comment.object_attributes.note; body = this.replaceMentions(body); body = await this.replaceImages(body, true); body = emoji.emojify(body); diff --git a/src/Connections/GitlabRepo.ts b/src/Connections/GitlabRepo.ts index 05e3eb9b..06e08489 100644 --- a/src/Connections/GitlabRepo.ts +++ b/src/Connections/GitlabRepo.ts @@ -897,15 +897,15 @@ ${data.description}`; ); } - public async onCommentCreated(event: IGitLabWebhookNoteEvent) { - if (this.hookFilter.shouldSkip('merge_request', 'merge_request.review')) { - return; - } - log.info(`onCommentCreated ${this.roomId} ${this.toString()} !${event.merge_request?.iid} ${event.object_attributes.id}`); + public async onMergeRequestCommentCreated(event: IGitLabWebhookNoteEvent) { if (!event.merge_request || event.object_attributes.noteable_type !== "MergeRequest") { // Not a MR comment return; } + if (this.hookFilter.shouldSkip('merge_request', 'merge_request.review') || !this.matchesLabelFilter(event.merge_request)) { + return; + } + log.info(`onCommentCreated ${this.roomId} ${this.toString()} !${event.merge_request?.iid} ${event.object_attributes.id}`); this.debounceMergeRequestReview(event.user, event.merge_request, event.project, { commentCount: 1, diff --git a/src/Gitlab/WebhookTypes.ts b/src/Gitlab/WebhookTypes.ts index c331df03..27e1e806 100644 --- a/src/Gitlab/WebhookTypes.ts +++ b/src/Gitlab/WebhookTypes.ts @@ -33,7 +33,7 @@ export interface IGitlabMergeRequest { iid: number; author_id: number; state: 'opened'|'closed'|'merged'; - + labels: IGitLabLabel[]; } export interface IGitLabMergeRequestObjectAttributes extends IGitlabMergeRequest { @@ -180,11 +180,12 @@ export interface IGitLabNote { noteable_type: 'MergeRequest'; author_id: number; noteable_id: number; - description: string; discussion_id?: string; + url: string; } export interface IGitLabWebhookNoteEvent { + object_kind: 'note', user: IGitlabUser; event_type: string; project: IGitlabProject; diff --git a/src/config/sections/encryption.ts b/src/config/sections/encryption.ts index 1dc96d16..a802d79a 100644 --- a/src/config/sections/encryption.ts +++ b/src/config/sections/encryption.ts @@ -1,7 +1,5 @@ import { ConfigError } from "../../errors"; import { configKey } from "../Decorators"; -import { BridgeConfigCache } from "./cache"; -import { BridgeConfigQueue } from "./queue"; interface BridgeConfigEncryptionYAML { storagePath: string; diff --git a/src/feeds/FeedReader.ts b/src/feeds/FeedReader.ts index 699598ad..7b7968c8 100644 --- a/src/feeds/FeedReader.ts +++ b/src/feeds/FeedReader.ts @@ -182,7 +182,7 @@ export class FeedReader { try { observedFeedUrls.add(normalizeUrl(conn.feedUrl)); } catch (err: unknown) { - log.error(`Invalid feedUrl for connection ${conn.connectionId}: ${conn.feedUrl}. It will not be tracked`); + log.error(`Invalid feedUrl for connection ${conn.connectionId}: ${conn.feedUrl}. It will not be tracked`, err); } } this.feedQueue.populate([...observedFeedUrls]); diff --git a/tests/connections/GitlabRepoTest.ts b/tests/connections/GitlabRepoTest.ts index 58315ba2..c3d05ab2 100644 --- a/tests/connections/GitlabRepoTest.ts +++ b/tests/connections/GitlabRepoTest.ts @@ -7,6 +7,7 @@ import { expect } from "chai"; import { BridgeConfigGitLab } from "../../src/config/Config"; import { IBridgeStorageProvider } from "../../src/Stores/StorageProvider"; import { IntentMock } from "../utils/IntentMock"; +import { IGitlabMergeRequest, IGitlabProject, IGitlabUser, IGitLabWebhookNoteEvent } from "../../src/Gitlab/WebhookTypes"; const ROOM_ID = "!foo:bar"; @@ -15,21 +16,26 @@ const GITLAB_ORG_REPO = { repo: "a-fake-repo", }; -const GITLAB_MR = { +const GITLAB_MR: IGitlabMergeRequest = { + author_id: 0, + labels: [], state: "opened", iid: 1234, url: `https://gitlab.example.com/${GITLAB_ORG_REPO.org}/${GITLAB_ORG_REPO.repo}/issues/1234`, title: "My MR", }; -const GITLAB_USER = { +const GITLAB_USER: IGitlabUser = { name: "Alice", username: "alice", + avatar_url: "", + email: "alice@example.org" }; -const GITLAB_PROJECT = { +const GITLAB_PROJECT: IGitlabProject = { path_with_namespace: `${GITLAB_ORG_REPO.org}/${GITLAB_ORG_REPO.repo}`, web_url: `https://gitlab.example.com/${GITLAB_ORG_REPO.org}/${GITLAB_ORG_REPO.repo}`, + homepage: "", }; const GITLAB_ISSUE_CREATED_PAYLOAD = { @@ -39,7 +45,7 @@ const GITLAB_ISSUE_CREATED_PAYLOAD = { project: GITLAB_PROJECT, }; -const GITLAB_MR_COMMENT = { +const GITLAB_MR_COMMENT: IGitLabWebhookNoteEvent = { 'object_kind': 'note', 'event_type': 'note', 'merge_request': GITLAB_MR, @@ -47,15 +53,24 @@ const GITLAB_MR_COMMENT = { 'discussion_id': '6babfc4ad3be2355db286ed50be111a5220d5751', 'note': 'I am starting a new thread', 'noteable_type': 'MergeRequest', - 'url': 'https://gitlab.com/tadeuszs/my-awesome-project/-/merge_requests/2#note_1455087141' + 'url': 'https://gitlab.com/tadeuszs/my-awesome-project/-/merge_requests/2#note_1455087141', + 'id': 1455087141, + 'author_id': 12345, + 'noteable_id': 1, }, 'project': GITLAB_PROJECT, 'user': GITLAB_USER, + repository: { + 'description': 'A repo', + 'homepage': 'https://gitlab.com/tadeuszs/my-awesome-project', + 'name': 'a-repo', + 'url': 'https://gitlab.com/tadeuszs/my-awesome-project' + }, }; const COMMENT_DEBOUNCE_MS = 25; -function createConnection(state: Record = {}, isExistingState=false): { connection: GitLabRepoConnection, intent: IntentMock } { +function createConnection(state: Partial = {}, isExistingState=false): { connection: GitLabRepoConnection, intent: IntentMock } { const mq = createMessageQueue(); mq.subscribe('*'); const as = AppserviceMock.create(); @@ -164,10 +179,10 @@ describe("GitLabRepoConnection", () => { }); }); - describe("onCommentCreated", () => { + describe("onMergeRequestCommentCreated", () => { it("will handle an MR comment", async () => { const { connection, intent } = createConnection(); - await connection.onCommentCreated(GITLAB_MR_COMMENT as never); + await connection.onMergeRequestCommentCreated(GITLAB_MR_COMMENT); await waitForDebouncing(); intent.expectEventMatches( (ev: any) => ev.content.body.includes('**Alice** commented on MR'), @@ -175,10 +190,41 @@ describe("GitLabRepoConnection", () => { ); }); + it("will filter out issues not matching includingLabels.", async () => { + const { connection, intent } = createConnection({ + includingLabels: ["include-me"] + }); + // ..or issues with no labels + await connection.onMergeRequestCommentCreated(GITLAB_MR_COMMENT); + await waitForDebouncing(); + intent.expectNoEvent(); + }); + + it.only("will filter out issues matching excludingLabels.", async () => { + const { connection, intent } = createConnection({ + excludingLabels: ["exclude-me"] + }); + // ..or issues with no labels + await connection.onMergeRequestCommentCreated({ + ...GITLAB_MR_COMMENT, + merge_request: { + ...GITLAB_MR, + labels: [{ + id: 0, + title: 'exclude-me' + } as any] + } + }); + await waitForDebouncing(); + intent.expectNoEvent(); + }); + + + it("will debounce MR comments", async () => { const { connection, intent } = createConnection(); - await connection.onCommentCreated(GITLAB_MR_COMMENT as never); - await connection.onCommentCreated({ + await connection.onMergeRequestCommentCreated(GITLAB_MR_COMMENT); + await connection.onMergeRequestCommentCreated({ ...GITLAB_MR_COMMENT, 'object_attributes': { ...GITLAB_MR_COMMENT.object_attributes, @@ -197,9 +243,9 @@ describe("GitLabRepoConnection", () => { it("will add new comments in a Matrix thread", async () => { const { connection, intent } = createConnection(); - await connection.onCommentCreated(GITLAB_MR_COMMENT as never); + await connection.onMergeRequestCommentCreated(GITLAB_MR_COMMENT); await waitForDebouncing(); - await connection.onCommentCreated(GITLAB_MR_COMMENT as never); + await connection.onMergeRequestCommentCreated(GITLAB_MR_COMMENT); await waitForDebouncing(); expect(intent.sentEvents.length).to.equal(2); intent.expectEventMatches( @@ -211,14 +257,14 @@ describe("GitLabRepoConnection", () => { it("will correctly map new comments to aggregated discussions", async () => { const { connection, intent } = createConnection(); - await connection.onCommentCreated({ + await connection.onMergeRequestCommentCreated({ ...GITLAB_MR_COMMENT, 'object_attributes': { ...GITLAB_MR_COMMENT.object_attributes, 'discussion_id': 'disc1', }, } as never); - await connection.onCommentCreated({ + await connection.onMergeRequestCommentCreated({ ...GITLAB_MR_COMMENT, 'object_attributes': { ...GITLAB_MR_COMMENT.object_attributes, @@ -228,7 +274,7 @@ describe("GitLabRepoConnection", () => { await waitForDebouncing(); expect(intent.sentEvents.length).to.equal(1); - await connection.onCommentCreated({ + await connection.onMergeRequestCommentCreated({ ...GITLAB_MR_COMMENT, 'object_attributes': { ...GITLAB_MR_COMMENT.object_attributes, @@ -243,7 +289,7 @@ describe("GitLabRepoConnection", () => { 1 ); - await connection.onCommentCreated({ + await connection.onMergeRequestCommentCreated({ ...GITLAB_MR_COMMENT, 'object_attributes': { ...GITLAB_MR_COMMENT.object_attributes, @@ -300,7 +346,7 @@ describe("GitLabRepoConnection", () => { it("will include issues matching includingLabels.", async () => { const { connection, intent } = createConnection({ - includingIssues: ["include-me"] + includingLabels: ["include-me"] }); await connection.onMergeRequestOpened({ ...GITLAB_ISSUE_CREATED_PAYLOAD,