Uh oh!
There was an error while loading. Please reload this page.
feat: show requested skill strengths in search - #55
Merged
Conversation
📝 WalkthroughWalkthroughThis PR enhances the CRM Discord bot's search functionality by adding skill strength parsing and display. Three new helper methods parse contact skill attributes from JSON payloads, normalize skill names, and format requested skills with their strength values for display in search results. Unit tests verify correct handling of skill strengths and malformed payloads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
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)
1336-1343:⚠️ Potential issue | 🟠 MajorAdd error handling for optional
cSkillAttrsfield in search.The
/search-memberscommand currently fails ifcSkillAttrsis unavailable in the workspace. EspoCRM's REST API will return a 500 error if a field inselectexists in metadata but the corresponding database column is missing or misconfigured. SincecSkillAttrsis a custom field that may not exist in all workspaces, implement a fallback retry without it:Suggested fix
- search_params = {+ base_select = (+ "id,name,emailAddress,c508Email,cDiscordUsername,cDiscordUserID,"+ "phoneNumber,type,resumeIds,resumeNames,resumeTypes,skills"+ )+ search_params = { "where": where_filters, "maxSize": 10, - "select": "id,name,emailAddress,c508Email,cDiscordUsername,cDiscordUserID,phoneNumber,type,resumeIds,resumeNames,resumeTypes,skills,cSkillAttrs",+ "select": f"{base_select},cSkillAttrs", } -- response = self.espo_api.request("GET", "Contact", search_params)+ try:+ response = self.espo_api.request("GET", "Contact", search_params)+ except EspoAPIError as exc:+ if "cSkillAttrs" in str(exc):+ logger.warning(+ "cSkillAttrs unavailable in this CRM; retrying search without it: %s",+ exc,+ )+ search_params["select"] = base_select+ response = self.espo_api.request("GET", "Contact", search_params)+ else:+ raise🤖 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 1336 - 1343, The search currently includes "cSkillAttrs" in the select and will raise if the field/column is missing; wrap the call to self.espo_api.request("GET", "Contact", search_params) in error handling that detects a 500/field-missing response and retries the request with a fallback search_params that removes "cSkillAttrs" from the select string (keep other keys like "where" and "maxSize" unchanged), then set contacts = response.get("list", []) from the successful call; ensure this logic lives around where search_params, response and contacts are defined so the retry path populates contacts the same way.
🧹 Nitpick comments (1)
tests/unit/test_crm.py (1)
229-289: Add one happy-path test for JSON-stringcSkillAttrs.These tests cover dict payloads and malformed payloads well, but not the valid JSON-string path the parser supports. Adding that case would fully cover the new parsing behavior.
✅ Suggested test addition
+ `@pytest.mark.asyncio`+ async def test_search_contacts_parses_skill_attrs_json_string(+ self, crm_cog, mock_interaction, mock_member_role+ ):+ mock_interaction.user.roles = [mock_member_role]+ crm_cog.espo_api.request.return_value = {+ "list": [+ {+ "id": "contact123",+ "name": "John Doe",+ "emailAddress": "john@example.com",+ "type": "Member",+ "cSkillAttrs": '{"python":{"strength":"5"},"sql":{"strength":4}}',+ }+ ]+ }++ await crm_cog.search_members.callback(+ crm_cog, mock_interaction, None, "python,sql"+ )++ embed = mock_interaction.followup.send.call_args.kwargs["embed"]+ field_value = embed.fields[0].value+ assert "🧠 Skills: python (5), sql (4)" in 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 229 - 289, Add a new async pytest in tests/unit/test_crm.py that exercises the JSON-string path for cSkillAttrs: create a test (e.g., test_search_contacts_parses_json_string_cskillattrs) which sets mock_interaction.user.roles = [mock_member_role], sets crm_cog.espo_api.request.return_value to return a contact whose "cSkillAttrs" value is a JSON string (json.dumps of the same dict used in test_search_contacts_shows_skill_strengths), call crm_cog.search_members.callback(crm_cog, mock_interaction, None, "python,sql,aws"), then assert mock_interaction.followup.send was called and the embed (from call_args.kwargs["embed"]) contains the expected skills/strengths text similar to the existing assertions; follow the same structure and naming used by test_search_contacts_shows_skill_strengths and test_search_contacts_ignores_broken_skill_attrs so the test integrates consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 1336-1343: The search currently includes "cSkillAttrs" in the
select and will raise if the field/column is missing; wrap the call to
self.espo_api.request("GET", "Contact", search_params) in error handling that
detects a 500/field-missing response and retries the request with a fallback
search_params that removes "cSkillAttrs" from the select string (keep other keys
like "where" and "maxSize" unchanged), then set contacts = response.get("list",
[]) from the successful call; ensure this logic lives around where
search_params, response and contacts are defined so the retry path populates
contacts the same way.
---
Nitpick comments:
In `@tests/unit/test_crm.py`:
- Around line 229-289: Add a new async pytest in tests/unit/test_crm.py that
exercises the JSON-string path for cSkillAttrs: create a test (e.g.,
test_search_contacts_parses_json_string_cskillattrs) which sets
mock_interaction.user.roles = [mock_member_role], sets
crm_cog.espo_api.request.return_value to return a contact whose "cSkillAttrs"
value is a JSON string (json.dumps of the same dict used in
test_search_contacts_shows_skill_strengths), call
crm_cog.search_members.callback(crm_cog, mock_interaction, None,
"python,sql,aws"), then assert mock_interaction.followup.send was called and the
embed (from call_args.kwargs["embed"]) contains the expected skills/strengths
text similar to the existing assertions; follow the same structure and naming
used by test_search_contacts_shows_skill_strengths and
test_search_contacts_ignores_broken_skill_attrs so the test integrates
consistently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary
Enhances the
/search-memberscommand to include requested-skill strength annotations (for examplepython (5)) directly in results whencSkillAttrsis available and valid.cSkillAttrsvalues with best-effort fallback for malformed data.selectfields to includeskillsandcSkillAttrsso skill metadata is available in results.cSkillAttrshandling, plus select fields assertions.Related Issue
None.
How Has This Been Tested?
Ran unit tests for
tests/unit/test_crm.pyand validated formatting/parsing scenarios through assertions.Summary by CodeRabbit
Release Notes