chore: resolve code quality findings - #31
Conversation
CodeQL's quality suite flagged src/door_sync/civicrm/client.py:34 as an
unused module variable. _DEFAULT_ACTIVE_STATUSES was defined but never
referenced: the client reads self._config.active_statuses, and the
config default ("Current","Grace","New") is defined independently in the
config loader. Removing the stale duplicate eliminates a second,
drift-prone "source of truth" for the default.
No behavior change; pyrefly/ruff/pytest green (343 passed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
Code Coverage OverviewLanguages: Python Python / code-coverage/pytestThe overall coverage remains at 93%, unchanged from the Updated |
There was a problem hiding this comment.
Pull request overview
Removes a stale, unused module-level constant in the CiviCRM client to address a GitHub Code Quality finding and reduce configuration drift risk (the active membership statuses default is already owned by the config loader).
Changes:
- Removed unused
_DEFAULT_ACTIVE_STATUSESconstant fromsrc/door_sync/civicrm/client.py.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test_alert.py:
- Mailgun tests now assert resp.raise_for_status() is called (the HTTP
status is checked, so a 4xx/5xx surfaces as a failure). Dropped the
no-op lambda override that had blocked the assertion.
- Dropped redundant manual __enter__/__exit__ on the SMTP server mock:
mock_cls.return_value is a MagicMock that already supports the context
manager (and __exit__ returns False, so it won't swallow exceptions);
_send_smtp uses `with server:` with no `as`.
test_unifi_client.py:
- _patched_tls uses real ssl constants (CERT_NONE / PROTOCOL_TLS_CLIENT /
TLSVersion) instead of magic 0s (PROTOCOL_TLS_CLIENT is 2, not 0).
- Assert the FC-mismatch message with a plain "expected 42" string, not
f"expected {42}".
- Moved local imports (replace, config.load) to module top.
- Added an explicit non-ASCII regular-digit case (Bengali ১) to the
_parse_sync_alias rejection test, verifying the isascii() guard.
343 passed; pyrefly 0 errors; ruff clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolves GitHub Code Quality findings (CodeQL quality suite + AI findings).
CodeQL quality finding
src/door_sync/civicrm/client.py:34—_DEFAULT_ACTIVE_STATUSESflagged as an unused module variable. Verified true positive (defined once, referenced nowhere; the client usesself._config.active_statusesand the default lives in the config loader). Removed.AI quality findings —
tests/test_alert.pyresp.raise_for_status()is called, so the HTTP status is actually checked (a 4xx/5xx surfaces as a failure, not a silent success). Removed the no-oplambdathat had blocked the assertion.__enter__/__exit__—mock_cls.return_valueis already aMagicMocksupporting the context-manager protocol (and__exit__returnsFalse, so it won't swallow exceptions);_send_smtpuseswith server:(noas).AI quality findings —
tests/test_unifi_client.py_patched_tls: use realsslconstants (CERT_NONE/PROTOCOL_TLS_CLIENT/TLSVersion) instead of magic0s (PROTOCOL_TLS_CLIENTis actually2)."expected 42"instead off"expected {42}".replace,config.load) to module top.১) to the_parse_sync_aliasrejection test, verifying theisascii()guard (not just the superscript case).All findings were verified against the code before applying; two of the AI suggestions were adjusted where their literal form was wrong (the
raise_for_statusassert needed the lambda removed first; "just use MagicMock" for the SMTP CM would have needed the redundant lines removed, not replaced).Verification
uv run pytest→ 343 passeduv run pyrefly check→ 0 errorsuv run ruff check ./ruff format --check .→ clean🤖 Generated with Claude Code