fix: refine candidate match formatting#177
Conversation
📝 WalkthroughWalkthroughThe pull request adds two new fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 refines the /match-candidates output formatting by preferring CRM full names (and linking to CRM when possible) and by showing a non-mention Discord username alongside the candidate entry, supported by expanding the shared candidate search result model.
Changes:
- Extend
CandidateMatch/search_candidatesto return bothcrm_nameanddiscord_username. - Update
/match-candidatesline formatter to render a CRM-linked display name (when available) and append a non-mention Discord username, removing “Discord ID” from normal output. - Add/update unit tests for the new mapping fields and formatting behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
apps/discord_bot/src/five08/discord_bot/cogs/jobs.py |
Adjusts match output formatting to prefer CRM name + CRM link and include a non-mention Discord username. |
packages/shared/src/five08/candidate_search.py |
Expands CandidateMatch and SQL selection/mapping to provide crm_name and discord_username. |
tests/unit/test_candidate_search.py |
Updates row→CandidateMatch mapping test to assert new fields. |
tests/unit/test_jobs.py |
Adds unit tests validating the updated match-candidate line formatting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| crm_name = discord.utils.escape_mentions( | ||
| candidate.crm_name or candidate.name or "Unknown" | ||
| ) | ||
| discord_username = ( | ||
| discord.utils.escape_mentions( | ||
| str(candidate.discord_username).lstrip("@") | ||
| ) | ||
| if candidate.discord_username | ||
| else None | ||
| ) |
There was a problem hiding this comment.
_build_match_candidate_lines now assumes candidate.crm_name and candidate.discord_username are present and string-like. In existing unit tests that patch search_candidates with plain Mock() candidates, missing attributes evaluate to truthy Mock objects and will flow into escape_mentions, producing incorrect output. Consider guarding with isinstance(..., str) (or explicitly coercing/normalizing) and/or tightening the expected candidate type so callers/tests must provide these fields (e.g., set them to None).
| crm_name = discord.utils.escape_mentions( | |
| candidate.crm_name or candidate.name or "Unknown" | |
| ) | |
| discord_username = ( | |
| discord.utils.escape_mentions( | |
| str(candidate.discord_username).lstrip("@") | |
| ) | |
| if candidate.discord_username | |
| else None | |
| ) | |
| # Normalize CRM/display name to a safe string. | |
| raw_crm_name: Any = getattr(candidate, "crm_name", None) | |
| if not isinstance(raw_crm_name, str) or not raw_crm_name.strip(): | |
| raw_crm_name = getattr(candidate, "name", None) | |
| if not isinstance(raw_crm_name, str) or not raw_crm_name.strip(): | |
| raw_crm_name = "Unknown" | |
| crm_name = discord.utils.escape_mentions(raw_crm_name) | |
| # Normalize Discord username to a safe string, or omit if not a string. | |
| raw_discord_username: Any = getattr(candidate, "discord_username", None) | |
| if isinstance(raw_discord_username, str) and raw_discord_username.strip(): | |
| discord_username = discord.utils.escape_mentions( | |
| raw_discord_username.lstrip("@") | |
| ) | |
| else: | |
| discord_username = None |
| import os | ||
| from types import SimpleNamespace | ||
|
|
||
| os.environ.setdefault("DISCORD_BOT_TOKEN", "test") | ||
| os.environ.setdefault("ESPO_API_KEY", "test") | ||
| os.environ.setdefault("ESPO_BASE_URL", "https://crm.example.invalid") | ||
| os.environ.setdefault("KIMAI_BASE_URL", "https://kimai.example.invalid") | ||
| os.environ.setdefault("KIMAI_API_TOKEN", "test") | ||
|
|
||
| from five08.discord_bot.cogs.jobs import JobsCog |
There was a problem hiding this comment.
This test module sets environment variables at import time to allow importing JobsCog (which imports settings). That makes test order/import behavior matter and duplicates the repo-wide mock_env_vars autouse fixture in tests/conftest.py. Prefer moving the JobsCog import inside the test functions (after fixtures run) or using monkeypatch in a fixture so env setup is centralized and doesn't leak across modules.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_candidate_search.py (1)
193-218: Consider adding missing fields to_make_rowbase dict.The base dict doesn't include
crm_name,discord_username,has_crm_link, ordiscord_user_id. Whilesearch_candidateshandles missing keys gracefully viarow.get(), adding explicit defaults would improve test maintainability and make the test data structure align with the actual CandidateMatch fields.♻️ Suggested addition to base dict
def _make_row(**overrides: object) -> dict: """Build a minimal fake DB row dict.""" base: dict = { "crm_contact_id": "c1", "name": "Alice", + "crm_name": "Alice CRM", + "discord_username": None, + "discord_user_id": None, + "has_crm_link": True, "email_508": "alice@508.dev",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_candidate_search.py` around lines 193 - 218, The _make_row helper is missing several fields used by CandidateMatch; update the base dict inside _make_row(**overrides) to include explicit default keys crm_name (empty string), discord_username (None or empty string), has_crm_link (False), and discord_user_id (None) so tests mirror the real CandidateMatch shape; keep overrides behavior intact so callers can still override these defaults and ensure any code referencing row.get(...) still passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test_candidate_search.py`:
- Around line 193-218: The _make_row helper is missing several fields used by
CandidateMatch; update the base dict inside _make_row(**overrides) to include
explicit default keys crm_name (empty string), discord_username (None or empty
string), has_crm_link (False), and discord_user_id (None) so tests mirror the
real CandidateMatch shape; keep overrides behavior intact so callers can still
override these defaults and ensure any code referencing row.get(...) still
passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1c23263-7960-4e91-9544-edae21cbbb95
📒 Files selected for processing (4)
apps/discord_bot/src/five08/discord_bot/cogs/jobs.pypackages/shared/src/five08/candidate_search.pytests/unit/test_candidate_search.pytests/unit/test_jobs.py
Description
This change improves
/match-candidatesoutput by preferring CRM full names for the displayed person title and linking the name to the CRM record when available.It now includes a non-mention Discord username in the same result line for easy identity context, with no fallback to Discord ID in normal output.
search_candidatesnow returns bothcrm_nameanddiscord_usernamefrom the matching query so the bot can render those fields separately and consistently.Unit tests were added for both the new candidate mapping fields and the formatter behavior.
How Has This Been Tested?
uv run pytest tests/unit/test_candidate_search.py tests/unit/test_jobs.py(29 passed)Summary by CodeRabbit
New Features
Tests