Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revolt Support #1057

Merged
merged 15 commits into from Feb 12, 2024
Merged

Revolt Support #1057

merged 15 commits into from Feb 12, 2024

Conversation

ktwrd
Copy link
Contributor

@ktwrd ktwrd commented Feb 6, 2024

Description:

Revolt Apprise Support Added

Setup

  • Create a bot account (Settings -> Bot -> Create Bot)
  • Invite bot to a server you're an admin on
  • Create a channel that the bot can access
  • Copy Bot Token and Channel Id (right-click on Channel then click Copy Id) and put in test command below

New Service Completion Status

  • apprise/plugins/NotifyRevolt.py
  • KEYWORDS
    • add new service into this file (alphabetically).
  • README.md
    • add entry for new service to table (as a quick reference)
  • packaging/redhat/python-apprise.spec
    • add new service into the %global common_description

Checklist

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

Testing

I am unable to test this myself since my account got terminated a few months ago, but I know the API like it's the back of my hand. (Don't ask questions why, stay on-topic to the PR please)

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/ktwrd/apprise.git@revolt-support

# Test out the changes with the following command:
apprise -t "Test Title" -b "Test Message" \
  "revolt://bot_token/channel_id"

@caronc
Copy link
Owner

caronc commented Feb 6, 2024

This is amazing! Thank you for this PR! I'll do a better review of it tomorrow, but just at a glance through my phone, it seems you did everything! 🚀🚀🙏🙏🎉🎉

Very impressive!

@ktwrd
Copy link
Contributor Author

ktwrd commented Feb 6, 2024

This is amazing! Thank you for this PR! I'll do a better review of it tomorrow, but just at a glance through my phone, it seems you did everything! 🚀🚀🙏🙏🎉🎉

Very impressive!

Thanks!

I'm a wee bit surprised that the unit testing worked as intended (copy & pasted from discord but removed markdown testing) since it's my first time contributing to a python project in quite a while.

I didn't implement sending attachments because the API is a bit weird for that and afaik Revolt's Websockets (which is still WIP) doesn't support that, and only bot/user accounts support it.

It would be a bit annoying to refer to the attachment documentation since that's not really a thing, and the only way I figured it out when I was making my C# library a while ago, was to look at the source code for autumn.

@caronc
Copy link
Owner

caronc commented Feb 7, 2024

seems a couple of test cases are failing, i can help have a peak with you tomorrow.

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e9beea2) 99.27% compared to head (4a65913) 99.26%.

Files Patch % Lines
apprise/plugins/NotifyRevolt.py 97.69% 1 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1057      +/-   ##
==========================================
- Coverage   99.27%   99.26%   -0.02%     
==========================================
  Files         135      136       +1     
  Lines       17603    17733     +130     
  Branches     3593     3616      +23     
==========================================
+ Hits        17475    17602     +127     
- Misses        119      120       +1     
- Partials        9       11       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@caronc
Copy link
Owner

caronc commented Feb 8, 2024

@ktwrd ; i made some small changes to grant you 100% test coverage. The only thing i can't confirm is if it works. i presume it works for you when you try it out in a development environment?

I notice you have:

    # The 2000 characters above defined by the body_maxlen include that of the
    # title.  Setting this to True ensures overflow options behave properly
    overflow_amalgamate_title = True

Is this your intention; this was an edge case for Telegram, but didnt' apply to many other plugins. I'm guessing it impacts you too? Hence 2000 characters is the limit, no matter what... if a title exists of 1000 characters (for whatever reason), you will only have 1000 characters left to provide with the body? I only ask because i can see in your payload the title and body are in different parts of the payload... - just confirming#1057

@ktwrd
Copy link
Contributor Author

ktwrd commented Feb 8, 2024

@ktwrd ; i made some small changes to grant you 100% test coverage. The only thing i can't confirm is if it works. i presume it works for you when you try it out in a development environment?

I notice you have:

    # The 2000 characters above defined by the body_maxlen include that of the
    # title.  Setting this to True ensures overflow options behave properly
    overflow_amalgamate_title = True

Is this your intention; this was an edge case for Telegram, but didnt' apply to many other plugins. I'm guessing it impacts you too? Hence 2000 characters is the limit, no matter what... if a title exists of 1000 characters (for whatever reason), you will only have 1000 characters left to provide with the body? I only ask because i can see in your payload the title and body are in different parts of the payload... - just confirming#1057

I was unsure how to implement this (since I copy & pasted a lot from Discord) but revolt does have a 100 character limit for embed titles.

@caronc
Copy link
Owner

caronc commented Feb 8, 2024

Perfect, fixed in last commit for you! body_maxlen of 2000 is okay as well?

I'll let you test out your configuration, the easiest way to do it is via Docker (see here) but basically:

docker-compose run --rm test.py311 bash

# inside container
bin/apprise -b "test" -vv revolt://credentials 

# boom; see if it works for you :)

Edit: @ktwrd Hence please confirm to me this works for you for the final merge

@ktwrd
Copy link
Contributor Author

ktwrd commented Feb 12, 2024

@caronc It works!
screenshot

@caronc caronc merged commit 6764590 into caronc:master Feb 12, 2024
12 checks passed
@caronc
Copy link
Owner

caronc commented Feb 12, 2024

Merged! 🙏🚀

@caronc caronc mentioned this pull request Feb 18, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants