Filter GitLab comments with include/exclude label config. (#1018)

* Filter GitLab comments with include/exclude label config.

* Start working on tests

* fix types

* fixup tests

* changelog

* lint

changelog.d/1756.bugfix

@@ -0,0 +1 @@
+GitLab merge request comments are now correctly filtered based on label include / exclude configuration.

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<IGitLabWebhookIssueStateEvent, GitLabIssueConnection>(
@@ -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);

src/CommentProcessor.ts

@@ -77,7 +77,7 @@ export class CommentProcessor {
     }
 
     public async getEventBodyForGitLabNote(comment: IGitLabWebhookNoteEvent): Promise<MatrixMessageContent> {
-        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);

src/Connections/GitlabRepo.ts

@@ -897,15 +897,15 @@ ${data.description}`;
         );
     }
 
-    public async onCommentCreated(event: IGitLabWebhookNoteEvent) {
+    public async onMergeRequestCommentCreated(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}`);
         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,

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;

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;

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]);

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<string, unknown> = {}, isExistingState=false): { connection: GitLabRepoConnection, intent: IntentMock } {
+function createConnection(state: Partial<GitLabRepoConnectionState> = {}, 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.onMergeRequestCommentCreated(GITLAB_MR_COMMENT);
-			await connection.onCommentCreated({
+			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,