Create Settings::SMTP (#13943)

* Fix typo

* Add SMTP configs to Settings::General

* Expand Settings's show page

* Expand Settings::General's constants

* Move smtp_enabled? logic to ApplicationMailer

* Apply suggestions from code review

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

* Add missing descriptions and placeholders

* Remove production guard clause

* Change delivery_method to a callback

* Create Settings::SMTP

* Run migration

* Move constants

* Remove SMTP from Settings::General

* Create SMTPSettingsController

* Add back guard clause

* Change which perform_deliveries configuration to use from

* Update config/environments/production.rb

* Rename migration to singular

* Run migration again

* Fix name

* Alphabetize and add validation for authentication

* Move settings and enabled? logic to Settings::SMTP

* Change after_action to before_action

* Fix broken spec

* Fix broken spec

* Create SMTP specs

* Provide default for port

* Create spec for ApplicationMailer

* Fix broken spec

* Move smtp_enabled?

* Search and replace

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

app/controllers/admin/settings/smtp_settings_controller.rb

@@ -0,0 +1,9 @@
+module Admin
+  module Settings
+    class SMTPSettingsController < Admin::Settings::BaseController
+      def authorization_resource
+        ::Settings::SMTP
+      end
+    end
+  end
+end

app/lib/constants/settings/smtp.rb

@@ -0,0 +1,33 @@
+module Constants
+  module Settings
+    module SMTP
+      DETAILS = {
+        address: {
+          description: "Address of the remote mail server",
+          placeholder: "ie. 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"
+        },
+        user_name: {
+          description: "If your mail server requires authentication, copy the username from your server",
+          placeholder: ""
+        },
+        password: {
+          description: "If your mail server requires authentication, copy the password from your server",
+          placeholder: ""
+        },
+        domain: {
+          description: "If you need to specify a HELO domain, you can do it here",
+          placeholder: ""
+        }
+      }.freeze
+    end
+  end
+end

app/mailers/application_mailer.rb

@@ -8,6 +8,8 @@ class ApplicationMailer < ActionMailer::Base
   helper AuthenticationHelper
 
   before_action :use_custom_host
+  before_action :set_delivery_options
+  before_action :set_perform_deliveries
 
   default(
     from: -> { email_from },
@@ -36,4 +38,14 @@ class ApplicationMailer < ActionMailer::Base
   def use_custom_host
     ActionMailer::Base.default_url_options[:host] = Settings::General.app_domain
   end
+
+  def set_perform_deliveries
+    self.perform_deliveries = ForemInstance.smtp_enabled?
+  end
+
+  protected
+
+  def set_delivery_options
+    self.smtp_settings = Settings::SMTP.settings
+  end
 end

app/models/forem_instance.rb

@@ -25,6 +25,6 @@ class ForemInstance
   end
 
   def self.smtp_enabled?
-    Rails.configuration.action_mailer.perform_deliveries
+    (Settings::SMTP.user_name.present? && Settings::SMTP.password.present?) || ENV["SENDGRID_API_KEY"].present?
   end
 end

app/models/settings/smtp.rb

@@ -0,0 +1,40 @@
+module Settings
+  class SMTP < RailsSettings::Base
+    self.table_name = :settings_smtp
+
+    OPTIONS = %i[address port authentication user_name password domain].freeze
+
+    # The configuration is cached, change this if you want to force update
+    # the cache, or call Settings::Smtp.clear_cache
+    cache_prefix { "v1" }
+
+    field :address, type: :string, default: ApplicationConfig["SMTP_ADDRESS"]
+    field :authentication, type: :string, default: ApplicationConfig["SMTP_AUTHENTICATION"],
+                           validates: { inclusion: %w[plain login cram_md5] }
+    field :domain, type: :string, default: ApplicationConfig["SMTP_DOMAIN"]
+    field :password, type: :string, default: ApplicationConfig["SMTP_PASSWORD"]
+    field :port, type: :integer, default: ApplicationConfig["SMTP_PORT"].presence || 25
+    field :user_name, type: :string, default: ApplicationConfig["SMTP_USER_NAME"]
+
+    class << self
+      def settings
+        return sendgrid_settings if ENV["SENDGRID_API_KEY"].present?
+
+        keys.index_with { |k| public_send(k) }.symbolize_keys
+      end
+
+      private
+
+      def sendgrid_settings
+        {
+          address: "smtp.sendgrid.net",
+          port: 587,
+          authentication: :plain,
+          user_name: "apikey",
+          password: ENV["SENDGRID_API_KEY"],
+          domain: ::Settings::General.app_domain
+        }
+      end
+    end
+  end
+end

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

@@ -1036,6 +1036,29 @@
           </div>
         <% end %>
 
+        <%= form_for(Settings::SMTP.new, url: admin_settings_smtp_settings_path) do |f| %>
+          <div class="card mt-3">
+            <%= render partial: "admin/shared/card_header",
+                       locals: { header: "SMTP Settings", 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] %>
+
+                  </div>
+                <% end %>
+                <%= render "form_submission", f: f %>
+              </fieldset>
+            </div>
+          </div>
+        <% end %>
+
         <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
           <div class="card mt-3">
             <%= render partial: "admin/shared/card_header",

config/environments/production.rb

@@ -127,27 +127,8 @@ Rails.application.configure do
   protocol = ENV["APP_PROTOCOL"] || "http://"
 
   config.action_mailer.delivery_method = :smtp
-  config.action_mailer.perform_deliveries =  ENV["SMTP_PASWORD"].present? || ENV["SENDGRID_API_KEY"].present?
+  config.action_mailer.perform_deliveries = false
   config.action_mailer.default_url_options = { host: protocol + ENV["APP_DOMAIN"].to_s }
-  ActionMailer::Base.smtp_settings = if ENV["SENDGRID_API_KEY"].present?
-                                       {
-                                         address: "smtp.sendgrid.net",
-                                         port: 587,
-                                         authentication: :plain,
-                                         user_name: "apikey",
-                                         password: ENV["SENDGRID_API_KEY"],
-                                         domain: ENV["APP_DOMAIN"]
-                                       }
-                                     else
-                                       {
-                                         address: ENV["SMTP_ADDRESS"],
-                                         port: ENV["SMTP_PORT"],
-                                         authentication: ENV["SMTP_AUTHENTICATION"],
-                                         user_name: ENV["SMTP_USER_NAME"],
-                                         password: ENV["SMTP_PASSWORD"],
-                                         domain: ENV["SMTP_DOMAIN"]
-                                       }
-                                     end
 
   if ENV["HEROKU_APP_URL"].present? && ENV["HEROKU_APP_URL"] != ENV["APP_DOMAIN"]
     config.middleware.use Rack::HostRedirect,

config/initializers/inflections.rb

@@ -11,6 +11,6 @@
 # end
 
 # These inflection rules are supported but not enabled by default:
-# ActiveSupport::Inflector.inflections(:en) do |inflect|
-#   inflect.acronym 'RESTful'
-# end
+ActiveSupport::Inflector.inflections(:en) do |inflect|
+  inflect.acronym "SMTP"
+end

config/routes/admin.rb

@@ -21,6 +21,7 @@ namespace :admin do
     resources :general_settings, only: [:create]
     resources :mandatory_settings, only: [:create]
     resources :rate_limits, only: [:create]
+    resources :smtp_settings, only: [:create]
     resources :user_experiences, only: [:create]
   end
   namespace :users do

db/migrate/20210609164958_create_settings_smtp.rb

@@ -0,0 +1,16 @@
+class CreateSettingsSMTP < ActiveRecord::Migration[6.1]
+  def self.up
+    create_table :settings_smtp do |t|
+      t.string :var, null: false
+      t.text :value, null: true
+
+      t.timestamps
+    end
+
+    add_index :settings_smtp, :var, unique: true
+  end
+
+  def self.down
+    drop_table :settings_smtp
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2021_06_09_103939) do
+ActiveRecord::Schema.define(version: 2021_06_09_164958) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
@@ -1118,6 +1118,14 @@ ActiveRecord::Schema.define(version: 2021_06_09_103939) do
     t.index ["var"], name: "index_settings_rate_limits_on_var", unique: true
   end
 
+  create_table "settings_smtp", force: :cascade do |t|
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.text "value"
+    t.string "var", null: false
+    t.index ["var"], name: "index_settings_smtp_on_var", unique: true
+  end
+
   create_table "settings_user_experiences", force: :cascade do |t|
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false

spec/mailers/application_mailer_spec.rb

@@ -0,0 +1,27 @@
+require "rails_helper"
+
+RSpec.describe ApplicationMailer, type: :mailer do
+  let(:user) { create(:user) }
+  let(:email) { VerificationMailer.with(user_id: user.id).account_ownership_verification_email }
+
+  describe "#set_perform_deliveries" do
+    it "changes perform_deliveries from true to false if smtp is not enabled" do
+      expect { email.deliver_now }.to change(described_class, :perform_deliveries).from(true).to(false)
+    end
+
+    it "changes perform_deliveries from false to true if smtp is enabled" do
+      described_class.perform_deliveries = false
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
+
+      expect { email.deliver_now }.to change(described_class, :perform_deliveries).from(false).to(true)
+    end
+  end
+
+  describe "#set_delivery_options" do
+    it "evoked Settings::SMTP.settings during callback" do
+      allow(Settings::SMTP).to receive(:settings)
+      email.deliver_now
+      expect(Settings::SMTP).to have_received(:settings)
+    end
+  end
+end

spec/mailers/notify_mailer_spec.rb

@@ -260,6 +260,8 @@ RSpec.describe NotifyMailer, type: :mailer do
     end
 
     it "tracks the feedback message ID after delivery" do
+      user # Instantiate user to pre-generate welcome email
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
       assert_emails 1 do
         email.deliver_now
       end
@@ -268,6 +270,8 @@ RSpec.describe NotifyMailer, type: :mailer do
     end
 
     it "tracks the email_type as the UTM campaign" do
+      user # Instantiate user to pre-generate welcome email
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
       assert_emails 1 do
         email.deliver_now
       end

spec/models/follow_spec.rb

@@ -81,6 +81,7 @@ RSpec.describe Follow, type: :model do
     end
 
     it "sends an email notification" do
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
       user_2.update_column(:email_follower_notifications, true)
       expect do
         Sidekiq::Testing.inline! do

spec/models/forem_instance_spec.rb

@@ -76,4 +76,23 @@ RSpec.describe ForemInstance, type: :model do
       expect(described_class.dev_to?).to be(false)
     end
   end
+
+  describe ".smtp_enabled?" do
+    it "return false when no credential is provided" do
+      expect(described_class.smtp_enabled?).to be(false)
+    end
+
+    it "returns true if user_name and password are present" do
+      allow(Settings::SMTP).to receive(:user_name).and_return("something")
+      allow(Settings::SMTP).to receive(:password).and_return("something")
+
+      expect(described_class.smtp_enabled?).to be(true)
+    end
+
+    it "returns true if sendgrid api key is available" do
+      ENV["SENDGRID_API_KEY"] = "something"
+      expect(described_class.smtp_enabled?).to be(true)
+      ENV["SENDGRID_API_KEY"] = nil
+    end
+  end
 end

spec/models/message_spec.rb

@@ -122,6 +122,7 @@ RSpec.describe Message, type: :model do
 
   context "when callbacks are triggered after create" do
     before do
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
       chat_channel.add_users([user, user2])
     end
 

spec/models/settings/smtp_spec.rb

@@ -0,0 +1,40 @@
+require "rails_helper"
+
+RSpec.describe Settings::SMTP do
+  describe "::settings" do
+    it "use default sendgrid config if SENDGRID_API_KEY is available" do
+      key = "something"
+      domain = "test.com"
+      allow(ApplicationConfig).to receive(:[]).with("SENDGRID_API_KEY").and_return(key)
+      ENV["SENDGRID_API_KEY"] = "something"
+      allow(Settings::General).to receive(:app_domain).and_return(domain)
+
+      expect(described_class.settings).to eq({
+                                               address: "smtp.sendgrid.net",
+                                               port: 587,
+                                               authentication: :plain,
+                                               user_name: "apikey",
+                                               password: key,
+                                               domain: domain
+                                             })
+      ENV["SENDGRID_API_KEY"] = nil
+    end
+
+    it "uses Settings::SMTP config if SENDGRID_API_KEY is not available" do
+      described_class.address = "smtp.google.com"
+      described_class.port = 25
+      described_class.authentication = "plain"
+      described_class.user_name = "username"
+      described_class.password = "password"
+
+      expect(described_class.settings).to eq({
+                                               address: "smtp.google.com",
+                                               port: 25,
+                                               authentication: "plain",
+                                               user_name: "username",
+                                               password: "password",
+                                               domain: ""
+                                             })
+    end
+  end
+end

spec/requests/admin/chat_channel_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe "/admin/apps/chat_channels", type: :request do
     around { |example| perform_enqueued_jobs(&example) }
 
     it "creates chat_channel for with users as moderator" do
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
       user.add_role(:super_admin)
       sign_in user
       expect do

spec/requests/admin/feedback_messages_spec.rb

@@ -86,6 +86,7 @@ RSpec.describe "/admin/moderation/reports", type: :request do
       end
 
       it "creates a new email message with the same params" do
+        allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
         post send_email_admin_reports_path, params: send_email_params
 
         expect(EmailMessage.last.attributes).to include(email_message_attributes)

spec/requests/admin/users_spec.rb

@@ -121,6 +121,7 @@ RSpec.describe "/admin/users", type: :request do
 
   describe "POST /admin/users/:id/verify_email_ownership" do
     it "allows a user to verify email ownership" do
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
       post verify_email_ownership_admin_user_path(user.id), params: { user_id: user.id }
 
       path = verify_email_authorizations_path(

spec/requests/feedback_messages_spec.rb

@@ -3,6 +3,10 @@ require "rails_helper"
 RSpec.describe "feedback_messages", type: :request do
   let(:user) { create(:user) }
 
+  before do
+    allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
+  end
+
   describe "POST /feedback_messages" do
     def mock_recaptcha_verification
       # rubocop:disable RSpec/AnyInstance

spec/requests/user/user_settings_spec.rb

@@ -321,6 +321,7 @@ RSpec.describe "UserSettings", type: :request do
       end
 
       it "sends an email" do
+        allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
         expect do
           sidekiq_perform_enqueued_jobs do
             send_request

spec/services/exporter/service_spec.rb

@@ -8,7 +8,11 @@ RSpec.describe Exporter::Service, type: :service do
   let(:other_user_article) { create(:article, user: other_user) }
 
   before do
-    ActionMailer::Base.deliveries.clear
+    allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
+  end
+
+  after do
+    ApplicationMailer.deliveries.clear
   end
 
   def valid_instance(user)
@@ -71,6 +75,7 @@ RSpec.describe Exporter::Service, type: :service do
     context "when emailing the user" do
       it "delivers one email" do
         service = valid_instance(article.user)
+        ApplicationMailer.deliveries.clear
         service.export(article.user.email)
         expect(ActionMailer::Base.deliveries.count).to eq(1)
       end

spec/system/admin/user_email_tools_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe "Admin Email Tools within User management", type: :system do
 
   context "when looking at a user with recorded emails in the admin panel" do
     before do
+      allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
       sign_in admin
       visit admin_user_path(user)
 

spec/workers/organizations/delete_worker_spec.rb

@@ -38,6 +38,7 @@ RSpec.describe Organizations::DeleteWorker, type: :worker do
       end
 
       it "sends the notification" do
+        allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
         expect do
           worker.perform(org.id, user.id)
         end.to change(ActionMailer::Base.deliveries, :count).by(1)

spec/workers/users/delete_worker_spec.rb

@@ -6,6 +6,10 @@ RSpec.describe Users::DeleteWorker, type: :worker do
   let(:mailer) { double }
   let(:message_delivery) { double }
 
+  before do
+    allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
+  end
+
   describe "#perform" do
     let!(:user) { create(:user) }
     let(:delete) { Users::Delete }