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

Re-work message clean up, re-work of the admin system and cleaning up left guilds #53

Merged
merged 12 commits into from
Dec 18, 2020

Conversation

Edwinexd
Copy link
Contributor

Fixes #52 by deleting a guilds database entries after leaving the guild / if the guild gets deleted.
In order to do this I added another database function (remove_guild) which just removes everything directly connected to the guild (message, reactionroles, systemchannels). Please note: This will NOT remove the admin role id from the database as admins table does not refer to which guild the admin role belongs to.

@eibex
Copy link
Owner

eibex commented Dec 15, 2020

Thanks a lot for this PR! I will test it later this evening but it looks good.

My bad for forgetting to attach guild IDs to admin IDs.

I have only a single question: would this work if the bot and/or discord api was down when the guild gets deleted?

@Edwinexd
Copy link
Contributor Author

Edwinexd commented Dec 15, 2020

No it would not work as it is done right now, it could be done by fetching every guild that the bot has database entries in every now and then but that will be a lot of API calls and it would probably be wise to in that case store the guilds "to be removed" for 24 hrs then re-check to deal with any potential invalid responses from Discords API. Hope that makes any sense, I'd be able to help do that if it sounds good 👍

@eibex
Copy link
Owner

eibex commented Dec 15, 2020

That was what I tried to achieve in the current master on lines 266-297, but it is not working as intended.

The similar code for deleted messages (lines 222-256) seems to be working correctly when the message is deleted but the guild is not.

My current guess is that if the guild is deleted the except and/or if functions are not triggered properly. I haven't had the time to debug it sadly.

@eibex
Copy link
Owner

eibex commented Dec 15, 2020

It would be great if we could go the 24h route so that the change is retroactive and does not assume 100% uptime.

@Edwinexd
Copy link
Contributor Author

👍 I took a quick look at 266-297 and I think the reason it is currently not working is because: db.fetch_all_guilds() only checks for guilds that have set up a "system channel". Although it is probably wise to redo-it with the "24h system". I'll try to get to work on it tomorrow. On another note we should prob redo the admin roles system to save the guild it belongs aswell as its probably good to get that sorted now so when the redone cleaning is done it wont need another redo to patch the role ids.

@eibex
Copy link
Owner

eibex commented Dec 15, 2020

Thanks for spotting it so quickly. I agree with re-working the admin system as well. I hope I can get a good look at it this weekend.

Thanks for taking interest in this project 😃

…e & interaction

Please note: Other function calls will be needed when calling get_admins, remove_admin and add_admin
@Edwinexd
Copy link
Contributor Author

Glad I can help! :)
I redid the admin database structure and added the table needed to use the "24h system" so far, will get to editing bot.py & adding database functions to interact with the new tables

@Edwinexd Edwinexd changed the title Remove a guilds database entries once leaving a guild Re-work message clean up, re-work of the admin system and cleaning up left guilds Dec 16, 2020
Also added another task loop to check "unreachable" guilds every 6 hrs instead of every 24 hrs
@eibex
Copy link
Owner

eibex commented Dec 16, 2020

Thanks for the quick changes!

This weekend I will add the necessary updates for database migration. I think the most painless way to do this is to outright delete the old admins table, recreate it with the new column and let users re-run the command to add admins.

Once I did that and tested everything I will merge and release 2.1.0

@eibex
Copy link
Owner

eibex commented Dec 16, 2020

The reason I don’t check for the actual Administrator permission (only done for adding and removing admins) is to have other users manage the bot by giving them a certain role without giving them full control over the server.

(Unless this isn’t what you meant with checking for actual admins)

The code could be (or most likely it is) inefficient as I am not a developer and this is just a hobby of mine.

@Edwinexd
Copy link
Contributor Author

Oh my bad! I ment the isadmin() function which checks if the user has the admin role, my bad for being unclear!

@Edwinexd
Copy link
Contributor Author

Hey! I just noticed that anyone with an admin role can kill, restart or update the bot, that can be abused if its added to multiple servers and in one of them decides to mess with it they can without any problem, I suggest to add this decor to the management commands https://discordpy.readthedocs.io/en/latest/ext/commands/api.html?highlight=is_owner#discord.ext.commands.is_owner to only make those commands usable by the bot owner

Copy link
Contributor Author

@Edwinexd Edwinexd left a comment

Choose a reason for hiding this comment

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

Edit for
b9bc29d * Clean up admin roles that cant be migrated not guilds

@eibex
Copy link
Owner

eibex commented Dec 17, 2020

I agree with the is_owner decorator for those three commands as I do not even see a reason for which other administrators need to have access to them.

I see that you already made the code to ALTER the admins table. What's left to do now? I can complete the remaining tasks tomorrow if you want, you already did a ton of work (thanks!).

I added it to more than just the 3 I mentioned before, please review and check if this is something you agree with.
@Edwinexd
Copy link
Contributor Author

I don't think there is anything more to do than to write a little changelog. No worries! 🙂

@eibex eibex merged commit c48c8c1 into eibex:master Dec 18, 2020
@eibex
Copy link
Owner

eibex commented Dec 18, 2020

Thanks again for taking the time to make such a big PR!

Repository owner deleted a comment from Edwinexd Jul 23, 2021
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.

DB not self-cleaning after deleting servers
2 participants