Skip to content

Fix discord role apply gating #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
michaelmwu merged 4 commits into from
Mar 9, 2026
Merged

Fix discord role apply gating #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
28f8cf8
fix: gate discord role apply on linked contact
michaelmwu Mar 9, 2026
91728db
Fix inferred discord role-apply state
michaelmwu Mar 9, 2026
2892fbd
Preserve discord role apply deny audit and gating
michaelmwu Mar 9, 2026
d462f40
Fix async context for view-disabled state test
michaelmwu Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 42 additions & 3 deletions apps/discord_bot/src/five08/discord_bot/cogs/crm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,7 +1023,9 @@ async def on_submit(self, interaction: discord.Interaction) -> None:
self.confirmation_view.discord_role_suggestions = normalized
for item in self.confirmation_view.children:
if isinstance(item, ResumeApplyDiscordRolesButton):
item.disabled = not bool(normalized)
item.disabled = not (
self.confirmation_view.can_apply_discord_roles and bool(normalized)
)

if normalized:
count = len(normalized)
Expand Down Expand Up @@ -1269,11 +1271,12 @@ async def callback(self, interaction: discord.Interaction) -> None:
class ResumeApplyDiscordRolesButton(discord.ui.Button["ResumeUpdateConfirmationView"]):
"""Button that applies suggested Discord roles to the linked member."""

def __init__(self) -> None:
def __init__(self, *, disabled: bool = False) -> None:
super().__init__(
label="Apply Discord Roles",
style=discord.ButtonStyle.success,
custom_id="resume_apply_discord_roles",
disabled=disabled,
)

async def callback(self, interaction: discord.Interaction) -> None:
Expand Down Expand Up @@ -1306,6 +1309,18 @@ def _audit_apply_roles_event(
resource_id=view.contact_id,
)

if not view.can_apply_discord_roles:
_audit_apply_roles_event(
"denied",
"missing_linked_user",
{"reason": "button_disabled_for_role_application"},
Comment on lines +1312 to +1316
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new early-return branch for unlinked contacts is important behavior (and was added specifically to guard a regression), but there’s no unit test covering the callback being triggered while view.can_apply_discord_roles is false (e.g., via a stale interaction/custom_id). Adding a test that asserts the ephemeral error message (and, if possible, that audit is called) would help prevent this from regressing again.

Copilot uses AI. Check for mistakes.
)
await interaction.response.send_message(
"❌ Discord roles can only be applied after linking this contact to a Discord user.",
ephemeral=True,
)
return

