Fix boosting & unboosting preventing a boost from appearing in the TL (#11405)

* Fix boosting & unboosting preventing a boost from appearing in the TL

* Add tests

* Avoids side effects when aggregate_reblogs isn't true
This commit is contained in:
ThibG 2019-07-30 13:18:23 +02:00 committed by Eugen Rochko
parent 648cdbc04a
commit ff789a751a
2 changed files with 26 additions and 5 deletions

View File

@ -35,7 +35,7 @@ class FeedManager
end end
def unpush_from_home(account, status) def unpush_from_home(account, status)
return false unless remove_from_feed(:home, account.id, status) return false unless remove_from_feed(:home, account.id, status, account.user&.aggregates_reblogs?)
redis.publish("timeline:#{account.id}", Oj.dump(event: :delete, payload: status.id.to_s)) redis.publish("timeline:#{account.id}", Oj.dump(event: :delete, payload: status.id.to_s))
true true
end end
@ -53,7 +53,7 @@ class FeedManager
end end
def unpush_from_list(list, status) def unpush_from_list(list, status)
return false unless remove_from_feed(:list, list.id, status) return false unless remove_from_feed(:list, list.id, status, list.account.user&.aggregates_reblogs?)
redis.publish("timeline:list:#{list.id}", Oj.dump(event: :delete, payload: status.id.to_s)) redis.publish("timeline:list:#{list.id}", Oj.dump(event: :delete, payload: status.id.to_s))
true true
end end
@ -105,7 +105,7 @@ class FeedManager
oldest_home_score = redis.zrange(timeline_key, 0, 0, with_scores: true)&.first&.last&.to_i || 0 oldest_home_score = redis.zrange(timeline_key, 0, 0, with_scores: true)&.first&.last&.to_i || 0
from_account.statuses.select('id, reblog_of_id').where('id > ?', oldest_home_score).reorder(nil).find_each do |status| from_account.statuses.select('id, reblog_of_id').where('id > ?', oldest_home_score).reorder(nil).find_each do |status|
remove_from_feed(:home, into_account.id, status) remove_from_feed(:home, into_account.id, status, into_account.user&.aggregates_reblogs?)
end end
end end
@ -275,10 +275,11 @@ class FeedManager
# with reblogs, and returning true if a status was removed. As with # with reblogs, and returning true if a status was removed. As with
# `add_to_feed`, this does not trigger push updates, so callers must # `add_to_feed`, this does not trigger push updates, so callers must
# do so if appropriate. # do so if appropriate.
def remove_from_feed(timeline_type, account_id, status) def remove_from_feed(timeline_type, account_id, status, aggregate_reblogs = true)
timeline_key = key(timeline_type, account_id) timeline_key = key(timeline_type, account_id)
reblog_key = key(timeline_type, account_id, 'reblogs')
if status.reblog? if status.reblog? && (aggregate_reblogs.nil? || aggregate_reblogs)
# 1. If the reblogging status is not in the feed, stop. # 1. If the reblogging status is not in the feed, stop.
status_rank = redis.zrevrank(timeline_key, status.id) status_rank = redis.zrevrank(timeline_key, status.id)
return false if status_rank.nil? return false if status_rank.nil?
@ -287,6 +288,7 @@ class FeedManager
reblog_set_key = key(timeline_type, account_id, "reblogs:#{status.reblog_of_id}") reblog_set_key = key(timeline_type, account_id, "reblogs:#{status.reblog_of_id}")
redis.srem(reblog_set_key, status.id) redis.srem(reblog_set_key, status.id)
redis.zrem(reblog_key, status.reblog_of_id)
# 3. Re-insert another reblog or original into the feed if one # 3. Re-insert another reblog or original into the feed if one
# remains in the set. We could pick a random element, but this # remains in the set. We could pick a random element, but this
# set should generally be small, and it seems ideal to show the # set should generally be small, and it seems ideal to show the
@ -294,12 +296,14 @@ class FeedManager
other_reblog = redis.smembers(reblog_set_key).map(&:to_i).min other_reblog = redis.smembers(reblog_set_key).map(&:to_i).min
redis.zadd(timeline_key, other_reblog, other_reblog) if other_reblog redis.zadd(timeline_key, other_reblog, other_reblog) if other_reblog
redis.zadd(reblog_key, other_reblog, status.reblog_of_id) if other_reblog
# 4. Remove the reblogging status from the feed (as normal) # 4. Remove the reblogging status from the feed (as normal)
# (outside conditional) # (outside conditional)
else else
# If the original is getting deleted, no use for reblog references # If the original is getting deleted, no use for reblog references
redis.del(key(timeline_type, account_id, "reblogs:#{status.id}")) redis.del(key(timeline_type, account_id, "reblogs:#{status.id}"))
redis.srem(reblog_key, status.id)
end end
redis.zrem(timeline_key, status.id) redis.zrem(timeline_key, status.id)

View File

@ -247,6 +247,23 @@ RSpec.describe FeedManager do
expect(FeedManager.instance.push_to_home(account, reblogs.last)).to be false expect(FeedManager.instance.push_to_home(account, reblogs.last)).to be false
end end
it 'saves a new reblog of a recently-reblogged status when previous reblog has been deleted' do
account = Fabricate(:account)
reblogged = Fabricate(:status)
old_reblog = Fabricate(:status, reblog: reblogged)
# The first reblog should be accepted
expect(FeedManager.instance.push_to_home(account, old_reblog)).to be true
# The first reblog should be successfully removed
expect(FeedManager.instance.unpush_from_home(account, old_reblog)).to be true
reblog = Fabricate(:status, reblog: reblogged)
# The second reblog should be accepted
expect(FeedManager.instance.push_to_home(account, reblog)).to be true
end
it 'does not save a new reblog of a multiply-reblogged-then-unreblogged status' do it 'does not save a new reblog of a multiply-reblogged-then-unreblogged status' do
account = Fabricate(:account) account = Fabricate(:account)
reblogged = Fabricate(:status) reblogged = Fabricate(:status)