Uh oh!
There was an error while loading. Please reload this page.
feat: extend update-contact fields - #174
Conversation
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExtended the Discord bot's CRM contact update functionality to support three additional fields: desired hours, website links, and location details. Added corresponding parsing helpers, validation logic, and embed rendering updates. Included comprehensive test coverage for the new functionality. 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
Extends the Discord /update-contact CRM command to support updating additional contact fields (location, desired hours, and website links) with input parsing/validation and richer embed output, plus unit tests covering the new behaviors.
Changes:
- Added new
/update-contactparameters:location,desired_hours, andwebsite, with validation and audit/UX messaging. - Implemented parsing helpers for desired hours, website links, and location/timezone extraction.
- Added unit tests covering location parsing and desired-hours validation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
apps/discord_bot/src/five08/discord_bot/cogs/crm.py | Adds new update-contact fields, parsing helpers, and embed output for the updated fields. |
tests/unit/test_crm.py | Adds async unit tests for location parsing and desired-hours validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| has_updates = any( | ||
| bool(value) for value in (github, linkedin, skills, rate_range) | ||
| bool(value) | ||
| for value in ( | ||
| github, | ||
| linkedin, | ||
| skills, | ||
| rate_range, | ||
| desired_hours, | ||
| website, | ||
| location, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
has_updates treats any non-empty string as an update (including whitespace-only inputs). This causes inputs like website=" "/location=" " to skip the "Provide at least one..." error and fall through to the later "No valid updatable fields" message. Consider checking value.strip() for the string parameters when computing has_updates so the initial validation matches the later parsing behavior.
| if not normalized and "://" not in candidate and "." in candidate: | ||
| normalized = normalize_website_url(f"https://{candidate}") |
There was a problem hiding this comment.
normalize_website_url() already supports scheme-less domains (it prepends https:// when needed). The extra fallback that manually prepends https:// is likely redundant and can be removed/simplified to keep link parsing logic in one place.
| ifnotnormalizedand"://"notincandidateand"."incandidate: | |
| normalized=normalize_website_url(f"https://{candidate}") |
| country_aliases = { | ||
| "us": "United States", | ||
| "usa": "United States", | ||
| "u.s.": "United States", | ||
| "u.s": "United States", | ||
| "united states": "United States", | ||
| "united states of america": "United States", |
There was a problem hiding this comment.
_parse_location_input rebuilds several large mapping literals (country_aliases, us_states, timezone maps, city hints, etc.) on every call. Even if this command is user-invoked, hoisting these to module/class constants (and precomputing derived sets like state_names) will reduce per-call overhead and make the function easier to scan/maintain.
| if is_country_token(token): | ||
| country = normalize_country_token(token) | ||
| else: | ||
| city = normalize_city(token) |
There was a problem hiding this comment.
In the single-segment case, state abbreviations/names and non-aliased countries won’t be recognized: e.g. location="CA" becomes addressCity="Ca" instead of addressState="California", and location="France" becomes a city. Since you already have is_state_token() and normalize_country_token(), consider using them in this branch (e.g., check state first, then country via normalize_country_token(), else fall back to city).
| ifis_country_token(token): | |
| country=normalize_country_token(token) | |
| else: | |
| city=normalize_city(token) | |
| ifis_state_token(token): | |
| state=normalize_state_token(token) | |
| else: | |
| normalized_country=normalize_country_token(token) | |
| ifnormalized_country: | |
| country=normalized_country | |
| else: | |
| city=normalize_city(token) |
| if is_country_token(country_token): | ||
| country = normalize_country_token(country_token) | ||
| else: | ||
| country = normalize_country_token(country_token) |
There was a problem hiding this comment.
This if/else is redundant: both branches assign country = normalize_country_token(country_token). It can be simplified to a single assignment (and keep the is_country_token check only if it’s used for different behavior).
| ifis_country_token(country_token): | |
| country=normalize_country_token(country_token) | |
| else: | |
| country=normalize_country_token(country_token) | |
| country=normalize_country_token(country_token) |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 7451-7467: The _parse_website_links function currently drops
malformed tokens; change it to return both the list of normalized, deduplicated
links and a list of invalid tokens (e.g., return Tuple[list[str], list[str]] or
similar) so callers like update_contact can reject mixed input; keep the
existing normalization logic (calls to normalize_website_url) and dedupe via
seen, but whenever a candidate cannot be normalized (after the https://
fallback) append it to an invalid_tokens list instead of skipping it silently,
and update callers to handle the returned invalid list and fail fast with the
exact bad entries.
- Around line 7674-7680: The one-segment branch currently sends any token not
matched by is_country_token() into normalize_city(), causing countries/states to
be misclassified; update the logic in the single-segment handling so it first
tests normalize_state_token(token) (or an is_state_token helper), then
normalize_country_token(token), and only then falls back to
normalize_city(token); adjust assignment targets to
addressState/addressCountry/addressCity respectively and reuse/extend existing
helpers (is_country_token, normalize_country_token, normalize_state_token,
normalize_city) to locate the change.
In `@tests/unit/test_crm.py`:
- Around line 2831-2865: The test
test_update_contact_success_updates_location_hours_website uses "New York, NY,
USA, UTC-05:00" which can mask a bad parser that copies city into addressState;
update the mocked location string passed to crm_cog.update_contact.callback to
use a clearly distinct city/state pair (e.g., "Austin, TX, USA, UTC-06:00") so
assertions on update_payload["addressCity"] vs update_payload["addressState"]
and update_payload["cTimezone"] verify proper normalization and mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 397927df-e88c-45f1-a13a-7b487a62e258
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm.pytests/unit/test_crm.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Description
Adds update-contact support for location, desired hours, and website updates with parsing/validation and embed output.
Adds unit tests covering location parsing and desired-hours validation.
Related Issue
N/A
How Has This Been Tested?
Not run (git hooks ran ruff, ruff-format, mypy).
Summary by CodeRabbit