Apprise Integration #2796

caronc · 2024-02-03T20:42:24Z

Just a light stab at Apprise integrations right into SABnzbd

Multiple URLs supported by simply separating each with a space and/or comma.

Updated Screenshot

Rules:

Default Apprise URLs (comma/space separated if more then one) applies to all ✔️ boxes below.
Enabled categories can optionally have URLs provided that will over-ride the default Apprise URL configuration. Multiple URLs can also optionally be specified here if the intent is to target more then one location.

Consider the screenshot above:

A ntfy and email is sent for Job Finished, Job Failed and/or Other Message categories.
In the event of a Disk Full error, ONLY a Discord notice is sent instead.

caronc · 2024-02-03T20:55:28Z

@Safihre : Would you have a sample [black] configuration i would need to run to pass these config files? when i download black and run it, it formats way more then i changed/added. The config/instructions you require of black may be right in front of me, i'm just overlooking it 🙂

thezoggy · 2024-02-03T20:59:19Z

@Safihre : Would you have a sample [black] configuration i would need to run to pass these config files? when i download black and run it, it formats way more then i changed/added. The config/instructions you require of black may be right in front of me, i'm just overlooking it 🙂

you can get it from:
https://github.com/sabnzbd/sabnzbd/blob/develop/.github/workflows/integration_testing.yml#L6

thezoggy · 2024-02-03T21:01:03Z

looks like this also requires additional dependencies,

ModuleNotFoundError: No module named 'yaml'

PyYAML ?

caronc · 2024-02-03T21:07:32Z

@thezoggy : I got to step out for about 3 hrs or so, but hopefully these test cases will pass. I'll have a look if not when i get back. Thanks for your help!

I'd say don't merge this yet anyway, i cna update the AppriseAssets object to use the SABNzbd icons and other customizations.

thezoggy · 2024-02-03T21:11:32Z

looks like PyYAML does not do universal binaries for macos

PyInstaller.utils.osx.IncompatibleBinaryArchError: /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/yaml/_yaml.cpython-312-darwin.so is not a fat binary!

which apparently they decided not to do:
yaml/pyyaml#629

Safihre · 2024-02-04T10:15:11Z

Does it really need yaml? Where is it used?

caronc · 2024-02-04T13:01:57Z

Yes, if you use the apprise:// plugin, it can fetch from a centralized Apprise API server.

I ended up having to do other things last night. I'll take another stab at it today.

I'll just move the yaml Python code over as per link Zoggy shared. The other thing i wanted to do was adapt the Assets to use your icons. I presume that might be what you want since it was the case for the nzb-notify script. It won't be much effort

Edit: I'm now seeing the problem; requirements.txt through dependencies pull in pyYAML whether i include it here or not. I don't have plans on removing the dependency upstream in Apprise. Not sure the best course of action here.

This may be usefully for testing with requirements.txt:

pyYAML --no-binary pyYAML

caronc · 2024-02-04T18:58:19Z

Had to re-add PyYAML into git actions pipe runner with the --no-binary as it simply does not build the MacOS version without it.

Safihre · 2024-02-05T07:53:14Z

requirements.txt

+PyYAML; sys_platform != 'darwin'
+
+# Apprise Requirements
+requests


Please specify the exact versions. It's important for the packaging reproducibility.

Safihre · 2024-02-05T07:56:26Z

requirements.txt

@@ -54,7 +55,17 @@ notify2==0.3.1; sys_platform != 'win32' and sys_platform != 'darwin'
 # Uncomment line below or manually install after installing requirements.
 # pygobject>=3.10.2; sys_platform != 'win32' and sys_platform != 'darwin'

+# PyYAML does not have binary packages for Mac Users
+PyYAML --no-binary=PyYAML; sys_platform == 'darwin'
+PyYAML; sys_platform != 'darwin'


Why not have non-binary for all?

It's more efficient to use the binary when it's available. It's unfortunate that macos environment is impacted into a situation where they can't use it.

Safihre · 2024-02-05T07:57:34Z

sabnzbd/cfg.py

+apprise_prio_warning = OptionBool("apprise", "apprise_prio_warning", False)
+apprise_prio_error = OptionBool("apprise", "apprise_prio_error", False)
+apprise_prio_queue_done = OptionBool("apprise", "apprise_prio_queue_done", False)
+apprise_prio_other = OptionBool("apprise", "apprise_prio_other", True)


Shouldn't we also have priorities? Since we have direct integration of Apprise now, then we could specify it, right?

I took this to a slightly different level so i could accommodate the power of apprise. If i only focused on PushOver Priorities, you'd only be addressing 1 of the 100+ services Apprise can adapt to. See general description in PR for updated screenshot.

Safihre · 2024-02-05T08:43:41Z

sabnzbd/notifier.py

+        app_desc="SABnzbd Notification",
+        app_url="http://sabnzbd.org/",
+        image_path_mask=os.path.join(os.path.dirname(__file__), "apprise", "apprise-{TYPE}.png"),
+        image_url_mask="https://raw.githubusercontent.com/sabnzbd/sabnzbd/develop/sabnzbd/apprise/apprise-{TYPE}.png",


Should we maybe host these at sabnzbd.org?
https://github.com/sabnzbd/sabnzbd.github.io/tree/master/images/icons

Sure we can, let me know the URL i should use. Even using the one you just shared now:

https://github.com/sabnzbd/sabnzbd.github.io/tree/master/images/icons becomes https://raw.githubusercontent.com/sabnzbd/sabnzbd.github.io/master/images/icons/ for direct reference.

Perhaps you man the url:

https://github.com/sabnzbd/sabnzbd.github.io/tree/master/images/icons (as it is now) should change to: https://sabnzbd.org/images/icons/ ?

So ... ?

# Prepare our Asset Object asset = apprise.AppriseAsset( app_id="SABnzbd", app_desc="SABnzbd Notification", app_url="http://sabnzbd.org/", image_path_mask=os.path.join(os.path.dirname(__file__), "apprise", "apprise-{TYPE}.png"), image_url_mask="https://sabnzbd.org/images/icons/apprise/apprise-{TYPE}.png", image_url_logo="https://sabnzbd.org/images/icons/" "apple-touch-icon-120x120-precomposed.png/apple-touch-icon-180x180-precomposed.png", )

Yes that last one is what I mean. If you open a PR to add them to https://github.com/sabnzbd/sabnzbd.github.io/tree/master/images/icons then it will automatically be available at sabnzbd.org.

So just to follow up with this, we still need the images in this repository (somewhere), because notifications differ from where they get their data from. For Discord, Slack, etc, no problem,; they'll get their image from the web. But other services actually need their image uploaded as part of the payload for it's assembly. So the Apprise plugins are smart enough to determine if they need to go to the cloud for the error/success/info/warning message or to look on the local disk. Otherwise, it looks like we're good to go I see you merged abnzbd.github.io/309

caronc · 2024-02-06T02:30:37Z

I seemed to have broke things with the latest change, i'll have to work it out tomorrow if i have time (otherwise on the weekend). I created a new function called get_target() in the notify.py file and for some reason it can't retrieve the new dynamic fields i generated. Once get_target() works, the documented info in the new up-to-date screenshot should work great.

jcfp · 2024-02-06T07:28:57Z

sabnzbd/notifier.py

