FIX: Consistently notify lowest post number if post_moved notification generation (#30448)

We currently query the posts table without an order when notifying users of moved posts. Generally the query will return the lowest post number post (b/c ID correlates with post_number in most cases) but not always. This adds an order to the post query in notify_moved_posts job.

Also I removed some if statement nesting with early returns / guard clauses.
This commit is contained in:
Mark VanLandingham 2024-12-23 09:53:43 -06:00 committed by GitHub
parent d69b7da3ca
commit df1fc5bca8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 23 additions and 16 deletions

View File

@ -6,18 +6,21 @@ module Jobs
raise Discourse::InvalidParameters.new(:post_ids) if args[:post_ids].blank? raise Discourse::InvalidParameters.new(:post_ids) if args[:post_ids].blank?
raise Discourse::InvalidParameters.new(:moved_by_id) if args[:moved_by_id].blank? raise Discourse::InvalidParameters.new(:moved_by_id) if args[:moved_by_id].blank?
# Make sure we don't notify the same user twice (in case multiple posts were moved at once.)
users_notified = Set.new
posts = posts =
Post Post
.includes(:user, :topic)
.where(id: args[:post_ids]) .where(id: args[:post_ids])
.where("user_id <> ?", args[:moved_by_id]) .where("user_id <> ?", args[:moved_by_id])
.includes(:user, :topic) .order(post_number: :asc)
if posts.present? return if posts.blank?
moved_by = User.find_by(id: args[:moved_by_id]) moved_by = User.find_by(id: args[:moved_by_id])
# Make sure we don't notify the same user twice (in case multiple posts were moved at once.)
users_notified = Set.new
posts.each do |p| posts.each do |p|
if users_notified.exclude?(p.user_id) next if users_notified.include?(p.user_id)
p.user.notifications.create( p.user.notifications.create(
notification_type: Notification.types[:moved_post], notification_type: Notification.types[:moved_post],
topic_id: p.topic_id, topic_id: p.topic_id,
@ -28,6 +31,4 @@ module Jobs
end end
end end
end end
end
end
end end

View File

@ -29,6 +29,12 @@ RSpec.describe Jobs::NotifyMovedPosts do
}.to change(moved_post_notifications, :count).by(2) }.to change(moved_post_notifications, :count).by(2)
end end
it "notifies on the post with lowest post number" do
Jobs::NotifyMovedPosts.new.execute(post_ids: [p1.id, p3.id], moved_by_id: admin.id)
expect(moved_post_notifications.last.post_number).to eq(p1.post_number)
end
context "when moved by one of the posters" do context "when moved by one of the posters" do
it "create one notifications, because the poster is the mover" do it "create one notifications, because the poster is the mover" do
expect { expect {