mirror of
https://github.com/discourse/discourse-solved.git
synced 2025-05-02 04:04:53 +00:00
FIX: Multiple topics may have the same post as its solution
This commit is contained in:
Nat
parent
a8a3554cec
commit
4bb2c7651f
@ -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
|
||||
|
||||
|
||||
@ -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
|
||||
@ -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
|
||||
|
||||
|
||||
44
spec/models/copy_solved_topic_custom_field_spec.rb
Normal file
44
spec/models/copy_solved_topic_custom_field_spec.rb
Normal file
@ -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
|
||||
@ -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
|
||||
Loading…
x
Reference in New Issue
Block a user