diff --git a/app/services/base_bookmarkable.rb b/app/services/base_bookmarkable.rb index 111e84bc7da..dacc02c07e4 100644 --- a/app/services/base_bookmarkable.rb +++ b/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 diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb index d72f98cce65..5690d30db67 100644 --- a/app/services/post_bookmarkable.rb +++ b/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 diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index a08cce71d22..45ce54adfd0 100644 --- a/app/services/topic_bookmarkable.rb +++ b/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 diff --git a/spec/services/base_bookmarkable_spec.rb b/spec/services/base_bookmarkable_spec.rb new file mode 100644 index 00000000000..3d3ed3749e1 --- /dev/null +++ b/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 diff --git a/spec/services/post_bookmarkable_spec.rb b/spec/services/post_bookmarkable_spec.rb index 9f0d0397515..bfb92e27ceb 100644 --- a/spec/services/post_bookmarkable_spec.rb +++ b/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 diff --git a/spec/services/topic_bookmarkable_spec.rb b/spec/services/topic_bookmarkable_spec.rb index 46368592f8c..65e47a2a0f6 100644 --- a/spec/services/topic_bookmarkable_spec.rb +++ b/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