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

✨ Discord notification provider #280

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ajvpot
Copy link

@ajvpot ajvpot commented Jul 5, 2023

Description

Adds a notification provider for Discord. The user needs to provide a webhook URL similar to Slack.

Has This Been Tested?

Yes

image

Checklist:

  • I've read the contributing guidelines of this project
  • I've installed and used .pre_commit on all my code
    The linter returns some errors on code I didn't change in this PR, ignored those.
  • I have documented my code, particularly in hard-to-understand areas
  • I have made any necessary corresponding changes to the documentation

@ajvpot ajvpot changed the title Discord notification provider ✨ Add Discord notification provider Jul 5, 2023
@ajvpot ajvpot force-pushed the discord_provider branch from 25d53ed to 85f9a0c Compare July 5, 2023 23:39
@ajvpot ajvpot changed the title ✨ Add Discord notification provider ✨ Discord notification provider Jul 5, 2023
@ajvpot ajvpot force-pushed the discord_provider branch from 85f9a0c to 2ab5a06 Compare July 6, 2023 00:00
@ajvpot ajvpot force-pushed the discord_provider branch from 2ab5a06 to 13a2e75 Compare July 6, 2023 02:05
@juftin
Copy link
Owner

juftin commented Jul 9, 2023

Love this. I'm on an extended trip right now so I'm be a bit slow for review. I will be back home and have final review on this within a couple weeks. Everything looks very good though, thank you for the contribution!

@ajvpot
Copy link
Author

ajvpot commented Aug 6, 2023

Hey @juftin, hope your vacation went well! Please let me know if there's anything I can update on this.

Comment on lines +70 to +99
# Remove items that will be templated as part of the embed.
del formatted_dict["Recreation Area"]
del formatted_dict["Booking Date"]
del formatted_dict["Booking End Date"]
del formatted_dict["Facility Name"]
del formatted_dict["Booking Link"]
del formatted_dict["Campsite Site Name"]
del formatted_dict["Campsite Loop Name"]
del formatted_dict["Recreation Area Id"]
del formatted_dict["Facility Id"]
del formatted_dict["Campsite Id"]

return {
"author": {
"name": f"🏕 {campsite.recreation_area}"
},
"title": f"{campsite.facility_name} {campsite.campsite_loop_name} #{campsite.campsite_site_name}",
"description": f"{campsite.booking_date:%Y/%m/%d} to {campsite.booking_end_date:%Y/%m/%d}",
"url": campsite.booking_url,
"color": 2375436,
"fields": [
{
"name": key,
"value": str(value)
} for key, value in formatted_dict.items()
],
"footer": {
"text": "camply, the campsite finder ⛺️"
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple notes on this - I think there are a good number of fields that we want to keep from our formatted_dict here - although it can be quite verbose when looking at the notifications the extra context can be useful.

  • This title field here assumes that the campsite has a numeric campsite_loop_name - but in a good number of campsites this is a string value.
  • There are provider's who don't allow the passing of campsite ID via the booking URL - for those cases we'll need to keep the campsite ID in the message
  • Is it possible to have a notification open in a "collapsed" view that can be expanded? This would be nice to have the default view be simple, but with expanded details.

See here for the test-notifications CLI command:

camply test-notifications --notifications discord
image

Here's the same notification, that instead uses message_title and doesn't remove all the fields - I think some of these can be omitted though like Booking Date / Booking URL which are provided elsewhere
image

def __init__(self):
super().__init__()
self.session.headers.update({"Content-Type": "application/json"})
if any([DiscordConfig.DISCORD_WEBHOOK is None, DiscordConfig.DISCORD_WEBHOOK == ""]):
Copy link
Owner

@juftin juftin Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple lines like this that haven't been formatted by black / ruff - that's why we're seeing linting errors.

pre-commit resolves this with hatch - so either of these commands will format the codebase for you:

hatch run format
pre-commit run --all-files


This part is kind of confusing. CI/CD runs both linting (black/ruff) and type checking (mypy) on the codebase inside the Lint job - but only linting errors actually cause it to fail. type-checking issues (on related and unrelated code) will still show up as annotations on the PR but won't actually cause the Lint job to fail.

image

@samuelrey
Copy link

Any updates here? The discord integration would be useful. Happy to pick this up & run, but def don't want to step on any feet!

@juftin
Copy link
Owner

juftin commented Jun 29, 2024

I haven't spent any time on this, I can take a closer look soon though. In the meantime Discord notifications are supported via the AppRise notification provider

https://juftin.com/camply/command_line_usage/#send-a-notification-using-apprise-compatible-services

https://github.com/caronc/apprise

@samuelrey
Copy link

This was my first time using Apprise, but it was easier to setup than I thought.

There may be some additional behavior that this PR adds, but I haven't read it in depth at this point.

For future readers, create a webhook on your discord server, copy the URL and set APPRISE_URL with that value (either with camply configure or export APPRISE_URL={my_discord_webhook}). You can then run the test-notification command straight away.

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.

3 participants