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

Update requirement version and add switcher_kis services #23477

Merged
merged 19 commits into from Jun 14, 2019

Conversation

TomerFi
Copy link
Contributor

@TomerFi TomerFi commented Apr 27, 2019

Description:

  • Updated aioswitcher requirement from 2019.3.21 to 2019.4.26.

  • Added two services for the switcher_kis component:

    • switcher_kis.set_auto_off for setting the auto-shutdown configuration value on the device.
      Takes one argument named auto_off of type time_period_str (example: "02:30").
    • switcher_kis.update_device_name for setting the name configuration value on the device.
      Takes one argument named 'name' of type string with 2 < length < 32 (example: "My Switcher Device").
  • Added test cases for the new services.

  • Fixed a couple of left-overs change requests from the previous PR Added component named switcher_kis switcher water heater integration. #22325.

  • Added ServiceCallType in homeassistant.helpers.typing for use with static typing.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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 (example).
  • New dependencies have been added to requirements in the manifest (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@ghost
Copy link

ghost commented Apr 27, 2019

Hey there @TomerFi, mind taking a look at this pull request as its been labeled with a integration (switcher_kis) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

async def async_set_auto_off_service(service: ServiceCallType) -> None:
"""Use for handling setting device auto-off service calls."""
from aioswitcher.api import SwitcherV2Api
async with SwitcherV2Api(hass.loop, device_data.ip_addr, phone_id,
Copy link
Member

Choose a reason for hiding this comment

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

You should validate that the user has at least access to control a single entity of this integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much, I'll get right on it!
:-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved with the use of async_listen_platform in this commit.

Copy link
Member

Choose a reason for hiding this comment

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

Reading this again, I think @balloob wants us to check the access rights for the user to run the service on an entity.

https://developers.home-assistant.io/blog/2019/03/11/user-permissions.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartinHjelmare
Oh yes I see that now.
I'll have a closer look tonight and push the adjustments.
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in this commit.

Copy link
Contributor Author

@TomerFi TomerFi May 7, 2019

Choose a reason for hiding this comment

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

Now that I think about it...
I should probably add a component verification before the permission verification.
Otherwise a user can pass any permitted entity and he will pass the verification process, because there is no actual use for the entity in there two services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added domain and entity_id validation in this commit.
The domain validation might seem redundant, but it won't be once I'll integrate the sensor platform representing the device's schedules.
For now, I can easily remove it if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartinHjelmare
Any thoughts?
:-)

@TomerFi TomerFi force-pushed the add-switcher_kis-services branch from 7134421 to 7decb90 Compare May 7, 2019 12:20
await swapi.set_device_name(service.data[CONF_NAME])

hass.services.async_register(
DOMAIN, SERVICE_UPDATE_DEVICE_NAME_NAME,
Copy link
Member

Choose a reason for hiding this comment

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

What's the use of allowing people to set a device name? Since it has a unique ID, users can just use the entity registry to change the name and entity ID.

We should only offer things as a service that might be used for automations. Administrative tasks should be done in the app for the product.

Copy link
Contributor Author

@TomerFi TomerFi Jun 6, 2019

Choose a reason for hiding this comment

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

@balloob
Yeah but that entity registry modification will of course only reflect in home assistant.
This service sets the actual device name so it's also reflected in the vendor's app.

It's on a nice-to-have basis I believe,
I can drop this service if you think in redundant.

Please advice...

Copy link
Member

Choose a reason for hiding this comment

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

Let's drop it. We generally avoid putting device management into HA (exceptions if integration is about a protocol like Zigbee/Z-Wave)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balloob
Removed in this commit.

Thanks!
:-)

@balloob balloob merged commit fe8a330 into home-assistant:dev Jun 14, 2019
@balloob
Copy link
Member

balloob commented Jun 14, 2019

Awesome! 🐬

@balloob balloob mentioned this pull request Jun 26, 2019
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Oct 12, 2019
…ant#23477)

* Update aioswitcher requirement to 2019.4.26.

* Removed unnecessary legacy function call.

* Fixed log message capital first letter.

* Replaced None argument with empty dict.

* Replaced guard.

* Added ServiceCallType.

* Added set_auto_off and update_device_name services to the component.

* Added test cases for service calls.

* Conditioned the component services registry with the platform discovery.

* Update homeassistant/components/switcher_kis/__init__.py

Co-Authored-By: TomerFi <tomer.figenblat@gmail.com>

* Update homeassistant/components/switcher_kis/__init__.py

Co-Authored-By: TomerFi <tomer.figenblat@gmail.com>

* Resolved change requests.

* Added ContextType.

* Addes permission verification for service calls.

* Added test cases for permision verification and more.

* Replaced POLICY_CONTROL with the more suited POLICY_EDIT.

* More appropriate function name.

* Added domain and entity_id validation for calling services.

* Removed service for setting the vendor's device name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants