FIX: Polymorphic bookmarks for bookmark report (#16693)

This allows the category_id filter for the bookmark
report to work with polymorphic bookmarks. Honestly this
is a little hardcode-y at the moment but until we go and
make this report a lot more flexible with more filters
I don't think it's worth the work to add extra interfaces
to RegisteredBookmarkable and BaseBookmarkable to make
this more flexible. This is enough for now.

app/models/bookmark.rb

@@ -214,8 +214,6 @@ class Bookmark < ActiveRecord::Base
     end
   end
 
-  # TODO (martin) [POLYBOOK] Make a separate PR for reports
-  # functionality as the bookmarkables will have to define this.
   def self.count_per_day(opts = nil)
     opts ||= {}
     result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago)
@@ -225,7 +223,15 @@ class Bookmark < ActiveRecord::Base
     end
 
     if opts[:category_id]
-      result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id]))
+      if SiteSetting.use_polymorphic_bookmarks
+        result = result
+          .joins("LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
+          .joins("LEFT JOIN topics ON (topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic') OR (topics.id = posts.topic_id)")
+          .where("topics.deleted_at IS NULL AND posts.deleted_at IS NULL")
+          .merge(Topic.in_category_and_subcategories(opts[:category_id]))
+      else
+        result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id]))
+      end
     end
 
     result.group('date(bookmarks.created_at)')

spec/models/bookmark_spec.rb

@@ -8,7 +8,8 @@ describe Bookmark do
       bookmark = Fabricate(:bookmark, post: post)
       user = bookmark.user
 
-      bookmark_2 = Fabricate.build(:bookmark,
+      bookmark_2 = Fabricate.build(
+        :bookmark,
         post: post,
         user: user
       )
@@ -21,7 +22,8 @@ describe Bookmark do
       bookmark = Fabricate(:bookmark, post: post, for_topic: false)
       user = bookmark.user
 
-      bookmark_2 = Fabricate(:bookmark,
+      bookmark_2 = Fabricate(
+        :bookmark,
         post: post,
         user: user,
         for_topic: true
@@ -29,7 +31,8 @@ describe Bookmark do
 
       expect(bookmark_2.valid?).to eq(true)
 
-      bookmark_3 = Fabricate.build(:bookmark,
+      bookmark_3 = Fabricate.build(
+        :bookmark,
         post: post,
         user: user,
         for_topic: true
@@ -142,51 +145,107 @@ describe Bookmark do
     describe "#count_per_day" do
       let(:category) { Fabricate(:category) }
       let(:topic_in_category) { Fabricate(:topic, category: category) }
-      let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) }
-      let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) }
-      let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) }
-      let!(:bookmark4) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) }
-      let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) }
 
-      it "gets the count of bookmarks grouped by date within the last 30 days by default" do
+      context "for non-polymorphic bookmarks" do
-        expect(Bookmark.count_per_day).to eq({
+        let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) }
-          1.day.ago.to_date => 1,
+        let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) }
-          2.days.ago.to_date => 1,
+        let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) }
-          3.days.ago.to_date => 2
+        let!(:bookmark4) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) }
-        })
+        let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) }
+
+        it "gets the count of bookmarks grouped by date within the last 30 days by default" do
+          expect(Bookmark.count_per_day).to eq({
+            1.day.ago.to_date => 1,
+            2.days.ago.to_date => 1,
+            3.days.ago.to_date => 2
+          })
+        end
+
+        it "respects the start_date option" do
+          expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({
+            1.day.ago.to_date => 1,
+          })
+        end
+
+        it "respects the since_days_ago option" do
+          expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({
+            1.day.ago.to_date => 1,
+          })
+        end
+
+        it "respects the end_date option" do
+          expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({
+            2.days.ago.to_date => 1,
+            3.days.ago.to_date => 2,
+          })
+        end
+
+        it "respects the category_id option" do
+          expect(Bookmark.count_per_day(category_id: category.id)).to eq({
+            3.days.ago.to_date => 1,
+          })
+        end
+
+        it "does not include deleted posts or topics" do
+          bookmark4.post.trash!
+          expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+          bookmark4.post.recover!
+          bookmark4.topic.trash!
+          expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+        end
       end
 
-      it "respects the start_date option" do
+      context "for polymorphic bookmarks" do
-        expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({
+        before do
-          1.day.ago.to_date => 1,
+          SiteSetting.use_polymorphic_bookmarks = true
-        })
+        end
-      end
 
-      it "respects the since_days_ago option" do
+        let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) }
-        expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({
+        let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) }
-          1.day.ago.to_date => 1,
+        let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) }
-        })
+        let!(:bookmark4) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) }
-      end
+        let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) }
 
-      it "respects the end_date option" do
+        it "gets the count of bookmarks grouped by date within the last 30 days by default" do
-        expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({
+          expect(Bookmark.count_per_day).to eq({
-          2.days.ago.to_date => 1,
+            1.day.ago.to_date => 1,
-          3.days.ago.to_date => 2,
+            2.days.ago.to_date => 1,
-        })
+            3.days.ago.to_date => 2
-      end
+          })
+        end
 
-      it "respects the category_id option" do
+        it "respects the start_date option" do
-        expect(Bookmark.count_per_day(category_id: category.id)).to eq({
+          expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({
-          3.days.ago.to_date => 1,
+            1.day.ago.to_date => 1,
-        })
+          })
-      end
+        end
 
-      it "does not include deleted posts or topics" do
+        it "respects the since_days_ago option" do
-        bookmark4.post.trash!
+          expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({
-        expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+            1.day.ago.to_date => 1,
-        bookmark4.post.recover!
+          })
-        bookmark4.topic.trash!
+        end
-        expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+
+        it "respects the end_date option" do
+          expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({
+            2.days.ago.to_date => 1,
+            3.days.ago.to_date => 2,
+          })
+        end
+
+        it "respects the category_id option" do
+          expect(Bookmark.count_per_day(category_id: category.id)).to eq({
+            3.days.ago.to_date => 1,
+          })
+        end
+
+        it "does not include deleted posts or topics" do
+          bookmark4.bookmarkable.trash!
+          expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+          bookmark4.bookmarkable.recover!
+          bookmark4.bookmarkable.topic.trash!
+          expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+        end
       end
     end