UX: Improve email testing admin tool. #6308
Conversation
|
You've signed the CLA, nbianca. Thank you! This pull request is ready for review. |
| function() { | ||
| self.set("sentTestEmail", true); | ||
| function(response) { | ||
| self.set("sentTestEmailMessage", response.sentTestEmailMessage); |
There was a problem hiding this comment.
While you're in there, you could improve this to use the (response) => form of functions, which would mean you could use this instead of self. I'd like to remove the self pattern from our code.
| @@ -11,7 +11,13 @@ def test | |||
| params.require(:email_address) | |||
| begin | |||
| Jobs::TestEmail.new.execute(to_address: params[:email_address]) | |||
| render body: nil | |||
| if SiteSetting.disable_emails == "yes" | |||
| render json: { sentTestEmailMessage: 'admin.email.sent_test_disabled' } | |||
There was a problem hiding this comment.
It would be better to directly respond with the translated string instead of the name of the string
nbianca
force-pushed
the
send_email
branch
2 times, most recently
from
68e2efc
to
d448ac9
Compare
| elsif SiteSetting.disable_emails == "non-staff" && !User.find_by_email(params[:email_address])&.staff? | ||
| render json: { sentTestEmailMessage: I18n.t("admin.email.sent_test_disabled_for_non_staff") } | ||
| else | ||
| render json: { sentTestEmailMessage: I18n.t("admin.email.sent_test") } |
There was a problem hiding this comment.
Our convention is to use snake_case instead of camelCase on the server side even for keys in the payload. We should add a spec for this as well 😸
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/email-test-area-should-warn-if-emails-are-disabled/95269/6 |
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/send-test-email-doesnt-show-a-status/109272/3 |
Fixes this.