+    asset = apprise.AppriseAsset(
+        app_id="SABnzbd",
+        app_desc="SABnzbd Notification",
+        app_url="http://sabnzbd.org/",


Great catch! Done!

Safihre · 2024-02-06T09:19:46Z

interfaces/Config/templates/config_notify.tmpl

+                        </label>
+                        <input type="checkbox" name="${section_label}_target_${type}_enable" id="${section_label}_target_${type}_enable" value="1" <!--#if int($getVar($section_label + '_target_' + $type + '_enable')) > 0 then 'checked="checked"' else ""#--> />
+
+                        <input type="text" name="${section_label}_target_${type}" id="${section_label}_target_${type}" value="$getVar($section_label + '_target_' + $type)" />


Let's add a placeholder="$T('default')" to the overrides input.

sabnzbd/notifier.py

Safihre · 2024-02-06T09:23:25Z

The build broke because you used str | bool | None: which isn't supported on Python 3.8.
You need to use Union[str, bool, None].

Safihre · 2024-02-06T10:20:10Z

@thezoggy sidenote: requests is introduced with this PR so I give up my resistance 😁
Where do you see the main benefit of replacing urllib with requests?

thezoggy · 2024-02-06T22:22:19Z

@thezoggy sidenote: requests is introduced with this PR so I give up my resistance 😁 Where do you see the main benefit of replacing urllib with requests?

we already uses requests in our tests you know. :)

using requests imho is mainly just to use code that just handles all the urllib quirks and made it cleaner to use. which back in the day when we were doing python2 or early python3 it would had prob made things a lot cleaner (which is mainly when I was bringing it up). now with more modern python its a little less important as its more of a glorified wrapper for urllib3, but then there are extensions that solve various scenarios that while prob aren't great use to us but of course are great for things like notification wrappers that need to do various authentication schemes/async threading and such. I would think there prob is benefits to using it to cleanup the getipaddress/urlgrabber/rss stuff...

caronc · 2024-02-07T02:27:05Z

I'll address your concerns tomorrow, unfortunately I couldn't action it tonight.

Safihre · 2024-02-07T05:44:17Z

I'll address your concerns tomorrow, unfortunately I couldn't action it tonight.

No rush, version 4.3.0 is still a while 🤗

caronc · 2024-02-07T20:37:22Z

I think i'm good for another round of reviews. Consider that Apprise supports file attachments too. So if you can think of edge cases (such as a failure) or whatnot where you way wish to also include all of the log files (or a core dump, or whatever you can think of) to be sent with the notification as well, that is very easy to adapt. (just food for thought)

thezoggy · 2024-02-07T22:37:33Z

can squash most of the commits for this to cleanup history a bit

also from earlier review, i noted that that notifier part should be wrapped in try to catch if something goes wrong just like we do with the rest of the notifiers

caronc · 2024-02-07T23:26:24Z

@thezoggy : I'm not sure if try/catch is necessary. Under the hood of notify and add is the catch all.

As for squash, sure, I don't mind doing it, but you can use the Squash & Merge button build right into GitHub too if you want.

Either way, i have no problems actioning both of your requests! 🙂

Edit: Squashed commits into 1
Edit 2: Updated image/graphic on PR (at top) to reflect changes and current implementation

Safihre · 2024-02-08T08:37:04Z

Indeed I always use squash and merge!
The try catch does seem like a good safety guard, as a crash in the notifier could crash a whole job post processing. Just to be safe!

caronc · 2024-02-08T14:21:28Z

a crash in the notifier could crash a whole job post processing.

No problem; try/catch added

Safihre · 2024-02-09T09:27:37Z

sabnzbd/notifier.py

+
+    if not test:
+        # Get a list of tags that are set to use the common list
+        target = get_target(notification_type, "apprise")


Since you use OptionStr, it's guaranteed to be a string.
I would suggest to replace these 3 lines by 1 line using the walrus operator:

if target := get_target(notification_type, "apprise"):

Never used this before; code shifted to conform to your suggestion.

Safihre · 2024-02-09T09:27:39Z

sabnzbd/notifier.py

