FIX: Make sure bookmark serializer works with deleted topics + posts (#9195)

app/models/bookmark.rb

@@ -39,6 +39,10 @@ class Bookmark < ActiveRecord::Base
     end
   end
 
+  def no_reminder?
+    self.reminder_at.blank? && self.reminder_type.blank?
+  end
+
   scope :pending_reminders, ->(before_time = Time.now.utc) do
     where("reminder_at IS NOT NULL AND reminder_at <= :before_time", before_time: before_time)
   end

app/serializers/user_bookmark_serializer.rb

@@ -25,67 +25,75 @@ class UserBookmarkSerializer < ApplicationSerializer
              :slug,
              :username
 
+  def topic
+    @topic ||= object.topic || Topic.unscoped.find(object.topic_id)
+  end
+
+  def post
+    @post ||= object.post || Post.unscoped.find(object.post_id)
+  end
+
   def closed
-    object.topic.closed
+    topic.closed
   end
 
   def archived
-    object.topic.archived
+    topic.archived
   end
 
   def linked_post_number
-    object.post.post_number
+    post.post_number
   end
 
   def title
-    object.topic.title
+    topic.title
   end
 
   def deleted
-    object.topic.deleted_at.present? || object.post.deleted_at.present?
+    topic.deleted_at.present? || post.deleted_at.present?
   end
 
   def hidden
-    object.post.hidden
+    post.hidden
   end
 
   def category_id
-    object.topic.category_id
+    topic.category_id
   end
 
   def archetype
-    object.topic.archetype
+    topic.archetype
   end
 
   def archived
-    object.topic.archived
+    topic.archived
   end
 
   def closed
-    object.topic.closed
+    topic.closed
   end
 
   def highest_post_number
-    object.topic.highest_post_number
+    topic.highest_post_number
   end
 
   def bumped_at
-    object.topic.bumped_at
+    topic.bumped_at
   end
 
   def raw
-    object.post.raw
+    post.raw
   end
 
   def cooked
-    object.post.cooked
+    post.cooked
   end
 
   def slug
-    object.topic.slug
+    topic.slug
   end
 
   def username
-    object.post.user.username
+    post.user.username
   end
 end

lib/bookmark_query.rb

@@ -5,7 +5,7 @@
 # in the user/activity/bookmarks page.
 
 class BookmarkQuery
-  def initialize(user, params)
+  def initialize(user, params = {})
     @user = user
     @params = params
   end

spec/lib/bookmark_reminder_notification_handler_spec.rb

@@ -9,72 +9,100 @@ RSpec.describe BookmarkReminderNotificationHandler do
 
   before do
     SiteSetting.enable_bookmarks_with_reminders = true
-  end
-  fab!(:reminder) do
-    Fabricate(
-      :bookmark,
-      user: user,
-      reminder_type: Bookmark.reminder_types[:at_desktop],
-      reminder_at: nil,
-      reminder_set_at: Time.zone.now
-    )
-  end
-  let(:user_agent) { "Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36" }
-
-  before do
     Discourse.redis.flushall
   end
 
-  context "when there are pending bookmark at desktop reminders" do
-    before do
-      described_class.cache_pending_at_desktop_reminder(user)
+  describe "#send_notification" do
+    fab!(:bookmark) do
+      Fabricate(:bookmark_next_business_day_reminder, user: user)
     end
 
-    context "when the user agent is for mobile" do
-      let(:user_agent) { "Mozilla/5.0 (iPhone; CPU iPhone OS 9_1 like Mac OS X) AppleWebKit/601.1.46 (KHTML, like Gecko) Version/9.0 Mobile/13B143 Safari/601.1" }
-      it "does not attempt to send any reminders" do
+    it "creates a bookmark reminder notification with the correct details" do
+      subject.send_notification(bookmark)
+      notif = bookmark.user.notifications.last
+      expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
+      expect(notif.topic_id).to eq(bookmark.topic_id)
+      expect(notif.post_number).to eq(bookmark.post.post_number)
+      data = JSON.parse(notif.data)
+      expect(data["topic_title"]).to eq(bookmark.topic.title)
+      expect(data["display_username"]).to eq(bookmark.user.username)
+      expect(data["bookmark_name"]).to eq(bookmark.name)
+    end
+
+    context "when the post has been deleted" do
+      it "clears the reminder and does not send a notification" do
+        bookmark.post.trash!
+        bookmark.reload
+        subject.send_notification(bookmark)
+        expect(bookmark.reload.no_reminder?).to eq(true)
+      end
+    end
+  end
+
+  describe "#send_at_desktop_reminder" do
+    fab!(:reminder) do
+      Fabricate(
+        :bookmark,
+        user: user,
+        reminder_type: Bookmark.reminder_types[:at_desktop],
+        reminder_at: nil,
+        reminder_set_at: Time.zone.now
+      )
+    end
+    let(:user_agent) { "Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36" }
+
+    context "when there are pending bookmark at desktop reminders" do
+      before do
+        described_class.cache_pending_at_desktop_reminder(user)
+      end
+
+      context "when the user agent is for mobile" do
+        let(:user_agent) { "Mozilla/5.0 (iPhone; CPU iPhone OS 9_1 like Mac OS X) AppleWebKit/601.1.46 (KHTML, like Gecko) Version/9.0 Mobile/13B143 Safari/601.1" }
+        it "does not attempt to send any reminders" do
+          DistributedMutex.expects(:synchronize).never
+          send_reminder
+        end
+      end
+
+      context "when the user agent is for desktop" do
+        let(:user_agent) { "Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36" }
+
+        it "deletes the key in redis" do
+          send_reminder
+          expect(described_class.user_has_pending_at_desktop_reminders?(user)).to eq(false)
+        end
+
+        it "sends a notification to the user and clears the reminder_at" do
+          send_reminder
+          expect(Notification.where(user: user, notification_type: Notification.types[:bookmark_reminder]).count).to eq(1)
+          expect(reminder.reload.reminder_type).to eq(nil)
+          expect(reminder.reload.reminder_last_sent_at).not_to eq(nil)
+          expect(reminder.reload.reminder_set_at).to eq(nil)
+        end
+      end
+    end
+
+    context "when there are no pending bookmark at desktop reminders" do
+      it "does nothing" do
         DistributedMutex.expects(:synchronize).never
         send_reminder
       end
     end
 
-    context "when the user agent is for desktop" do
-      let(:user_agent) { "Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36" }
-
-      it "deletes the key in redis" do
-        send_reminder
-        expect(described_class.user_has_pending_at_desktop_reminders?(user)).to eq(false)
+    context "when enable bookmarks with reminders is disabled" do
+      before do
+        SiteSetting.enable_bookmarks_with_reminders = false
       end
 
-      it "sends a notification to the user and clears the reminder_at" do
+      it "does nothing" do
+        BrowserDetection.expects(:device).never
         send_reminder
-        expect(Notification.where(user: user, notification_type: Notification.types[:bookmark_reminder]).count).to eq(1)
-        expect(reminder.reload.reminder_type).to eq(nil)
-        expect(reminder.reload.reminder_last_sent_at).not_to eq(nil)
-        expect(reminder.reload.reminder_set_at).to eq(nil)
       end
     end
-  end
 
-  context "when there are no pending bookmark at desktop reminders" do
-    it "does nothing" do
-      DistributedMutex.expects(:synchronize).never
-      send_reminder
+    def send_reminder
+      subject.send_at_desktop_reminder(user: user, request_user_agent: user_agent)
     end
   end
 
-  context "when enable bookmarks with reminders is disabled" do
-    before do
-      SiteSetting.enable_bookmarks_with_reminders = false
-    end
-
-    it "does nothing" do
-      BrowserDetection.expects(:device).never
-      send_reminder
-    end
-  end
-
-  def send_reminder
-    subject.send_at_desktop_reminder(user: user, request_user_agent: user_agent)
-  end
 end

spec/serializers/user_bookmark_serializer_spec.rb

@@ -3,12 +3,31 @@
 require 'rails_helper'
 
 RSpec.describe UserBookmarkSerializer do
-  let(:bookmark) do
-    Fabricate(:bookmark)
-    Bookmark.all.includes(post: :user).includes(:topic).last
-  end
+  let(:user) { Fabricate(:user) }
+  let(:post) { Fabricate(:post, user: user) }
+  let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post, topic: post.topic) }
 
