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

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

assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.hbs

@@ -0,0 +1 @@
+{{remind-assigns-frequency user=model}}

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 {

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);
+    }
+  }
+});

assets/javascripts/discourse/templates/components/remind-assigns-frequency.hbs

@@ -0,0 +1,10 @@
+{{#if siteSettings.assign_enabled}}
+  <div class="controls controls-dropdown">
+    <label>{{i18n "discourse_assign.reminders_frequency.description"}}</label>
+    {{combo-box valueAttribute="value" 
+                content=availableFrequencies 
+                value=selectedFrequency
+                onSelect=(action "setFrequency")
+    }}
+  </div>
+{{/if}}

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:

config/locales/server.en.yml

@@ -27,6 +27,7 @@ en:
     reminders_frequency:
         never: 'never'
         daily: 'daily'
+        weekly: 'weekly'
         monthly: 'monthly'
         quarterly: 'quarterly'
   assign_mailer:

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

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
+      DB.query_single(<<~SQL
-        .joins(<<~SQL
+        SELECT topic_custom_fields.value
-          LEFT OUTER JOIN user_custom_fields ON topic_custom_fields.value::INT = user_custom_fields.user_id
+        FROM topic_custom_fields
-          AND user_custom_fields.name = '#{PendingAssignsReminder::REMINDED_AT}'
+
-        SQL
+        LEFT OUTER JOIN user_custom_fields AS last_reminder
-        ).joins("INNER JOIN users ON topic_custom_fields.value::INT = users.id")
+        ON topic_custom_fields.value::INT = last_reminder.user_id
-        .where("users.moderator OR users.admin")
+        AND last_reminder.name = '#{PendingAssignsReminder::REMINDED_AT}'
-        .where(<<~SQL
+
-          user_custom_fields.value IS NULL OR
+        LEFT OUTER JOIN user_custom_fields AS user_frequency
-          user_custom_fields.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{interval})
+        ON topic_custom_fields.value::INT = user_frequency.user_id
-        SQL
+        AND user_frequency.name = '#{PendingAssignsReminder::REMINDERS_FREQUENCY}'
-        ).where("topic_custom_fields.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * ?)", interval)
+
-        .where(name: TopicAssigner::ASSIGNED_TO_ID)
+        INNER JOIN users ON topic_custom_fields.value::INT = users.id
-        .group('topic_custom_fields.value').having('COUNT(topic_custom_fields.value) > 1')
+        INNER JOIN topics ON topics.id = topic_custom_fields.topic_id AND (topics.deleted_at IS NULL)
-        .pluck('topic_custom_fields.value')
+
+        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

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
+  def frequency_in_words(user)
-    ::RemindAssignsFrequencySiteSettings.frequency_for(SiteSetting.remind_assigns_frequency)
+    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)

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:

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)
+    def assign_multiple_tasks_to(user, assigned_on: 3.months.ago)
-      PendingAssignsReminder::REMINDER_THRESHOLD.times { assign_one_task_to(user, assigned_on) }
+      PendingAssignsReminder::REMINDER_THRESHOLD.times do
+        assign_one_task_to(user, assigned_on: assigned_on)
+      end
     end
   end
 end

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