-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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), | ||
), | ||
] |
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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;""", | ||
), | ||
] |
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) | ||
|
@@ -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())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.