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

first cut at keyword subscriptions #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions herring/herring/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
'django.contrib.admin',
'django.contrib.auth',
'django.contrib.contenttypes',
'django.contrib.postgres',
'django.contrib.sessions',
'django.contrib.messages',
'django.contrib.staticfiles',
Expand Down
42 changes: 42 additions & 0 deletions herring/puzzles/discordbot.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,30 @@ async def on_raw_reaction_add(self, payload: discord.RawReactionActionEvent):
target_channel = message.channel_mentions[0]
await self.add_user_to_puzzle(payload.member, target_channel.name)

# TODO: Merge this with the above when not in ultra-conservative mode
Copy link
Owner

Choose a reason for hiding this comment

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

Go ahead and merge it, and eliminate the requirement that it be the bot's message while you're at it? (Per separate discussion about this being a desirable feature.) So that just any SIGNUP_EMOJI react, to any message the bot can see that mentions a puzzle channel, counts.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh sorry, this is what Paul already fixed in #91. Rebase on top of that instead, presumably.

try:
if payload.emoji.name == SIGNUP_EMOJI:
member = self.guild.get_member(payload.user_id)
if member is not None:
dm_channel = member.dm_channel
if dm_channel is None:
# This seems to be the case if the bot is restarted and
# an old message is reacted to.
dm_channel = await member.create_dm()
if dm_channel.id == payload.channel_id:
message: discord.Message = await dm_channel.fetch_message(payload.message_id)
if message.author.id == self.bot.user.id:
# Can't remove the user's reactions from a DM, apparently...

# message.channel_mentions is always an empty list in
# DMs, per the documentation, so we must do what we
# must do...
match = re.search('\(#([^)]*)\)', message.content)
if match is not None:
await self.add_user_to_puzzle(member, match[1])
except Exception as e:
logging.error(f"on_raw_reaction_add: error in subscription reaction handler", e)

@commands.Cog.listener()
async def on_message(self, message: discord.Message):
logging.info("on_message: %s", message)
Expand Down Expand Up @@ -1186,6 +1210,24 @@ def get_channel_pair(self, puzzle_name):
voice_channel: discord.VoiceChannel = get(self.guild.voice_channels, name=puzzle_name)
return text_channel, voice_channel

async def send_subscription_messages(self, puzzle_name: str, discord_ids: list[str], message_text: str):
await self._really_ready.wait()
channel: discord.TextChannel = get(self.guild.text_channels, name=puzzle_name)
if channel is None:
logging.error(f"Couldn't get Discord channel {puzzle_name} in send_subscription_messages!")
return
message_text += f" {SIGNUP_EMOJI} this message to join, then click here to jump to the channel: {channel.mention}."
for discord_identifier in discord_ids:
member = self.guild.get_member_named(discord_identifier)
if member is None:
logging.warning(f"couldn't find member named {discord_identifier}")
continue
if member in channel.overwrites:
continue
message = await member.send(message_text)
await message.add_reaction(SIGNUP_EMOJI)


@lazy_object
def DISCORD_ANNOUNCER() -> Optional[HerringAnnouncerBot]:
if not settings.HERRING_ACTIVATE_DISCORD:
Expand Down
2 changes: 1 addition & 1 deletion herring/puzzles/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,5 @@ class Meta:
class UserProfileForm(forms.ModelForm):
class Meta:
model = UserProfile
fields = ('discord_identifier',)
fields = ('discord_identifier', 'subscriptions')
# Exclude avatar_url because, to my knowledge, it's literally never used.
23 changes: 23 additions & 0 deletions herring/puzzles/migrations/0016_auto_20240109_0509.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Generated by Django 3.2.16 on 2024-01-09 05:09

from django.db import migrations, models


class Migration(migrations.Migration):
Copy link
Owner

Choose a reason for hiding this comment

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

You will need to remove this migration from your PR -- I have not found a way to get Django to stop generating it. It happens because we set the default value of the hunt_id field based on an environment variable that we change every year. (Some better way of configuring that is obviously on the todo list but it's not really a practical necessity.) Even though the default value is always supplied explicitly by Django, and the database field's own default value is never used, Django still really really wants to create a migration to irrelevantly change the database's default value.


dependencies = [
('puzzles', '0015_channelparticipation_display_name'),
]

operations = [
migrations.AlterField(
model_name='puzzle',
name='hunt_id',
field=models.IntegerField(default=2024),
),
migrations.AlterField(
model_name='round',
name='hunt_id',
field=models.IntegerField(default=2024),
),
]
22 changes: 22 additions & 0 deletions herring/puzzles/migrations/0017_userprofile_subscriptions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Generated by Django 3.2.16 on 2024-01-09 05:50

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('puzzles', '0016_auto_20240109_0509'),
]

operations = [
migrations.AddField(
Copy link
Owner

Choose a reason for hiding this comment

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

What is the expected format of the field? It looks like either space-separated or comma-separated might work ok?

model_name='userprofile',
name='subscriptions',
field=models.TextField(blank=True, null=True),
),
migrations.RunSQL(
sql="""CREATE INDEX puzzles_userprofile_subscriptions_search ON "puzzles_userprofile" USING GIN (to_tsvector('english', COALESCE("puzzles_userprofile"."subscriptions", '')));""",
reverse_sql="""DROP INDEX puzzles_userprofile_subscriptions_search;""",
),
]
3 changes: 2 additions & 1 deletion herring/puzzles/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,10 @@ class UserProfile(models.Model, JSONMixin):
)
avatar_url = models.CharField(max_length=200, **optional)
discord_identifier = models.CharField(max_length=200, **optional)
subscriptions = models.TextField(**optional)

