From abe81420382dc7fc6800da05a710866fb02bcae0 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Mon, 27 May 2019 10:53:37 -0300 Subject: [PATCH] FEATURE: Users can override reminders frequency (#30) * FEATURE: Users can override reminders frequency * Changes: - Avoid creating a user custom field when the used didn't override the frequency - Sanitize frequency value using coercion - Minor fixes * Sanitize query and user query single --- .../remind_assigns_frequency_site_settings.rb | 5 ++ .../remind-assigns-frequency.hbs | 1 + .../initializers/extend-for-assigns.js.es6 | 9 ++++ .../remind-assigns-frequency.js.es6 | 34 +++++++++++++ .../components/remind-assigns-frequency.hbs | 10 ++++ config/locales/client.en.yml | 2 + config/locales/server.en.yml | 1 + ...5428_add_reminds_assign_frequency_index.rb | 8 ++++ jobs/scheduled/enqueue_reminders.rb | 48 ++++++++++++------- lib/pending_assigns_reminder.rb | 13 +++-- plugin.rb | 11 +++++ spec/jobs/scheduled/enqueue_reminders_spec.rb | 34 +++++++++++-- spec/models/user_custom_field_spec.rb | 19 ++++++++ 13 files changed, 170 insertions(+), 25 deletions(-) create mode 100644 assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.hbs create mode 100644 assets/javascripts/discourse/components/remind-assigns-frequency.js.es6 create mode 100644 assets/javascripts/discourse/templates/components/remind-assigns-frequency.hbs create mode 100644 db/migrate/20190503145428_add_reminds_assign_frequency_index.rb create mode 100644 spec/models/user_custom_field_spec.rb diff --git a/app/models/remind_assigns_frequency_site_settings.rb b/app/models/remind_assigns_frequency_site_settings.rb index 0ad6e55..305d43a 100644 --- a/app/models/remind_assigns_frequency_site_settings.rb +++ b/app/models/remind_assigns_frequency_site_settings.rb @@ -10,6 +10,7 @@ class RemindAssignsFrequencySiteSettings < EnumSiteSetting end DAILY_MINUTES = 24 * 60 * 1 + WEEKLY_MINUTES = DAILY_MINUTES * 7 MONTHLY_MINUTES = DAILY_MINUTES * 30 QUARTERLY_MINUTES = DAILY_MINUTES * 90 @@ -22,6 +23,10 @@ class RemindAssignsFrequencySiteSettings < EnumSiteSetting name: 'discourse_assign.reminders_frequency.daily', value: DAILY_MINUTES }, + { + name: 'discourse_assign.reminders_frequency.weekly', + value: WEEKLY_MINUTES + }, { name: 'discourse_assign.reminders_frequency.monthly', value: MONTHLY_MINUTES diff --git a/assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.hbs b/assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.hbs new file mode 100644 index 0000000..195e9fd --- /dev/null +++ b/assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.hbs @@ -0,0 +1 @@ +{{remind-assigns-frequency user=model}} diff --git a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 index 7ad9b70..638031c 100644 --- a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 +++ b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 @@ -226,6 +226,15 @@ function initialize(api) { "notification.discourse_assign.assign_notification", "user-plus" ); + + api.modifyClass("controller:preferences/notifications", { + actions: { + save() { + this.get("saveAttrNames").push("custom_fields"); + this._super(...arguments); + } + } + }); } export default { diff --git a/assets/javascripts/discourse/components/remind-assigns-frequency.js.es6 b/assets/javascripts/discourse/components/remind-assigns-frequency.js.es6 new file mode 100644 index 0000000..29e9831 --- /dev/null +++ b/assets/javascripts/discourse/components/remind-assigns-frequency.js.es6 @@ -0,0 +1,34 @@ +import computed from "ember-addons/ember-computed-decorators"; + +export default Ember.Component.extend({ + selectedFrequency: null, + + @computed("user.reminders_frequency") + availableFrequencies() { + return this.get("user.reminders_frequency").map(freq => { + return { + name: I18n.t(freq.name), + value: freq.value, + selected: false + }; + }); + }, + + didInsertElement() { + let current_frequency = this.get( + "user.custom_fields.remind_assigns_frequency" + ); + + if (current_frequency === undefined) { + current_frequency = this.get("siteSettings.remind_assigns_frequency"); + } + + this.set("selectedFrequency", current_frequency); + }, + + actions: { + setFrequency(newFrequency) { + this.set("user.custom_fields.remind_assigns_frequency", newFrequency); + } + } +}); diff --git a/assets/javascripts/discourse/templates/components/remind-assigns-frequency.hbs b/assets/javascripts/discourse/templates/components/remind-assigns-frequency.hbs new file mode 100644 index 0000000..e7b3e7b --- /dev/null +++ b/assets/javascripts/discourse/templates/components/remind-assigns-frequency.hbs @@ -0,0 +1,10 @@ +{{#if siteSettings.assign_enabled}} +
+ + {{combo-box valueAttribute="value" + content=availableFrequencies + value=selectedFrequency + onSelect=(action "setFrequency") + }} +
+{{/if}} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 1d1cfaf..c09e9be 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -26,8 +26,10 @@ en: title: "claim" help: "Assign topic to yourself" reminders_frequency: + description: "Frequency for receiving assigned topics reminders" never: 'Never' daily: 'Daily' + weekly: 'Weekly' monthly: 'Monthly' quarterly: 'Quarterly' user: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index a33fa3f..184c4a1 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -27,6 +27,7 @@ en: reminders_frequency: never: 'never' daily: 'daily' + weekly: 'weekly' monthly: 'monthly' quarterly: 'quarterly' assign_mailer: diff --git a/db/migrate/20190503145428_add_reminds_assign_frequency_index.rb b/db/migrate/20190503145428_add_reminds_assign_frequency_index.rb new file mode 100644 index 0000000..1cdf2a5 --- /dev/null +++ b/db/migrate/20190503145428_add_reminds_assign_frequency_index.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddRemindsAssignFrequencyIndex < ActiveRecord::Migration[5.2] + def change + add_index :user_custom_fields, %i[name user_id], name: :idx_user_custom_fields_remind_assigns_frequency, + unique: true, where: "name = 'remind_assigns_frequency'" + end +end diff --git a/jobs/scheduled/enqueue_reminders.rb b/jobs/scheduled/enqueue_reminders.rb index 0f7cfa2..dcfee89 100644 --- a/jobs/scheduled/enqueue_reminders.rb +++ b/jobs/scheduled/enqueue_reminders.rb @@ -12,27 +12,41 @@ module Jobs private def skip_enqueue? - SiteSetting.remind_assigns_frequency.nil? || SiteSetting.remind_assigns_frequency.zero? + SiteSetting.remind_assigns_frequency.nil? || !SiteSetting.assign_enabled? end def user_ids - interval = SiteSetting.remind_assigns_frequency + global_frequency = SiteSetting.remind_assigns_frequency + frequency = ActiveRecord::Base.sanitize_sql("COALESCE(user_frequency.value, '#{global_frequency}')::INT") - TopicCustomField - .joins(<<~SQL - LEFT OUTER JOIN user_custom_fields ON topic_custom_fields.value::INT = user_custom_fields.user_id - AND user_custom_fields.name = '#{PendingAssignsReminder::REMINDED_AT}' - SQL - ).joins("INNER JOIN users ON topic_custom_fields.value::INT = users.id") - .where("users.moderator OR users.admin") - .where(<<~SQL - user_custom_fields.value IS NULL OR - user_custom_fields.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{interval}) - SQL - ).where("topic_custom_fields.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * ?)", interval) - .where(name: TopicAssigner::ASSIGNED_TO_ID) - .group('topic_custom_fields.value').having('COUNT(topic_custom_fields.value) > 1') - .pluck('topic_custom_fields.value') + DB.query_single(<<~SQL + SELECT topic_custom_fields.value + FROM topic_custom_fields + + LEFT OUTER JOIN user_custom_fields AS last_reminder + ON topic_custom_fields.value::INT = last_reminder.user_id + AND last_reminder.name = '#{PendingAssignsReminder::REMINDED_AT}' + + LEFT OUTER JOIN user_custom_fields AS user_frequency + ON topic_custom_fields.value::INT = user_frequency.user_id + AND user_frequency.name = '#{PendingAssignsReminder::REMINDERS_FREQUENCY}' + + INNER JOIN users ON topic_custom_fields.value::INT = users.id + INNER JOIN topics ON topics.id = topic_custom_fields.topic_id AND (topics.deleted_at IS NULL) + + WHERE (users.moderator OR users.admin) + AND #{frequency} > 0 + AND ( + last_reminder.value IS NULL OR + last_reminder.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) + ) + AND topic_custom_fields.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) + AND topic_custom_fields.name = '#{TopicAssigner::ASSIGNED_TO_ID}' + + GROUP BY topic_custom_fields.value + HAVING COUNT(topic_custom_fields.value) > 1 + SQL + ) end end end diff --git a/lib/pending_assigns_reminder.rb b/lib/pending_assigns_reminder.rb index 1ba4c56..cf2a7e3 100644 --- a/lib/pending_assigns_reminder.rb +++ b/lib/pending_assigns_reminder.rb @@ -2,6 +2,7 @@ class PendingAssignsReminder REMINDED_AT = 'last_reminded_at' + REMINDERS_FREQUENCY = 'remind_assigns_frequency' REMINDER_THRESHOLD = 2 def remind(user) @@ -50,7 +51,7 @@ class PendingAssignsReminder assignments_link: "#{Discourse.base_url}/u/#{user.username_lower}/activity/assigned", newest_assignments: newest_list, oldest_assignments: oldest_list, - frequency: frequency_in_words + frequency: frequency_in_words(user) ) end @@ -71,8 +72,14 @@ class PendingAssignsReminder ) end - def frequency_in_words - ::RemindAssignsFrequencySiteSettings.frequency_for(SiteSetting.remind_assigns_frequency) + def frequency_in_words(user) + frequency = if user.custom_fields&.has_key?(REMINDERS_FREQUENCY) + user.custom_fields[REMINDERS_FREQUENCY] + else + SiteSetting.remind_assigns_frequency + end + + ::RemindAssignsFrequencySiteSettings.frequency_for(frequency) end def update_last_reminded(user) diff --git a/plugin.rb b/plugin.rb index bd68200..3df6f66 100644 --- a/plugin.rb +++ b/plugin.rb @@ -29,6 +29,17 @@ after_initialize do require 'topic_assigner' require 'pending_assigns_reminder' + frequency_field = PendingAssignsReminder::REMINDERS_FREQUENCY + register_editable_user_custom_field frequency_field + User.register_custom_field_type frequency_field, :integer + DiscoursePluginRegistry.serialized_current_user_fields << frequency_field + add_to_serializer(:user, :reminders_frequency) do + RemindAssignsFrequencySiteSettings.values + end + add_model_callback(UserCustomField, :before_save) do + self.value = self.value.to_i if self.name == frequency_field + end + =begin TODO: Remove this once 2.3 becomes the new stable. Also remove: diff --git a/spec/jobs/scheduled/enqueue_reminders_spec.rb b/spec/jobs/scheduled/enqueue_reminders_spec.rb index 61765a6..6bae3d7 100644 --- a/spec/jobs/scheduled/enqueue_reminders_spec.rb +++ b/spec/jobs/scheduled/enqueue_reminders_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Jobs::EnqueueReminders do before do SiteSetting.remind_assigns_frequency = RemindAssignsFrequencySiteSettings::MONTHLY_MINUTES + SiteSetting.assign_enabled = true end describe '#execute' do @@ -49,7 +50,29 @@ RSpec.describe Jobs::EnqueueReminders do it 'does not enqueue reminders if the topic was just assigned to the user' do just_assigned = DateTime.now - assign_multiple_tasks_to(user, just_assigned) + assign_multiple_tasks_to(user, assigned_on: just_assigned) + + assert_reminders_enqueued(0) + end + + it 'enqueues a reminder when the user overrides the global frequency' do + SiteSetting.remind_assigns_frequency = 0 + user.custom_fields.merge!( + PendingAssignsReminder::REMINDERS_FREQUENCY => RemindAssignsFrequencySiteSettings::DAILY_MINUTES + ) + user.save_custom_fields + + assign_multiple_tasks_to(user) + + assert_reminders_enqueued(1) + end + + it "doesn't count assigns from deleted topics" do + deleted_post = Fabricate(:post) + assign_one_task_to(user, post: deleted_post) + (PendingAssignsReminder::REMINDER_THRESHOLD - 1).times { assign_one_task_to(user) } + + deleted_post.topic.trash! assert_reminders_enqueued(0) end @@ -58,15 +81,16 @@ RSpec.describe Jobs::EnqueueReminders do expect { subject.execute({}) }.to change(Jobs::RemindUser.jobs, :size).by(expected_amount) end - def assign_one_task_to(user, assigned_on = 3.months.ago) + def assign_one_task_to(user, assigned_on: 3.months.ago, post: Fabricate(:post)) freeze_time(assigned_on) do - post = Fabricate(:post) TopicAssigner.new(post.topic, user).assign(user) end end - def assign_multiple_tasks_to(user, assigned_on = 3.months.ago) - PendingAssignsReminder::REMINDER_THRESHOLD.times { assign_one_task_to(user, assigned_on) } + def assign_multiple_tasks_to(user, assigned_on: 3.months.ago) + PendingAssignsReminder::REMINDER_THRESHOLD.times do + assign_one_task_to(user, assigned_on: assigned_on) + end end end end diff --git a/spec/models/user_custom_field_spec.rb b/spec/models/user_custom_field_spec.rb new file mode 100644 index 0000000..c43ed34 --- /dev/null +++ b/spec/models/user_custom_field_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe UserCustomField do + before { SiteSetting.assign_enabled = true } + + let(:field_name) { PendingAssignsReminder::REMINDERS_FREQUENCY } + let(:new_field) { UserCustomField.new(name: field_name, user_id: 1) } + + it 'coerces the value to be an integer' do + new_field.value = 'DROP TABLE USERS;' + + new_field.save! + saved_field = new_field.reload + + expect(saved_field.value).to eq('0') + end +end