fix: guard config flow against empty account list#25
Merged
Conversation
If get_linked_accounts() returns [] (login succeeds but no accounts on the profile), the user_step and reconfigure_step now return an error instead of silently falling through to an empty selection screen where no_accounts_selected fires in an unbreakable loop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds the error string referenced by the empty-account guard added in the previous commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe config flow now aborts when authentication succeeds but the API returns no linked accounts; a new localized error string and tests cover the initial user and reconfigure flows. ChangesEmpty Account Validation and Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
custom_components/national_grid/config_flow.py (1)
59-64: 💤 Low valueConsider abort flow instead of error for better UX.
When
no_accounts_foundoccurs, the user cannot resolve it by re-entering credentials since authentication already succeeded. An abort with reasonno_accounts_foundmight provide clearer UX, indicating the user must add accounts through National Grid before configuring this integration.Current implementation is functional and the error message does clarify the situation, so this is optional.
Alternative approach using abort
else: if not self._accounts: _LOGGER.error( "Login succeeded but no accounts returned for %s", self._username, ) - _errors["base"] = "no_accounts_found" + return self.async_abort(reason="no_accounts_found") else:Note: This would also require adding
no_accounts_foundto theabortsection in strings.json rather thanerror.🤖 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 `@custom_components/national_grid/config_flow.py` around lines 59 - 64, Replace the current error path that sets _errors["base"]="no_accounts_found" when self._accounts is empty with an abort flow so the config flow stops with a clear reason; locate the block referencing self._accounts, _LOGGER and _username and instead call the flow abort helper (e.g., self.async_abort or self._abort_if_unique with reason "no_accounts_found") so the UI shows an abort page (remember to add "no_accounts_found" to the abort strings in strings.json) while keeping the existing log line for diagnostics.
🤖 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.
Nitpick comments:
In `@custom_components/national_grid/config_flow.py`:
- Around line 59-64: Replace the current error path that sets
_errors["base"]="no_accounts_found" when self._accounts is empty with an abort
flow so the config flow stops with a clear reason; locate the block referencing
self._accounts, _LOGGER and _username and instead call the flow abort helper
(e.g., self.async_abort or self._abort_if_unique with reason
"no_accounts_found") so the UI shows an abort page (remember to add
"no_accounts_found" to the abort strings in strings.json) while keeping the
existing log line for diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 715cb30e-6924-41d7-81c8-42719fd60893
📒 Files selected for processing (4)
custom_components/national_grid/config_flow.pycustom_components/national_grid/strings.jsoncustom_components/national_grid/translations/en.jsontests/test_config_flow.py
Showing a form error when login succeeds but the profile has no accounts implies the user can fix it by editing credentials. An abort page is semantically correct: the flow cannot proceed and there is nothing to retry. Moves the no_accounts_found key from config.error to config.abort in both strings files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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)
custom_components/national_grid/config_flow.py (1)
60-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid logging raw usernames in error paths.
Line 61 and Line 147 log the full username, which can leak PII (often email) into logs. Prefer a redacted value or remove the identifier from the message.
Proposed fix
- _LOGGER.error( - "Login succeeded but no accounts returned for %s", - self._username, - ) + _LOGGER.error( + "Login succeeded but no linked accounts were returned", + ) ... - _LOGGER.error( - "Reconfigure: login succeeded but no accounts returned for %s", - reconfigure_entry.data[CONF_USERNAME], - ) + _LOGGER.error( + "Reconfigure: login succeeded but no linked accounts were returned", + )Also applies to: 146-149
🤖 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 `@custom_components/national_grid/config_flow.py` around lines 60 - 63, The error logs currently call _LOGGER.error with self._username which can leak PII; update both occurrences (the _LOGGER.error call referencing self._username and the similar one around lines 146-149) to either remove the username from the message or log a redacted version (e.g., mask most characters or only show a non-PII suffix/prefix) before passing it to _LOGGER.error; locate the calls by searching for _LOGGER.error(...) that reference self._username in the config_flow.py methods handling login/results and replace them with a safe, redacted identifier or omit the identifier entirely.
🤖 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 `@custom_components/national_grid/config_flow.py`:
- Around line 60-63: The error logs currently call _LOGGER.error with
self._username which can leak PII; update both occurrences (the _LOGGER.error
call referencing self._username and the similar one around lines 146-149) to
either remove the username from the message or log a redacted version (e.g.,
mask most characters or only show a non-PII suffix/prefix) before passing it to
_LOGGER.error; locate the calls by searching for _LOGGER.error(...) that
reference self._username in the config_flow.py methods handling login/results
and replace them with a safe, redacted identifier or omit the identifier
entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 310b70fa-4027-4bdc-8585-a86c5e092c4c
📒 Files selected for processing (4)
custom_components/national_grid/config_flow.pycustom_components/national_grid/strings.jsoncustom_components/national_grid/translations/en.jsontests/test_config_flow.py
✅ Files skipped from review due to trivial changes (1)
- custom_components/national_grid/translations/en.json
Log messages recording "no accounts returned" passed the username (an email address) as a format argument, which would appear in HA log files and diagnostics exports. Drop the argument — the message is diagnostic without identifying the account holder. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
What
If
get_linked_accounts()returns an empty list (login succeeds but the profile has no billing accounts), the config flow previously fell through silently to an empty account-selection screen. The user would see an empty checklist, be unable to select anything, and hitno_accounts_selectedin an unbreakable loop with no path forward.This PR adds an explicit guard:
async_step_user: if accounts list is empty after successful auth, log an error and re-display the credentials form with ano_accounts_founderror messageasync_step_reconfigure: same guard for the reconfigure pathChanges
config_flow.py— empty-list check after_fetch_accounts()in both stepsstrings.json/translations/en.json— newno_accounts_founderror keytests/test_config_flow.py— two new tests covering both pathsTest plan
pytest tests/test_config_flow.pypassesruff check/ruff format --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit