Relay all DMs sent to the bot to #dm_log by lemonsaurus · Pull Request #1041 · python-discord/bot

lemonsaurus · 2020-07-12T18:31:32Z

This PR adds a feature which will relay DMs sent to the bot through a simple webhook relay system. It also provides a mechanism for responding to these DMs, should you need to do so.

The mechanism for responding will by default respond to the last person who DMd the bot, but can optionally take a member parameter to reply to a specific member.

Refactoring Duck Pond and Python News

To do this in a DRY way, I first refactored the send_webhook helper used in Duck Pond into a generic util, which could be used for both Duck Pond and the DM Relay cog.

As it turns out, we had a similar mechanism in use over in the Python News handler as well, so I had to refactor that as well to use this util.

Fixing the DuckPond tests

Holy shit, DuckPond has ten million tests, and half of them broke when I made the first change. So I had to fix some of those broken tests.

Add the DMRelay cog

Next, we add a cog that will just relay anything that is a DM sent by a human to the right channel. The cog also adds the command to allow you to respond. Easy peasy.

Closes #667.

#667

Some of the tests were failing because they were expecting send_webhook to be a method of the DuckPond cog, other tests simply were no longer applicable, and have been removed. #667

This shouldn't be used as a replacement for ModMail, but I think it makes sense to have the feature just in case #dm-log provides an interesting use-case where responding as the bot makes sense. It's a bit of a curiosity, and Ves hates it, but I included it anyway. #667

#667

MarkKoz

One more thing.

Also now catches the exception if a user has disabled DMs, and adds a red cross reaction. #667

kosayoda

Sweet!
As it stands, moderators can !reply anywhere in the server. Is that something we want?

lemonsaurus · 2020-07-14T12:57:10Z

As it stands, moderators can !reply anywhere in the server. Is that something we want?

Hmm, good question. We might want to contain that in the #dm-log channel so that all the dm traffic (both in and out) is in one place. I'll make that change.

Numerlor · 2020-07-14T13:04:14Z

Should the cog make some attempts to mitigate issues when the !reply command is used for the last user, but an another DM comes in while the invokation message is being sent and processed?

lemonsaurus · 2020-07-14T13:06:06Z

Should the cog make some attempts to mitigate issues when the !reply command is used for the last user, but an another DM comes in while the invokation message is being sent and processed?

I did consider this, and I wonder how much of a problem it's really going to be.

How would you suggest we could go about mitigating this anyway?

lemonsaurus · 2020-07-14T13:09:06Z

the easiest approach would be to just remove the whole last user feature altogether and always require a member to be specified. More technical approach would be some sort of locking mechanism, but I'm not really keen to complicate this. This feature is absolutely not meant to be used for anything serious, so it feels like even if the DM gets delivered to the wrong person, it shouldn't really do any damage.

Numerlor · 2020-07-14T13:15:51Z

Perhaps some sort of a delay on the setting the user would be enough to give time for the person writing it to notice the change without influencing other things.
Are there any stats on how many dms the bot gets? If it's something relatively rare and the command is supposed to be used mostly for some shenanigans then the current approach ought to be fine. But if it's also supposed to be used sparingly then making the user a required argument shouldn't be that much of a problem for the people replying.

lemonsaurus · 2020-07-14T13:24:56Z

Yeah, I'm leaning towards just simplifying and requiring the user to prevent this kind of race condition issue.

We don't have any such stats from what I'm aware. I'm curious myself. Maybe this PR should add stats for that, too.

If you're typing up a reply and the bot gets another DM while you're typing, you might accidentally send your reply to the wrong person. This could happen even if you're very attentive, because it might be a matter of milliseconds. The complexity to prevent this isn't worth the convenience of the feature, and it's nice to get rid of the caching as well, so I've decided to just make .reply require a user for every reply. #1041

lemonsaurus · 2020-07-14T23:17:17Z

oh shit, that last commit was supposed to be two commits, but anyway I've made the changes requested by @kosayoda and @Numerlor now:

It is no longer possible to use .reply outside the #dm-log channel.
.reply now requires a user to be specified for each call. This is to prevent situations where you would reply to the wrong person. It also has the added benefit of removing a bit of complexity (to do with caching)

kosayoda

Works great now.

Without this, it is difficult to know precisely who the user that is DMing us is, which might be useful to us. #1041

#1041

When we're using the !reply command, using a regular UserConverter is somewhat problematic. For example, if I wanted to send the message "lemon loves you", then I'd try to write `!reply lemon loves you` - however, the optional User converter would then try to convert `lemon` into a User, which it would successfully do since there's like 60 lemons on our server. As a result, the message "loves you" would be sent to a user called lemon.. god knows which one. To solve this bit of ambiguity, I introduce a new converter which only converts user mentions or user IDs into User, not strings that may be intended as part of the message you are sending. #1041

