From 6e12858bdee65110f5861e0583951f85497f9312 Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Mon, 7 Apr 2025 11:42:36 +0800 Subject: [PATCH] FIX: Accepting another answer does not commit (#360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an answer already exists, clicking "✅ Solution" on another post works, but does not commit. This commit fixes that and also adds a test, and a transaction around accepting a solution (deleting the topic timer, previous user action, etc). --- app/models/discourse_solved/solved_topic.rb | 2 +- plugin.rb | 135 ++++++++++---------- spec/integration/solved_spec.rb | 20 +++ 3 files changed, 91 insertions(+), 66 deletions(-) diff --git a/app/models/discourse_solved/solved_topic.rb b/app/models/discourse_solved/solved_topic.rb index 1c666e5..1b9dd35 100644 --- a/app/models/discourse_solved/solved_topic.rb +++ b/app/models/discourse_solved/solved_topic.rb @@ -7,7 +7,7 @@ module DiscourseSolved 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 + belongs_to :topic_timer, dependent: :destroy validates :topic_id, presence: true validates :answer_post_id, presence: true diff --git a/plugin.rb b/plugin.rb index db582f3..5a849cf 100644 --- a/plugin.rb +++ b/plugin.rb @@ -33,74 +33,80 @@ after_initialize do DistributedMutex.synchronize("discourse_solved_toggle_answer_#{topic.id}") do solved = topic.solved - 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 - - solved ||= - DiscourseSolved::SolvedTopic.new(topic:, answer_post: post, accepter: acting_user) - - notification_data = { - message: "solved.accepted_notification", - display_username: acting_user.username, - topic_title: topic.title, - title: "solved.notification.title", - }.to_json - - unless acting_user.id == post.user_id - Notification.create!( - notification_type: Notification.types[:custom], - user_id: post.user_id, - topic_id: post.topic_id, - post_number: post.post_number, - data: notification_data, - ) - end - - if SiteSetting.notify_on_staff_accept_solved && acting_user.id != topic.user_id - Notification.create!( - notification_type: Notification.types[:custom], - user_id: topic.user_id, - topic_id: post.topic_id, - post_number: post.post_number, - data: notification_data, - ) - end - - auto_close_hours = 0 - if topic&.category.present? - auto_close_hours = topic.category.custom_fields["solved_topics_auto_close_hours"].to_i - auto_close_hours = 175_200 if auto_close_hours > 175_200 # 20 years - end - - auto_close_hours = SiteSetting.solved_topics_auto_close_hours if auto_close_hours == 0 - - if (auto_close_hours > 0) && !topic.closed - topic_timer = - topic.set_or_create_timer( - TopicTimer.types[:silent_close], - nil, - based_on_last_post: true, - duration_minutes: auto_close_hours * 60, + ActiveRecord::Base.transaction do + if previous_accepted_post_id = solved&.answer_post_id + UserAction.where( + action_type: UserAction::SOLVED, + target_post_id: previous_accepted_post_id, + ).destroy_all + solved.destroy! + 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, ) - solved.topic_timer = topic_timer + end - MessageBus.publish("/topic/#{topic.id}", reload_topic: true) + solved = + DiscourseSolved::SolvedTopic.new(topic:, answer_post: post, accepter: acting_user) + + unless acting_user.id == post.user_id + Notification.create!( + notification_type: Notification.types[:custom], + user_id: post.user_id, + topic_id: post.topic_id, + post_number: post.post_number, + data: { + message: "solved.accepted_notification", + display_username: acting_user.username, + topic_title: topic.title, + title: "solved.notification.title", + }.to_json, + ) + end + + if SiteSetting.notify_on_staff_accept_solved && acting_user.id != topic.user_id + Notification.create!( + notification_type: Notification.types[:custom], + user_id: topic.user_id, + topic_id: post.topic_id, + post_number: post.post_number, + data: { + message: "solved.accepted_notification", + display_username: acting_user.username, + topic_title: topic.title, + title: "solved.notification.title", + }.to_json, + ) + end + + auto_close_hours = 0 + if topic&.category.present? + auto_close_hours = topic.category.custom_fields["solved_topics_auto_close_hours"].to_i + auto_close_hours = 175_200 if auto_close_hours > 175_200 # 20 years + end + + auto_close_hours = SiteSetting.solved_topics_auto_close_hours if auto_close_hours == 0 + + if (auto_close_hours > 0) && !topic.closed + topic_timer = + topic.set_or_create_timer( + TopicTimer.types[:silent_close], + nil, + based_on_last_post: true, + duration_minutes: auto_close_hours * 60, + ) + solved.topic_timer = topic_timer + + MessageBus.publish("/topic/#{topic.id}", reload_topic: true) + end + + solved.save! end - solved.save! - if WebHook.active_web_hooks(:accepted_solution).exists? payload = WebHook.generate_payload(:post, post) WebHook.enqueue_solved_hooks(:accepted_solution, post, payload) @@ -127,7 +133,6 @@ after_initialize do topic_id: post.topic_id, post_number: post.post_number, )&.destroy! - solved.topic_timer.destroy! if solved.topic_timer solved.destroy! end diff --git a/spec/integration/solved_spec.rb b/spec/integration/solved_spec.rb index 7a18531..9a65f76 100644 --- a/spec/integration/solved_spec.rb +++ b/spec/integration/solved_spec.rb @@ -540,6 +540,26 @@ RSpec.describe "Managing Posts solved status" do end end + describe "#accept_answer!" do + it "marks the post as the accepted answer correctly" do + user = Fabricate(:user, trust_level: 1) + topic = Fabricate(:topic, user:) + reply1 = Fabricate(:post, topic:, user:, post_number: 2) + reply2 = Fabricate(:post, topic:, user:, post_number: 3) + + DiscourseSolved.accept_answer!(reply1, user) + topic.reload + + expect(topic.solved.answer_post_id).to eq(reply1.id) + expect(topic.solved.topic_timer).to eq(topic.public_topic_timer) + + DiscourseSolved.accept_answer!(reply2, user) + topic.reload + + expect(topic.solved.answer_post_id).to eq(reply2.id) + end + end + describe "user actions stream modifier" do it "correctly list solutions" do t1 = Fabricate(:topic)