if not interaction.guild:
await interaction.response.send_message(
"❌ Discord roles can only be managed inside a server.",
Expand Down Expand Up @@ -1605,6 +1620,7 @@ def __init__(
parsed_seniority: str | None = None,
discord_role_suggestions: list[str] | None = None,
discord_role_target_user_id: str | None = None,
can_apply_discord_roles: bool = False,
) -> None:
super().__init__(timeout=300)
self.crm_cog = crm_cog
Expand All @@ -1615,6 +1631,11 @@ def __init__(
self.link_discord = link_discord
self.parsed_seniority = parsed_seniority
self.discord_role_target_user_id = discord_role_target_user_id
self.can_apply_discord_roles = bool(
can_apply_discord_roles
or discord_role_target_user_id
or (isinstance(link_discord, dict) and bool(link_discord.get("user_id")))
)
Comment on lines +1634 to +1638
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can_apply_discord_roles is computed with an or chain, so an explicitly provided False is overridden when discord_role_target_user_id or link_discord['user_id'] is present. If the intent is to preserve an explicit flag (True/False) and only infer when it’s not provided, consider making the parameter bool | None = None and computing self.can_apply_discord_roles = inferred if can_apply_discord_roles is None else can_apply_discord_roles.

Copilot uses AI. Check for mistakes.
self.discord_role_suggestions = list(
dict.fromkeys(discord_role_suggestions or [])
)
Expand All @@ -1637,7 +1658,9 @@ def __init__(
self.add_item(ResumeEditRolesButton())
if self.discord_role_suggestions:
self.add_item(ResumeEditDiscordRolesButton())
self.add_item(ResumeApplyDiscordRolesButton())
self.add_item(
ResumeApplyDiscordRolesButton(disabled=not self.can_apply_discord_roles)
)
Comment on lines +1661 to +1663
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change disables the Apply button based on self.can_apply_discord_roles, which will flip existing unit-test expectations (e.g., tests that currently assert the Apply button is enabled even when no Discord link/target user ID is set). Please update/add unit tests to cover both states: disabled when there is no linked/target Discord user, and enabled when discord_role_target_user_id (or a valid link_discord.user_id) is present.

Copilot uses AI. Check for mistakes.
self.add_item(ResumeEditLocationButton())

def _set_seniority_override(self, value: str) -> str:
Expand Down Expand Up @@ -3667,6 +3690,7 @@ def _build_role_suggestions_embed(
locality_roles: list[str] | None = None,
extracted_profile: dict[str, Any] | None = None,
current_discord_roles: list[str] | None = None,
can_apply_discord_roles: bool = False,
) -> discord.Embed | None:
"""Build a separate embed suggesting Discord roles to add based on resume data.

Expand All @@ -3689,6 +3713,16 @@ def _build_role_suggestions_embed(
description=f"Roles to **add** for **{contact_name}** based on resume — never remove existing roles.",
color=0x57F287,
)
if not can_apply_discord_roles:
embed.add_field(
name="🔒 Link required",
value=(
"This contact is not currently linked to a Discord user, so suggested roles "
"cannot be applied automatically. Link them first in CRM or use "
"`/link-discord-user`."
),
inline=False,
)
Comment on lines +3716 to +3725
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_build_role_suggestions_embed now conditionally adds the "🔒 Link required" field. There are existing unit tests covering this embed builder, but none appear to assert this new field behavior. Add/adjust tests to verify the field is present when can_apply_discord_roles is false and absent when it’s true, so the UX/validation contract doesn’t regress.

Copilot uses AI. Check for mistakes.

if technical:
embed.add_field(
Expand Down Expand Up @@ -3890,6 +3924,7 @@ async def _run_resume_extract_and_preview(
role_suggestions_embed: discord.Embed | None = None
suggested_discord_roles: list[str] = []
discord_role_target_user_id: str | None = None
can_apply_discord_roles = False
if action_name == "crm.reprocess_resume" or (
action_name == "crm.upload_resume" and link_member
):
Expand Down Expand Up @@ -3932,10 +3967,12 @@ async def _run_resume_extract_and_preview(
suggested_discord_roles = list(
dict.fromkeys(technical_suggestions + locality_suggestions)
)
can_apply_discord_roles = bool(discord_role_target_user_id or link_member)
role_suggestions_embed = self._build_role_suggestions_embed(
contact_name=contact_name,
technical_roles=technical_suggestions,
locality_roles=locality_suggestions,
can_apply_discord_roles=can_apply_discord_roles,
)

if not proposed_updates and not link_member and not parsed_seniority:
Expand Down Expand Up @@ -3970,6 +4007,7 @@ async def _run_resume_extract_and_preview(
parsed_seniority=parsed_seniority,
discord_role_suggestions=suggested_discord_roles,
discord_role_target_user_id=discord_role_target_user_id,
can_apply_discord_roles=can_apply_discord_roles,
)
if action_name != "crm.reprocess_resume":
self._audit_command(
Expand Down Expand Up @@ -4015,6 +4053,7 @@ async def _run_resume_extract_and_preview(
parsed_seniority=parsed_seniority,
discord_role_suggestions=suggested_discord_roles,
discord_role_target_user_id=discord_role_target_user_id,
can_apply_discord_roles=can_apply_discord_roles,
)
if action_name != "crm.reprocess_resume":
self._audit_command(
Expand Down
82 changes: 82 additions & 0 deletions tests/unit/test_crm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,27 @@ async def test_edit_discord_roles_modal_prepopulates_values(self, crm_cog):

assert modal.discord_roles_input.default == "Backend\nOperations"

@pytest.mark.asyncio
async def test_apply_discord_roles_button_disabled_without_linked_discord_user(
self, crm_cog
):
"""Apply button should be disabled if no Discord link target is available."""
view = ResumeUpdateConfirmationView(
crm_cog=crm_cog,
requester_id=123,
contact_id="contact-1",
contact_name="Test User",
proposed_updates={},
discord_role_suggestions=["Backend", "Operations"],
)

apply_button = next(
child
for child in view.children
if isinstance(child, ResumeApplyDiscordRolesButton)
)
assert apply_button.disabled is True

@pytest.mark.asyncio
async def test_edit_discord_roles_modal_submit_updates_suggested_roles(
self, crm_cog
Expand All @@ -769,6 +790,7 @@ async def test_edit_discord_roles_modal_submit_updates_suggested_roles(
contact_name="Test User",
proposed_updates={},
discord_role_suggestions=["Backend"],
discord_role_target_user_id="1001",
)
modal = ResumeEditDiscordRolesModal(confirmation_view=view)
modal.discord_roles_input._value = (
Expand All @@ -792,6 +814,66 @@ async def test_edit_discord_roles_modal_submit_updates_suggested_roles(
ephemeral=True,
)

@pytest.mark.asyncio
async def test_edit_discord_roles_modal_submit_keeps_apply_disabled_without_link(
self, crm_cog
):
"""Apply Discord Roles remains disabled until the contact is linked."""
view = ResumeUpdateConfirmationView(
crm_cog=crm_cog,
requester_id=123,
contact_id="contact-1",
contact_name="Test User",
proposed_updates={},
discord_role_suggestions=["Backend"],
)
modal = ResumeEditDiscordRolesModal(confirmation_view=view)
modal.discord_roles_input._value = "Backend\nOperations"
interaction = AsyncMock()
interaction.response = AsyncMock()
interaction.response.send_message = AsyncMock()

await modal.on_submit(interaction)

apply_button = next(
child
for child in view.children
if isinstance(child, ResumeApplyDiscordRolesButton)
)
assert apply_button.disabled is True
interaction.response.send_message.assert_called_once_with(
"✅ Discord roles updated to 2 roles.",
ephemeral=True,
)

def test_build_role_suggestions_embed_marks_link_required_when_not_linked(
self, crm_cog
):
"""Role suggestions embed should include a link required callout when role apply is blocked."""
embed = crm_cog._build_role_suggestions_embed(
contact_name="Test User",
technical_roles=["Backend"],
locality_roles=["APAC"],
can_apply_discord_roles=False,
)

assert embed is not None
assert any(field.name == "🔒 Link required" for field in embed.fields)

def test_build_role_suggestions_embed_hides_link_required_when_linked(
self, crm_cog
):
"""Role suggestions embed should hide link required callout when application is enabled."""
embed = crm_cog._build_role_suggestions_embed(
contact_name="Test User",
technical_roles=["Backend"],
locality_roles=["APAC"],
can_apply_discord_roles=True,
)

assert embed is not None
assert all(field.name != "🔒 Link required" for field in embed.fields)

@pytest.mark.asyncio
async def test_apply_discord_roles_button_applies_roles_and_reports_results(
self, crm_cog
Expand Down