forum: fix topics being incorrectly marked as unread (again).

Second attempt at 71690cacc. Fix topics on page 2+ being still marked as
unread after the user has marked all topics as read.

app/controllers/forum_topics_controller.rb

@@ -24,9 +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, :updater, :forum_topic_visit_by_current_user).load if request.format.html?
     @forum_topics = @forum_topics.includes(:creator, :original_post).load if request.format.atom?
 
     respond_with(@forum_topics)

app/models/forum_topic.rb

@@ -15,6 +15,7 @@ class ForumTopic < ApplicationRecord
   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_one :forum_topic_visit_by_current_user, -> { where(user_id: CurrentUser.id) }, class_name: "ForumTopicVisit"
   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
 
@@ -58,14 +59,18 @@ class ForumTopic < ApplicationRecord
     end
 
     def read_by_user(user)
-      return none if user.last_forum_read_at.nil? || user.last_forum_read_at < '2000-01-01'
+      last_forum_read_at = user.last_forum_read_at || "2000-01-01".to_time
 
       read_topics = user.visited_forum_topics.where("forum_topic_visits.last_read_at >= forum_topics.updated_at")
-      old_topics = where("? >= forum_topics.updated_at", user.last_forum_read_at)
+      old_topics = where("? >= forum_topics.updated_at", last_forum_read_at)
 
       where(id: read_topics).or(where(id: old_topics))
     end
 
+    def unread_by_user(user)
+      where.not(id: ForumTopic.read_by_user(user))
+    end
+
     def sticky_first
       order(is_sticky: :desc, updated_at: :desc)
     end
@@ -110,17 +115,10 @@ class ForumTopic < ApplicationRecord
         ForumTopicVisit.create(:user_id => user.id, :forum_topic_id => id, :last_read_at => updated_at)
       end
 
-      has_unread_topics =
-        ForumTopic
-        .permitted
-        .active
-        .where("forum_topics.updated_at >= ?", user.last_forum_read_at)
-        .joins("left join forum_topic_visits on (forum_topic_visits.forum_topic_id = forum_topics.id and forum_topic_visits.user_id = #{user.id})")
-        .where("(forum_topic_visits.id is null or forum_topic_visits.last_read_at < forum_topics.updated_at)")
-        .exists?
+      unread_topics = ForumTopic.permitted.active.unread_by_user(user)
 
-      unless has_unread_topics
-        user.update_attribute(:last_forum_read_at, Time.now)
+      if !unread_topics.exists?
+        user.update!(last_forum_read_at: Time.zone.now)
         ForumTopicVisit.prune!(user)
       end
     end
@@ -138,6 +136,16 @@ class ForumTopic < ApplicationRecord
     user.level >= min_level
   end
 
+  # XXX forum_topic_visit_by_current_user is a hack to reduce queries on the forum index.
+  def is_read?
+    return true if CurrentUser.is_anonymous?
+
+    topic_last_read_at = forum_topic_visit_by_current_user&.last_read_at || "2000-01-01".to_time
+    forum_last_read_at = CurrentUser.last_forum_read_at || "2000-01-01".to_time
+
+    (topic_last_read_at >= updated_at) || (forum_last_read_at >= updated_at)
+  end
+
   def create_mod_action_for_delete
     ModAction.log("deleted forum topic ##{id} (title: #{title})", :forum_topic_delete)
   end

app/views/forum_topics/_listing.html.erb

@@ -4,7 +4,7 @@
       <span class="sticky">Sticky:</span>
     <% end %>
 
-    <% if CurrentUser.is_member? && new_forum_topic?(topic, read_forum_topics) %>
+    <% if !topic.is_read? %>
       <span class="new">NEW</span>
     <% end %>
 

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, read_forum_topics: @read_forum_topics %>
+    <%= render "listing", forum_topics: @forum_topics %>
 
     <%= numbered_paginator(@forum_topics) %>
   </div>

test/functional/forum_topics_controller_test.rb

@@ -128,6 +128,52 @@ class ForumTopicsControllerTest < ActionDispatch::IntegrationTest
           assert_select "a.forum-post-link", count: 0, text: @topic2.title
         end
       end
+
+      context "when listing topics" do
+        should "always show topics as read for anonymous users" do
+          get forum_topics_path
+          assert_select "span.new", count: 0
+        end
+
+        should "show topics as read after viewing them" do
+          get_auth forum_topics_path, @user
+          assert_response :success
+          assert_select "span.new", count: 3
+
+          get_auth forum_topic_path(@forum_topic.id), @user
+          assert_response :success
+
+          get_auth forum_topics_path, @user
+          assert_response :success
+          assert_select "span.new", count: 2
+        end
+
+        should "show topics as read after marking all as read" do
+          get_auth forum_topics_path, @user
+          assert_response :success
+          assert_select "span.new", count: 3
+
+          post_auth mark_all_as_read_forum_topics_path, @user
+          assert_response 302
+
+          get_auth forum_topics_path, @user
+          assert_response :success
+          assert_select "span.new", count: 0
+        end
+
+        should "show topics on page 2 as read after marking all as read" do
+          get_auth forum_topics_path(page: 2, limit: 1), @user
+          assert_response :success
+          assert_select "span.new", count: 1
+
+          post_auth mark_all_as_read_forum_topics_path, @user
+          assert_response 302
+
+          get_auth forum_topics_path(page: 2, limit: 1), @user
+          assert_response :success
+          assert_select "span.new", count: 0
+        end
+      end
     end
 
     context "edit action" do

test/unit/forum_topic_test.rb

@@ -15,80 +15,6 @@ class ForumTopicTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
 
-    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)
-        end
-
-        context "and no visits for a topic" do
-          setup do
-            @topic.update_column(:updated_at, 1.day.from_now)
-          end
-
-          should "return nothing" do
-            assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
-          end
-        end
-
-        context "and a visit for a topic" do
-          setup do
-            @topic.update_column(:updated_at, 1.day.from_now)
-          end
-
-          context "that predates the topic" do
-            setup do
-              FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 16.hours.from_now)
-            end
-
-            should "return nothing" do
-              assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
-            end
-          end
-
-          context "that postdates the topic" do
-            setup do
-              FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 2.days.from_now)
-            end
-
-            should "return the topic" do
-              assert_equal([@topic.id], ForumTopic.read_by_user(@user).map(&:id))
-            end
-          end
-        end
-      end
-
-      context "with a blank @user.last_forum_read_at" do
-        context "and no visits" do
-          should "return nothing" do
-            assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
-          end
-        end
-
-        context "and a visit" do
-          context "that predates the topic" do
-            setup do
-              FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 1.day.ago)
-            end
-
-            should "return nothing" do
-              assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
-            end
-          end
-
-          context "that postdates the topic" do
-            setup do
-              FactoryBot.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 1.day.from_now)
-            end
-
-            should "return nothing" do
-              assert_equal([], ForumTopic.read_by_user(@user).map(&:id))
-            end
-          end
-        end
-      end
-    end
-
     context "#mark_as_read!" do
       context "without a previous visit" do
         should "create a new visit" do