From 0aed2533ac4c672787fa7d010a75e427d946d030 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Fri, 26 May 2017 09:04:13 -0400
Subject: [PATCH] Revert unread optimisation, has too many edge cases

---
 .../scheduled/update_first_topic_unread_at.rb |  9 ----
 app/models/topic.rb                           |  4 --
 app/models/topic_user.rb                      | 13 ------
 app/models/user_stat.rb                       | 24 -----------
 db/fixtures/999_topics.rb                     | 15 ++++++-
 ...70524182846_add_unread_tracking_columns.rb | 43 +------------------
 ...0526125321_drop_unread_tracking_columns.rb |  8 ++++
 lib/post_creator.rb                           |  6 +--
 lib/topic_query.rb                            |  1 -
 spec/components/post_creator_spec.rb          |  4 --
 spec/models/topic_user_spec.rb                | 30 -------------
 spec/models/user_stat_spec.rb                 | 28 ------------
 12 files changed, 26 insertions(+), 159 deletions(-)
 delete mode 100644 app/jobs/scheduled/update_first_topic_unread_at.rb
 create mode 100644 db/migrate/20170526125321_drop_unread_tracking_columns.rb

diff --git a/app/jobs/scheduled/update_first_topic_unread_at.rb b/app/jobs/scheduled/update_first_topic_unread_at.rb
deleted file mode 100644
index c5b34156334..00000000000
--- a/app/jobs/scheduled/update_first_topic_unread_at.rb
+++ /dev/null
@@ -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
diff --git a/app/models/topic.rb b/app/models/topic.rb
index c3fbbf98937..7f5b34b3988 100644
--- a/app/models/topic.rb
+++ b/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
diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb
index 3272fa8f189..924d012bca8 100644
--- a/app/models/topic_user.rb
+++ b/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,
diff --git a/app/models/user_stat.rb b/app/models/user_stat.rb
index 3416f3233c3..50761737e3f 100644
--- a/app/models/user_stat.rb
+++ b/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
diff --git a/db/fixtures/999_topics.rb b/db/fixtures/999_topics.rb
index 9587290a228..abdc372e8a4 100644
--- a/db/fixtures/999_topics.rb
+++ b/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!"
diff --git a/db/migrate/20170524182846_add_unread_tracking_columns.rb b/db/migrate/20170524182846_add_unread_tracking_columns.rb
index 3f326140487..28cb3d7d251 100644
--- a/db/migrate/20170524182846_add_unread_tracking_columns.rb
+++ b/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"
-    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
+    # no op, no need to create all data, next migration will delete it
   end
 
   def down
-    execute "DROP FUNCTION first_unread_topic_for(int)"
-    remove_column :user_stats, :first_topic_unread_at
-    remove_column :topics, :last_unread_at
+    raise "Can not be reverted"
   end
 end
diff --git a/db/migrate/20170526125321_drop_unread_tracking_columns.rb b/db/migrate/20170526125321_drop_unread_tracking_columns.rb
new file mode 100644
index 00000000000..3d118702189
--- /dev/null
+++ b/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
diff --git a/lib/post_creator.rb b/lib/post_creator.rb
index dc815ecf77a..cfcdc7b1cd9 100644
--- a/lib/post_creator.rb
+++ b/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
diff --git a/lib/topic_query.rb b/lib/topic_query.rb
index f32c850309b..a392b94bf4e 100644
--- a/lib/topic_query.rb
+++ b/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
diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb
index 45828924b8a..dcfecfe83fb 100644
--- a/spec/components/post_creator_spec.rb
+++ b/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
 
diff --git a/spec/models/topic_user_spec.rb b/spec/models/topic_user_spec.rb
index 97e41cff75e..bcc481c9c85 100644
--- a/spec/models/topic_user_spec.rb
+++ b/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
diff --git a/spec/models/user_stat_spec.rb b/spec/models/user_stat_spec.rb
index 86968285018..f440d78b210 100644
--- a/spec/models/user_stat_spec.rb
+++ b/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) }