BJReplay / ha-solcast-solar

Solcast Integration for Home Assistant
Apache License 2.0
183 stars 34 forks source link

Configuration option names missing after HA upgrade to 2024.9.0 #161

Closed autoSteve closed 2 months ago

autoSteve commented 2 months ago

This is being investigated.

image
BJReplay commented 2 months ago

So, you know how I like to risk breaking things by testing...

It's not 4.1.5...

Note the 4.1.3 in the top left of the screenshot.

Yep, I downgraded to test.

image

It's either HA

image

Or HACS

image

I'm guessing that there's been a recent change, and it now obeys the label=

About to look for change notes on the front end, as well as issues, as I imagine this has borked every integration

CONFIG_OPTIONS = [
    selector.SelectOptionDict(value="configure_api", label="option1"),
    selector.SelectOptionDict(value="configure_dampening", label="option2"),
    selector.SelectOptionDict(value="configure_customsensor", label="option3"),
    selector.SelectOptionDict(value="configure_attributes", label="option4"),
]
autoSteve commented 2 months ago

Just worked out the same thing.

I spun up an older Home Assistant instance (which also featured the older HACS) and upgrading to v4.1.5 is fine.

autoSteve commented 2 months ago

Googling SelectOptionDict reveals a whole bunch of not much.

BJReplay commented 2 months ago

Detailed reading through the release notes of 2024.9.0 and 2.0.1 did not spot any obvious items.

Not one other of my integrations uses that approach, so I couldn't repro with them.

I guess you could raise an issue saying that the Select Option now displays the label when it used to display the value, but that's sort of what you'd expect it to do!

autoSteve commented 2 months ago

I am puckering for an evening of trawling 2024.9.0 commits.

