UX: When a policy reminder notification is sent mark it as high priority (#12)

UX: When a policy reminder notification is sent mark it as high priority (#12)

This is so policy reminders do not fall by the wayside. They are given the same priority as bookmark reminder and PM notifications, so they show with the green notification bubble.

Any existing reminder notification will be deleted as well, we don’t want the user to end up with tons of duplicate notifications if the policy reminder is aggressive.

diff --git a/jobs/scheduled/check_policy.rb b/jobs/scheduled/check_policy.rb
index 77a274a..18cfff0 100644
--- a/jobs/scheduled/check_policy.rb
+++ b/jobs/scheduled/check_policy.rb
@@ -29,11 +29,13 @@ module Jobs
           post.post_policy.update(last_reminded_at: Time.zone.now)
 
           missing_users(post).each do |user|
+            clear_existing_notification(user, post)
             user.notifications.create!(
               notification_type: Notification.types[:topic_reminder],
               topic_id: post.topic_id,
               post_number: post.post_number,
-              data: { topic_title: post.topic.title, display_username: user.username }.to_json
+              data: { topic_title: post.topic.title, display_username: user.username }.to_json,
+              high_priority: true
             )
           end
         end
@@ -88,5 +90,16 @@ module Jobs
     def missing_users(post)
       post.post_policy.not_accepted_by
     end
+
+    def clear_existing_notification(user, post)
+      existing_notification = Notification.find_by(
+        notification_type: Notification.types[:topic_reminder],
+        topic_id: post.topic_id,
+        post_number: post.post_number,
+        user: user
+      )
+      return if existing_notification.blank?
+      existing_notification.delete
+    end
   end
 end
diff --git a/spec/lib/check_policy_spec.rb b/spec/lib/check_policy_spec.rb
index b0571d1..9c949af 100644
--- a/spec/lib/check_policy_spec.rb
+++ b/spec/lib/check_policy_spec.rb
@@ -228,7 +228,7 @@ describe DiscoursePolicy::CheckPolicy do
     end
   end
 
-  it "will correctly notify users" do
+  it "will correctly notify users with high priority notifications" do
     SiteSetting.queue_jobs = false
     freeze_time
 
@@ -250,7 +250,42 @@ describe DiscoursePolicy::CheckPolicy do
     DiscoursePolicy::CheckPolicy.new.execute
     DiscoursePolicy::CheckPolicy.new.execute
 
+    user1_notifications = user1.notifications.where(notification_type: Notification.types[:topic_reminder], topic_id: post.topic_id, post_number: 1)
+    expect(user1_notifications.count).to eq(1)
+    expect(user1_notifications.first.high_priority).to eq(true)
+    user2_notifications = user2.notifications.where(notification_type: Notification.types[:topic_reminder], topic_id: post.topic_id, post_number: 1)
+    expect(user2_notifications.count).to eq(1)
+    expect(user2_notifications.first.high_priority).to eq(true)
+  end
+
+  it "will delete the existing policy reminder notification before creating a new one" do
+    SiteSetting.queue_jobs = false
+    freeze_time
+
+    raw = <<~MD
+     [policy group=#{group.name} reminder=weekly]
+     I always open **doors**!
+     [/policy]
+    MD
+
+    post = create_post(raw: raw, user: Fabricate(:admin))
+
+    DiscoursePolicy::CheckPolicy.new.execute
+
+    expect(user1.notifications.where(notification_type: Notification.types[:topic_reminder]).count).to eq(0)
+
+    freeze_time 2.weeks.from_now
+
+    DiscoursePolicy::CheckPolicy.new.execute
+
+    user1_notification = user1.notifications.where(notification_type: Notification.types[:topic_reminder], topic_id: post.topic_id, post_number: 1).last
+    expect(user1_notification).not_to eq(nil)
+
+    freeze_time 2.weeks.from_now
+
+    DiscoursePolicy::CheckPolicy.new.execute
+
     expect(user1.notifications.where(notification_type: Notification.types[:topic_reminder], topic_id: post.topic_id, post_number: 1).count).to eq(1)
-    expect(user2.notifications.where(notification_type: Notification.types[:topic_reminder], topic_id: post.topic_id, post_number: 1).count).to eq(1)
+    expect(Notification.find_by(id: user1_notification.id)).to eq(nil)
   end
 end

GitHub sha: d03dbd00

This commit appears in #12 which was merged by martin.