DEV: Add bookmark_id to bookmark reminder_handler notifications (#17547)

This is so we can join the Notification table onto the
Bookmark table. A slight refactor was needed to ensure
that the required values are always included and the
consumer does not need to think about this.

The discourse-chat and discourse-data-explorer plugins
will be updated to take advantage of this commit.

app/services/base_bookmarkable.rb

@@ -98,6 +98,33 @@ class BaseBookmarkable
     raise NotImplementedError
   end
 
+  ##
+  # Can be used by the inheriting class via reminder_handler, most of the
+  # time we just want to make a Notification for a bookmark reminder, this
+  # gives consumers a way to do it without having provide all of the required
+  # data themselves.
+  #
+  # @param [Bookmark] bookmark          The bookmark that we are sending the reminder notification for.
+  # @param [Hash]     notification_data Any data, either top-level (e.g. topic_id, post_number) or inside
+  #                                     the data sub-key, which should be stored when the notification is
+  #                                     created.
+  # @return [void]
+  def self.send_reminder_notification(bookmark, notification_data)
+    if notification_data[:data].blank? ||
+        notification_data[:data][:bookmarkable_url].blank? ||
+        notification_data[:data][:title].blank?
+      raise Discourse::InvalidParameters.new("A `data` key must be present with at least `bookmarkable_url` and `title` entries.")
+    end
+
+    notification_data[:data] = notification_data[:data].merge(
+      display_username: bookmark.user.username,
+      bookmark_name: bookmark.name,
+      bookmark_id: bookmark.id
+    ).to_json
+    notification_data[:notification_type] = Notification.types[:bookmark_reminder]
+    bookmark.user.notifications.create!(notification_data)
+  end
+
   ##
   # Access control is dependent on what has been bookmarked, the appropriate guardian
   # can_see_X? method should be called from the bookmarkable class to determine

app/services/post_bookmarkable.rb

@@ -39,16 +39,14 @@ class PostBookmarkable < BaseBookmarkable
   end
 
   def self.reminder_handler(bookmark)
-    bookmark.user.notifications.create!(
-      notification_type: Notification.types[:bookmark_reminder],
+    send_reminder_notification(
+      bookmark,
       topic_id: bookmark.bookmarkable.topic_id,
       post_number: bookmark.bookmarkable.post_number,
       data: {
         title: bookmark.bookmarkable.topic.title,
-        display_username: bookmark.user.username,
-        bookmark_name: bookmark.name,
         bookmarkable_url: bookmark.bookmarkable.url
-      }.to_json
+      }
     )
   end
 

app/services/topic_bookmarkable.rb

@@ -36,16 +36,14 @@ class TopicBookmarkable < BaseBookmarkable
   end
 
   def self.reminder_handler(bookmark)
-    bookmark.user.notifications.create!(
-      notification_type: Notification.types[:bookmark_reminder],
+    send_reminder_notification(
+      bookmark,
       topic_id: bookmark.bookmarkable_id,
       post_number: 1,
       data: {
         title: bookmark.bookmarkable.title,
-        display_username: bookmark.user.username,
-        bookmark_name: bookmark.name,
         bookmarkable_url: bookmark.bookmarkable.first_post.url
-      }.to_json
+      }
     )
   end
 

spec/services/base_bookmarkable_spec.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+describe BaseBookmarkable do
+  fab!(:bookmark) { Fabricate(:bookmark, bookmarkable: Fabricate(:post)) }
+
+  describe "#send_reminder_notification" do
+    it "raises an error if the data, data.bookmarkable_url, or data.title values are missing from notification_data" do
+      expect { BaseBookmarkable.send_reminder_notification(bookmark, {}) }.to raise_error(Discourse::InvalidParameters)
+      expect { BaseBookmarkable.send_reminder_notification(bookmark, { data: {} }) }.to raise_error(Discourse::InvalidParameters)
+      expect { BaseBookmarkable.send_reminder_notification(bookmark, { data: { title: "test", bookmarkable_url: "test" } }) }.not_to raise_error
+    end
+
+    it "creates a Notification with the required data from the bookmark" do
+      BaseBookmarkable.send_reminder_notification(
+        bookmark,
+        {
+          topic_id: bookmark.bookmarkable.topic_id,
+          post_number: bookmark.bookmarkable.post_number,
+          data: {
+            title: bookmark.bookmarkable.topic.title,
+            bookmarkable_url: bookmark.bookmarkable.url
+          }
+        }
+      )
+      notif = bookmark.user.notifications.last
+      expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
+      expect(notif.topic_id).to eq(bookmark.bookmarkable.topic_id)
+      expect(notif.post_number).to eq(bookmark.bookmarkable.post_number)
+      expect(notif.data).to eq(
+        {
+          title: bookmark.bookmarkable.topic.title,
+          bookmarkable_url: bookmark.bookmarkable.url,
+          display_username: bookmark.user.username,
+          bookmark_name: bookmark.name,
+          bookmark_id: bookmark.id
+        }.to_json
+      )
+    end
+
+    it "does not allow the consumer to override display_username, bookmark_name, or bookmark_id" do
+      BaseBookmarkable.send_reminder_notification(
+        bookmark,
+        {
+          topic_id: bookmark.bookmarkable.topic_id,
+          post_number: bookmark.bookmarkable.post_number,
+          data: {
+            title: bookmark.bookmarkable.topic.title,
+            bookmarkable_url: bookmark.bookmarkable.url,
+            display_username: "bad username",
+            bookmark_name: "bad name",
+            bookmark_id: -89854
+          }
+        }
+      )
+
+      notif = bookmark.user.notifications.last
+      data = JSON.parse(notif[:data])
+      expect(data[:display_username]).not_to eq("bad username")
+      expect(data[:name]).not_to eq("bad name")
+      expect(data[:bookmark_id]).not_to eq(-89854)
+    end
+  end
+end

spec/services/post_bookmarkable_spec.rb

@@ -82,9 +82,10 @@ describe PostBookmarkable do
       expect(notif.data).to eq(
         {
           title: bookmark1.bookmarkable.topic.title,
+          bookmarkable_url: bookmark1.bookmarkable.url,
           display_username: bookmark1.user.username,
           bookmark_name: bookmark1.name,
-          bookmarkable_url: bookmark1.bookmarkable.url
+          bookmark_id: bookmark1.id
         }.to_json
       )
     end

spec/services/topic_bookmarkable_spec.rb

@@ -78,9 +78,10 @@ describe TopicBookmarkable do
       expect(notif.data).to eq(
         {
           title: bookmark1.bookmarkable.title,
+          bookmarkable_url: bookmark1.bookmarkable.first_post.url,
           display_username: bookmark1.user.username,
           bookmark_name: bookmark1.name,
-          bookmarkable_url: bookmark1.bookmarkable.first_post.url
+          bookmark_id: bookmark1.id
         }.to_json
       )
     end