Merge pull request #38 from NeuromatchAcademy/feature-filter-boosts

[Bugfix] Make filter boosts respect local vs. remote scope

app/models/public_feed.rb

@@ -25,13 +25,14 @@ def get(limit, max_id = nil, since_id = nil, min_id = nil)
     scope.merge!(without_local_only_scope) unless allow_local_only?
     scope.merge!(without_replies_scope) unless with_replies?
     scope.merge!(without_reblogs_scope) unless with_reblogs?
-    scope.merge!(without_duplicate_reblogs) if with_reblogs?
     scope.merge!(local_only_scope) if local_only?
     scope.merge!(remote_only_scope) if remote_only?
     scope.merge!(account_filters_scope) if account?
     scope.merge!(media_only_scope) if media_only?
     scope.merge!(language_scope) if account&.chosen_languages.present?
 
+    scope.merge!(without_duplicate_reblogs) if with_reblogs?
+
     scope.cache_ids.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id)
   end
 
@@ -91,15 +92,21 @@ def without_reblogs_scope
     Status.without_reblogs
   end
 
+  def max_boost_id_scope
+    Status.where(<<~SQL.squish)
+      "statuses"."id" = (
+        SELECT MAX(id)
+        FROM "statuses" "s2"
+        WHERE "s2"."reblog_of_id" = "statuses"."reblog_of_id"
+          #{'AND ("s2"."local" = true OR "s2"."uri" IS NULL)' if local_only?}
+          #{'AND "s2"."local" = false AND "s2"."uri" IS NOT NULL' if remote_only?}
+        )
+    SQL
+  end
+
   def without_duplicate_reblogs
     Status.where(statuses: { reblog_of_id: nil })
-          .or(Status.where(<<~SQL.squish))
-            "statuses"."id" = (
-              SELECT MAX(id)
-              FROM statuses s2
-              WHERE s2.reblog_of_id = statuses.reblog_of_id
-            )
-          SQL
+          .or(max_boost_id_scope)
   end
 
   def media_only_scope

db/migrate/20240129222920_add_index_reblog_of_id_and_status_to_statuses.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class AddIndexReblogOfIdAndStatusToStatuses < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  def change
+    add_index :statuses, [:reblog_of_id, :id], order: { reblog_of_id: 'DESC NULLS LAST', id: 'DESC' }, algorithm: :concurrently
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_09_07_150100) do
+ActiveRecord::Schema[7.1].define(version: 2024_01_29_222920) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -993,6 +993,7 @@
     t.index ["in_reply_to_account_id"], name: "index_statuses_on_in_reply_to_account_id", where: "(in_reply_to_account_id IS NOT NULL)"
     t.index ["in_reply_to_id"], name: "index_statuses_on_in_reply_to_id", where: "(in_reply_to_id IS NOT NULL)"
     t.index ["reblog_of_id", "account_id"], name: "index_statuses_on_reblog_of_id_and_account_id"
+    t.index ["reblog_of_id", "id"], name: "index_statuses_on_reblog_of_id_and_id", order: { reblog_of_id: "DESC NULLS LAST", id: :desc }
     t.index ["uri"], name: "index_statuses_on_uri", unique: true, opclass: :text_pattern_ops, where: "(uri IS NOT NULL)"
   end
 

spec/models/public_feed_spec.rb

@@ -35,6 +35,11 @@
     context 'with with_reblogs option' do
       subject { described_class.new(nil, with_reblogs: true).get(20).map(&:id) }
 
+      let!(:poster)  { Fabricate(:account, domain: nil) }
+      let!(:booster) { Fabricate(:account, domain: nil) }
+      let!(:second_booster) { Fabricate(:account, domain: nil) }
+      let!(:remote_booster) { Fabricate(:account, domain: 'example.com') }
+
       it 'does include boosts' do
         status = Fabricate(:status)
         boost = Fabricate(:status, reblog_of_id: status.id)
@@ -44,10 +49,6 @@
       end
 
       it 'only includes the most recent boost' do
-        poster = Fabricate(:account, domain: nil)
-        booster = Fabricate(:account, domain: nil)
-        second_booster = Fabricate(:account, domain: nil)
-
         status = Fabricate(:status, account: poster)
         boost = Fabricate(:status, reblog_of_id: status.id, account: poster)
         second_boost = Fabricate(:status, reblog_of_id: status.id, account: booster)
@@ -60,33 +61,55 @@
       end
 
       it 'filters duplicate boosts across pagination' do
-        poster = Fabricate(:account, domain: nil)
-        booster = Fabricate(:account, domain: nil)
-
         status = Fabricate(:status, account: poster)
-        # sleep for 2ms to make sure the other posts come in a greater snowflake ID
-        sleep(0.002)
 
-        boost = Fabricate(:status, reblog_of_id: status.id, account: poster)
+        boost = Fabricate(:status, reblog_of_id: status.id, id: status.id + 1, account: poster)
 
         # sleep for 2ms to make sure the other posts come in a greater snowflake ID
         sleep(0.002)
 
-        (1..20).each do |_i|
-          Fabricate(:status, account: poster)
+        n_posts = 20
+        (1..n_posts).each do |i|
+          Fabricate(:status, account: poster, id: boost.id + i)
         end
 
         # before a second boost, the second page should still include the original boost
         second_page = described_class.new(nil, with_reblogs: true).get(20, boost.id + 1).map(&:id)
         expect(second_page).to include(boost.id)
 
         # after a second boost, the second page should no longer include the original boost
-        second_boost = Fabricate(:status, reblog_of_id: status.id, account: booster)
+        second_boost = Fabricate(:status, reblog_of_id: status.id, id: boost.id + n_posts + 1, account: booster)
         second_page = described_class.new(nil, with_reblogs: true).get(20, boost.id + 1).map(&:id)
 
         expect(subject).to include(second_boost.id)
         expect(second_page).to_not include(boost.id)
       end
+
+      context 'with local option' do
+        subject { described_class.new(nil, with_reblogs: true, local: true, remote: false).get(20).map(&:id) }
+
+        it 'shows the most recent local boost when there is a more recent remote boost' do
+          status = Fabricate(:status, account: poster)
+          local_boost = Fabricate(:status, reblog_of_id: status.id, local: true, account: booster)
+          remote_boost = Fabricate(:status, reblog_of_id: status.id, id: local_boost.id + 1, local: false, uri: 'https://example.com/boosturl', account: remote_booster)
+
+          expect(subject).to include(local_boost.id)
+          expect(subject).to_not include(remote_boost.id)
+        end
+      end
+
+      context 'with remote option' do
+        subject { described_class.new(nil, with_reblogs: true, local: false, remote: true).get(20).map(&:id) }
+
+        it 'shows the most recent remote boost when there is a more recent local boost' do
+          status = Fabricate(:status, account: poster)
+          remote_boost = Fabricate(:status, reblog_of_id: status.id, local: false, uri: 'https://example.com/boosturl', account: remote_booster)
+          local_boost = Fabricate(:status, reblog_of_id: status.id, id: remote_boost.id + 1, local: true, account: booster)
+
+          expect(subject).to include(remote_boost.id)
+          expect(subject).to_not include(local_boost.id)
+        end
+      end
     end
 
     it 'filters out silenced accounts' do