Skip to content

Migrate HomematicIP Cloud services to admin services#32107

Merged
balloob merged 4 commits into
home-assistant:devfrom
SukramJ:HmIP-AdminServices
Feb 25, 2020
Merged

Migrate HomematicIP Cloud services to admin services#32107
balloob merged 4 commits into
home-assistant:devfrom
SukramJ:HmIP-AdminServices

Conversation

@SukramJ
Copy link
Copy Markdown
Contributor

@SukramJ SukramJ commented Feb 23, 2020

Breaking change

The user needs to be in the administrator role to execute these HomematicIP Cloud services:

  • homematicip_cloud.dump_hap_config
  • homematicip_cloud.reset_energy_counter

Proposed change

Migrate HomematicIP Cloud services to admin services

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

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 23, 2020

Codecov Report

Merging #32107 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #32107      +/-   ##
==========================================
+ Coverage   94.72%   94.74%   +0.01%     
==========================================
  Files         767      767              
  Lines       55486    55499      +13     
==========================================
+ Hits        52559    52582      +23     
+ Misses       2927     2917      -10
Impacted Files Coverage Δ
...assistant/components/homematicip_cloud/services.py 100% <100%> (ø) ⬆️
homeassistant/components/axis/camera.py 96.42% <0%> (-0.35%) ⬇️
homeassistant/components/darksky/sensor.py 96.81% <0%> (ø) ⬆️
homeassistant/components/yr/sensor.py 88.05% <0%> (ø) ⬆️
homeassistant/components/withings/const.py 100% <0%> (ø) ⬆️
homeassistant/components/wsdot/sensor.py 93.84% <0%> (ø) ⬆️
homeassistant/components/fido/sensor.py 100% <0%> (ø) ⬆️
homeassistant/components/integration/sensor.py 88.76% <0%> (ø) ⬆️
...omeassistant/components/here_travel_time/sensor.py 100% <0%> (ø) ⬆️
homeassistant/components/dsmr/sensor.py 100% <0%> (ø) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a678c6f...74fffa2. Read the comment docs.

@frenck
Copy link
Copy Markdown
Member

frenck commented Feb 23, 2020

This might be a breaking change for users. Please add a breaking change section to your OP.

@SukramJ
Copy link
Copy Markdown
Contributor Author

SukramJ commented Feb 23, 2020

Hi @frenck i added a breaking change paragraph to this PR.
But why is this a breaking change? How could a user without admin privileges execute a service?
There is currently no ui option. That's why i didn't add a breaking change paragraph before.

@SukramJ
Copy link
Copy Markdown
Contributor Author

SukramJ commented Feb 23, 2020

Thanks @frenck. I found the breaking change: When the service is called by a switch then a user might get an unauthorized exception.
I previously only thought about automations that run in an admin context and won't cause a breaking change.

@frenck
Copy link
Copy Markdown
Member

frenck commented Feb 23, 2020

The service can be used in the Lovelace UI as a button click for example...

async_register_admin_service(
hass=hass,
domain=HMIPC_DOMAIN,
service=SERVICE_SET_ACTIVE_CLIMATE_PROFILE,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(note: I'm not familiar with homematic)

Isn't this something like a climate preset ? Should this be an admin service or should it use the service helper verify_domain_control where they can call this service if they can control any homematic device?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll add verify_domain_control and make this service user accessible.

@SukramJ SukramJ requested a review from balloob February 24, 2020 06:33
@balloob balloob merged commit e0586df into home-assistant:dev Feb 25, 2020
@SukramJ SukramJ deleted the HmIP-AdminServices branch February 25, 2020 09:19
@lock lock Bot locked and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants