-
Notifications
You must be signed in to change notification settings - Fork 77
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
Context menu button, Ban Spammer #548
base: main
Are you sure you want to change the base?
Conversation
services/spam-ban/__snapshots__/spam-banning.service.test.js.snap
Outdated
Show resolved
Hide resolved
let reply; | ||
|
||
if (!message.member) { | ||
message.react("❌"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming we're reacting to the banned message? So this reaction will be done on a message in a public channel? Not sure we'd want that, but do confirm with others though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button will be mainly used on automod messages that are blocked and logged, Which only staff can see. We wont be using this for messages that automod fails to block or doesnt delete. We will use manual modding to make sure that any left over spam msgs will be deleted by us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so this command is only for the automod messages. Isn't automod a bot though? Won't message.author.bot
always return true? How are we able to get the offending user's id from an automod message? I'm drawing blanks on the documentation, so some links could be helpful 😅
I definitely should set up automod in a dummy server to test this myself instead of bombarding this PR lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are valid questions which I myself had, and made up an environment to explore. To my surprise however is that the Autobot logs actually act as if those message were posted by the user. message.author is actually the user who's message is blocked by Automod. Yeah certainly set up an environment and test things out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.js
Outdated
@@ -13,6 +13,7 @@ const config = { | |||
gettingHiredChannelId: process.env.DISCORD_GETTING_HIRED_CHANNEL_ID, | |||
botSpamPlaygroundChannelId: '513125912070455296', | |||
FAQChannelId: '823266307293839401', | |||
moderationLog: '922520585018433536' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presuming (based on context), that this is an channel ID, shouldn't this be moderationLogChannelId
to preserve consistency in how we name them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats right, this should be a channel ID. ++
const embedMessage = new EmbedBuilder() | ||
.setTitle("Banned: Compromised account / Spam") | ||
.setDescription( | ||
`Account is compromised and is used to spam phishing links. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Account is compromised and is used to spam phishing links. | |
`Your account has been banned from The Odin Project Discord as it is compromised and is used to spam phishing links. |
`Account is compromised and is used to spam phishing links. | ||
|
||
If you would still like to continue using our server, make sure to change your password and recover your account. | ||
After that send a detailed contact information to appeal the ban on [email protected]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After that send a detailed contact information to appeal the ban on theodinprojectcontact@gmail.com`, | |
After that send a ban appeal containing contact information to theodinprojectcontact@gmail.com`, |
Additional: Not a team member, but it might be easier if the instructions also say to add something like [Ban Appeal] to the subject or send it to something like [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your input is valueable and always appreciated. We might also add a template for them to fill in the email too
channel.send({ embeds: [embed] }); | ||
} | ||
|
||
static #isAdmin(member) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it might be worth (probably in a follow up) to refactor check if a user is admin logic to a separate file; I think this is either the third or fourth occurrence of basically this exact code snippet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is duplicated in may other services. This is what im indicating in my PR message. Many of those including tests should be moved to utils. But not sure if we are doing it in this PR or an upcoming one, wanted to see what maintainers think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has now happened in #567
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know, I will integrate it
@TheOdinProject/moderators I would like the mod team to weigh on this. What the message to the user would be like, what kind of template. And perhaps some kind of a form so that they can use for the appeal? This is the current template of the message
I would like to especially hear from @JustWaveThings since this was originally your idea, is this how you imagined it to be, or do you think it needs some improvements? |
I'll be happy to put together a message to send on the ban. I will look at the functionality of it tonight. Thank you so much for doing this! |
@Mclilzee, Here's my take on the message the bot could send:
I don't know if we need to get fancy with the message. I'm also fine with just sending what we send now in the ban message. |
This looks great! Thank you, Fred. This is much better than whatever I can cook with my broken English. |
Very minor nit, but I think that the starting This should also be an Your, otherwise you're switching between modes of address |
156ac29
to
d3ecdb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick few nits on grammar/spelling etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, single n
in file name ban-spammer.js
.
I probably won't have a great deal of time to do a thorough review of the feature itself, I'm more doing little bits here and there, so I'll be cheeky and leave the rump of the stuff to Xari.
Co-authored-by: MaoShizhong <[email protected]>
Co-authored-by: Asartea <[email protected]>
f319ac5
to
c7f63ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mclilzee great stuff 🔥🔥 🔥
} | ||
|
||
static async #announceBan(interaction, message) { | ||
const channelID = config.channels.moderationLogChannelId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this channel id in the config file?
static async handleInteraction(interaction) { | ||
const message = interaction.options.getMessage('message'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming we'll be calling this command only inside the automod channel. We'll add this condition in our discord server's setting. Though a sanity check here won't hurt. Can we add a guard here to check if this command is only called in that channel?
|
||
// The mod who initialized the interaction | ||
user: { | ||
id: '007', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
Phishing links spam is an ever-increasing occurrence on discord. The mod team have to keep banning these users with the same message so that they know how to contact us if they wish to rejoin the server.
This PR will automate that with a click of a button.
This PR
Issue
Closes #504
Additional Information
Make sure to change the config.js moderationLog ID to the channel ID of your test server to see the log messages while testing.
There are currently some duplication of functions in some of the other services, I think it would be a good idea to move those to utils. Another duplication happen is with the tests, There is a buildInteraction function in test utils but that one is made for a specific purpose only, I would need to change it and refactor all the tests that calls it in order to generalize it to be used by these new tests. I didn't want this PR to be too big and include those refactors. So I included some create interaction mocks functions in the test files.
Let me know how would you like to proceed with the refactors.
Pull Request Requirements
location of change: brief description of change
format, e.g.Callbacks command: Update verbiage
Because
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section