Skip to content

Mobile app to notify when sensor is disabled#71561

Merged
balloob merged 3 commits intodevfrom
mobile-app-error-disable-sensor
May 17, 2022
Merged

Mobile app to notify when sensor is disabled#71561
balloob merged 3 commits intodevfrom
mobile-app-error-disable-sensor

Conversation

@balloob
Copy link
Copy Markdown
Member

@balloob balloob commented May 9, 2022

Breaking change

Proposed change

This will add a new is_disabled field set to True to the success response for a sensor that is disabled. This allows the app to update the settings and stop reporting the sensor.

I initially had made it an error but realized that it would be backwards incompatible.

CC @home-assistant/android @home-assistant/ios-developers

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

  • 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:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (mobile_app) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label May 9, 2022
@balloob balloob force-pushed the mobile-app-error-disable-sensor branch from 273944b to d07f4ab Compare May 9, 2022 14:52
@frenck
Copy link
Copy Markdown
Member

frenck commented May 9, 2022

Just wondering... how does that app know that a sensor is enabled again... without updating it?

@balloob
Copy link
Copy Markdown
Member Author

balloob commented May 9, 2022

It will need to sync the entity registry. We can add a new webhook for this or they use existing entity registry command. I guess we could store an integer ("Current sync state") in the config entry that we send back to indicate if a sync is required.

@frenck
Copy link
Copy Markdown
Member

frenck commented May 9, 2022

It will need to sync the entity registry.

Right, but if we go that path, is this PR still needed?

@jpelgrom
Copy link
Copy Markdown
Member

jpelgrom commented May 9, 2022

It will need to sync the entity registry

Unless I'm mistaken, there is currently no option to be 100% certain that a certain entity in the registry matches an entity the app has registered, because the available entity registry (via websocket) doesn't expose a stable ID for the sensor (the unique_store_key in the webhook_register_sensor function) and while the app can guess the most likely entity ID, users can change it.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented May 11, 2022

I talked to @zacwest (in person!). We decided to update get_config webhook to include the enabled state of entities.

@balloob balloob mentioned this pull request May 13, 2022
22 tasks
@balloob balloob force-pushed the mobile-app-error-disable-sensor branch from d07f4ab to a11e906 Compare May 13, 2022 21:40
@balloob
Copy link
Copy Markdown
Member Author

balloob commented May 13, 2022

I've sketched out a full plan based on my conversations with Zac and pushed to this branch.

Updated docs for this is home-assistant/developers.home-assistant#1325

preview: https://deploy-preview-1325--developers-home-assistant.netlify.app/docs/api/native-app-integration/sensors#keeping-sensors-in-sync-with-home-assistant

@balloob balloob merged commit 0b09376 into dev May 17, 2022
@balloob balloob deleted the mobile-app-error-disable-sensor branch May 17, 2022 16:05
@github-actions github-actions bot locked and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed core has-tests integration: mobile_app new-feature small-pr PRs with less than 30 lines. smash Indicator this PR is close to finish for merging or closing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants