FIX: respect creation date when paginating group activity posts (#24993)

* FIX: respect creation date when paginating group activity posts

There are scenarios where the chronological order of posts doesn't match the order of their IDs. For instance, when moving the first post from one topic or PM to another, a new post (with a higher ID) will be created, but it will retain the original creation time.

This PR changes the group activity page and endpoint to paginate posts using created_at instead of relying on ID ordering.

app/assets/javascripts/discourse/app/controllers/group-activity-posts.js

@@ -20,11 +20,11 @@ export default Controller.extend({
       this.set("loading", true);
       const posts = this.model;
       if (posts && posts.length) {
-        const beforePostId = posts[posts.length - 1].get("id");
+        const before = posts[posts.length - 1].get("created_at");
         const group = this.get("group.model");
 
         let categoryId = this.get("groupActivity.category_id");
-        const opts = { beforePostId, type: this.type, categoryId };
+        const opts = { before, type: this.type, categoryId };
 
         group
           .findPosts(opts)

app/assets/javascripts/discourse/app/models/group.js

@@ -429,8 +429,8 @@ const Group = RestModel.extend({
     const type = opts.type || "posts";
     const data = {};
 
-    if (opts.beforePostId) {
-      data.before_post_id = opts.beforePostId;
+    if (opts.before) {
+      data.before = opts.before;
     }
 
     if (opts.categoryId) {

app/controllers/groups_controller.rb

@@ -194,7 +194,8 @@ class GroupsController < ApplicationController
     group = find_group(:group_id)
     guardian.ensure_can_see_group_members!(group)
 
-    posts = group.posts_for(guardian, params.permit(:before_post_id, :category_id)).limit(20)
+    posts =
+      group.posts_for(guardian, params.permit(:before_post_id, :before, :category_id)).limit(20)
     render_serialized posts.to_a, GroupPostSerializer
   end
 
@@ -202,7 +203,8 @@ class GroupsController < ApplicationController
     group = find_group(:group_id)
     guardian.ensure_can_see_group_members!(group)
 
-    @posts = group.posts_for(guardian, params.permit(:before_post_id, :category_id)).limit(50)
+    @posts =
+      group.posts_for(guardian, params.permit(:before_post_id, :before, :category_id)).limit(50)
     @title =
       "#{SiteSetting.title} - #{I18n.t("rss_description.group_posts", group_name: group.name)}"
     @link = Discourse.base_url

app/models/group.rb

@@ -429,6 +429,7 @@ class Group < ActiveRecord::Base
 
     result = guardian.filter_allowed_categories(result)
     result = result.where("posts.id < ?", opts[:before_post_id].to_i) if opts[:before_post_id]
+    result = result.where("posts.created_at < ?", opts[:before].to_datetime) if opts[:before]
     result.order("posts.created_at desc")
   end
 

spec/models/group_spec.rb

@@ -74,6 +74,16 @@ RSpec.describe Group do
       posts = group.posts_for(Guardian.new)
       expect(posts).not_to include(p)
     end
+
+    it "filters results by datetime using the before parameter" do
+      p1 = Fabricate(:post)
+      p2 = Fabricate(:post, created_at: p1.created_at + 2.minute)
+      group.add(p1.user)
+
+      posts = group.posts_for(Guardian.new, before: p1.created_at + 1.minute)
+      expect(posts).to include(p1)
+      expect(posts).not_to include(p2)
+    end
   end
 
   describe "#builtin" do

spec/system/group_activity_spec.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+describe "Group activity", type: :system do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:group) { Fabricate(:group) }
+
+  context "when on the posts activity page" do
+    let(:posts_page) { PageObjects::Pages::GroupActivityPosts.new }
+
+    before do
+      group.add(user)
+      sign_in(user)
+
+      40.times { Fabricate(:post, user: user, topic: Fabricate(:topic, user: user)) }
+
+      # higher id, older post
+      older_post =
+        Fabricate(:post, user: user, topic: Fabricate(:topic, user: user), raw: "older post")
+      older_post.update!(created_at: 1.day.ago)
+    end
+
+    it "loads and paginates the results by chronology" do
+      posts_page.visit(group)
+
+      expect(posts_page).to have_user_stream_item(count: 20)
+      expect(posts_page).not_to have_content("older post")
+
+      posts_page.scroll_to_last_item
+
+      expect(posts_page).to have_user_stream_item(count: 40)
+      expect(posts_page).not_to have_content("older post")
+
+      posts_page.scroll_to_last_item
+
+      expect(posts_page).to have_content("older post")
+      expect(posts_page).to have_user_stream_item(count: 41)
+    end
+  end
+end

spec/system/page_objects/pages/group_activity_posts.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class GroupActivityPosts < PageObjects::Pages::Base
+      def visit(group)
+        page.visit("/g/#{group.name}/activity/posts")
+        self
+      end
+
+      def has_user_stream_item?(count:)
+        has_css?(".user-stream-item", count: count)
+      end
+
+      def scroll_to_last_item
+        page.execute_script <<~JS
+          document.querySelector('.user-stream-item:last-of-type').scrollIntoView(true);
+        JS
+      end
+    end
+  end
+end