From f2074f256e52bdea19b7ad3729094cafd3ff5e17 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Tue, 26 Nov 2019 11:41:52 +0200 Subject: [PATCH] FEATURE: Improve assign mailer site setting (#57) --- app/mailers/assign_mailer.rb | 6 +++ app/models/assign_mailer_site_settings.rb | 22 +++++++++++ config/locales/client.en.yml | 4 ++ config/locales/server.en.yml | 2 +- config/settings.yml | 4 +- ...4425_rename_site_setting_assign_emailer.rb | 17 +++++++++ lib/topic_assigner.rb | 2 +- spec/lib/topic_assigner_spec.rb | 37 ++++++++++++++++++- 8 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 app/models/assign_mailer_site_settings.rb create mode 100644 db/migrate/20191119174425_rename_site_setting_assign_emailer.rb diff --git a/app/mailers/assign_mailer.rb b/app/mailers/assign_mailer.rb index 0028ffd..09b5dc1 100644 --- a/app/mailers/assign_mailer.rb +++ b/app/mailers/assign_mailer.rb @@ -5,6 +5,12 @@ require_dependency 'email/message_builder' class AssignMailer < ActionMailer::Base include Email::BuildEmailHelper + def self.levels + @levels ||= Enum.new(never: 'never', + different_users: 'different_users', + always: 'always') + end + def send_assignment(to_address, topic, assigned_by) opts = { template: 'assign_mailer', diff --git a/app/models/assign_mailer_site_settings.rb b/app/models/assign_mailer_site_settings.rb new file mode 100644 index 0000000..83238a8 --- /dev/null +++ b/app/models/assign_mailer_site_settings.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require_dependency 'enum_site_setting' + +class AssignMailerSiteSettings < EnumSiteSetting + + def self.valid_value?(val) + values.any? { |v| v[:value].to_s == val.to_s } + end + + def self.values + @values ||= [ + { name: 'discourse_assign.assign_mailer.never', value: AssignMailer.levels[:never] }, + { name: 'discourse_assign.assign_mailer.different_users', value: AssignMailer.levels[:different_users] }, + { name: 'discourse_assign.assign_mailer.always', value: AssignMailer.levels[:always] } + ] + end + + def self.translate_names? + true + end +end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 2a50c7d..86139c1 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -30,6 +30,10 @@ en: claim: title: "claim" help: "Assign topic to yourself" + assign_mailer: + never: 'Never' + different_users: 'Only if assigner and assignee are different users' + always: 'Always' reminders_frequency: description: "Frequency for receiving assigned topics reminders" never: "Never" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 03b2246..977c98f 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -9,7 +9,7 @@ en: assign_other_regex: "Regex that needs to pass for assigning topics to others via mention. Example 'your list'." unassign_on_group_archive: "When a message is archived by a group, unassign message (reassign if moved back to inbox)" unassign_on_close: "When a topic is closed unassign topic" - assign_mailer_enabled: "When enabled, the assigned user will receive a notification email on each assignment" + assign_mailer: "When to send notification email for assignments" remind_assigns: "Remind users about pending assigns." remind_assigns_frequency: "Frequency for reminding users about assigned topics." max_assigned_topics: "Maximum number of topics that can be assigned to a user." diff --git a/config/settings.yml b/config/settings.yml index 173b7dc..68ae55d 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -12,7 +12,9 @@ plugins: assigns_user_url_path: client: true default: "/u/{username}/activity/assigned" - assign_mailer_enabled: false + assign_mailer: + default: "never" + enum: "AssignMailerSiteSettings" remind_assigns_frequency: client: true enum: "RemindAssignsFrequencySiteSettings" diff --git a/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb b/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb new file mode 100644 index 0000000..b26a4cf --- /dev/null +++ b/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class RenameSiteSettingAssignEmailer < ActiveRecord::Migration[6.0] + def up + execute "UPDATE site_settings + SET name = 'assign_mailer', value = '#{AssignMailer.levels[:always]}', data_type = #{SiteSettings::TypeSupervisor.types[:enum]} + WHERE name = 'assign_mailer_enabled' AND value = 't' AND data_type = #{SiteSettings::TypeSupervisor.types[:enum]}" + + execute "UPDATE site_settings + SET name = 'assign_mailer', value = '#{AssignMailer.levels[:never]}', data_type = #{SiteSettings::TypeSupervisor.types[:enum]} + WHERE name = 'assign_mailer_enabled' AND value = 'f' AND data_type = #{SiteSettings::TypeSupervisor.types[:enum]}" + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index 5bffc4c..5d2587c 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -189,7 +189,7 @@ class ::TopicAssigner ) end - if SiteSetting.assign_mailer_enabled + if SiteSetting.assign_mailer == AssignMailer.levels[:always] || (SiteSetting.assign_mailer == AssignMailer.levels[:different_users] && @assigned_by.id != assign_to.id) if !@topic.muted?(assign_to) message = AssignMailer.send_assignment(assign_to.email, @topic, @assigned_by) Email::Sender.new(message, :assign_message).send diff --git a/spec/lib/topic_assigner_spec.rb b/spec/lib/topic_assigner_spec.rb index e9c41f9..4d9d2e9 100644 --- a/spec/lib/topic_assigner_spec.rb +++ b/spec/lib/topic_assigner_spec.rb @@ -116,7 +116,7 @@ RSpec.describe TopicAssigner do end it "doesn't assign the same user more than once" do - SiteSetting.assign_mailer_enabled = true + SiteSetting.assign_mailer = AssignMailer.levels[:always] another_mod = Fabricate(:moderator, groups: [assign_allowed_group]) Email::Sender.any_instance.expects(:send).once @@ -284,4 +284,39 @@ RSpec.describe TopicAssigner do expect(TopicQuery.new(moderator, assigned: moderator.username).list_latest.topics).to eq([topic]) end end + + context "assign_emailer" do + let(:post) { Fabricate(:post) } + let(:topic) { post.topic } + let(:moderator) { Fabricate(:moderator, groups: [assign_allowed_group]) } + let(:moderator2) { Fabricate(:moderator, groups: [assign_allowed_group]) } + + it "send an email if set to 'always'" do + SiteSetting.assign_mailer = AssignMailer.levels[:always] + + expect { TopicAssigner.new(topic, moderator).assign(moderator) } + .to change { ActionMailer::Base.deliveries.size }.by(1) + end + + it "doesn't send an email if the assigner and assignee are not different" do + SiteSetting.assign_mailer = AssignMailer.levels[:different_users] + + expect { TopicAssigner.new(topic, moderator).assign(moderator2) } + .to change { ActionMailer::Base.deliveries.size }.by(1) + end + + it "doesn't send an email if the assigner and assignee are not different" do + SiteSetting.assign_mailer = AssignMailer.levels[:different_users] + + expect { TopicAssigner.new(topic, moderator).assign(moderator) } + .to change { ActionMailer::Base.deliveries.size }.by(0) + end + + it "doesn't send an email" do + SiteSetting.assign_mailer = AssignMailer.levels[:never] + + expect { TopicAssigner.new(topic, moderator).assign(moderator2) } + .to change { ActionMailer::Base.deliveries.size }.by(0) + end + end end