fix: merge existing override properties when setting override#602
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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/test_managers.py (1)
51-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the POST body per call instead of cumulative
caplog.text.These checks can false-pass because
caplog.textis cumulative for the whole block, and the expected message at Lines 72-74 is identical to the one from Lines 62-64. More importantly, the regression here is about the/overridepayload, but this test only verifies debug output, not what was actually posted. Please clearcaplogbetween calls and, ideally, assert the captured POST payload for each invocation so the test exercises the bugfix directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_managers.py` around lines 51 - 104, The assertions currently inspect cumulative caplog.text which can false-pass; instead, after each mock_aioclient.post(...) + await test_charger.set_override(...) call, clear or reset caplog (e.g., caplog.clear()) and assert the actual POST payload sent to the mocked client for that specific call (inspect mock_aioclient requests/calls/last_request for the body) to verify the /override JSON contains the expected fields; replace the repeated checks against caplog.text with per-call caplog.clear() plus an assertion against the mock_aioclient request body for test_charger.set_override calls.
🧹 Nitpick comments (1)
openevsehttp/commands.py (1)
455-472: ⚡ Quick winValidate
assetsbefore iterating/logging it.Line 457 trusts GitHub’s nested
assetspayload aslist[dict]. If that field is a dict, string, or a list containing non-mappings, Line 462 and the fallback log on Line 471 will call.get(...)on invalid items and raise instead of cleanly returningNone. This path also misreports a missingbuildenvasNone.bin, which makes the new diagnostics misleading.Proposed hardening
download_url = None buildenv = self._config.get("buildenv") assets = message.get("assets", []) - _LOGGER.debug("Matching buildenv '%s' against assets", buildenv) - if buildenv and assets: + if not buildenv: + _LOGGER.debug("Cannot resolve firmware asset: missing buildenv in config.") + assets = [] + elif not isinstance(assets, list): + _LOGGER.debug("Invalid GitHub assets payload: %r", assets) + assets = [] + else: + _LOGGER.debug("Matching buildenv '%s' against assets", buildenv) target_filename = f"{buildenv}.bin" for asset in assets: - if asset.get("name") == target_filename: + if not isinstance(asset, Mapping): + continue + if asset.get("name") == target_filename: download_url = asset.get("browser_download_url") _LOGGER.debug("Found matching firmware asset: %s", download_url) break - if not download_url: + if buildenv and not download_url: _LOGGER.debug( "Could not find asset matching target filename '%s.bin' in assets: %s", buildenv, - [asset.get("name") for asset in assets] if assets else "None", + [asset.get("name") for asset in assets if isinstance(asset, Mapping)] + if assets + else "None", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openevsehttp/commands.py` around lines 455 - 472, The assets handling is not validating types and can crash when assets is not a list of dicts and also logs a misleading "None.bin" when buildenv is missing; update the block around download_url/buildenv/assets to first ensure assets is a list and filter it to only mappings (e.g., using isinstance checks) before iterating or calling asset.get, build target_filename only if buildenv is truthy (avoid f"{buildenv}.bin" when buildenv is None/empty), and change the fallback _LOGGER.debug to show a clear message when buildenv is missing and to list only valid asset names (from filtered mappings) so no .get is called on invalid items — adjust the code paths that reference download_url, target_filename, and asset.get accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_managers.py`:
- Around line 51-104: The assertions currently inspect cumulative caplog.text
which can false-pass; instead, after each mock_aioclient.post(...) + await
test_charger.set_override(...) call, clear or reset caplog (e.g.,
caplog.clear()) and assert the actual POST payload sent to the mocked client for
that specific call (inspect mock_aioclient requests/calls/last_request for the
body) to verify the /override JSON contains the expected fields; replace the
repeated checks against caplog.text with per-call caplog.clear() plus an
assertion against the mock_aioclient request body for test_charger.set_override
calls.
---
Nitpick comments:
In `@openevsehttp/commands.py`:
- Around line 455-472: The assets handling is not validating types and can crash
when assets is not a list of dicts and also logs a misleading "None.bin" when
buildenv is missing; update the block around download_url/buildenv/assets to
first ensure assets is a list and filter it to only mappings (e.g., using
isinstance checks) before iterating or calling asset.get, build target_filename
only if buildenv is truthy (avoid f"{buildenv}.bin" when buildenv is
None/empty), and change the fallback _LOGGER.debug to show a clear message when
buildenv is missing and to list only valid asset names (from filtered mappings)
so no .get is called on invalid items — adjust the code paths that reference
download_url, target_filename, and asset.get accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b35441a9-288c-441d-b540-fc5d39ef64b1
📒 Files selected for processing (3)
openevsehttp/commands.pytests/test_commands.pytests/test_managers.py
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Hermes Agent Code Review — PR #602Verdict: 🟢 LGTM (with minor suggestions) SummaryThis PR fixes firstof9/openevse#633 by preserving existing override state when re-applying overrides. The core logic is clean and well-tested. What Changed
Review Details✅ No Critical Issues
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_commands.py`:
- Around line 1262-1288: The pytest.raises match pattern in
test_update_firmware_assets_invalid_type uses unescaped dots, making the regex
too permissive; update the match string passed to pytest.raises in function
test_update_firmware_assets_invalid_type (and the similar occurrence at the
other test) to escape literal periods (e.g., use "\." for each dot) or build the
pattern via re.escape so it matches the exact message "Could not resolve latest
firmware download URL from GitHub." instead of treating "." as a wildcard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f6ac21e-8145-4c3d-9977-4c5731f76e7b
📒 Files selected for processing (3)
openevsehttp/commands.pytests/test_commands.pytests/test_managers.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_managers.py
- openevsehttp/commands.py
…t and empty firmware bytes validation
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Hermes Agent ReviewVerdict: Changes Requested (1 warning, 2 suggestions)
|
2 participants
This PR merges the existing manual override properties retrieved from
get_override()into the POST payload ofset_override()to prevent clearing previously set attributes.Fixes firstof9/openevse#633.
Summary by CodeRabbit
Bug Fixes
Improvements