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

Add config flow to panasonic_viera component #33829

Merged
merged 47 commits into from
Apr 18, 2020
Merged

Add config flow to panasonic_viera component #33829

merged 47 commits into from
Apr 18, 2020

Conversation

joogps
Copy link
Contributor

@joogps joogps commented Apr 8, 2020

Breaking change

The only breaking change for existing users is that the YAML configuration isn't based on a platform anymore, so instead of this:

media_player:
  - platform: panasonic_viera
    host: 192.168.1.10

They'll need to use this:

panasonic_viera:
  host: 192.168.1.10

The latter supports multiple configurations. Also, after restarting Home Assistant, the user will be prompted to finish configuration and pairing through a config flow.

Proposed change

As an update to #33433, this pull request adds encryption support for the panasonic_viera component through YAML or the UI, via a config flow. I also rearranged the code, fixed a few stability issues and upgraded requirements.

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
panasonic_viera:
  host: 192.168.1.10
  name: Living Room TV

Additional information

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

@project-bot project-bot bot added this to Needs review in Dev Apr 8, 2020
@MartinHjelmare MartinHjelmare changed the title Updating the panasonic_viera component Update the panasonic_viera component Apr 8, 2020
@MartinHjelmare
Copy link
Member

MartinHjelmare commented Apr 8, 2020

The lint job fails now because access mode has changed on some files. Eg:
homeassistant/components/panasonic_viera/init.py 100644 → 100755

Please revert those changes.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please add tests for the config flow. It needs 100% coverage. The scaffold script should help you get started with those tests. More examples is eg the hue integration config flow tests.

homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Apr 8, 2020
@joogps
Copy link
Contributor Author

joogps commented Apr 9, 2020

So, I think I just finished to add the suggested changes and refactoring. However, I still need to create the tests. Any resources to help me on that?

@MartinHjelmare
Copy link
Member

The scaffold script should help you get started with those tests. More examples is eg the hue integration config flow tests.

@joogps
Copy link
Contributor Author

joogps commented Apr 10, 2020

I think I learned how to create and run tests. However, I couldn't manage to replace the RemoteControl class with a Mock class.

This is what I have right now:

def get_mock_remote(host="1.2.3.4", encrypted=False, app_id=None, encryption_key=None):
    """Return a mock remote."""
    mock_remote = Mock()
    mock_remote._host = host
    mock_remote._type = TV_TYPE_ENCRYPTED if encrypted else TV_TYPE_NONENCRYPTED
    mock_remote._app_id = app_id
    mock_remote._enc_key = encryption_key

    return mock_remote


async def test_flow_encrypted(hass):
    result = await hass.config_entries.flow.async_init(
        DOMAIN, context={"source": config_entries.SOURCE_USER}
    )

    assert result["type"] == "form" # passes
    assert result["step_id"] == "user" # passes

    mock_remote = get_mock_remote(encrypted=True)

    with patch("panasonic_viera.RemoteControl", return_value=mock_remote,), patch(
        "homeassistant.components.panasonic_viera.async_setup", return_value=True,
    ), patch(
        "homeassistant.components.panasonic_viera.async_setup_entry", return_value=True,
    ):
        result2 = await hass.config_entries.flow.async_configure(
            result["flow_id"], {CONF_HOST: "1.2.3.4", CONF_NAME: DEFAULT_NAME},
        )

    assert result2["type"] == "form"
    assert result2["step_id"] == "pairing" # returns AssertionError: assert 'user'
    assert result2["errors"] == {} # returns AssertionError: assert {'base': 'not_connected'}

(There are other tests, of course, this is only the first example)

@MartinHjelmare
Copy link
Member

The target for the patch needs to patch where the name is used not where it's defined. Ie: "homeassistant.components.panasonic_viera.config_flow.RemoteControl".

https://docs.python.org/3/library/unittest.mock.html#where-to-patch

@joogps
Copy link
Contributor Author

joogps commented Apr 11, 2020

I think everything's fine now. Working on the docs.

@joogps
Copy link
Contributor Author

joogps commented Apr 11, 2020

I made a pull request on the documentation repo: home-assistant/home-assistant.io#12843

@joogps
Copy link
Contributor Author

joogps commented Apr 12, 2020

Documentation approved! :D

homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/panasonic_viera/media_player.py Outdated Show resolved Hide resolved
tests/components/panasonic_viera/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/panasonic_viera/test_config_flow.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare added the waiting-for-upstream We're waiting for a change upstream label Apr 15, 2020
@joogps
Copy link
Contributor Author