-  subject { described_class.new(bookmark) }
+  it "serializes all properties correctly" do
+    s = serialized
+    expect(s.id).to eq(bookmark.id)
+    expect(s.created_at).to eq(bookmark.created_at)
+    expect(s.topic_id).to eq(bookmark.topic_id)
+    expect(s.linked_post_number).to eq(bookmark.post.post_number)
+    expect(s.post_id).to eq(bookmark.post_id)
+    expect(s.name).to eq(bookmark.name)
+    expect(s.reminder_at).to eq(bookmark.reminder_at)
+    expect(s.title).to eq(bookmark.topic.title)
+    expect(s.deleted).to eq(false)
+    expect(s.hidden).to eq(false)
+    expect(s.closed).to eq(false)
+    expect(s.archived).to eq(false)
+    expect(s.category_id).to eq(bookmark.topic.category_id)
+    expect(s.archetype).to eq(bookmark.topic.archetype)
+    expect(s.highest_post_number).to eq(1)
+    expect(s.bumped_at.to_s).to eq(bookmark.topic.bumped_at.to_s)
+    expect(s.slug).to eq(bookmark.topic.slug)
+    expect(s.username).to eq(bookmark.post.user.username)
+  end
 
   context "when the topic is deleted" do
     before do
@@ -16,7 +35,8 @@ RSpec.describe UserBookmarkSerializer do
       bookmark.reload
     end
     it "still returns the topic title because the relationship is unscoped" do
-      expect(subject.title).not_to eq(nil)
+      serialized
+      expect(serialized.title).not_to eq(nil)
     end
   end
 
@@ -26,10 +46,16 @@ RSpec.describe UserBookmarkSerializer do
       bookmark.reload
     end
     it "still returns the post number because the relationship is unscoped" do
-      expect(subject.linked_post_number).not_to eq(nil)
+      serialized
+      expect(serialized.linked_post_number).not_to eq(nil)
     end
     it "still returns the post username" do
-      expect(subject.username).not_to eq(nil)
+      serialized
+      expect(serialized.username).not_to eq(nil)
     end
   end
+
+  def serialized
+    described_class.new(BookmarkQuery.new(bookmark.user, {}).list_all.to_ary.last)
+  end
 end