From cc96f30e4794052c4ccaadcc0431fb9ff653d324 Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 22 Jan 2020 18:33:03 -0600 Subject: [PATCH] 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 | 3 +- app/models/forum_topic.rb | 32 +++++--- app/views/forum_topics/_listing.html.erb | 2 +- app/views/forum_topics/index.html.erb | 2 +- .../forum_topics_controller_test.rb | 46 ++++++++++++ test/unit/forum_topic_test.rb | 74 ------------------- 6 files changed, 69 insertions(+), 90 deletions(-) diff --git a/app/controllers/forum_topics_controller.rb b/app/controllers/forum_topics_controller.rb index 89536b629..6c0e42559 100644 --- a/app/controllers/forum_topics_controller.rb +++ b/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) diff --git a/app/models/forum_topic.rb b/app/models/forum_topic.rb index 57916e0a4..9516c4e6c 100644 --- a/app/models/forum_topic.rb +++ b/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 diff --git a/app/views/forum_topics/_listing.html.erb b/app/views/forum_topics/_listing.html.erb index ce25f1b4c..c7a932eb4 100644 --- a/app/views/forum_topics/_listing.html.erb +++ b/app/views/forum_topics/_listing.html.erb @@ -4,7 +4,7 @@ Sticky: <% end %> - <% if CurrentUser.is_member? && new_forum_topic?(topic, read_forum_topics) %> + <% if !topic.is_read? %> NEW <% end %> diff --git a/app/views/forum_topics/index.html.erb b/app/views/forum_topics/index.html.erb index a457b13e0..fda176d56 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 %>

- <%= render "listing", forum_topics: @forum_topics, read_forum_topics: @read_forum_topics %> + <%= render "listing", forum_topics: @forum_topics %> <%= numbered_paginator(@forum_topics) %> diff --git a/test/functional/forum_topics_controller_test.rb b/test/functional/forum_topics_controller_test.rb index 0eb4faa14..f31a09d40 100644 --- a/test/functional/forum_topics_controller_test.rb +++ b/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 diff --git a/test/unit/forum_topic_test.rb b/test/unit/forum_topic_test.rb index f4371659f..406171906 100644 --- a/test/unit/forum_topic_test.rb +++ b/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