caronc / apprise

Apprise - Push Notifications that work with just about every platform!
https://hub.docker.com/r/caronc/apprise
BSD 2-Clause "Simplified" License
10.91k stars 384 forks source link

Attachments should honor overflow=truncate setting (and not send more then 1) #1091

Closed pomeloy closed 2 months ago

pomeloy commented 3 months ago

:bulb: The Idea Pushover does not support messages with multiple attachments. As a workaround, the Apprise plugin currently splits a given notification into multiple messages when passed a list of attachments. This is a great way to circumvent this limitation, but sometimes results in confusion when receiving multiple messages with more than one attachment in a short period of time.

There should be a new, optional parameter turning this behavior off. This would enhance user experience especially when using the Pushover plugin in parallel to another notification service that natively supports multiple attachments.

:hammer: Breaking Feature No, as the new parameter should be optional.

caronc commented 2 months ago

I think you really stumbled onto somthing here. I'd like you to get credit for the fix but i don't quite think what you submitted is what we need. It's still a bit too complicated. Attached is a .patch file. apprise-truncate-attachment.patch.gz

gunzip  /path/to/apprise-truncate-attachment.patch.gz
git apply /path/to/apprise-truncate-attachment.patch

It's based on our discussion, but a more clean lightweight version. I also added some unit tests to verify it works correctly. Please make a PR with this and i'll gladly accept it. It's a great find on your part.

This will enforce that ?upstream=truncate applied to any URL (a per-url basis) doesn't allow a chain of attachments to roll through beyond the first.

pomeloy commented 2 months ago

Thanks so much for helping me out with the PR! I'm still not too versed in all of this. Just to clarify: would you still define a new attachment_maxcount (or however it should be called) variable for the plugin? And with the patch the SPLIT option would not get honored concerning attachments, is that something you would still let the specific notifier service plugin handle? (such as is the case right now with the Pushover plugin)

caronc commented 2 months ago

I would roll back and just apply the attached patch as it will solve your problem. Or just close your PR and start a fresh one.

While the is nothing wrong with your solution, it's a bit more code then what is needed to solve the problem unless there is something I'm missing?

pomeloy commented 2 months ago

Obviously your solution is much cleaner. I just thought that maybe you were keen on also having the overflow=split option respected when handling the attachments. Personally, your patch is totally sufficient for me.

caronc commented 2 months ago

Closing this off as i think we nailed this! Thank you again for your issue and help! Don't ever hesitate to reach out if you find anything else! :slightly_smiling_face: