Discord Event Announcements Finished #129

royh02 · 2021-04-25T07:49:00Z

Announcements:

Weekly
1 Day in advance
Day of
Hour before
Now

Example:

Now, In 1 hour, Today, Tomorrow are done. Week is not

royh02 · 2021-04-25T07:51:07Z

Also, I made a change to the Event Model to use TimeField instead of CharField.

currently just chatter

robertquitt

Good work so far!

Be sure to install pre-commit so that the autoformatter, black, runs.

Be sure to make the database migration with manage.py makemigrations

I like the fact that we're finally using the events table for something useful!

apps/db_data/models.py

robertquitt · 2021-04-27T08:53:20Z

apps/db_data/models.py

@@ -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")


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

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

robertquitt · 2021-04-27T08:54:07Z

apps/discordbot/annoucements.py

+    """
+    today = datetime.date.today()
+    now = datetime.datetime.now()
+    upcoming_events = Event.objects.filter(date__gte=today).filter(time__gte=now)


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

Made this fix in announcements.py

robertquitt · 2021-04-27T08:56:51Z

apps/discordbot/bot.py

@@ -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()


Is this intentional? Take a look at 201c5bf

MB I think I messed up when resolving the merge conflict

robertquitt · 2021-04-27T09:06:30Z

apps/discordbot/annoucements.py

+        events = Event.objects \
+                    .filter(date=today, time__lte=times[requested_tdelta]) \
+                    .filter(time__gte=now) \
+                    .order_by("time")


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

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

robertquitt · 2021-04-27T09:13:06Z

apps/discordbot/annoucements.py

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


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

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.

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

robertquitt · 2021-04-27T09:13:49Z

apps/discordbot/bot.py

+                ).result(TIMEOUT_SECS)
+
+
+        schedule.every().sunday.at("17:00").do(partial(announcer, "week"))


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.

Added the constants

robertquitt · 2021-05-17T09:33:36Z

apps/discordbot/annoucements.py

@@ -0,0 +1,52 @@
+import datetime
+
+import pytz


Either use the builtin django timezone interface or add this to requirements.txt

robertquitt · 2021-05-17T09:34:48Z

apps/db_data/templatetags/db_data_tags.py

@@ -9,7 +9,7 @@

 @register.simple_tag
 def get_upcoming_events():
-    return Event.objects.filter(date__gte=datetime.date.today())
+    return Event.objects.filter(date_time__gte=datetime.datetime.today())


Prefer to use datetime.now()

robertquitt · 2021-05-17T09:37:45Z

apps/db_data/models.py

-    def __str__(self):
-        return f"{self.name} ({self.date})"


Is there a reason to delete this? It's used by the admin interface

I made a change to that to be consistent with the DateTimeField. I made a null check because of an issue that came up after migrating and all events had null date_time, which messed up the admin interface.

def __str__(self): if not self.date_time: return f"{self.name}" return f"{self.name} ({self.get_date_string()})"

Maybe we can keep the old date and time fields for backward compatibility?

We could configure the migration to convert from the existing date field to the new datetime field

https://docs.djangoproject.com/en/2.2/topics/migrations/#data-migrations

Just the date right, because the current CharField for time is harder to parse since there isn't a consistent pattern

That's right, the CharField was just there as a hack to display a time range. You can just ignore it, since the CharField is only for events in the past.

robertquitt · 2021-05-17T09:40:12Z

apps/discordbot/annoucements.py

+    return events
+
+
+def get_events_in_time_range(range):


Might be worth annotating this with types to make it more clear

Suggested change

def get_events_in_time_range(range):

from typing import Tuple

def get_events_in_time_range(range: Tuple[datetime, datetime]):

@robertquitt is your policy to use annotations only when things are unclear without them? Also in this case could have two args start_time, end_time instead, and use * to unpack when passing in timeify().

Great suggestion, that's a much better solution

I totally forgot about this Python functionality, thanks!

robertquitt · 2021-05-17T09:43:00Z

apps/discordbot/annoucements.py

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


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

robertquitt · 2021-05-17T09:56:11Z

apps/discordbot/annoucements.py

+            hour=23, minute=59, second=59
+        ),
+        "today": now.replace(hour=23, minute=59, second=59),
+        "hour": (now + datetime.timedelta(hours=1)).replace(minute=59, second=59),


Why do you need these replacements?

robertquitt · 2021-05-17T09:57:35Z

apps/discordbot/annoucements.py

+    end_times = {
+        "week": now + datetime.timedelta(weeks=1),
+        "tomorrow": (now + datetime.timedelta(days=1)).replace(
+            hour=23, minute=59, second=59
+        ),
+        "today": now.replace(hour=23, minute=59, second=59),
+        "hour": (now + datetime.timedelta(hours=1)).replace(minute=59, second=59),
+        "now": now + datetime.timedelta(minutes=10),
+    }
+    start_times = {
+        "week": now,
+        "tomorrow": (now + datetime.timedelta(days=1)).replace(
+            hour=0, minute=0, second=0
+        ),
+        "today": now,
+        "hour": (now + datetime.timedelta(hours=1)).replace(minute=0, second=0),
+        "now": now,


I am really confused by this. Why do you need to replace minutes and seconds? Why does now only go 10 minutes in advance if you only report every 30 minutes?

I'm not sure how to get events happening 1 day in advance. Currently, I'm also running into an issue that seems to be that the replace messes up the timezones. This is because 00:00:00 in UTC is not the same as midnight in PT, and this causes some events to be announced wrongly. I did a lot of digging on SO but there wasn't much besides using replace, which is buggy.

robertquitt · 2021-05-17T09:57:55Z

requirements.txt

@@ -14,6 +14,7 @@ slack_bolt

 # apps.discordbot
 discord.py
+schedule


Add pytz here or remove pytz from the code

robertquitt · 2021-05-17T09:58:06Z

apps/csua_backend/wsgi.py

@@ -15,6 +15,8 @@
 """
 import os

