Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: Move the post and topic custom fields into a table #309

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
5 changes: 1 addition & 4 deletions app/lib/before_head_close.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,7 @@ def html
},
}

if accepted_answer =
Post.find_by(
id: topic.custom_fields[::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD],
)
if accepted_answer = topic.solution&.post
question_json["answerCount"] = 1
question_json[:acceptedAnswer] = {
"@type" => "Answer",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,7 @@ class AssignsReminderForTopicsQuery < PluginInitializer
def apply_plugin_api
plugin.register_modifier(:assigns_reminder_assigned_topics_query) do |query|
next query if !SiteSetting.ignore_solved_topics_in_assigned_reminder
query.where.not(
id:
TopicCustomField.where(
name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD,
).pluck(:topic_id),
)
query.where.not(id: DiscourseSolved::Solution.pluck(:topic_id))
Lhcfl marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
Expand Down
10 changes: 10 additions & 0 deletions app/models/discourse-solved/solution.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

module ::DiscourseSolved
class Solution < ActiveRecord::Base
belongs_to :accepter, foreign_key: :accepter_user_id, class_name: :user
belongs_to :post, foreign_key: :answer_post_id
belongs_to :topic
belongs_to :topic_timer, dependent: :destroy
end
end
24 changes: 11 additions & 13 deletions db/fixtures/001_badges.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@
first_solution_query = <<~SQL
SELECT post_id, user_id, created_at AS granted_at
FROM (
SELECT p.id AS post_id, p.user_id, pcf.created_at,
ROW_NUMBER() OVER (PARTITION BY p.user_id ORDER BY pcf.created_at) AS row_number
FROM post_custom_fields pcf
JOIN badge_posts p ON pcf.post_id = p.id
JOIN topics t ON p.topic_id = t.id
WHERE pcf.name = 'is_accepted_answer'
AND p.user_id <> t.user_id -- ignore topics solved by OP
AND (:backfill OR p.id IN (:post_ids))
SELECT p.id AS post_id, p.user_id, dss.created_at,
ROW_NUMBER() OVER (PARTITION BY p.user_id ORDER BY dss.created_at) AS row_number
FROM discourse_solved_solutions dss
JOIN badge_posts p ON dss.answer_post_id = p.id
JOIN topics t ON p.topic_id = t.id
WHERE p.user_id <> t.user_id -- ignore topics solved by OP
AND (:backfill OR p.id IN (:post_ids))
) x
WHERE row_number = 1
SQL
Expand All @@ -32,12 +31,11 @@

def solved_query_with_count(min_count)
<<~SQL
SELECT p.user_id, MAX(pcf.created_at) AS granted_at
FROM post_custom_fields pcf
JOIN badge_posts p ON pcf.post_id = p.id
SELECT p.user_id, MAX(dss.created_at) AS granted_at
FROM discourse_solved_solutions dss
JOIN badge_posts p ON dss.answer_post_id = p.id
JOIN topics t ON p.topic_id = t.id
WHERE pcf.name = 'is_accepted_answer'
AND p.user_id <> t.user_id -- ignore topics solved by OP
WHERE p.user_id <> t.user_id -- ignore topics solved by OP
AND (:backfill OR p.id IN (:post_ids))
GROUP BY p.user_id
HAVING COUNT(*) >= #{min_count}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# frozen_string_literal: true
class MoveSolvedTopicCustomFieldToDiscourseSolvedSolutions < ActiveRecord::Migration[7.1]
Lhcfl marked this conversation as resolved.
Show resolved Hide resolved
def up
create_table :discourse_solved_solutions do |t|
t.integer :topic_id, null: false
t.integer :answer_post_id, null: false
t.integer :accepter_user_id, null: true
Lhcfl marked this conversation as resolved.
Show resolved Hide resolved
t.integer :topic_timer_id, null: true
Lhcfl marked this conversation as resolved.
Show resolved Hide resolved
t.timestamps
end

add_index :discourse_solved_solutions, :topic_id, unique: true
add_index :discourse_solved_solutions, :answer_post_id, unique: true
Lhcfl marked this conversation as resolved.
Show resolved Hide resolved

execute <<-SQL
INSERT INTO discourse_solved_solutions (
topic_id,
answer_post_id,
topic_timer_id,
accepter_user_id,
created_at,
updated_at
) SELECT DISTINCT
tc.topic_id,
CAST(tc.value AS INTEGER),
CAST(tc2.value AS INTEGER),
ua.acting_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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an index in user_actions for target_topic_id?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query plan is:

                                                          QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------
 Unique  (cost=14.85..14.87 rows=1 width=32)
   ->  Sort  (cost=14.85..14.86 rows=1 width=32)
         Sort Key: tc.topic_id, ((tc.value)::integer), ((tc2.value)::integer), ua.acting_user_id, tc.created_at, tc.updated_at
         ->  Nested Loop  (cost=0.42..14.84 rows=1 width=32)
               ->  Nested Loop Left Join  (cost=0.00..6.16 rows=1 width=84)
                     Join Filter: (tc2.topic_id = tc.topic_id)
                     ->  Seq Scan on topic_custom_fields tc  (cost=0.00..3.08 rows=1 width=52)
                           Filter: ((name)::text = 'accepted_answer_post_id'::text)
                     ->  Seq Scan on topic_custom_fields tc2  (cost=0.00..3.08 rows=1 width=36)
                           Filter: ((name)::text = 'solved_auto_close_topic_timer_id'::text)
               ->  Index Only Scan using idx_unique_rows on user_actions ua  (cost=0.42..8.66 rows=1 width=8)
                     Index Cond: ((action_type = 15) AND (target_topic_id = tc.topic_id))
(12 rows)

The query is using idx_unique_rows on user_actions ua, so i think yeah, it is using an index

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> Seq Scan on topic_custom_fields tc (cost=0.00..3.08 rows=1 width=52) Filter: ((name)::text = 'accepted_answer_post_id'::text)

This looks interesting to me. I wonder if we need a index on name. From my rusty knowledge Seq Scan can be costly if the table is large and if there are no indexes to filter on (e.g. the accepted_answer_post_id)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just noticed that custom_fields does not an index on name... 😢

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it interesting that we have no index on topic_custom_fields.name. Wondering if it makes sense to have one added in core.

We have this one:

#  topic_custom_fields_value_key_idx                             (value,name) WHERE ((value IS NOT NULL) AND (char_length(value) < 400))

Which I find a bit strange, why not (name, value)?

ON ua.target_topic_id = tc.topic_id
WHERE tc.name = 'accepted_answer_post_id'
AND ua.action_type = #{UserAction::SOLVED}
SQL

execute <<-SQL
DELETE FROM post_custom_fields
WHERE name = 'is_accepted_answer'
SQL

execute <<-SQL
DELETE FROM topic_custom_fields
WHERE name = 'solved_auto_close_topic_timer_id'
SQL
Comment on lines +41 to +49
Copy link
Contributor

@nattsw nattsw Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the deletions here are ok though..

... if we have better knowledge or data, and we know that there are many rows to be deleted, we can consider deletion in batches. Let me do a quick check on who may be a heavy user of discourse-solved...

Copy link
Contributor

@nattsw nattsw Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ok looks like we do have some hundreds of thousands of rows (as mentioned in chat) so perhaps nice to batch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a trick to make it a bit better:

      DELETE FROM topic_custom_fields
      WHERE id IN (SELECT id FROM topic_custom_fields WHERE name = 'solved_auto_close_topic_timer_id')

... but it needs to be tested.

end

def down
raise ActiveRecord::IrreversibleMigration
end
end
78 changes: 34 additions & 44 deletions plugin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@
after_initialize do
module ::DiscourseSolved
PLUGIN_NAME = "discourse-solved"
AUTO_CLOSE_TOPIC_TIMER_CUSTOM_FIELD = "solved_auto_close_topic_timer_id"
ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD = "accepted_answer_post_id"
ENABLE_ACCEPTED_ANSWERS_CUSTOM_FIELD = "enable_accepted_answers"
IS_ACCEPTED_ANSWER_CUSTOM_FIELD = "is_accepted_answer"

class Engine < ::Rails::Engine
engine_name PLUGIN_NAME
Expand All @@ -44,6 +42,7 @@ class Engine < ::Rails::Engine
require_relative "app/lib/user_summary_extension"
require_relative "app/lib/web_hook_extension"
require_relative "app/serializers/concerns/topic_answer_mixin"
require_relative "app/models/discourse-solved/solution.rb"

require_relative "app/lib/plugin_initializers/assigned_reminder_exclude_solved"
DiscourseSolved::AssignsReminderForTopicsQuery.new(self).apply_plugin_api
Expand All @@ -52,18 +51,18 @@ def self.accept_answer!(post, acting_user, topic: nil)
topic ||= post.topic

DistributedMutex.synchronize("discourse_solved_toggle_answer_#{topic.id}") do
accepted_id = topic.custom_fields[ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD].to_i

if accepted_id > 0
if p2 = Post.find_by(id: accepted_id)
p2.custom_fields.delete(IS_ACCEPTED_ANSWER_CUSTOM_FIELD)
p2.save!

UserAction.where(action_type: UserAction::SOLVED, target_post_id: p2.id).destroy_all
end
old_solution = topic.solution
Lhcfl marked this conversation as resolved.
Show resolved Hide resolved

if old_solution.present?
UserAction.where(
action_type: UserAction::SOLVED,
target_post_id: old_solution.answer_post_id,
).destroy_all
old_solution.destroy!
end

post.custom_fields[IS_ACCEPTED_ANSWER_CUSTOM_FIELD] = "true"
solution = DiscourseSolved::Solution.create(topic:, post:, accepter_user_id: acting_user.id)

topic.custom_fields[ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD] = post.id

UserAction.log_action!(
Expand Down Expand Up @@ -118,13 +117,13 @@ def self.accept_answer!(post, acting_user, topic: nil)
duration_minutes: auto_close_hours * 60,
)

topic.custom_fields[AUTO_CLOSE_TOPIC_TIMER_CUSTOM_FIELD] = topic_timer.id
solution.topic_timer_id = topic_timer.id

MessageBus.publish("/topic/#{topic.id}", reload_topic: true)
end

topic.save!
post.save!
solution.save!

if WebHook.active_web_hooks(:accepted_solution).exists?
payload = WebHook.generate_payload(:post, post)
Expand All @@ -142,17 +141,10 @@ def self.unaccept_answer!(post, topic: nil)
return if topic.nil?

DistributedMutex.synchronize("discourse_solved_toggle_answer_#{topic.id}") do
post.custom_fields.delete(IS_ACCEPTED_ANSWER_CUSTOM_FIELD)
topic.solution&.destroy!
topic.custom_fields.delete(ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD)

if timer_id = topic.custom_fields[AUTO_CLOSE_TOPIC_TIMER_CUSTOM_FIELD]
topic_timer = TopicTimer.find_by(id: timer_id)
topic_timer.destroy! if topic_timer
topic.custom_fields.delete(AUTO_CLOSE_TOPIC_TIMER_CUSTOM_FIELD)
end

topic.save!
post.save!

# TODO remove_action! does not allow for this type of interface
UserAction.where(action_type: UserAction::SOLVED, target_post_id: post.id).destroy_all
Expand Down Expand Up @@ -190,6 +182,12 @@ def self.skip_db?
::PostSerializer.prepend(DiscourseSolved::PostSerializerExtension)
::UserSummary.prepend(DiscourseSolved::UserSummaryExtension)
::Topic.attr_accessor(:accepted_answer_user_id)
::Topic.has_one(:solution, class_name: ::DiscourseSolved::Solution.to_s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this and the next line should be in a Extension module.

::Post.has_one(
:solution,
class_name: ::DiscourseSolved::Solution.to_s,
foreign_key: :answer_post_id,
)
::TopicPostersSummary.alias_method(:old_user_ids, :user_ids)
::TopicPostersSummary.prepend(DiscourseSolved::TopicPostersSummaryExtension)
[
Expand Down Expand Up @@ -246,19 +244,13 @@ def self.skip_db?

Discourse::Application.routes.append { mount ::DiscourseSolved::Engine, at: "solution" }

on(:post_destroyed) do |post|
if post.custom_fields[::DiscourseSolved::IS_ACCEPTED_ANSWER_CUSTOM_FIELD] == "true"
::DiscourseSolved.unaccept_answer!(post)
end
end
on(:post_destroyed) { |post| ::DiscourseSolved.unaccept_answer!(post) if post.solution }

add_api_key_scope(
:solved,
{ answer: { actions: %w[discourse_solved/answer#accept discourse_solved/answer#unaccept] } },
)

topic_view_post_custom_fields_allowlister { [::DiscourseSolved::IS_ACCEPTED_ANSWER_CUSTOM_FIELD] }

register_html_builder("server:before-head-close-crawler") do |controller|
DiscourseSolved::BeforeHeadClose.new(controller).html
end
Expand All @@ -270,8 +262,7 @@ def self.skip_db?
Report.add_report("accepted_solutions") do |report|
report.data = []

accepted_solutions =
TopicCustomField.where(name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD)
accepted_solutions = DiscourseSolved::Solution

category_id, include_subcategories = report.add_category_filter
if category_id
Expand All @@ -288,17 +279,17 @@ def self.skip_db?
end

accepted_solutions
.where("topic_custom_fields.created_at >= ?", report.start_date)
.where("topic_custom_fields.created_at <= ?", report.end_date)
.group("DATE(topic_custom_fields.created_at)")
.order("DATE(topic_custom_fields.created_at)")
.where("discourse_solved_solutions.created_at >= ?", report.start_date)
.where("discourse_solved_solutions.created_at <= ?", report.end_date)
.group("DATE(discourse_solved_solutions.created_at)")
.order("DATE(discourse_solved_solutions.created_at)")
.count
.each { |date, count| report.data << { x: date, y: count } }
report.total = accepted_solutions.count
report.prev30Days =
accepted_solutions
.where("topic_custom_fields.created_at >= ?", report.start_date - 30.days)
.where("topic_custom_fields.created_at <= ?", report.start_date)
.where("discourse_solved_solutions.created_at >= ?", report.start_date - 30.days)
.where("discourse_solved_solutions.created_at <= ?", report.start_date)
.count
end

Expand All @@ -307,10 +298,8 @@ def self.skip_db?
condition = <<~SQL
EXISTS (
SELECT 1
FROM topic_custom_fields
FROM discourse_solved_solutions
WHERE topic_id = topics.id
AND name = '#{::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD}'
AND value IS NOT NULL
)
SQL

Expand All @@ -328,7 +317,10 @@ def self.skip_db?
scope.can_accept_answer?(topic, object) && accepted_answer
end
add_to_serializer(:post, :accepted_answer) do
post_custom_fields[::DiscourseSolved::IS_ACCEPTED_ANSWER_CUSTOM_FIELD] == "true"
if topic&.custom_fields&.[](::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD).nil?
return false
end
topic.solution.answer_post_id == object.id
end
add_to_serializer(:post, :topic_accepted_answer) do
topic&.custom_fields&.[](::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD).present?
Expand Down Expand Up @@ -408,10 +400,8 @@ def self.skip_db?
sql = <<~SQL
NOT EXISTS (
SELECT 1
FROM topic_custom_fields
FROM discourse_solved_solutions
WHERE topic_id = topics.id
AND name = '#{::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD}'
AND value IS NOT NULL
)
SQL

Expand Down
4 changes: 4 additions & 0 deletions spec/fabricators/extend_topic_fabricator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
transient :custom_topic_name
transient :value
after_create do |top, transients|
if (transients[:custom_topic_name] == DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD)
Lhcfl marked this conversation as resolved.
Show resolved Hide resolved
post = Fabricate(:post)
Fabricate(:solution, topic_id: top.id, answer_post_id: post.id)
end
custom_topic =
TopicCustomField.new(
topic_id: top.id,
Expand Down
6 changes: 6 additions & 0 deletions spec/fabricators/solution_fabricator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true

Fabricator(:solution, from: DiscourseSolved::Solution) do
topic_id
answer_post_id
end
Loading