Weird. #1041

#1041

Add upstream changes & resolve config conflicts introduced by #1041. In the conflicting parts of the diff, this commit also re-sorts constants which were added upstream to lexicographical order. This does **not** re-sort all constants added upstream.

Leon Sandøy added 8 commits July 12, 2020 14:40

Add the #dm_log ID to constants.

cb5e361

#667

Refactor Duck Pond embed sender to be a util.

9042325

#667

Refactor python_news.py to use webhook util.

ef65033

#667

Relay all DMs and embeds to #dm-log.

3fce243

#667

Replace channel ID with webhook ID for dm_log.

5007e73

#667

Only relay DMs, and only from humans.

4349fde

#667

Fix DuckPond tests now that send_webhook is gone.

df1730e

Some of the tests were failing because they were expecting send_webhook to be a method of the DuckPond cog, other tests simply were no longer applicable, and have been removed. #667

lemonsaurus requested a review from a team as a code owner July 12, 2020 18:31

lemonsaurus requested review from dementati and tagptroll1 and removed request for a team July 12, 2020 18:31

ghost added the needs 2 approvals label Jul 12, 2020

MarkKoz requested changes Jul 13, 2020

View reviewed changes

Comment thread bot/cogs/dm_relay.py Outdated

Comment thread bot/cogs/python_news.py

Comment thread bot/utils/webhooks.py

Comment thread bot/cogs/dm_relay.py Outdated

Comment thread bot/cogs/dm_relay.py

ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Jul 13, 2020

Numerlor reviewed Jul 13, 2020

View reviewed changes

Comment thread bot/cogs/dm_relay.py Outdated

MarkKoz added p: 3 - low Low Priority t: feature New feature or request labels Jul 13, 2020

Leon Sandøy added 5 commits July 13, 2020 15:13

Better docstring for DMRelay cog.

ed23687

#667

Add avatar_url in python_news.py

ab15466

#667

Only mods+ may use the commands in this cog.

87c2ef7

#667

Don't run on_message if self.webhook is None.

3119369

#667

Remove redundant clean_content variable.

aa0b20b

#667

ghost added needs 2 approvals and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jul 13, 2020

MarkKoz requested changes Jul 13, 2020

View reviewed changes

Comment thread bot/cogs/dm_relay.py

ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Jul 13, 2020

Store last DM user in RedisCache.

ea62b6b

Also now catches the exception if a user has disabled DMs, and adds a red cross reaction. #667

ghost added the needs 2 approvals label Jul 13, 2020

Merge branch 'master' into dm_relay

02f7799

MarkKoz approved these changes Jul 13, 2020

View reviewed changes

ghost added needs 1 approval and removed needs 2 approvals labels Jul 13, 2020

kosayoda suggested changes Jul 14, 2020

View reviewed changes

ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 1 approval labels Jul 14, 2020

ghost added needs 1 approval and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jul 14, 2020

Merge branch 'master' into dm_relay

e940cef

kosayoda approved these changes Jul 15, 2020

View reviewed changes

ghost removed the needs 1 approval label Jul 15, 2020

Merge branch 'master' into dm_relay

d8b570c

kosayoda merged commit caf5be2 into master Jul 15, 2020

kosayoda deleted the dm_relay branch July 15, 2020 03:50

lemonsaurus pushed a commit that referenced this pull request Jul 15, 2020

Add the user ID to the username in dm relays.

226eb68

Without this, it is difficult to know precisely who the user that is DMing us is, which might be useful to us. #1041

lemonsaurus pushed a commit that referenced this pull request Jul 15, 2020

Add some stats for DMs sent and received.

ed6d848

#1041

lemonsaurus pushed a commit that referenced this pull request Jul 15, 2020

How did that except except block get in?

80c1dbb

Weird. #1041

lemonsaurus pushed a commit that referenced this pull request Jul 15, 2020

Oops, these prints shouldn't be here.

14141a2

#1041

Uh oh!

Conversation

lemonsaurus commented Jul 12, 2020

Refactoring Duck Pond and Python News

Fixing the DuckPond tests

Add the DMRelay cog

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MarkKoz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kosayoda left a comment

Choose a reason for hiding this comment

Uh oh!

lemonsaurus commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Numerlor commented Jul 14, 2020

Uh oh!

lemonsaurus commented Jul 14, 2020

Uh oh!

lemonsaurus commented Jul 14, 2020

Uh oh!

Numerlor commented Jul 14, 2020

Uh oh!

lemonsaurus commented Jul 14, 2020

Uh oh!

lemonsaurus commented Jul 14, 2020

Uh oh!

kosayoda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lemonsaurus commented Jul 14, 2020 •

edited

Loading