[codex] Add member agreement contact selection#329
Conversation
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Warning Review limit reached
More reviews will be available in 17 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors the ChangesMember Agreement Contact Selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Adds an interactive contact disambiguation flow to the Discord /send-member-agreement command so ambiguous CRM searches present a button-based selection UI, then routes the chosen contact through the existing single-contact send/audit logic.
Changes:
- Introduces
MemberAgreementSelectionView+ per-contact buttons to select which CRM contact should receive the member agreement. - Refactors the member-agreement send logic into a shared
_send_member_agreement_for_contact_flowused by both the slash command and the selection button callback. - Adds unit tests covering the ambiguous-search selection view and the button callback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/test_crm.py | Adds focused unit tests for the new multi-contact selection UI and callback flow. |
| apps/discord_bot/src/five08/discord_bot/cogs/crm.py | Implements the member agreement selection view/buttons and updates the command flow to use it when multiple contacts match. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 26a90f07d1
ℹ️ 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".
| await self.view.crm_cog._send_member_agreement_for_contact_flow( | ||
| interaction=interaction, | ||
| contact=self.contact, | ||
| search_term=self.view.search_term, | ||
| ) |
There was a problem hiding this comment.
When the requester double-clicks a contact button or clicks another contact before the DocuSeal request returns, both component callbacks can reach this awaited send path because the view is only disabled after _send_member_agreement_for_contact_flow finishes. Each callback can create its own DocuSeal submission, so an ambiguous search can send duplicate agreements or agreements to multiple matches; guard the view with an in-progress flag or disable/edit the controls before awaiting the send.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
tests/unit/test_crm.py (1)
7669-7711: ⚡ Quick winAdd a non-requester callback test for the selection button.
You already cover the happy path. Please add one test where
interaction.user.id != requester_idand assert_send_member_agreement_for_contact_flowis not called; this locks the requester-validation contract against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_crm.py` around lines 7669 - 7711, Add a new async pytest that mirrors test_member_agreement_selection_button_sends_selected_contact but sets interaction.user.id to a value different from the view.requester_id (e.g., requester_id=123 and interaction.user.id=999), calls the same button.callback obtained from MemberAgreementSelectionView after add_contact_button, and asserts crm_cog._send_member_agreement_for_contact_flow.assert_not_awaited(); also assert interaction.response.defer and interaction.message.edit were not awaited and that the view buttons remain enabled (i.e., not all children disabled) to lock requester-validation behavior in MemberAgreementSelectionView and add_contact_button.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 733-738: The branches that reject non-requesters (checks against
self.requester_id that call interaction.response.send_message and return) must
also emit an audit record before returning; update the member-agreement button
callback handlers in crm.py to POST an audit event to the worker ingest endpoint
(use AUDIT_API_BASE_URL and API_SHARED_SECRET) whenever you send the "Only the
command requester..." or similar denial messages (the branches using
interaction.response.send_message and self.requester_id around those callbacks).
Ensure the audit payload includes actor id, action ("member-agreement_denied" or
similar), target/context (guild/message/request id), timestamp, and a reason,
sign or authorize the request with API_SHARED_SECRET, and only then return after
emitting the audit.
In `@tests/unit/test_crm.py`:
- Around line 7643-7650: Replace real-looking personal emails in the test
fixture entries that use the "emailAddress" field (e.g., the records with "id":
"crm-456" / name "Wilson Kao" and the other record around the same block) with
safe reserved example addresses (for example using example.com or clearly
synthetic domains) so tests avoid storing potential PII; update the two
occurrences noted (current values like "will.gutierrez@gmail.com" and
"lairwaves5888@gmail.com" and the similar entries around lines 7674-7678) to use
addresses such as "user@example.com" or "wilson.kao@example.com".
---
Nitpick comments:
In `@tests/unit/test_crm.py`:
- Around line 7669-7711: Add a new async pytest that mirrors
test_member_agreement_selection_button_sends_selected_contact but sets
interaction.user.id to a value different from the view.requester_id (e.g.,
requester_id=123 and interaction.user.id=999), calls the same button.callback
obtained from MemberAgreementSelectionView after add_contact_button, and asserts
crm_cog._send_member_agreement_for_contact_flow.assert_not_awaited(); also
assert interaction.response.defer and interaction.message.edit were not awaited
and that the view buttons remain enabled (i.e., not all children disabled) to
lock requester-validation behavior in MemberAgreementSelectionView and
add_contact_button.
🪄 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: 07880d37-577a-4702-b99c-ab7420af863f
📒 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a879d39aae
ℹ️ 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".
| search_term=search_term, | ||
| ) | ||
|
|
||
| for i, contact in enumerate(contacts[:5], 1): |
There was a problem hiding this comment.
When an ambiguous search returns more than five contacts, this loop only inspects contacts[:5] for both display and buttons. If the first five matches are already signed but a later match is unsigned, has_send_options becomes false and the command sends an embed with no controls, even though there is a valid contact that could receive the agreement; users then cannot select that result from this flow and are not told that eligible matches were omitted.
Useful? React with 👍 / 👎.
| for i, contact in enumerate(contacts[:5], 1): | ||
| name = contact.get("name", "Unknown") | ||
| email = self._contact_preferred_email(contact) or "No email" | ||
| contact_id = contact.get("id", "") | ||
| signed_at = self._contact_text_value( | ||
| contact.get(MEMBER_AGREEMENT_SIGNED_AT_FIELD) | ||
| ) | ||
| agreement_status = ( | ||
| f"✅ Already signed: `{signed_at}`" | ||
| if signed_at | ||
| else "📝 Not signed; send/resend allowed" | ||
| ) | ||
| contact_info = f"📧 {email}\n🆔 ID: `{contact_id}`\n{agreement_status}" | ||
| embed.add_field(name=f"{i}. {name}", value=contact_info, inline=True) | ||
| if not signed_at: | ||
| view.add_contact_button(contact) | ||
|
|
| embed.add_field( | ||
| name="💡 Tip", | ||
| value=( | ||
| "Select an unsigned contact button to continue. " | ||
| "Prior send requests are not tracked, so unsigned contacts can be resent." | ||
| ), | ||
| inline=False, | ||
| ) | ||
|
|
||
| has_send_options = len(view.children) > 0 | ||
| if has_send_options: | ||
| message = await interaction.followup.send( | ||
| embed=embed, | ||
| view=view, | ||
| ephemeral=True, | ||
| wait=True, | ||
| ) | ||
| view.set_message(message) | ||
| return | ||
|
|
||
| await interaction.followup.send( | ||
| embed=embed, | ||
| ephemeral=True, | ||
| ) |
Summary
/send-member-agreementfinds multiple CRM contacts instead of returning only a refine-search message.Validation
uv run pytest tests/unit/test_crm.py -k "send_member_agreement or member_agreement_selection" -quv run ruff check apps/discord_bot/src/five08/discord_bot/cogs/crm.py tests/unit/test_crm.pygit diff --checkSummary by CodeRabbit