Skip to content

Commit

Permalink
Fix boosting & unboosting preventing a boost from appearing in the TL (
Browse files Browse the repository at this point in the history
…mastodon#11405)

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

* Add tests

* Avoids side effects when aggregate_reblogs isn't true
  • Loading branch information
ClearlyClaire authored and Gargron committed Jul 30, 2019
1 parent 648cdbc commit ff789a7
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 5 deletions.
14 changes: 9 additions & 5 deletions app/lib/feed_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def push_to_home(account, status)
end

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))
true
end
Expand All @@ -53,7 +53,7 @@ def push_to_list(list, status)
end

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))
true
end
Expand Down Expand Up @@ -105,7 +105,7 @@ def unmerge_from_timeline(from_account, into_account)
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|
remove_from_feed(:home, into_account.id, status)
remove_from_feed(:home, into_account.id, status, into_account.user&.aggregates_reblogs?)
end
end

Expand Down Expand Up @@ -275,10 +275,11 @@ def add_to_feed(timeline_type, account_id, status, aggregate_reblogs = true)
# with reblogs, and returning true if a status was removed. As with
# `add_to_feed`, this does not trigger push updates, so callers must
# 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)
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.
status_rank = redis.zrevrank(timeline_key, status.id)
return false if status_rank.nil?
Expand All @@ -287,19 +288,22 @@ def remove_from_feed(timeline_type, account_id, status)
reblog_set_key = key(timeline_type, account_id, "reblogs:#{status.reblog_of_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
# remains in the set. We could pick a random element, but this
# set should generally be small, and it seems ideal to show the
# oldest potential such reblog.
other_reblog = redis.smembers(reblog_set_key).map(&:to_i).min

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)
# (outside conditional)
else
# If the original is getting deleted, no use for reblog references
redis.del(key(timeline_type, account_id, "reblogs:#{status.id}"))
redis.srem(reblog_key, status.id)
end

redis.zrem(timeline_key, status.id)
Expand Down
17 changes: 17 additions & 0 deletions spec/lib/feed_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,23 @@
expect(FeedManager.instance.push_to_home(account, reblogs.last)).to be false
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
account = Fabricate(:account)
reblogged = Fabricate(:status)
Expand Down

0 comments on commit ff789a7

Please sign in to comment.