Conversation
📝 WalkthroughWalkthroughAdded a requester-guarded overwrite confirmation UI and expanded Discord-linking logic to detect no-op and conflicting link states, optionally require explicit overwrite, and normalize legacy username-embedded IDs before updating CRM. Changes
Sequence DiagramsequenceDiagram
participant User as Discord User
participant Cog as CRM Cog
participant CRM as CRM API
participant View as Confirmation View
User->>Cog: Link Discord Account
Cog->>CRM: GET Contact (select includes cDiscordUserID, cDiscordUsername)
CRM-->>Cog: Contact Data
alt Contact already linked to same Discord user
Cog-->>User: "Nothing changed" (ephemeral)
else Contact linked to different Discord user and allow_overwrite is false
Cog-->>User: Send message with overwrite required + Confirmation View
View-->>User: Display Overwrite / Cancel buttons
alt User clicks Overwrite
View->>Cog: confirm_overwrite() -> calls _perform_discord_linking(..., allow_overwrite=True)
Cog->>CRM: PUT updated contact
CRM-->>Cog: Success
Cog-->>User: Link Updated
else User clicks Cancel
View-->>User: Cancellation message (ephemeral)
end
else Contact not linked or overwrite allowed
Cog->>CRM: PUT contact with new Discord link
CRM-->>Cog: Success
Cog-->>User: Link Created/Updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3248133758
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR updates the /link-discord-user CRM command to prevent silently overwriting an existing contact’s Discord linkage by introducing explicit overwrite confirmation when the stored Discord ID differs, and by treating identical linkage requests as a no-op. It also adds unit tests for the new behaviors.
Changes:
- Added
DiscordLinkOverwriteConfirmationViewto require explicit confirmation before overwriting an existing (different) Discord user ID on a contact. - Added helper accessors for stored Discord ID/username and updated
_perform_discord_linkingto support no-op, username refresh, and confirmation-required flows. - Expanded unit tests to cover username-only refresh, no-op, and overwrite-confirmation-required scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/discord_bot/src/five08/discord_bot/cogs/crm.py | Adds overwrite confirmation UI and implements guarded linking logic based on stored Discord ID/username. |
| tests/unit/test_crm.py | Adds unit coverage for no-op and overwrite-confirmation flows and updates the success case to reflect username refresh behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py (1)
6393-6400:⚠️ Potential issue | 🔴 CriticalReload the contact before checking for overwrite conflicts.
For the normal non-ID
/link-discord-userpath, Line 7833 passes a lookup row from_search_contacts_for_lookup(), and the defaultselecton Line 5658 does not includecDiscordUserID. In that flowexisting_discord_user_idis usuallyNone, so the overwrite confirmation never appears. Re-fetchingContact/{contact_id}here also avoids confirming against stale data if the record changes before the button is clicked.🔄 Possible fix
try: contact_id = contact.get("id") - contact_name = contact.get("name", "Unknown") + if not contact_id: + await interaction.followup.send("❌ Contact ID not found.") + return False + + current_contact = self.espo_api.request("GET", f"Contact/{contact_id}") + if isinstance(current_contact, dict): + contact = current_contact + contact_name = contact.get("name", "Unknown")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py` around lines 6393 - 6400, Reload the contact record from the API before reading cDiscordUserID/cDiscordUsername to avoid stale or incomplete lookup rows: in the handler where you compute contact_id, contact_name, discord_display, discord_user_id and then call _contact_discord_user_id/_contact_discord_username, perform a fresh fetch of Contact/{contact_id} (the same call used elsewhere to retrieve full contact fields) and use that fresh contact object for existing_discord_user_id and existing_discord_username checks; this ensures the overwrite confirmation logic (used by the /link-discord-user flow and button handlers) sees the current cDiscordUserID and cDiscordUsername rather than the possibly truncated row returned by _search_contacts_for_lookup().
🤖 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 6406-6443: The early-return branches inside
_perform_discord_linking currently call _audit_command which can raise and break
the Discord flow; change those calls to use the best-effort _audit_command_safe
instead (preserving the same parameters: interaction,
action="crm.link_discord_user.execute", result, metadata,
resource_type="crm_contact", resource_id=str(contact_id)) so the no-op and
overwrite-confirmation paths continue even if audit writes fail.
- Around line 6197-6205: _contact_discord_user_id currently only reads
cDiscordUserID and ignores legacy "(ID: 123)" suffixes stored in
cDiscordUsername, causing records to be treated as unlinked; update the helper
(_contact_discord_user_id) to, after normalizing cDiscordUserID as shown, also
check contact.get("cDiscordUsername") for a trailing "(ID: 123)" pattern and
extract and return the numeric ID when present (preserve existing checks like
empty string and "No Discord"); reference cDiscordUserID and cDiscordUsername in
your change and return the recovered ID as the normalized string so the
overwrite guard treats the record as linked.
---
Outside diff comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 6393-6400: Reload the contact record from the API before reading
cDiscordUserID/cDiscordUsername to avoid stale or incomplete lookup rows: in the
handler where you compute contact_id, contact_name, discord_display,
discord_user_id and then call
_contact_discord_user_id/_contact_discord_username, perform a fresh fetch of
Contact/{contact_id} (the same call used elsewhere to retrieve full contact
fields) and use that fresh contact object for existing_discord_user_id and
existing_discord_username checks; this ensures the overwrite confirmation logic
(used by the /link-discord-user flow and button handlers) sees the current
cDiscordUserID and cDiscordUsername rather than the possibly truncated row
returned by _search_contacts_for_lookup().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca243515-5af8-4606-9aae-3cdcc16cc232
📒 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py (1)
6406-6470:⚠️ Potential issue | 🔴 CriticalReload the contact before evaluating the overwrite guard.
These branches read
cDiscordUserIDandcDiscordUsernamefrom thecontactsnapshot returned by the earlier lookup or selection. If another admin relinks the same CRM record before this method runs — or before the overwrite button is clicked — the stale snapshot can still fall through to thePUTand silently replace the newer link, which defeats the guard this PR is adding.🛠️ Minimal change
try: contact_id = contact.get("id") - contact_name = contact.get("name", "Unknown") + if not contact_id: + await interaction.followup.send("❌ Contact ID not found.") + return False + current_contact = self.espo_api.request("GET", f"Contact/{contact_id}") + contact_name = current_contact.get("name", contact.get("name", "Unknown")) @@ - existing_discord_user_id = self._contact_discord_user_id(contact) - existing_discord_username = self._contact_discord_username(contact) + existing_discord_user_id = self._contact_discord_user_id(current_contact) + existing_discord_username = self._contact_discord_username(current_contact)
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0490c228-077e-4389-ac5d-4ae97acf9054
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm.pytests/unit/test_crm.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_crm.py
Description
Prevent
/link-discord-userfrom silently replacing a different Discord account on an existing contact.Treat matching Discord link requests as a no-op, allow username-only refreshes when the Discord ID is unchanged, and require explicit overwrite confirmation when the stored Discord ID differs.
Add unit coverage for the successful username refresh path, the no-op path, and the overwrite confirmation path.
Related Issue
None.
How Has This Been Tested?
uv run pytest tests/unit/test_crm.py -k "link_discord_user"Summary by CodeRabbit
New Features
Bug Fixes