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

Add image support to hangouts notifications #16560

Merged
merged 20 commits into from
Sep 25, 2018
Merged

Add image support to hangouts notifications #16560

merged 20 commits into from
Sep 25, 2018

Conversation

quazzie
Copy link
Contributor

@quazzie quazzie commented Sep 11, 2018

Description:

Adds image support to hangouts notification

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6254

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
homeassistant/components/hangouts/hangouts_bot.py Outdated Show resolved Hide resolved
Copy link
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.

Instead to try to validate the url use a schematic like other notify platforms like:

{
   "photo": [
       "file": "...",
       "url": "...",
   ]
}

Or simple:

{
   "image_file": "",
   "image_url": "",
}

You can self select your style, but don't use this urlparse handling.

Examples:

After that it should be ready to merge.

* change to image_file/url
@quazzie
Copy link
Contributor Author

quazzie commented Sep 21, 2018

@pvizeli this ok ?

@pvizeli
Copy link
Member

pvizeli commented Sep 25, 2018

We should create a helper in future

@pvizeli pvizeli merged commit bc8d323 into home-assistant:dev Sep 25, 2018
@ghost ghost removed the in progress label Sep 25, 2018
@quazzie quazzie deleted the patch-2 branch September 25, 2018 13:06
@balloob
Copy link
Member

balloob commented Oct 5, 2018

So this PR added incorrect YAML to services.yaml which caused Home Assistant to not boot at all. Which begs the question, did anyone run this PR @quazzie ?

@quazzie
Copy link
Contributor Author

quazzie commented Oct 8, 2018

Hmm :( sorry last commit (rename to image_file/_url) was made with github edit as i was away so no that last commit was not run. Stupid mistake, sorry.

@balloob balloob mentioned this pull request Oct 12, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
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.

None yet

5 participants