From a41019fbffcf02484fc522f38eb761d4e60fa41d Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 16 Apr 2024 15:41:15 +1000
Subject: [PATCH] Simplify SMTP auth method selection

Instead of using the auth method name, let's just not supply username
and password when we don't want to authenticate. The two affected
servers AU and CA don't have credentials set anyway. This is compatible.

The specs needed changing though.
---
 lib/spree/core/mail_settings.rb           | 21 +++++--------
 spec/lib/spree/core/mail_settings_spec.rb | 37 +++++++++--------------
 2 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/lib/spree/core/mail_settings.rb b/lib/spree/core/mail_settings.rb
index 07d8dbee7a..0373f7fac7 100644
--- a/lib/spree/core/mail_settings.rb
+++ b/lib/spree/core/mail_settings.rb
@@ -20,18 +20,14 @@ module Spree
       private
 
       def mail_server_settings
-        settings = if need_authentication?
-                     basic_settings.merge(user_credentials)
-                   else
-                     basic_settings
-                   end
+        settings = basic_settings.merge(user_credentials)
 
         settings.merge(enable_starttls_auto: secure_connection?)
       end
 
       def user_credentials
-        { user_name: Config.smtp_username,
-          password: Config.smtp_password }
+        { user_name: Config.smtp_username.presence,
+          password: Config.smtp_password.presence }
       end
 
       def basic_settings
@@ -42,13 +38,10 @@ module Spree
       end
 
       def authentication
-        return unless need_authentication?
-
-        Config.mail_auth_type.presence
-      end
-
-      def need_authentication?
-        Config.mail_auth_type != 'None'
+        # "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
index d1a2fc99b4..6a40495695 100644
--- a/spec/lib/spree/core/mail_settings_spec.rb
+++ b/spec/lib/spree/core/mail_settings_spec.rb
@@ -8,12 +8,12 @@ module Spree
       let!(:subject) { MailSettings.new }
 
       context "overrides appplication defaults" do
-        context "authentication method is none" do
+        context "authentication method is login" 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.mail_auth_type = "login"
             Config.smtp_username = "schof"
             Config.smtp_password = "hellospree!"
             Config.secure_connection_type = "TLS"
@@ -23,31 +23,22 @@ module Spree
           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 nil }
+          it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq "login" }
           it { expect(ActionMailer::Base.smtp_settings[:enable_starttls_auto]).to be_truthy }
-
-          it "doesnt touch user name config" do
-            expect(ActionMailer::Base.smtp_settings[:user_name]).to be_nil
-          end
-
-          it "doesnt touch password config" do
-            expect(ActionMailer::Base.smtp_settings[:password]).to be_nil
-          end
-        end
-      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
+
+        context "authentication method is none" do
+          before do
+            Config.mail_auth_type = "None"
+            subject.override!
+          end
+
+          it "doesn't store 'None' as auth method" do
+            expect(ActionMailer::Base.smtp_settings[:authentication]).to eq nil
+          end
+        end
       end
     end
   end