FIX: Accepting another answer does not commit (#360)

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).

Author: nattsw

Diff for: app/models/discourse_solved/solved_topic.rb

@@ -7,7 +7,7 @@ class SolvedTopic < ActiveRecord::Base
     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 for: plugin.rb

@@ -33,74 +33,80 @@ def self.accept_answer!(post, acting_user, topic: nil)
       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
-
-          MessageBus.publish("/topic/#{topic.id}", reload_topic: true)
+          end
+
+          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 @@ def self.unaccept_answer!(post, topic: nil)
             topic_id: post.topic_id,
             post_number: post.post_number,
           )&.destroy!
-          solved.topic_timer.destroy! if solved.topic_timer
           solved.destroy!
         end
 

Diff for: spec/integration/solved_spec.rb

@@ -540,6 +540,26 @@
     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)