From 58eeb212f5e0d297afeae6f358ef06866231f3f8 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Tue, 19 Nov 2019 19:47:40 +0200 Subject: [PATCH 1/2] FEATURE: Improve assign mailer site setting --- 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 | 13 +++++++ lib/topic_assigner.rb | 2 +- spec/lib/topic_assigner_spec.rb | 37 ++++++++++++++++++- 7 files changed, 80 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/models/assign_mailer_site_settings.rb b/app/models/assign_mailer_site_settings.rb new file mode 100644 index 00000000..80dbc95f --- /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: 'never' }, + { name: 'discourse_assign.assign_mailer.different_users', value: 'different_users' }, + { name: 'discourse_assign.assign_mailer.always', value: 'always' } + ] + end + + def self.translate_names? + true + end +end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 2a50c7d9..86139c13 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 03b2246d..977c98f2 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 173b7dce..68ae55d0 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 00000000..0b8e5bc7 --- /dev/null +++ b/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class RenameSiteSettingAssignEmailer < ActiveRecord::Migration[6.0] + def up + execute "UPDATE site_settings + SET name = 'assign_mailer', data_type = #{SiteSettings::TypeSupervisor.types[:enum]} + WHERE name = 'assign_mailer_enabled' 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 5bffc4c6..fbec4115 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -189,7 +189,7 @@ def assign(assign_to, silent: false) ) end - if SiteSetting.assign_mailer_enabled + if SiteSetting.assign_mailer == 'always' || (SiteSetting.assign_mailer == '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 e9c41f9a..a74dc50c 100644 --- a/spec/lib/topic_assigner_spec.rb +++ b/spec/lib/topic_assigner_spec.rb @@ -116,7 +116,7 @@ def assert_publish_topic_state(topic, user) end it "doesn't assign the same user more than once" do - SiteSetting.assign_mailer_enabled = true + SiteSetting.assign_mailer = 'always' another_mod = Fabricate(:moderator, groups: [assign_allowed_group]) Email::Sender.any_instance.expects(:send).once @@ -284,4 +284,39 @@ def assigned_to?(asignee) 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 = '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 = '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 = '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 = 'never' + + expect { TopicAssigner.new(topic, moderator).assign(moderator2) } + .to change { ActionMailer::Base.deliveries.size }.by(0) + end + end end From 901a4c24fe40eeeaf7bd13c8a152c8ceb8783446 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Fri, 22 Nov 2019 13:57:15 +0200 Subject: [PATCH 2/2] DEV: Use an enum for assign_mailer site setting --- app/mailers/assign_mailer.rb | 6 ++++++ app/models/assign_mailer_site_settings.rb | 6 +++--- ...0191119174425_rename_site_setting_assign_emailer.rb | 8 ++++++-- lib/topic_assigner.rb | 2 +- spec/lib/topic_assigner_spec.rb | 10 +++++----- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/app/mailers/assign_mailer.rb b/app/mailers/assign_mailer.rb index 0028ffd3..09b5dc19 100644 --- a/app/mailers/assign_mailer.rb +++ b/app/mailers/assign_mailer.rb @@ -5,6 +5,12 @@ 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 index 80dbc95f..83238a87 100644 --- a/app/models/assign_mailer_site_settings.rb +++ b/app/models/assign_mailer_site_settings.rb @@ -10,9 +10,9 @@ def self.valid_value?(val) def self.values @values ||= [ - { name: 'discourse_assign.assign_mailer.never', value: 'never' }, - { name: 'discourse_assign.assign_mailer.different_users', value: 'different_users' }, - { name: 'discourse_assign.assign_mailer.always', value: 'always' } + { 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 diff --git a/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb b/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb index 0b8e5bc7..b26a4cf1 100644 --- a/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb +++ b/db/migrate/20191119174425_rename_site_setting_assign_emailer.rb @@ -3,8 +3,12 @@ class RenameSiteSettingAssignEmailer < ActiveRecord::Migration[6.0] def up execute "UPDATE site_settings - SET name = 'assign_mailer', data_type = #{SiteSettings::TypeSupervisor.types[:enum]} - WHERE name = 'assign_mailer_enabled' AND data_type = #{SiteSettings::TypeSupervisor.types[:enum]}" + 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 diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index fbec4115..5d2587c3 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -189,7 +189,7 @@ def assign(assign_to, silent: false) ) end - if SiteSetting.assign_mailer == 'always' || (SiteSetting.assign_mailer == 'different_users' && @assigned_by.id != assign_to.id) + 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 a74dc50c..4d9d2e90 100644 --- a/spec/lib/topic_assigner_spec.rb +++ b/spec/lib/topic_assigner_spec.rb @@ -116,7 +116,7 @@ def assert_publish_topic_state(topic, user) end it "doesn't assign the same user more than once" do - SiteSetting.assign_mailer = 'always' + SiteSetting.assign_mailer = AssignMailer.levels[:always] another_mod = Fabricate(:moderator, groups: [assign_allowed_group]) Email::Sender.any_instance.expects(:send).once @@ -292,28 +292,28 @@ def assigned_to?(asignee) let(:moderator2) { Fabricate(:moderator, groups: [assign_allowed_group]) } it "send an email if set to 'always'" do - SiteSetting.assign_mailer = 'always' + 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 = 'different_users' + 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 = 'different_users' + 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 = 'never' + SiteSetting.assign_mailer = AssignMailer.levels[:never] expect { TopicAssigner.new(topic, moderator).assign(moderator2) } .to change { ActionMailer::Base.deliveries.size }.by(0)