From 554c2fd8af79c173e81d7193ea649fa848076123 Mon Sep 17 00:00:00 2001 From: aschmitz Date: Tue, 17 Oct 2017 04:45:06 -0500 Subject: Clean up reblog tracking keys, related improvements (#5428) * Clean up reblog-tracking sets from FeedManager Builds on #5419, with a few minor optimizations and cleanup of sets after they are no longer needed. * Update tests, fix multiply-reblogged case Previously, we would have lost the fact that a given status was reblogged if the displayed reblog of it was removed, now we don't. Also added tests to make sure FeedManager#trim cleans up our reblog tracking keys, fixed up FeedCleanupScheduler to use the right loop, and fixed the test for it. --- spec/lib/feed_manager_spec.rb | 68 +++++++++++++++++++--- .../scheduler/feed_cleanup_scheduler_spec.rb | 8 ++- 2 files changed, 65 insertions(+), 11 deletions(-) (limited to 'spec') diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb index 454c3afec..0f97a579e 100644 --- a/spec/lib/feed_manager_spec.rb +++ b/spec/lib/feed_manager_spec.rb @@ -211,6 +211,22 @@ RSpec.describe FeedManager do expect(FeedManager.instance.push('type', account, reblogs.last)).to be false end + it 'does not save a new reblog of a multiply-reblogged-then-unreblogged status' do + account = Fabricate(:account) + reblogged = Fabricate(:status) + reblogs = 3.times.map { Fabricate(:status, reblog: reblogged) } + + # Accept the reblogs + FeedManager.instance.push('type', account, reblogs[0]) + FeedManager.instance.push('type', account, reblogs[1]) + + # Unreblog the first one + FeedManager.instance.unpush('type', account, reblogs[0]) + + # The last reblog should still be ignored + expect(FeedManager.instance.push('type', account, reblogs.last)).to be false + end + it 'saves a new reblog of a long-ago-reblogged status' do account = Fabricate(:account) reblogged = Fabricate(:status) @@ -230,6 +246,38 @@ RSpec.describe FeedManager do end end + describe '#trim' do + let(:receiver) { Fabricate(:account) } + + it 'cleans up reblog tracking keys' do + reblogged = Fabricate(:status) + status = Fabricate(:status, reblog: reblogged) + another_status = Fabricate(:status, reblog: reblogged) + reblogs_key = FeedManager.instance.key('type', receiver.id, 'reblogs') + reblog_set_key = FeedManager.instance.key('type', receiver.id, "reblogs:#{reblogged.id}") + + FeedManager.instance.push('type', receiver, status) + FeedManager.instance.push('type', receiver, another_status) + + # We should have a tracking set and an entry in reblogs. + expect(Redis.current.exists(reblog_set_key)).to be true + expect(Redis.current.zrange(reblogs_key, 0, -1)).to eq [reblogged.id.to_s] + + # Push everything off the end of the feed. + FeedManager::MAX_ITEMS.times do + FeedManager.instance.push('type', receiver, Fabricate(:status)) + end + + # `trim` should be called automatically, but do it anyway, as + # we're testing `trim`, not side effects of `push`. + FeedManager.instance.trim('type', receiver.id) + + # We should not have any reblog tracking data. + expect(Redis.current.exists(reblog_set_key)).to be false + expect(Redis.current.zrange(reblogs_key, 0, -1)).to be_empty + end + end + describe '#unpush' do let(:receiver) { Fabricate(:account) } @@ -265,20 +313,22 @@ RSpec.describe FeedManager do expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to be_empty end - it 'leaves a reblogged status if another reblog was in feed' do - reblogged = Fabricate(:status) - status = Fabricate(:status, reblog: reblogged) - another_status = Fabricate(:status, reblog: reblogged) + it 'leaves a multiply-reblogged status if another reblog was in feed' do + reblogged = Fabricate(:status) + reblogs = 3.times.map { Fabricate(:status, reblog: reblogged) } - FeedManager.instance.push('type', receiver, status) - FeedManager.instance.push('type', receiver, another_status) + reblogs.each do |reblog| + FeedManager.instance.push('type', receiver, reblog) + end # The reblogging status should show up under normal conditions. - expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [status.id.to_s] + expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [reblogs.first.id.to_s] - FeedManager.instance.unpush('type', receiver, status) + reblogs[0...-1].each do |reblog| + FeedManager.instance.unpush('type', receiver, reblog) + end - expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [another_status.id.to_s] + expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [reblogs.last.id.to_s] end it 'sends push updates' do diff --git a/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb b/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb index b8487b03f..7fae680ba 100644 --- a/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb +++ b/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb @@ -9,14 +9,18 @@ describe Scheduler::FeedCleanupScheduler do it 'clears feeds of inactives' do Redis.current.zadd(feed_key_for(inactive_user), 1, 1) Redis.current.zadd(feed_key_for(active_user), 1, 1) + Redis.current.zadd(feed_key_for(inactive_user, 'reblogs'), 2, 2) + Redis.current.sadd(feed_key_for(inactive_user, 'reblogs:2'), 3) subject.perform expect(Redis.current.zcard(feed_key_for(inactive_user))).to eq 0 expect(Redis.current.zcard(feed_key_for(active_user))).to eq 1 + expect(Redis.current.exists(feed_key_for(inactive_user, 'reblogs'))).to be false + expect(Redis.current.exists(feed_key_for(inactive_user, 'reblogs:2'))).to be false end - def feed_key_for(user) - FeedManager.instance.key(:home, user.account_id) + def feed_key_for(user, subtype = nil) + FeedManager.instance.key(:home, user.account_id, subtype) end end -- cgit