New notifier refactor by gazpachoking · Pull Request #1544 · Flexget/Flexget

gazpachoking · 2016-12-10T04:07:13Z

NOTE: ~~Everything here is still a draft, mostly untested, and meant for discussion still.~~ Getting mostly up to snuff at this point, though still open for other changes.

Motivation for changes:

Further separate the different types of notification generation plugins for simplification, and easier customization of each message type. Simplify the framework to not need advanced knowledge about the context of a message.

Detailed changes:

The notification framework only sends one message at a time now, and doesn't have knowledge of the 'context' of the message. (also makes a future log notifier easier, which isn't task or entry context)
plugins in the 'notifier' group now get params as (title, message, config) with all templates already rendered. title and message will be the message as constructed by one of the notification message generator plugins (notify_abort, notify_task, etc.), and config is the config for the transport plugin (email, prowl, etc.) as specified and validated by its schema (any text fields in the config will have jinja templates rendered against the appropriate context before being passed to transport plugin though)
to send a message, you send (title, message, outputs, [template_renderer]) where outputs is a list of configured notifier output plugins. template_renderer should be a function that renders jinja strings or templates against the appropriate context for the message. It will be applied to title, message, and any string keys in the output plugin configs.
Remove title and message params from each notifier plugin?
A function can be passed in which will be used to render all text fields in the notification config for convenience (so each notification generator doesn't have to do it themselves.)
The common base class was removed from the notification generators, and split into multiple separate files which now handle the different contexts of the notifications themselves.
Move notifications to a new notify phase. Add a way to suppress task aborts from this phase. (Is the way I did this okay? Is there something nicer?)
notify_task now only notifies when there are accepted or failed entries. Removed and tweaked templates so that we don't generate blank emails anymore.
Notifications are sent on --test runs now too.
Update the notification output plugins to take the title and message (add url as a default message field too?) separate than the output plugin config. (all of them now done, question about url as a default field remains open though)

Breaking changes

I've changed some names that I thought made sense, but we could backtrack if they don't seem worth it to break backwards compatibility in places.

Things to finish before merging

Make the sns notifier a regular output plugin again.
Combine notify_task, _entry, _abort, again? I'm still not really certain about that one, I sorta like the split.
Notify_task and _entries are combined, should _abort go in there too?
The common framework used to send notifications is now named notification_framework so that the task plugin can be named notify. Not great, but it's only used from other plugins, not user facing.
Consider the case where there are no other output plugins in a task but a notifier. In this instance it may be desirable to let failures in the notification cause entry failures/task aborts, as there won't be any side effects.
Consider whether the notification framework should provide 'url' as a default field. There are several notifiers which all support 'url', but it wouldn't be 100%. 'subject' is already included though, and that isn't used 100% either, so url might be just as appropriate. Leaning to 'no' on this.

… entries

Use it to test notify_abort.

liiight · 2016-12-10T13:56:52Z

flexget/plugins/notifiers/email.py


-    def notify(self, to, message, title, smtp_host, smtp_port, smtp_username=None, smtp_password=None, smtp_tls=None,
-               smtp_ssl=None, html=None, **kwargs):
+    def notify(self, title, message, config):


This is how I initially did it, by passing config dict. @paranoidi advised me to pass it a literal params for clarity's sake. I can't say I disagree

But... it wasn't passing in literal params, it was passing in a config dict, with other stuff added to it, which was being keyword argument expanded. This way we are passing in our actual literal params, which are the ones generated by the notification system, title, message, and the config, as specified and validated by the schema of the plugin, like we do for our other plugin methods. The difference is that these parameters are all the discrete ones that the caller knows about, and doesn't mix in the ones the callee is defining in its own schema.

@liiight What you had was different, it was a single dict called "data" and contained message and configuration in same dict. This is what I proposed as first ideal solution but there were some challenges with that at the time. This is fine.

liiight · 2016-12-10T13:58:01Z

flexget/plugins/notifiers/notify.py

-
+                title = template_renderer(title)
+            except RenderError as e:
+                log.error('Error rendering notification title: %s', e)


So you got rid of falling back to default in case of render error?

It falls back to the non rendered jinja string.

I liked having a fallback dict, but it's no biggie I guess

I sorta feel like having the unrendered string will be a better indication of error when debugging why their config isn't working rather than getting some string that isn't to do with the title/message they defined.

liiight · 2016-12-10T14:03:36Z

flexget/plugins/notifiers/notify_entry.py

+                           '{{ title }}'
+                           '{% endif %}'
+            },
+            'template': {'type': 'string'},


