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

Handle adding reactions to messages from users who have blocked the bot #2580

Merged
merged 9 commits into from
Jan 29, 2024

Conversation

TizzySaurus
Copy link
Contributor

@TizzySaurus TizzySaurus commented May 4, 2023

Completes the bot-side of #2486.

What

When the bot fails to add a reaction to a message because the user has the bot blocked (e.g. creating an incident, editing an eval invocation, etc.) the bot handles this via a log.debug statement, and if a message is passed to the handler then will also send a message to the message's channel in discord (which deletes after 30 seconds -- long enough to give the user a chance to unblock the bot from that message).

Works For:

  • Commands
  • Events
  • Schedules

Testing:

To test this, simply block the bot and then either create an incident, or edit an eval command (or anything else that causes the bot to react to your message).

Example:

Screenshot 2023-05-04 at 23 39 44

Screenshot 2023-05-04 at 23 38 58

@TizzySaurus TizzySaurus added t: bug Something isn't working a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) p: 2 - normal Normal Priority s: waiting for author Waiting for author to address a review or respond to a comment labels May 4, 2023
Copy link
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

scheduling is something that is handled by bot-core, so the fix would need to be there. The error handler should probably be ported there first, so we don't have to duplicate this in multiple places. It might make sense to have central error handling that errors from everywhere are handled by, but i've not looked into it so i'm not too sure.

bot/bot.py Outdated Show resolved Hide resolved
bot/bot.py Outdated Show resolved Hide resolved
@TizzySaurus
Copy link
Contributor Author

scheduling is something that is handled by bot-core, so the fix would need to be there. The error handler should probably be ported there first, so we don't have to duplicate this in multiple places.

So are you suggesting to merge this PR as-is, and add the schedule handling to bot-core at a later time? I'd be okay with this, as long as the exceptions raised aren't going to cause sentry alerts etc.

bot/bot.py Outdated Show resolved Hide resolved
bot/utils/helpers.py Outdated Show resolved Hide resolved
bot/utils/helpers.py Outdated Show resolved Hide resolved
bot/utils/helpers.py Outdated Show resolved Hide resolved
bot/exts/backend/error_handler.py Outdated Show resolved Hide resolved
bot/utils/helpers.py Outdated Show resolved Hide resolved
@TizzySaurus
Copy link
Contributor Author

For the record, @ChrisLovering and I have come up with a solution for handling errors in the schedules (see python-discord/bot-core#177).

@TizzySaurus TizzySaurus marked this pull request as ready for review May 6, 2023 21:29
@wookie184
Copy link
Contributor

I don't understand how the bot-core PR would help. Would we have to change all uses of create_task to add an error handler? That doesn't seem great, compared to just handling it there directly.

@TizzySaurus
Copy link
Contributor Author

TizzySaurus commented May 7, 2023

I don't understand how the bot-core PR would help. Would we have to change all uses of create_task to add an error handler? That doesn't seem great, compared to just handling it there directly.

Yeah, you're right, in reality we'd want the handle_forbidden_from_block function to be in bot-core, and automatically added in create_task. I'll make that change to the bot-core PR 👍

In fact, this change would make the done_callbacks entirely pointless, since this would be in place of that. So maybe that should be removed at the same time?

@TizzySaurus
Copy link
Contributor Author

This is essentially stalled by python-discord/bot-core#177 now. Once that's merged, I can remove handle_forbidden_from_block from here, and migrate to importing from bot-core.

@TizzySaurus TizzySaurus added the s: stalled Something is blocking further progress label May 7, 2023
@TizzySaurus
Copy link
Contributor Author

python-discord/bot-core#177 has now been merged, so will be continuing on with this.

@TizzySaurus TizzySaurus removed the s: stalled Something is blocking further progress label Jun 11, 2023
@TizzySaurus
Copy link
Contributor Author

This PR is now ready for review! 🎉

@TizzySaurus TizzySaurus added s: needs review Author is waiting for someone to review and approve and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jun 11, 2023
@ChrisLovering ChrisLovering enabled auto-merge (squash) June 11, 2023 13:53
@Xithrius Xithrius added up for grabs Available for anyone to work on and removed s: needs review Author is waiting for someone to review and approve up for grabs Available for anyone to work on labels Jan 28, 2024
@Xithrius Xithrius self-assigned this Jan 28, 2024
bot/bot.py Outdated Show resolved Hide resolved
@Xithrius Xithrius disabled auto-merge January 29, 2024 00:20
@Xithrius Xithrius enabled auto-merge (squash) January 29, 2024 00:20
Copy link
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Tested and seems to work nicely.

@Xithrius Xithrius merged commit f10aea9 into main Jan 29, 2024
5 checks passed
@Xithrius Xithrius deleted the fix-2486 branch January 29, 2024 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) p: 2 - normal Normal Priority t: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants