From bb402f5a27a3d0b5cdea4b7ff202dd5dd05d3c49 Mon Sep 17 00:00:00 2001 From: r888888888 Date: Thu, 17 Jul 2014 15:53:36 -0700 Subject: [PATCH] fixes #2197 --- app/controllers/forum_topics_controller.rb | 17 +- app/logical/session_loader.rb | 15 -- app/models/forum_topic.rb | 64 ++++---- app/models/forum_topic_visit.rb | 31 +--- app/models/user.rb | 6 +- app/views/forum_topics/index.html.erb | 2 +- ...0140701224800_create_forum_topic_visits.rb | 1 + db/structure.sql | 7 + test/factories/forum_topic_visit.rb | 4 + .../forum_topics_controller_test.rb | 12 -- test/unit/forum_topic_test.rb | 153 ++++++++++-------- 11 files changed, 134 insertions(+), 178 deletions(-) create mode 100644 test/factories/forum_topic_visit.rb diff --git a/app/controllers/forum_topics_controller.rb b/app/controllers/forum_topics_controller.rb index 3683b6017..f0e7956eb 100644 --- a/app/controllers/forum_topics_controller.rb +++ b/app/controllers/forum_topics_controller.rb @@ -18,8 +18,7 @@ class ForumTopicsController < ApplicationController def index @query = ForumTopic.active.search(params[:search]) - @forum_topics = @query.order("is_sticky DESC, updated_at DESC").paginate(params[:page], :limit => params[:limit], :search_count => params[:search]) - @read_forum_topic_ids = read_forum_topic_ids + @forum_topics = @query.includes([:creator, :updater]).order("is_sticky DESC, updated_at DESC").paginate(params[:page], :limit => params[:limit], :search_count => params[:search]) respond_with(@forum_topics) do |format| format.xml do render :xml => @forum_topics.to_xml(:root => "forum-topics") @@ -29,12 +28,10 @@ class ForumTopicsController < ApplicationController def show @forum_topic = ForumTopic.find(params[:id]) + @forum_topic.mark_as_read!(CurrentUser.user) @forum_posts = ForumPost.search(:topic_id => @forum_topic.id).order("forum_posts.id").paginate(params[:page]) @forum_posts.each # hack to force rails to eager load respond_with(@forum_topic) - unless CurrentUser.user.is_anonymous? - session[:read_forum_topics] = @forum_topic.mark_as_read(read_forum_topic_ids) - end end def create @@ -67,7 +64,7 @@ class ForumTopicsController < ApplicationController def mark_all_as_read CurrentUser.user.update_attribute(:last_forum_read_at, Time.now) - session[:read_forum_topics] = "" + ForumTopicVisit.prune!(CurrentUser.user) redirect_to forum_topics_path, :notice => "All topics marked as read" end @@ -95,14 +92,6 @@ private end end - def read_forum_topics - session[:read_forum_topics].to_s - end - - def read_forum_topic_ids - read_forum_topics.scan(/(\S+) (\S+)/) - end - def check_privilege(forum_topic) if !forum_topic.editable_by?(CurrentUser.user) raise User::PrivilegeError diff --git a/app/logical/session_loader.rb b/app/logical/session_loader.rb index ed736de57..9de3cab00 100644 --- a/app/logical/session_loader.rb +++ b/app/logical/session_loader.rb @@ -13,10 +13,6 @@ class SessionLoader load_session_user elsif cookie_password_hash_valid? load_cookie_user - - # this means a new session is being created, so assume - # all the old forum topics can be marked as read - update_forum_last_read_at else load_session_for_api end @@ -104,16 +100,5 @@ private def set_time_zone Time.zone = CurrentUser.user.time_zone end - - def update_forum_last_read_at - unless CurrentUser.user.is_anonymous? - if CurrentUser.user.last_forum_read_at && CurrentUser.user.last_forum_read_at < CurrentUser.user.last_logged_in_at - CurrentUser.user.update_column(:last_forum_read_at, CurrentUser.user.last_logged_in_at) - elsif CurrentUser.user.last_forum_read_at.nil? - CurrentUser.user.update_column(:last_forum_read_at, CurrentUser.user.last_logged_in_at) - end - CurrentUser.user.update_column(:last_logged_in_at, Time.now) - end - end end diff --git a/app/models/forum_topic.rb b/app/models/forum_topic.rb index 902a971e8..f7d4c01ff 100644 --- a/app/models/forum_topic.rb +++ b/app/models/forum_topic.rb @@ -74,8 +74,34 @@ class ForumTopic < ActiveRecord::Base end 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 = nil) + user ||= CurrentUser.user + + match = ForumTopicVisit.where(:user_id => user.id, :forum_topic_id => id).first + if match + match.update_attribute(:last_read_at, updated_at) + else + ForumTopicVisit.create(:user_id => user.id, :forum_topic_id => id, :last_read_at => updated_at) + end + + # user.update_attribute(:last';¬≥÷_forum_read_at, ForumTopicVisit.where(:user_id => user.id, :forum_topic_id => id).minimum(:last_read_at) || updated_at) + end + end + extend SearchMethods include CategoryMethods + include VisitMethods def editable_by?(user) creator_id == user.id || user.is_janitor? @@ -94,7 +120,7 @@ class ForumTopic < ActiveRecord::Base end def last_page - (posts.count / Danbooru.config.posts_per_page.to_f).ceil + (response_count / Danbooru.config.posts_per_page.to_f).ceil end def presenter(forum_posts) @@ -105,42 +131,6 @@ class ForumTopic < ActiveRecord::Base super + [:text_index] end - def check!(user) - ForumTopicVisit.check!(user, self) - end - - def mark_as_read(read_forum_topic_ids) - hash = read_forum_topic_ids.inject({}) do |hash, x| - hash[x[0].to_s] = x[1].to_s - hash - end - hash[id.to_s] = updated_at.to_i.to_s - result = hash.to_a.flatten.join(" ") - while result.size > 500 - ids = result.scan(/\S+/) - result = ids[(ids.size / 2)..-1].join(" ") - end - update_last_forum_read_at(hash.keys) - result - end - - def update_last_forum_read_at(read_forum_topic_ids) - query = ForumTopic.where("true") - if CurrentUser.user.last_forum_read_at.present? - query = query.where("updated_at >= ?", CurrentUser.last_forum_read_at) - end - if read_forum_topic_ids.any? - query = query.where("id not in (?)", read_forum_topic_ids) - end - query = query.order("updated_at asc") - topic = query.first - if topic - CurrentUser.user.update_attribute(:last_forum_read_at, topic.updated_at) - else - CurrentUser.user.update_attribute(:last_forum_read_at, Time.now) - end - end - def merge(topic) ForumPost.where(:id => topic.posts.map(&:id)).update_all(:topic_id => id) update_attribute(:is_deleted, true) diff --git a/app/models/forum_topic_visit.rb b/app/models/forum_topic_visit.rb index 7409735d4..4df979466 100644 --- a/app/models/forum_topic_visit.rb +++ b/app/models/forum_topic_visit.rb @@ -1,30 +1,9 @@ class ForumTopicVisit < ActiveRecord::Base - def self.check!(user, topic) - match = where(:user_id => user.id, :forum_topic_id => topic.id).first - result = false - if match - if match.last_read_at < topic.updated_at - result = true - end - match.update_attribute(:last_read_at, topic.updated_at) - else - create(:user_id => user.id, :forum_topic_id => topic.id, :last_read_at => topic.updated_at) - end - result - end + belongs_to :user + belongs_to :forum_topic + attr_accessible :user_id, :user, :forum_topic_id, :forum_topic, :last_read_at - def self.check_list!(user, topics) - matches = where(:user_id => user.id, :forum_topic_id => topics.map(&:id)).to_a.inject({}) do |hash, x| - hash[x.forum_topic_id] = x - hash - end - topics.each do |topic| - if matches[topic.id] - matches[topic.id].update_attribute(:last_read_at, topic.updated_at) - else - create(:user_id => user.id,, :forum_topic_id => topic.id, :last_read_at => topic.updated_at) - end - end - matches + def self.prune!(user) + where("last_read_at < ?", user.last_forum_read_at).delete_all end end diff --git a/app/models/user.rb b/app/models/user.rb index 17d0dcfe1..de64dd744 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -428,10 +428,10 @@ class User < ActiveRecord::Base module ForumMethods def has_forum_been_updated? return false unless is_gold? - newest_topic = ForumTopic.order("updated_at desc").first - return false if newest_topic.nil? + max_updated_at = ForumTopic.maximum(:updated_at) + return false if max_updated_at.nil? return true if last_forum_read_at.nil? - return newest_topic.updated_at > last_forum_read_at + return max_updated_at > last_forum_read_at end end diff --git a/app/views/forum_topics/index.html.erb b/app/views/forum_topics/index.html.erb index cacf5555d..2674ea267 100644 --- a/app/views/forum_topics/index.html.erb +++ b/app/views/forum_topics/index.html.erb @@ -24,7 +24,7 @@ Sticky: <% end %> - <% unless topic.read_by?(CurrentUser.user, @read_forum_topic_ids) %> + <% unless topic.read_by?(CurrentUser.user) %> NEW <% end %> diff --git a/db/migrate/20140701224800_create_forum_topic_visits.rb b/db/migrate/20140701224800_create_forum_topic_visits.rb index 6c4f1051a..f01ff4db9 100644 --- a/db/migrate/20140701224800_create_forum_topic_visits.rb +++ b/db/migrate/20140701224800_create_forum_topic_visits.rb @@ -10,5 +10,6 @@ class CreateForumTopicVisits < ActiveRecord::Migration add_index :forum_topic_visits, :user_id add_index :forum_topic_visits, :forum_topic_id + add_index :forum_topic_visits, :last_read_at end end diff --git a/db/structure.sql b/db/structure.sql index f291c8d4c..660244f05 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -6131,6 +6131,13 @@ CREATE INDEX index_forum_posts_on_topic_id ON forum_posts USING btree (topic_id) CREATE INDEX index_forum_topic_visits_on_forum_topic_id ON forum_topic_visits USING btree (forum_topic_id); +-- +-- Name: index_forum_topic_visits_on_last_read_at; Type: INDEX; Schema: public; Owner: -; Tablespace: +-- + +CREATE INDEX index_forum_topic_visits_on_last_read_at ON forum_topic_visits USING btree (last_read_at); + + -- -- Name: index_forum_topic_visits_on_user_id; Type: INDEX; Schema: public; Owner: -; Tablespace: -- diff --git a/test/factories/forum_topic_visit.rb b/test/factories/forum_topic_visit.rb new file mode 100644 index 000000000..8195bab48 --- /dev/null +++ b/test/factories/forum_topic_visit.rb @@ -0,0 +1,4 @@ +FactoryGirl.define do + factory(:forum_topic_visit) do + end +end diff --git a/test/functional/forum_topics_controller_test.rb b/test/functional/forum_topics_controller_test.rb index 2f0c65204..ad21ed045 100644 --- a/test/functional/forum_topics_controller_test.rb +++ b/test/functional/forum_topics_controller_test.rb @@ -21,18 +21,6 @@ class ForumTopicsControllerTest < ActionController::TestCase get :show, {:id => @forum_topic.id} assert_response :success end - - context "when the read_forum_topics session exceeds the maximum cookie size" do - setup do - @cookie_data = 1.upto(10_000).to_a.join(" ") - end - - should "truncate" do - get :show, {:id => @forum_topic.id}, {:user_id => @user.id, :read_forum_topics => @cookie_data} - assert_response :success - assert_equal(395, session[:read_forum_topics].size) - end - end end context "index action" do diff --git a/test/unit/forum_topic_test.rb b/test/unit/forum_topic_test.rb index 3ec43074c..3e6a9b65d 100644 --- a/test/unit/forum_topic_test.rb +++ b/test/unit/forum_topic_test.rb @@ -14,55 +14,102 @@ class ForumTopicTest < ActiveSupport::TestCase CurrentUser.ip_addr = nil end - context "#update_last_forum_read_at" do - setup do - @topics = [@topic] - 1.upto(6) do |i| - Timecop.travel(i.days.from_now) do - @topics << FactoryGirl.create(:forum_topic, :title => "xxx") - end - end - @read_forum_topic_ids = [] - @read_forum_topic_ids << @topics[0] - @read_forum_topic_ids << @topics[2] - @read_forum_topic_ids << @topics[4] - end - - context "when the user's last_forum_read_at is null" do + context "#read_by?" do + context "with a populated @user.last_forum_read_at" do setup do - @user.update_attribute(:last_forum_read_at, nil) + @user.update_attribute(:last_forum_read_at, Time.now) end - should "return the oldest unread topic" do - @topic.update_last_forum_read_at(@read_forum_topic_ids) - @user.reload - assert_equal(@topics[1].updated_at.to_i, @user.last_forum_read_at.to_i) - end - - context "when all topics have been read" do + context "and no visits for a topic" do setup do - @read_forum_topic_ids = ForumTopic.all.map(&:id) - @timestamp = Time.now - Time.stubs(:now).returns(@timestamp) + @topic.update_column(:updated_at, 1.day.from_now) end - should "return the current time" do - @topic.update_last_forum_read_at(@read_forum_topic_ids) - @user.reload - assert_equal(@timestamp.to_i, @user.last_forum_read_at.to_i) + should "return false" do + assert_equal(false, @topic.read_by?(@user)) + 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 + FactoryGirl.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)) + end + end + + context "that postdates the topic" do + setup do + FactoryGirl.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)) + end end end end - context "when the user's last_forum_read_at is 2 days from now" do - setup do - @user.update_attribute(:last_forum_read_at, 2.days.from_now) + 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)) + end end - should "return the oldest unread topic" do - @topic.update_last_forum_read_at(@read_forum_topic_ids) + context "and a visit" do + context "that predates the topic" do + setup do + FactoryGirl.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)) + end + end + + context "that postdates the topic" do + setup do + FactoryGirl.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 1.days.from_now) + end + + should "return true" do + assert_equal(true, @topic.read_by?(@user)) + end + end + end + end + end + + context "#mark_as_read!" do + context "without a previous visit" do + should "create a new visit" do + assert_difference("ForumTopicVisit.count", 1) do + @topic.mark_as_read!(@user) + end @user.reload - assert_equal(@topics[3].updated_at.to_i, @user.last_forum_read_at.to_i) + assert_equal(@topic.updated_at, ForumTopicVisit.last.last_read_at) + end + end + + context "with a previous visit" do + setup do + FactoryGirl.create(:forum_topic_visit, user: @user, forum_topic: @topic, last_read_at: 1.day.ago) + end + + should "update the visit" do + assert_difference("ForumTopicVisit.count", 0) do + @topic.mark_as_read!(@user) + end + @user.reload + assert_equal(@topic.updated_at, ForumTopicVisit.last.last_read_at) end end end @@ -80,40 +127,6 @@ class ForumTopicTest < ActiveSupport::TestCase end end - context "#read_by?" do - context "for a topic that was never read by the user" do - should "return false" do - assert_equal(false, @topic.read_by?(@user, [[(@topic.id + 1).to_s, "1"]])) - end - end - - context "for a topic that was read by the user but has been updated since then" do - should "return false" do - assert_equal(false, @topic.read_by?(@user, [["#{@topic.id}", "#{1.day.ago.to_i}"]])) - end - end - - context "for a topic that was read by the user and has not been updated since" do - should "return true" do - assert_equal(true, @topic.read_by?(@user, [["#{@topic.id}", "#{1.day.from_now.to_i}"]])) - end - end - end - - context "#mark_as_read" do - should "include the topic id and updated_at timestamp" do - plus_one = @topic.id + 1 - result = @topic.mark_as_read([["#{plus_one}", "2"]]) - assert_equal("#{plus_one} 2 #{@topic.id} #{@topic.updated_at.to_i}", result) - end - - should "prune the string if it gets too long" do - array = (1..1_000).to_a.map(&:to_s).in_groups_of(2) - result = @topic.mark_as_read(array) - assert_operator result.size, :<=, 500 - end - end - context "constructed with nested attributes for its original post" do should "create a matching forum post" do assert_difference(["ForumTopic.count", "ForumPost.count"], 1) do