Conversation
rhendric
force-pushed
the
rhendric/subscriptions
branch
from
cecad89 to
8125b02
Compare
gwillen
left a comment
gwillen
left a comment
There was a problem hiding this comment.
Looks generally good to me, see comments.
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
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.
|
|
||
| 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.
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.)
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( |
There was a problem hiding this comment.
What is the expected format of the field? It looks like either space-separated or comma-separated might work ok?
| 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 |
There was a problem hiding this comment.
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.
Oh sorry, this is what Paul already fixed in #91. Rebase on top of that instead, presumably.
|
|
||
| // 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.
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.
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...
2 participants
Things I'd work on if I didn't want to keep this as small as possible for you:
But I'm pretty jazzed about this anyway and I hope you get a chance to take a look.