diff --git a/CLAUDE.md b/CLAUDE.md index 22da103..f8c4369 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -79,6 +79,9 @@ DynamoDB stores only sites, organisers, and registrations. `commands/github/invite_flow.py` contains the shared org-invite + team-add logic used by both the slash command (`add_member.py`) and the message shortcut (`add_member_shortcut.py`). Both pass a `reply` callback for message delivery. +Channel replies are wrapped in `_safe_reply()` which catches exceptions; on +failure, `_reply_or_dm()` falls back to DMing the caller. On success the caller +always receives a DM confirmation as a failsafe. ### On-call rotation @@ -111,7 +114,7 @@ members have been assigned in the current window, it cycles through again. - Two-tier permissions: `@core-team` Slack user group = global admin, site organisers = scoped to their hackathon site - All bot responses are ephemeral (only visible to the caller) unless explicitly - posting to a channel (e.g. `github add-member` posts visible thread replies) + posting to a channel (e.g. `github add` posts visible thread replies) - Form YAML supports `options_from: sites` for dynamic option lists populated from DynamoDB, and `options_from: countries` for type-ahead country search - GitHub API calls use a fine-grained PAT with `admin:org` scope diff --git a/README.md b/README.md index 40566b8..c42535b 100644 --- a/README.md +++ b/README.md @@ -9,27 +9,27 @@ AWS ECS Fargate + DynamoDB. This app adds two slash-commands which can be used by anyone in the nf-core slack. -All responses are **ephemeral** (only visible to you), except -`github add-member` which posts visible thread replies. +All responses are **ephemeral** (only visible to you), except `github add` which +posts visible thread replies. See [docs/commands.md](docs/commands.md) for the full command reference. ## General Automation ```bash -/nf-core help # General help -/nf-core github add-member @user # Invite to nf-core GitHub org -/nf-core github add-member # Invite by GitHub username +/nf-core help # General help +/nf-core github add @user # Invite to nf-core GitHub org +/nf-core github add # Invite by GitHub username ``` -The `github add-member` functionality works best when coming from the +The `github add` functionality works best when coming from the `#github-invitations` channel: right-click any message → **More actions** → **Add to GitHub org** to invite the message author. This automatically finds the GitHub username from the Slack workflow message and sends them an invite, with membership in the _Collaborators_ team. -The slach commands `/nf-core github add-member` are mostly for convenience when +The slash commands `/nf-core github add` are mostly for convenience when replying elsewhere in Slack. ## Hackathon Registrations diff --git a/docs/commands.md b/docs/commands.md index e72c3a1..cb5c93f 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -285,10 +285,10 @@ These commands manage nf-core GitHub organisation membership. All require Show GitHub command help. -### `github add-member` +### `github add` ``` -/nf-core github add-member <@slack-user|github-username> +/nf-core github add <@slack-user|github-username> ``` Invite a user to the nf-core GitHub organisation and add them to the @@ -304,8 +304,8 @@ Invite a user to the nf-core GitHub organisation and add them to the **Examples:** ``` -/nf-core github add-member @alice -/nf-core github add-member octocat +/nf-core github add @alice +/nf-core github add octocat ``` ### Message Shortcut: Add to GitHub org @@ -319,7 +319,7 @@ threads). **Permissions:** `@core-team` only. -### How `add-member` Works +### How `add` Works 1. **Permission check** — verifies you're in the `@core-team` Slack user group 2. **Target resolution** — determines who to invite: diff --git a/docs/slack-app-setup.md b/docs/slack-app-setup.md index fbd0a24..844f817 100644 --- a/docs/slack-app-setup.md +++ b/docs/slack-app-setup.md @@ -44,7 +44,7 @@ public URL. - **Request URL:** `https://example.com/slack/events` (Socket Mode ignores this, but Slack requires a value) - **Short Description:** `nf-core community bot` - - **Usage Hint:** `[help | github add-member | hackathon register]` + - **Usage Hint:** `[help | github add | hackathon register]` - **Escape channels, users, and links sent to your app:** check this box 4. Click **Save** diff --git a/src/nf_core_bot/commands/github/add_member.py b/src/nf_core_bot/commands/github/add_member.py index 9b3c686..9b44a5f 100644 --- a/src/nf_core_bot/commands/github/add_member.py +++ b/src/nf_core_bot/commands/github/add_member.py @@ -1,10 +1,10 @@ -"""``/nf-core github add-member`` — invite a user to the nf-core GitHub org. +"""``/nf-core github add`` — invite a user to the nf-core GitHub org. Usage (explicit Slack mention): - ``/nf-core github add-member @slack-user`` + ``/nf-core github add @slack-user`` Usage (explicit GitHub username): - ``/nf-core github add-member octocat`` + ``/nf-core github add octocat`` To invite the author of a specific message, use the "Add to GitHub org" message shortcut (right-click a message → More actions) instead. @@ -39,7 +39,7 @@ async def handle_add_member( command: dict[str, str], args: list[str], ) -> None: - """Handle ``/nf-core github add-member [target]``.""" + """Handle ``/nf-core github add [target]``.""" await ack() # ── 1. Permission check ────────────────────────────────────────── @@ -66,7 +66,7 @@ async def handle_add_member( target_user_id = mention_match.group(1) github_username = await get_github_username(client, target_user_id) if github_username is None: - await _warn_missing_github(client, channel_id, thread_ts, target_user_id) + await _warn_missing_github(client, channel_id, thread_ts, target_user_id, respond) return else: # Treat it as a plain GitHub username — validate first @@ -81,7 +81,7 @@ async def handle_add_member( else: # No argument provided await respond( - "Usage: `/nf-core github add-member [@user | github-username]`\n\n" + "Usage: `/nf-core github add [@user | github-username]`\n\n" "You can also right-click a message and use *More actions → Add to GitHub org* " "to invite the message author.", response_type="ephemeral", @@ -95,7 +95,7 @@ async def handle_add_member( async def _reply(text: str) -> None: await _thread_reply(client, channel_id, thread_ts, text) - await invite_and_greet(github_username, user_id, _reply, greeting_user_id=target_user_id) + await invite_and_greet(github_username, user_id, _reply, greeting_user_id=target_user_id, client=client) # ── Helpers ────────────────────────────────────────────────────────── @@ -106,16 +106,25 @@ async def _warn_missing_github( channel_id: str, thread_ts: str, target_user_id: str, + respond: Respond | None = None, ) -> None: - """Post a visible thread reply telling the user to add their GitHub username.""" + """Post a visible thread reply telling the user to add their GitHub username. + + Falls back to an ephemeral *respond()* if the channel reply fails. + """ text = ( f"<@{target_user_id}> — please add your GitHub username to your Slack profile!\n" "Go to your profile → *Edit profile* → fill in the *GitHub* field.\n" "\n\n" "Once done, a core-team member can re-run: " - "`/nf-core github add-member `" + "`/nf-core github add `" ) - await _thread_reply(client, channel_id, thread_ts, text) + try: + await _thread_reply(client, channel_id, thread_ts, text) + except Exception: + logger.exception("Channel reply failed in _warn_missing_github") + if respond: + await respond(text, response_type="ephemeral") async def _thread_reply( diff --git a/src/nf_core_bot/commands/github/add_member_shortcut.py b/src/nf_core_bot/commands/github/add_member_shortcut.py index de447ed..c8f8c5b 100644 --- a/src/nf_core_bot/commands/github/add_member_shortcut.py +++ b/src/nf_core_bot/commands/github/add_member_shortcut.py @@ -102,9 +102,13 @@ async def handle_add_member_shortcut( "Go to your profile → *Edit profile* → fill in the *GitHub* field.\n" "\n\n" "Once done, a core-team member can try this action again, or use: " - "`/nf-core github add-member `" + "`/nf-core github add `" ) - await client.chat_postMessage(channel=channel_id, thread_ts=thread_ts, text=text) + try: + await client.chat_postMessage(channel=channel_id, thread_ts=thread_ts, text=text) + except Exception: + logger.exception("Channel reply failed when warning about missing GitHub username") + await client.chat_postEphemeral(channel=channel_id, user=caller_id, text=text) return else: # Workflow/bot message — try to extract GitHub handle from the text @@ -116,7 +120,7 @@ async def handle_add_member_shortcut( user=caller_id, text=( "Couldn't find a GitHub username in this message.\n" - "Use `/nf-core github add-member ` instead." + "Use `/nf-core github add ` instead." ), ) return @@ -131,4 +135,4 @@ async def handle_add_member_shortcut( async def _reply(text: str) -> None: await client.chat_postMessage(channel=channel_id, thread_ts=thread_ts, text=text) - await invite_and_greet(github_username, caller_id, _reply, greeting_user_id=greeting_user_id) + await invite_and_greet(github_username, caller_id, _reply, greeting_user_id=greeting_user_id, client=client) diff --git a/src/nf_core_bot/commands/github/invite_flow.py b/src/nf_core_bot/commands/github/invite_flow.py index b11fe33..088c0ff 100644 --- a/src/nf_core_bot/commands/github/invite_flow.py +++ b/src/nf_core_bot/commands/github/invite_flow.py @@ -10,53 +10,106 @@ if TYPE_CHECKING: from collections.abc import Awaitable, Callable + from slack_sdk.web.async_client import AsyncWebClient + logger = logging.getLogger(__name__) +async def _safe_reply(reply: Callable[[str], Awaitable[None]], text: str) -> bool: + """Call *reply* and return ``True``. If *reply* raises, log and return ``False``.""" + try: + await reply(text) + return True + except Exception: + logger.exception("Channel reply failed") + return False + + +async def _dm_caller(client: AsyncWebClient, caller_user_id: str, text: str) -> None: + """Send a DM to the person who triggered the command (best-effort).""" + try: + resp = await client.conversations_open(users=[caller_user_id]) + dm_channel = resp["channel"]["id"] + await client.chat_postMessage(channel=dm_channel, text=text) + except Exception: + logger.exception("Failed to DM caller %s", caller_user_id) + + +async def _reply_or_dm( + reply: Callable[[str], Awaitable[None]], + client: AsyncWebClient, + caller_user_id: str, + text: str, +) -> None: + """Try the channel *reply*; fall back to a DM if it fails.""" + if not await _safe_reply(reply, text): + await _dm_caller(client, caller_user_id, text) + + async def invite_and_greet( github_username: str, caller_user_id: str, reply: Callable[[str], Awaitable[None]], greeting_user_id: str | None = None, + *, + client: AsyncWebClient, ) -> bool: """Invite *github_username* to the nf-core org and contributors team. *reply* is called with message text for errors and the final greeting. + If *reply* raises (e.g. ``channel_not_found``), the error is logged and + the caller is notified via DM as a fallback. + Returns ``True`` on success. """ + # ── 1. Org invite ──────────────────────────────────────────────── try: org_result = await invite_to_org(github_username) except Exception: logger.exception("Network error inviting %s to org", github_username) - await reply(f"Failed to reach the GitHub API while inviting `{github_username}`. Please try again later.") + msg = f"Failed to reach the GitHub API while inviting `{github_username}`. Please try again later." + await _reply_or_dm(reply, client, caller_user_id, msg) return False if not org_result.ok: - await reply(f"Failed to invite `{github_username}` to the nf-core GitHub org:\n>{org_result.message}") + msg = f"Failed to invite `{github_username}` to the nf-core GitHub org:\n>{org_result.message}" + await _reply_or_dm(reply, client, caller_user_id, msg) return False + # ── 2. Team add ────────────────────────────────────────────────── try: team_result = await add_to_team(github_username) except Exception: logger.exception("Network error adding %s to team", github_username) - await reply( + msg = ( f"Invited `{github_username}` to the org, but failed to reach the GitHub API " "when adding to the contributors team. Please try again later." ) + await _reply_or_dm(reply, client, caller_user_id, msg) return False if not team_result.ok: - await reply( + msg = ( f"Invited `{github_username}` to the org, but failed to add to the " f"contributors team:\n>{team_result.message}" ) + await _reply_or_dm(reply, client, caller_user_id, msg) return False + # ── 3. Success greeting ────────────────────────────────────────── greeting = f"Hi <@{greeting_user_id}>, " if greeting_user_id else f"Hi `{github_username}`, " - await reply( + msg = ( f"{greeting}<@{caller_user_id}> has just added you to the nf-core GitHub organisation, " "welcome! :tada:\n\n" "You should have received an invite — you can either check your e-mail " "or click on this link to accept: https://github.com/orgs/nf-core/invitation" ) + await _safe_reply(reply, msg) + + # Always DM the caller as a failsafe confirmation + dm_text = ( + f"Done! `{github_username}` has been invited to the nf-core GitHub org and added to the contributors team." + ) + await _dm_caller(client, caller_user_id, dm_text) + return True diff --git a/src/nf_core_bot/commands/help.py b/src/nf_core_bot/commands/help.py index b5058fe..c9ebee6 100644 --- a/src/nf_core_bot/commands/help.py +++ b/src/nf_core_bot/commands/help.py @@ -30,8 +30,8 @@ ] GITHUB_COMMANDS: list[tuple[str, str, str]] = [ - ("github add-member @user", "Invite a Slack user to nf-core GitHub org", "admin"), - ("github add-member ", "Invite a GitHub user to nf-core GitHub org", "admin"), + ("github add @user", "Invite a Slack user to nf-core GitHub org", "admin"), + ("github add ", "Invite a GitHub user to nf-core GitHub org", "admin"), ] ONCALL_COMMANDS: list[tuple[str, str, str]] = [ diff --git a/src/nf_core_bot/commands/router.py b/src/nf_core_bot/commands/router.py index 109189b..ab1ec29 100644 --- a/src/nf_core_bot/commands/router.py +++ b/src/nf_core_bot/commands/router.py @@ -209,7 +209,7 @@ async def _route_admin( # ── GitHub dispatch ────────────────────────────────────────────────── _GITHUB_DISPATCH: dict[str, object] = { - "add-member": handle_add_member, + "add": handle_add_member, } diff --git a/tests/helpers.py b/tests/helpers.py new file mode 100644 index 0000000..ba01c5f --- /dev/null +++ b/tests/helpers.py @@ -0,0 +1,27 @@ +"""Reusable test helpers for Slack client mocking.""" + +from __future__ import annotations + +from unittest.mock import AsyncMock + + +def make_slack_client() -> AsyncMock: + """Build an ``AsyncWebClient`` mock whose ``conversations_open`` returns a DM channel.""" + client = AsyncMock() + client.conversations_open.return_value = {"channel": {"id": "D_DM"}} + return client + + +def channel_messages(client: AsyncMock, channel: str = "C_CHAN") -> list[str]: + """Return all ``chat_postMessage`` texts sent to *channel*.""" + texts: list[str] = [] + for call in client.chat_postMessage.call_args_list: + ch = call.kwargs.get("channel", call.args[0] if call.args else "") + if ch == channel: + texts.append(call.kwargs.get("text", "")) + return texts + + +def dm_messages(client: AsyncMock) -> list[str]: + """Return all ``chat_postMessage`` texts sent to the DM channel.""" + return channel_messages(client, "D_DM") diff --git a/tests/test_add_member.py b/tests/test_add_member.py index cca9a64..2eca63b 100644 --- a/tests/test_add_member.py +++ b/tests/test_add_member.py @@ -6,6 +6,7 @@ from nf_core_bot.checks.github import GitHubResult from nf_core_bot.commands.github.add_member import handle_add_member +from tests.helpers import channel_messages, dm_messages, make_slack_client def _command(channel: str = "C_CHAN", thread_ts: str = "") -> dict[str, str]: @@ -24,7 +25,7 @@ class TestPermissionGate: async def test_non_core_team_denied(self, _mock_perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_USER", _command(), ["octocat"]) @@ -46,26 +47,28 @@ async def test_valid_username_invites(self, mock_team: AsyncMock, mock_org: Asyn ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) mock_org.assert_awaited_once_with("octocat") mock_team.assert_awaited_once_with("octocat") - # Final thread reply should mention success - client.chat_postMessage.assert_awaited() - final_text = client.chat_postMessage.call_args.kwargs.get( - "text", client.chat_postMessage.call_args[1].get("text", "") - ) - assert "welcome" in final_text.lower() - assert "nf-core/invitation" in final_text - assert "<@U_ADMIN>" in final_text # mentions who triggered it + + # Channel reply should contain the welcome greeting + chan_texts = channel_messages(client) + assert any("welcome" in t.lower() for t in chan_texts) + assert any("nf-core/invitation" in t for t in chan_texts) + assert any("<@U_ADMIN>" in t for t in chan_texts) + + # Caller should also get a DM confirmation + dm_texts = dm_messages(client) + assert any("octocat" in t for t in dm_texts) @patch("nf_core_bot.commands.github.add_member.is_core_team", return_value=True) async def test_invalid_username_rejected(self, _perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["I don't know"]) @@ -80,7 +83,7 @@ async def test_url_as_username_normalised(self, _perm: AsyncMock) -> None: """Passing a full GitHub URL should extract and validate the username.""" ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() with ( patch("nf_core_bot.commands.github.invite_flow.invite_to_org") as mock_org, @@ -110,33 +113,35 @@ async def test_mention_resolves_github_username( ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["<@U01234TARGET>"]) mock_ghuser.assert_awaited_once_with(client, "U01234TARGET") mock_org.assert_awaited_once_with("octocat") - # Success message should greet the target user by Slack mention - last_call = client.chat_postMessage.call_args_list[-1] - text = last_call.kwargs["text"] - assert "<@U01234TARGET>" in text # greeting addresses the target - assert "<@U_ADMIN>" in text # mentions who triggered it + + # Channel reply should greet the target user + chan_texts = channel_messages(client) + assert any("<@U01234TARGET>" in t for t in chan_texts) + assert any("<@U_ADMIN>" in t for t in chan_texts) + + # DM confirmation should be sent + dm_texts = dm_messages(client) + assert any("octocat" in t for t in dm_texts) @patch("nf_core_bot.commands.github.add_member.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.add_member.get_github_username", return_value=None) async def test_mention_missing_github_warns(self, _mock_ghuser: AsyncMock, _perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(thread_ts="123"), ["<@U01234TARGET>"]) # Should post a warning about missing GitHub username client.chat_postMessage.assert_awaited() - text = client.chat_postMessage.call_args.kwargs.get( - "text", client.chat_postMessage.call_args[1].get("text", "") - ) - assert "GitHub username" in text + chan_texts = channel_messages(client) + assert any("GitHub username" in t for t in chan_texts) # ── No-argument usage ──────────────────────────────────────────────── @@ -147,7 +152,7 @@ class TestNoArgs: async def test_no_args_shows_usage(self, _perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), []) @@ -166,15 +171,12 @@ async def test_org_invite_network_error(self, mock_org: AsyncMock, _perm: AsyncM ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - client.chat_postMessage.assert_awaited() - text = client.chat_postMessage.call_args.kwargs.get( - "text", client.chat_postMessage.call_args[1].get("text", "") - ) - assert "Failed to reach the GitHub API" in text + chan_texts = channel_messages(client) + assert any("Failed to reach the GitHub API" in t for t in chan_texts) @patch("nf_core_bot.commands.github.add_member.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.invite_flow.invite_to_org") @@ -183,15 +185,12 @@ async def test_org_invite_api_failure(self, mock_org: AsyncMock, _perm: AsyncMoc ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - client.chat_postMessage.assert_awaited() - text = client.chat_postMessage.call_args.kwargs.get( - "text", client.chat_postMessage.call_args[1].get("text", "") - ) - assert "Failed to invite" in text + chan_texts = channel_messages(client) + assert any("Failed to invite" in t for t in chan_texts) @patch("nf_core_bot.commands.github.add_member.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.invite_flow.invite_to_org") @@ -202,15 +201,12 @@ async def test_team_add_network_error(self, mock_team: AsyncMock, mock_org: Asyn ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - client.chat_postMessage.assert_awaited() - text = client.chat_postMessage.call_args.kwargs.get( - "text", client.chat_postMessage.call_args[1].get("text", "") - ) - assert "failed to reach the GitHub API" in text + chan_texts = channel_messages(client) + assert any("failed to reach the GitHub API" in t for t in chan_texts) @patch("nf_core_bot.commands.github.add_member.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.invite_flow.invite_to_org") @@ -221,12 +217,64 @@ async def test_team_add_api_failure(self, mock_team: AsyncMock, mock_org: AsyncM ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - client.chat_postMessage.assert_awaited() - text = client.chat_postMessage.call_args.kwargs.get( - "text", client.chat_postMessage.call_args[1].get("text", "") - ) - assert "failed to add to the" in text + chan_texts = channel_messages(client) + assert any("failed to add to the" in t for t in chan_texts) + + +# ── DM failsafe ────────────────────────────────────────────────────── + + +class TestDmFailsafe: + @patch("nf_core_bot.commands.github.add_member.is_core_team", return_value=True) + @patch("nf_core_bot.commands.github.invite_flow.invite_to_org") + @patch("nf_core_bot.commands.github.invite_flow.add_to_team") + async def test_dm_sent_on_success(self, mock_team: AsyncMock, mock_org: AsyncMock, _perm: AsyncMock) -> None: + """Caller always receives a DM confirmation on success.""" + mock_org.return_value = GitHubResult(ok=True, message="ok") + mock_team.return_value = GitHubResult(ok=True, message="ok") + + ack = AsyncMock() + respond = AsyncMock() + client = make_slack_client() + + await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) + + client.conversations_open.assert_awaited() + dm_texts = dm_messages(client) + assert any("octocat" in t and "invited" in t for t in dm_texts) + + @patch("nf_core_bot.commands.github.add_member.is_core_team", return_value=True) + @patch("nf_core_bot.commands.github.invite_flow.invite_to_org") + @patch("nf_core_bot.commands.github.invite_flow.add_to_team") + async def test_dm_fallback_when_channel_reply_fails( + self, mock_team: AsyncMock, mock_org: AsyncMock, _perm: AsyncMock + ) -> None: + """When channel reply fails, caller gets the message via DM instead.""" + mock_org.return_value = GitHubResult(ok=True, message="ok") + mock_team.return_value = GitHubResult(ok=True, message="ok") + + ack = AsyncMock() + respond = AsyncMock() + client = make_slack_client() + + # First chat_postMessage (channel reply) fails, subsequent ones (DMs) succeed + call_count = 0 + + async def _side_effect(**kwargs: str) -> dict: # type: ignore[type-arg] + nonlocal call_count + call_count += 1 + if call_count == 1: + raise RuntimeError("channel_not_found") + return {"ok": True} + + client.chat_postMessage.side_effect = _side_effect + + await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) + + # DM should still be sent + client.conversations_open.assert_awaited() + assert call_count >= 2 # channel reply failed + DM sent diff --git a/tests/test_add_member_shortcut.py b/tests/test_add_member_shortcut.py index 9f845cc..36beee6 100644 --- a/tests/test_add_member_shortcut.py +++ b/tests/test_add_member_shortcut.py @@ -10,6 +10,7 @@ _extract_requester_from_text, handle_add_member_shortcut, ) +from tests.helpers import channel_messages, dm_messages, make_slack_client def _shortcut( @@ -38,7 +39,7 @@ class TestPermissionGate: @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=False) async def test_non_core_team_denied(self, _perm: AsyncMock) -> None: ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) @@ -47,7 +48,8 @@ async def test_non_core_team_denied(self, _perm: AsyncMock) -> None: text = client.chat_postEphemeral.call_args.kwargs["text"] assert "restricted" in text.lower() # Should NOT proceed to GitHub calls - client.chat_postMessage.assert_not_awaited() + _chan = channel_messages(client) + assert not _chan # ── GitHub username resolution ─────────────────────────────────────── @@ -58,14 +60,13 @@ class TestGithubUsernameResolution: @patch("nf_core_bot.commands.github.add_member_shortcut.get_github_username", return_value=None) async def test_missing_github_warns(self, _ghuser: AsyncMock, _perm: AsyncMock) -> None: ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) - client.chat_postMessage.assert_awaited_once() - text = client.chat_postMessage.call_args.kwargs["text"] - assert "GitHub username" in text - assert "<@U_TARGET>" in text + chan_texts = channel_messages(client) + assert any("GitHub username" in t for t in chan_texts) + assert any("<@U_TARGET>" in t for t in chan_texts) @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.add_member_shortcut.get_github_username", return_value="octocat") @@ -78,19 +79,23 @@ async def test_success_invites_and_adds( mock_team.return_value = GitHubResult(ok=True, message="active") ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) mock_org.assert_awaited_once_with("octocat") mock_team.assert_awaited_once_with("octocat") - # Final message should be a welcome with invite link - last_call = client.chat_postMessage.call_args_list[-1] - text = last_call.kwargs["text"] - assert "welcome" in text.lower() - assert "nf-core/invitation" in text - assert "<@U_TARGET>" in text # greeting addresses the message author - assert "<@U_ADMIN>" in text # mentions who triggered it + + # Channel reply should be a welcome with invite link + chan_texts = channel_messages(client) + assert any("welcome" in t.lower() for t in chan_texts) + assert any("nf-core/invitation" in t for t in chan_texts) + assert any("<@U_TARGET>" in t for t in chan_texts) # greeting addresses the message author + assert any("<@U_ADMIN>" in t for t in chan_texts) # mentions who triggered it + + # Caller should also get a DM confirmation + dm_texts = dm_messages(client) + assert any("octocat" in t for t in dm_texts) # ── Thread reply placement ─────────────────────────────────────────── @@ -108,14 +113,16 @@ async def test_replies_in_thread_when_in_thread( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() sc = _shortcut(message_ts="222.333", thread_ts="111.000") await handle_add_member_shortcut(ack, sc, client) - # Should reply in the parent thread, not the message itself - last_call = client.chat_postMessage.call_args_list[-1] - assert last_call.kwargs["thread_ts"] == "111.000" + # The greeting (channel reply) should be in the parent thread, not the message itself + chan_calls = [c for c in client.chat_postMessage.call_args_list if c.kwargs.get("channel") == "C_CHAN"] + greeting_call = [c for c in chan_calls if "welcome" in c.kwargs.get("text", "").lower()] + assert greeting_call + assert greeting_call[0].kwargs["thread_ts"] == "111.000" @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.add_member_shortcut.get_github_username", return_value="octocat") @@ -128,14 +135,16 @@ async def test_replies_to_message_when_not_in_thread( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() sc = _shortcut(message_ts="222.333") await handle_add_member_shortcut(ack, sc, client) - # Should reply to the message itself (creating a thread) - last_call = client.chat_postMessage.call_args_list[-1] - assert last_call.kwargs["thread_ts"] == "222.333" + # The greeting should reply to the message itself (creating a thread) + chan_calls = [c for c in client.chat_postMessage.call_args_list if c.kwargs.get("channel") == "C_CHAN"] + greeting_call = [c for c in chan_calls if "welcome" in c.kwargs.get("text", "").lower()] + assert greeting_call + assert greeting_call[0].kwargs["thread_ts"] == "222.333" # ── Workflow / bot message handling ─────────────────────────────────── @@ -223,24 +232,28 @@ async def test_workflow_message_extracts_handle( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() sc = _workflow_shortcut("By <@U0REQUESTER> at March 13th, 2026\n*Which is your GitHub handle?*\nMuteebaAzhar") await handle_add_member_shortcut(ack, sc, client) mock_org.assert_awaited_once_with("MuteebaAzhar") mock_team.assert_awaited_once_with("MuteebaAzhar") - # Welcome message should mention the requester and the caller - last_call = client.chat_postMessage.call_args_list[-1] - text = last_call.kwargs["text"] - assert "<@U0REQUESTER>" in text # greeting addresses workflow requester - assert "<@U_ADMIN>" in text # mentions who triggered it - assert "nf-core/invitation" in text + + # Channel reply should mention the requester and the caller + chan_texts = channel_messages(client) + assert any("<@U0REQUESTER>" in t for t in chan_texts) # greeting addresses workflow requester + assert any("<@U_ADMIN>" in t for t in chan_texts) # mentions who triggered it + assert any("nf-core/invitation" in t for t in chan_texts) + + # DM confirmation should be sent + dm_texts = dm_messages(client) + assert any("MuteebaAzhar" in t for t in dm_texts) @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=True) async def test_workflow_message_no_handle_shows_error(self, _perm: AsyncMock) -> None: ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() sc = _workflow_shortcut("Just a random bot message") await handle_add_member_shortcut(ack, sc, client) @@ -261,12 +274,15 @@ async def test_org_invite_network_error(self, mock_org: AsyncMock, _ghuser: Asyn mock_org.side_effect = RuntimeError("connection refused") ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) - last_msg = client.chat_postMessage.call_args_list[-1] - assert "Failed to reach the GitHub API" in last_msg.kwargs["text"] + chan_texts = channel_messages(client) + # Channel reply may or may not work — check DM too + dm_texts = dm_messages(client) + all_texts = chan_texts + dm_texts + assert any("Failed to reach the GitHub API" in t for t in all_texts) @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.add_member_shortcut.get_github_username", return_value="octocat") @@ -275,12 +291,14 @@ async def test_org_invite_api_failure(self, mock_org: AsyncMock, _ghuser: AsyncM mock_org.return_value = GitHubResult(ok=False, message="422 — Validation failed") ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) - last_msg = client.chat_postMessage.call_args_list[-1] - assert "Failed to invite" in last_msg.kwargs["text"] + chan_texts = channel_messages(client) + dm_texts = dm_messages(client) + all_texts = chan_texts + dm_texts + assert any("Failed to invite" in t for t in all_texts) @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=True) @patch("nf_core_bot.commands.github.add_member_shortcut.get_github_username", return_value="octocat") @@ -293,9 +311,68 @@ async def test_team_add_network_error( mock_team.side_effect = RuntimeError("timeout") ack = AsyncMock() - client = AsyncMock() + client = make_slack_client() + + await handle_add_member_shortcut(ack, _shortcut(), client) + + chan_texts = channel_messages(client) + dm_texts = dm_messages(client) + all_texts = chan_texts + dm_texts + assert any("failed to reach the GitHub API" in t for t in all_texts) + + +# ── DM failsafe ────────────────────────────────────────────────────── + + +class TestDmFailsafe: + @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=True) + @patch("nf_core_bot.commands.github.add_member_shortcut.get_github_username", return_value="octocat") + @patch("nf_core_bot.commands.github.invite_flow.invite_to_org") + @patch("nf_core_bot.commands.github.invite_flow.add_to_team") + async def test_dm_sent_on_success( + self, mock_team: AsyncMock, mock_org: AsyncMock, _ghuser: AsyncMock, _perm: AsyncMock + ) -> None: + """Caller always receives a DM confirmation on success.""" + mock_org.return_value = GitHubResult(ok=True, message="ok") + mock_team.return_value = GitHubResult(ok=True, message="ok") + + ack = AsyncMock() + client = make_slack_client() + + await handle_add_member_shortcut(ack, _shortcut(), client) + + client.conversations_open.assert_awaited() + dm_texts = dm_messages(client) + assert any("octocat" in t and "invited" in t for t in dm_texts) + + @patch("nf_core_bot.commands.github.add_member_shortcut.is_core_team", return_value=True) + @patch("nf_core_bot.commands.github.add_member_shortcut.get_github_username", return_value="octocat") + @patch("nf_core_bot.commands.github.invite_flow.invite_to_org") + @patch("nf_core_bot.commands.github.invite_flow.add_to_team") + async def test_channel_reply_failure_falls_back_to_dm( + self, mock_team: AsyncMock, mock_org: AsyncMock, _ghuser: AsyncMock, _perm: AsyncMock + ) -> None: + """When channel reply fails, caller still gets the DM.""" + mock_org.return_value = GitHubResult(ok=True, message="ok") + mock_team.return_value = GitHubResult(ok=True, message="ok") + + ack = AsyncMock() + client = make_slack_client() + + # First chat_postMessage (channel reply) fails, subsequent ones (DMs) succeed + call_count = 0 + + async def _side_effect(**kwargs: str) -> dict: # type: ignore[type-arg] + nonlocal call_count + call_count += 1 + if call_count == 1: + raise RuntimeError("channel_not_found") + return {"ok": True} + + client.chat_postMessage.side_effect = _side_effect await handle_add_member_shortcut(ack, _shortcut(), client) - last_msg = client.chat_postMessage.call_args_list[-1] - assert "failed to reach the GitHub API" in last_msg.kwargs["text"] + # DM should still be sent + client.conversations_open.assert_awaited() + assert call_count >= 2 diff --git a/tests/test_router_integration.py b/tests/test_router_integration.py index c933f20..8e8ea7e 100644 --- a/tests/test_router_integration.py +++ b/tests/test_router_integration.py @@ -180,8 +180,8 @@ async def test_export_receives_ack_respond_client_body_rest(self, monkeypatch): class TestGithubDispatchTypes: async def test_add_member_receives_correct_args(self, monkeypatch): mock = AsyncMock() - monkeypatch.setitem(router_mod._GITHUB_DISPATCH, "add-member", mock) - await dispatch(AsyncMock(), AsyncMock(), AsyncMock(), _command("github add-member octocat")) + monkeypatch.setitem(router_mod._GITHUB_DISPATCH, "add", mock) + await dispatch(AsyncMock(), AsyncMock(), AsyncMock(), _command("github add octocat")) mock.assert_awaited_once() args = mock.call_args[0] assert len(args) == 6 # ack, respond, client, user_id, command, rest