-
Notifications
You must be signed in to change notification settings - Fork 83
DEV: Move the post and topic custom fields into a table #309
Changes from all commits
150f7f4
56f8327
29f744e
c00ca89
ba79689
2c0aff5
1a07c87
15bc182
d10c7c3
ca19bb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# frozen_string_literal: true | ||
class MoveSolvedTopicCustomFieldToDiscourseSolvedSolutions < ActiveRecord::Migration[7.1] | ||
disable_ddl_transaction! | ||
|
||
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 | ||
t.integer :topic_timer_id | ||
t.timestamps | ||
end | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an index in user_actions for target_topic_id? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This looks interesting to me. I wonder if we need a index on name. From my rusty knowledge There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I just noticed that custom_fields does not an index on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it interesting that we have no index on We have this one:
Which I find a bit strange, why not |
||
ON ua.target_topic_id = tc.topic_id | ||
WHERE tc.name = 'accepted_answer_post_id' | ||
AND ua.action_type = #{UserAction::SOLVED} | ||
SQL | ||
|
||
add_index :discourse_solved_solutions, :topic_id, unique: true, algorithm: :concurrently | ||
add_index :discourse_solved_solutions, :answer_post_id, unique: true, algorithm: :concurrently | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is a trick to make it a bit better:
... but it needs to be tested. |
||
end | ||
|
||
def down | ||
raise ActiveRecord::IrreversibleMigration | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -52,18 +51,16 @@ 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 | ||
if topic.solution.present? | ||
UserAction.where( | ||
action_type: UserAction::SOLVED, | ||
target_post_id: topic.solution.answer_post_id, | ||
).destroy_all | ||
topic.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!( | ||
|
@@ -118,13 +115,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) | ||
|
@@ -142,17 +139,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 | ||
|
@@ -190,6 +180,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
::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) | ||
[ | ||
|
@@ -246,19 +242,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 | ||
|
@@ -270,8 +260,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 | ||
|
@@ -288,17 +277,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 | ||
|
||
|
@@ -307,10 +296,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 | ||
|
||
|
@@ -328,7 +315,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? | ||
|
@@ -408,10 +398,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 | ||
|
||
|
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 |
Uh oh!
There was an error while loading. Please reload this page.