+
+    try:
+        # The below notifies anything added to our list
+        return (


No need to use 1 liner here, let's just split it.

Safihre · 2024-02-09T09:28:34Z

sabnzbd/notifier.py

+def get_target(notification_type: str, section: str) -> Union[str, bool, None]:
+    """Check target of `notification_type` in `section` if enabled is set"""
+    try:
+        return (


No need to use one-liner here.

Safihre · 2024-02-09T09:29:40Z

sabnzbd/api.py

@@ -657,7 +657,7 @@ def _api_warnings(name, kwargs):
 LOG_JSON_RE = re.compile(rb"'(apikey|api|username|password)': '(.*?)'", re.I)
 LOG_INI_HIDE_RE = re.compile(
    rb"(apikey|api|user|username|password|email_pwd|email_account|email_to|email_from|pushover_token|pushover_userkey"
-    rb"|pushbullet_apikey|prowl_apikey|growl_password|growl_server|IPv[4|6] address|Public address IPv[4|6]-only|Local IPv6 address)\s?=.*",
+    rb"|apprise_urls|pushbullet_apikey|prowl_apikey|growl_password|growl_server|IPv[4|6] address|Public address IPv[4|6]-only|Local IPv6 address)\s?=.*",


Probably we need to add the other specific-URL's?

I think this is done correctly now (i cheated a bit)

Safihre · 2024-02-09T09:32:52Z

sabnzbd/notifier.py

+        app_id="SABnzbd",
+        app_desc="SABnzbd Notification",
+        app_url="https://sabnzbd.org/",
+        image_path_mask=os.path.join(os.path.dirname(__file__), "apprise", "apprise-{TYPE}.png"),


I don't think we should have any images in the /sabnzbd directory, it's only for code.
We should put it in /icons/apprise.

Do you want me to hardcode /icons/apprise or find it relative to the directory. This is the edge case with Apprise, there are like 70 services that just use the icons found on the web, but then there are like 30 that use the local icons stored on the server (True self-hosted vs pulling from the cloud).

This was why i originally stored the images here and then cross-referenced them here via both web and locally (it's now i do it with apprise, so it's just 1 directory to manage - link.

At the end of the day, to truly get the best use of Apprise, you need some images stored locally. If you want, i can just not set a mask over-ride at all, and you'll use the ones that ship with Apprise instead of some custom SABnzbd ones i rigged up quickly?

I can fix this one up when all the code is fine, we have some constants to handle this :)

Safihre

Almost!

Safihre · 2024-02-09T19:26:19Z

sabnzbd/notifier.py

+        if target := get_target(notification_type, "apprise"):
+            if isinstance(target, str):
+                if target:


This can be just 1 line:

Suggested change

if target := get_target(notification_type, "apprise"):

if isinstance(target, str):

if target:

if target := get_target(notification_type, "apprise"):

The only catch is that target can return None, or False. Also if it's an empty string, we want to process it. That means an over-ride was specified, but no URLs were provided (so to use the default). If i do if target :=, how is it going to handle an empty string case (perhaps it's already skipped over at this point, so default logic won't be applied? - in short, i want to get inside that if-block if we get an empty string returned.

I'll see if i can refactor get_target a bet better so it can conform to your minimized example.

Edit: Done

Safihre · 2024-02-09T19:27:42Z

requirements.txt

@@ -54,7 +55,17 @@ notify2==0.3.1; sys_platform != 'win32' and sys_platform != 'darwin'
 # Uncomment line below or manually install after installing requirements.
 # pygobject>=3.10.2; sys_platform != 'win32' and sys_platform != 'darwin'

+# PyYAML does not have binary packages for Mac Users
+PyYAML==6.0.1 --no-binary=PyYAML; sys_platform == 'darwin'
+PyYAML==6.0.1; sys_platform != 'darwin'


We can remove this special darwin stuff here as this is only for run-from-source users, for who pip will get the correct version. The --no-binary in build_release will fix it for the release.

requirements.txt

Safihre · 2024-02-09T19:55:00Z

sabnzbd/notifier.py

+
+    if not test:
+        # Get a list of tags that are set to use the common list
+        if target := get_target(notification_type, "apprise"):


Ah sorry I missed that about the empty string!

no worries; it makes a bit more sense now.. True - use Default, otherwise string means there are URLs to load.

Safihre · 2024-02-13T10:24:26Z

sabnzbd/skintext.py

+    "explain-apprise_urls": TT("Use a comma and/or space to identify more then one URL"),  #: Apprise settings
+    "explain-apprise_extra_urls": TT(
+        "Optionally override the default URL(s) for specific cases. To disable a notification for a specific category:"
+        " simply enable it below, but do not provide it a URL to trigger off of. Alternatively, use comma and/or "


I don't understand this. What can users do here? And how is it different from unchecking a notification-type?

It's the same thing. You can drop this if you like. Was just trying to make it clear that anything specified in the individual entries overrides the default (does not stack with it)

Safihre · 2024-02-13T10:27:32Z

Push a few changes, what do you think? :)
Just this last question.
And we definitely need to update: https://github.com/sabnzbd/sabnzbd.github.io/blob/master/wiki/configuration/4.3/notifications.html

caronc · 2024-02-13T11:25:41Z

requirements.txt

pyYAML is needed by Apprise. Zoggys advice to define these 2 entries was a really good one. I don't think we should remove it from the requirements.txt.

Also, the way it was gave every system that had the ability to leverage the binary package to do so yet still be compatible for the systems that couldn't

It's not removed, it's added a few lines down:
https://github.com/sabnzbd/sabnzbd/pull/2796/files#diff-4d7c51b1efe9043e44439a949dfd92e5827321b34082903477fd04876edb7552R55
But without the special darwin stuff, as this is handled already here:
https://github.com/sabnzbd/sabnzbd/pull/2796/files#diff-ec12760430cbac327a0a49aff7d95b2803a24a5d066c654f37a78f9177fdf36aR108

caronc · 2024-02-13T11:31:57Z

Push a few changes, what do you think? :) Just this last question.

Just the one comment on pyYAML. Not sure about the pyobject changes as it's not really related to this PR. I'll just trust you there. I can't see the GUI/HTML tweaks as I'm on my mobile phone right now, but i have no doubt you nailed them. 🙂 I can try to peek later when I'm at my PC.🚀

And we definitely need to update: https://github.com/sabnzbd/sabnzbd.github.io/blob/master/wiki/configuration/4.3/notifications.html

I'll try to have a look.

Safihre · 2024-02-13T14:06:12Z

@caronc Could you maybe double check if everything still works with my changes? I don't have any of the API-based services myself, only windows:// :)

caronc · 2024-02-13T14:56:58Z

@caronc Could you maybe double check if everything still works with my changes? I don't have any of the API-based services myself, only windows:// :)

Absolutely! I'm at my day job, but I'll do my best to test it when i can and get back you!🚀

Safihre · 2024-02-15T20:09:08Z

@caronc Did you manage to check if it is still working as it should? :)

caronc · 2024-02-15T21:57:02Z

It works fine for me

Safihre · 2024-02-16T11:31:07Z

@caronc Thanks!
You called this PR the Basic integration. What do you think would be the advanced integration?
I also think I will add a warning that users can switch to this integration if we detect that they use sabnzb-notify.py.
We also still need to update the documentation. I created this issue for it: sabnzbd/sabnzbd.github.io#311
Otherwise I'll just forget.

caronc · 2024-02-16T12:58:51Z

I think it started off as being simple, but with notification overrides on each category and references to your own images/icons, you got the full deal. So I'd say it's "Apprise Integration" at this point now 🙂

Safihre · 2024-03-11T13:35:06Z

@caronc Is there a way we can 'unittest' this?
Make it call some endpoint? Just to make sure that the integration is working, even if Apprise is updated.

caronc · 2024-03-12T02:29:44Z

I don't see why we can't. There is almost 100% test coverage with Apprise already

Are you just looking to test the function notifier.send_apprise() function?

Safihre · 2024-03-12T06:36:10Z

Yes, test if there's no crash when actually calling it. Due to missing imports or something like that.
Or you don't have any imports in try/except?

caronc · 2024-04-01T19:10:38Z

Yes, test if there's no crash when actually calling it. Due to missing imports or something like that. Or you don't have any imports in try/except?

Yes, there is a global try/catch inside of the apprise.notify() that catches everything, but i added some test cases anyway to help you still achive what you're asking. 🚀

caronc mentioned this pull request Feb 4, 2024

Support priorities caronc/nzb-notify#81

Open

Safihre requested changes Feb 5, 2024

View reviewed changes

Safihre reviewed Feb 5, 2024

View reviewed changes

jcfp reviewed Feb 6, 2024

View reviewed changes

Safihre reviewed Feb 6, 2024

View reviewed changes

sabnzbd/notifier.py Show resolved Hide resolved

caronc mentioned this pull request Feb 7, 2024

Added Apprise Notification Icons sabnzbd/sabnzbd.github.io#309

Merged

caronc requested a review from Safihre February 7, 2024 21:03

caronc added 2 commits February 7, 2024 19:17

Basic Apprise Integration

f8129e2

Merge branch 'develop' into apprise-notifications

1b04ecb

added try/catch

43847c3

Safihre requested changes Feb 9, 2024

View reviewed changes

comments/code review adapted

e39d44a

Safihre reviewed Feb 9, 2024

View reviewed changes

refactored get_target() to better work with walrus operator

5efb9df

Safihre reviewed Feb 9, 2024

View reviewed changes

caronc added 3 commits February 9, 2024 15:32

Merge branch 'develop' into apprise-notifications

cc97db6

Added explicit requests and markdown dependancies

6cff4ce

requests-oauthlib dep's added

bd882fe

Safihre reviewed Feb 13, 2024

View reviewed changes

Updates

82d1b23

Merge branch 'develop' into apprise-notifications

a34ef89

caronc commented Feb 13, 2024

View reviewed changes

Safihre merged commit ad43a18 into sabnzbd:develop Feb 16, 2024
12 checks passed

caronc deleted the apprise-notifications branch February 17, 2024 16:32

caronc changed the title ~~Basic Apprise Integration~~ Apprise Integration Feb 21, 2024

caronc mentioned this pull request Apr 1, 2024

Added tests cases to wrap calls to the Apprise integration #2831

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2024

Apprise Integration #2796

Apprise Integration #2796

Conversation

caronc commented Feb 3, 2024 • edited

caronc commented Feb 3, 2024

thezoggy commented Feb 3, 2024

thezoggy commented Feb 3, 2024 • edited

caronc commented Feb 3, 2024

thezoggy commented Feb 3, 2024 • edited

Safihre commented Feb 4, 2024

caronc commented Feb 4, 2024 • edited

caronc commented Feb 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caronc commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Safihre commented Feb 6, 2024 • edited

Safihre commented Feb 6, 2024

thezoggy commented Feb 6, 2024

caronc commented Feb 7, 2024

Safihre commented Feb 7, 2024

caronc commented Feb 7, 2024

thezoggy commented Feb 7, 2024

caronc commented Feb 7, 2024 • edited

Safihre commented Feb 8, 2024

caronc commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Safihre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caronc Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Safihre commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caronc commented Feb 13, 2024

Safihre commented Feb 13, 2024

caronc commented Feb 13, 2024

Safihre commented Feb 15, 2024

caronc commented Feb 15, 2024

Safihre commented Feb 16, 2024

caronc commented Feb 16, 2024

Safihre commented Mar 11, 2024

caronc commented Mar 12, 2024

Safihre commented Mar 12, 2024

caronc commented Apr 1, 2024

caronc commented Feb 3, 2024 •

edited

thezoggy commented Feb 3, 2024 •

edited

thezoggy commented Feb 3, 2024 •

edited

caronc commented Feb 4, 2024 •

edited

Safihre commented Feb 6, 2024 •

edited

caronc commented Feb 7, 2024 •

edited

caronc Feb 9, 2024 •

edited