Fix config flow to persist cpids data in HA config entries #1712
Conversation
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
nlindn
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
WalkthroughUpdates config flow logic in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CF as OCPP ConfigFlow
participant HA as hass.config_entries
U->>CF: Start CP user/measurands step
CF->>CF: Build cp_data / update _measurands
alt Auto-config enabled or measurands updated
CF->>HA: async_update_entry(entry, data=self._data)
HA-->>CF: Updated
CF-->>U: async_abort("Added/Updated charge point")
else No update condition
CF-->>U: async_abort("Added/Updated charge point")
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. ✨ Finishing Touches
🧪 Generate unit tests
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1712 +/- ##
=======================================
Coverage 94.25% 94.26%
=======================================
Files 12 12
Lines 2751 2755 +4
=======================================
+ Hits 2593 2597 +4
Misses 158 158 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/ocpp/config_flow.py (1)
193-213: Validate at least one measurand and update the correct CP; reload after update.
- Currently, selecting none still passes (empty set is a subset), resulting in an empty string measurands value. Validate non-empty.
- Avoid
[-1]; target the CP byself._cp_id.- Same reload concern as above.
if user_input is not None: selected_measurands = [m for m, value in user_input.items() if value] - if not set(selected_measurands).issubset(set(MEASURANDS)): + if not selected_measurands or not set(selected_measurands).issubset(set(MEASURANDS)): errors["base"] = "no_measurands_selected" return self.async_show_form( step_id="measurands", data_schema=STEP_USER_MEASURANDS_SCHEMA, errors=errors, ) else: self._measurands = ",".join(selected_measurands) - self._data[CONF_CPIDS][-1][self._cp_id][CONF_MONITORED_VARIABLES] = ( - self._measurands - ) - - self.hass.config_entries.async_update_entry( - self._entry, data=self._data - ) - return self.async_abort(reason="Added/Updated charge point") + # Update the targeted CP entry by id + for d in self._data.get(CONF_CPIDS, []): + if self._cp_id in d: + d[self._cp_id][CONF_MONITORED_VARIABLES] = self._measurands + break + # Option A: + return self.async_update_reload_and_abort( + entry=self._entry, + data=self._data, + reason="cp_added_or_updated", + ) + # Option B: + # self.hass.config_entries.async_update_entry(self._entry, data=self._data) + # await self.hass.config_entries.async_reload(self._entry.entry_id) + # return self.async_abort(reason="cp_added_or_updated")
🧹 Nitpick comments (1)
custom_components/ocpp/config_flow.py (1)
181-182: Use a translation key-style abort reason.Use a slug (e.g.,
cp_added_or_updated) and add it totranslations/*.jsonfor localization instead of a sentence with spaces and slash.- return self.async_abort(reason="Added/Updated charge point") + return self.async_abort(reason="cp_added_or_updated")Also applies to: 212-213
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/ocpp/config_flow.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: drc38
PR: lbbrhzn/ocpp#1498
File: tests/test_config_flow.py:124-124
Timestamp: 2025-01-28T09:21:24.150Z
Learning: The OCPP integration does not use options flow for configuration updates. Error handling in the config flow is primarily focused on measurand selection validation with the "no_measurands_selected" error.
🧬 Code graph analysis (1)
custom_components/ocpp/config_flow.py (1)
tests/test_config_flow.py (1)
test_successful_discovery_flow(74-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
Comment on lines
+170
to
+173
| cpids_list = self._data.get(CONF_CPIDS, []).copy() | ||
| cpids_list.append({self._cp_id: cp_data}) | ||
| self._data = {**self._data, CONF_CPIDS: cpids_list} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent duplicate CPIDs and fix CPID mismatch (discovery vs. user input).
Good switch to immutable update, but we always append and we key by self._cp_id while duplicate-checking uses user_input[CONF_CPID] (nested field not covered by _async_abort_entries_match). This can create duplicate CPIDs and ignores a potential mismatch between discovered cp_id and the value entered in the form.
- Update-or-insert the CPID (not always append).
- Optionally enforce that
user_input[CONF_CPID] == self._cp_idor ignore the form’s CPID to avoid drift.
Apply:
- cpids_list = self._data.get(CONF_CPIDS, []).copy()
- cpids_list.append({self._cp_id: cp_data})
- self._data = {**self._data, CONF_CPIDS: cpids_list}
+ cpids_list = list(self._data.get(CONF_CPIDS, [])) # shallow copy
+ # Update if CP already exists, else append
+ existing_idx = next((i for i, d in enumerate(cpids_list) if self._cp_id in d), -1)
+ if existing_idx >= 0:
+ cpids_list[existing_idx][self._cp_id] = cp_data
+ cp_idx = existing_idx
+ else:
+ cpids_list.append({self._cp_id: cp_data})
+ cp_idx = len(cpids_list) - 1
+ self._data = {**self._data, CONF_CPIDS: cpids_list}Optional (enforce discovery CPID):
- # Don't allow duplicate cpids to be used
- self._async_abort_entries_match({CONF_CPID: user_input[CONF_CPID]})
+ # Enforce the discovered CPID; reject mismatches early
+ if user_input[CONF_CPID] != self._cp_id:
+ return self.async_abort(reason="cp_id_mismatch")Also applies to: 162-169
🤖 Prompt for AI Agents
In custom_components/ocpp/config_flow.py around lines 162-173, the current code
always appends a new CPID entry keyed by self._cp_id and can create duplicates
or drift from the CPID supplied in user_input; change this to update-or-insert
the cpids list instead of always appending, by searching existing entries for a
matching CPID key and replacing its data if found, otherwise append; also
resolve the potential mismatch by either enforcing user_input[CONF_CPID] ==
self._cp_id (raise/abort if different) or ignore the form CPID and always use
self._cp_id as the canonical key/value before updating the list; apply the same
fix to the earlier block at lines 162-169.
Comment on lines
174
to
+182
| if user_input[CONF_MONITORED_VARIABLES_AUTOCONFIG]: | ||
| self._data[CONF_CPIDS][-1][self._cp_id][CONF_MONITORED_VARIABLES] = ( | ||
| DEFAULT_MONITORED_VARIABLES | ||
| ) | ||
| return self.async_update_reload_and_abort( | ||
| self._entry, | ||
| data_updates=self._data, | ||
| self.hass.config_entries.async_update_entry( | ||
| self._entry, data=self._data | ||
| ) | ||
| return self.async_abort(reason="Added/Updated charge point") | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Write to the correct CP entry and ensure runtime picks up changes (reload).
- When autoconfig is true, we currently update
[-1]. If we updated an existing CP above,[-1]may point to the wrong CP. Use the computedcp_idx. - After updating entry data, reload the entry or use the HA helper to ensure the running integration observes the new CP immediately.
- if user_input[CONF_MONITORED_VARIABLES_AUTOCONFIG]:
- self._data[CONF_CPIDS][-1][self._cp_id][CONF_MONITORED_VARIABLES] = (
- DEFAULT_MONITORED_VARIABLES
- )
- self.hass.config_entries.async_update_entry(
- self._entry, data=self._data
- )
- return self.async_abort(reason="Added/Updated charge point")
+ if user_input[CONF_MONITORED_VARIABLES_AUTOCONFIG]:
+ self._data[CONF_CPIDS][cp_idx][self._cp_id][CONF_MONITORED_VARIABLES] = (
+ DEFAULT_MONITORED_VARIABLES
+ )
+ # Option A (preferred if available):
+ return self.async_update_reload_and_abort(
+ entry=self._entry,
+ data=self._data,
+ reason="cp_added_or_updated",
+ )
+ # Option B (fallback):
+ # self.hass.config_entries.async_update_entry(self._entry, data=self._data)
+ # await self.hass.config_entries.async_reload(self._entry.entry_id)
+ # return self.async_abort(reason="cp_added_or_updated")To confirm whether async_update_reload_and_abort is available in your HA base and used elsewhere in the repo:
🏁 Script executed:
#!/bin/bash
rg -n "async_update_reload_and_abort|async_reload\(" -g '!**/venv/**' -C2Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search for async_update_reload_and_abort and async_reload usages in the codebase
rg -n "async_update_reload_and_abort" -C2 .
rg -n "async_reload\(" -C2 .Length of output: 2445
🏁 Script executed:
#!/bin/bash
# Search for cp_idx occurrences and context in config_flow.py
rg -n "cp_idx" -C3 custom_components/ocpp/config_flow.pyLength of output: 0
Compute correct CP index and reload entry after update (config_flow.py 174–182)
- Don’t use
[-1]; locate the CP’s position inself._data[CONF_CPIDS], e.g.:cp_list = self._data[CONF_CPIDS] cp_idx = next(i for i, cp in enumerate(cp_list) if self._cp_id in cp) self._data[CONF_CPIDS][cp_idx][self._cp_id][CONF_MONITORED_VARIABLES] = DEFAULT_MONITORED_VARIABLES
- After calling
self.hass.config_entries.async_update_entry(self._entry, data=self._data), invoke the existing reload helper to apply changes immediately:await async_reload_entry(self.hass, self._entry)
🤖 Prompt for AI Agents
In custom_components/ocpp/config_flow.py around lines 174 to 182, the code uses
[-1] to set monitored variables for a charge point which can target the wrong
CP; find the actual index of the CP in self._data[CONF_CPIDS] by locating the
dict that contains self._cp_id, set CONF_MONITORED_VARIABLES on that computed
index instead of using [-1], call
self.hass.config_entries.async_update_entry(self._entry, data=self._data) as
before, and then await the existing async_reload_entry helper for this entry to
apply the changes immediately.
drc38
approved these changes
Sep 8, 2025
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.
This PR fixes an issue in the configuration flow where newly added charge points were not being appended to the
"cpids"list incore.config_entries. As a result, when Home Assistant was restarted, previously added chargers were rediscovered, again as not added.This resolves the bug reported in #1698 and ensures that charge points are correctly tracked across restarts.
Changes:
cpidslist incore.config_entriesTesting:
core.config_entries.Summary by CodeRabbit
Refactor
Bug Fixes