Improve the UX of the SMTP Section

app/controllers/stories/feeds_controller.rb

@@ -65,8 +65,8 @@ def signed_in_base_feed
     end
 
     def signed_out_base_feed
-      strategy = AbExperiment.get(experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, controller: self, user: current_user,
-                                  default_value: AbExperiment::ORIGINAL_VARIANT)
+      strategy = AbExperiment.get(experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, controller: self,
+                                  user: current_user, default_value: AbExperiment::ORIGINAL_VARIANT)
       feed = if strategy.weighted_query_strategy?
                Articles::Feeds::WeightedQueryStrategy.new(user: current_user, page: @page, tags: params[:tag])
              elsif Settings::UserExperience.feed_strategy == "basic"

app/javascript/admin/controllers/config_controller.js

@@ -16,6 +16,8 @@ const emailAuthModalBody = `
   <p>If you disable Email address as a registration option, people cannot create an account with their email address.</p>
   <p>However, people who have already created an account using their email address can continue to login.</p>`;
 
+// NOTE: In an effort to move away from Stimulus and create consistency across the codebase
+// we are using vanilla JavaScript (app/javascript/packs/admin/config) to handle any new interactions.
 export default class ConfigController extends Controller {
   static targets = [
     'authenticationProviders',

app/javascript/packs/admin/config/smtp.js

@@ -0,0 +1,23 @@
+document
+  .getElementById('settings_smtp_own_email_server')
+  ?.addEventListener('change', (event) => {
+    const [customSMTPSection] = document.getElementsByClassName(
+      'js-custom-smtp-section',
+    );
+
+    if (event.target.checked) {
+      customSMTPSection.classList.remove('hidden');
+    } else {
+      // when the user indicates that they do not want to use their own server
+      // we clear the form values except for those fields that have a default value.
+      const inputs = customSMTPSection.getElementsByTagName('input');
+      const haveDefaultValues = ['authentication'];
+
+      for (const input of inputs) {
+        if (!haveDefaultValues.some((el) => input.name.includes(el))) {
+          input.value = '';
+        }
+      }
+      customSMTPSection.classList.add('hidden');
+    }
+  });

app/lib/constants/settings/smtp.rb

@@ -4,16 +4,17 @@ module SMTP
       DETAILS = {
         address: {
           description: "Address of the remote mail server",
-          placeholder: "ie. smtp.gmail.com"
+          placeholder: "i.e. smtp.gmail.com"
         },
         port: {
           description: "The port that your mail server runs on",
           placeholder: "25"
         },
         authentication: {
           description: " If your mail server requires authentication, " \
-                       "you need to specify the authentication type here",
-          placeholder: "ie. plain, login, or cram_md5"
+                       "you need to specify the authentication type here. " \
+                       " i.e. plain, login, or cram_md5",
+          placeholder: "i.e. plain, login, or cram_md5"
         },
         user_name: {
           description: "If your mail server requires authentication, copy the username from your server",

app/models/forem_instance.rb

@@ -32,6 +32,14 @@ def self.smtp_enabled?
     Settings::SMTP.provided_minimum_settings? || ENV["SENDGRID_API_KEY"].present?
   end
 
+  def self.sendgrid_enabled?
+    ENV["SENDGRID_API_KEY"].present?
+  end
+
+  def self.only_sendgrid_enabled?
+    ForemInstance.sendgrid_enabled? && !Settings::SMTP.provided_minimum_settings?
+  end
+
   def self.invitation_only?
     Settings::Authentication.invite_only_mode?
   end

app/models/settings/smtp.rb

@@ -2,11 +2,12 @@ module Settings
   class SMTP < Base
     self.table_name = :settings_smtp
 
-    OPTIONS = %i[address port authentication user_name password domain].freeze
+    OPTIONS = %i[user_name password address authentication domain port].freeze
+    AUTHENTICATION_METHODS = %w[plain login cram_md5].freeze
 
     setting :address, type: :string, default: ApplicationConfig["SMTP_ADDRESS"].presence
     setting :authentication, type: :string, default: ApplicationConfig["SMTP_AUTHENTICATION"].presence,
-                             validates: { inclusion: %w[plain login cram_md5] }
+                             validates: { inclusion: AUTHENTICATION_METHODS }
     setting :domain, type: :string, default: ApplicationConfig["SMTP_DOMAIN"].presence
     setting :password, type: :string, default: ApplicationConfig["SMTP_PASSWORD"].presence
     setting :port, type: :integer, default: ApplicationConfig["SMTP_PORT"].presence || 25

app/services/articles/feeds/weighted_query_strategy.rb

@@ -119,7 +119,7 @@ class WeightedQueryStrategy
         # Weight to give to the number of comments on the article.
         comments_count_factor: {
           clause: "articles.comments_count",
-          cases: (0..9).map { |n| [n, 0.8 + 0.02 * n] },
+          cases: (0..9).map { |n| [n, 0.8 + (0.02 * n)] },
           fallback: 1,
           requires_user: false,
           group_by: "articles.comments_count"
@@ -190,7 +190,7 @@ class WeightedQueryStrategy
         # user follows and the article has.
         matching_tags_factor: {
           clause: "LEAST(10.0, SUM(followed_tags.points))::integer",
-          cases: (0..9).map { |n| [n, 0.70 + 0.0303 * n] },
+          cases: (0..9).map { |n| [n, 0.70 + (0.0303 * n)] },
           fallback: 1,
           requires_user: true,
           joins: ["LEFT OUTER JOIN taggings

app/views/admin/settings/forms/_smtp.html.erb

@@ -1,22 +1,47 @@
+<%= javascript_packs_with_chunks_tag "admin/config/smtp", defer: true %>
+
 <%= form_for(Settings::SMTP.new,
              url: admin_settings_smtp_settings_path,
-             html: { data: { action: "submit->config#updateConfigurationSettings" } }) do |f| %>
+             html: { data: {
+               action: "submit->config#updateConfigurationSettings",
+               testid: "emailServerSettings"
+             } }) do |f| %>
   <div class="card mt-3" id="smtp-section">
     <%= render partial: "admin/shared/card_header",
-               locals: { header: "SMTP Settings", state: "collapse", target: "smtpSettingsBodyContainer", expanded: false } %>
+               locals: { header: "Email Server Settings (SMTP)", state: "collapse", target: "smtpSettingsBodyContainer", expanded: false } %>
     <div id="smtpSettingsBodyContainer" class="card-body collapse hide" aria-labelledby="smtpSettingsBodyContainer">
       <fieldset class="grid gap-4">
-        <% Settings::SMTP::OPTIONS.each do |config_key| %>
-          <div class="crayons-field">
-            <%= admin_config_label config_key %>
-            <%= admin_config_description Constants::Settings::SMTP::DETAILS[config_key][:description] %>
-            <%= f.text_field config_key,
-                             class: "crayons-textfield",
-                             value: Settings::SMTP.public_send(config_key),
-                             placeholder: Constants::Settings::SMTP::DETAILS[config_key][:placeholder] %>
 
+        <% if ForemInstance.sendgrid_enabled? %>
+          <div class="crayons-field crayons-field--checkbox mt-3">
+            <%= f.check_box :own_email_server,
+                            checked: Settings::SMTP.provided_minimum_settings?,
+                            class: "crayons-checkbox" %>
+            <%= f.label :own_email_server, class: "crayons-field__label" do %>
+              Use my own email server
+            <% end %>
           </div>
         <% end %>
+
+        <% if ForemInstance.only_sendgrid_enabled? %>
+          <p class="mb-3">
+            As a Forem Cloud client, we provide an email server managed by the Forem team. All settings are managed by us and the from and reply email addresses are set as <%= ForemInstance.email %>. However, you can override this to use your own email server.
+          </p>
+        <% end %>
+
+        <div class="js-custom-smtp-section <%= "hidden" if ForemInstance.only_sendgrid_enabled? %>">
+          <% Settings::SMTP::OPTIONS.each do |config_key| %>
+            <div class="crayons-field mt-3">
+              <%= admin_config_label config_key, model: Settings::SMTP %>
+              <%= admin_config_description Constants::Settings::SMTP::DETAILS[config_key][:description] %>
+              <%= f.text_field config_key,
+                               class: "crayons-textfield",
+                               value: Settings::SMTP.public_send(config_key),
+                               placeholder: Constants::Settings::SMTP::DETAILS[config_key][:placeholder],
+                               "data-config-target": "smtpSetting#{config_key.to_s.camelize(:upper)}" %>
+            </div>
+          <% end %>
+        </div>
       </fieldset>
       <%= render "update_setting_button", f: f %>
     </div>

app/views/admin/settings/show.html.erb

@@ -34,6 +34,7 @@
         <%= render partial: "forms/community" %>
         <%= render partial: "forms/credits" %>
         <%= render partial: "forms/emails" %>
+        <%= render partial: "forms/smtp" %>
         <%= render partial: "forms/google_analytics" %>
         <%= render partial: "forms/images", locals: { logo_allowed_types: @logo_allowed_types, logo_max_file_size: @logo_max_file_size } %>
         <%= render partial: "forms/mascot" %>
@@ -42,7 +43,6 @@
         <%= render partial: "forms/newsletter" %>
         <%= render partial: "forms/onboarding" %>
         <%= render partial: "forms/rate_limit" %>
-        <%= render partial: "forms/smtp" %>
         <%= render partial: "forms/sponsors" %>
         <%= render partial: "forms/tags" %>
         <%= render partial: "forms/user_experience" %>

cypress/integration/seededFlows/adminFlows/config/emailServerSettingsSection.spec.js

@@ -0,0 +1,36 @@
+describe('Email Server Settings Section', () => {
+  beforeEach(() => {
+    cy.testSetup();
+    cy.fixture('users/adminUser.json').as('user');
+
+    cy.get('@user').then((user) => {
+      cy.loginAndVisit(user, '/admin/customization/config');
+    });
+  });
+
+  describe('email server settings', () => {
+    it('updates the smtp fields', () => {
+      cy.findByTestId('emailServerSettings').as('emailServerSettings');
+
+      cy.get('@emailServerSettings')
+        .findByText('Email Server Settings (SMTP)')
+        .click();
+
+      cy.get('@emailServerSettings').within(() => {
+        cy.findByText('Email Server Settings (SMTP)').click();
+        cy.findByLabelText('User name').clear().type('jane_doe');
+        cy.findByLabelText('Password').clear().type('abc123456');
+        cy.findByLabelText('Address').clear().type('smtp.gmail.com');
+        cy.findByLabelText('Authentication').clear().type('plain');
+        cy.findByText('Update Settings').click();
+      });
+
+      cy.url().should('contains', '/admin/customization/config');
+      cy.findByText('Successfully updated settings.').should('be.visible');
+      cy.findByLabelText('User name').should('have.value', 'jane_doe');
+      cy.findByLabelText('Password').should('have.value', 'abc123456');
+      cy.findByLabelText('Address').should('have.value', 'smtp.gmail.com');
+      cy.findByLabelText('Authentication').should('have.value', 'plain');
+    });
+  });
+});

cypress/integration/seededFlows/adminFlows/pages/landingPage.spec.js

@@ -112,8 +112,7 @@ describe('Set a landing page from the admin portal', () => {
     cy.findByRole('main').within(() => {
       cy.findByRole('checkbox', {
         name: "Use as 'Locked Screen' Determines if this page will be used as a landing page for anonymous viewers.",
-      })
-        .check();
+      }).check();
 
       cy.findAllByRole('button', { name: 'Update Page' }).first().click();
     });
@@ -139,8 +138,7 @@ describe('Set a landing page from the admin portal', () => {
     cy.findByRole('main').within(() => {
       cy.findByRole('checkbox', {
         name: "Use as 'Locked Screen' Determines if this page will be used as a landing page for anonymous viewers.",
-      })
-        .check();
+      }).check();
 
       cy.findAllByRole('button', { name: 'Cancel' }).first().click();
 

spec/models/forem_instance_spec.rb

@@ -108,4 +108,29 @@
       expect(described_class.contact_email).to be(email)
     end
   end
+
+  describe ".only_sendgrid_enabled?" do
+    it "returns false when the minimum SMTP settings are provided" do
+      allow(Settings::SMTP).to receive(:user_name).and_return("something")
+      allow(Settings::SMTP).to receive(:password).and_return("something")
+      allow(Settings::SMTP).to receive(:address).and_return("something")
+
+      expect(described_class.only_sendgrid_enabled?).to be(false)
+    end
+
+    it "returns false when Sendgrid is not enabled" do
+      allow(described_class).to receive(:sendgrid_enabled?).and_return(false)
+      expect(described_class.only_sendgrid_enabled?).to be(false)
+    end
+
+    it "returns true if Sendgrid is enabled and the minimum SMTP settings are not provided" do
+      allow(described_class).to receive(:sendgrid_enabled?).and_return(true)
+
+      allow(Settings::SMTP).to receive(:user_name).and_return(nil)
+      allow(Settings::SMTP).to receive(:password).and_return(nil)
+      allow(Settings::SMTP).to receive(:address).and_return(nil)
+
+      expect(described_class.only_sendgrid_enabled?).to be(true)
+    end
+  end
 end

spec/system/admin/config/admin_updates_smtp_settings_spec.rb

@@ -0,0 +1,111 @@
+require "rails_helper"
+
+RSpec.describe "Admin updates SMTP Settings", type: :system do
+  let(:admin) { create(:user, :super_admin) }
+
+  before do
+    sign_in admin
+  end
+
+  # We're unable to set and unset an ENV variable in Cypress to test different scenarios
+  # hence, we test the view layouts with Capybara, and we test successful updates with Cypress.
+  context "when Sendgrid is not enabled and SMTP is not enabled" do
+    before do
+      allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(false)
+      allow(Settings::SMTP).to receive(:address).and_return(nil)
+      allow(Settings::SMTP).to receive(:user_name).and_return(nil)
+      allow(Settings::SMTP).to receive(:password).and_return(nil)
+      visit admin_config_path
+    end
+
+    it "does not show the 'Use my own email server' checkbox" do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).not_to have_content("Use my own email server")
+      end
+    end
+
+    it "shows the SMTP Form", :aggregate_failures do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).to have_selector(".js-custom-smtp-section")
+        expect(page).not_to have_selector(".js-custom-smtp-section.hidden")
+      end
+    end
+  end
+
+  context "when Sendgrid is enabled and SMTP is not enabled" do
+    before do
+      allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(true)
+      allow(ForemInstance).to receive(:email).and_return("yo@forem.com")
+      allow(Settings::SMTP).to receive(:address).and_return(nil)
+      allow(Settings::SMTP).to receive(:user_name).and_return(nil)
+      allow(Settings::SMTP).to receive(:password).and_return(nil)
+      visit admin_config_path
+    end
+
+    it "shows the checkbox to allow one to toggle ones own server" do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).to have_content("Use my own email server")
+      end
+    end
+
+    it "shows a description" do
+      within("form[data-testid='emailServerSettings']") do
+        # rubocop:disable Layout/LineLength
+        expect(page).to have_content("As a Forem Cloud client, we provide an email server managed by the Forem team. All settings are managed by us and the from and reply email addresses are set as yo@forem.com. However, you can override this to use your own email server.")
+        # rubocop:enable Layout/LineLength
+      end
+    end
+
+    it "does not show an SMTP Form" do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).to have_selector(".js-custom-smtp-section.hidden")
+      end
+    end
+  end
+
+  context "when Sendgrid is not enabled and SMTP is enabled" do
+    before do
+      allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(false)
+      allow(Settings::SMTP).to receive(:address).and_return("smtp.gmail.com")
+      allow(Settings::SMTP).to receive(:user_name).and_return("jane_doe")
+      allow(Settings::SMTP).to receive(:password).and_return("abc123456")
+      visit admin_config_path
+    end
+
+    it "does not show the 'Use my own email server' checkbox" do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).not_to have_content("Use my own email server")
+      end
+    end
+
+    it "shows the SMTP Form", :aggregate_failures do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).to have_selector(".js-custom-smtp-section")
+        expect(page).not_to have_selector(".js-custom-smtp-section.hidden")
+      end
+    end
+  end
+
+  context "when Sendgrid is enabled and SMTP is enabled" do
+    before do
+      allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(true)
+      allow(Settings::SMTP).to receive(:address).and_return("smtp.gmail.com")
+      allow(Settings::SMTP).to receive(:user_name).and_return("jane_doe")
+      allow(Settings::SMTP).to receive(:password).and_return("abc123456")
+      visit admin_config_path
+    end
+
+    it "shows the 'Use my own email server' checkbox" do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).to have_content("Use my own email server")
+      end
+    end
+
+    it "shows an SMTP Form", :aggregate_failures do
+      within("form[data-testid='emailServerSettings']") do
+        expect(page).to have_selector(".js-custom-smtp-section")
+        expect(page).not_to have_selector(".js-custom-smtp-section.hidden")
+      end
+    end
+  end
+end