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.

Sign up for GitHub

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

Fix google assistant request sync service call #17415

Merged
merged 3 commits into from
Nov 27, 2018
Merged

Fix google assistant request sync service call #17415

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9f45ec
Update __init__.py
awarecan Oct 13, 2018
f8fb1d1
Add optional agent_user_id field to request_sync service
awarecan Oct 16, 2018
0d942fe
Update services.yaml
balloob Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion homeassistant/components/google_assistant/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,11 @@ async def request_sync_service_handler(call: ServiceCall):
websession = async_get_clientsession(hass)
try:
with async_timeout.timeout(5, loop=hass.loop):
agent_user_id = call.context.user_id or DEFAULT_AGENT_USER_ID
res = await websession.post(
REQUEST_SYNC_BASE_URL,
params={'key': api_key},
json={'agent_user_id': call.context.user_id})
json={'agent_user_id': agent_user_id})
Copy link
Member

Choose a reason for hiding this comment

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

We should always send the same agent_user_id as was set in https://github.com/home-assistant/home-assistant/blob/bd450ee9ffdb7e7aa765070a1c86118cde6f9e1b/homeassistant/components/google_assistant/smart_home.py#L244-L247

If it's not the same, it won't trigger a resync.

So we should just hardcode this to home-assistant since this is a single tenant skill anyway?

Copy link
Member

Choose a reason for hiding this comment

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

This is also noted in the original documentation as well. Failing to set a consistent Agent User ID causes the user to relink assistant so they can issue a sync command.

https://www.home-assistant.io/components/google_assistant/#troubleshooting-the-request_sync-service

I think hard-coding it is a much better approach so the user doesn't need to worry about it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is fine to supply a const user id for now, since we don't have permission system in place yet. All users will get same access anyway. However, after we introduce permission, thing will become interesting.

Just a reminder, we need a way to supply context.user to system triggered event, not only for this particular issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rethink it again. I should not revert user id to a fixed value only because it breaks automation script. Instead, I am going to add an optional field to the service call, so if user need it, can be provided in the automation.

Copy link
Member

Choose a reason for hiding this comment

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

What is the use case of supporting multiple agent_user_id ? It's linked to a test skill and this is the only instance?

Copy link
Contributor Author

@awarecan awarecan Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If you have multiple Google users, they can link to different HA user. If those HA users have different permission, they will get different devices linked in Google Assistant.

Copy link
Member

Choose a reason for hiding this comment

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

good point.

In that case we should have this service fail if not called from the context of a user?

_LOGGER.info("Submitted request_sync request to Google")
res.raise_for_status()
except aiohttp.ClientResponseError:
Expand Down