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

mobile_app improvements #21607

Merged
merged 36 commits into from Mar 9, 2019
Merged

Conversation

robbiet480
Copy link
Member

@robbiet480 robbiet480 commented Mar 3, 2019

Description:

This PR contains improvements to the existing mobile_app implementation work done in #21475.

  • Cloudhooks support
  • Websocket messages for eventual frontend integration
  • Support for app specific platforms to implement their own logic.

This PR depends on #21606 being merged.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

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.

@robbiet480
Copy link
Member Author

robbiet480 commented Mar 3, 2019

@balloob A few things that need to be figured out for Cloudhooks support:

  • What if a user isn't initially a subscriber to Cloud and later becomes one. We want apps to use Cloudhooks whenever possible, so we need a way to get the Cloudhook URL to the app. The easiest way I see to do this is adding a GET /api/devices/{webhook_id} call back in and tell apps to check this every so often. It will return everything we know about the registration, including Cloudhook if necessary.
  • A user can disable (really delete) a webhook or Cloudhook from the configuration panel. There's no way for the app to know it's been intentionally deleted though since we (correctly) return 200's instead of errors if the hook doesn't exist so that they can't be guessed. I think the way to fix this is to store all webhook ID's ever generated by mobile_app separately from registrations. Whenever a request comes into a deleted webhook, we return a 404 instead of a 200. That way, the app can signal to the user that it's unable to connect.
  • Combining the first and second questions, What if a user was a cloud subscriber but no longer is? The app has the Cloudhook URL and the webhook ID, so it can fall back to the webhook ID but it wouldn't know the calls are failing because of the aforementioned 200 issue. I guess the fix for this is the same as the second suggested fix, return a 404 for previously registered hooks. However, maybe we should use a different response to signal to the app that the registration is still valid but to stop using the Cloudhook.

@robbiet480
Copy link
Member Author

robbiet480 commented Mar 3, 2019

  • Fixed problem 1 by implementing two new headers that are sent on local calls only: X-Cloud-Hook-ID and X-Cloud-Hook-URL. If these headers exist in a webhook call, the app should store the values and begin using them for all future updates.

  • Fixed problem 2, by implementing my 404 idea but instead returning a 410 Gone.

@robbiet480 robbiet480 added this to the 0.89.0 milestone Mar 3, 2019
@robbiet480
Copy link
Member Author

robbiet480 commented Mar 3, 2019

@balloob Can you take a look at the work i've done in 78e8494 and give it a archetecture tuneup like only you can really provide? Not sure if we should continue just having this one function hang out or actually implement more of a class system. Also, I've had to remove the vol.In(WEBHOOK_TYPES) from the webhook schema. Should we make app specific code report what webhook types its able to handle in advance so that we can still maintain the payload validation?

@robbiet480
Copy link
Member Author

robbiet480 commented Mar 3, 2019

For testing & docs purposes, here's a valid registration payload:

{
	"app_data": {
		"curl": true
	},
	"app_id": "io.homeassistant.curl",
	"app_name": "cURL",
	"app_version": "1.0.0",
	"device_name": "MBP",
	"manufacturer": "haxx.se",
	"model": "cURL",
	"os_version": "1.0",
	"supports_encryption": true
}

@robbiet480
Copy link
Member Author

@balloob If there can be only one frontend fix for 0.89 in relation to mobile_app it should probably be disabling the ability to delete/disable cloud hooks in the cloud panel.

@robbiet480
Copy link
Member Author

@balloob Final comment of the night: I'm considering renaming this to something like native_app or app_integration since there is nothing at all in this code that is specific to mobile. Thoughts?

Copy link
Member

@balloob balloob 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 few removals, then ok to merge.

@balloob balloob merged commit 9ab0753 into home-assistant:dev Mar 9, 2019
@ghost ghost removed the in progress label Mar 9, 2019
@robbiet480 robbiet480 modified the milestones: mobile_app, 0.90.0 Mar 14, 2019
@balloob balloob removed this from the 0.90.0 milestone Mar 15, 2019
@balloob balloob mentioned this pull request Mar 20, 2019
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