From a235d9d5abe623683d26867ada8929e7b9975d90 Mon Sep 17 00:00:00 2001 From: Chris Caron Date: Sat, 11 Jan 2020 18:39:53 -0500 Subject: [PATCH] Attachment consistency across supported notification services (#194) * some test cleanup * Attachment handling more consistent across plugins --- apprise/plugins/NotifyDiscord.py | 14 +++++++ apprise/plugins/NotifyEmail.py | 22 +++++------ apprise/plugins/NotifyPushBullet.py | 15 ++++++- apprise/plugins/NotifyPushSafer.py | 11 +++++- apprise/plugins/NotifyPushover.py | 8 +++- apprise/plugins/NotifySlack.py | 14 ++++++- apprise/plugins/NotifyTelegram.py | 24 +++++++----- test/test_discord_plugin.py | 28 ++++++++++--- test/test_pushbullet.py | 61 +++++++++++++++++++++++++++++ test/test_pushsafer.py | 2 +- test/test_rest_plugins.py | 10 ++--- test/test_slack_plugin.py | 38 ++++++++++++++++++ test/test_telegram.py | 7 ++-- 13 files changed, 210 insertions(+), 44 deletions(-) diff --git a/apprise/plugins/NotifyDiscord.py b/apprise/plugins/NotifyDiscord.py index af6bafd4..254d9285 100644 --- a/apprise/plugins/NotifyDiscord.py +++ b/apprise/plugins/NotifyDiscord.py @@ -51,6 +51,7 @@ from ..common import NotifyType from ..utils import parse_bool from ..utils import validate_regex from ..AppriseLocale import gettext_lazy as _ +from ..attachment.AttachBase import AttachBase class NotifyDiscord(NotifyBase): @@ -312,6 +313,19 @@ class NotifyDiscord(NotifyBase): # Always call throttle before any remote server i/o is made self.throttle() + # Perform some simple error checking + if isinstance(attach, AttachBase): + if not attach: + # We could not access the attachment + self.logger.error( + 'Could not access attachment {}.'.format( + attach.url(privacy=True))) + return False + + self.logger.debug( + 'Posting Discord attachment {}'.format( + attach.url(privacy=True))) + # Our attachment path (if specified) files = None try: diff --git a/apprise/plugins/NotifyEmail.py b/apprise/plugins/NotifyEmail.py index d903ca55..222e32e4 100644 --- a/apprise/plugins/NotifyEmail.py +++ b/apprise/plugins/NotifyEmail.py @@ -573,21 +573,22 @@ class NotifyEmail(NotifyBase): # First attach our body to our content as the first element base.attach(content) - attach_error = False - # Now store our attachments for attachment in attach: if not attachment: # We could not load the attachment; take an early # exit since this isn't what the end user wanted - self.logger.warning( - 'The specified attachment could not be referenced:' - ' {}.'.format(attachment.url(privacy=True))) + # We could not access the attachment + self.logger.error( + 'Could not access attachment {}.'.format( + attachment.url(privacy=True))) - # Mark our failure - attach_error = True - break + return False + + self.logger.debug( + 'Preparing Email attachment {}'.format( + attachment.url(privacy=True))) with open(attachment.path, "rb") as abody: app = MIMEApplication( @@ -600,11 +601,6 @@ class NotifyEmail(NotifyBase): base.attach(app) - if attach_error: - # Mark our error and quit early - has_error = True - break - # bind the socket variable to the current namespace socket = None diff --git a/apprise/plugins/NotifyPushBullet.py b/apprise/plugins/NotifyPushBullet.py index af239c40..4a3dd849 100644 --- a/apprise/plugins/NotifyPushBullet.py +++ b/apprise/plugins/NotifyPushBullet.py @@ -147,6 +147,19 @@ class NotifyPushBullet(NotifyBase): # We need to upload our payload first so that we can source it # in remaining messages for attachment in attach: + + # Perform some simple error checking + if not attachment: + # We could not access the attachment + self.logger.error( + 'Could not access attachment {}.'.format( + attachment.url(privacy=True))) + return False + + self.logger.debug( + 'Preparing PushBullet attachment {}'.format( + attachment.url(privacy=True))) + # prepare payload payload = { 'file_name': attachment.name, @@ -253,7 +266,7 @@ class NotifyPushBullet(NotifyBase): continue self.logger.info( - 'Sent PushBullet attachment (%s) to "%s".' % ( + 'Sent PushBullet attachment ({}) to "{}".'.format( attach_payload['file_name'], recipient)) return not has_error diff --git a/apprise/plugins/NotifyPushSafer.py b/apprise/plugins/NotifyPushSafer.py index a56b28f0..8e056087 100644 --- a/apprise/plugins/NotifyPushSafer.py +++ b/apprise/plugins/NotifyPushSafer.py @@ -548,15 +548,22 @@ class NotifyPushSafer(NotifyBase): # prepare payload if not attachment: # We could not access the attachment - self.logger.warning( - 'Could not access {}.'.format( + self.logger.error( + 'Could not access attachment {}.'.format( attachment.url(privacy=True))) return False if not attachment.mimetype.startswith('image/'): # Attachment not supported; continue peacefully + self.logger.debug( + 'Ignoring unsupported PushSafer attachment {}.'.format( + attachment.url(privacy=True))) continue + self.logger.debug( + 'Posting PushSafer attachment {}'.format( + attachment.url(privacy=True))) + try: with io.open(attachment.path, 'rb') as f: # Output must be in a DataURL format (that's what diff --git a/apprise/plugins/NotifyPushover.py b/apprise/plugins/NotifyPushover.py index 0d7c46db..48bcb786 100644 --- a/apprise/plugins/NotifyPushover.py +++ b/apprise/plugins/NotifyPushover.py @@ -364,8 +364,8 @@ class NotifyPushover(NotifyBase): # Perform some simple error checking if not attach: # We could not access the attachment - self.logger.warning( - 'Could not access {}.'.format( + self.logger.error( + 'Could not access attachment {}.'.format( attach.url(privacy=True))) return False @@ -398,6 +398,10 @@ class NotifyPushover(NotifyBase): return False + self.logger.debug( + 'Posting Pushover attachment {}'.format( + attach.url(privacy=True))) + # Default Header headers = { 'User-Agent': self.app_id, diff --git a/apprise/plugins/NotifySlack.py b/apprise/plugins/NotifySlack.py index e16885e6..6a05bcbd 100644 --- a/apprise/plugins/NotifySlack.py +++ b/apprise/plugins/NotifySlack.py @@ -435,8 +435,18 @@ class NotifySlack(NotifyBase): if attach and self.mode is SlackMode.BOT and attach_channel_list: # Send our attachments (can only be done in bot mode) for attachment in attach: - self.logger.info( - 'Posting Slack Attachment {}'.format(attachment.name)) + + # Perform some simple error checking + if not attachment: + # We could not access the attachment + self.logger.error( + 'Could not access attachment {}.'.format( + attachment.url(privacy=True))) + return False + + self.logger.debug( + 'Posting Slack attachment {}'.format( + attachment.url(privacy=True))) # Prepare API Upload Payload _payload = { diff --git a/apprise/plugins/NotifyTelegram.py b/apprise/plugins/NotifyTelegram.py index 1928480e..73bdf658 100644 --- a/apprise/plugins/NotifyTelegram.py +++ b/apprise/plugins/NotifyTelegram.py @@ -267,18 +267,22 @@ class NotifyTelegram(NotifyBase): path = None if isinstance(attach, AttachBase): + if not attach: + # We could not access the attachment + self.logger.error( + 'Could not access attachment {}.'.format( + attach.url(privacy=True))) + return False + + self.logger.debug( + 'Posting Telegram attachment {}'.format( + attach.url(privacy=True))) + # Store our path to our file path = attach.path file_name = attach.name mimetype = attach.mimetype - if not path: - # We could not access the attachment - self.logger.warning( - 'Could not access {}.'.format( - attach.url(privacy=True))) - return False - # Process our attachment function_name, key = \ next(((x['function_name'], x['key']) for x in self.mime_lookup @@ -642,10 +646,10 @@ class NotifyTelegram(NotifyBase): if attach: # Send our attachments now (if specified and if it exists) for attachment in attach: - sent_attachment = self.send_media( - payload['chat_id'], notify_type, attach=attachment) + if not self.send_media( + payload['chat_id'], notify_type, + attach=attachment): - if not sent_attachment: # We failed; don't continue has_error = True break diff --git a/test/test_discord_plugin.py b/test/test_discord_plugin.py index a29e0bec..7cb5ea97 100644 --- a/test/test_discord_plugin.py +++ b/test/test_discord_plugin.py @@ -174,11 +174,8 @@ def test_discord_attachments(mock_post): webhook_token = 'D' * 64 # Prepare Mock return object - response = mock.Mock() - response.status_code = requests.codes.ok - - # Throw an exception on the second call to requests.post() - mock_post.side_effect = [response, OSError()] + mock_post.return_value = requests.Request() + mock_post.return_value.status_code = requests.codes.ok # Test our markdown obj = Apprise.instantiate( @@ -187,5 +184,26 @@ def test_discord_attachments(mock_post): # attach our content attach = AppriseAttachment(os.path.join(TEST_VAR_DIR, 'apprise-test.gif')) + assert obj.notify( + body='body', title='title', notify_type=NotifyType.INFO, + attach=attach) is True + + # An invalid attachment will cause a failure + path = os.path.join(TEST_VAR_DIR, '/invalid/path/to/an/invalid/file.jpg') + attach = AppriseAttachment(path) + assert obj.notify( + body='body', title='title', notify_type=NotifyType.INFO, + attach=path) is False + + # Throw an exception on the second call to requests.post() + mock_post.return_value = None + response = mock.Mock() + response.status_code = requests.codes.ok + mock_post.side_effect = [response, OSError()] + + # update our attachment to be valid + attach = AppriseAttachment(os.path.join(TEST_VAR_DIR, 'apprise-test.gif')) + # Test our markdown + # We'll fail now because of an internal exception assert obj.send(body="test", attach=attach) is False diff --git a/test/test_pushbullet.py b/test/test_pushbullet.py index 67b9147d..5004f865 100644 --- a/test/test_pushbullet.py +++ b/test/test_pushbullet.py @@ -59,6 +59,7 @@ def test_pushbullet_attachments(mock_post): "upload_url": "https://upload.pushbullet.com/abcd123", }) response.status_code = requests.codes.ok + mock_post.return_value = response # prepare our attachment attach = AppriseAttachment(os.path.join(TEST_VAR_DIR, 'apprise-test.gif')) @@ -67,7 +68,23 @@ def test_pushbullet_attachments(mock_post): obj = Apprise.instantiate( 'pbul://{}/?format=markdown'.format(access_token)) + # Send a good attachment + assert obj.notify(body="test", attach=attach) is True + + # Add another attachment so we drop into the area of the PushBullet code + # that sends remaining attachments (if more detected) + attach.add(os.path.join(TEST_VAR_DIR, 'apprise-test.gif')) + + # Send our attachments + assert obj.notify(body="test", attach=attach) is True + + # An invalid attachment will cause a failure + path = os.path.join(TEST_VAR_DIR, '/invalid/path/to/an/invalid/file.jpg') + attach = AppriseAttachment(path) + assert obj.notify(body="test", attach=attach) is False + # Throw an exception on the first call to requests.post() + mock_post.return_value = None mock_post.side_effect = requests.RequestException() # We'll fail now because of an internal exception @@ -99,3 +116,47 @@ def test_pushbullet_attachments(mock_post): # We'll fail because of an invalid json object assert obj.send(body="test", attach=attach) is False + + # + # Test bad responses + # + + # Prepare a bad response + response.content = dumps({ + "file_name": "cat.jpg", + "file_type": "image/jpeg", + "file_url": "https://dl.pushb.com/abc/cat.jpg", + "upload_url": "https://upload.pushbullet.com/abcd123", + }) + bad_response = mock.Mock() + bad_response.content = response.content + bad_response.status_code = 400 + + # Throw an exception on the third call to requests.post() + mock_post.return_value = bad_response + + # prepare our attachment + attach = AppriseAttachment(os.path.join(TEST_VAR_DIR, 'apprise-test.gif')) + + # We'll fail now because we were unable to send the attachment + assert obj.send(body="test", attach=attach) is False + + # Throw an exception on the second call + mock_post.side_effect = [response, bad_response, response] + assert obj.send(body="test", attach=attach) is False + + # Throw an OSError + mock_post.side_effect = [response, OSError()] + assert obj.send(body="test", attach=attach) is False + + # Throw an exception on the third call + mock_post.side_effect = [response, response, bad_response] + assert obj.send(body="test", attach=attach) is False + + # Throw an exception on the fourth call + mock_post.side_effect = [response, response, response, bad_response] + assert obj.send(body="test", attach=attach) is False + + # A good message + mock_post.side_effect = [response, response, response, response] + assert obj.send(body="test", attach=attach) is True diff --git a/test/test_pushsafer.py b/test/test_pushsafer.py index 2e845223..7bbe6f1a 100644 --- a/test/test_pushsafer.py +++ b/test/test_pushsafer.py @@ -40,7 +40,7 @@ TEST_VAR_DIR = os.path.join(os.path.dirname(__file__), 'var') @mock.patch('requests.post') -def test_notify_pushsafer_plugin(mock_post, tmpdir): +def test_notify_pushsafer_plugin(mock_post): """ API: NotifyPushSafer() Tests diff --git a/test/test_rest_plugins.py b/test/test_rest_plugins.py index a7953921..f2781a02 100644 --- a/test/test_rest_plugins.py +++ b/test/test_rest_plugins.py @@ -4210,11 +4210,11 @@ def test_rest_plugins(mock_post, mock_get): # Check that we were expecting this exception to happen try: if not isinstance(e, response): - raise + raise e except TypeError: print('%s Unhandled response %s' % (url, type(e))) - raise + raise e # # Stage 2: without title defined @@ -4250,7 +4250,7 @@ def test_rest_plugins(mock_post, mock_get): except Exception as e: # Check that we were expecting this exception to happen if not isinstance(e, response): - raise + raise e # Tidy our object and allow any possible defined deconstructors to # be executed. @@ -4265,11 +4265,11 @@ def test_rest_plugins(mock_post, mock_get): # Handle our exception if instance is None: print('%s %s' % (url, str(e))) - raise + raise e if not isinstance(e, instance): print('%s %s' % (url, str(e))) - raise + raise e @mock.patch('requests.get') diff --git a/test/test_slack_plugin.py b/test/test_slack_plugin.py index f8d334c5..9ee6b67e 100644 --- a/test/test_slack_plugin.py +++ b/test/test_slack_plugin.py @@ -22,12 +22,14 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN # THE SOFTWARE. +import os import six import mock import pytest import requests from apprise import plugins from apprise import NotifyType +from apprise import AppriseAttachment from json import dumps @@ -35,6 +37,9 @@ from json import dumps import logging logging.disable(logging.CRITICAL) +# Attachment Directory +TEST_VAR_DIR = os.path.join(os.path.dirname(__file__), 'var') + @mock.patch('requests.post') def test_slack_oauth_access_token(mock_post): @@ -52,6 +57,11 @@ def test_slack_oauth_access_token(mock_post): request.content = dumps({ 'ok': True, 'message': '', + + # Attachment support + 'file': { + 'url_private': 'http://localhost', + } }) request.status_code = requests.codes.ok @@ -73,6 +83,34 @@ def test_slack_oauth_access_token(mock_post): # apprise room was found assert obj.send(body="test") is True + # Test Valid Attachment + path = os.path.join(TEST_VAR_DIR, 'apprise-test.gif') + attach = AppriseAttachment(path) + assert obj.notify( + body='body', title='title', notify_type=NotifyType.INFO, + attach=attach) is True + + # Test invalid attachment + path = os.path.join(TEST_VAR_DIR, '/invalid/path/to/an/invalid/file.jpg') + assert obj.notify( + body='body', title='title', notify_type=NotifyType.INFO, + attach=path) is False + + # Test case where expected return attachment payload is invalid + request.content = dumps({ + 'ok': True, + 'message': '', + + # Attachment support + 'file': None + }) + path = os.path.join(TEST_VAR_DIR, 'apprise-test.gif') + attach = AppriseAttachment(path) + # We'll fail because of the bad 'file' response + assert obj.notify( + body='body', title='title', notify_type=NotifyType.INFO, + attach=attach) is False + # Slack requests pay close attention to the response to determine # if things go well... this is not a good JSON response: request.content = '{' diff --git a/test/test_telegram.py b/test/test_telegram.py index af57b5ca..dce9458d 100644 --- a/test/test_telegram.py +++ b/test/test_telegram.py @@ -43,7 +43,7 @@ TEST_VAR_DIR = os.path.join(os.path.dirname(__file__), 'var') @mock.patch('requests.get') @mock.patch('requests.post') -def test_notify_telegram_plugin(mock_post, mock_get, tmpdir): +def test_notify_telegram_plugin(mock_post, mock_get): """ API: NotifyTelegram() Tests @@ -205,13 +205,14 @@ def test_notify_telegram_plugin(mock_post, mock_get, tmpdir): assert len(obj.targets) == 1 assert obj.targets[0] == '12345' - path = os.path.join(TEST_VAR_DIR, 'apprise-test.gif') - attach = AppriseAttachment(path) + attach = AppriseAttachment(os.path.join(TEST_VAR_DIR, 'apprise-test.gif')) assert obj.notify( body='body', title='title', notify_type=NotifyType.INFO, attach=attach) is True + # An invalid attachment will cause a failure path = os.path.join(TEST_VAR_DIR, '/invalid/path/to/an/invalid/file.jpg') + attach = AppriseAttachment(path) assert obj.notify( body='body', title='title', notify_type=NotifyType.INFO, attach=path) is False