(Instead of investigating further the impact of adding a 30/60 minute breakdown by site... Which is not a terrible suggestion at face value, but an early look reveals it would become a non-trivial implementation. We'd need more attribute config, with default of 'off', which means translations, etc, etc. Because one user. Yuck.)

I did not notice this options thing after the upgrade to HACS 2.0.1, and can't think how that could be implicated anyway. A manual install without HACS would behave the same way. So it's gotta be HA. Or our implementation approach.

We probably need to change the approach.

BJReplay commented 2 months ago

Yeah, I'm assuming it's buried as a trivial commit somewhere to fix something that was using value that should have been using label and the labels were not displaying correctly, and it has fixed that.

SelectOptionDict does show up in a change for Anthropic, so that's the crumb that I'm going to follow to see if there's an associated change somewhere else.

https://github.com/home-assistant/core/commit/36ec1b33fe0039d838586730768730c2ea4e054c#diff-14393a47f72aaf242cfb5f4c18251530472d5934d0587d1bda8b797f3edadb1aR167

BJReplay commented 2 months ago

Can't find anything in core except usage using it as you would expect from the names of the elements 🤣

gcoan commented 2 months ago

Thanks guys, this helps reinforce my strategy of not upgrading to the .0 release when it comes out and waiting until at least .2 to get the bugs ironed out

Somebody has to trail-blaze 😎

One annoying feature with HACS 2.0 is its classification and the filtering. If something that I've downloaded then has an upgrade for it, it disappears from the 'downloaded' filter list and only appears on the 'pending update' list. I'd want it to still be on the downloaded list as it's still downloaded, just not on the latest version. Maybe I will feel strongly enough to bug this. Or maybe I will just wait for others to...

BJReplay commented 2 months ago

If something that I've downloaded then has an upgrade for it, it disappears from the 'downloaded' filter list and only appears on the 'pending update' list.

You can change the filters and defaults for the display to suit your preferences.

BJReplay commented 2 months ago

One annoying feature with HACS 2.0

This is not a HACS bug, it is a home assistant change that has caught us out.

autoSteve commented 2 months ago

And after quite some time investigating I have come up with precisely zip.

The first level config dialogue is borked. Second level seems okay, getting strings/translations just fine.

In our Python code, if I collapse all second level options into one single first level (which would be my preference given less clicking), results in all the options that were second level now becoming borked because first level.

It is like the strings.json / translations are not being referenced or found. And yet, when I look at other integrations that do exactly the same thing they are fine.

This is not how Friday evenings should be ordered. I think I need to step away from this for a while and watch another ep. of Squid Game for the fourth time...

BJReplay commented 2 months ago

Step away!

My only suggestion would be to repeat the value= for the label=, and see what happens.

e.g.

`selector.SelectOptionDict(value="configure_api", label="configure_api"), `
autoSteve commented 2 months ago

Custard. No strings/translate.

image
BJReplay commented 2 months ago

Oh, and finally, could it be https://github.com/BJReplay/ha-solcast-solar/blob/main/custom_components%2Fsolcast_solar%2Fconfig_flow.py#L128 (the translate option)?

BJReplay commented 2 months ago

Oh, and finally, could it be https://github.com/BJReplay/ha-solcast-solar/blob/main/custom_components%2Fsolcast_solar%2Fconfig_flow.py#L128 (the translate option)?

No. I deleted that line.

No change.

BJReplay commented 2 months ago

Take a break.

New installs will still be prompted for API.

Existing installs will survive.

BJReplay commented 2 months ago

No strings/translate.

I will look for hints about this, but we know that it doesn't work for 4.1.3, so it isn't the refactoring that has caused it - it is a latent issue that has surfaced under 2024.9.

Take a break.

BJReplay commented 2 months ago

Take a break.

New installs will still be prompted for API.

Existing installs will survive.

That said, is this a clue:

In config_flow.py

from .const import DOMAIN, TITLE, CONFIG_OPTIONS, API_QUOTA, CUSTOM_HOUR_SENSOR, BRK_ESTIMATE, BRK_ESTIMATE10, BRK_ESTIMATE90, BRK_SITE, BRK_HALFHOURLY, BRK_HOURLY

From __init__.py

from .const import (
    API_QUOTA,
    BRK_ESTIMATE,
    BRK_ESTIMATE10,
    BRK_ESTIMATE90,
    BRK_SITE,
    BRK_HALFHOURLY,
    BRK_HOURLY,
    CUSTOM_HOUR_SENSOR,
    DOMAIN,
    KEY_ESTIMATE,
    SERVICE_CLEAR_DATA,
    SERVICE_UPDATE,
    SERVICE_QUERY_FORECAST_DATA,
    SERVICE_SET_DAMPENING,
    SERVICE_SET_HARD_LIMIT,
    SERVICE_REMOVE_HARD_LIMIT,
    SOLCAST_URL,
)

Could that mean config options aren't imported?

BJReplay commented 2 months ago

Could that mean config options aren't imported?

No.

Me, clutching at straws, edits files, finds no change.

BJReplay commented 2 months ago

https://community.home-assistant.io/t/config-flow-strings-json-not-working/504312

autoSteve commented 2 months ago

I have made progress. I did not watch Squid Game. I am disappointed in myself that I did not watch Squid Game because Friday night.

This is a collapse of most options to a single configuration dialogue.

It works.

It is not without its idiosyncrasies, noting the title for the X hour sensor. But it is a step in the right direction.

Yet to include the dampening, but honestly, no one manually configures dampening in a dialogue.

Thoughts?

image
autoSteve commented 2 months ago

https://community.home-assistant.io/t/config-flow-strings-json-not-working/504312

First thing I checked. There would have been a metric sh*t tonne going wrong if this were it.

autoSteve commented 2 months ago

(@gcoan would be well pleased if implemented as proposed, with some tweaking. It gets rid of the slider for the X hour sensor, while still doing number validation 1<->144...)

BJReplay commented 2 months ago

https://developers.home-assistant.io/docs/internationalization/core/

Edited 25th Aug.

Mentions selector strings belong in a different hierarchy.

BJReplay commented 2 months ago

There would have been a metric sh*t tonne going wrong if this were it

Fair enough.

BJReplay commented 2 months ago

but honestly, no one manually configures dampening in a dialogue

I do.

We're all individuals

I'm not.

autoSteve commented 2 months ago

Mentions selector strings belong in a different hierarchy.

It's what do.

"selector": {
    "solcast_config_action": {
        "options": {
            "configure_api": "Solcast account details",
            "configure_dampening": "Configure dampening",
            "configure_customsensor": "Configure custom hours sensor",
            "configure_attributes": "Configure available attributes"
        }
    }
BJReplay commented 2 months ago

Thoughts?

Yeah, run with it, but it is fully weird.

autoSteve commented 2 months ago

Fully sick, bro.

I mean, being fully sick, bro.

I mean, fully being sick in a messed up integration drank too much on a Saturday night and passed out in the dunnies way, and can't see its translations let alone its feet, bro... 🤮

I currently wrangle collapsible config sections in a single configuration dialogue, which is not getting strings/translations yet. It needs work.

Fun times. Fun times...

That's for the morning with fresh eyes. Binge Squid Game repeat for now...

image
gcoan commented 2 months ago

Squid games original in Korean or the competition (mainly American participants)?

I didn't enjoy the latter version as much as the original, didn't have the same element of danger and I knew what the puzzles were going to be. Fun trivia fact, the turning doll game (first elimination round) was filmed inside Cardington Hangar which is about 10 miles away from me. Originally an airship hangar for the R101 and R100 it's now a film set.

New dialog looks more compact

BJReplay commented 2 months ago

So, the final point that I got to tonight was that translations are definitely picked up (tested by editing translations/en.json for that dialog - e.g. the trailing 4 in the screenshot below), but not for the labels, and my only theory is that perhaps they are hidden from the translation process now because of the way they are defined in const.py.

(Oh, and my point is that they used to be picked up, but now are not, which is the breaking change in 2024.9 HA).

image

autoSteve commented 2 months ago

Squid games original in Korean

Korean for the win.

autoSteve commented 2 months ago

(Kills entire branch of coding, throws hands in air and heads to bed... Lord knows what I just threw away, but good riddance. It got weird... Back to v4.1.5.)

BJReplay commented 2 months ago

(Kills entire branch of coding, throws hands in air and heads to bed... Lord knows what I just threw away, but good riddance. It got weird... Back to v4.1.5.)

I've been throwing mostly json at a wall, seeing what will stick, and the answer is not much.

One thought was that both the translation key and the name of the selection was called the same thing (solcast_config_action) so I tried renaming one, but as I say, throwing things at a wall.

I can find very few examples of the SelectSelectorConfig being used in config flows - the best (worst) example is a meross integration that has the options as constants and the labels as long English strings that have no translations, all hard coded in the config_flow.py

I think that raising an issue on the frontend wouldn't be unreasonable but wouldn't solve the problem.

TBH, your single form and dampening only via API is probably good enough, but my only other thought would be to add a checkbox / toggle to configure dampening, and if selected, save the rest of the config, and show the existing dampening config form?

autoSteve commented 2 months ago

First cut is at autoSteve on the Simplify-configure branch, @BJReplay.

Much clean up required. Translations required.

image
BJReplay commented 2 months ago

First cut is at autoSteve on the Simplify-configure branch, @BJReplay.

Looks good @autoSteve.

I have it running, and my update script updated to grab it to retest as and when.

Let me know if you want me to do mechanical moving of translations once you've got the first one done.

autoSteve commented 2 months ago

Second cut, clean-up done and committed, translations to go (some have changed description in en, so will need re-translation). Further validation has also been added (like checking that API limit values are >1 numeric.

I actually greatly prefer what we've done compared to how it was. The modify dampening is a bit awkward, and is my only gripe.

BJReplay commented 2 months ago

Probably should go up as a Full Release given the impact of this change for anyone needing to reconfigure (though, to be fair, current configure UI is like almost any modern web app nowadays, UI is discovery by clicking 🤣)

autoSteve commented 2 months ago

Translations pushed.

I'll also push a manifest and readme, then PR it. I think we've sweated 4.1.4 (180 dl)/4.1.5 (129 dl) enough to justify a 4.1.6 latest.

Do bash my input validations before we do.

BJReplay commented 2 months ago

Do bash my input validations before we do.

On it.

BJReplay commented 2 months ago

By the way, the reason I have manual dampening factors is that my Powerwall cables run in the same conduit as my solar cables. When the Powerwall is (was) charging overnight, it appears as if the solar panels are consuming power. By setting dampening to zero for overnight, I don't have my panels illuminating the moon.

BJReplay commented 2 months ago

Do bash my input validations before we do.

Could not break (other than, as mentioned in the comment on the release PR) I didn't test the API key field).

Please to see that I couldn't set 50.1 or "A" as the quota.