I used file_templates to differentiate this from template plugin

Hmm, I guess we do have a couple different kinds of templates, not sure if file_template really helps clarify things much or not though.

notification template maybe? seems long and too verbose

liiight · 2016-12-10T14:04:12Z

flexget/plugins/notifiers/notify_entry.py

+            return
+        # If a file template is defined, it overrides message
+        if config.get('template'):
+            body = get_template(config['template'], 'entry')


This could raise ValueError

Good point. Need to catch that.

gazpachoking · 2016-12-10T19:46:57Z

I was also wondering if 'via' might be more appropriate for the list of notifier outputs than 'to', e.g.:

notify_task:
  via:
  - email:
      to: blah@blah.com

Eliminates the double nested 'to' option in the email case. I think it's a bit more accurate description as well, i.e. the notification is being sent 'via' one of the transport plugins, and 'to' a recipient defined in that transport plugin config. (BTW, maybe need some more consistent terminology to discuss these plugins. What do we call the message generators, (notify_task, notify_abort, etc.,) vs the transport ones, (email, pushbullet, etc.?) similar to the problem of 'list plugins' (list_match, list_add, etc.) vs 'list_plugins' (imdb_list, trakt_list, etc.))

liiight · 2016-12-11T09:15:25Z

I like using via but I'm not sure if this warrants ANOTHER upgrade actions in spite all the noise this change already made...
About the semantics, I suggest we lock down the manged list semantics first 😛

gazpachoking · 2016-12-11T22:18:47Z

About the semantics, I suggest we lock down the manged list semantics first

I think the terminology should be the same, and I don't mean necessarily for user facing docs. We have a pattern of defining an api the plugins in a certain group should implement. We then have plugins in that group, and plugins that use plugins in that group. This applies both to the list plugins, and the notification plugins. I just want to figure out what the hell to call things when discussing. 😛

liiight · 2017-01-02T09:17:56Z

I'd say making notify_abort work like the other two makes sense, no need to have two schema paradigms.
About having no output plugins in the task, I don't know how common of an issue that is, and if it's worth supporting as a real life scenario. maybe handle that problem if and when it arises?

andocromn · 2017-01-03T20:21:06Z

@liiight I wanted to comment on the "no other output plugins" consideration since I was the one to raise this concern. Although I agree its probably not common, the email plugin used to be just a normal output so there was nothing wrong with this configuration. I raised the concern to address the changes in the behavior resulting from introduction of the notify phase and discuss how to best maintain the previous functionality while still implementing the improvements. As a result of the changes associated with the notify_phase, a task that only has a notification output will no longer retry sending the notification if the notification fails. I think that if the notification is the only output, and the notification fails, it should consider that entry to be failed

As an alterantive, I'm not sure if this was considered, but the best way of handling may be to create a notification queue. This way, regardless of the output, any failed notifications could be retried on the next run of the task and would be sent to the user without the program re-downloading

# Conflicts: # flexget/plugins/notifiers/email.py

…gins

liiight · 2017-01-05T05:02:18Z

@gazpachoking notifyosd is a crappy name but it's not original name https://wiki.ubuntu.com/NotifyOSD

gazpachoking · 2017-01-05T05:08:38Z

@gazpachoking notifyosd is a crappy name but it's not original name https://wiki.ubuntu.com/NotifyOSD

Oh hmm. It works with this too, right? https://wiki.archlinux.org/index.php/Desktop_notifications 'notify osd' seems to be just one implementation for unity.

gazpachoking · 2017-01-05T05:11:29Z

Speaking of which, should we change it to this https://pypi.python.org/pypi/notify2 , instead of requiring gtk as the dependency?

