From 1efe5879f8e5d21f2b5db958115e939540655ecb Mon Sep 17 00:00:00 2001 From: zhanglei Date: Sun, 2 Aug 2026 16:47:18 +0800 Subject: [PATCH 1/2] T001: notify GitLab MR approval changes --- gitlab_bot.py | 14 ++ src/approval_notification.py | 204 ++++++++++++++++++++++++++++ tests/fixtures/approval_webhook.py | 45 ++++++ tests/test_approval_notification.py | 168 +++++++++++++++++++++++ 4 files changed, 431 insertions(+) create mode 100644 src/approval_notification.py create mode 100644 tests/fixtures/approval_webhook.py create mode 100644 tests/test_approval_notification.py diff --git a/gitlab_bot.py b/gitlab_bot.py index d1f1dc6..c740641 100644 --- a/gitlab_bot.py +++ b/gitlab_bot.py @@ -17,6 +17,7 @@ from dotenv import load_dotenv +from src.approval_notification import ApprovalNotificationHooks, LogChannel from src.config import ( bot_gitlab_token, bot_gitlab_url, @@ -57,6 +58,7 @@ def _load_gitlab_bot(): issue_hooks = IssueHooks() merge_request_hooks = MergeRequestHooks() note_hooks = NoteHooks() +approval_notification_hooks = ApprovalNotificationHooks(LogChannel()) @bot.router.register("Issue Hook", action="open") @@ -101,6 +103,18 @@ async def merge_request_reopen_event(event, gl, *args, **kwargs): await merge_request_hooks.merge_request_reopen_event(event, gl, args, kwargs) +@bot.router.register("Merge Request Hook", action="approved") +@bot.router.register("Merge Request Hook", action="approval") +async def merge_request_approval_event(event, gl, *args, **kwargs): + await approval_notification_hooks.handle(event, gl, *args, **kwargs) + + +@bot.router.register("Merge Request Hook", action="unapproved") +@bot.router.register("Merge Request Hook", action="unapproval") +async def merge_request_unapproval_event(event, gl, *args, **kwargs): + await approval_notification_hooks.handle(event, gl, *args, **kwargs) + + @bot.router.register("Note Hook", noteable_type="MergeRequest") async def note_merge_request_event(event, gl, *args, **kwargs): if not ignore_event(event): diff --git a/src/approval_notification.py b/src/approval_notification.py new file mode 100644 index 0000000..c4fe251 --- /dev/null +++ b/src/approval_notification.py @@ -0,0 +1,204 @@ +# Copyright 2026 Lei Zhang +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import json +import logging +from abc import ABC, abstractmethod +from dataclasses import asdict, dataclass +from typing import Any, Dict, Mapping, Optional + +APPROVAL_ACTIONS = frozenset(("approval", "approved", "unapproval", "unapproved")) +NORMALIZED_ACTIONS = { + "approval": "approval", + "approved": "approval", + "unapproval": "unapproval", + "unapproved": "unapproval", +} + + +@dataclass +class MergeRequestNotification: + """Normalized, channel-independent information about an MR review action.""" + + source: str + event_type: str + action: str + webhook_action: str + message: str + project: Dict[str, Any] + merge_request: Dict[str, Any] + actor: Dict[str, Any] + occurred_at: Optional[str] + raw_payload: Optional[Mapping[str, Any]] = None + + +class Channel(ABC): + """Asynchronous destination for normalized notifications.""" + + @abstractmethod + async def send(self, notification: MergeRequestNotification) -> None: + raise NotImplementedError + + +class LogChannel(Channel): + """Write normalized notifications as searchable JSON log records.""" + + def __init__(self, logger: Optional[logging.Logger] = None): + self.logger = logger or logging.getLogger(__name__) + + async def send(self, notification: MergeRequestNotification) -> None: + self.logger.info(json.dumps(asdict(notification), ensure_ascii=False)) + + +def _first_value(data: Mapping[str, Any], *keys: str) -> Any: + for key in keys: + value = data.get(key) + if value is not None and value != "": + return value + return None + + +def _require_mapping(data: Any, name: str) -> Mapping[str, Any]: + if not isinstance(data, Mapping): + raise ValueError(f"{name} must be an object") + return data + + +def _require_value(data: Mapping[str, Any], name: str) -> Any: + value = data.get(name) + if value is None or value == "": + raise ValueError(f"{name} is required") + return value + + +def _build_message( + action: str, + actor_name: str, + title: str, + iid: Any, + project_label: str, + mr_url: Optional[str], +) -> str: + if action == "approval": + action_text = "approved" + else: + action_text = "canceled approval (unapproval) for" + + message = f"{actor_name} {action_text} MR !{iid}: {title} (project: {project_label})" + if mr_url: + message += f" {mr_url}" + else: + message += " (MR URL unavailable)" + return message + + +def build_notification(data: Mapping[str, Any]) -> MergeRequestNotification: + """Convert a GitLab merge request webhook payload into a notification.""" + + payload = _require_mapping(data, "payload") + attributes = _require_mapping(payload.get("object_attributes"), "object_attributes") + project_data = _require_mapping(payload.get("project"), "project") + actor_data = _require_mapping(payload.get("user"), "user") + + webhook_action = _require_value(attributes, "action") + if not isinstance(webhook_action, str) or webhook_action not in APPROVAL_ACTIONS: + raise ValueError(f"unsupported approval action: {webhook_action}") + action = NORMALIZED_ACTIONS[webhook_action] + + project_id = _require_value(project_data, "id") + iid = _require_value(attributes, "iid") + username = _require_value(actor_data, "username") + + project_path = _first_value(project_data, "path_with_namespace", "path", "name") + project_url = _first_value(project_data, "web_url", "url") + mr_url = _first_value(attributes, "url", "web_url") + if mr_url is None and project_url is not None: + mr_url = f"{str(project_url).rstrip('/')}/-/merge_requests/{iid}" + + title = attributes.get("title") + title_text = str(title) if title is not None and title != "" else "(untitled)" + project_label = str(project_path) if project_path is not None else str(project_id) + actor_name = str(_first_value(actor_data, "name", "username")) + if actor_name != str(username): + actor_name = f"{actor_name} (@{username})" + + return MergeRequestNotification( + source="gitlab", + event_type="merge_request_review", + action=action, + webhook_action=webhook_action, + message=_build_message( + action, + actor_name, + title_text, + iid, + project_label, + str(mr_url) if mr_url is not None else None, + ), + project={ + "id": project_id, + "path": project_path, + "url": project_url, + }, + merge_request={ + "iid": iid, + "title": title, + "url": mr_url, + }, + actor={ + "id": actor_data.get("id"), + "username": username, + "name": actor_data.get("name"), + }, + occurred_at=_first_value(attributes, "actioned_at", "updated_at"), + ) + + +class ApprovalNotificationHooks: + """Handle approval webhooks without invoking any GitLab API.""" + + def __init__(self, channel: Channel, logger: Optional[logging.Logger] = None): + self.channel = channel + self.logger = logger or logging.getLogger(__name__) + + async def handle(self, event, *args, **kwargs) -> None: + try: + data = event.data + if not isinstance(data, Mapping): + raise ValueError("payload must be an object") + attributes = data.get("object_attributes") + if not isinstance(attributes, Mapping): + raise ValueError("object_attributes must be an object") + action = attributes.get("action") + if action is None or action == "": + raise ValueError("object_attributes.action is required") + if action not in APPROVAL_ACTIONS: + self.logger.debug("Skip approval notification for action=%r", action) + return + notification = build_notification(data) + except Exception as exc: + self.logger.error("invalid approval webhook: %s", exc) + return + + try: + await self.channel.send(notification) + except Exception as exc: + self.logger.error( + "approval notification channel failed (action=%s, project=%s, mr_iid=%s): %s", + notification.action, + notification.project.get("path") or notification.project.get("id"), + notification.merge_request.get("iid"), + exc, + exc_info=True, + ) diff --git a/tests/fixtures/approval_webhook.py b/tests/fixtures/approval_webhook.py new file mode 100644 index 0000000..e352eda --- /dev/null +++ b/tests/fixtures/approval_webhook.py @@ -0,0 +1,45 @@ +"""Representative approval and unapproval webhook payloads. + +These payloads follow the GitLab merge request webhook shape. Replace or +augment them with a redacted GitLab Community Edition 16.11 capture when the +target-instance smoke fixture is available. +""" + +from copy import deepcopy + + +def make_approval_webhook(action="approval", username="reviewer"): + return { + "object_kind": "merge_request", + "event_type": "merge_request", + "user": { + "id": 7, + "name": "Reviewer", + "username": username, + }, + "project": { + "id": 76, + "name": "Project", + "path_with_namespace": "group/project", + "web_url": "https://gitlab.example.com/group/project", + }, + "object_attributes": { + "action": action, + "iid": 12, + "title": "Add feature", + "url": "https://gitlab.example.com/group/project/-/merge_requests/12", + "updated_at": "2026-08-02T10:00:00Z", + }, + } + + +APPROVAL_WEBHOOK = make_approval_webhook() +UNAPPROVAL_WEBHOOK = make_approval_webhook(action="unapproval") + + +def copy_webhook(action="approval", username="reviewer"): + if action == "approval" and username == "reviewer": + return deepcopy(APPROVAL_WEBHOOK) + if action == "unapproval" and username == "reviewer": + return deepcopy(UNAPPROVAL_WEBHOOK) + return make_approval_webhook(action=action, username=username) diff --git a/tests/test_approval_notification.py b/tests/test_approval_notification.py new file mode 100644 index 0000000..9d163a0 --- /dev/null +++ b/tests/test_approval_notification.py @@ -0,0 +1,168 @@ +import asyncio +import json +import logging +from types import SimpleNamespace + +import pytest +from gidgetlab.sansio import Event + +import gitlab_bot +from src.approval_notification import ( + ApprovalNotificationHooks, + LogChannel, + build_notification, +) +from tests.fixtures.approval_webhook import copy_webhook + + +class RecordingChannel: + def __init__(self): + self.notifications = [] + + async def send(self, notification): + self.notifications.append(notification) + + +class FailingChannel: + async def send(self, notification): + raise RuntimeError("channel unavailable") + + +def make_event(action="approval", username="reviewer"): + return SimpleNamespace(data=copy_webhook(action=action, username=username)) + + +def test_build_notification_contains_human_readable_message_and_links(): + notification = build_notification(copy_webhook()) + + assert notification.action == "approval" + assert "reviewer" in notification.message + assert "Add feature" in notification.message + assert "!12" in notification.message + assert "https://gitlab.example.com/group/project/-/merge_requests/12" in notification.message + assert notification.project["id"] == 76 + assert notification.merge_request["iid"] == 12 + assert notification.actor["username"] == "reviewer" + + +def test_build_unapproval_notification_uses_human_readable_action(): + notification = build_notification(copy_webhook(action="unapproval")) + + assert notification.action == "unapproval" + assert "取消" in notification.message or "unapproval" in notification.message + assert "Add feature" in notification.message + + +def test_log_channel_emits_structured_json_with_readable_message(caplog): + logger = logging.getLogger("test.approval_notification") + channel = LogChannel(logger) + notification = build_notification(copy_webhook()) + caplog.set_level(logging.INFO, logger="test.approval_notification") + + asyncio.run(channel.send(notification)) + + payload = json.loads(caplog.records[-1].message) + assert payload["message"] == notification.message + assert payload["action"] == "approval" + assert payload["merge_request"]["url"].endswith("/merge_requests/12") + + +@pytest.mark.parametrize( + ("action", "expected_action"), + [("approval", "approval"), ("approved", "approval"), ("unapproval", "unapproval"), ("unapproved", "unapproval")], +) +@pytest.mark.parametrize("username", ["reviewer", "review-bot"]) +def test_router_sends_user_and_bot_approval_events_without_global_filter( + monkeypatch, action, expected_action, username +): + channel = RecordingChannel() + monkeypatch.setattr(gitlab_bot, "approval_notification_hooks", ApprovalNotificationHooks(channel)) + monkeypatch.setattr(gitlab_bot, "bot_gitlab_username", "review-bot") + event_data = copy_webhook(action=action, username=username) + event = Event(event_data, event="Merge Request Hook") + + asyncio.run(gitlab_bot.bot.router.dispatch(event, None)) + + assert len(channel.notifications) == 1 + assert channel.notifications[0].action == expected_action + assert channel.notifications[0].webhook_action == action + + +def test_noncritical_fields_are_null_and_notification_is_still_sent(): + payload = copy_webhook() + payload["project"].pop("path_with_namespace") + payload["project"].pop("name") + payload["project"].pop("web_url") + payload["object_attributes"].pop("title") + payload["object_attributes"].pop("url") + payload["user"].pop("id") + payload["user"].pop("name") + payload["object_attributes"].pop("updated_at") + channel = RecordingChannel() + + asyncio.run(ApprovalNotificationHooks(channel).handle(SimpleNamespace(data=payload))) + + notification = channel.notifications[0] + assert notification.project["path"] is None + assert notification.project["url"] is None + assert notification.merge_request["title"] is None + assert notification.merge_request["url"] is None + assert notification.actor["id"] is None + assert notification.actor["name"] is None + assert notification.occurred_at is None + + +def test_approval_notification_does_not_call_gitlab_api(): + channel = RecordingChannel() + hooks = ApprovalNotificationHooks(channel) + api = SimpleNamespace( + getitem=lambda *_args, **_kwargs: pytest.fail("approval notification must not call GitLab API"), + post=lambda *_args, **_kwargs: pytest.fail("approval notification must not call GitLab API"), + ) + + asyncio.run(hooks.handle(make_event(), api)) + + assert len(channel.notifications) == 1 + + +def test_channel_failure_is_logged_and_does_not_escape(caplog): + logger = logging.getLogger("test.approval_notification.failure") + hooks = ApprovalNotificationHooks(FailingChannel(), logger=logger) + caplog.set_level(logging.ERROR, logger="test.approval_notification.failure") + + asyncio.run(hooks.handle(make_event())) + + assert "channel unavailable" in caplog.text + + +def test_invalid_payload_is_logged_and_skipped(caplog): + logger = logging.getLogger("test.approval_notification.invalid") + channel = RecordingChannel() + hooks = ApprovalNotificationHooks(channel, logger=logger) + caplog.set_level(logging.ERROR, logger="test.approval_notification.invalid") + invalid_event = SimpleNamespace(data={"object_attributes": {"action": "approval"}}) + + asyncio.run(hooks.handle(invalid_event)) + + assert channel.notifications == [] + assert "invalid approval webhook" in caplog.text + + +def test_non_target_action_is_skipped(): + channel = RecordingChannel() + hooks = ApprovalNotificationHooks(channel) + + asyncio.run(hooks.handle(make_event(action="open"))) + + assert channel.notifications == [] + + +def test_duplicate_delivery_is_attempted_each_time(): + channel = RecordingChannel() + hooks = ApprovalNotificationHooks(channel) + event = make_event() + + asyncio.run(hooks.handle(event)) + asyncio.run(hooks.handle(event)) + + assert len(channel.notifications) == 2 From 4a01ec6daf00f474235d26cdb255a78742f7878a Mon Sep 17 00:00:00 2001 From: zhanglei Date: Sun, 2 Aug 2026 16:58:33 +0800 Subject: [PATCH 2/2] T002: isolate test configuration from local env --- tests/conftest.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 tests/conftest.py diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..80032c9 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,18 @@ +"""Keep test configuration independent from a developer's local .env file.""" + +import os + +os.environ.update( + { + "BOT_GIT_COMMIT_MESSAGE_CHECK_ENABLED": "true", + "BOT_GIT_COMMIT_SUBJECT_REGEX_ENABLED": "true", + "BOT_GIT_COMMIT_SUBJECT_REGEX": ( + r"^(\[(fix|feat)\]:\[.*]\[.*\]|" + r"\[(docs|style|ref|test|chore|tag|revert|perf)\]:\[.*\])$" + ), + "BOT_GIT_EMAIL_DOMAIN": "asiainfo.com", + "BOT_GITLAB_MERGE_REQUEST_MILESTONE_REQUIRED": "false", + "BOT_GITLAB_MERGE_REQUEST_ISSUE_REQUIRED": "false", + "BOT_GITLAB_MERGE_REQUEST_APPROVAL_ENABLED": "true", + } +)