From 55c1eb4d605eabf9190e171af4de59a74b238f9e Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Tue, 25 Mar 2025 14:51:32 +0800 Subject: [PATCH] DEV: Move solved custom fields into a table (#342) Related: - https://github.com/discourse/discourse-solved/pull/309 - https://github.com/discourse/discourse-solved/pull/341 Requires: - https://github.com/discourse/discourse/pull/31954 This commit converts all use of post and topic custom fields into a dedicated table: - migration for copying custom field into table - swap app usage of custom fields to table This commit does not attempt to fix issues or optimise, and does not delete old data from custom fields _yet_. --- .discourse-compatibility | 1 + app/models/discourse_solved/solved_topic.rb | 34 ++++ .../discourse_solved/topic_answer_mixin.rb | 2 +- db/fixtures/001_badges.rb | 24 ++- ...4_create_discourse_solved_solved_topics.rb | 13 ++ ...field_to_discourse_solved_solved_topics.rb | 48 +++++ ...7_add_index_for_discourse_solved_topics.rb | 24 +++ lib/discourse_assign/entry_point.rb | 10 +- lib/discourse_dev/discourse_solved.rb | 8 +- lib/discourse_solved/before_head_close.rb | 5 +- lib/discourse_solved/register_filters.rb | 50 ++--- lib/discourse_solved/topic_extension.rb | 7 + .../topic_view_serializer_extension.rb | 8 +- .../user_summary_extension.rb | 2 +- plugin.rb | 184 ++++++------------ spec/fabricators/solved_topic_fabricator.rb | 6 + spec/integration/solved_spec.rb | 103 +++------- spec/requests/topics_controller_spec.rb | 25 +-- spec/serializers/topic_answer_mixin_spec.rb | 5 +- spec/serializers/user_card_serializer_spec.rb | 1 + 20 files changed, 272 insertions(+), 288 deletions(-) create mode 100644 app/models/discourse_solved/solved_topic.rb create mode 100644 db/migrate/20250318024824_create_discourse_solved_solved_topics.rb create mode 100644 db/migrate/20250318024953_copy_solved_topic_custom_field_to_discourse_solved_solved_topics.rb create mode 100644 db/migrate/20250318025147_add_index_for_discourse_solved_topics.rb create mode 100644 lib/discourse_solved/topic_extension.rb create mode 100644 spec/fabricators/solved_topic_fabricator.rb diff --git a/.discourse-compatibility b/.discourse-compatibility index c9823b7..d2002c3 100644 --- a/.discourse-compatibility +++ b/.discourse-compatibility @@ -1,3 +1,4 @@ +< 3.5.0.beta2-dev: e82c6ae1ca38ccebb34669148f8de93a3028906e < 3.5.0.beta1-dev: 5450a5ef4e2ae35185320fc6af9678621026e148 < 3.4.0.beta4-dev: 3f724bf3114cc7877fa757bc8035f13a7390c739 < 3.4.0.beta2-dev: 1bbdfd8f5681171dc3f0e9ea93cd56997dc7938a diff --git a/app/models/discourse_solved/solved_topic.rb b/app/models/discourse_solved/solved_topic.rb new file mode 100644 index 0000000..1c666e5 --- /dev/null +++ b/app/models/discourse_solved/solved_topic.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module DiscourseSolved + class SolvedTopic < ActiveRecord::Base + self.table_name = "discourse_solved_solved_topics" + + belongs_to :topic, class_name: "Topic" + belongs_to :answer_post, class_name: "Post", foreign_key: "answer_post_id" + belongs_to :accepter, class_name: "User", foreign_key: "accepter_user_id" + belongs_to :topic_timer + + validates :topic_id, presence: true + validates :answer_post_id, presence: true + validates :accepter_user_id, presence: true + end +end + +# == Schema Information +# +# Table name: discourse_solved_solved_topics +# +# id :bigint not null, primary key +# topic_id :integer not null +# answer_post_id :integer not null +# accepter_user_id :integer not null +# topic_timer_id :integer +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_discourse_solved_solved_topics_on_answer_post_id (answer_post_id) UNIQUE +# index_discourse_solved_solved_topics_on_topic_id (topic_id) UNIQUE +# diff --git a/app/serializers/concerns/discourse_solved/topic_answer_mixin.rb b/app/serializers/concerns/discourse_solved/topic_answer_mixin.rb index 62030ac..ebff49c 100644 --- a/app/serializers/concerns/discourse_solved/topic_answer_mixin.rb +++ b/app/serializers/concerns/discourse_solved/topic_answer_mixin.rb @@ -7,7 +7,7 @@ module DiscourseSolved end def has_accepted_answer - object.custom_fields[::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD].present? + object&.solved.present? end def include_has_accepted_answer? diff --git a/db/fixtures/001_badges.rb b/db/fixtures/001_badges.rb index d89ba24..7addd2b 100644 --- a/db/fixtures/001_badges.rb +++ b/db/fixtures/001_badges.rb @@ -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, dsst.created_at, + ROW_NUMBER() OVER (PARTITION BY p.user_id ORDER BY dsst.created_at) AS row_number + FROM discourse_solved_solved_topics dsst + JOIN badge_posts p ON dsst.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 @@ -32,12 +31,11 @@ end 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(dsst.created_at) AS granted_at + FROM discourse_solved_solved_topics dsst + JOIN badge_posts p ON dsst.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} diff --git a/db/migrate/20250318024824_create_discourse_solved_solved_topics.rb b/db/migrate/20250318024824_create_discourse_solved_solved_topics.rb new file mode 100644 index 0000000..befd523 --- /dev/null +++ b/db/migrate/20250318024824_create_discourse_solved_solved_topics.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +# +class CreateDiscourseSolvedSolvedTopics < ActiveRecord::Migration[7.2] + def change + create_table :discourse_solved_solved_topics do |t| + t.integer :topic_id, null: false + t.integer :answer_post_id, null: false + t.integer :accepter_user_id, null: false + t.integer :topic_timer_id + t.timestamps + end + end +end 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 new file mode 100644 index 0000000..6633fda --- /dev/null +++ b/db/migrate/20250318024953_copy_solved_topic_custom_field_to_discourse_solved_solved_topics.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true +# +class CopySolvedTopicCustomFieldToDiscourseSolvedSolvedTopics < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + BATCH_SIZE = 5000 + + def up + last_id = 0 + loop do + rows = DB.query(<<~SQL, last_id: last_id, batch_size: BATCH_SIZE) + INSERT INTO discourse_solved_solved_topics ( + topic_id, + answer_post_id, + topic_timer_id, + accepter_user_id, + created_at, + updated_at + ) + SELECT DISTINCT ON (tc.topic_id) + tc.topic_id, + CAST(tc.value AS INTEGER), + CAST(tc2.value AS INTEGER), + COALESCE(ua.acting_user_id, -1), + 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 + ORDER BY tc.topic_id, ua.created_at DESC + LIMIT :batch_size + SQL + + break if rows.length == 0 + last_id += BATCH_SIZE + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20250318025147_add_index_for_discourse_solved_topics.rb b/db/migrate/20250318025147_add_index_for_discourse_solved_topics.rb new file mode 100644 index 0000000..cc87d32 --- /dev/null +++ b/db/migrate/20250318025147_add_index_for_discourse_solved_topics.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true +# +class AddIndexForDiscourseSolvedTopics < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + def change + remove_index :discourse_solved_solved_topics, + :topic_id, + algorithm: :concurrently, + unique: true, + if_exists: true + remove_index :discourse_solved_solved_topics, + :answer_post_id, + algorithm: :concurrently, + unique: true, + if_exists: true + + add_index :discourse_solved_solved_topics, :topic_id, unique: true, algorithm: :concurrently + add_index :discourse_solved_solved_topics, + :answer_post_id, + unique: true, + algorithm: :concurrently + end +end diff --git a/lib/discourse_assign/entry_point.rb b/lib/discourse_assign/entry_point.rb index 2b6b7c4..12821f6 100644 --- a/lib/discourse_assign/entry_point.rb +++ b/lib/discourse_assign/entry_point.rb @@ -2,15 +2,13 @@ module DiscourseAssign class EntryPoint + # TODO: These four plugin api usages should ideally be in the assign plugin, not the solved plugin. + # They have been moved here from plugin.rb as part of the custom fields migration. + def self.inject(plugin) 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::SolvedTopic.select(:topic_id)) end plugin.register_modifier(:assigned_count_for_user_query) do |query, user| diff --git a/lib/discourse_dev/discourse_solved.rb b/lib/discourse_dev/discourse_solved.rb index 472da64..3d72e7e 100644 --- a/lib/discourse_dev/discourse_solved.rb +++ b/lib/discourse_dev/discourse_solved.rb @@ -11,24 +11,24 @@ module DiscourseDev solved_category = DiscourseDev::Record.random( - Category.where( + ::Category.where( read_restricted: false, id: records.pluck(:id), parent_category_id: nil, ), ) - CategoryCustomField.create!( + ::CategoryCustomField.create!( category_id: solved_category.id, name: ::DiscourseSolved::ENABLE_ACCEPTED_ANSWERS_CUSTOM_FIELD, value: "true", ) puts "discourse-solved enabled on category '#{solved_category.name}' (#{solved_category.id})." elsif type == :topic - topics = Topic.where(id: records.pluck(:id)) + topics = ::Topic.where(id: records.pluck(:id)) unless SiteSetting.allow_solved_on_all_topics solved_category_id = - CategoryCustomField + ::CategoryCustomField .where(name: ::DiscourseSolved::ENABLE_ACCEPTED_ANSWERS_CUSTOM_FIELD, value: "true") .first .category_id diff --git a/lib/discourse_solved/before_head_close.rb b/lib/discourse_solved/before_head_close.rb index b07b9bf..ec9d0c5 100644 --- a/lib/discourse_solved/before_head_close.rb +++ b/lib/discourse_solved/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.solved&.answer_post question_json["answerCount"] = 1 question_json[:acceptedAnswer] = { "@type" => "Answer", diff --git a/lib/discourse_solved/register_filters.rb b/lib/discourse_solved/register_filters.rb index 75c14bf..b4ebfff 100644 --- a/lib/discourse_solved/register_filters.rb +++ b/lib/discourse_solved/register_filters.rb @@ -4,24 +4,16 @@ module DiscourseSolved class RegisterFilters def self.register(plugin) solved_callback = ->(scope) do - sql = <<~SQL - topics.id IN ( - SELECT topic_id - FROM topic_custom_fields - WHERE name = '#{::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD}' - AND value IS NOT NULL - ) - SQL - - scope.where(sql).where("topics.archetype <> ?", Archetype.private_message) + scope.joins( + "INNER JOIN discourse_solved_solved_topics ON discourse_solved_solved_topics.topic_id = topics.id", + ).where("topics.archetype <> ?", Archetype.private_message) end + unsolved_callback = ->(scope) do - scope = scope.where <<~SQL + scope = scope.where(<<~SQL) topics.id NOT IN ( SELECT topic_id - FROM topic_custom_fields - WHERE name = '#{::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD}' - AND value IS NOT NULL + FROM discourse_solved_solved_topics ) SQL @@ -29,21 +21,21 @@ module DiscourseSolved tag_ids = Tag.where(name: SiteSetting.enable_solved_tags.split("|")).pluck(:id) scope = scope.where <<~SQL, tag_ids - topics.id IN ( - SELECT t.id - FROM topics t - JOIN category_custom_fields cc - ON t.category_id = cc.category_id - AND cc.name = '#{::DiscourseSolved::ENABLE_ACCEPTED_ANSWERS_CUSTOM_FIELD}' - AND cc.value = 'true' - ) - OR - topics.id IN ( - SELECT topic_id - FROM topic_tags - WHERE tag_id IN (?) - ) - SQL + topics.id IN ( + SELECT t.id + FROM topics t + JOIN category_custom_fields cc + ON t.category_id = cc.category_id + AND cc.name = '#{::DiscourseSolved::ENABLE_ACCEPTED_ANSWERS_CUSTOM_FIELD}' + AND cc.value = 'true' + ) + OR + topics.id IN ( + SELECT topic_id + FROM topic_tags + WHERE tag_id IN (?) + ) + SQL end scope.where("topics.archetype <> ?", Archetype.private_message) diff --git a/lib/discourse_solved/topic_extension.rb b/lib/discourse_solved/topic_extension.rb new file mode 100644 index 0000000..cfd6757 --- /dev/null +++ b/lib/discourse_solved/topic_extension.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module DiscourseSolved::TopicExtension + extend ActiveSupport::Concern + + prepended { has_one :solved, class_name: "DiscourseSolved::SolvedTopic", dependent: :destroy } +end diff --git a/lib/discourse_solved/topic_view_serializer_extension.rb b/lib/discourse_solved/topic_view_serializer_extension.rb index 05008cc..22807fe 100644 --- a/lib/discourse_solved/topic_view_serializer_extension.rb +++ b/lib/discourse_solved/topic_view_serializer_extension.rb @@ -43,12 +43,6 @@ module DiscourseSolved::TopicViewSerializerExtension end def accepted_answer_post_id - id = object.topic.custom_fields[::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD] - # a bit messy but race conditions can give us an array here, avoid - begin - id && id.to_i - rescue StandardError - nil - end + object.topic.solved&.answer_post_id end end diff --git a/lib/discourse_solved/user_summary_extension.rb b/lib/discourse_solved/user_summary_extension.rb index 5194c03..9dcc509 100644 --- a/lib/discourse_solved/user_summary_extension.rb +++ b/lib/discourse_solved/user_summary_extension.rb @@ -4,6 +4,6 @@ module DiscourseSolved::UserSummaryExtension extend ActiveSupport::Concern def solved_count - UserAction.where(user: @user).where(action_type: UserAction::SOLVED).count + DiscourseSolved::SolvedTopic.where(accepter: @user).count end end diff --git a/plugin.rb b/plugin.rb index c5ac174..37a0fb3 100644 --- a/plugin.rb +++ b/plugin.rb @@ -18,10 +18,7 @@ register_asset "stylesheets/mobile/solutions.scss", :mobile 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" end require_relative "lib/discourse_solved/engine.rb" @@ -34,27 +31,25 @@ 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 + solved = topic.solved - 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 previous_accepted_post_id = solved&.answer_post_id + UserAction.where( + action_type: UserAction::SOLVED, + target_post_id: previous_accepted_post_id, + ).destroy_all + else + UserAction.log_action!( + action_type: UserAction::SOLVED, + user_id: post.user_id, + acting_user_id: acting_user.id, + target_post_id: post.id, + target_topic_id: post.topic_id, + ) end - post.custom_fields[IS_ACCEPTED_ANSWER_CUSTOM_FIELD] = "true" - topic.custom_fields[ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD] = post.id - - UserAction.log_action!( - action_type: UserAction::SOLVED, - user_id: post.user_id, - acting_user_id: acting_user.id, - target_post_id: post.id, - target_topic_id: post.topic_id, - ) + solved ||= + DiscourseSolved::SolvedTopic.new(topic:, answer_post: post, accepter: acting_user) notification_data = { message: "solved.accepted_notification", @@ -99,14 +94,12 @@ after_initialize do based_on_last_post: true, duration_minutes: auto_close_hours * 60, ) - - topic.custom_fields[AUTO_CLOSE_TOPIC_TIMER_CUSTOM_FIELD] = topic_timer.id + solved.topic_timer = topic_timer MessageBus.publish("/topic/#{topic.id}", reload_topic: true) end - topic.save! - post.save! + solved.save! if WebHook.active_web_hooks(:accepted_solution).exists? payload = WebHook.generate_payload(:post, post) @@ -120,41 +113,28 @@ after_initialize do def self.unaccept_answer!(post, topic: nil) topic ||= post.topic topic ||= Topic.unscoped.find_by(id: post.topic_id) - return if topic.nil? + return if topic.solved.nil? DistributedMutex.synchronize("discourse_solved_toggle_answer_#{topic.id}") do - post.custom_fields.delete(IS_ACCEPTED_ANSWER_CUSTOM_FIELD) - topic.custom_fields.delete(ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD) + solved = topic.solved - 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 - - # yank notification - notification = + ActiveRecord::Base.transaction do + UserAction.where(action_type: UserAction::SOLVED, target_post_id: post.id).destroy_all Notification.find_by( notification_type: Notification.types[:custom], user_id: post.user_id, topic_id: post.topic_id, post_number: post.post_number, - ) - - notification.destroy! if notification + )&.destroy! + solved.topic_timer.destroy! if solved.topic_timer + solved.destroy! + end if WebHook.active_web_hooks(:unaccepted_solution).exists? payload = WebHook.generate_payload(:post, post) WebHook.enqueue_solved_hooks(:unaccepted_solution, post, payload) end - DiscourseEvent.trigger(:unaccepted_solution, post) end end @@ -168,6 +148,7 @@ after_initialize do ::Guardian.prepend(DiscourseSolved::GuardianExtensions) ::WebHook.prepend(DiscourseSolved::WebHookExtension) ::TopicViewSerializer.prepend(DiscourseSolved::TopicViewSerializerExtension) + ::Topic.prepend(DiscourseSolved::TopicExtension) ::Category.prepend(DiscourseSolved::CategoryExtension) ::PostSerializer.prepend(DiscourseSolved::PostSerializerExtension) ::UserSummary.prepend(DiscourseSolved::UserSummaryExtension) @@ -183,49 +164,10 @@ after_initialize do ].each { |klass| klass.include(DiscourseSolved::TopicAnswerMixin) } end - # we got to do a one time upgrade - if !::DiscourseSolved.skip_db? - unless Discourse.redis.get("solved_already_upgraded") - unless UserAction.where(action_type: UserAction::SOLVED).exists? - Rails.logger.info("Upgrading storage for solved") - sql = <<~SQL - INSERT INTO user_actions(action_type, - user_id, - target_topic_id, - target_post_id, - acting_user_id, - created_at, - updated_at) - SELECT :solved, - p.user_id, - p.topic_id, - p.id, - t.user_id, - pc.created_at, - pc.updated_at - FROM - post_custom_fields pc - JOIN - posts p ON p.id = pc.post_id - JOIN - topics t ON t.id = p.topic_id - WHERE - pc.name = 'is_accepted_answer' AND - pc.value = 'true' AND - p.user_id IS NOT NULL - SQL - - DB.exec(sql, solved: UserAction::SOLVED) - end - Discourse.redis.set("solved_already_upgraded", "true") - end - end - - topic_view_post_custom_fields_allowlister { [::DiscourseSolved::IS_ACCEPTED_ANSWER_CUSTOM_FIELD] } - TopicList.preloaded_custom_fields << ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD + register_category_list_topics_preloader_associations(:solved) if SiteSetting.solved_enabled + register_topic_preloader_associations(:solved) if SiteSetting.solved_enabled + Search.custom_topic_eager_load { [:solved] } if SiteSetting.solved_enabled Site.preloaded_category_custom_fields << ::DiscourseSolved::ENABLE_ACCEPTED_ANSWERS_CUSTOM_FIELD - Search.preloaded_topic_custom_fields << ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD - CategoryList.preloaded_topic_custom_fields << ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD add_api_key_scope( :solved, @@ -244,10 +186,10 @@ after_initialize do report.data = [] accepted_solutions = - TopicCustomField - .joins(:topic) - .where("topics.archetype <> ?", Archetype.private_message) - .where(name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD) + DiscourseSolved::SolvedTopic.joins(:topic).where( + "topics.archetype <> ?", + Archetype.private_message, + ) category_id, include_subcategories = report.add_category_filter if category_id @@ -263,17 +205,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_solved_topics.created_at >= ?", report.start_date) + .where("discourse_solved_solved_topics.created_at <= ?", report.end_date) + .group("DATE(discourse_solved_solved_topics.created_at)") + .order("DATE(discourse_solved_solved_topics.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_solved_topics.created_at >= ?", report.start_date - 30.days) + .where("discourse_solved_solved_topics.created_at <= ?", report.start_date) .count end @@ -282,10 +224,8 @@ after_initialize do condition = <<~SQL EXISTS ( SELECT 1 - FROM topic_custom_fields - WHERE topic_id = topics.id - AND name = '#{::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD}' - AND value IS NOT NULL + FROM discourse_solved_solved_topics + WHERE discourse_solved_solved_topics.topic_id = topics.id ) SQL @@ -315,18 +255,10 @@ after_initialize do add_to_serializer(:post, :can_unaccept_answer) 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" - end - add_to_serializer(:post, :topic_accepted_answer) do - topic&.custom_fields&.[](::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD).present? - end + add_to_serializer(:post, :accepted_answer) { topic&.solved&.answer_post_id == object.id } + add_to_serializer(:post, :topic_accepted_answer) { topic&.solved&.present? } - 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) } on(:filter_auto_bump_topics) do |_category, filters| filters.push( @@ -334,10 +266,8 @@ after_initialize do sql = <<~SQL NOT EXISTS ( SELECT 1 - FROM topic_custom_fields - WHERE topic_id = topics.id - AND name = '#{::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD}' - AND value IS NOT NULL + FROM discourse_solved_solved_topics + WHERE discourse_solved_solved_topics.topic_id = topics.id ) SQL @@ -390,7 +320,7 @@ after_initialize do add_to_class(:composer_messages_finder, :check_topic_is_solved) do return if !SiteSetting.solved_enabled || SiteSetting.disable_solved_education_message return if !replying? || @topic.blank? || @topic.private_message? - return if @topic.custom_fields[::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD].blank? + return if @topic.solved.nil? { id: "solved_topic", @@ -402,15 +332,17 @@ after_initialize do } end - register_topic_list_preload_user_ids do |topics, user_ids, topic_list| - answer_post_ids = - TopicCustomField - .select("value::INTEGER") - .where(name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD) + register_topic_list_preload_user_ids do |topics, user_ids| + # [{ topic_id => answer_user_id }, ... ] + topics_with_answer_poster = + DiscourseSolved::SolvedTopic + .joins(:answer_post) .where(topic_id: topics.map(&:id)) - answer_user_ids = Post.where(id: answer_post_ids).pluck(:topic_id, :user_id).to_h - topics.each { |topic| topic.accepted_answer_user_id = answer_user_ids[topic.id] } - user_ids.concat(answer_user_ids.values) + .pluck(:topic_id, "posts.user_id") + .to_h + + topics.each { |topic| topic.accepted_answer_user_id = topics_with_answer_poster[topic.id] } + user_ids.concat(topics_with_answer_poster.values) end DiscourseSolved::RegisterFilters.register(self) diff --git a/spec/fabricators/solved_topic_fabricator.rb b/spec/fabricators/solved_topic_fabricator.rb new file mode 100644 index 0000000..d24e47e --- /dev/null +++ b/spec/fabricators/solved_topic_fabricator.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +Fabricator(:solved_topic, from: DiscourseSolved::SolvedTopic) do + topic + answer_post { Fabricate(:post) } + accepter { Fabricate(:user) } +end diff --git a/spec/integration/solved_spec.rb b/spec/integration/solved_spec.rb index 77bd320..7a18531 100644 --- a/spec/integration/solved_spec.rb +++ b/spec/integration/solved_spec.rb @@ -30,31 +30,21 @@ RSpec.describe "Managing Posts solved status" do fab!(:solvable_tag) { Fabricate(:tag) } fab!(:solved_in_category) do - Fabricate( - :custom_topic, - category: solvable_category, - custom_topic_name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD, - value: "42", - ) + topic = Fabricate(:topic, category: solvable_category) + Fabricate(:solved_topic, topic:, answer_post: Fabricate(:post, topic:)) + topic end fab!(:solved_in_tag) do - Fabricate( - :custom_topic, - tags: [solvable_tag], - custom_topic_name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD, - value: "42", - ) + topic = Fabricate(:topic, tags: [solvable_tag]) + Fabricate(:solved_topic, topic:, answer_post: Fabricate(:post, topic:)) + topic end fab!(:solved_pm) do - Fabricate( - :custom_topic, - category_id: nil, - archetype: Archetype.private_message, - custom_topic_name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD, - value: "42", - ) + topic = Fabricate(:topic, archetype: Archetype.private_message, category_id: nil) + Fabricate(:solved_topic, topic:, answer_post: Fabricate(:post, topic:)) + topic end fab!(:unsolved_in_category) { Fabricate(:topic, category: solvable_category) } @@ -136,39 +126,15 @@ RSpec.describe "Managing Posts solved status" do category end fab!(:tag) - fab!(:topic_unsolved) do - Fabricate( - :custom_topic, - user: user, - category: category_enabled, - custom_topic_name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD, - ) - end + fab!(:topic_unsolved) { Fabricate(:topic, user:, category: category_enabled) } fab!(:topic_unsolved_2) { Fabricate(:topic, user: user, tags: [tag]) } fab!(:topic_solved) do - Fabricate( - :custom_topic, - user: user, - category: category_enabled, - custom_topic_name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD, - ) - end - fab!(:topic_disabled_1) do - Fabricate( - :custom_topic, - user: user, - category: category_disabled, - custom_topic_name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD, - ) - end - fab!(:topic_disabled_2) do - Fabricate( - :custom_topic, - user: user, - category: category_disabled, - custom_topic_name: "another_custom_field", - ) + topic = Fabricate(:topic, category: category_enabled) + Fabricate(:solved_topic, topic:, answer_post: Fabricate(:post, topic:)) + topic end + fab!(:topic_disabled_1) { Fabricate(:topic, category: category_disabled) } + fab!(:topic_disabled_2) { Fabricate(:topic, category: category_disabled) } fab!(:post_unsolved) { Fabricate(:post, topic: topic_unsolved) } fab!(:post_unsolved_2) { Fabricate(:post, topic: topic_unsolved_2) } fab!(:post_solved) do @@ -258,18 +224,14 @@ 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(topic.solved.answer_post_id).to eq(p1.id) 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.solved.topic_timer).to eq(topic.public_topic_timer) expect(topic.public_topic_timer.execute_at).to eq_time(Time.zone.now + 2.hours) - expect(topic.public_topic_timer.based_on_last_post).to eq(true) end @@ -284,18 +246,14 @@ 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(topic_2.solved.answer_post_id).to eq(post_2.id) 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.solved.topic_timer).to eq(topic_2.public_topic_timer) expect(topic_2.public_topic_timer.execute_at).to eq_time(Time.zone.now + 4.hours) - expect(topic_2.public_topic_timer.based_on_last_post).to eq(true) end @@ -332,7 +290,7 @@ RSpec.describe "Managing Posts solved status" do p1.reload topic.reload - expect(p1.custom_fields["is_accepted_answer"]).to eq("true") + expect(topic.solved.answer_post_id).to eq(p1.id) expect(topic.public_topic_timer).to eq(nil) expect(topic.closed).to eq(true) end @@ -348,7 +306,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(topic.solved.answer_post_id).to eq(p1.id) end it "removes the solution when the post is deleted" do @@ -357,15 +315,12 @@ RSpec.describe "Managing Posts solved status" do post "/solution/accept.json", params: { id: reply.id } expect(response.status).to eq(200) - reply.reload - expect(reply.custom_fields["is_accepted_answer"]).to eq("true") - expect(reply.topic.custom_fields["accepted_answer_post_id"].to_i).to eq(reply.id) + expect(topic.solved.answer_post_id).to eq(reply.id) PostDestroyer.new(Discourse.system_user, reply).destroy + reply.topic.reload - reply.reload - expect(reply.custom_fields["is_accepted_answer"]).to eq(nil) - expect(reply.topic.custom_fields["accepted_answer_post_id"]).to eq(nil) + expect(topic.solved).to be(nil) end it "does not allow you to accept a whisper" do @@ -395,6 +350,7 @@ RSpec.describe "Managing Posts solved status" do before do SiteSetting.solved_topics_auto_close_hours = 2 DiscourseSolved.accept_answer!(p1, user) + topic.reload end it "should unmark the post as solved" do @@ -405,12 +361,13 @@ 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.topic.custom_fields["accepted_answer_post_id"]).to eq(nil) + expect(topic.solved).to be(nil) end end it "triggers a webhook" do + DiscourseSolved.accept_answer!(p1, user) + Fabricate(:solved_web_hook) post "/solution/unaccept.json", params: { id: p1.id } @@ -466,8 +423,9 @@ RSpec.describe "Managing Posts solved status" do expect(p1.topic.assignment.status).to eq("New") DiscourseSolved.accept_answer!(p1, user) + topic.reload - expect(p1.reload.custom_fields["is_accepted_answer"]).to eq("true") + expect(topic.solved.answer_post_id).to eq(p1.id) expect(p1.topic.assignment.reload.status).to eq("Done") end @@ -482,7 +440,6 @@ 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.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..608f305 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -45,10 +45,7 @@ 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(:solved_topic, topic: topic, answer_post: p2) get "/t/#{topic.slug}/#{topic.id}" @@ -68,10 +65,7 @@ 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(:solved_topic, topic: topic, answer_post: p2) get "/t/#{topic.slug}/#{topic.id}.json" @@ -87,10 +81,7 @@ 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(:solved_topic, topic: topic, answer_post: p2) get "/t/#{topic.slug}/#{topic.id}.json" @@ -106,10 +97,7 @@ 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(:solved_topic, topic: topic, answer_post: p2) get "/t/#{topic.slug}/#{topic.id}" @@ -120,10 +108,7 @@ RSpec.describe TopicsController do another_tag = Fabricate(:tag) DiscourseTagging.add_or_create_tags_by_name(topic, [another_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(:solved_topic, topic: topic, answer_post: p2) get "/t/#{topic.slug}/#{topic.id}" diff --git a/spec/serializers/topic_answer_mixin_spec.rb b/spec/serializers/topic_answer_mixin_spec.rb index 94ac0c2..489e24a 100644 --- a/spec/serializers/topic_answer_mixin_spec.rb +++ b/spec/serializers/topic_answer_mixin_spec.rb @@ -7,10 +7,7 @@ describe DiscourseSolved::TopicAnswerMixin do let(:post) { Fabricate(:post, topic: topic) } let(:guardian) { Guardian.new } - before do - topic.custom_fields["accepted_answer_post_id"] = post.id - topic.save_custom_fields - end + before { Fabricate(:solved_topic, topic: topic, answer_post: post) } it "should have true for `has_accepted_answer` field in each serializer" do [ diff --git a/spec/serializers/user_card_serializer_spec.rb b/spec/serializers/user_card_serializer_spec.rb index 5c93b44..b4886ba 100644 --- a/spec/serializers/user_card_serializer_spec.rb +++ b/spec/serializers/user_card_serializer_spec.rb @@ -12,6 +12,7 @@ describe UserCardSerializer do post1 = Fabricate(:post, user: user) DiscourseSolved.accept_answer!(post1, Discourse.system_user) + post1.topic.reload expect(serializer.as_json[:accepted_answers]).to eq(1) post2 = Fabricate(:post, user: user)