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

add notification toggles and checks #58

Merged
merged 9 commits into from
Feb 16, 2021
Merged

add notification toggles and checks #58

merged 9 commits into from
Feb 16, 2021

Conversation

eibex
Copy link
Owner

@eibex eibex commented Feb 14, 2021

Describe the PR changes

  • Adds a new table called guild_settings: I would like to use this for anything guild related in the future, I tried to merge systemchannels into this but I couldn't find an elegant way yet. I will figure it out before merging... Most likely going to copy the contents into a new column and then delete the old table in the one_to_two function
  • Moved the admin migration introduced by @Edwinexd from core.database to core.schema (I believe everything should still be working correctly)
  • Adds a toggle command rl!notify
  • Guilds are inserted into the table when either rl!notify is first used or when the bot checks for it (a user adds or removes a reaction from themselves). In the first case, the bot toggles the notification on, in the latter, the bot sets notifications to off (the "default")

I still need to test this properly, but comments are welcome

Closes #50

@eibex eibex force-pushed the notify-on-new-role branch from ace43a5 to 48256b8 Compare February 14, 2021 22:44
@eibex eibex force-pushed the notify-on-new-role branch from 48256b8 to 26e5109 Compare February 14, 2021 22:44
bot.py Outdated Show resolved Hide resolved
bot.py Outdated Show resolved Hide resolved
@eibex
Copy link
Owner Author

eibex commented Feb 15, 2021

Thanks, I noticed them yesterday too but I forgot about them pretty quickly.
I pushed some extra changes to merge systemchannels into guild_settings... Modified the function to delete system channels from the database to convert the db value into a 0. I think I need to make some extra changes as

bool([(0,)]) == True

and in the if statements we only check if the list is empty or not.

EDIT: I guess it was faster to actually make the change than explaining it

@eibex eibex force-pushed the notify-on-new-role branch from 25fd4bb to 2fdda6b Compare February 15, 2021 22:18
@eibex eibex force-pushed the notify-on-new-role branch from 2fdda6b to aa07bb9 Compare February 15, 2021 22:19
@eibex
Copy link
Owner Author

eibex commented Feb 15, 2021

What do you think of merging the two tables? Does it make sense not to create a new table for every single new feature?
EDIT: I'm starting to think it's not such a great idea... Managing updates, insertions and replacements is becoming a nightmare.

@Edwinexd
Copy link
Contributor

I personally prefer having everything guild-specific in its own table (eg in this case system channels & notification setting ) while a bit more annoying to mange updates it is easier to remember how to get something from the database eg: I want the guild system channel I'll go read it from the guild settings table as it makes sense to be there.

While not a problem with having 1 table for each setting it requires storing the guild id multiple times in multiple tables using up unnecessary space. I know it won't use up that much space but it feels like it is better to do it now then later if it is something that wants implementation later on.

@eibex
Copy link
Owner Author

eibex commented Feb 16, 2021

Alright, I will tweak the old db functions to maybe use INSERT OR IGNORE followed by an UPDATE to avoid headaches. (Since REPLACE would make values not specified NULL).

@eibex
Copy link
Owner Author

eibex commented Feb 16, 2021

After testing and inspecting the DB manually this looks good to go for me. Let me know if you have any comments before merging.

@Edwinexd
Copy link
Contributor

Looks good👍

@eibex eibex merged commit df50af3 into master Feb 16, 2021
@eibex eibex deleted the notify-on-new-role branch February 16, 2021 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification on new role (guild)
2 participants