class Json:
include_fields = ['avatar_url', 'discord_identifier']
include_fields = ['avatar_url', 'discord_identifier', 'subscriptions']

def __str__(self):
return "profile for " + self.user.__str__()
Expand Down
14 changes: 13 additions & 1 deletion herring/puzzles/signals.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import re
from django.contrib.auth.models import User
from django.db.models.signals import pre_save, post_save
from django.dispatch import receiver
from django.db import transaction
from puzzles.tasks import create_puzzle_sheet_and_channel, post_answer, post_update, create_round_category
from puzzles.tasks import create_puzzle_sheet_and_channel, post_answer, post_update, create_round_category, notify_subscribers
from puzzles.models import Puzzle, Round, UserProfile

@receiver(post_save, sender=Puzzle)
Expand All @@ -17,11 +18,22 @@ def before_puzzle_save(sender, instance, **kwargs):
if instance.answer != instance.tracker.previous('answer'):
post_answer.delay(instance.slug, instance.answer)

old_keywords = set()
new_keywords = set()

if instance.tags != instance.tracker.previous('tags') and instance.tracker.previous('tags') is not None:
post_update.delay(instance.slug, 'tags', instance.tags)
old_keywords.update(re.findall('\w+', instance.tracker.previous('tags').lower()))
Copy link
Owner

Choose a reason for hiding this comment

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

Technically you are ignoring the fact that tags are supposed to be a list, and treating them as a bag of words. I don't strenuously object to that, but I figured I'd note it. (I think probably in fact they should be stored more cleverly than a comma-separated string in the database, but that's where we currently are.)

new_keywords.update(re.findall('\w+', instance.tags.lower()))

if instance.note != instance.tracker.previous('note') and instance.tracker.previous('note') is not None:
post_update.delay(instance.slug, 'notes', instance.note)
old_keywords.update(re.findall('\w+', instance.tracker.previous('note').lower()))
new_keywords.update(re.findall('\w+', instance.note.lower()))

new_keywords.difference_update(old_keywords)
if len(new_keywords) > 0 and instance.answer == '':
notify_subscribers.delay(instance.slug, '|'.join(new_keywords))


@receiver(post_save, sender=Round)
Expand Down
6 changes: 6 additions & 0 deletions herring/puzzles/static/style.less
Original file line number Diff line number Diff line change
Expand Up @@ -440,3 +440,9 @@ button, a.button, input[type=submit] {
div.editable > span {
pointer-events: none;
}

// Possibly this should apply to all labels, but trying to be
// ultra-conservative right now
label[for="id_subscriptions"] {
Copy link
Owner

Choose a reason for hiding this comment

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

Does this literally only affect the visual appearance of the admin form? Or what is this restyling? If it's just the admin form, I don't care as long as it still works, make it pretty however you want. But I'm actually confused about why it's necessary -- the admin form appears to be styled fine to me, with labels at the top, and if I try adding this style to one of them for testing, it totally breaks the page...

Copy link
Owner

Choose a reason for hiding this comment

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

Ohhh, the admin and non-admin default forms share the exact same property for this. Amazing. I think if you apply this it might break the admin form, I'm not sure. Presumably there's some intended way to style just the user-side form...

vertical-align: top;
}
35 changes: 35 additions & 0 deletions herring/puzzles/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from celery import shared_task
from datetime import datetime, timezone
from django.conf import settings
from django.contrib.postgres.search import SearchQuery, SearchVector
from django.db import transaction
import json
import kombu.exceptions
Expand Down Expand Up @@ -112,6 +113,40 @@ def post_update(slug, updated_field, value):
post_local_and_global(slug, local_message, global_message)


@optional_task
@shared_task(rate_limit=0.5)
def notify_subscribers(slug: str, keywords: str):
logging.warning("tasks: notify_subscribers(%s, %r)", slug, keywords)

try:
puzzle = Puzzle.objects.get(slug=slug)
except Puzzle.DoesNotExist:
return

try:
users = UserProfile.objects.annotate(search=SearchVector('subscriptions', config='english')).filter(
search=SearchQuery(keywords, search_type='raw'),
discord_identifier__isnull=False,
)
users = list(users)
except Exception as e:
logging.error("tasks: notify_subscribers failed while searching subscriptions", e)
return

if len(users) == 0:
return

discord_ids = [u.discord_identifier for u in users]

message = f"Based on your subscriptions, you might want to join \"{puzzle.name}\" (#{slug})."
if puzzle.tags:
message += f" It has tags \"{puzzle.tags}\"."
if puzzle.note:
message += f" It has notes \"{puzzle.note}\"."
if settings.HERRING_ACTIVATE_DISCORD:
do_in_discord(DISCORD_ANNOUNCER.send_subscription_messages(slug, discord_ids, message))


@optional_task
@shared_task(bind=True, max_retries=10, default_retry_delay=5, rate_limit=0.25) # rate_limit is in tasks/sec
def create_puzzle_sheet_and_channel(self, slug):
Expand Down