Skip to content

Add webhook + IFTTT example#16817

Merged
balloob merged 10 commits intodevfrom
webhook
Sep 30, 2018
Merged

Add webhook + IFTTT example#16817
balloob merged 10 commits intodevfrom
webhook

Conversation

@balloob
Copy link
Copy Markdown
Member

@balloob balloob commented Sep 24, 2018

Description:

This adds a new webhook component as proposed in home-assistant/architecture#80

To test it, I have updated the IFTTT component with a config flow to register a webhook.

screen shot 2018-09-24 at 4 54 31 pm

If we like architecture, I can add tests. Tests added

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Sep 24, 2018

One thing that I don't like about this approach is that we show the config info before the user confirms that they want to set it up. Would be better once they hit submit, however, we don't currently support showing a message when a config entry has been created.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Sep 24, 2018

We should also abort if the host is a local IP address

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Sep 24, 2018

image

@bind_hass
def async_register(hass, webhook_id, handler):
"""Register a webhook."""
handlers = hass.data.setdefault(webhook_id, {})
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.

Why do we need a dict to store the handler when each webhook id will only get one handler?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ugh that is a mistake, that should have been DOMAIN.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(still need to write tests)

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Looks good

})
assert result['type'] == data_entry_flow.RESULT_TYPE_FORM, result

result = await hass.config_entries.flow.async_configure(result['flow_id'], {})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Sep 25, 2018

Tests added, this is ready for review.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Sep 28, 2018

CC @dgomes I think that the webhook component is what the push camera could use too.

@dgomes
Copy link
Copy Markdown
Contributor

dgomes commented Sep 28, 2018

Sure thing, I'm currently a bit overwhelmed at work but will issue a PR with this ASAP

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.

6 participants