liiight · 2017-01-05T05:11:37Z

Yeah, looks better

liiight · 2017-01-05T08:07:08Z

Sweet, windows osd. Works on 7 and up I suppose?

gazpachoking · 2017-01-05T13:46:16Z

Sweet, windows osd. Works on 7 and up I suppose

Works on 10 at least, (though not optimal integration with the new action center,) not sure how far back it goes, I assume it works fine on 7.

gazpachoking · 2017-01-05T13:47:31Z

What should we call the osd notifier now that it's a bit more generic? Would 'popup' or 'desktop' or something be more appropriate?

liiight · 2017-01-05T22:11:48Z

~~I think on_screen_display is generic enough~~ Maybe toast?. Also, I think it's schema should also be a Boolean, so you could just do on_screen_display: yes

liiight · 2017-01-06T07:20:00Z

Hmm,got this crash while testing it:

4 VERBOSE  task          ser             ACCEPTED: `series.s01e01.720p.hdtv-flexget` by accept_all plugin                           2017-01-06 01:14 VERBOSE  details       ser             Summary - Accepted: 1 (Rejected: 0 Undecided: 0 Failed: 0)                                 2017-01-06 01:14 WARNING  task          ser             Task doesn't have any output plugins, you should add (at least) one!                       2017-01-06 01:14 CRITICAL task          ser             BUG: Unhandled error in plugin notify: Missing parentheses in call to 'print' (__init__.py, line 39)                                        Traceback (most recent call last):
  File "/data/data/com.termux/files/home/Flexget/flexget/task.py", line 490, in __run_plugin          return method(*args, **kwargs)
  File "/data/data/com.termux/files/home/Flexget/flexget/event.py", line 23, in __call__              return self.func(*args, **kwargs)              File "/data/data/com.termux/files/home/Flexget/flexget/plugins/notifiers/notify.py", line 129, in on_task_notify                                     send_notification(config['task']['title'], template, config['task']['via'], template_renderer=task.render)                                       File "/data/data/com.termux/files/home/Flexget/flexget/plugins/notifiers/notification_framework.py", line 120, in send_notification                  notifier.notify(title, message, rendered_config)  # TODO: Update notifiers for new api          File "/data/data/com.termux/files/home/Flexget/flexget/plugins/notifiers/on_screen_display.py", line 28, in linux_notify                             from gi.repository import Notify               File "/data/data/com.termux/files/usr/lib/python3.5/site-packages/gi/__init__.py", line 39          print url                                                ^                                    SyntaxError: Missing parentheses in call to 'print'

liiight · 2017-01-06T12:00:26Z

I'm not so sure anymore about the via keyword now that we have the task and entries levels:

notify:                                                                                            
  task:                                                                                    
    via:                                                                                     
       - on_screen_display: {}

This feels too much to me...

gazpachoking · 2017-01-07T16:48:28Z

Hmm,got this crash while testing it:

Very tough to read that traceback. Looks like maybe the gobject introspection doesn't support python 3? I'm thinking of reverting the changes to osd for now and dealing with that in a separate PR. Maybe include the rename, and allow for bool schema, that way we could upgrade it outside of next branch.

gazpachoking · 2017-01-07T17:29:24Z

I'm not so sure anymore about the via keyword now that we have the task and entries levels:

Dunno what there is to be done about that. We could move the via to directly under notify, but that would mean you couldn't configure e.g. aborts to notify via a different method than entries.

Allow boolean configuration for toast.

Update toast plugin registration to use interfaces # Conflicts: # flexget/plugins/notifiers/notify.py # flexget/plugins/notifiers/notify_osd.py # flexget/plugins/notifiers/sns.py

gazpachoking added 9 commits December 9, 2016 21:27

Roughly sketch a new notifier api, rewrite notifiy_task to use it

273cfce

Don't send task notifications for uninteresting task runs

7d272a6

Don't send task notifications for uninteresting task runs, really.

925cce1

Flesh out new notifier refactor a bit

f74ce51

Add an ability for certain phases to not trigger task aborts on error.

4eca0d5

Convert email and pushover notifier outputs to new api

