Fix loading Home TL from database not respecting min_id and not including DMs (#1744)

* Rework tests

* Add tests

* Fix HomeFeed#get with min_id fetching from database

* Minor code cleanup and optimizations

* Add tests

* Take DMs into account when fetching home TL from database

* Fix not listing own DMs in Home timeline

* Add tests

* Please CodeClimate

app/models/home_feed.rb

@@ -16,34 +16,47 @@ class HomeFeed < Feed
     since_id = since_id.to_i if since_id.present?
     min_id   = min_id.to_i if min_id.present?
 
-    statuses = from_redis(limit, max_id, since_id, min_id)
+    if min_id.present?
+      redis_min_id = fetch_min_redis_id
+      return from_redis(limit, max_id, since_id, min_id) if redis_min_id && min_id >= redis_min_id
 
+      statuses = from_database(limit, redis_min_id, since_id, min_id)
       return statuses if statuses.size >= limit
 
-    redis_min_id = from_redis(1, nil, nil, 0).first&.id if min_id.present? || since_id.present?
-    redis_sufficient = redis_min_id && (
-      (min_id.present? && min_id >= redis_min_id) ||
-      (since_id.present? && since_id >= redis_min_id)
-    )
-
-    unless redis_sufficient
       remaining_limit = limit - statuses.size
-      max_id = statuses.last.id unless statuses.empty?
+      min_id = statuses.first.id unless statuses.empty?
-      statuses += from_database(remaining_limit, max_id, since_id, min_id)
+      from_redis(remaining_limit, max_id, since_id, min_id) + statuses
+    else
+      statuses = from_redis(limit, max_id, since_id, min_id)
+      return statuses if statuses.size >= limit
+
+      if since_id.present?
+        redis_min_id = fetch_min_redis_id
+        return statuses if redis_min_id.present? && since_id >= redis_min_id
       end
 
-    statuses
+      remaining_limit = limit - statuses.size
+      max_id = statuses.last.id unless statuses.empty?
+      statuses + from_database(remaining_limit, max_id, since_id, min_id)
+    end
   end
 
   protected
 
   def from_database(limit, max_id, since_id, min_id)
-    # Note that this query will not contains direct messages
+    scope = Status.where(account: @account.following)
-    Status
+    scope = scope.left_outer_joins(:mentions)
-      .where(account: [@account] + @account.following)
+    scope = scope.where(visibility: %i(public unlisted private)).or(scope.where(mentions: { account_id: @account.id })).group(Status.arel_table[:id])
-      .where(visibility: [:public, :unlisted, :private])
+    scope = scope.or(Status.where(account: @account))
+    scope
       .to_a_paginated_by_id(limit, min_id: min_id, max_id: max_id, since_id: since_id)
       .reject { |status| FeedManager.instance.filter?(:home, status, @account) }
       .sort_by { |status| -status.id }
   end
+
+  private
+
+  def fetch_min_redis_id
+    redis.zrangebyscore(key, '(0', '(+inf', limit: [0, 1], with_scores: true).first&.first&.to_i
+  end
 end

spec/models/home_feed_spec.rb

@@ -2,32 +2,82 @@ require 'rails_helper'
 
 RSpec.describe HomeFeed, type: :model do
   let(:account)  { Fabricate(:account) }
+  let(:followed) { Fabricate(:account) }
+  let(:other)    { Fabricate(:account) }
 
   subject { described_class.new(account) }
 
   describe '#get' do
     before do
+      account.follow!(followed)
+
       Fabricate(:status, account: account,  id: 1)
       Fabricate(:status, account: account,  id: 2)
-      Fabricate(:status, account: account, id: 3)
+      status = Fabricate(:status, account: followed, id: 3)
+      Fabricate(:mention, account: account, status: status)
       Fabricate(:status, account: account,  id: 10)
+      Fabricate(:status, account: other,    id: 11)
+      Fabricate(:status, account: followed, id: 12, visibility: :private)
+      Fabricate(:status, account: followed, id: 13, visibility: :direct)
+      Fabricate(:status, account: account,  id: 14, visibility: :direct)
+      dm = Fabricate(:status, account: followed, id: 15, visibility: :direct)
+      Fabricate(:mention, account: account, status: dm)
     end
 
     context 'when feed is generated' do
       before do
+        FeedManager.instance.populate_home(account)
+
+        # Add direct messages because populate_home does not do that
         Redis.current.zadd(
           FeedManager.instance.key(:home, account.id),
-          [[4, 4], [3, 3], [2, 2], [1, 1]]
+          [[14, 14], [15, 15]]
         )
       end
 
       it 'gets statuses with ids in the range from redis with database' do
-        results = subject.get(3)
+        results = subject.get(5)
 
-        expect(results.map(&:id)).to eq [3, 2, 1]
+        expect(results.map(&:id)).to eq [15, 14, 12, 10, 3]
         expect(results.first.attributes.keys).to eq %w(id updated_at)
       end
 
+      it 'with since_id present' do
+        results = subject.get(5, nil, 3, nil)
+        expect(results.map(&:id)).to eq [15, 14, 12, 10]
+      end
+
+      it 'with min_id present' do
+        results = subject.get(3, nil, nil, 0)
+        expect(results.map(&:id)).to eq [3, 2, 1]
+      end
+    end
+
+    context 'when feed is only partial' do
+      before do
+        FeedManager.instance.populate_home(account)
+
+        # Add direct messages because populate_home does not do that
+        Redis.current.zadd(
+          FeedManager.instance.key(:home, account.id),
+          [[14, 14], [15, 15]]
+        )
+
+        Redis.current.zremrangebyrank(FeedManager.instance.key(:home, account.id), 0, -2)
+      end
+
+      it 'gets statuses with ids in the range from redis with database' do
+        results = subject.get(5)
+
+        expect(results.map(&:id)).to eq [15, 14, 12, 10, 3]
+        expect(results.first.attributes.keys).to eq %w(id updated_at)
+      end
+
+      it 'with since_id present' do
+        results = subject.get(5, nil, 3, nil)
+        expect(results.map(&:id)).to eq [15, 14, 12, 10]
+      end
+
       it 'with min_id present' do
         results = subject.get(3, nil, nil, 0)
         expect(results.map(&:id)).to eq [3, 2, 1]
@@ -40,9 +90,14 @@ RSpec.describe HomeFeed, type: :model do
       end
 
       it 'returns from database' do
-        results = subject.get(3)
+        results = subject.get(5)
 
-        expect(results.map(&:id)).to eq [10, 3, 2]
+        expect(results.map(&:id)).to eq [15, 14, 12, 10, 3]
+      end
+
+      it 'with since_id present' do
+        results = subject.get(5, nil, 3, nil)
+        expect(results.map(&:id)).to eq [15, 14, 12, 10]
       end
 
       it 'with min_id present' do