Revised Home Assistant setup flow #1450
Conversation
removed unnecessary wiki_command_overrides
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if latitude == 0 and longitude == 0: | ||
| _LOGGER.warning("Home location not set in Home Assistant configuration") | ||
| else: | ||
| _LOGGER.debug("Fetching property info for coordinates: (%s, %s)", latitude, longitude) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
| if latitude == 0 and longitude == 0: | ||
| _LOGGER.warning("Home location not set in Home Assistant configuration") | ||
| else: | ||
| _LOGGER.debug("Fetching property info for coordinates: (%s, %s)", latitude, longitude) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
|
|
||
| # Make get a regular method that returns a response object, not a coroutine | ||
| def get(self, url, **kwargs): | ||
| if "maps.googleapis.com" in url: |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1450 +/- ##
==========================================
- Coverage 86.82% 79.30% -7.53%
==========================================
Files 9 13 +4
Lines 1131 1324 +193
==========================================
+ Hits 982 1050 +68
- Misses 149 274 +125 ☔ View full report in Codecov by Sentry. |
|
thanks @davida72 Couple of questions- 1/ what’s the upgrade/ reconfigure path for existing users? Any breaking changes here? 2/ Are we are increasing our dependency on 3rd party apis with this approach ? Is selecting the council from a drop down an issue that needs to be fixed? Flip side of this approach is people can’t select a council they are forced to choose the one their lat long dictates. I’ll do some more checking when I have some more time |
Nothing to upgrade or reconfigure. I was careful to be consistent.
Good question. If the API calls fail for any reason then we revert back to the previous system automatically. If it's available we'll use it. If it isn't we won't.
I should have made clear that it just defaults to the auto-detected council, but the user is free to choose another. My hope is that it makes it easy for a user to find their council, whatever it's called. |
I can't see how the user won't need to upgrade? Do all the fields apply the same names as before? |
Yes. What's loaded and saved is exactly the same. It's just the flow that's different. |
|
Here's a comparison of what's saved to JSON Data Comparison: Brighton Old vs Brighton New
Summary of Differences6 fields differ between the two objects:
19 fields are identical between both objects. JSON Data Comparison: Clackmannanshire Old vs Clackmannanshire New
Summary of Differences6 fields differ between the two objects:
16 fields are identical between both objects. Note: Unlike the Brighton comparison, these Clackmannanshire configurations use Google Calendar as the data source (GooglePublicCalendarCouncil parser) rather than web scraping, and don't include web driver or browser-related settings. JSON Data Comparison: Huntingdonshire Old vs Huntingdonshire New
Summary of Differences6 fields differ between the two objects:
16 fields are identical between both objects. Note: These configurations include a UPRN (Unique Property Reference Number) "10012048679" for property identification. JSON Data Comparison: Ipswich Old vs Ipswich New
Summary of Differences6 fields differ between the two objects:
15 fields are identical between both objects. Note: These configurations include custom icon color mappings for different bin types (Recycling=black, Rubbish=green, Garden Waste=brown) and use "Siloam Place" as the address identifier. |
|
Three other points:
The URL is stored in |
|
@davida72 I had a bit of time to test this, but I need more time to fully test it. Initial feedback (some opinionated sorry) 1/ The language translations are missing in custom_components/uk_bin_collection/translations. Ill do some more testing and can see you have put in some SERIOUS work on this so it is appreciated |
|
Appreciate you taking a look. I know you're busy. My motivation for the changes was that I found initial setup as a user too complicated. It asked for information it didn't need, the flow was confusing in places, and the documentation (in my case) was wrong. I felt that it undersold the huge body of work underneath it. So my efforts in re-working the flow were with those considerations in mind: asking only for the information it needs, putting dynamic instructions in the flow, pre-populating where possible.
The translation files could be created fairly easily, although in places I'm using text from input.json which is in English only. The existing setup uses hard-coded English strings in places, I think, e.g. Selenium and Chromium checks.
3/ I don’t want to create a dependency on a location lookup—selecting a council from a drop-down is easy enough The API calls get postcode, lad24cd, street name, town and admin ward, which can be used to create a degree of automation and avoid errors during the setup. It's not just used in the drop-down. To me this is a notable improvement, but I sense that you don't want to use an API. I'd query the word 'dependency' since the flow works fine if either of the APIs fail, but maybe there's just something about using a third-party service by default that you don't like. Would you feel more comfortable if it was a choice made by the user? e.g. a checkbox that asked Automatically detect my location. On the point of the drop-down, some councils use an official name that might be different from the one that a user expects, e.g. Hull City Council is officially called Kingston upon Hull.
My feeling is that if you understand what it's asking and why, then the form is fine. I found it overwhelming as a first-time-user and it was a frustrating experience trying to get it to work. It asked for a URL it didn't need; it told me to enter information that was incorrect; it didn't check my Selenium server. I agree that it's personal preference, but I prefer the step-by-step approach of getting one set of information correct before moving on to the next step; besides - it didn't all fit on my screen! I'm ambivalent about this though. Most of my frustrations could be resolved with better messaging and better checks.
Got it. |
|
@davida72 - just to validate - when I tried a Selenium based council the checks wernt there in the form - sorry to confirm but were these removed? |
|
I see what you mean. You're asking where they are in the UI, not in the code. If a known, working Selenium is found then it will pre-populate it in the input box. If a user tries to progress without one then it notifies them, as per the screenshot earlier in this thread. My motivation for this was my experience in setting this step up for the first time. ❌ Selenium not found at http://localhost:4444 This was all true, but unhelpful. I knew I didn't have Selenium on those URLs and I knew I didn't have Chromium. Selenium was set up at http://mini:4444 for me. I entered that in and continued, but there was no check to see if it worked. After a lot of trial and error I found out that on my Home Assistant machine mini wasn't resolving to its IP address. So in the revised flow, there's an additional check going out of the form to see if the user-entered Selenium URL works. |
2 participants
Overview
This PR changes the Home Assistant setup process, which I hope makes for a more intuitive onboarding process while maintaining full compatibility with existing configurations. The changes include more checks, pre-population of fields and clear error messages.
The setup is now a four step process:
Name and Council
Council Specific Information
wiki_notefrominput.jsonis used as a description, so no need to refer to the wiki.input.json.Selenium
Advanced
Configure
input.json, for troubleshooting purposes.Housekeeping
input.jsonto remove unnecessarywiki_command_url_override.input.jsonthat will always be from the master version.Notes
wiki_command_url_overridein the council data as the method of establishing whether the user needs to enter a URL, but perhaps this isn't the best method.original_parser, but I'll leave that for another occasion.input.jsonwould benefit from some more work.Council is auto-detected in Step 1, using auto-detected street name as default name.
Or reverts to a fallback if no lat/lon or no council is found.
In Step 2, only necessary information is asked from the user.
It will pre-populate the auto-detected postcode.
It will offer up a placeholder URL taken from
input.jsonif a URL is required.But it will check that it's been modified before moving on.
Step 3 are the Selenium checks, if the council requires it. It pre-populates with the auto-detected Selenium server.
It won't let the user progress without a working Selenium server.
Or a working Chromium.
Step 4 is Advanced Settings.
Doing checks on timeout value.
And update frequency value.
On Configure, the user has the option of using test settings taken from
input.json