From 4bb2c7651ff0ac75b3e4911a80b1d7ebf4e55239 Mon Sep 17 00:00:00 2001 From: Nat Date: Wed, 26 Mar 2025 21:55:06 +0800 Subject: [PATCH] FIX: Multiple topics may have the same post as its solution --- ...field_to_discourse_solved_solved_topics.rb | 36 ++++++++----- ...tes_from_discourse_solved_solved_topics.rb | 38 ++++++++++++++ ...field_to_discourse_solved_solved_topics.rb | 36 ++++++++----- .../copy_solved_topic_custom_field_spec.rb | 44 ++++++++++++++++ ...rom_discourse_solved_solved_topics_spec.rb | 52 +++++++++++++++++++ 5 files changed, 178 insertions(+), 28 deletions(-) create mode 100644 db/migrate/20250318024954_remove_duplicates_from_discourse_solved_solved_topics.rb create mode 100644 spec/models/copy_solved_topic_custom_field_spec.rb create mode 100644 spec/models/remove_duplicates_from_discourse_solved_solved_topics_spec.rb diff --git a/db/migrate/20250318024953_copy_solved_topic_custom_field_to_discourse_solved_solved_topics.rb b/db/migrate/20250318024953_copy_solved_topic_custom_field_to_discourse_solved_solved_topics.rb index a76d2b2..17387dc 100644 --- a/db/migrate/20250318024953_copy_solved_topic_custom_field_to_discourse_solved_solved_topics.rb +++ b/db/migrate/20250318024953_copy_solved_topic_custom_field_to_discourse_solved_solved_topics.rb @@ -23,23 +23,31 @@ class CopySolvedTopicCustomFieldToDiscourseSolvedSolvedTopics < ActiveRecord::Mi created_at, updated_at ) - SELECT DISTINCT ON (tc.topic_id) + SELECT tc.topic_id, - CAST(tc.value AS INTEGER), - CAST(tc2.value AS INTEGER), - COALESCE(ua.acting_user_id, -1), + tc.answer_post_id, + tc.topic_timer_id, + tc.accepter_user_id, tc.created_at, tc.updated_at - FROM topic_custom_fields tc - LEFT JOIN topic_custom_fields tc2 - ON tc2.topic_id = tc.topic_id - AND tc2.name = 'solved_auto_close_topic_timer_id' - LEFT JOIN user_actions ua - ON ua.target_topic_id = tc.topic_id - AND ua.action_type = 15 - WHERE tc.name = 'accepted_answer_post_id' - AND tc.id > :last_id - AND tc.id <= :last_id + :batch_size + FROM ( + SELECT + tc.topic_id, + CAST(tc.value AS INTEGER) AS answer_post_id, + CAST(tc2.value AS INTEGER) AS topic_timer_id, + COALESCE(ua.acting_user_id, -1) AS accepter_user_id, + tc.created_at, + tc.updated_at, + ROW_NUMBER() OVER (PARTITION BY tc.topic_id ORDER BY tc.created_at ASC) AS rn_topic, + ROW_NUMBER() OVER (PARTITION BY CAST(tc.value AS INTEGER) ORDER BY tc.created_at ASC) AS rn_answer + FROM topic_custom_fields tc + LEFT JOIN topic_custom_fields tc2 ON tc2.topic_id = tc.topic_id AND tc2.name = 'solved_auto_close_topic_timer_id' + LEFT JOIN user_actions ua ON ua.target_topic_id = tc.topic_id AND ua.action_type = 15 + WHERE tc.name = 'accepted_answer_post_id' + AND tc.id > :last_id + AND tc.id <= :last_id + :batch_size + ) tc + WHERE tc.rn_topic = 1 AND tc.rn_answer = 1 ON CONFLICT DO NOTHING SQL diff --git a/db/migrate/20250318024954_remove_duplicates_from_discourse_solved_solved_topics.rb b/db/migrate/20250318024954_remove_duplicates_from_discourse_solved_solved_topics.rb new file mode 100644 index 0000000..c7b61af --- /dev/null +++ b/db/migrate/20250318024954_remove_duplicates_from_discourse_solved_solved_topics.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +class RemoveDuplicatesFromDiscourseSolvedSolvedTopics < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + def up + puts "before" + puts DiscourseSolved::SolvedTopic.count + # remove duplicates on answer_post_id based on earliest created_at + DB.exec(<<~SQL) + DELETE FROM discourse_solved_solved_topics + WHERE id NOT IN ( + SELECT id FROM ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY answer_post_id ORDER BY created_at) as row_num + FROM discourse_solved_solved_topics + ) t WHERE row_num = 1 + ) + SQL + + # remove duplicates on topic_id based on earliest created_at + DB.exec(<<~SQL) + DELETE FROM discourse_solved_solved_topics + WHERE id NOT IN ( + SELECT id FROM ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY topic_id ORDER BY created_at) as row_num + FROM discourse_solved_solved_topics + ) t WHERE row_num = 1 + ) + SQL + + puts "done" + puts DiscourseSolved::SolvedTopic.count + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20250325074111_copy_remaining_solved_topic_custom_field_to_discourse_solved_solved_topics.rb b/db/migrate/20250325074111_copy_remaining_solved_topic_custom_field_to_discourse_solved_solved_topics.rb index 25adfab..152c26c 100644 --- a/db/migrate/20250325074111_copy_remaining_solved_topic_custom_field_to_discourse_solved_solved_topics.rb +++ b/db/migrate/20250325074111_copy_remaining_solved_topic_custom_field_to_discourse_solved_solved_topics.rb @@ -25,23 +25,31 @@ class CopyRemainingSolvedTopicCustomFieldToDiscourseSolvedSolvedTopics < ActiveR created_at, updated_at ) - SELECT DISTINCT ON (tc.topic_id) + SELECT tc.topic_id, - CAST(tc.value AS INTEGER), - CAST(tc2.value AS INTEGER), - COALESCE(ua.acting_user_id, -1), + tc.answer_post_id, + tc.topic_timer_id, + tc.accepter_user_id, tc.created_at, tc.updated_at - FROM topic_custom_fields tc - LEFT JOIN topic_custom_fields tc2 - ON tc2.topic_id = tc.topic_id - AND tc2.name = 'solved_auto_close_topic_timer_id' - LEFT JOIN user_actions ua - ON ua.target_topic_id = tc.topic_id - AND ua.action_type = 15 - WHERE tc.name = 'accepted_answer_post_id' - AND tc.id > :last_id - AND tc.id <= :last_id + :batch_size + FROM ( + SELECT + tc.topic_id, + CAST(tc.value AS INTEGER) AS answer_post_id, + CAST(tc2.value AS INTEGER) AS topic_timer_id, + COALESCE(ua.acting_user_id, -1) AS accepter_user_id, + tc.created_at, + tc.updated_at, + ROW_NUMBER() OVER (PARTITION BY tc.topic_id ORDER BY tc.created_at ASC) AS rn_topic, + ROW_NUMBER() OVER (PARTITION BY CAST(tc.value AS INTEGER) ORDER BY tc.created_at ASC) AS rn_answer + FROM topic_custom_fields tc + LEFT JOIN topic_custom_fields tc2 ON tc2.topic_id = tc.topic_id AND tc2.name = 'solved_auto_close_topic_timer_id' + LEFT JOIN user_actions ua ON ua.target_topic_id = tc.topic_id AND ua.action_type = 15 + WHERE tc.name = 'accepted_answer_post_id' + AND tc.id > :last_id + AND tc.id <= :last_id + :batch_size + ) tc + WHERE tc.rn_topic = 1 AND tc.rn_answer = 1 ON CONFLICT DO NOTHING SQL diff --git a/spec/models/copy_solved_topic_custom_field_spec.rb b/spec/models/copy_solved_topic_custom_field_spec.rb new file mode 100644 index 0000000..f67bd30 --- /dev/null +++ b/spec/models/copy_solved_topic_custom_field_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require_relative "../../db/migrate/20250318024953_copy_solved_topic_custom_field_to_discourse_solved_solved_topics" + +RSpec.describe CopySolvedTopicCustomFieldToDiscourseSolvedSolvedTopics, type: :migration do + let(:migration) { described_class.new } + + describe "handling duplicates" do + it "ensures only unique topic_id and answer_post_id are inserted" do + topic = Fabricate(:topic) + post1 = Fabricate(:post, topic: topic) + Fabricate( + :topic_custom_field, + topic: topic, + name: "accepted_answer_post_id", + value: post1.id.to_s, + ) + # explicit duplicate + Fabricate( + :topic_custom_field, + topic: topic, + name: "accepted_answer_post_id", + value: post1.id.to_s, + ) + + second_topic = Fabricate(:topic) + post2 = Fabricate(:post, topic: second_topic) + Fabricate( + :topic_custom_field, + topic: second_topic, + name: "accepted_answer_post_id", + value: post2.id.to_s, + ) + + migration.up + expected_count = DiscourseSolved::SolvedTopic.count + + expect(expected_count).to eq(2) + + expect(DiscourseSolved::SolvedTopic.where(topic_id: topic.id).count).to eq(1) + expect(DiscourseSolved::SolvedTopic.where(answer_post_id: post1.id).count).to eq(1) + end + end +end diff --git a/spec/models/remove_duplicates_from_discourse_solved_solved_topics_spec.rb b/spec/models/remove_duplicates_from_discourse_solved_solved_topics_spec.rb new file mode 100644 index 0000000..3df83b6 --- /dev/null +++ b/spec/models/remove_duplicates_from_discourse_solved_solved_topics_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require_relative "../../db/migrate/20250318024954_remove_duplicates_from_discourse_solved_solved_topics" + +RSpec.describe RemoveDuplicatesFromDiscourseSolvedSolvedTopics, type: :migration do + let(:migration) { described_class.new } + + before do + # temp drop unique constraints to allow testing duplicate entries + ActiveRecord::Base.connection.execute( + "DROP INDEX IF EXISTS index_discourse_solved_solved_topics_on_topic_id;", + ) + ActiveRecord::Base.connection.execute( + "DROP INDEX IF EXISTS index_discourse_solved_solved_topics_on_answer_post_id;", + ) + end + + after do + DiscourseSolved::SolvedTopic.delete_all + + # reapply unique indexes + ActiveRecord::Base.connection.execute( + "CREATE UNIQUE INDEX index_discourse_solved_solved_topics_on_topic_id ON discourse_solved_solved_topics (topic_id);", + ) + ActiveRecord::Base.connection.execute( + "CREATE UNIQUE INDEX index_discourse_solved_solved_topics_on_answer_post_id ON discourse_solved_solved_topics (answer_post_id);", + ) + end + + describe "removal of duplicate answer_post_ids" do + it "keeps only the earliest record for each answer_post_id" do + topic1 = Fabricate(:topic) + post1 = Fabricate(:post, topic: topic1) + topic2 = Fabricate(:topic) + post2 = Fabricate(:post, topic: topic2) + + earlier = Fabricate(:solved_topic, topic: topic1, answer_post: post1, created_at: 2.days.ago) + Fabricate(:solved_topic, topic: topic1, answer_post: post1, created_at: 1.day.ago) + Fabricate(:solved_topic, topic: topic1, answer_post: post1, created_at: Date.today) + another = Fabricate(:solved_topic, topic: topic2, answer_post: post2, created_at: Date.today) + + expect(DiscourseSolved::SolvedTopic.count).to eq(4) + migration.up + + expect(DiscourseSolved::SolvedTopic.count).to eq(2) + expect(DiscourseSolved::SolvedTopic.pluck(:id, :answer_post_id)).to contain_exactly( + [earlier.id, post1.id], + [another.id, post2.id], + ) + end + end +end