From 2dcc7dd5d1c2da2a3f3c03fa8316da723badcd47 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 2 Jun 2026 14:37:30 +0300 Subject: [PATCH] Remove dead paginate entry point and clarify resolved-anchor recording The Paginator.paginate class method bypassed the instance, discarding the resolved_anchor state the controller reads after .call; it had no callers, so drop it and keep the single new(...).call entry point. Extract the activity anchor side effect into record_resolved_anchor so the intent is explicit at the call site, and pin the server contract with a request spec asserting an unresolvable activity anchor omits the resolved-comment value. --- .../work_packages/activities_tab/paginator.rb | 22 +++++++++---------- .../work_packages/activities_tab_spec.rb | 7 ++++++ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/app/services/work_packages/activities_tab/paginator.rb b/app/services/work_packages/activities_tab/paginator.rb index 6318542aaa2..13f86816118 100644 --- a/app/services/work_packages/activities_tab/paginator.rb +++ b/app/services/work_packages/activities_tab/paginator.rb @@ -62,10 +62,6 @@ class WorkPackages::ActivitiesTab::Paginator include Pagy::Method include WorkPackages::ActivitiesTab::JournalSortingInquirable - def self.paginate(work_package, params = {}) - new(work_package, params).call - end - attr_reader :work_package, :params, :filter, :resolved_anchor def initialize(work_package, params = {}) @@ -163,10 +159,7 @@ class WorkPackages::ActivitiesTab::Paginator activity_at, anchor_id = locate_anchor(anchor_type, target_record_id) return nil unless activity_at && anchor_id - # A comment anchor already matches the DOM's data-anchor-comment-id. An - # activity anchor has no rendered counterpart, so hand the comment id it - # resolved to back to the client to scroll to instead. - @resolved_anchor = anchor_id if anchor_type.activity? + record_resolved_anchor(anchor_type, anchor_id) rows_ahead = scope .where("(journals.created_at, journals.id) > (?, ?)", activity_at, anchor_id) @@ -175,6 +168,13 @@ class WorkPackages::ActivitiesTab::Paginator (rows_ahead / limit) + 1 end + # A comment anchor already matches the DOM's data-anchor-comment-id. An activity + # anchor has no rendered counterpart, so hand the comment id it resolved to back + # to the client to scroll to instead. + def record_resolved_anchor(anchor_type, anchor_id) + @resolved_anchor = anchor_id if anchor_type.activity? + end + def locate_anchor(anchor_type, target_record_id) if anchor_type.comment? visible_journals.where(id: target_record_id).pick(:created_at, :id) @@ -190,9 +190,9 @@ class WorkPackages::ActivitiesTab::Paginator .pick(:created_at, :id) end - # Eager-loads and the sequence version are applied to the page slice here, not - # to activities_scope, so the count query stays off them. Changeset journals - # map back to Changeset records; work package journals go through the wrapper. + # Eager-loads are applied to the page slice here, not to activities_scope, so + # the count query stays off them. Changeset journals map back to Changeset + # records; work package journals go through the wrapper. def load_activities(page_relation) journals = page_journals(page_relation) changesets = load_changesets(journals.select { changeset?(it) }.map(&:journable_id)) diff --git a/spec/requests/work_packages/activities_tab_spec.rb b/spec/requests/work_packages/activities_tab_spec.rb index 8715d475b9c..e975a199e90 100644 --- a/spec/requests/work_packages/activities_tab_spec.rb +++ b/spec/requests/work_packages/activities_tab_spec.rb @@ -73,5 +73,12 @@ RSpec.describe "Work package activities tab", expect(response).to have_http_status(:ok) expect(response.body).not_to include(resolved_value_attribute) end + + it "omits the resolved-comment value for an unresolvable activity anchor" do + get work_package_activities_path(work_package), params: { anchor: "activity-999999" } + + expect(response).to have_http_status(:ok) + expect(response.body).not_to include(resolved_value_attribute) + end end end