From 4a5869b60cfdc9b31c8049da7a7441d3c75ac3dc Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 25 Jan 2021 13:24:18 +0100 Subject: [PATCH 1/2] Remove ability to toggle mail delivery OFN requires mails to work so there's no point in having this conditional with the maintenance cost it entails. --- app/models/spree/app_configuration.rb | 1 - .../spree/admin/mail_methods/_form.html.haml | 3 - db/seeds.rb | 1 - lib/spree/core/mail_settings.rb | 10 +-- .../admin/mail_methods_controller_spec.rb | 3 +- spec/lib/spree/core/mail_settings_spec.rb | 67 ++++++++----------- 6 files changed, 31 insertions(+), 54 deletions(-) diff --git a/app/models/spree/app_configuration.rb b/app/models/spree/app_configuration.rb index 1edba583de..67b843a4ce 100644 --- a/app/models/spree/app_configuration.rb +++ b/app/models/spree/app_configuration.rb @@ -92,7 +92,6 @@ module Spree preference :s3_host_alias, :string # Default mail headers settings - preference :enable_mail_delivery, :boolean, default: false preference :mails_from, :string, default: 'ofn@example.com' preference :mail_bcc, :string, default: 'ofn@example.com' preference :intercept_email, :string, default: nil diff --git a/app/views/spree/admin/mail_methods/_form.html.haml b/app/views/spree/admin/mail_methods/_form.html.haml index 02b7ea11dd..f987a5b28b 100644 --- a/app/views/spree/admin/mail_methods/_form.html.haml +++ b/app/views/spree/admin/mail_methods/_form.html.haml @@ -2,9 +2,6 @@ .row %fieldset.no-border-bottom %legend{align: "center"}= t("spree.general") - .field - = preference_field_tag("enable_mail_delivery", Spree::Config[:enable_mail_delivery], type: :boolean) - = label_tag :enable_mail_delivery, t("spree.enable_mail_delivery") .field = label_tag :mails_from, t("spree.send_mails_as") %br/ diff --git a/db/seeds.rb b/db/seeds.rb index 23d907c4bf..95629e77ef 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -4,7 +4,6 @@ require 'yaml' def set_mail_configuration MailConfiguration.entries= { - enable_mail_delivery: true, mail_host: ENV.fetch('MAIL_HOST'), mail_domain: ENV.fetch('MAIL_DOMAIN'), mail_port: ENV.fetch('MAIL_PORT'), diff --git a/lib/spree/core/mail_settings.rb b/lib/spree/core/mail_settings.rb index 5fd94c8fa3..dde88bbb22 100644 --- a/lib/spree/core/mail_settings.rb +++ b/lib/spree/core/mail_settings.rb @@ -12,13 +12,9 @@ module Spree end def override! - if Config.enable_mail_delivery - ActionMailer::Base.default_url_options[:host] ||= Config.site_url - ActionMailer::Base.smtp_settings = mail_server_settings - ActionMailer::Base.perform_deliveries = true - else - ActionMailer::Base.perform_deliveries = false - end + ActionMailer::Base.default_url_options[:host] ||= Config.site_url + ActionMailer::Base.smtp_settings = mail_server_settings + ActionMailer::Base.perform_deliveries = true end private diff --git a/spec/controllers/spree/admin/mail_methods_controller_spec.rb b/spec/controllers/spree/admin/mail_methods_controller_spec.rb index 793242d0fc..f91617e671 100644 --- a/spec/controllers/spree/admin/mail_methods_controller_spec.rb +++ b/spec/controllers/spree/admin/mail_methods_controller_spec.rb @@ -10,7 +10,7 @@ describe Spree::Admin::MailMethodsController do context "#update" do it "should reinitialize the mail settings" do expect(Spree::Core::MailSettings).to receive(:init) - spree_put :update, enable_mail_delivery: "1", mails_from: "ofn@example.com" + spree_put :update, mails_from: "ofn@example.com" end end @@ -24,7 +24,6 @@ describe Spree::Admin::MailMethodsController do has_spree_role?: true, locale: nil) allow(controller).to receive_messages(spree_current_user: user) - Spree::Config[:enable_mail_delivery] = "1" ActionMailer::Base.perform_deliveries = true expect { diff --git a/spec/lib/spree/core/mail_settings_spec.rb b/spec/lib/spree/core/mail_settings_spec.rb index ed799f980f..4039addaf2 100644 --- a/spec/lib/spree/core/mail_settings_spec.rb +++ b/spec/lib/spree/core/mail_settings_spec.rb @@ -7,60 +7,47 @@ module Spree describe MailSettings do let!(:subject) { MailSettings.new } - context "enable delivery" do - before { Config.enable_mail_delivery = true } - - 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 } - - 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 + context "overrides appplication defaults" do + context "authentication method is none" do before do - Config.mail_auth_type = "login" + 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 - 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!" } + 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 } + + 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 "do not enable delivery" do + context "when mail_auth_type is other than none" do before do - Config.enable_mail_delivery = false + Config.mail_auth_type = "login" + Config.smtp_username = "schof" + Config.smtp_password = "hellospree!" subject.override! end - it { expect(ActionMailer::Base.perform_deliveries).to be_falsy } + 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 end end end From 5677c86f9bdc4713da5b72fda70327a2aa8df050 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 25 Jan 2021 13:33:11 +0100 Subject: [PATCH 2/2] Remove enable_mail_delivery preference from DB It's no longer used. --- ...210125123000_remove_enable_mail_delivery_preference.rb | 8 ++++++++ db/schema.rb | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20210125123000_remove_enable_mail_delivery_preference.rb diff --git a/db/migrate/20210125123000_remove_enable_mail_delivery_preference.rb b/db/migrate/20210125123000_remove_enable_mail_delivery_preference.rb new file mode 100644 index 0000000000..a08642e931 --- /dev/null +++ b/db/migrate/20210125123000_remove_enable_mail_delivery_preference.rb @@ -0,0 +1,8 @@ +class RemoveEnableMailDeliveryPreference < ActiveRecord::Migration + def up + Spree::Preference.delete_all("key ilike '%enable_mail_delivery%'") + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index fc360b11e1..7e26c00970 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20210115143738) do +ActiveRecord::Schema.define(version: 20210125123000) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql"