Add skills edit modal and skills delta preview#166
Merged
michaelmwu merged 4 commits intomainfrom Mar 5, 2026
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a Discord UI flow for editing extracted resume skills (including per-skill strength) before applying CRM updates, and improves the resume preview embed by summarizing skill deltas.
Changes:
- Add an “Edit Skills” button + modal to the resume update confirmation view, including parsing/serialization of skill strengths.
- Add skills delta summaries (added/removed/strength changes) to the resume preview embed.
- Extend unit tests to cover skills button/modal presence, prepopulation, and submission behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
apps/discord_bot/src/five08/discord_bot/cogs/crm.py |
Implements skills edit modal/button, skill normalization/strength parsing, and skills delta preview formatting. |
tests/unit/test_crm.py |
Adds unit tests for skills edit UI behavior and modal submission outcomes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3015
to
+3024
| if field_name == "skills": | ||
| delta = format_skill_delta( | ||
| change.get("current"), change.get("proposed") | ||
| ) | ||
| if delta: | ||
| truncated_delta = truncate_preview_value( | ||
| delta, field_name=field_name, label=label | ||
| ) | ||
| lines.append(f"**{label}**: `{truncated_delta}`") | ||
| continue |
There was a problem hiding this comment.
The new skills-specific delta rendering in _build_resume_preview_embed (the format_skill_delta branch for field_name == "skills") isn’t covered by unit tests. Adding tests that assert the “Added/Removed/Strengths” summary for a few representative skill snapshots (add/remove/strength change, and no-op) would help prevent regressions in the preview UX.
📝 WalkthroughWalkthroughAdds an interactive "Edit Skills" modal and button to the CRM resume confirmation flow, plus parsing/normalization helpers and skill-delta rendering for proposed updates. Changes
Sequence DiagramsequenceDiagram
participant User
participant Button as ResumeEditSkillsButton
participant Modal as ResumeEditSkillsModal
participant View as ResumeUpdateConfirmationView
participant Parser as SkillParser
User->>Button: click Edit Skills
Button->>Modal: open with proposed_updates
Modal->>Parser: request prepopulate (skills + strengths)
Parser-->>Modal: return formatted lines
Modal->>User: show editable skills list
User->>Modal: submit edited lines
Modal->>Parser: parse into skills + cSkillAttrs
Parser-->>Modal: return normalized skills and strengths map
Modal->>View: update proposed_updates
View->>Parser: compute skill delta between current and proposed
Parser-->>View: return Adds/Removals/Strengths
View->>User: render updated confirmation with delta
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![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 (3)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py (2)
3015-3024: Use_field_label(field)for preview labels here as well.This block still renders with
labelfrom the change payload. Using_field_label(field_name)keeps configurable aliases (like LinkedIn) consistent with the rest of the preview/apply UI.💡 Suggested fix
if field_name == "skills": + rendered_label = ( + ResumeUpdateConfirmationView._field_label(field_name) + if field_name + else label + ) delta = format_skill_delta( change.get("current"), change.get("proposed") ) if delta: truncated_delta = truncate_preview_value( delta, field_name=field_name, label=label ) - lines.append(f"**{label}**: `{truncated_delta}`") + lines.append(f"**{rendered_label}**: `{truncated_delta}`") continueBased on learnings: In
apps/discord_bot/src/five08/discord_bot/cogs/crm.py, when rendering field labels in preview embeds, always useResumeUpdateConfirmationView._field_label(field)so configurable aliases are respected consistently.🤖 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 3015 - 3024, The preview rendering for the "skills" branch uses the incoming `label` from the change payload instead of the canonical label helper; update this block to derive the display label via `ResumeUpdateConfirmationView._field_label(field_name)` (instead of using `label`) before calling `truncate_preview_value` and when formatting the `lines` entry; keep the rest of the logic (`format_skill_delta`, `truncate_preview_value`, `delta` check, and the `lines.append` behavior) the same so aliases like LinkedIn are applied consistently.
985-996: Invalid:strengthvalues are silently ignored.Entries like
python:abcorpython:8currently degrade to plain skills without feedback, which is easy to miss for users editing strengths.💡 Suggested direction
if ":" in line: token, raw_strength = line.rsplit(":", 1) token = token.strip() raw_strength = raw_strength.strip() if raw_strength: try: parsed_strength = int(float(raw_strength)) except (TypeError, ValueError): - parsed_strength = None - if parsed_strength and 1 <= parsed_strength <= 5: - strength_value = parsed_strength + parsed_strength = None + if parsed_strength is None or not 1 <= parsed_strength <= 5: + continue # or collect invalid lines and report in on_submit + strength_value = parsed_strength🤖 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 985 - 996, The current parsing block in crm.py (variables token, raw_strength, parsed_strength, strength_value) silently ignores invalid strengths like "python:abc" or out-of-range "python:8"; change it so that when raw_strength is present but cannot be parsed to an integer or is not in 1..5 you do not silently drop it but surface the problem: either raise/return a clear ValueError with the offending token and raw_strength, or log/warn and collect invalid entries to return to the caller so the user can be informed; specifically update the code around the token/raw_strength parsing to detect parse failures and out-of-range values and emit a clear error message (including token and raw_strength) instead of leaving parsed_strength None and continuing.tests/unit/test_crm.py (1)
464-493: Avoid coupling this test to one internalcSkillAttrsrepresentation.Line 487 currently assumes
cSkillAttrsis always a JSON string. If the implementation stores a dict directly, behavior is still correct but this test fails.Proposed test hardening
- parsed_attrs = json.loads(view.proposed_updates["cSkillAttrs"]) + raw_attrs = view.proposed_updates["cSkillAttrs"] + parsed_attrs = json.loads(raw_attrs) if isinstance(raw_attrs, str) else raw_attrs assert parsed_attrs == { "python": {"strength": 5}, "rust": {"strength": 4}, }🤖 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 464 - 493, The test test_edit_skills_modal_submit_updates_proposed assumes view.proposed_updates["cSkillAttrs"] is a JSON string; make it robust by checking the type of view.proposed_updates["cSkillAttrs"] after modal.on_submit (in the test) and parsing only if it is a string (e.g., call json.loads when isinstance(..., str), otherwise use it directly as a dict) so the assertions on parsed_attrs work whether ResumeEditSkillsModal.on_submit or ResumeUpdateConfirmationView stores cSkillAttrs as a dict or a JSON string; update references to view.proposed_updates["cSkillAttrs"] and parsed_attrs accordingly and keep the rest of the assertions the same (also keep mock_interaction.response.send_message.assert_called_once()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 942-946: The modal's skills_input.default is being set from
_build_default_lines without guarding against skills_input.max_length (1000),
which can cause modal failures; update the code that sets
self.skills_input.default to join the default lines from _build_default_lines
and then enforce the max length by truncating the resulting string to
self.skills_input.max_length (optionally adding an ellipsis) or only assigning
when within the limit—refer to the symbols self.skills_input, max_length and
_build_default_lines to locate and change the assignment so the default never
exceeds the configured max_length.
- Around line 1305-1321: The function _parse_skill_strengths currently uses
skill = str(raw_skill).strip() and then strengths[skill.casefold()] which only
casefolds and can mismatch the normalized skill keys used elsewhere; change this
to normalize the skill the same way the rest of the codebase does before using
it as a map key (e.g., replace skill = str(raw_skill).strip() with a call to
your canonical normalizer like normalized = normalize_skill(str(raw_skill)) and
then store strengths[normalized] = strength), or if no helper exists, apply the
same normalization steps used elsewhere (trim, remove/normalize
punctuation/aliases, and lower/casefold) so the key format matches other skill
lookups.
---
Nitpick comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 3015-3024: The preview rendering for the "skills" branch uses the
incoming `label` from the change payload instead of the canonical label helper;
update this block to derive the display label via
`ResumeUpdateConfirmationView._field_label(field_name)` (instead of using
`label`) before calling `truncate_preview_value` and when formatting the `lines`
entry; keep the rest of the logic (`format_skill_delta`,
`truncate_preview_value`, `delta` check, and the `lines.append` behavior) the
same so aliases like LinkedIn are applied consistently.
- Around line 985-996: The current parsing block in crm.py (variables token,
raw_strength, parsed_strength, strength_value) silently ignores invalid
strengths like "python:abc" or out-of-range "python:8"; change it so that when
raw_strength is present but cannot be parsed to an integer or is not in 1..5 you
do not silently drop it but surface the problem: either raise/return a clear
ValueError with the offending token and raw_strength, or log/warn and collect
invalid entries to return to the caller so the user can be informed;
specifically update the code around the token/raw_strength parsing to detect
parse failures and out-of-range values and emit a clear error message (including
token and raw_strength) instead of leaving parsed_strength None and continuing.
In `@tests/unit/test_crm.py`:
- Around line 464-493: The test test_edit_skills_modal_submit_updates_proposed
assumes view.proposed_updates["cSkillAttrs"] is a JSON string; make it robust by
checking the type of view.proposed_updates["cSkillAttrs"] after modal.on_submit
(in the test) and parsing only if it is a string (e.g., call json.loads when
isinstance(..., str), otherwise use it directly as a dict) so the assertions on
parsed_attrs work whether ResumeEditSkillsModal.on_submit or
ResumeUpdateConfirmationView stores cSkillAttrs as a dict or a JSON string;
update references to view.proposed_updates["cSkillAttrs"] and parsed_attrs
accordingly and keep the rest of the assertions the same (also keep
mock_interaction.response.send_message.assert_called_once()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84eaf969-f143-4564-a9a4-e0564645f979
📒 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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py:1000
- In
_parse_skill_lines, the conditionif parsed_strength and 1 <= parsed_strength <= 5:is redundant and can be misleading (it relies on truthiness rather than the range check). Using only the explicit range check (after confirmingparsed_strength is not None) makes the validation clearer and avoids accidental edge cases if the allowed range ever changes.
try:
parsed_strength = int(float(raw_strength))
except (TypeError, ValueError):
parsed_strength = None
if parsed_strength and 1 <= parsed_strength <= 5:
strength_value = parsed_strength
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if existing_strength is None and strength is not None: | ||
| parsed[key] = (existing_name, strength) | ||
| continue | ||
| parsed[key] = (normalized, strength) |
There was a problem hiding this comment.
parse_skill_snapshot() normalizes skill names with normalize_skill() and then reuses that normalized value for display in the delta. Since normalize_skill() lowercases and can alter punctuation, the preview delta can show different names than the original current/proposed strings (e.g., C++ -> c++). Consider storing both a comparison key (normalized/casefolded) and a display name (original trimmed token / parsed base) so the delta preserves the original formatting while still matching skills robustly.
Suggested change
| parsed[key] = (normalized, strength) | |
| # Store the original (trimmed) name for display, using the normalized | |
| # form only as the comparison key. | |
| parsed[key] = (name, strength) |
Comment on lines
+976
to
+1012
| @staticmethod | ||
| def _parse_skill_lines(raw: str) -> tuple[list[str], dict[str, int]]: | ||
| skills: list[str] = [] | ||
| strengths: dict[str, int] = {} | ||
| seen: set[str] = set() | ||
|
|
||
| for raw_line in raw.splitlines(): | ||
| line = raw_line.strip() | ||
| if not line: | ||
| continue | ||
|
|
||
| token = line | ||
| strength_value: int | None = None | ||
| if ":" in line: | ||
| token, raw_strength = line.rsplit(":", 1) | ||
| token = token.strip() | ||
| raw_strength = raw_strength.strip() | ||
| if raw_strength: | ||
| try: | ||
| parsed_strength = int(float(raw_strength)) | ||
| except (TypeError, ValueError): | ||
| parsed_strength = None | ||
| if parsed_strength and 1 <= parsed_strength <= 5: | ||
| strength_value = parsed_strength | ||
|
|
||
| normalized_skill = normalize_skill(token) | ||
| if not normalized_skill: | ||
| continue | ||
| key = normalized_skill.casefold() | ||
| if key not in seen: | ||
| seen.add(key) | ||
| skills.append(normalized_skill) | ||
| if strength_value is not None: | ||
| strengths[key] = strength_value | ||
|
|
||
| return skills, strengths | ||
|
|
There was a problem hiding this comment.
ResumeEditSkillsModal._parse_skill_lines() reimplements the same skill:strength parsing logic that already exists in CRMCog._parse_skill_updates() (including normalization + strength validation). This duplication increases the chance the two paths drift (e.g., different invalid-entry handling or strength parsing rules). Consider reusing the existing parser (or extracting a shared helper in five08.skills) so skills parsing rules stay consistent across the cog and the modal.
Suggested change
| @staticmethod | |
| def _parse_skill_lines(raw: str) -> tuple[list[str], dict[str, int]]: | |
| skills: list[str] = [] | |
| strengths: dict[str, int] = {} | |
| seen: set[str] = set() | |
| for raw_line in raw.splitlines(): | |
| line = raw_line.strip() | |
| if not line: | |
| continue | |
| token = line | |
| strength_value: int | None = None | |
| if ":" in line: | |
| token, raw_strength = line.rsplit(":", 1) | |
| token = token.strip() | |
| raw_strength = raw_strength.strip() | |
| if raw_strength: | |
| try: | |
| parsed_strength = int(float(raw_strength)) | |
| except (TypeError, ValueError): | |
| parsed_strength = None | |
| if parsed_strength and 1 <= parsed_strength <= 5: | |
| strength_value = parsed_strength | |
| normalized_skill = normalize_skill(token) | |
| if not normalized_skill: | |
| continue | |
| key = normalized_skill.casefold() | |
| if key not in seen: | |
| seen.add(key) | |
| skills.append(normalized_skill) | |
| if strength_value is not None: | |
| strengths[key] = strength_value | |
| return skills, strengths | |
| def _parse_skill_lines(self, raw: str) -> tuple[list[str], dict[str, int]]: | |
| """ | |
| Parse skills and strengths from the textarea contents by delegating to | |
| the shared CRM parsing logic, so normalization and validation remain | |
| consistent across the cog and the modal. | |
| """ | |
| return self.confirmation_view.crm_cog._parse_skill_updates(raw) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/test_crm.py (2)
464-493: MakecSkillAttrsassertion resilient to representation changes.The test currently assumes
cSkillAttrsis always a JSON string. If internals switch to storing a dict inproposed_updates, this test fails despite correct behavior.Suggested assertion hardening
- parsed_attrs = json.loads(view.proposed_updates["cSkillAttrs"]) + raw_attrs = view.proposed_updates["cSkillAttrs"] + parsed_attrs = json.loads(raw_attrs) if isinstance(raw_attrs, str) else raw_attrs assert parsed_attrs == { "python": {"strength": 5}, "rust": {"strength": 4}, }🤖 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 464 - 493, The assertion for cSkillAttrs in test_edit_skills_modal_submit_updates_proposed assumes view.proposed_updates["cSkillAttrs"] is a JSON string; update the test to handle both representations by checking the type of view.proposed_updates["cSkillAttrs"] (e.g., if it's a str, json.loads it; if it's already a dict, use it directly) before comparing to the expected dict so that ResumeEditSkillsModal.on_submit and view.proposed_updates can store either form without breaking the test.
328-356: Add acSkillAttrs-only visibility test for the Skills button.Nice coverage for
skillspresence/absence. I’d also cover the case where onlycSkillAttrsis proposed, since that path can still represent skill edits.Suggested test addition
+ `@pytest.mark.asyncio` + async def test_resume_update_view_adds_skills_button_when_only_skill_attrs_proposed( + self, crm_cog + ): + """Edit Skills button should appear when only cSkillAttrs is proposed.""" + view = ResumeUpdateConfirmationView( + crm_cog=crm_cog, + requester_id=123, + contact_id="contact-1", + contact_name="Test User", + proposed_updates={"cSkillAttrs": {"python": {"strength": 4}}}, + ) + + assert any(isinstance(child, ResumeEditSkillsButton) for child in view.children)🤖 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 328 - 356, Add a test that ensures ResumeEditSkillsButton is shown when ResumeUpdateConfirmationView receives a proposed_updates payload that only contains the cSkillAttrs key: create a new async test (similar to test_resume_update_view_adds_skills_button_when_skills_proposed) that constructs ResumeUpdateConfirmationView with proposed_updates={"cSkillAttrs": {...}} (use a minimal non-empty dict/structure) and assert any(isinstance(child, ResumeEditSkillsButton) for child in view.children); reuse the crm_cog fixture and mirror the existing test naming/style to keep coverage consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 2876-2907: parse_skill_snapshot currently stringifies all inputs
which mis-parses list/set/tuple inputs; update parse_skill_snapshot to detect
iterable inputs (list/tuple/set) and treat them as a sequence of skill entries
rather than calling str(value). Specifically: if value is None/empty return {};
if isinstance(value, (list, tuple, set)) build tokens from the iterable (strip
items and skip empties) instead of splitting the string, and otherwise continue
to handle string input by splitting on commas; keep the same downstream parsing
logic (regex match for "name(number)", normalize_skill, key collision handling)
so existing behavior (function parse_skill_snapshot, usage of normalize_skill,
token handling and parsed dict keys) remains intact.
- Around line 976-996: The on_submit flow currently treats malformed-only skills
as empty because _parse_skill_lines discards the _invalid result from
_parse_skill_updates; update _parse_skill_lines to return or surface invalid
entries (or call _parse_skill_updates directly in on_submit) and in on_submit
check for any invalid entries before clearing proposed_updates: if invalid
entries exist, do not pop "skills" or "cSkillAttrs", instead return an
error/validation message to the user describing the malformed lines (using
skills_input.value and the invalid list from _parse_skill_updates). Reference:
_parse_skill_lines, _parse_skill_updates, on_submit, skills_input, and
confirmation_view.proposed_updates ("skills"/"cSkillAttrs").
---
Nitpick comments:
In `@tests/unit/test_crm.py`:
- Around line 464-493: The assertion for cSkillAttrs in
test_edit_skills_modal_submit_updates_proposed assumes
view.proposed_updates["cSkillAttrs"] is a JSON string; update the test to handle
both representations by checking the type of
view.proposed_updates["cSkillAttrs"] (e.g., if it's a str, json.loads it; if
it's already a dict, use it directly) before comparing to the expected dict so
that ResumeEditSkillsModal.on_submit and view.proposed_updates can store either
form without breaking the test.
- Around line 328-356: Add a test that ensures ResumeEditSkillsButton is shown
when ResumeUpdateConfirmationView receives a proposed_updates payload that only
contains the cSkillAttrs key: create a new async test (similar to
test_resume_update_view_adds_skills_button_when_skills_proposed) that constructs
ResumeUpdateConfirmationView with proposed_updates={"cSkillAttrs": {...}} (use a
minimal non-empty dict/structure) and assert any(isinstance(child,
ResumeEditSkillsButton) for child in view.children); reuse the crm_cog fixture
and mirror the existing test naming/style to keep coverage consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab875b97-94ed-419a-a3c2-780850c77888
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm.pytests/unit/test_crm.py
Comment on lines
+976
to
+996
| def _parse_skill_lines(self, raw: str) -> tuple[list[str], dict[str, int]]: | ||
| line_tokens = [line.strip() for line in raw.splitlines() if line.strip()] | ||
| flattened = ", ".join(line_tokens) | ||
| parsed_skills, requested_strengths, _invalid = ( | ||
| self.confirmation_view.crm_cog._parse_skill_updates(flattened) | ||
| ) | ||
| return parsed_skills, requested_strengths | ||
|
|
||
| async def on_submit(self, interaction: discord.Interaction) -> None: | ||
| raw = self.skills_input.value or "" | ||
| requested_skills, requested_strengths = self._parse_skill_lines(raw) | ||
|
|
||
| if not requested_skills and not requested_strengths: | ||
| self.confirmation_view.proposed_updates.pop("skills", None) | ||
| self.confirmation_view.proposed_updates.pop("cSkillAttrs", None) | ||
| await interaction.response.send_message( | ||
| "✅ Skills updates cleared. Click **Confirm Updates** to apply.", | ||
| ephemeral=True, | ||
| ) | ||
| return | ||
|
|
There was a problem hiding this comment.
Invalid skill lines currently trigger unintended “clear all skills” behavior.
On Line 980, _parse_skill_updates returns invalid entries, but they’re discarded. Then on Line 988, malformed-only input is treated as empty and clears skills/cSkillAttrs. This can silently wipe pending skill edits.
💡 Suggested fix
- def _parse_skill_lines(self, raw: str) -> tuple[list[str], dict[str, int]]:
+ def _parse_skill_lines(
+ self, raw: str
+ ) -> tuple[list[str], dict[str, int], list[str]]:
line_tokens = [line.strip() for line in raw.splitlines() if line.strip()]
flattened = ", ".join(line_tokens)
- parsed_skills, requested_strengths, _invalid = (
+ parsed_skills, requested_strengths, invalid = (
self.confirmation_view.crm_cog._parse_skill_updates(flattened)
)
- return parsed_skills, requested_strengths
+ return parsed_skills, requested_strengths, invalid
async def on_submit(self, interaction: discord.Interaction) -> None:
raw = self.skills_input.value or ""
- requested_skills, requested_strengths = self._parse_skill_lines(raw)
+ requested_skills, requested_strengths, invalid_entries = (
+ self._parse_skill_lines(raw)
+ )
+
+ if invalid_entries:
+ invalid_preview = ", ".join(f"`{entry}`" for entry in invalid_entries[:8])
+ await interaction.response.send_message(
+ "❌ Invalid skill entries: "
+ f"{invalid_preview}. Use `skill` or `skill:1-5`.",
+ ephemeral=True,
+ )
+ return🤖 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 976 - 996,
The on_submit flow currently treats malformed-only skills as empty because
_parse_skill_lines discards the _invalid result from _parse_skill_updates;
update _parse_skill_lines to return or surface invalid entries (or call
_parse_skill_updates directly in on_submit) and in on_submit check for any
invalid entries before clearing proposed_updates: if invalid entries exist, do
not pop "skills" or "cSkillAttrs", instead return an error/validation message to
the user describing the malformed lines (using skills_input.value and the
invalid list from _parse_skill_updates). Reference: _parse_skill_lines,
_parse_skill_updates, on_submit, skills_input, and
confirmation_view.proposed_updates ("skills"/"cSkillAttrs").
Comment on lines
+2876
to
+2907
| def parse_skill_snapshot(value: Any) -> dict[str, tuple[str, int | None]]: | ||
| if value is None: | ||
| return {} | ||
| text = str(value).strip() | ||
| if not text or text.casefold() == "none": | ||
| return {} | ||
| tokens = [item.strip() for item in text.split(",") if item.strip()] | ||
| parsed: dict[str, tuple[str, int | None]] = {} | ||
| for token in tokens: | ||
| name = token | ||
| strength: int | None = None | ||
| match = re.match(r"^(.*)\((\d+)\)$", token) | ||
| if match: | ||
| name = match.group(1).strip() | ||
| try: | ||
| strength = int(match.group(2)) | ||
| except ValueError: | ||
| strength = None | ||
| name = name.strip() | ||
| if not name: | ||
| continue | ||
| normalized = normalize_skill(name) | ||
| if not normalized: | ||
| continue | ||
| key = normalized.casefold() | ||
| if key in parsed: | ||
| existing_name, existing_strength = parsed[key] | ||
| if existing_strength is None and strength is not None: | ||
| parsed[key] = (existing_name, strength) | ||
| continue | ||
| parsed[key] = (name, strength) | ||
| return parsed |
There was a problem hiding this comment.
parse_skill_snapshot should handle list/set values directly, not via str(value).
Line 2879 stringifies all inputs. If current/proposed arrives as a list, delta parsing can mis-tokenize and show incorrect adds/removals.
💡 Suggested fix
def parse_skill_snapshot(value: Any) -> dict[str, tuple[str, int | None]]:
if value is None:
return {}
- text = str(value).strip()
- if not text or text.casefold() == "none":
- return {}
- tokens = [item.strip() for item in text.split(",") if item.strip()]
+ tokens: list[str]
+ if isinstance(value, (list, tuple, set)):
+ tokens = [str(item).strip() for item in value if str(item).strip()]
+ else:
+ text = str(value).strip()
+ if not text or text.casefold() == "none":
+ return {}
+ tokens = [item.strip() for item in text.split(",") if item.strip()]
parsed: dict[str, tuple[str, int | None]] = {}🤖 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 2876 -
2907, parse_skill_snapshot currently stringifies all inputs which mis-parses
list/set/tuple inputs; update parse_skill_snapshot to detect iterable inputs
(list/tuple/set) and treat them as a sequence of skill entries rather than
calling str(value). Specifically: if value is None/empty return {}; if
isinstance(value, (list, tuple, set)) build tokens from the iterable (strip
items and skip empties) instead of splitting the string, and otherwise continue
to handle string input by splitting on commas; keep the same downstream parsing
logic (regex match for "name(number)", normalize_skill, key collision handling)
so existing behavior (function parse_skill_snapshot, usage of normalize_skill,
token handling and parsed dict keys) remains intact.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Related Issue
How Has This Been Tested?
Summary by CodeRabbit
New Features
Tests