Fix thread depth computation in statuses_controller (#9426)

* Add test that should currently fail

* Fix depth computation (will still fail if statuses have been filtered out)

* Fix handling of broken threads
This commit is contained in:
ThibG 2018-12-05 02:12:29 +01:00 committed by Eugen Rochko
parent a61ce1c947
commit e88c6a5c3c
2 changed files with 21 additions and 15 deletions

View file

@ -65,12 +65,13 @@ class StatusesController < ApplicationController
private private
def create_descendant_thread(depth, statuses) def create_descendant_thread(starting_depth, statuses)
depth = starting_depth + statuses.size
if depth < DESCENDANTS_DEPTH_LIMIT if depth < DESCENDANTS_DEPTH_LIMIT
{ statuses: statuses } { statuses: statuses, starting_depth: starting_depth }
else else
next_status = statuses.pop next_status = statuses.pop
{ statuses: statuses, next_status: next_status } { statuses: statuses, starting_depth: starting_depth, next_status: next_status }
end end
end end
@ -102,15 +103,18 @@ class StatusesController < ApplicationController
if descendants.present? if descendants.present?
statuses = [descendants.first] statuses = [descendants.first]
depth = 1 starting_depth = 0
descendants.drop(1).each_with_index do |descendant, index| descendants.drop(1).each_with_index do |descendant, index|
if descendants[index].id == descendant.in_reply_to_id if descendants[index].id == descendant.in_reply_to_id
depth += 1
statuses << descendant statuses << descendant
else else
@descendant_threads << create_descendant_thread(depth, statuses) @descendant_threads << create_descendant_thread(starting_depth, statuses)
# The thread is broken, assume it's a reply to the root status
starting_depth = 0
# ... unless we can find its ancestor in one of the already-processed threads
@descendant_threads.reverse_each do |descendant_thread| @descendant_threads.reverse_each do |descendant_thread|
statuses = descendant_thread[:statuses] statuses = descendant_thread[:statuses]
@ -119,18 +123,16 @@ class StatusesController < ApplicationController
end end
if index.present? if index.present?
depth += index - statuses.size starting_depth = descendant_thread[:starting_depth] + index + 1
break break
end end
depth -= statuses.size
end end
statuses = [descendant] statuses = [descendant]
end end
end end
@descendant_threads << create_descendant_thread(depth, statuses) @descendant_threads << create_descendant_thread(starting_depth, statuses)
end end
@max_descendant_thread_id = @descendant_threads.pop[:statuses].first.id if descendants.size >= DESCENDANTS_LIMIT @max_descendant_thread_id = @descendant_threads.pop[:statuses].first.id if descendants.size >= DESCENDANTS_LIMIT

View file

@ -115,14 +115,18 @@ describe StatusesController do
end end
it 'assigns @descendant_threads for threads with :next_status key if they are hitting the depth limit' do it 'assigns @descendant_threads for threads with :next_status key if they are hitting the depth limit' do
stub_const 'StatusesController::DESCENDANTS_DEPTH_LIMIT', 1 stub_const 'StatusesController::DESCENDANTS_DEPTH_LIMIT', 2
status = Fabricate(:status) status = Fabricate(:status)
child = Fabricate(:status, in_reply_to_id: status.id) child0 = Fabricate(:status, in_reply_to_id: status.id)
child1 = Fabricate(:status, in_reply_to_id: child0.id)
child2 = Fabricate(:status, in_reply_to_id: child0.id)
get :show, params: { account_username: status.account.username, id: status.id } get :show, params: { account_username: status.account.username, id: status.id }
expect(assigns(:descendant_threads)[0][:statuses].pluck(:id)).not_to include child.id expect(assigns(:descendant_threads)[0][:statuses].pluck(:id)).not_to include child1.id
expect(assigns(:descendant_threads)[0][:next_status].id).to eq child.id expect(assigns(:descendant_threads)[1][:statuses].pluck(:id)).not_to include child2.id
expect(assigns(:descendant_threads)[0][:next_status].id).to eq child1.id
expect(assigns(:descendant_threads)[1][:next_status].id).to eq child2.id
end end
it 'returns a success' do it 'returns a success' do