Prevent Pushover from discarding notifications with unsupported attachments

[YoRyan]

Description:

When processing a Pushover notification with an unsupported (non-image) attachment, Apprise does not drop the attachment and send the notification without it, as intended. Instead, it silently discards the entire notification:

$ apprise -vvv -b 'Hello, World!' -t 'Test Notification' -a ./README.md pover://${userToken}@${appToken}
2022-08-22 02:44:54,449 - DEBUG - Loaded Pushover URL: pover://[email protected]//?priority=normal&format=text&overflow=upstream&rto=4.0&cto=4.0&verify=yes
2022-08-22 02:44:54,449 - DEBUG - Loading attachment: ./README.md
2022-08-22 02:44:54,452 - DEBUG - Using selector: EpollSelector
2022-08-22 02:44:54,452 - INFO - Notifying 1 service(s) asynchronously.
2022-08-22 02:44:54,454 - DEBUG - Ignored unsupported Pushover attachment (text/markdown): file://./README.md
(command exits; notification not sent)

This issue can be fixed by factoring out a premature return statement.

Checklist

• [x] The code change is tested and works locally.
• [x] There is no commented out code in this PR.
• [x] No lint errors (use flake8)
• [x] 100% test coverage

Testing

Anyone can help test this source code as follows:

# Create a virtual environment to work in as follows:
python3 -m venv apprise

# Change into our new directory
cd apprise

# Activate our virtual environment
source bin/activate

# Install the branch
pip install git+https://github.com/caronc/apprise.git@<this.branch-name>

# Test out the changes with the following command:
apprise -vvv -t "Test Title" -b "Test Message" -a "./README.md" pover://${userToken}@${appToken}

[caronc]

Hi @YoRyan , I'll have to think about this.

The design in Apprise was actually the invert of what this patch does.

Think of a situation where you're sending 7 different endpoints with an attachment. Perhaps a core dump file or whatever. I can't think of a reason why you would just send the text "check this out" (as an example) without the attachment.

I also based it on the likes of Google mail. If the attachment is to big (for example), you can't send the email.

I kind of like the logic to skip over the services that don't support attachments vs sending just the body only.

Maybe this should be a configuration... Or a switch in the AppriseAsset class to enable/disable the outcome you're expecting?

Thoughts?

[YoRyan]

Well, the UX for this scenario isn't great: If Pushover is your sole target, Apprise will not only refuse to produce any notifications - it will also not notify you of the incompatible attachments, because it will report a non-error (True) condition to the caller. To my mind, True should mean "at least one notification was successfully created," which isn't the case here. And in my case, the app I'm using with Apprise insists on attaching incompatible files to its notifications; it is not possible to change this without utilizing my dev skills, as I'm sure is the case for many similar apps. Indeed, until I activated debug logging, I had no clue Apprise was working at all, as I was completely baffled by the lack of any notifications being produced.

The behavior of the Pushover plugin also seems to be at odds with that of the other plugins. The Twitter and PushSafer plugins also filter for unsupported attachments, but they retain the notification text, even if the attachment(s) are rejected. Meanwhile, plugins like Windows that do not support any attachments also happily continue to notify with text only. So Pushover's "eat the notification, still do not report an error" logic seems to be the outlier here.

In my opinion, the Pushover plugin should be made consistent with the behavior of the other plugins. Or it should at least report this as a failure condition, so that the user knows that something is amiss. Perhaps the two-state True/False return status could be reexamined as well; we could take inspiration from asyncio.gather(), which performs a similar function to Apprise.notify() - namely, firing off one or multiple concurrent tasks - but returns distinct results for each individual task.

[caronc]

Fair enough; i think this PR should perhaps also make it so all of the other plugins are consistent with this though.

[codecov-commenter]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (593d2c4) compared to base (f1990b1). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #650   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          112       112           
  Lines        14603     14603           
  Branches      2775      2775           
=========================================
  Hits         14603     14603           

Impacted Files	Coverage Δ
apprise/plugins/NotifyPushover.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[YoRyan]

If you do a grep for .mimetype in ./apprise/plugins, you'll find the following plugins filter attachments against their MIME type:

• NotifyPushover.py (rejects attachment, returns True, no notification)
• NotifyPushSafer.py (skips attachment, continues with notification)
• NotifySMSEagle.py (skips attachment, continues with notification)
• NotifyTwitter.py (skips attachment, continues with notification)

This PR would merely make Pushover consistent with Push Safer, SMS Eagle, and Twitter.