Skip to content

Allow Twitter notifications to include media#8282

Merged
balloob merged 3 commits into
home-assistant:devfrom
MikeChristianson:media-for-notifications
Jul 11, 2017
Merged

Allow Twitter notifications to include media#8282
balloob merged 3 commits into
home-assistant:devfrom
MikeChristianson:media-for-notifications

Conversation

@MikeChristianson
Copy link
Copy Markdown
Contributor

@MikeChristianson MikeChristianson commented Jul 2, 2017

Allow Twitter notifications to include media.

The notifier uses the Twitter-recommended async chunked media/upload approach and tries to convey the correct mime type of the media. The implementation is based on https://github.com/geduldig/TwitterAPI/blob/master/examples/upload_video.py.

notify:
  - name: twitter
    platform: twitter
    consumer_key: 
    consumer_secret: 
    access_token: 
    access_token_secret: 

automation twitter:
  alias: Send a test message
  initial_state: False
  hide_entity: False
  trigger:
    platform: time
    seconds: 00
  action:
    - service: notify.twitter
      data:
        message: "Test at {{ now().strftime('%I:%M:%S %p') }}."
        data:
          media: /tmp/my-awesome-video.mp4
    - service: homeassistant.turn_off
      entity_id: automation.send_a_test_message

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @MikeChristianson,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@mention-bot
Copy link
Copy Markdown

@MikeChristianson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pvizeli, @balloob and @fabaff to be potential reviewers.

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.

Other notify platforms are using ATTR_DATA for it. That way if a platform requires extra metadata, we don't need to add extra fields. So please add it to Twitter via ATTR_DATA.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I attempted that approach, initially, but was not successful. I'll give it another try and look for examples.

Comment thread tests/testing_config/automations.yaml Outdated
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.

Please remove this file. We fixed the issue in our tests that left this artifact.

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.

Media path has to be allowed by hass.config.is_allowed_path

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.

This is very inefficient. You are now generating up to 99 values. Prefer

if 199 > resp.status_code < 300:

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.

Shouldn't this method not be called if media_path is not set ?

Also, prefer a guard clause:

if not media_path:
    return

media_type, _ = mimetipes.guess_type(…)

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.

Since media_id is going to be None, why not just return None and you can remove the else.

…mentation. The Twitter notifier uses the Twitter-recommended async chunked media/upload approach and tries to convey the correct mime type of the media. Twitter implementation based on https://github.com/geduldig/TwitterAPI/blob/master/examples/upload_video.py.
balloob: "Please remove this file. We fixed the issue in our tests that left this artifact."
balloob: "…prefer a guard clause"
balloob: "This is very inefficient. You are now generating up to 99 values."
balloob: "Since media_id is going to be None, why not just return None and you can remove the else."
… if a platform requires extra metadata, we don't need to add extra fields. So please add it to Twitter via ATTR_DATA."
@MikeChristianson
Copy link
Copy Markdown
Contributor Author

@balloob Please have another look, thanks.

@MikeChristianson MikeChristianson changed the title Allow notifications to include media, with Twitter as the first implementation. Allow Twitter notifications to include media Jul 9, 2017
@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 11, 2017

Awesome! 🐬 🎉

@balloob balloob merged commit 92dc767 into home-assistant:dev Jul 11, 2017
@MikeChristianson MikeChristianson deleted the media-for-notifications branch July 11, 2017 05:17
@MikeChristianson
Copy link
Copy Markdown
Contributor Author

Thanks, @balloob! 🎊 Glad I could help the community, too.

@balloob balloob mentioned this pull request Jul 13, 2017
MikeChristianson added a commit to MikeChristianson/home-assistant that referenced this pull request Jul 17, 2017
data may be None if twitter data property unconfigured:
  File "/opt/homeassistant/homeassistant_venv/lib/python3.4/site-packages/homeassistant/components/notify/twitter.py", line 63, in send_message
    media = data.get(ATTR_MEDIA)
fabaff pushed a commit that referenced this pull request Jul 17, 2017
data may be None if twitter data property unconfigured:
  File "/opt/homeassistant/homeassistant_venv/lib/python3.4/site-packages/homeassistant/components/notify/twitter.py", line 63, in send_message
    media = data.get(ATTR_MEDIA)
@balloob balloob mentioned this pull request Jul 29, 2017
@onerealfunnyguy
Copy link
Copy Markdown

Can you please update the component page to include the correct formatting for images, can send txt tweet no problems, yet with the above example using both a local path and http:// I can not get the notify to work once I add the include media code.

dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
* Allow notifications to include media, with Twitter as the first implementation. The Twitter notifier uses the Twitter-recommended async chunked media/upload approach and tries to convey the correct mime type of the media. Twitter implementation based on https://github.com/geduldig/TwitterAPI/blob/master/examples/upload_video.py.

* Changes based on balloob's review:
balloob: "Please remove this file. We fixed the issue in our tests that left this artifact."
balloob: "…prefer a guard clause"
balloob: "This is very inefficient. You are now generating up to 99 values."
balloob: "Since media_id is going to be None, why not just return None and you can remove the else."

* balloob: "Other notify platforms are using ATTR_DATA for it. That way if a platform requires extra metadata, we don't need to add extra fields. So please add it to Twitter via ATTR_DATA."
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
…ome-assistant#8513)

data may be None if twitter data property unconfigured:
  File "/opt/homeassistant/homeassistant_venv/lib/python3.4/site-packages/homeassistant/components/notify/twitter.py", line 63, in send_message
    media = data.get(ATTR_MEDIA)
@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017
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.

6 participants