Uh oh!
There was an error while loading. Please reload this page.
fix: view-skills command now shows full contact profile (#61) - #225
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 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.
🧹 Nitpick comments (1)
tests/unit/test_crm.py (1)
3312-3314: Also assert the fallback path still includes the👤 Profilefield.The fallback test checks skills at index 1, but it doesn’t explicitly verify that index 0 is the profile field. Adding that assertion will lock in the new contract for this path too.
Suggested test hardening
- assert "python" in embed.fields[1].value- assert "sql" in embed.fields[1].value- assert "/5" not in embed.fields[1].value+ profile_field = embed.fields[0]+ assert profile_field.name == "👤 Profile"++ skills_field = embed.fields[1]+ assert "python" in skills_field.value+ assert "sql" in skills_field.value+ assert "/5" not in skills_field.value🤖 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 3312 - 3314, The test currently asserts skills in embed.fields[1] but doesn't verify the fallback includes the profile field; update the unit test (where embed is checked) to also assert that embed.fields[0].name (or embed.fields[0].value) contains the profile indicator "👤 Profile" so the fallback contract is enforced along with the existing skill checks for embed.fields[1].
🤖 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_crm.py`:
- Around line 3312-3314: The test currently asserts skills in embed.fields[1]
but doesn't verify the fallback includes the profile field; update the unit test
(where embed is checked) to also assert that embed.fields[0].name (or
embed.fields[0].value) contains the profile indicator "👤 Profile" so the
fallback contract is enforced along with the existing skill checks for
embed.fields[1].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab5fd205-a904-4e63-b525-1debdbb57748
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm.pytests/unit/test_crm.py
Before
After
Added full contact profile display to the /view-skills command.
Previously only skills were shown. Now a Profile field appears above
the skills list, showing email, type, 508 email, and Discord —
consistent with what /search-members displays.
The original standalone CRM Profile link field has been consolidated
into the new Profile field.
Related Issue:
View skills command should also show full profile, not just show skills #61
How Has This Been Tested?
Updated existing unit tests in tests/unit/test_crm.py to assert that
the embed now includes a Profile field containing email, type, and
508 email.
Summary by CodeRabbit