Email preferences still show when emails are disabled

There are a couple of strange things that happen from a UX standpoint when all emails are disabled.
I would qualify these as bugs.

  1. 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.

from discourse/global-notice.js at master · discourse/discourse · GitHub

   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 FIX: Show 'emails disabled' to staff users when disabled for non-staf… · discourse/discourse@acc121f · GitHub in this fix, the idea seems to the emails disabled should have been shown to stuff only, but the specs discourse/email-notice-test.js at master · discourse/discourse · GitHub 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.

  1. 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 discourse/preferences.hbs at master · discourse/discourse · GitHub

    <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 :wink:

      {{#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

1 Like

I forgot to mention that I also wrap the whole template

with

  {{#if (show-emails-preferences model siteSettings.disable_emails)}}
    {{#unless siteSettings.disable_mailing_list_mode}}
…

Discourse cannot work without email.

Your can’t register an account, reset your password, get notifications, or use email logins. It’s an essential element. That banner is there because it’s telling everyone that the site is broken.

It’s not a bug that you can’t disable that warning. It’s a feature.

What about when you use only SSO for authentication?
It’s one of discourse features to disallow email logins and allow SSO as your only authentication system.
You can set both “enable local logins” && “enable local logins via email” to false.
As well as “disable emails” to “yes”.
Maybe it wouldn’t work as a standalone product on its own, but it can as a part of a bigger system. Also, if that was true, “discourse cannot work without email”, why would you have all these settings to break the system?

“That banner is there because it’s telling everyone that the site is broken.”
Again, I don’t think “everyone” needs to know.
At least in my opinion, only the staff or admin needs to know. Not the consumers of the product.

Btw, that’s how we’re using it. We have a main website where you do your login/logout/register/reset password, etc.
The only way to get to discourse is via SSO. That way we can easily provision permissions via groups.

1 Like

@pfaffman on a separate note, what did you think about that method?

Is that the right level/way (speaking about the discourse way) of doing things?

I haven’t had a lot of time to explore the code too much and extract a strict “discourse way” of coding things.

Ya, you might be on to something. I just encountered a discourse instance in the wild that has SSO enabled but it is also showing the “Email is disabled” banner and I’m not even logged in.

With SSO they won’t have to worry about password resets, so besides email notifications that people won’t receive I’m not sure why we HAVE to show this banner to everyone if SSO is enabled.

Maybe we at least update it to not show the banner if sso is enabled and you aren’t logged in?

2 Likes

@blake I personally think it’s a notification that should be shown to Staff only.
A regular user won’t be able to do anything about it, beside maybe opening a topic on why he’s seeing the message and what does it mean.
It’s just showing to the wrong people, that’s all :wink:

Btw: i’ve hidden it on my site via css, but I can see a case for the message to show, so that wouldn’t be the correct solution in the distro code.