Merge pull request #12384 from mkllnk/mail-config

Don't pass invalid auth method "None" to net-smtp
openfoodfoundation · Apr 18, 2024 · 5b04357 · 5b04357
2 parents e59237e + 5cd53e0
commit 5b04357
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 61 deletions.
diff --git a/lib/spree/core/mail_settings.rb b/lib/spree/core/mail_settings.rb
@@ -20,29 +20,22 @@ def override!
       private
 
       def mail_server_settings
-        settings = if need_authentication?
-                     basic_settings.merge(user_credentials)
-                   else
-                     basic_settings
-                   end
-
-        settings.merge(enable_starttls_auto: secure_connection?)
-      end
-
-      def user_credentials
-        { user_name: Config.smtp_username,
-          password: Config.smtp_password }
-      end
-
-      def basic_settings
-        { address: Config.mail_host,
+        {
+          address: Config.mail_host,
           domain: Config.mail_domain,
           port: Config.mail_port,
-          authentication: Config.mail_auth_type }
+          authentication:,
+          enable_starttls_auto: secure_connection?,
+          user_name: Config.smtp_username.presence,
+          password: Config.smtp_password.presence,
+        }
       end
 
-      def need_authentication?
-        Config.mail_auth_type != 'None'
+      def authentication
+        # "None" is an option in the UI but not a real authentication type.
+        # We should remove it from our host configurations but I'm keeping
+        # this check for backwards-compatibility for now.
+        Config.mail_auth_type.presence unless Config.mail_auth_type == "None"
       end
 
       def secure_connection?

diff --git a/spec/lib/spree/core/mail_settings_spec.rb b/spec/lib/spree/core/mail_settings_spec.rb
@@ -2,52 +2,37 @@
 
 require 'spec_helper'
 
-module Spree
-  module Core
-    describe MailSettings do
-      let!(:subject) { MailSettings.new }
-
-      context "overrides appplication defaults" do
-        context "authentication method is none" do
-          before do
-            Config.mail_host = "smtp.example.com"
-            Config.mail_domain = "example.com"
-            Config.mail_port = 123
-            Config.mail_auth_type = MailSettings::SECURE_CONNECTION_TYPES[0]
-            Config.smtp_username = "schof"
-            Config.smtp_password = "hellospree!"
-            Config.secure_connection_type = "TLS"
-            subject.override!
-          end
-
-          it { expect(ActionMailer::Base.smtp_settings[:address]).to eq "smtp.example.com" }
-          it { expect(ActionMailer::Base.smtp_settings[:domain]).to eq "example.com" }
-          it { expect(ActionMailer::Base.smtp_settings[:port]).to eq 123 }
-          it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq "None" }
-          it { expect(ActionMailer::Base.smtp_settings[:enable_starttls_auto]).to be_truthy }
+describe Spree::Core::MailSettings do
+  context "overrides appplication defaults" do
+    context "authentication method is login" do
+      before do
+        Spree::Config.mail_host = "smtp.example.com"
+        Spree::Config.mail_domain = "example.com"
+        Spree::Config.mail_port = 123
+        Spree::Config.mail_auth_type = "login"
+        Spree::Config.smtp_username = "schof"
+        Spree::Config.smtp_password = "hellospree!"
+        Spree::Config.secure_connection_type = "TLS"
+        subject.override!
+      end
 
-          it "doesnt touch user name config" do
-            expect(ActionMailer::Base.smtp_settings[:user_name]).to be_nil
-          end
+      it { expect(ActionMailer::Base.smtp_settings[:address]).to eq "smtp.example.com" }
+      it { expect(ActionMailer::Base.smtp_settings[:domain]).to eq "example.com" }
+      it { expect(ActionMailer::Base.smtp_settings[:port]).to eq 123 }
+      it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq "login" }
+      it { expect(ActionMailer::Base.smtp_settings[:enable_starttls_auto]).to be_truthy }
+      it { expect(ActionMailer::Base.smtp_settings[:user_name]).to eq "schof" }
+      it { expect(ActionMailer::Base.smtp_settings[:password]).to eq "hellospree!" }
+    end
 
-          it "doesnt touch password config" do
-            expect(ActionMailer::Base.smtp_settings[:password]).to be_nil
-          end
-        end
+    context "authentication method is none" do
+      before do
+        Spree::Config.mail_auth_type = "None"
+        subject.override!
       end
 
-      context "when mail_auth_type is other than none" do
-        before do
-          Config.mail_auth_type = "login"
-          Config.smtp_username = "schof"
-          Config.smtp_password = "hellospree!"
-          subject.override!
-        end
-
-        context "overrides user credentials" do
-          it { expect(ActionMailer::Base.smtp_settings[:user_name]).to eq "schof" }
-          it { expect(ActionMailer::Base.smtp_settings[:password]).to eq "hellospree!" }
-        end
+      it "doesn't store 'None' as auth method" do
+        expect(ActionMailer::Base.smtp_settings[:authentication]).to eq nil
       end
     end
   end