From b5603f0d3947572d97333a11e94d8ef125285fcb Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Sun, 19 Jan 2020 18:18:17 -0600
Subject: [PATCH] forum: optimize unread forum topics on forum index.

Avoid doing one SQL query per topic when checking for new topics on the
forum index.

This also changes it so that forum topics aren't always marked as new
for anonymous users.
---
 app/controllers/forum_topics_controller.rb |  2 ++
 app/helpers/forum_topics_helper.rb         |  4 ++++
 app/models/forum_topic.rb                  | 17 ++++++--------
 app/models/user.rb                         |  2 ++
 app/views/admin/dashboards/show.html.erb   |  2 +-
 app/views/forum_topics/_listing.html.erb   |  2 +-
 app/views/forum_topics/index.html.erb      |  2 +-
 test/unit/forum_topic_test.rb              | 26 +++++++++++-----------
 8 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/app/controllers/forum_topics_controller.rb b/app/controllers/forum_topics_controller.rb
index 9aef4ea80..89536b629 100644
--- a/app/controllers/forum_topics_controller.rb
+++ b/app/controllers/forum_topics_controller.rb
@@ -24,6 +24,8 @@ class ForumTopicsController < ApplicationController
     params[:limit] ||= 40
 
     @forum_topics = ForumTopic.paginated_search(params)
+    @read_forum_topics = @forum_topics.read_by_user(CurrentUser.user)
+
     @forum_topics = @forum_topics.includes(:creator, :updater).load if request.format.html?
     @forum_topics = @forum_topics.includes(:creator, :original_post).load if request.format.atom?
 
diff --git a/app/helpers/forum_topics_helper.rb b/app/helpers/forum_topics_helper.rb
index 98b9ad8ed..f96eac9c9 100644
--- a/app/helpers/forum_topics_helper.rb
+++ b/app/helpers/forum_topics_helper.rb
@@ -6,4 +6,8 @@ module ForumTopicsHelper
   def available_min_user_levels
     ForumTopic::MIN_LEVELS.select { |name, level| level <= CurrentUser.level }.to_a
   end
+
+  def new_forum_topic?(topic, read_forum_topics)
+    !read_forum_topics.map(&:id).include?(topic.id)
+  end
 end
diff --git a/app/models/forum_topic.rb b/app/models/forum_topic.rb
index 8d35b984c..b34f28c2e 100644
--- a/app/models/forum_topic.rb
+++ b/app/models/forum_topic.rb
@@ -14,8 +14,10 @@ class ForumTopic < ApplicationRecord
   belongs_to :creator, class_name: "User"
   belongs_to_updater
   has_many :posts, -> {order("forum_posts.id asc")}, :class_name => "ForumPost", :foreign_key => "topic_id", :dependent => :destroy
+  has_many :forum_topic_visits
   has_many :moderation_reports, through: :posts
   has_one :original_post, -> {order("forum_posts.id asc")}, class_name: "ForumPost", foreign_key: "topic_id", inverse_of: :topic
+
   before_validation :initialize_is_deleted, :on => :create
   validates_presence_of :title
   validates_associated :original_post
@@ -55,6 +57,11 @@ class ForumTopic < ApplicationRecord
       where("min_level <= ?", CurrentUser.level)
     end
 
+    def read_by_user(user)
+      return none if user.last_forum_read_at.nil? || user.last_forum_read_at < '2000-01-01'
+      merge(user.visited_forum_topics.where("forum_topic_visits.last_read_at >= forum_topics.updated_at OR ? >= forum_topics.updated_at", user.last_forum_read_at))
+    end
+
     def sticky_first
       order(is_sticky: :desc, updated_at: :desc)
     end
@@ -89,16 +96,6 @@ class ForumTopic < ApplicationRecord
   end
 
   module VisitMethods
-    def read_by?(user = nil)
-      user ||= CurrentUser.user
-
-      if user.last_forum_read_at && updated_at <= user.last_forum_read_at
-        return true
-      end
-
-      ForumTopicVisit.where("user_id = ? and forum_topic_id = ? and last_read_at >= ?", user.id, id, updated_at).exists?
-    end
-
     def mark_as_read!(user = CurrentUser.user)
       return if user.is_anonymous?
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 7510ff332..f4db96924 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -94,6 +94,8 @@ class User < ApplicationRecord
   has_many :wiki_page_versions, foreign_key: :updater_id
   has_many :feedback, :class_name => "UserFeedback", :dependent => :destroy
   has_many :forum_post_votes, dependent: :destroy, foreign_key: :creator_id
+  has_many :forum_topic_visits, dependent: :destroy
+  has_many :visited_forum_topics, through: :forum_topic_visits, source: :forum_topic
   has_many :moderation_reports, as: :model
   has_many :pools, foreign_key: :creator_id
   has_many :posts, :foreign_key => "uploader_id"
diff --git a/app/views/admin/dashboards/show.html.erb b/app/views/admin/dashboards/show.html.erb
index e45999905..9adf1441c 100644
--- a/app/views/admin/dashboards/show.html.erb
+++ b/app/views/admin/dashboards/show.html.erb
@@ -19,7 +19,7 @@
 
     <div class="section">
       <h2>Forum Topics</h2>
-      <%= render "forum_topics/listing", :forum_topics => @dashboard.forum_topics %>
+      <%= render "forum_topics/listing", forum_topics: @dashboard.forum_topics, read_forum_topics: @dashboard.forum_topics.read_by_user(CurrentUser.user) %>
     </div>
   </div>
 </div>
diff --git a/app/views/forum_topics/_listing.html.erb b/app/views/forum_topics/_listing.html.erb
index 2a6075ee3..ce25f1b4c 100644
--- a/app/views/forum_topics/_listing.html.erb
+++ b/app/views/forum_topics/_listing.html.erb
@@ -4,7 +4,7 @@
       <span class="sticky">Sticky:</span>
     <% end %>
 
-    <% unless topic.read_by?(CurrentUser.user) %>
+    <% if CurrentUser.is_member? && new_forum_topic?(topic, read_forum_topics) %>
       <span class="new">NEW</span>
     <% end %>
 
diff --git a/app/views/forum_topics/index.html.erb b/app/views/forum_topics/index.html.erb
index f59fd4af6..a457b13e0 100644
--- a/app/views/forum_topics/index.html.erb
+++ b/app/views/forum_topics/index.html.erb
@@ -11,7 +11,7 @@
       <%= ForumTopic::CATEGORIES.map {|id, name| link_to_unless_current(name, forum_topics_path(:search => {:category_id => id}))}.join(", ").html_safe %>
     </p>
 
-    <%= render "listing", :forum_topics => @forum_topics %>
+    <%= render "listing", forum_topics: @forum_topics, read_forum_topics: @read_forum_topics %>
 
     <%= numbered_paginator(@forum_topics) %>
   </div>
diff --git a/test/unit/forum_topic_test.rb b/test/unit/forum_topic_test.rb
index 1dc61b35c..f4371659f 100644
--- a/test/unit/forum_topic_test.rb
+++ b/test/unit/forum_topic_test.rb
@@ -15,7 +15,7 @@ class ForumTopicTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
 
-    context "#read_by?" do
+    context "read_by_user" do
       context "with a populated @user.last_forum_read_at" do
         setup do
           @user.update_attribute(:last_forum_read_at, Time.now)
@@ -26,8 +26,8 @@ class ForumTopicTest < ActiveSupport::TestCase
             @topic.update_column(:updated_at, 1.day.from_now)
           end
 
-          should "return false" do
-            assert_equal(false, @topic.read_by?(@user))
+          should "return nothing" do
+            assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
           end
         end
 
@@ -41,8 +41,8 @@ class ForumTopicTest < ActiveSupport::TestCase
               FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 16.hours.from_now)
             end
 
-            should "return false" do
-              assert_equal(false, @topic.read_by?(@user))
+            should "return nothing" do
+              assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
             end
           end
 
@@ -51,8 +51,8 @@ class ForumTopicTest < ActiveSupport::TestCase
               FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 2.days.from_now)
             end
 
-            should "return true" do
-              assert_equal(true, @topic.read_by?(@user))
+            should "return the topic" do
+              assert_equal([@topic.id], ForumTopic.read_by_user(@user).map(&:id))
             end
           end
         end
@@ -60,8 +60,8 @@ class ForumTopicTest < ActiveSupport::TestCase
 
       context "with a blank @user.last_forum_read_at" do
         context "and no visits" do
-          should "return false" do
-            assert_equal(false, @topic.read_by?(@user))
+          should "return nothing" do
+            assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
           end
         end
 
@@ -71,8 +71,8 @@ class ForumTopicTest < ActiveSupport::TestCase
               FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 1.day.ago)
             end
 
-            should "return false" do
-              assert_equal(false, @topic.read_by?(@user))
+            should "return nothing" do
+              assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
             end
           end
 
@@ -81,8 +81,8 @@ class ForumTopicTest < ActiveSupport::TestCase
               FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 1.day.from_now)
             end
 
-            should "return true" do
-              assert_equal(true, @topic.read_by?(@user))
+            should "return nothing" do
+              assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
             end
           end
         end