+from apps.discordbot.bot import csua_bot


robertquitt · 2021-05-17T10:06:01Z

apps/discordbot/bot.py

+        TOMORROW = "tomorrow"
+        TODAY = "today"
+        HOUR = "hour"
+        NOW = "now"


Rename NOW

dlzou · 2021-05-19T00:57:29Z

apps/discordbot/bot.py

+        csua_bot.announcements_thread = threading.Thread(
+            target=csua_bot.event_announcement, daemon=True
+        )
+        csua_bot.announcements_thread.start()


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?

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?

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?

robertquitt · 2021-05-19T03:47:56Z

apps/db_data/models.py

@@ -168,8 +169,7 @@ class Sponsorship(models.Model):
 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)
+    date_time = models.DateTimeField(null=False)


We need to have some way to store the start and end datetimes, not just the start

royh02 added 4 commits February 27, 2021 23:53

update repo + debugging event announcements

d37cdb6

temp update with broken announce

f4691b6

Updated bot to include announcement feature

a17e20d

Now, In 1 hour, Today, Tomorrow are done. Week is not

weekly announcement works now

77117a7

royh02 requested a review from aninrusimha April 25, 2021 07:49

royh02 self-assigned this Apr 25, 2021

royh02 linked an issue Apr 25, 2021 that may be closed by this pull request

Public CSUA calendar + Event integration #94

Open

royh02 added 2 commits April 25, 2021 14:58

announcement channel id added for testing

6d34857

currently just chatter

Merge branch 'master' into master

5afa421

royh02 requested a review from dlzou April 26, 2021 03:50

robertquitt requested changes Apr 27, 2021

View reviewed changes

royh02 added 2 commits May 16, 2021 16:58

Migrated database + Fixed timezones + Refactored

480a860

Merge branch 'master' of https://github.com/royh02/csua-backend

2c15d57

royh02 requested a review from robertquitt May 17, 2021 00:26

royh02 force-pushed the master branch from 650e9a1 to 2c15d57 Compare May 17, 2021 09:23

royh02 added 2 commits May 17, 2021 02:27

black format

ab0875f

black format again

86c479e

robertquitt reviewed May 17, 2021

View reviewed changes

dlzou reviewed May 19, 2021

View reviewed changes

robertquitt reviewed May 19, 2021

View reviewed changes

royh02 added 3 commits May 29, 2021 22:17

separated start/end times, fixed timedelta logic, removed pytz

b25d208

properly set up migration and refactored

f220330

fixed channel ids

b725aa8

royh02 merged commit 2e2f937 into CSUA:master Jun 4, 2021

		).result(TIMEOUT_SECS)


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

	def get_events_in_time_range(range):
	from typing import Tuple
	def get_events_in_time_range(range: Tuple[datetime, datetime]):

Discord Event Announcements Finished #129

Discord Event Announcements Finished #129

Conversation

royh02 commented Apr 25, 2021

royh02 commented Apr 25, 2021

robertquitt left a comment

Choose a reason for hiding this comment

robertquitt Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertquitt May 19, 2021 • edited Loading

Choose a reason for hiding this comment

royh02 May 21, 2021 • edited Loading

Choose a reason for hiding this comment

robertquitt Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertquitt Apr 27, 2021 •

edited

Loading

robertquitt May 19, 2021 •

edited

Loading

royh02 May 21, 2021 •

edited

Loading

robertquitt Jun 2, 2021 •

edited

Loading