joogps commented Apr 15, 2020

Oof... I guess everything is working now!

@MartinHjelmare MartinHjelmare removed the waiting-for-upstream We're waiting for a change upstream label Apr 15, 2020
@MartinHjelmare
Copy link
Member

This sounds weird:

Also, after restarting Home Assistant, the user will be prompted to finish configuration and pairing through a config flow

The user should not need to complete the flow on every restart if the device has already been set up.

@joogps
Copy link
Contributor Author

joogps commented Apr 15, 2020

This sounds weird:

Also, after restarting Home Assistant, the user will be prompted to finish configuration and pairing through a config flow

The user should not need to complete the flow on every restart if the device has already been set up.

Oh, it sounded very ambiguous. I was talking about the first restart after changing configuration.yaml

After that the config entry is saved and it sets itself up automatically (unless the user modifies the config for the entity)

@joogps
Copy link
Contributor Author

joogps commented Apr 16, 2020

I really don't know why Azure is reporting translation errors when the only file I changed was config_flow.py

@joogps
Copy link
Contributor Author

joogps commented Apr 17, 2020

ok, I screwed everything up

@joogps
Copy link
Contributor Author

joogps commented Apr 17, 2020

I think I was able to fix everything... Thankfully, I had made a copy of the integration before pushing, just to make sure I had a backup. However, I didn't do the same with the tests, but I was able to get a conflicted copy and remove the conflicts; at least I got a chance to review and re-organize some of the code... @MartinHjelmare

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

It might be possible to make the config flow more DRY by using some error handler helper like we did in the media player platform. But I think this can be a job for a future PR. It's fine like this. Just one comment.

homeassistant/components/panasonic_viera/config_flow.py Outdated Show resolved Hide resolved
@joogps
Copy link
Contributor Author

joogps commented Apr 18, 2020

Now the import step redirects to the user step. The tests were also changed since, now, connection errors in import lead to the initial form, and not to an abortion.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

Dev automation moved this from Review in progress to Reviewer approved Apr 18, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Just one more question?

homeassistant/components/panasonic_viera/manifest.json Outdated Show resolved Hide resolved
@joogps
Copy link
Contributor Author

joogps commented Apr 18, 2020

done!

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@joogps
Copy link
Contributor Author

joogps commented Apr 18, 2020

Oh, I also updated the docs to resolve conflicts that were being reported with the original branch (I don't know If you have writing permissions there)

@MartinHjelmare
Copy link
Member

We have a very good team for the docs that will review your PR there.

@joogps
Copy link
Contributor Author

joogps commented Apr 18, 2020

I'd like to thank you a lot, @MartinHjelmare for helping me with this. I must admit that at first I felt a bit intimated by all the changes that were asked, but it was worth it. As I have already said, this is the first time I ever contribute to the source code of a such a big project as Home Assistant (I'm 14 y/o btw, don't blame me), so it was very... exciting. Looking forward to contributing even more in the future!

thank you 😊

@joogps
Copy link
Contributor Author

joogps commented Apr 18, 2020

We have a very good team for the docs that will review your PR there.

oh, ok! They were very fast when I created the pull request so it really shouldn't take long

@MartinHjelmare MartinHjelmare changed the title Update the panasonic_viera component Add config flow to panasonic_viera component Apr 18, 2020
@MartinHjelmare
Copy link
Member

Cheers, good job and welcome to the community! 🎉

@MartinHjelmare MartinHjelmare merged commit 42b6ec2 into home-assistant:dev Apr 18, 2020
Dev automation moved this from Reviewer approved to Done Apr 18, 2020
@joogps joogps deleted the panasonic_viera branch April 18, 2020 03:19
@ramondunker
Copy link

Thanks for your work @joogps.

How do I use the send_key service?

@joogps
Copy link
Contributor Author

joogps commented Apr 18, 2020

First of all, thank you! :)

Second of all, the send_key service hasn’t been implemented yet, since I’m planning to create a default send_key for all media_player entities so then it becomes easier to implement remote control interfaces (for example, the HomeKit one)

@ramondunker
Copy link

ramondunker commented Apr 18, 2020 via email

@joogps
Copy link
Contributor Author

joogps commented Apr 19, 2020

@ramondunker I created the pull request: #34398

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

Panasonic GX800 series with Panasonic Viera TV integration doesn't works
4 participants