From 150f7f4a352f8820f759fa3cab3cb8d1c33a395d Mon Sep 17 00:00:00 2001 From: Lhc_fl Date: Thu, 5 Sep 2024 17:55:02 +0800 Subject: [PATCH] DEV: Move the post and topic custom fields into a table This PR does some preparatory work to prepare for the front-end display of who marked an answer as solved. It migrates the custom fields from discourse-solved of Topic and Post to a new table, except "accepted_answer_post_id" ref: /t/-/95318 --- app/lib/before_head_close.rb | 5 +- .../assigned_reminder_exclude_solved.rb | 7 +- app/models/discourse-solved/solution.rb | 10 +++ ...tom_field_to_discourse_solved_solutions.rb | 52 +++++++++++++ plugin.rb | 76 ++++++++----------- spec/fabricators/extend_topic_fabricator.rb | 4 + spec/fabricators/solution_fabricator.rb | 6 ++ spec/integration/solved_spec.rb | 26 +++---- spec/requests/topics_controller_spec.rb | 13 ++-- 9 files changed, 123 insertions(+), 76 deletions(-) create mode 100644 app/models/discourse-solved/solution.rb create mode 100644 db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb create mode 100644 spec/fabricators/solution_fabricator.rb diff --git a/app/lib/before_head_close.rb b/app/lib/before_head_close.rb index b07b9bf..945284d 100644 --- a/app/lib/before_head_close.rb +++ b/app/lib/before_head_close.rb @@ -40,10 +40,7 @@ class DiscourseSolved::BeforeHeadClose }, } - 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", diff --git a/app/lib/plugin_initializers/assigned_reminder_exclude_solved.rb b/app/lib/plugin_initializers/assigned_reminder_exclude_solved.rb index f3aa8f8..611a376 100644 --- a/app/lib/plugin_initializers/assigned_reminder_exclude_solved.rb +++ b/app/lib/plugin_initializers/assigned_reminder_exclude_solved.rb @@ -18,12 +18,7 @@ module DiscourseSolved 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)) end end end diff --git a/app/models/discourse-solved/solution.rb b/app/models/discourse-solved/solution.rb new file mode 100644 index 0000000..d4315d4 --- /dev/null +++ b/app/models/discourse-solved/solution.rb @@ -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 diff --git a/db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb b/db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb new file mode 100644 index 0000000..04ba044 --- /dev/null +++ b/db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true +class MoveSolvedTopicCustomFieldToDiscourseSolvedSolutions < ActiveRecord::Migration[7.1] + 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 + t.integer :topic_timer_id, null: true + t.timestamps + end + + add_index :discourse_solved_solutions, :topic_id, unique: true + add_index :discourse_solved_solutions, :answer_post_id, unique: true + + 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 + 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 + end + + def down + # raise ActiveRecord::IrreversibleMigration + remove_index :discourse_solved_solutions, :topic_id + remove_index :discourse_solved_solutions, :answer_post_id + + drop_table :discourse_solved_solutions + end +end diff --git a/plugin.rb b/plugin.rb index dc8f32b..a7e435c 100644 --- a/plugin.rb +++ b/plugin.rb @@ -19,10 +19,8 @@ register_asset "stylesheets/mobile/solutions.scss", :mobile 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 @@ after_initialize do 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,18 @@ after_initialize do 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 + old_solution = topic.solution - 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 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!( @@ -118,13 +117,13 @@ after_initialize do 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 +141,10 @@ after_initialize do 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 +182,12 @@ after_initialize do ::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) + ::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 +244,13 @@ after_initialize do 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 +262,7 @@ after_initialize do 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 +279,17 @@ after_initialize do 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 +298,8 @@ after_initialize do 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 +317,10 @@ after_initialize do 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 +400,8 @@ after_initialize do 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 diff --git a/spec/fabricators/extend_topic_fabricator.rb b/spec/fabricators/extend_topic_fabricator.rb index c41e4b6..ca38a27 100644 --- a/spec/fabricators/extend_topic_fabricator.rb +++ b/spec/fabricators/extend_topic_fabricator.rb @@ -3,6 +3,10 @@ Fabricator(:custom_topic, from: :topic) do transient :custom_topic_name transient :value after_create do |top, transients| + if (transients[:custom_topic_name] == DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD) + post = Fabricate(:post) + Fabricate(:solution, topic_id: top.id, answer_post_id: post.id) + end custom_topic = TopicCustomField.new( topic_id: top.id, diff --git a/spec/fabricators/solution_fabricator.rb b/spec/fabricators/solution_fabricator.rb new file mode 100644 index 0000000..868dc81 --- /dev/null +++ b/spec/fabricators/solution_fabricator.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +Fabricator(:solution, from: DiscourseSolved::Solution) do + topic_id + answer_post_id +end diff --git a/spec/integration/solved_spec.rb b/spec/integration/solved_spec.rb index 75e4a90..9906df2 100644 --- a/spec/integration/solved_spec.rb +++ b/spec/integration/solved_spec.rb @@ -248,15 +248,13 @@ RSpec.describe "Managing Posts solved status" do post "/solution/accept.json", params: { id: p1.id } expect(response.status).to eq(200) - expect(p1.reload.custom_fields["is_accepted_answer"]).to eq("true") + expect(p1.reload.solution.present?).to eq(true) topic.reload expect(topic.public_topic_timer.status_type).to eq(TopicTimer.types[:silent_close]) - expect(topic.custom_fields["solved_auto_close_topic_timer_id"].to_i).to eq( - topic.public_topic_timer.id, - ) + expect(topic.solution.topic_timer_id).to eq(topic.public_topic_timer.id) expect(topic.public_topic_timer.execute_at).to eq_time(Time.zone.now + 2.hours) @@ -274,15 +272,13 @@ RSpec.describe "Managing Posts solved status" do post "/solution/accept.json", params: { id: post_2.id } expect(response.status).to eq(200) - expect(post_2.reload.custom_fields["is_accepted_answer"]).to eq("true") + expect(post_2.reload.solution.present?).to eq(true) topic_2.reload expect(topic_2.public_topic_timer.status_type).to eq(TopicTimer.types[:silent_close]) - expect(topic_2.custom_fields["solved_auto_close_topic_timer_id"].to_i).to eq( - topic_2.public_topic_timer.id, - ) + expect(topic_2.solution.topic_timer_id).to eq(topic_2.public_topic_timer.id) expect(topic_2.public_topic_timer.execute_at).to eq_time(Time.zone.now + 4.hours) @@ -322,7 +318,7 @@ RSpec.describe "Managing Posts solved status" do p1.reload topic.reload - expect(p1.custom_fields["is_accepted_answer"]).to eq("true") + expect(p1.solution.present?).to eq(true) expect(topic.public_topic_timer).to eq(nil) expect(topic.closed).to eq(true) end @@ -338,7 +334,7 @@ RSpec.describe "Managing Posts solved status" do expect(response.status).to eq(200) p1.reload - expect(p1.custom_fields["is_accepted_answer"]).to eq("true") + expect(p1.solution.present?).to eq(true) end it "removes the solution when the post is deleted" do @@ -348,13 +344,13 @@ RSpec.describe "Managing Posts solved status" do expect(response.status).to eq(200) reply.reload - expect(reply.custom_fields["is_accepted_answer"]).to eq("true") + expect(reply.solution.present?).to eq(true) expect(reply.topic.custom_fields["accepted_answer_post_id"].to_i).to eq(reply.id) PostDestroyer.new(Discourse.system_user, reply).destroy reply.reload - expect(reply.custom_fields["is_accepted_answer"]).to eq(nil) + expect(reply.solution).to eq(nil) expect(reply.topic.custom_fields["accepted_answer_post_id"]).to eq(nil) end @@ -395,7 +391,7 @@ RSpec.describe "Managing Posts solved status" do expect(response.status).to eq(200) p1.reload - expect(p1.custom_fields["is_accepted_answer"]).to eq(nil) + expect(p1.solution).to eq(nil) expect(p1.topic.custom_fields["accepted_answer_post_id"]).to eq(nil) end end @@ -457,7 +453,7 @@ RSpec.describe "Managing Posts solved status" do expect(p1.topic.assignment.status).to eq("New") DiscourseSolved.accept_answer!(p1, user) - expect(p1.reload.custom_fields["is_accepted_answer"]).to eq("true") + expect(p1.reload.solution.present?).to eq(true) expect(p1.topic.assignment.reload.status).to eq("Done") end @@ -472,7 +468,7 @@ RSpec.describe "Managing Posts solved status" do DiscourseSolved.unaccept_answer!(p1) - expect(p1.reload.custom_fields["is_accepted_answer"]).to eq(nil) + expect(p1.reload.solution).to eq(nil) expect(p1.reload.topic.assignment.reload.status).to eq("New") end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 215b06a..8840830 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -45,11 +45,11 @@ RSpec.describe TopicsController do expect(response.body).to include(schema_json(0)) - p2.custom_fields["is_accepted_answer"] = true - p2.save_custom_fields topic.custom_fields["accepted_answer_post_id"] = p2.id topic.save_custom_fields + Fabricate(:solution, topic_id: topic.id, answer_post_id: p2.id) + get "/t/#{topic.slug}/#{topic.id}" expect(response.body).to include(schema_json(1)) @@ -68,10 +68,9 @@ RSpec.describe TopicsController do it "should include user name in output with the corresponding site setting" do SiteSetting.display_name_on_posts = true - p2.custom_fields["is_accepted_answer"] = true - p2.save_custom_fields topic.custom_fields["accepted_answer_post_id"] = p2.id topic.save_custom_fields + Fabricate(:solution, topic_id: topic.id, answer_post_id: p2.id) get "/t/#{topic.slug}/#{topic.id}.json" @@ -87,10 +86,9 @@ RSpec.describe TopicsController do it "should not include user name when site setting is disabled" do SiteSetting.display_name_on_posts = false - p2.custom_fields["is_accepted_answer"] = true - p2.save_custom_fields topic.custom_fields["accepted_answer_post_id"] = p2.id topic.save_custom_fields + Fabricate(:solution, topic_id: topic.id, answer_post_id: p2.id) get "/t/#{topic.slug}/#{topic.id}.json" @@ -106,10 +104,9 @@ RSpec.describe TopicsController do it "includes the correct schema information" do DiscourseTagging.add_or_create_tags_by_name(topic, [tag.name]) - p2.custom_fields["is_accepted_answer"] = true - p2.save_custom_fields topic.custom_fields["accepted_answer_post_id"] = p2.id topic.save_custom_fields + Fabricate(:solution, topic_id: topic.id, answer_post_id: p2.id) get "/t/#{topic.slug}/#{topic.id}"