From f5f374216673ac6b13c81623071784f795d69b6d Mon Sep 17 00:00:00 2001 From: Renato Atilio Date: Thu, 11 Jan 2024 13:37:27 -0300 Subject: [PATCH] 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/controllers/group-activity-posts.js | 4 +- .../javascripts/discourse/app/models/group.js | 4 +- app/controllers/groups_controller.rb | 6 ++- app/models/group.rb | 1 + spec/models/group_spec.rb | 10 +++++ spec/system/group_activity_spec.rb | 39 +++++++++++++++++++ .../pages/group_activity_posts.rb | 22 +++++++++++ 7 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 spec/system/group_activity_spec.rb create mode 100644 spec/system/page_objects/pages/group_activity_posts.rb diff --git a/app/assets/javascripts/discourse/app/controllers/group-activity-posts.js b/app/assets/javascripts/discourse/app/controllers/group-activity-posts.js index 0aa936d8d75..c9fad2389f5 100644 --- a/app/assets/javascripts/discourse/app/controllers/group-activity-posts.js +++ b/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) diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index f55dd81028b..dc8d9061b13 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/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) { diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 5d16b7e4b2c..e5880a05ede 100644 --- a/app/controllers/groups_controller.rb +++ b/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 diff --git a/app/models/group.rb b/app/models/group.rb index 9839d4d4e7d..e1d47537fc7 100644 --- a/app/models/group.rb +++ b/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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index a193ff3f64b..83611eb8ee7 100644 --- a/spec/models/group_spec.rb +++ b/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 diff --git a/spec/system/group_activity_spec.rb b/spec/system/group_activity_spec.rb new file mode 100644 index 00000000000..94be92ecb67 --- /dev/null +++ b/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 diff --git a/spec/system/page_objects/pages/group_activity_posts.rb b/spec/system/page_objects/pages/group_activity_posts.rb new file mode 100644 index 00000000000..58e2ffda576 --- /dev/null +++ b/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