FIX: Consistently notify lowest post number if post_moved notificatio…

…n generation (discourse#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.

app/jobs/regular/notify_moved_posts.rb

@@ -6,27 +6,28 @@ def execute(args)
       raise Discourse::InvalidParameters.new(:post_ids) if args[:post_ids].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 =
         Post
+          .includes(:user, :topic)
           .where(id: args[:post_ids])
           .where("user_id <> ?", args[:moved_by_id])
-          .includes(:user, :topic)
-      if posts.present?
-        moved_by = User.find_by(id: args[:moved_by_id])
+          .order(post_number: :asc)
+      return if posts.blank?
+
+      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|
+        next if users_notified.include?(p.user_id)
 
-        posts.each do |p|
-          if users_notified.exclude?(p.user_id)
-            p.user.notifications.create(
-              notification_type: Notification.types[:moved_post],
-              topic_id: p.topic_id,
-              post_number: p.post_number,
-              data: { topic_title: p.topic.title, display_username: moved_by.username }.to_json,
-            )
-            users_notified << p.user_id
-          end
-        end
+        p.user.notifications.create(
+          notification_type: Notification.types[:moved_post],
+          topic_id: p.topic_id,
+          post_number: p.post_number,
+          data: { topic_title: p.topic.title, display_username: moved_by.username }.to_json,
+        )
+        users_notified << p.user_id
       end
     end
   end

spec/jobs/notify_moved_posts_spec.rb

@@ -29,6 +29,12 @@
       }.to change(moved_post_notifications, :count).by(2)
     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
       it "create one notifications, because the poster is the mover" do
         expect {