From e88c6a5c3c188731f7784141b8835c410163cbeb Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 5 Dec 2018 02:12:29 +0100 Subject: [PATCH] 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 --- app/controllers/statuses_controller.rb | 24 +++++++++++--------- spec/controllers/statuses_controller_spec.rb | 12 ++++++---- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index 0f3fe198f4..15d59fd893 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -65,12 +65,13 @@ class StatusesController < ApplicationController private - def create_descendant_thread(depth, statuses) + def create_descendant_thread(starting_depth, statuses) + depth = starting_depth + statuses.size if depth < DESCENDANTS_DEPTH_LIMIT - { statuses: statuses } + { statuses: statuses, starting_depth: starting_depth } else next_status = statuses.pop - { statuses: statuses, next_status: next_status } + { statuses: statuses, starting_depth: starting_depth, next_status: next_status } end end @@ -101,16 +102,19 @@ class StatusesController < ApplicationController @descendant_threads = [] if descendants.present? - statuses = [descendants.first] - depth = 1 + statuses = [descendants.first] + starting_depth = 0 descendants.drop(1).each_with_index do |descendant, index| if descendants[index].id == descendant.in_reply_to_id - depth += 1 statuses << descendant 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| statuses = descendant_thread[:statuses] @@ -119,18 +123,16 @@ class StatusesController < ApplicationController end if index.present? - depth += index - statuses.size + starting_depth = descendant_thread[:starting_depth] + index + 1 break end - - depth -= statuses.size end statuses = [descendant] end end - @descendant_threads << create_descendant_thread(depth, statuses) + @descendant_threads << create_descendant_thread(starting_depth, statuses) end @max_descendant_thread_id = @descendant_threads.pop[:statuses].first.id if descendants.size >= DESCENDANTS_LIMIT diff --git a/spec/controllers/statuses_controller_spec.rb b/spec/controllers/statuses_controller_spec.rb index b4f3c5a081..1bb6636c60 100644 --- a/spec/controllers/statuses_controller_spec.rb +++ b/spec/controllers/statuses_controller_spec.rb @@ -115,14 +115,18 @@ describe StatusesController do end 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) - 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 } - expect(assigns(:descendant_threads)[0][:statuses].pluck(:id)).not_to include child.id - expect(assigns(:descendant_threads)[0][:next_status].id).to eq child.id + expect(assigns(:descendant_threads)[0][:statuses].pluck(:id)).not_to include child1.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 it 'returns a success' do