There are a couple of strange things that happen from a UX standpoint when all emails are disabled.
I would qualify these as bugs.
- Everyone including non-staff people get a message about the system not sending emails. It seems there is no way to silence this notification. I can’t come up with a reason why users would be alerted about this all the time. Maybe it would ok to show staff or admins about it. It might have been a conscious decision at some point. I just couldn’t find a discussion about it.
if (
this.siteSettings.disable_emails === "yes" ||
this.siteSettings.disable_emails === "non-staff"
) {
notices.push(
Notice.create({
text: I18n.t("emails_are_disabled"), // "All outgoing email has been globally disabled by an administrator. No email notifications of any kind will be sent."
id: "alert-emails-disabled",
})
);
}
I think the guard should be changed to
if (
this.get("currentUser.staff") && // or currentUser.admin
(this.siteSettings.disable_emails === "yes" ||
this.siteSettings.disable_emails === "non-staff")
) {
…
Or at least have a setting to decide who gets this notification. (ie: everyone/staff/admin/no-one)
also https://github.com/discourse/discourse/commit/acc121fd27e53fb70c888b58939027fbefba9c6f in this fix, the idea seems to the emails disabled should have been shown to stuff only, but the specs https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/tests/acceptance/email-notice-test.js#L24 verify that everyone sees the notice
on a minor note: I find all the test("when …
a bit confusing. for example: When enabled really tests for disable_emails = "yes"
which, while I get tests for the a change in the settings, but it really tests for emails disabled, and viceversa When disabled
tests for emails enabled. I guess they refer more to the values being set than the final effect, which is probably the convention in the project. In which case is totally fine. Just odd from a newcomer standpoint.
- whether disable_emails is set to “yes” or “non-staff” the email section still shows up in the user preferences page for everyone. Which is strange because inside the template there are checks that disable portions of it when digests or mailing lists are disabled.
As you can see there’s no check https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/app/templates/preferences.hbs#L18
<li class="nav-emails">
{{#link-to "preferences.emails"}}
{{i18n "user.preferences_nav.emails"}}
{{/link-to}}
</li>
In my version I have something like this: (I don’t know if this is how you guys would do it, btw) I’m open to suggestions
{{#if (show-emails-preferences model siteSettings.disable_emails)}}
<li class="nav-emails">
{{#link-to "preferences.emails"}}
{{i18n "user.preferences_nav.emails"}}
{{/link-to}}
</li>
{{/if}}
where show-emails-preferences
is defined in a helper as follows:
registerUnbound("show-emails-preferences", (model, disable_emails, args) => {
let result = true;
if(disable_emails === 'yes'){
result = false;
}else if(disable_emails === 'non-staff'){
result = model.staff ? true : false;
}else{
result = true;
}
return result;
});
Let me know if any of these seem worthwhile improvements, and I can make a PR
Cheers