Adjust reauth in awair config flow #72386
Conversation
|
Hey there @ahayworth, @danielsjf, mind taking a look at this pull request as it has been labeled with an integration ( |
| @@ -42,7 +45,9 @@ async def async_step_user(self, user_input: dict | None = None): | |||
| errors=errors, | |||
| ) | |||
|
|
|||
| async def async_step_reauth(self, user_input: dict | None = None): | |||
| async def async_step_reauth( | |||
| self, user_input: dict[str, str] | None = None | |||
There was a problem hiding this comment.
Maybe this is a bug, and should be split between async_step_reauth and async_step_reauth_confirm?
There was a problem hiding this comment.
I have adjusted this, but I think there might still be an underlying bug, since I think the user_input can never be None.
There was a problem hiding this comment.
@ahayworth / @danielsjf it seems to me that the old token is attempted again before asking the user for a new token.
Is this on purpose?
There was a problem hiding this comment.
In order to clarify the situation, I have introduced a new step async_step_reauth_confirm
Since we are passing the current data to the new method, there is no change in behavior.
There was a problem hiding this comment.
I will be honest and admit that I do not remember if there was an intentional "try the old token again" behavior. It's likely not what I intended. I found config flow rather confusing at the time I implemented it for Awair (I think it's gotten easier now!) ... so it's probably unintentional.
There was a problem hiding this comment.
This is turning into a bigger PR than originally planned, but I've reworked the config flow to avoid the "try the old token again" behavior. I've also split the tests accordingly and the strings/translations.
It would be great if you could try it and confirm it is working correctly.
There was a problem hiding this comment.
Hi @ahayworth, is there any chance you can test and/or approve the changes?
They are blocking #72830 and #72834
There was a problem hiding this comment.
Hi @epenet - respectfully, if you wish to propose a change it is best if you're able to test it yourself. You should be able to register an Awair dev token at their portal and test enough of the config flow yourself to know if this change works fine or not: https://developer.getawair.com/onboard/welcome
I haven't had a lot of time to devote to open source in general recently, and I don't have the time right now to figure out how to run a dev branch locally again and test it, unfortunately.
I believe the best past forward is for you to try and test your own changes here.
5d55d17 to
ee3c8ce
Compare
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
f30e8e9 to
1ee580a
Compare
|
Rebased to deal with our CI issues |
Proposed change
Needed for #72830 and #72834
Type of change
Additional information
Checklist
black --fast 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..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: