Add Discord role edit/preview flow for CRM resumes#195
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces Discord role suggestion and application functionality to the CRM resume processing workflow. New UI components enable users to view, edit, and apply role suggestions extracted from resumes. Implements role protection, hierarchy validation, permission checks, and audit logging for role applications. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bot as Discord Bot
participant Modal as Edit Modal
participant CRM as CRM/Database
participant AuditLog as Audit Log
User->>Bot: Submit resume for processing
activate Bot
Bot->>CRM: Extract profile & suggest roles
CRM-->>Bot: Suggested roles (tech + locality)
deactivate Bot
Bot->>User: Display role suggestions in embed
User->>Bot: Click edit roles button
activate Modal
Bot->>Modal: Open modal with suggestions
User->>Modal: Edit role list
User->>Modal: Submit edited roles
Modal-->>Bot: Updated suggestions
deactivate Modal
User->>Bot: Click apply roles button
activate Bot
Bot->>Bot: Validate permissions & hierarchy
Bot->>CRM: Apply non-protected roles to user
CRM-->>Bot: Roles applied (with status: applied/blocked/protected)
Bot->>AuditLog: Log role application event
Bot->>User: Display summary (applied/blocked/protected/not-found counts)
deactivate Bot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a Discord role edit/preview/apply flow for CRM resume processing. It introduces a modal for editing suggested Discord roles, an apply button that grants roles to linked members, and safeguards to prevent assigning protected roles (Member, Admin, Steering Committee).
Changes:
- Added
ResumeEditDiscordRolesModal,ResumeEditDiscordRolesButton, andResumeApplyDiscordRolesButtonclasses for the Discord role edit/apply UI flow. - Refactored
_build_discord_role_suggestionsinto a static method with case-insensitive filtering and added protected-role exclusion. - Extended
ResumeUpdateConfirmationViewto carry role suggestions and target user context, and added comprehensive unit tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/discord_bot/src/five08/discord_bot/cogs/crm.py | Added Discord role modal, edit/apply buttons, protected-role constant, refactored suggestion builder, and wired suggestions into the confirmation view and preview flow. |
| tests/unit/test_crm.py | Added tests for role suggestion filtering, modal submit behavior, and the apply button's role assignment and status reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for role in raw.splitlines(): | ||
| role_name = role.strip() | ||
| if not role_name: | ||
| continue | ||
| key = role_name.casefold() | ||
| if key in {name.casefold() for name in _DISCORD_ROLES_PROTECTED_FROM_APPLY}: |
There was a problem hiding this comment.
The set {name.casefold() for name in _DISCORD_ROLES_PROTECTED_FROM_APPLY} is reconstructed on every iteration of the loop. Since _DISCORD_ROLES_PROTECTED_FROM_APPLY is a module-level constant, compute the casefolded set once before the loop (or define it as a module-level constant alongside _DISCORD_ROLES_PROTECTED_FROM_APPLY).
| for role in raw.splitlines(): | |
| role_name = role.strip() | |
| if not role_name: | |
| continue | |
| key = role_name.casefold() | |
| if key in {name.casefold() for name in _DISCORD_ROLES_PROTECTED_FROM_APPLY}: | |
| protected_roles_casefolded = { | |
| name.casefold() for name in _DISCORD_ROLES_PROTECTED_FROM_APPLY | |
| } | |
| for role in raw.splitlines(): | |
| role_name = role.strip() | |
| if not role_name: | |
| continue | |
| key = role_name.casefold() | |
| if key in protected_roles_casefolded: |
| for role_name in suggested_roles: | ||
| if role_name.casefold() in { | ||
| name.casefold() for name in _DISCORD_ROLES_PROTECTED_FROM_APPLY | ||
| }: |
There was a problem hiding this comment.
Same issue as in the modal's on_submit — the casefolded protected-role set is rebuilt on every loop iteration. Compute it once before the loop to avoid redundant set construction. This pattern appears in three places across this PR (lines 1012, 1364, and 3533).
| for role_name in suggested_roles: | |
| if role_name.casefold() in { | |
| name.casefold() for name in _DISCORD_ROLES_PROTECTED_FROM_APPLY | |
| }: | |
| protected_casefolded = { | |
| name.casefold() for name in _DISCORD_ROLES_PROTECTED_FROM_APPLY | |
| } | |
| for role_name in suggested_roles: | |
| if role_name.casefold() in protected_casefolded: |
| continue | ||
| role_add.append(role) | ||
|
|
||
| if not role_add and not already_assigned and not protected: |
There was a problem hiding this comment.
This guard returns early with "None of the suggested roles are assignable" when there are no roles to add, none already assigned, and no protected roles — but it does not check missing or blocked. If all suggested roles fall into the missing or blocked categories, the code falls through to defer() and sends a confusing follow-up saying "✅ No new roles to apply." with missing/blocked lines, rather than giving the user a clear upfront warning. Consider also including missing and blocked in this guard condition, or restructuring so the summary always goes through the follow-up path.
| if not role_add and not already_assigned and not protected: | |
| if ( | |
| not role_add | |
| and not already_assigned | |
| and not protected | |
| and not missing | |
| and not blocked | |
| ): |
| ) | ||
| return | ||
|
|
||
| if not (bot_member.top_role > target_member.top_role): |
There was a problem hiding this comment.
The _Role.__gt__ helper in the test stub returns False when other is not a _Role instance, but discord.Role uses position-based comparison. More importantly, the > operator on Discord roles compares by position and by ID for equal positions. If the bot's top role and target's top role are at the same position, this check will block the action even though it might be valid. This is a minor edge case, but worth noting that the comparison semantics here depend on Discord.py's Role.__gt__ implementation, which this mirrors correctly for the typical case.
| current_discord_roles=["backend"], | ||
| ) | ||
|
|
||
| assert technical == [] |
There was a problem hiding this comment.
The test asserts technical == [] but the expected filtering logic is: suggest_technical_discord_roles returns ["Backend", "Manager", "Member", "Steering Committee"], then Manager is removed by DISCORD_ROLES_NEVER_SUGGEST, Member and Steering Committee are removed by _DISCORD_ROLES_PROTECTED_FROM_APPLY, and Backend is removed because "backend" is in current_discord_roles (case-insensitive match). This is correct. No issue here — disregard.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_crm.py (1)
1253-1254:⚠️ Potential issue | 🔴 CriticalDebug field in
_build_resume_preview_embedmust reference the debug filename.The test expects the Debug field value to include both the fallback reason and a reference to
"resume-extract-debug.json"so operators know to look for the debug attachment. Currently, the implementation at lines 3397-3399 only includes the fallback reason without mentioning the filename.The Debug field should be updated to include a reference to the debug JSON filename in its value, e.g.,
f"See resume-extract-debug.json for details. Fallback: {llm_fallback_reason}".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_crm.py` around lines 1253 - 1254, The Debug field created in _build_resume_preview_embed currently only contains the fallback reason; update the value assigned to that Debug field (the field in embed.fields with name "Debug") to include the debug filename reference, e.g. build the string like "See resume-extract-debug.json for details. Fallback: {llm_fallback_reason}" (use the existing llm_fallback_reason variable) so operators can find the debug attachment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 1266-1441: Add best-effort audit writes throughout
ResumeApplyDiscordRolesButton.callback: on every early exit (no guild, no linked
user, invalid ID, user not in server, unable to resolve bot_member, missing
Manage Roles, bot role too low, no suggestions), on HTTPException failure to add
roles, and on final success. Use the existing audit helper used elsewhere in the
resume flows (call it the project’s audit helper function e.g.,
write_audit_event / audit_helper.* used in other cogs) and invoke it inside a
try/except that logs but does not alter control flow so audit failures are
non-fatal; include view.contact_id, interaction.user as actor,
target_user_id_int, action="apply_discord_roles", outcome details
(denied/reason, failed/error, or success with roles applied/summary) in the
audit payload to mirror surrounding resume action audits.
- Around line 1002-1044: The modal currently treats free-form role names as an
authorization source; fix by enforcing a server-computed immutable allowlist and
intersection both at submit and at apply time: in on_submit use a trusted set
(e.g., DISCORD_ROLES_ALLOWLIST) instead of accepting any normalized entries and
set confirmation_view.discord_role_suggestions to the intersection of normalized
and that allowlist (still update ResumeApplyDiscordRolesButton.disabled
appropriately), and ensure the code path that actually applies roles (the
resume/apply handler referenced by the ResumeApplyDiscordRolesButton and the
apply logic previously around lines 1266-1379) re-validates by intersecting
requested roles with the same server allowlist and/or verifying staff
permissions before performing any role grants.
---
Outside diff comments:
In `@tests/unit/test_crm.py`:
- Around line 1253-1254: The Debug field created in _build_resume_preview_embed
currently only contains the fallback reason; update the value assigned to that
Debug field (the field in embed.fields with name "Debug") to include the debug
filename reference, e.g. build the string like "See resume-extract-debug.json
for details. Fallback: {llm_fallback_reason}" (use the existing
llm_fallback_reason variable) so operators can find the debug attachment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2740904f-ebd0-43f6-89ab-7112c0ca574f
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm.pytests/unit/test_crm.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if normalized: | ||
| count = len(normalized) | ||
| await interaction.response.send_message( | ||
| f"✅ Discord roles updated to {count} role{'s' if count != 1 else ''}.", | ||
| ephemeral=True, | ||
| ) | ||
| return |
There was a problem hiding this comment.
When both valid and protected roles are submitted, the confirmation message only reports the count of valid roles (e.g., "✅ Discord roles updated to 2 roles.") without mentioning that protected roles like "Steering Committee" and "Admin" were silently removed. This could confuse users who don't realize their entries were filtered out. Consider appending a note about blocked roles when blocked is non-empty, e.g., mentioning which protected roles were excluded.
Summary
Summary by CodeRabbit
Release Notes