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 Event Announcements Finished #129

Merged
merged 13 commits into from
Jun 4, 2021
5 changes: 4 additions & 1 deletion apps/db_data/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ class Event(models.Model):
name = models.CharField(max_length=70)
location = models.CharField(max_length=70)
date = models.DateField(null=True)
time = models.CharField(max_length=70)
robertquitt marked this conversation as resolved.
Show resolved Hide resolved
time = models.TimeField(null=True)
description = models.TextField()
link = models.URLField(blank=True)
category = models.ForeignKey(
Expand All @@ -187,6 +187,9 @@ def __str__(self):
def is_passed(self):
return self.date < datetime.date.today()

def get_time_string(self):
return self.time.strftime("%I:%M %p PST")
Copy link
Member

@robertquitt robertquitt Apr 27, 2021

Choose a reason for hiding this comment

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

We won't always be in daylight savings time, so PT would be more correct in this case

Copy link
Member Author

Choose a reason for hiding this comment

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

Used DateTimeField and made it TZ aware. Everything is stored as UTC and the getters will convert it to Pacific Time



class EventCategory(models.Model):
id = models.CharField(max_length=16, primary_key=True)
Expand Down
30 changes: 30 additions & 0 deletions apps/discordbot/annoucements.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import datetime
from apps.db_data.models import Event

def event_checker(requested_tdelta):
"""
Checks events in db to see if any match the day
Copy link
Member

Choose a reason for hiding this comment

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

Could you consider renaming this function to something like get_events_in_date_or_time_delta? The fact that this has 3 branches plus a dict lookup makes it hard to understand. Maybe you could have a helper function, get_events_in_time_range, that took a time range (start and end time) and returned events, and then have another function that translates "week", "day" etc to the correct time ranges to pass to get_events_in_time_range

Copy link
Member Author

Choose a reason for hiding this comment

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

I have split the function into 3, namely get_events_in_date_or_time_delta(requested_tdelta), and 2 helpers: get_events_in_time_range(range), timeify(requested_tdelta). I added docstrings in for added clarity.

Copy link
Member

Choose a reason for hiding this comment

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

I regret saying to call it get_events_in_date_or_time_delta. Maybe you should think of a better name yourself, like get_events_in_period

Copy link
Member

Choose a reason for hiding this comment

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

This docstring doesn't match what the function does

Redundant comments are neutral at best, but inconsistent comments are bad.

"""
today = datetime.date.today()
now = datetime.datetime.now()
upcoming_events = Event.objects.filter(date__gte=today).filter(time__gte=now)
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned earlier, if we remove date and only use time, then we could simplify this to just one filter

Copy link
Member Author

Choose a reason for hiding this comment

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

Made this fix in announcements.py

days = {
"week": today + datetime.timedelta(weeks=1),
"tomorrow": today + datetime.timedelta(days=1),
"today": today,
}
times = {
"hour": now + datetime.timedelta(hours=1),
"now": now + datetime.timedelta(minutes=1)
}
if requested_tdelta in times:
events = Event.objects \
.filter(date=today, time__lte=times[requested_tdelta]) \
.filter(time__gte=now) \
.order_by("time")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could consider using https://docs.djangoproject.com/en/2.2/ref/models/options/#django.db.models.Options.ordering

To avoid re-writing the order_by each time we query

Copy link
Member Author

Choose a reason for hiding this comment

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

Added into the Events model so that everything is already ordered in the DB

elif requested_tdelta == "week":
events = Event.objects.filter(
date__lte=days[requested_tdelta]).filter(date__gte=today).order_by("time")
else:
events = Event.objects.filter(date=days[requested_tdelta]).order_by("time")
return events
72 changes: 69 additions & 3 deletions apps/discordbot/bot.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,19 @@
import logging
import threading
import unicodedata

import discord
from functools import partial
import datetime, schedule, time
from decouple import config
import discord
from discord.embeds import Embed
from discord.utils import get
from django.core.exceptions import ValidationError
from django.core.validators import validate_email
from pyfiglet import figlet_format

from . import connect4, cowsay, xkcd
from .utils import send_verify_mail
from .annoucements import event_checker

intents = discord.Intents.all()
intents.presences = False
Expand All @@ -21,6 +24,7 @@
CSUA_PHILBOT_CLIENT_ID = config("BOT_ID", default=737930184837300274, cast=int)
HOSER_ROLE_ID = config("TEST_ROLE", default=785418569412116513, cast=int) # Verified
DEBUG_CHANNEL_ID = config("DEBUG_CHANNEL", default=788989977794707456, cast=int)
ANNOUNCEMENTS_CHANNEL_ID = config("ANNOUNCEMENTS_CHANNEL", default=784902200102354989, cast=int) # set to chatter for testing
TIMEOUT_SECS = 10

logger = logging.getLogger(__name__)
Expand All @@ -30,6 +34,8 @@ class CSUAClient(discord.Client):
async def on_ready(self):
print(f"{self.user} has connected to Discord")
self.is_phillip = self.user.id == CSUA_PHILBOT_CLIENT_ID
csua_bot.announcements_thread = threading.Thread(target=csua_bot.event_announcement, daemon=True)
csua_bot.announcements_thread.start()
Copy link
Member

Choose a reason for hiding this comment

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

The circular dependency makes me a bit uneasy. Haven't completely thought it through, but could announcements_thread and event_announcement be moved into this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally put this here because we need to wait for on_ready to trigger before the announcement_thread starts, or else the thread would start too early with no client ready. Do you have something else in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so I'm wondering why event_announcement is a method in CSUABot rather than in CSUAClient, or even a stand-alone function, considering there's an instance of CSUAClient in CSUABot. I suppose there isn't really an impact on functionality and it's more about design. My understanding is that CSUAClient holds the core parts running the bot, while CSUABot is sort of an internal API. Could we get a third opinion?

if self.is_phillip:
self.csua_guild = get(self.guilds, id=CSUA_GUILD_ID)
self.test_channel = get(self.csua_guild.channels, id=DEBUG_CHANNEL_ID)
Expand Down Expand Up @@ -141,6 +147,8 @@ def check_thumb(react, _):
if self.is_phillip:
await self.test_channel.send(f"{member} was sent registration email")




def emoji_letters(chars):
return [unicodedata.lookup(f"REGIONAL INDICATOR SYMBOL LETTER {c}") for c in chars]
Expand All @@ -162,11 +170,12 @@ class CSUABot:
def __init__(self):
self.loop = asyncio.new_event_loop()
self.thread = threading.Thread(target=self._start, daemon=True)
self.running = True
self.thread.start()
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? Take a look at 201c5bf

Copy link
Member Author

Choose a reason for hiding this comment

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

MB I think I messed up when resolving the merge conflict


def _start(self):
asyncio.set_event_loop(self.loop)
self.client = CSUAClient(intents=intents)

try:
self.loop.run_until_complete(self.client.start(TOKEN))
finally:
Expand All @@ -188,6 +197,63 @@ def promote_user_to_hoser(self, tag):
return True
return False

def event_announcement(self):
print('Announcements Thread started...')

times_msg = {
"week": "NEXT WEEK",
"today": "TODAY",
"tomorrow": "TOMORROW",
"hour": "IN 1 HOUR",
"now": "NOW",
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the constants you just defined?

}

def announcer(time_before):

events = event_checker(time_before)

if events:
msg = f"**What's happening {times_msg[time_before]}**"
asyncio.run_coroutine_threadsafe(
self.client.get_channel(ANNOUNCEMENTS_CHANNEL_ID).send(msg), self.loop
).result(TIMEOUT_SECS)
print('hey hey hey time to check') # debugging

send_embed(events)

def send_embed(events):
for event in events:
embed = discord.Embed(
title=event.name,
description=event.description,
colour=discord.Colour.red()
)
embed.add_field(name='Date', value=event.date, inline=True)
embed.add_field(name='Time', value=event.get_time_string())
embed.add_field(name='Link', value=event.link)
asyncio.run_coroutine_threadsafe(
self.client.get_channel(ANNOUNCEMENTS_CHANNEL_ID).send(embed=embed), self.loop
).result(TIMEOUT_SECS)


schedule.every().sunday.at("17:00").do(partial(announcer, "week"))
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the fact that we're using magic strings here. If "week" or "tomorrow" or "today" get accidentally misspelled, we might end up with a bug that we won't notice until the time comes around. My suggestion would be to define some constants like TODAY = "today" and just use those instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the constants

schedule.every().day.at("08:00").do(partial(announcer, "tomorrow"))
schedule.every().day.at("08:00").do(partial(announcer, "today"))
schedule.every().hour.do(partial(announcer, "hour"))
schedule.every(30).minutes.do(partial(announcer, "now"))

# For debugging
# schedule.every(10).seconds.do(partial(announcer, "week"))
# schedule.every(10).seconds.do(partial(announcer, "tomorrow"))
# schedule.every(10).seconds.do(partial(announcer, "today"))
# schedule.every(10).seconds.do(partial(announcer, "hour"))
# schedule.every(10).seconds.do(partial(announcer, "now"))
Copy link
Member

Choose a reason for hiding this comment

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

delete


while True:
schedule.run_pending()
time.sleep(5)



if TOKEN:
csua_bot = CSUABot()
Expand Down
Empty file added csua
Empty file.
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ slack_bolt

# apps.discordbot
discord.py
schedule
Copy link
Member

Choose a reason for hiding this comment

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

Add pytz here or remove pytz from the code

pyfiglet
cowpy

Expand Down