IFTTT notifications by kroozo · Pull Request #2053 · Flexget/Flexget

kroozo · 2018-01-02T14:58:36Z

Notification plugin to send notification to ifttt.com's webhook service.
(https://ifttt.com/maker_webhooks)

Motivation for changes:

Adds a simple notification plugin to send the notification to IFTTT webhook trigger that then can be used
in an IFTTT applet.

Config usage if relevant (new plugin or updated schema):

notify:
  task:
    via:
      - ifttt:
          keys:
            - some api key
            - other api key
          event: test

Log and/or tests output (preferably both):

2018-01-02 13:53 INFO     ifttt         notify_download Sent notification to key: some_valid_api_key
2018-01-02 13:53 ERROR    ifttt         notify_download Error sending to https://maker.ifttt.com/trigger/test/with/key/bad_key: 401 Client Error: Unauthorized for url: https://maker.ifttt.com/trigger/test/with/key/bad_key

Notification plugin to send notification to ifttt.com's webhook service. (https://ifttt.com/maker_webhooks)

liiight · 2018-01-02T23:09:33Z

flexget/plugins/notifiers/ifttt.py

+        :param str title: message subject
+        :param dict config: plugin config
+        """
+        notification_body = dict()


Just declare the dict with the data directly

liiight · 2018-01-02T23:10:54Z

flexget/plugins/notifiers/ifttt.py

+        notification_body = dict()
+        notification_body['value1'] = title
+        notification_body['value2'] = message
+        for key in config['keys']:


This won't work if keys is a string. You need to make sure it's a list. Look into other notifiers with prepare_data methods

liiight · 2018-01-02T23:13:32Z

flexget/plugins/notifiers/ifttt.py

+        for key in config['keys']:
+            url = self.url_template.format(config['event'], key)
+            try:
+                res = self.session.post(url, json = notification_body)


Remove spaces around key word calls.
Also, using Session() will raise on errors by default, so you'll never reach that else clause. Just remove the conditionals and assume that the notification was successful. Handle the failure in the except section

liiight · 2018-01-02T23:14:23Z

flexget/plugins/notifiers/ifttt.py

+                else:
+                    log.error("Error sending to: {}: {} {} -- {}".format(url, res.status_code, res.reason, res.text))
+            except HTTPError as e:
+                log.error("Error sending to {}: {}".format(url, e))


Please raise plugin.PluginWarning here. Look into other notifiers

liiight · 2018-01-02T23:15:09Z

flexget/plugins/notifiers/ifttt.py

+                    log.info("Sent notification to key: {}".format(key))
+                else:
+                    log.error("Error sending to: {}: {} {} -- {}".format(url, res.status_code, res.reason, res.text))
+            except HTTPError as e:


I think it's better to catch all RequestError here, just in case

cvium · 2018-01-03T08:25:53Z

flexget/plugins/notifiers/ifttt.py

+            raise PluginWarning("Failed to send notifications")
+
+    def prepare_config(self, config):
+        if isinstance(config['keys'], StringTypes):


if not isinstance(config['keys'], list):

I don't think we've ever used StringTypes. Either use str or the example I made.

list it is then :)
I am not a big fan of str with all the unicode hassle, that is why I usually resort to StringTypes. And testing for a list has other caveats (they end up being tuples sometimes for eg) but if it is more consistent with the rest of your code base, then sure. jsonschema makes it moot anyway most probably here.

kroozo · 2018-01-03T08:26:31Z

@liiight Fixed it up based on your comments, thanks.

Few questions:

I've moved the PluginWarning after the loop so that a bad endpoint does not kill the rest. However, would it make sense to raise a PluginError instead if all of them fail?
Tests: does it make any sense for you to add any? The whole thing is pretty straightforward. I can add unit tests around some of these, like key is string, key is list, bad key at the front, whatever, but for me that mostly just sounds like a burden that will need to be changed if anyone touches this anyway. The only thing that would be beneficial is checking if the IFTTT endpoint is breaking, but I would be hesitant to hook some random account of theirs into your CI.

Also, while I was scratching my own itch I realized that you actually have a feature request for this: https://feathub.com/Flexget/Flexget/+19

liiight · 2018-01-03T09:23:56Z

flexget/plugins/notifiers/ifttt.py

+            url = self.url_template.format(config['event'], key)
+            try:
+                self.session.post(url, json=notification_body)
+                log.info("Sent notification to key: {}".format(key))


let the logger handle its own interpolation:
log.info("Sent notification to key: %s", key)

kroozo added 2 commits January 2, 2018 15:44

IFTTT notifications

d43a5c8

Notification plugin to send notification to ifttt.com's webhook service. (https://ifttt.com/maker_webhooks)

remove unused import

ebcd311

liiight requested changes Jan 2, 2018

View reviewed changes

kroozo added 2 commits January 3, 2018 09:07

fix review comments

a4bff23

fix review comments Flexget#2

989671a

cvium reviewed Jan 3, 2018

View reviewed changes

liiight reviewed Jan 3, 2018

View reviewed changes

kroozo added 2 commits January 3, 2018 10:54

remove StringTypes comparison

66b0192

fix log string interpolation

90d4d17

liiight approved these changes Jan 3, 2018

View reviewed changes

liiight merged commit 1cc6e0d into Flexget:develop Jan 3, 2018

kroozo deleted the ifttt_notification branch January 3, 2018 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IFTTT notifications#2053

IFTTT notifications#2053
liiight merged 6 commits intoFlexget:developfrom
kroozo:ifttt_notification

kroozo commented Jan 2, 2018

Uh oh!

liiight Jan 2, 2018

Uh oh!

liiight Jan 2, 2018

Uh oh!

liiight Jan 2, 2018

Uh oh!

liiight Jan 2, 2018

Uh oh!

liiight Jan 2, 2018

Uh oh!

cvium Jan 3, 2018 •

edited

Loading

Uh oh!

kroozo Jan 3, 2018

Uh oh!

kroozo commented Jan 3, 2018

Uh oh!

liiight Jan 3, 2018

Uh oh!

kroozo Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kroozo commented Jan 2, 2018

Motivation for changes:

Config usage if relevant (new plugin or updated schema):

Log and/or tests output (preferably both):

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvium Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroozo commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cvium Jan 3, 2018 •

edited

Loading