6615de9

Refactor task level templates to all contain both accepted and failed…

24a80d3

… entries

Add a debug notifier output to use with tests.

33b73c7

Use it to test notify_abort.

Add basic notify_task tests.

ca196ff

liiight reviewed Dec 10, 2016

View reviewed changes

gazpachoking added 12 commits December 11, 2016 16:12

Update join plugin for new notification api

37e02ca

Update notify_osd plugin for new notification api

a12ce0c

Update notifymyandroid plugin for new notification api

d6a462c

Update prowl plugin for new notification api

2d74a69

Update pushalot plugin for new notification api

1240e54

Clean up some missed schema updates

905388e

Update rapidpush plugin for new notification api

7af591b

Update slack plugin for new notification api

658e7b0

Update sms_ru plugin for new notification api

a355fc5

Update sns plugin for new notification api

02955ad

Update telegram plugin for new notification api

c7e22ba

Update xmpp plugin for new notification api

26b6da7

gazpachoking added 2 commits December 11, 2016 18:01

Renamed 'to' option for all notification generators to 'via'

b119814

Improve notification system documentation.

e16e2a2

gazpachoking added 6 commits January 4, 2017 22:00

Combine notify_abort into notify plugin

d483ff4

Rename the notification framework plugin notification_framework

3a92bc3

Merge branch 'next' into new_notifier_api

09d8502

# Conflicts: # flexget/plugins/notifiers/email.py

Finish renaming to notification_framework

e21cce7

Remove backwards compatibility task level functionality of notify plu…

781f64c

…gins

Rename notify_osd to on_screen_display

d784e9d

Add a windows implementation to on_screen_display notifier

fc1da1b

Removed unneeded imports

4f4c2c9

gazpachoking added 2 commits January 8, 2017 10:22

Rename osd notifier to 'toast'.

aa35d83

Allow boolean configuration for toast.

Merge branch 'next' into new_notifier_api

b3e2a21

Update toast plugin registration to use interfaces # Conflicts: # flexget/plugins/notifiers/notify.py # flexget/plugins/notifiers/notify_osd.py # flexget/plugins/notifiers/sns.py

gazpachoking changed the title ~~[WIP] New notifier refactor~~ New notifier refactor Jan 8, 2017

gazpachoking added 2 commits January 8, 2017 10:58

Fix some leftover references to 'groups' after merge.

9377f25

notification_framework doesn't declare task interface

08c4c32

gazpachoking merged commit 846fd05 into next Jan 8, 2017

liiight mentioned this pull request Jan 8, 2017

Next #1587

Merged

gazpachoking deleted the new_notifier_api branch January 16, 2017 08:06

Conversation

gazpachoking commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for changes:

Detailed changes:

Breaking changes

Things to finish before merging

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gazpachoking Dec 10, 2016 • 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!

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!

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!

gazpachoking commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liiight commented Dec 11, 2016

Uh oh!

gazpachoking commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liiight commented Jan 2, 2017

Uh oh!

andocromn commented Jan 3, 2017

Uh oh!

liiight commented Jan 5, 2017

Uh oh!

gazpachoking commented Jan 5, 2017

Uh oh!

gazpachoking commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liiight commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liiight commented Jan 5, 2017

Uh oh!

gazpachoking commented Jan 5, 2017

Uh oh!

gazpachoking commented Jan 5, 2017

Uh oh!

liiight commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liiight commented Jan 6, 2017

Uh oh!

liiight commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gazpachoking commented Jan 7, 2017

Uh oh!

gazpachoking commented Jan 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

gazpachoking commented Dec 10, 2016 •

edited

Loading

gazpachoking Dec 10, 2016 •

edited

Loading

gazpachoking commented Dec 10, 2016 •

edited

Loading

gazpachoking commented Dec 11, 2016 •

edited

Loading

gazpachoking commented Jan 5, 2017 •

edited

Loading

liiight commented Jan 5, 2017 •

edited

Loading

liiight commented Jan 5, 2017 •

edited

Loading

liiight commented Jan 6, 2017 •

edited

Loading