Skip to content
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

User config flow and custom panel for Dynalite integration #77181

Merged
merged 13 commits into from May 10, 2023

Conversation

ziv1234
Copy link
Contributor

@ziv1234 ziv1234 commented Aug 22, 2022

Breaking change

The change deprecates the YAML configuration. it imports it into the UI and adds a repair issue, so it won't break a user configuration or change the existing behavior, but since it adds a "deprecation" warning, better to be explicit

Proposed change

The Dynalite configuration process is done today via YAML which we want to deprecate. However, the configuration is complex since it involved several items (Dynalite has no ability to query, so config has to be set manually).
This adds a user config flow to setup the basics (host / port). After this, a panel appears that can configure the various config entries in a straightforward interactive way

A few example screenshots:

Viewing all defined DYNET areas:
image

Configuring the system global settings:
image

Editing a specific DYNET area:
image

Editing a DYNET channel:
image

Defining a new DYNET preset:
image

Switching between different systems (multiple config entries)
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@ziv1234 ziv1234 marked this pull request as draft August 22, 2022 19:50
@ziv1234 ziv1234 changed the title DRAFT - User config flow and custom panel for Dynalite integration User config flow and custom panel for Dynalite integration Aug 22, 2022
@ziv1234 ziv1234 marked this pull request as ready for review August 23, 2022 10:29
@MartinHjelmare MartinHjelmare added this to By Code Owner in Dev Aug 27, 2022
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 22, 2022
@ziv1234
Copy link
Contributor Author

ziv1234 commented Sep 22, 2022

adding a comment so it does not auto close. nothing to add until it is reviewed :)

@ziv1234
Copy link
Contributor Author

ziv1234 commented Dec 4, 2022

had a merge conflict in manifest.json from another PR when it was approved. now all good

@github-actions
Copy link

github-actions bot commented Mar 4, 2023

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@ziv1234
Copy link
Contributor Author

ziv1234 commented Mar 5, 2023

hello. I rebased it again and fixed the conflicts. can someone please review this? it has been sitting for a very long time with no comments

Copy link
Contributor

@farmio farmio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did only see some minor points, but I'm not very familiar with websockets and panels yet 😬

homeassistant/components/dynalite/config_flow.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft March 7, 2023 20:47
Dev automation moved this from By Code Owner to Review in progress Mar 7, 2023
@home-assistant
Copy link

home-assistant bot commented Mar 7, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@farmio
Copy link
Contributor

farmio commented Mar 8, 2023

Oh, Is the yaml config not deprecated here? I guess it would make sense since when a user changes / reconfigures a bridge via the new panel, that is also configured via yaml it would not persist those changes, would it?

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 27, 2023

apologies. i have been out for a few weeks. will get on it quickly

@ziv1234 ziv1234 marked this pull request as ready for review April 30, 2023 10:19
@farmio
Copy link
Contributor

farmio commented May 1, 2023

Still not sure about this: #77181 (comment)
Is YAML still supported? Is it imported?

If imported and converted, I think it should be deprecated too, and a repair issue should be created for the user to remove stale yaml config then.

@ziv1234
Copy link
Contributor Author

ziv1234 commented May 7, 2023

Still not sure about this: #77181 (comment) Is YAML still supported? Is it imported?

If imported and converted, I think it should be deprecated too, and a repair issue should be created for the user to remove stale yaml config then.

the yaml config is still imported and converted. I did this, so it would not be a breaking change. do you have an example of a depracation / repair issue? i will check in knx as well

@farmio
Copy link
Contributor

farmio commented May 7, 2023

Knx doesn't import yaml.
Have a look at any of the recent release notes. You'll find a section "configurable from UI". There you can find some integrations that did such conversion and also a standard text for that in their PR description.

@ziv1234
Copy link
Contributor Author

ziv1234 commented May 7, 2023

Knx doesn't import yaml. Have a look at any of the recent release notes. You'll find a section "configurable from UI". There you can find some integrations that did such conversion and also a standard text for that in their PR description.

thanks. implemented per your suggestion from bratt...

Copy link
Contributor

@farmio farmio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor addition - not sure if this is required, but can't hurt.

homeassistant/components/dynalite/__init__.py Show resolved Hide resolved
Copy link
Contributor

@farmio farmio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

post was duplicated

@ziv1234 ziv1234 requested a review from farmio May 9, 2023 09:42
@ziv1234
Copy link
Contributor Author

ziv1234 commented May 9, 2023

implemented the last change

Copy link
Contributor

@farmio farmio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! 👍
I have tested it locally to see the panel in action (mocking the config entry since not having Dynalite equipment) and it seems to work as intended.

Dev automation moved this from Review in progress to Reviewer approved May 9, 2023
no real changes, but version number was messed up
@farmio farmio merged commit dd7db85 into home-assistant:dev May 10, 2023
52 checks passed
Dev automation moved this from Reviewer approved to Done May 10, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2023
@farmio farmio added the noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label May 31, 2023
@frenck frenck added the config-flow This integration migrates to the UI by adding a config flow label Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
by-code-owner cla-signed config-flow This integration migrates to the UI by adding a config flow has-tests integration: dynalite noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants