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

added discord webhook functionality #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

harbar20
Copy link

Hi!

I thought it'd be cool to add Discord functionality so that people who don't use Twitter or Slack have an option.

I've never used Ruby before whatsoever, so if there are any mistakes, let me know so I can fix them. Thanks!

@harbar20
Copy link
Author

I don't get how to fix that name bug. what do i do?

Copy link
Member

@dpca dpca left a comment

Choose a reason for hiding this comment

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

Thanks for adding this functionality! I haven't made a discord bot before but overall this approach looks like it will work. I added some comments to help get you unstuck with the name bug, but let me know if there's anything that's confusing or unclear and I can elaborate more.


def update(str)
@logger.info "[FakeDiscord]: #{str}"
end
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this class is missing an end statement, which may be causing some of the name errors. In Ruby, each class and function definition must have a corresponding end to close it out.

def initialize(logger)
@logger = logger
@discord = if ENV['ENVIRONMENT'] != 'test' && env_keys_exist?
Discordrb::Webhooks::Client.new(url: ENV['DISCORD_WEBHOOK_URL']).freeze
Copy link
Member

Choose a reason for hiding this comment

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

If the environment keys don't exist, we should instantiate a FakeDiscord instance here which just logs the output instead of sending to discord. Very useful for testing purposes.

@@ -0,0 +1,53 @@
require 'discordrb/webhooks'

class FakeDiscord
Copy link
Member

Choose a reason for hiding this comment

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

The FakeDiscord class should respond to the same commands as the real class, which from looking below are execute and then content = to set the text. Is there an easier way than setting execute and using the webhook API? Usually there's a REST-based way to send messages, but I'm not familiar with Discord so maybe not.


def send(clinic)
@logger.info "[DiscordClient] Sending message for #{clinic.title} (#{clinic.new_appointments} new appointments)"
text = clinic.discord_text
Copy link
Member

Choose a reason for hiding this comment

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

The BaseClinic class will need to have discord_text added to it, it can probably be similar to what the slack text looks like.

end

def post(clinics)
clinics.filter(&:should_discord_message?).each do |clinic|
Copy link
Member

Choose a reason for hiding this comment

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

BaseClinic will also need to have should_discord_message? defined there. It should be similar to the logic for whether to slack. The &:should_discord_message? is short for filter { |clinic| clinic.should_discord_message? }.

@@ -41,6 +41,7 @@ def tweet(clinic)
else
@twitter.update(text)
end
end
Copy link
Member

@dpca dpca Apr 20, 2021

Choose a reason for hiding this comment

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

This file should stay the same, moving the end to here may change the behavior.

clinic.save_message_time
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

We should ensure that there are newlines at the end of each file too.

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.

2 participants