Skip to content

FIX: Accepting another answer does not commit #360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 7, 2025
Merged

FIX: Accepting another answer does not commit #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/discourse_solved/solved_topic.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
137 changes: 71 additions & 66 deletions plugin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should make it more obvious the only difference between these 2 Notification.create calls is the user_id? Could be a signal to DRY these other attributes out to a common variable as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, but the notification should only be created in the case of the conditions above, I suspect which is why the original author didn't try to optimise further as well.

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)
Expand All @@ -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

Expand Down
20 changes: 20 additions & 0 deletions spec/integration/solved_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down