-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Feature - Ban Email #4980
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
base: main
Are you sure you want to change the base?
Feature - Ban Email #4980
Conversation
0eed629
to
19a3662
Compare
19a3662
to
f1f7718
Compare
- Made 'for example' on its own line. - Made unordered list for ban reasons. - Made it look a little nicer.
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 doing this @OmarMoataz!
I've left some minor feedback, let me know if I can help with anything.
class BanMailer < ApplicationMailer | ||
default from: 'The Odin Project <[email protected]>' | ||
|
||
def send_ban_email_to(user) |
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: Because this is a user related email, I think it should live in the UserMailer
along side the welcome email.
|
||
def send_email(project_submission_owner) | ||
ban_mailer = BanMailer.new | ||
ban_mailer.send_ban_email_to(project_submission_owner) |
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: I think this will wait until the email is sent within the same request/response cycle, slowing the response down. We can send the email in the background instead:
UserMailer.send_ban_email_to(project_submission_owner).deliver_later
@@ -0,0 +1,101 @@ | |||
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> |
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've left some minor feedback about the email content on the issue.
One of your project submissions contained NSFW or other highly offensive images | ||
One of your project submissions contained bigotry, such as racism, homophobia, hate speech | ||
One of your project submissions were plagiarized | ||
If you are unsure of the rules that were broken, you may reach out to us at [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.
Nit: Can we add a new line above this please?
Hey, I'll work on this a bit more. I realize there's one test failing as well but I haven't had enough time to sink my teeth into this sooner. I'll also work on your comments. Thanks, Kevin! |
Hey @OmarMoataz, hope things are well? do you need any help getting this finished? |
Because
Added ban email feature to notify the user that they've been banned.
This PR
Issue
Closes #4666
Additional Information
Pull Request Requirements
keyword: brief description of change
format, using one of the following keywords:Feature
- adds new or amends existing user-facing behaviorChore
- changes that have no user-facing value, refactors, dependency bumps, etcFix
- bug fixesBecause
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section