Revert unread optimisation, has too many edge cases

app/jobs/scheduled/update_first_topic_unread_at.rb

@@ -1,9 +0,0 @@
-module Jobs
-  class UpdateFirstTopicUnreadAt < Jobs::Scheduled
-    every 1.day
-
-    def execute(args)
-      UserStat.update_first_topic_unread_at!
-    end
-  end
-end

app/models/topic.rb

@@ -541,10 +541,6 @@ SQL
   def self.reset_highest(topic_id)
     result = exec_sql "UPDATE topics
                         SET
-                        last_unread_at = (
-                          SELECT MAX(created_at) FROM posts
-                          WHERE topic_id = :topic_id
-                        ),
                         highest_staff_post_number = (
                             SELECT COALESCE(MAX(post_number), 0) FROM posts
                             WHERE topic_id = :topic_id AND

app/models/topic_user.rb

@@ -146,19 +146,6 @@ SQL
       message[:notifications_reason_id] = reason_id if reason_id
       MessageBus.publish("/topic/#{topic_id}", message, user_ids: [user_id])
 
-      if notification_level && notification_level >= notification_levels[:tracking]
-        sql = <<SQL
-        UPDATE user_stats us
-        SET first_topic_unread_at = t.last_unread_at
-        FROM topics t
-        WHERE t.id = :topic_id AND
-          t.last_unread_at < us.first_topic_unread_at AND
-          us.user_id = :user_id
-SQL
-        exec_sql(sql, topic_id: topic_id, user_id: user_id)
-      end
-
-
       DiscourseEvent.trigger(:topic_notification_level_changed,
         notification_level,
         user_id,

app/models/user_stat.rb

@@ -81,30 +81,6 @@ class UserStat < ActiveRecord::Base
     update_columns(reset_bounce_score_after: nil, bounce_score: 0)
   end
 
-  def self.update_first_topic_unread_at!
-
-    exec_sql <<SQL
-    UPDATE user_stats us
-    SET first_topic_unread_at = COALESCE(X.first_unread_at, current_timestamp)
-    FROM
-    (
-      SELECT u.id user_id, MIN(last_unread_at) first_unread_at
-      FROM users u
-      JOIN topic_users tu ON tu.user_id = u.id
-      JOIN topics t ON t.id = tu.topic_id
-      WHERE notification_level > 1 AND last_read_post_number < CASE WHEN moderator OR admin
-                                                                THEN t.highest_staff_post_number
-                                                                ELSE t.highest_post_number
-                                                               END
-        AND t.deleted_at IS NULL
-      GROUP BY u.id
-    ) X
-    WHERE X.user_id = us.user_id AND X.first_unread_at <> first_topic_unread_at
-SQL
-
-    nil
-  end
-
   protected
 
   def trigger_badges

db/fixtures/999_topics.rb

@@ -68,15 +68,28 @@ end
 
 # run this later, cause we need to make sure new application controller resilience is in place first
 
+ColumnDropper.drop(
+  table: 'user_stats',
+  after_migration: 'DropUnreadTrackingColumns',
+  columns: %w{
+    first_topic_unread_at
+  },
+  on_drop: ->(){
+    STDERR.puts "Removing superflous user stats columns!"
+    ActiveRecord::Base.exec_sql "DROP FUNCTION IF EXISTS first_unread_topic_for(int)"
+  }
+)
+
 ColumnDropper.drop(
   table: 'topics',
-  after_migration: 'AddTopicColumnsBack',
+  after_migration: 'DropUnreadTrackingColumns',
   columns: %w{
     inappropriate_count
     bookmark_count
     off_topic_count
     illegal_count
     notify_user_count
+    last_unread_at
   },
   on_drop: ->(){
     STDERR.puts "Removing superflous topic columns!"

db/migrate/20170524182846_add_unread_tracking_columns.rb

@@ -1,48 +1,9 @@
 class AddUnreadTrackingColumns < ActiveRecord::Migration
   def up
-    add_column :user_stats, :first_topic_unread_at, :datetime, null: false, default: "epoch"
+    # no op, no need to create all data, next migration will delete it
-    add_column :topics, :last_unread_at, :datetime, null: false, default: "epoch"
-
-    execute <<SQL
-    UPDATE topics SET last_unread_at = COALESCE((
-      SELECT MAX(created_at)
-      FROM posts
-      WHERE topics.id = posts.topic_id
-    ), current_timestamp)
-SQL
-
-    execute <<SQL
-    UPDATE user_stats SET first_topic_unread_at = COALESCE((
-      SELECT MIN(last_unread_at) FROM topics
-      JOIN users u ON u.id = user_stats.user_id
-      JOIN topic_users tu ON tu.user_id = user_stats.user_id AND topics.id = tu.topic_id
-      WHERE notification_level > 1 AND last_read_post_number < CASE WHEN moderator OR admin
-                                                                THEN topics.highest_staff_post_number
-                                                                ELSE topics.highest_post_number
-                                                               END
-        AND topics.deleted_at IS NULL
-    ), current_timestamp)
-SQL
-
-    add_index :topics, [:last_unread_at]
-
-    # we need this function for performance reasons
-    execute <<SQL
-    CREATE OR REPLACE FUNCTION first_unread_topic_for(user_id int)
-    RETURNS timestamp AS
-    $$
-    SELECT COALESCE(first_topic_unread_at, 'epoch'::timestamp)
-    FROM users u
-    JOIN user_stats ON user_id = u.id
-    WHERE u.id = $1
-    $$
-    LANGUAGE SQL STABLE
-SQL
   end
 
   def down
-    execute "DROP FUNCTION first_unread_topic_for(int)"
+    raise "Can not be reverted"
-    remove_column :user_stats, :first_topic_unread_at
-    remove_column :topics, :last_unread_at
   end
 end

db/migrate/20170526125321_drop_unread_tracking_columns.rb

@@ -0,0 +1,8 @@
+class DropUnreadTrackingColumns < ActiveRecord::Migration
+  def up
+  end
+
+  def down
+    raise "Can not be reverted"
+  end
+end

lib/post_creator.rb

@@ -377,17 +377,15 @@ class PostCreator
   end
 
   def update_topic_stats
-    attrs = {last_unread_at: @post.created_at}
-
     if @post.post_type != Post.types[:whisper]
+      attrs = {}
       attrs[:last_posted_at] = @post.created_at
       attrs[:last_post_user_id] = @post.user_id
       attrs[:word_count] = (@topic.word_count || 0) + @post.word_count
       attrs[:excerpt] = @post.excerpt(220, strip_links: true) if new_topic?
       attrs[:bumped_at] = @post.created_at unless @post.no_bump
+      @topic.update_attributes(attrs)
     end
-
-    @topic.update_attributes(attrs)
   end
 
   def update_topic_auto_close

lib/topic_query.rb

@@ -294,7 +294,6 @@ class TopicQuery
 
     list
         .where("tu.last_read_post_number < topics.#{col_name}")
-        .where("topics.last_unread_at >= first_unread_topic_for(?)", user_id)
         .where("COALESCE(tu.notification_level, :regular) >= :tracking",
                regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking])
   end

spec/components/post_creator_spec.rb

@@ -411,12 +411,9 @@ describe PostCreator do
       expect(topic.posts_count).to eq(1)
       expect(topic.highest_staff_post_number).to eq(3)
 
-      expect(topic.last_unread_at).to eq(whisper_reply.created_at)
-
       topic.update_columns(highest_staff_post_number:0,
                            highest_post_number:0,
                            posts_count: 0,
-                           last_unread_at: 1.year.ago,
                            last_posted_at: 1.year.ago)
 
       Topic.reset_highest(topic.id)
@@ -426,7 +423,6 @@ describe PostCreator do
       expect(topic.posts_count).to eq(1)
       expect(topic.last_posted_at).to eq(first.created_at)
       expect(topic.highest_staff_post_number).to eq(3)
-      expect(topic.last_unread_at).to eq(whisper_reply.created_at)
     end
   end
 

spec/models/topic_user_spec.rb

@@ -38,36 +38,6 @@ describe TopicUser do
 
   end
 
-  describe "first unread" do
-    it "correctly update first unread as needed" do
-      time1 = 3.days.ago
-      time2 = 2.days.ago
-      time3 = 1.days.ago
-
-      topic1 = Fabricate(:topic, last_unread_at: time1)
-      topic2 = Fabricate(:topic, last_unread_at: time2)
-      topic3 = Fabricate(:topic, last_unread_at: time3)
-
-      user = Fabricate(:user)
-      user.user_stat.update_columns(first_topic_unread_at: Time.zone.now)
-
-      TopicUser.change(user.id, topic3, notification_level: tracking)
-
-      user.user_stat.reload
-      expect(user.user_stat.first_topic_unread_at).to be_within(1.second).of(time3)
-
-      TopicUser.change(user.id, topic2, notification_level: watching)
-
-      user.user_stat.reload
-      expect(user.user_stat.first_topic_unread_at).to be_within(1.second).of(time2)
-
-      TopicUser.change(user.id, topic1, notification_level: regular)
-
-      user.user_stat.reload
-      expect(user.user_stat.first_topic_unread_at).to be_within(1.second).of(time2)
-    end
-  end
-
   describe '#notification_levels' do
     context "verify enum sequence" do
       before do

spec/models/user_stat_spec.rb

@@ -10,34 +10,6 @@ describe UserStat do
     expect(user.user_stat.new_since).to be_present
   end
 
-  context "#update_first_topic_unread_at" do
-    it "updates date correctly for staff" do
-      now = Time.zone.now
-
-      admin = Fabricate(:admin)
-      topic = Fabricate(:topic,
-                          highest_staff_post_number: 7,
-                          highest_post_number: 1,
-                          last_unread_at: now
-                       )
-
-      UserStat.update_first_topic_unread_at!
-
-      admin.reload
-
-      expect(admin.user_stat.first_topic_unread_at).to_not be_within(5.years).of(now)
-
-      TopicUser.change(admin.id, topic.id, last_read_post_number: 1,
-                                           notification_level: NotificationLevels.all[:tracking])
-
-      UserStat.update_first_topic_unread_at!
-
-      admin.reload
-
-      expect(admin.user_stat.first_topic_unread_at).to be_within(1.second).of(now)
-    end
-  end
-
   context '#update_view_counts' do
 
     let(:user) { Fabricate(:user) }