From cc881b3646066649a77fb94bcf7b3cbcf3bf423d Mon Sep 17 00:00:00 2001 From: Phil Ewels Date: Thu, 19 Mar 2026 17:55:53 +0100 Subject: [PATCH 1/4] Fix github add: catch channel_not_found, always DM caller, rename to 'add' - Wrap channel reply (chat_postMessage) in try/except so channel_not_found no longer silently swallows results - Always DM the caller as a failsafe confirmation after invite - Fall back to ephemeral response when channel reply fails - Rename slash command from 'add-member' to 'add' - Update all docs, help text, and tests --- CLAUDE.md | 2 +- README.md | 14 +- docs/commands.md | 10 +- docs/slack-app-setup.md | 2 +- src/nf_core_bot/commands/github/add_member.py | 29 ++- .../commands/github/add_member_shortcut.py | 12 +- .../commands/github/invite_flow.py | 57 +++++- src/nf_core_bot/commands/help.py | 4 +- src/nf_core_bot/commands/router.py | 2 +- tests/test_add_member.py | 165 +++++++++++----- tests/test_add_member_shortcut.py | 178 ++++++++++++++---- tests/test_router_integration.py | 4 +- 12 files changed, 353 insertions(+), 126 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 22da103..5fb4960 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,7 +111,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..5056289 100644 --- a/src/nf_core_bot/commands/github/invite_flow.py +++ b/src/nf_core_bot/commands/github/invite_flow.py @@ -10,53 +10,100 @@ 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 invite_and_greet( github_username: str, caller_user_id: str, reply: Callable[[str], Awaitable[None]], greeting_user_id: str | None = None, + *, + client: AsyncWebClient | None = None, ) -> 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." + if not await _safe_reply(reply, msg) and client: + await _dm_caller(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}" + if not await _safe_reply(reply, msg) and client: + await _dm_caller(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." ) + if not await _safe_reply(reply, msg) and client: + await _dm_caller(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}" ) + if not await _safe_reply(reply, msg) and client: + await _dm_caller(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 + if client: + 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/test_add_member.py b/tests/test_add_member.py index cca9a64..320d3e5 100644 --- a/tests/test_add_member.py +++ b/tests/test_add_member.py @@ -16,6 +16,28 @@ def _command(channel: str = "C_CHAN", thread_ts: str = "") -> dict[str, str]: return cmd +def _make_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* (excludes DMs).""" + 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") + + # ── Permission check ───────────────────────────────────────────────── @@ -24,7 +46,7 @@ class TestPermissionGate: async def test_non_core_team_denied(self, _mock_perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_client() await handle_add_member(ack, respond, client, "U_USER", _command(), ["octocat"]) @@ -46,26 +68,28 @@ async def test_valid_username_invites(self, mock_team: AsyncMock, mock_org: Asyn ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_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_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["I don't know"]) @@ -80,7 +104,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_client() with ( patch("nf_core_bot.commands.github.invite_flow.invite_to_org") as mock_org, @@ -110,33 +134,35 @@ async def test_mention_resolves_github_username( ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_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_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 +173,7 @@ class TestNoArgs: async def test_no_args_shows_usage(self, _perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), []) @@ -166,15 +192,12 @@ async def test_org_invite_network_error(self, mock_org: AsyncMock, _perm: AsyncM ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_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 +206,12 @@ async def test_org_invite_api_failure(self, mock_org: AsyncMock, _perm: AsyncMoc ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_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 +222,12 @@ async def test_team_add_network_error(self, mock_team: AsyncMock, mock_org: Asyn ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_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 +238,64 @@ async def test_team_add_api_failure(self, mock_team: AsyncMock, mock_org: AsyncM ack = AsyncMock() respond = AsyncMock() - client = AsyncMock() + client = _make_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_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_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..6c192b2 100644 --- a/tests/test_add_member_shortcut.py +++ b/tests/test_add_member_shortcut.py @@ -31,6 +31,28 @@ def _shortcut( } +def _make_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") + + # ── Permission check ───────────────────────────────────────────────── @@ -38,7 +60,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_client() await handle_add_member_shortcut(ack, _shortcut(), client) @@ -47,7 +69,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 +81,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_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 +100,23 @@ async def test_success_invites_and_adds( mock_team.return_value = GitHubResult(ok=True, message="active") ack = AsyncMock() - client = AsyncMock() + client = _make_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 +134,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_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 +156,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_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 +253,28 @@ async def test_workflow_message_extracts_handle( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = AsyncMock() + client = _make_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_client() sc = _workflow_shortcut("Just a random bot message") await handle_add_member_shortcut(ack, sc, client) @@ -261,12 +295,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_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 +312,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_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 +332,68 @@ async def test_team_add_network_error( mock_team.side_effect = RuntimeError("timeout") ack = AsyncMock() - client = AsyncMock() + client = _make_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_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_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 From 931a59ce1c70d0f49b0d476f511602f4caed2884 Mon Sep 17 00:00:00 2001 From: Phil Ewels Date: Thu, 19 Mar 2026 18:21:26 +0100 Subject: [PATCH 2/4] Simplify invite_flow: extract _reply_or_dm helper, deduplicate test utilities - Extract repeated `if not _safe_reply ... _dm_caller` pattern into `_reply_or_dm()` helper (was duplicated 4 times in invite_and_greet) - Make `client` a required parameter in `invite_and_greet()` since all callers always provide it - Move shared test helpers (`make_slack_client`, `channel_messages`, `dm_messages`) from both test files into `tests/helpers.py` Co-Authored-By: Claude Opus 4.6 (1M context) --- .../commands/github/invite_flow.py | 34 +++++---- tests/helpers.py | 27 +++++++ tests/test_add_member.py | 69 +++++++----------- tests/test_add_member_shortcut.py | 72 +++++++------------ 4 files changed, 97 insertions(+), 105 deletions(-) create mode 100644 tests/helpers.py diff --git a/src/nf_core_bot/commands/github/invite_flow.py b/src/nf_core_bot/commands/github/invite_flow.py index 5056289..088c0ff 100644 --- a/src/nf_core_bot/commands/github/invite_flow.py +++ b/src/nf_core_bot/commands/github/invite_flow.py @@ -35,13 +35,24 @@ async def _dm_caller(client: AsyncWebClient, caller_user_id: str, text: str) -> 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 | None = None, + client: AsyncWebClient, ) -> bool: """Invite *github_username* to the nf-core org and contributors team. @@ -57,14 +68,12 @@ async def invite_and_greet( except Exception: logger.exception("Network error inviting %s to org", github_username) msg = f"Failed to reach the GitHub API while inviting `{github_username}`. Please try again later." - if not await _safe_reply(reply, msg) and client: - await _dm_caller(client, caller_user_id, msg) + await _reply_or_dm(reply, client, caller_user_id, msg) return False if not org_result.ok: msg = f"Failed to invite `{github_username}` to the nf-core GitHub org:\n>{org_result.message}" - if not await _safe_reply(reply, msg) and client: - await _dm_caller(client, caller_user_id, msg) + await _reply_or_dm(reply, client, caller_user_id, msg) return False # ── 2. Team add ────────────────────────────────────────────────── @@ -76,8 +85,7 @@ async def invite_and_greet( f"Invited `{github_username}` to the org, but failed to reach the GitHub API " "when adding to the contributors team. Please try again later." ) - if not await _safe_reply(reply, msg) and client: - await _dm_caller(client, caller_user_id, msg) + await _reply_or_dm(reply, client, caller_user_id, msg) return False if not team_result.ok: @@ -85,8 +93,7 @@ async def invite_and_greet( f"Invited `{github_username}` to the org, but failed to add to the " f"contributors team:\n>{team_result.message}" ) - if not await _safe_reply(reply, msg) and client: - await _dm_caller(client, caller_user_id, msg) + await _reply_or_dm(reply, client, caller_user_id, msg) return False # ── 3. Success greeting ────────────────────────────────────────── @@ -100,10 +107,9 @@ async def invite_and_greet( await _safe_reply(reply, msg) # Always DM the caller as a failsafe confirmation - if client: - 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) + 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/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 320d3e5..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]: @@ -16,28 +17,6 @@ def _command(channel: str = "C_CHAN", thread_ts: str = "") -> dict[str, str]: return cmd -def _make_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* (excludes DMs).""" - 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") - - # ── Permission check ───────────────────────────────────────────────── @@ -46,7 +25,7 @@ class TestPermissionGate: async def test_non_core_team_denied(self, _mock_perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_USER", _command(), ["octocat"]) @@ -68,7 +47,7 @@ async def test_valid_username_invites(self, mock_team: AsyncMock, mock_org: Asyn ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) @@ -76,20 +55,20 @@ async def test_valid_username_invites(self, mock_team: AsyncMock, mock_org: Asyn mock_team.assert_awaited_once_with("octocat") # Channel reply should contain the welcome greeting - chan_texts = _channel_messages(client) + 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) + 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 = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["I don't know"]) @@ -104,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 = _make_client() + client = make_slack_client() with ( patch("nf_core_bot.commands.github.invite_flow.invite_to_org") as mock_org, @@ -134,7 +113,7 @@ async def test_mention_resolves_github_username( ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["<@U01234TARGET>"]) @@ -142,12 +121,12 @@ async def test_mention_resolves_github_username( mock_org.assert_awaited_once_with("octocat") # Channel reply should greet the target user - chan_texts = _channel_messages(client) + 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) + 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) @@ -155,13 +134,13 @@ async def test_mention_resolves_github_username( async def test_mention_missing_github_warns(self, _mock_ghuser: AsyncMock, _perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = _make_client() + 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() - chan_texts = _channel_messages(client) + chan_texts = channel_messages(client) assert any("GitHub username" in t for t in chan_texts) @@ -173,7 +152,7 @@ class TestNoArgs: async def test_no_args_shows_usage(self, _perm: AsyncMock) -> None: ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), []) @@ -192,11 +171,11 @@ async def test_org_invite_network_error(self, mock_org: AsyncMock, _perm: AsyncM ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - chan_texts = _channel_messages(client) + 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) @@ -206,11 +185,11 @@ async def test_org_invite_api_failure(self, mock_org: AsyncMock, _perm: AsyncMoc ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - chan_texts = _channel_messages(client) + 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) @@ -222,11 +201,11 @@ async def test_team_add_network_error(self, mock_team: AsyncMock, mock_org: Asyn ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - chan_texts = _channel_messages(client) + 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) @@ -238,11 +217,11 @@ async def test_team_add_api_failure(self, mock_team: AsyncMock, mock_org: AsyncM ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member(ack, respond, client, "U_ADMIN", _command(), ["octocat"]) - chan_texts = _channel_messages(client) + chan_texts = channel_messages(client) assert any("failed to add to the" in t for t in chan_texts) @@ -260,12 +239,12 @@ async def test_dm_sent_on_success(self, mock_team: AsyncMock, mock_org: AsyncMoc ack = AsyncMock() respond = AsyncMock() - client = _make_client() + 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) + 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) @@ -280,7 +259,7 @@ async def test_dm_fallback_when_channel_reply_fails( ack = AsyncMock() respond = AsyncMock() - client = _make_client() + client = make_slack_client() # First chat_postMessage (channel reply) fails, subsequent ones (DMs) succeed call_count = 0 diff --git a/tests/test_add_member_shortcut.py b/tests/test_add_member_shortcut.py index 6c192b2..221ad64 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( @@ -31,27 +32,6 @@ def _shortcut( } -def _make_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") - # ── Permission check ───────────────────────────────────────────────── @@ -60,7 +40,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 = _make_client() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) @@ -69,7 +49,7 @@ 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 - _chan = _channel_messages(client) + _chan = channel_messages(client) assert not _chan @@ -81,11 +61,11 @@ 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 = _make_client() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) - chan_texts = _channel_messages(client) + 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) @@ -100,7 +80,7 @@ async def test_success_invites_and_adds( mock_team.return_value = GitHubResult(ok=True, message="active") ack = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) @@ -108,14 +88,14 @@ async def test_success_invites_and_adds( mock_team.assert_awaited_once_with("octocat") # Channel reply should be a welcome with invite link - chan_texts = _channel_messages(client) + 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) + dm_texts = dm_messages(client) assert any("octocat" in t for t in dm_texts) @@ -134,7 +114,7 @@ async def test_replies_in_thread_when_in_thread( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = _make_client() + client = make_slack_client() sc = _shortcut(message_ts="222.333", thread_ts="111.000") await handle_add_member_shortcut(ack, sc, client) @@ -156,7 +136,7 @@ async def test_replies_to_message_when_not_in_thread( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = _make_client() + client = make_slack_client() sc = _shortcut(message_ts="222.333") await handle_add_member_shortcut(ack, sc, client) @@ -253,7 +233,7 @@ async def test_workflow_message_extracts_handle( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = _make_client() + 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) @@ -262,19 +242,19 @@ async def test_workflow_message_extracts_handle( mock_team.assert_awaited_once_with("MuteebaAzhar") # Channel reply should mention the requester and the caller - chan_texts = _channel_messages(client) + 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) + 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 = _make_client() + client = make_slack_client() sc = _workflow_shortcut("Just a random bot message") await handle_add_member_shortcut(ack, sc, client) @@ -295,13 +275,13 @@ async def test_org_invite_network_error(self, mock_org: AsyncMock, _ghuser: Asyn mock_org.side_effect = RuntimeError("connection refused") ack = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) - chan_texts = _channel_messages(client) + chan_texts = channel_messages(client) # Channel reply may or may not work — check DM too - dm_texts = _dm_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) @@ -312,12 +292,12 @@ 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 = _make_client() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) - chan_texts = _channel_messages(client) - dm_texts = _dm_messages(client) + 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) @@ -332,12 +312,12 @@ async def test_team_add_network_error( mock_team.side_effect = RuntimeError("timeout") ack = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) - chan_texts = _channel_messages(client) - dm_texts = _dm_messages(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) @@ -358,12 +338,12 @@ async def test_dm_sent_on_success( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = _make_client() + client = make_slack_client() await handle_add_member_shortcut(ack, _shortcut(), client) client.conversations_open.assert_awaited() - dm_texts = _dm_messages(client) + 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) @@ -378,7 +358,7 @@ async def test_channel_reply_failure_falls_back_to_dm( mock_team.return_value = GitHubResult(ok=True, message="ok") ack = AsyncMock() - client = _make_client() + client = make_slack_client() # First chat_postMessage (channel reply) fails, subsequent ones (DMs) succeed call_count = 0 From f20b864eeafc7d02893d4a1b0f0315d55c9ee5d1 Mon Sep 17 00:00:00 2001 From: Phil Ewels Date: Thu, 19 Mar 2026 18:23:22 +0100 Subject: [PATCH 3/4] Update CLAUDE.md: document invite flow error handling and DM failsafe Co-Authored-By: Claude Opus 4.6 (1M context) --- CLAUDE.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 5fb4960..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 From 4409d1e6a899b5f4121563f20030db7909c4144f Mon Sep 17 00:00:00 2001 From: Phil Ewels Date: Thu, 19 Mar 2026 18:25:27 +0100 Subject: [PATCH 4/4] Fix formatting in test_add_member_shortcut.py Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/test_add_member_shortcut.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_add_member_shortcut.py b/tests/test_add_member_shortcut.py index 221ad64..36beee6 100644 --- a/tests/test_add_member_shortcut.py +++ b/tests/test_add_member_shortcut.py @@ -32,7 +32,6 @@ def _shortcut( } - # ── Permission check ─────────────────────────────────────────────────