-
-
Notifications
You must be signed in to change notification settings - Fork 36.3k
Set account number as required for Anglian Water config entry #157939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set account number as required for Anglian Water config entry #157939
Conversation
| vol.Required(CONF_PASSWORD): selector.TextSelector( | ||
| selector.TextSelectorConfig(type=selector.TextSelectorType.PASSWORD) | ||
| ), | ||
| vol.Required(CONF_ACCOUNT_NUMBER): selector.TextSelector(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we get the account number from the API and set it as default value? Or would that be counter productive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would require an extra step in the config flow as we can only get that information following a successful auth (following KISS - https://en.wikipedia.org/wiki/KISS_principle).
Perhaps for a follow up PR I could look at that what was originally proposed in the initial PR to add a select field with valid accounts for a release next year (it feels odd to say that)? More work needs to be done on the library to support that first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - the extra step makes sense. What I don't understand yet is why inputting the number solves the problem, does the API currently select the first "working" account automatically? Or what exactly is going wrong here? The other thing is: How do we get users for which this is currently broken back to a working state, is there a way to guide them without deleting the entry first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inputting the number solves the problem as currently the account number is collected from the claims in the authentication token, not the actual API itself. It seems Anglian Water don't keep their Azure B2C users up to date with whats on their internal SAP database and therefore when you move house (or the account owner changes), you are issued with a new account number, yet retain access to your old and inactive account number which is what is still stored in the B2C user profile.
Apologies if this doesn't make sense, I don't really know how best to explain this as it does get quite complicated the more you dig.
Users will need to delete the entry, there isn't any possible way to migrate them to a working state because we won't be able to know what their account numbers are and what is "correct".
Related discussion: #156225 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its worth noting I only have access to my own account which is effectively the golden path, there are no public API docs apart from what I've figured out from de-obfuscating frontend javascript & watching network requests in the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking if we can detect a broken entry and offer the user with a repair where they have to enter the missing account info - tbf I'm not even sure it would be worth the effort. I agree though that a select would be a bit nicer, but that's too much effort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yeah, I see what you mean now. I think it still might not quite work how we think because the account is always populated anyway in the config entry data.
A select would be a "nice to have", but I think that is currently too much effort for the immediate problem, especially as despite adding functionality to retrieve associated accounts (pantherale0/pyanglianwater#37) to the library, it still needs to validate each account which requires refactoring the library to support multiple accounts in one initialization (if that makes sense), I've already started this, but the code isn't published yet because it currently breaks too many things.
d77e9e2 to
9fbcf84
Compare
zweckj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok let's do it then, I'd like the select in a follow up though
Proposed change
This will be part 1 of this bugfix, another is to follow up.
During the initial PR we discussed how the account number field should be configured, in the end we decided to only show it if the initial smart meter validation fails.
Unfortunately the data from the API is not 100% reliable as mentioned in #157902 and despite our validations progressing without issue, it is clear an account still may not be active.
This PR introduces the account number as a mandatory field for setup, a version bump to the config flow is not required as we always stored the account number anyway.
I've also added the pyanglianwater library to the loggers in the manifest.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: