Send out email after registration denied, email confirmed (fixes #5547)#5553
Send out email after registration denied, email confirmed (fixes #5547)#5553
Conversation
Nutomic
requested review from
SleeplessOne1917,
dessalines,
dullbananas and
phiresky
as code owners
Nutomic
force-pushed
the
email-registration-denied
branch
from
ac86acd to
7425e19
Compare
There was a problem hiding this comment.
Lets use proper errors here now. Go through every function, make sure they return a LemmyResult<()>, forwarded from send_user_email
There was a problem hiding this comment.
Check L59 here, and remove the LemmyErrorType::InvalidEmailAddress as an error type (from errors.rs), and replace it with EmailSendFailed.
Then we can be sure to never forward email enumeration errors. Maybe add a comment about that also.
There was a problem hiding this comment.
But those are completely different errors.
There was a problem hiding this comment.
Kind of, but these are also our public errors, and we can be sure that we never leak that info to users. And then we can return results.
| if local_user_view.person.banned || !local_user_view.local_user.send_notifications_to_email { | ||
| return; |
There was a problem hiding this comment.
Lets return proper errors for the banned case.
if banned {
...
} else if !send_notifications {
...
Ok(())
} else {
...
send_email(...).await
}
There was a problem hiding this comment.
This is going to break if someone replies to a banned user, then the create post call will return an error from the notification email send. So not possible.
There was a problem hiding this comment.
Can't this be handled with .ok() at the API level?
| } | ||
|
|
||
| if let Some(user_email) = &local_user_view.local_user.email { | ||
| match send_email( |
There was a problem hiding this comment.
No need for this match, just forward the error, but ignore it using .ok() at the API level as necessary.
There was a problem hiding this comment.
I also feel like it might be cleaner to pass local_user.send_notifications_to_email as a required param to the send_email function. Then you can remove that check above.
There was a problem hiding this comment.
Same as above, best to leave it as is.
| deny_reason.unwrap_or("unknown".to_string()), | ||
| ); | ||
| send_email(&subject, &email, &user.person.name, &body, settings).await?; | ||
| Ok(()) |
| let email = user_email(user)?; | ||
| let body = lang.email_verified_body(); | ||
| send_email(&subject, &email, &user.person.name, body, settings).await?; | ||
| Ok(()) |
| context.settings(), | ||
| ) | ||
| .await | ||
| .await; |
There was a problem hiding this comment.
This will probably need an .ok()
There was a problem hiding this comment.
No because those functions dont return a result (same as before).
| context.settings(), | ||
| ) | ||
| .await | ||
| .await?; |
| context.settings(), | ||
| ) | ||
| .await | ||
| .await?; |
dessalines
left a comment
dessalines
left a comment
There was a problem hiding this comment.
My concerns are minor, you can merge as you see fit.
2 participants
Send out email when registration application is denied, and when email is confirmed. There were already some strings for these, so it seems like they were previously added and then removed?
While doing this I also noticed that email logic is very messy and spread across the whole codebase. So I decided to move it into a separate crate. This way its easier to manage, and also doesnt have to be rebuilt for unrelated changes.
Requires LemmyNet/lemmy-translations#159