fix: keep resume contact skills array-shaped#205
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the resume → CRM contact creation flow to keep skills in the array shape expected by EspoCRM, and updates typing/tests to match.
Changes:
- Send
skillsas a list of strings (instead of a comma-separated string) when building resume-derived contact creation payloads. - Widen resume create-contact payload type hints to allow list-valued fields to pass through.
- Update the CRM unit test assertion to cover array-shaped
skills.
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 |
Updates the resume create-contact payload test to assert skills is a list. |
apps/discord_bot/src/five08/discord_bot/cogs/crm.py |
Changes resume payload construction to emit skills as a list and widens relevant payload typing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughType annotations across CRM payload-building methods are updated from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
tests/unit/test_crm.py (1)
4318-4347:⚠️ Potential issue | 🟡 MinorAdd a flow-level assertion for
skillsas well.This only proves
_build_resume_create_contact_payload()returns a list. It still won't catch the original regression ifResumeCreateContactViewor_build_contact_payload_for_link_user()coercesskillsback to a comma-delimited string beforePOST /Contact, becausetest_upload_resume_link_user_shows_confirm_then_creates_contact()still stubs the builder with only{"name": "Resume Candidate"}.Suggested follow-up test change
with ( patch( "five08.discord_bot.cogs.crm.check_user_roles_with_hierarchy", return_value=True, ), patch.object( crm_cog, "_find_contact_by_discord_id", new=AsyncMock(return_value=None) ), patch.object( crm_cog, "_upload_resume_attachment_to_contact", new=AsyncMock() ) as mock_upload, patch.object( crm_cog, "_build_resume_create_contact_payload", - return_value={"name": "Resume Candidate"}, + return_value={ + "name": "Resume Candidate", + "skills": ["Python", "fastapi"], + }, ), patch( "five08.discord_bot.cogs.crm.settings.api_shared_secret", "test-shared-secret", ), ): ... crm_cog.espo_api.request.assert_called_once_with( "POST", "Contact", { "name": "Candidate User", "firstName": "Candidate", "lastName": "User", "cDiscordUsername": "candidateuser", "cDiscordUserID": "202", + "skills": ["Python", "fastapi"], }, )🤖 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 4318 - 4347, The test only asserts _build_resume_create_contact_payload returns a list for skills but does not assert the end-to-end flow preserves that list; update the flow-level test test_upload_resume_link_user_shows_confirm_then_creates_contact to include a stubbed builder payload that contains "skills": ["Python","fastapi"] (or assert on the outgoing POST payload) so the test verifies ResumeCreateContactView/_build_contact_payload_for_link_user does not coerce skills back to a comma string; reference the test name test_upload_resume_link_user_shows_confirm_then_creates_contact and the builder/function _build_resume_create_contact_payload (and ensure the POST payload assertion inspects the "skills" field).
🧹 Nitpick comments (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py (1)
5404-5410: Normalizeskillsbefore the first CRM write.This now keeps
skillsarray-shaped, but it still forwards raw extracted entries. Runningnormalize_skill_list()here would dedupe/canonicalize casing and aliases before creating the contact, which keeps this path consistent with the rest of the skill-handling flow.♻️ Suggested refinement
if skills: - normalized_skills = [ - str(item).strip() for item in skills if str(item).strip() - ] + normalized_skills = normalize_skill_list( + [str(item).strip() for item in skills if str(item).strip()] + ) if normalized_skills: payload["skills"] = normalized_skills🤖 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 5404 - 5410, The skills list is being trimmed but not canonicalized before the first CRM write; call the existing normalize_skill_list() on the cleaned list (the local normalized_skills) and assign its result to payload["skills"] so the contact creation uses deduped/canonicalized skill names and aliases; ensure you handle the case where normalize_skill_list() returns an empty list (don’t set payload["skills"] in that case) and keep the variable names skills, normalized_skills, and payload["skills"] as shown so the change is easy to locate.
🤖 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 `@tests/unit/test_crm.py`:
- Around line 4318-4347: The test only asserts
_build_resume_create_contact_payload returns a list for skills but does not
assert the end-to-end flow preserves that list; update the flow-level test
test_upload_resume_link_user_shows_confirm_then_creates_contact to include a
stubbed builder payload that contains "skills": ["Python","fastapi"] (or assert
on the outgoing POST payload) so the test verifies
ResumeCreateContactView/_build_contact_payload_for_link_user does not coerce
skills back to a comma string; reference the test name
test_upload_resume_link_user_shows_confirm_then_creates_contact and the
builder/function _build_resume_create_contact_payload (and ensure the POST
payload assertion inspects the "skills" field).
---
Nitpick comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 5404-5410: The skills list is being trimmed but not canonicalized
before the first CRM write; call the existing normalize_skill_list() on the
cleaned list (the local normalized_skills) and assign its result to
payload["skills"] so the contact creation uses deduped/canonicalized skill names
and aliases; ensure you handle the case where normalize_skill_list() returns an
empty list (don’t set payload["skills"] in that case) and keep the variable
names skills, normalized_skills, and payload["skills"] as shown so the change is
easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b558b5f-8ed3-4ae2-a5d8-4383f62755b3
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm.pytests/unit/test_crm.py
Description
Keep resume-derived contact creation payloads aligned with EspoCRM by sending
skillsas an array instead of a comma-separated string.Widen the related payload type hints in the resume create-contact flow so list-valued fields pass through cleanly.
Update the CRM unit test to cover the array-shaped
skillspayload.Related Issue
None.
How Has This Been Tested?
uv run pytest -q tests/unit/test_crm.py -k 'build_resume_create_contact_payload or upload_resume_link_user_shows_confirm_then_creates_contact or resume_create_contact_view'Summary by CodeRabbit