Code Maintenance/STC-462: Tidy the activity-tab paginator and centralise filter modes (#23552)

https://community.openproject.org/wp/STC-462

Two readability passes over the work package activity tab, no behaviour change. The paginator's private methods are reordered to follow their call order so the file reads top-down from `#call`, and the three activity filter modes (`:all`, `:only_comments`, `:only_changes`) — until now bare symbols duplicated across the controller, paginator, journal components and the hidden form — move into a single `WorkPackages::ActivitiesTab::Filters` module so the modes have one source of truth and can't drift apart. The diff reaches beyond the paginator into the controller, several components and a form, since that's where the symbols were scattered.

app/components/work_packages/activities_tab/journals/filter_and_sorting_component.html.erb

@@ -24,7 +24,7 @@
           )
           menu.with_item(
             label: t("activities.work_packages.activity_tab.label_activity_show_only_changes"),
-            href: update_filter_work_package_activities_path(work_package, filter: :only_changes),
+            href: update_filter_work_package_activities_path(work_package, filter: WorkPackages::ActivitiesTab::Filters::ONLY_CHANGES),
             content_arguments: {
               data: {
                 turbo_stream: true, action: "click->work-packages--activities-tab--index#setFilterToOnlyChanges",
@@ -35,7 +35,7 @@
           )
           menu.with_item(
             label: t("activities.work_packages.activity_tab.label_activity_show_only_comments"),
-            href: update_filter_work_package_activities_path(work_package, filter: :only_comments),
+            href: update_filter_work_package_activities_path(work_package, filter: WorkPackages::ActivitiesTab::Filters::ONLY_COMMENTS),
             content_arguments: {
               data: {
                 turbo_stream: true, action: "click->work-packages--activities-tab--index#setFilterToOnlyComments",

app/components/work_packages/activities_tab/journals/filter_and_sorting_component.rb

@@ -37,7 +37,7 @@ module WorkPackages
         include OpTurbo::Streamable
         include WorkPackages::ActivitiesTab::SharedHelpers
 
-        def initialize(work_package:, filter: :all)
+        def initialize(work_package:, filter: Filters::ALL)
           super
 
           @work_package = work_package
@@ -49,15 +49,15 @@ module WorkPackages
         attr_reader :work_package, :filter
 
         def show_all?
-          filter == :all
+          filter == Filters::ALL
         end
 
         def show_only_comments?
-          filter == :only_comments
+          filter == Filters::ONLY_COMMENTS
         end
 
         def show_only_changes?
-          filter == :only_changes
+          filter == Filters::ONLY_CHANGES
         end
       end
     end

app/components/work_packages/activities_tab/journals/item_component.rb

@@ -91,7 +91,7 @@ module WorkPackages
         end
 
         def show_comment_container?
-          (journal.notes.present? || noop?) && filter != :only_changes
+          (journal.notes.present? || noop?) && filter != Filters::ONLY_CHANGES
         end
 
         def noop?

app/components/work_packages/activities_tab/journals/item_component/details.html.erb

@@ -11,9 +11,9 @@
       }
     ) do |details_container|
       case filter
-      when :only_comments
+      when WorkPackages::ActivitiesTab::Filters::ONLY_COMMENTS
         render_empty_line(details_container) unless journal.notes.blank? && !journal.noop?
-      when :only_changes
+      when WorkPackages::ActivitiesTab::Filters::ONLY_CHANGES
         if has_details?
           render_details_header(details_container)
           render_details(details_container)

app/components/work_packages/activities_tab/journals/lazy_index_component.rb

@@ -37,7 +37,7 @@ module WorkPackages
         include OpTurbo::Streamable
         include WorkPackages::ActivitiesTab::SharedHelpers
 
-        def initialize(work_package:, journals:, paginator:, filter: :all)
+        def initialize(work_package:, journals:, paginator:, filter: Filters::ALL)
           super
 
           @work_package = work_package
@@ -81,7 +81,7 @@ module WorkPackages
         end
 
         def empty_state?
-          filter == :only_comments && journal_with_notes.empty?
+          filter == Filters::ONLY_COMMENTS && journal_with_notes.empty?
         end
 
         def wp_journals_grouped_emoji_reactions

app/components/work_packages/activities_tab/journals/page_component.rb

@@ -36,7 +36,7 @@ module WorkPackages
         include OpTurbo::Streamable
         include WorkPackages::ActivitiesTab::SharedHelpers
 
-        def initialize(journals:, emoji_reactions:, page:, filter: :all)
+        def initialize(journals:, emoji_reactions:, page:, filter: Filters::ALL)
           super
 
           @journals = journals

app/components/work_packages/activities_tab/journals/revision_component.rb

@@ -67,7 +67,7 @@ module WorkPackages
         attr_reader :changeset, :filter
 
         def render?
-          filter != :only_comments
+          filter != Filters::ONLY_COMMENTS
         end
 
         def user_name

app/components/work_packages/activities_tab/lazy_index_component.rb

@@ -37,7 +37,7 @@ module WorkPackages
       include WorkPackages::ActivitiesTab::SharedHelpers
       include WorkPackages::ActivitiesTab::StimulusControllers
 
-      def initialize(work_package:, journals:, paginator:, last_server_timestamp:, filter: :all, resolved_anchor: nil)
+      def initialize(work_package:, journals:, paginator:, last_server_timestamp:, filter: Filters::ALL, resolved_anchor: nil)
         super
 
         @work_package = work_package

app/controllers/work_packages/activities_tab_controller.rb

@@ -34,6 +34,8 @@ class WorkPackages::ActivitiesTabController < ApplicationController
   include WorkPackages::ActivitiesTab::JournalSortingInquirable
   include WorkPackages::ActivitiesTab::StimulusControllers
 
+  Filters = WorkPackages::ActivitiesTab::Filters
+
   before_action :find_work_package
   before_action :find_journal, only: %i[emoji_actions item_actions edit cancel_edit update toggle_reaction]
   before_action :set_filter
@@ -174,7 +176,7 @@ class WorkPackages::ActivitiesTabController < ApplicationController
     respond_with_turbo_streams
   end
 
-  def toggle_reaction # rubocop:disable Metrics/AbcSize
+  def toggle_reaction
     emoji_reaction_service = EmojiReactions::ToggleEmojiReactionService
       .call(user: User.current,
             reactable: @journal,
@@ -184,7 +186,7 @@ class WorkPackages::ActivitiesTabController < ApplicationController
       update_via_turbo_stream(
         component: WorkPackages::ActivitiesTab::Journals::ItemComponent::Show.new(
           journal: @journal,
-          filter: params[:filter]&.to_sym || :all,
+          filter: @filter,
           grouped_emoji_reactions: grouped_emoji_reactions_for_journal
         )
       )
@@ -245,7 +247,7 @@ class WorkPackages::ActivitiesTabController < ApplicationController
   end
 
   def set_filter
-    @filter = (params[:filter] || params.dig(:journal, :filter))&.to_sym || :all
+    @filter = Filters.cast(params[:filter] || params.dig(:journal, :filter))
   end
 
   def sanitized_journal_notes
@@ -257,7 +259,7 @@ class WorkPackages::ActivitiesTabController < ApplicationController
   end
 
   def handle_successful_create_call(call)
-    if @filter == :only_changes
+    if @filter == Filters::ONLY_CHANGES
       handle_only_changes_filter_on_create
     else
       handle_other_filters_on_create(call)
@@ -265,7 +267,7 @@ class WorkPackages::ActivitiesTabController < ApplicationController
   end
 
   def handle_only_changes_filter_on_create
-    @filter = :all # reset filter
+    @filter = Filters::ALL # reset filter
     # we need to update the whole tab in order to reset the filter
     # as the added journal would not be shown otherwise
     replace_whole_tab
@@ -362,7 +364,7 @@ class WorkPackages::ActivitiesTabController < ApplicationController
                  .journals
                  .internal_visible
 
-    if @filter == :only_comments
+    if @filter == Filters::ONLY_COMMENTS
       journals = journals.where.not(notes: "")
     end
 

app/forms/work_packages/activities_tab/journals/hidden_form.rb

@@ -36,7 +36,7 @@ module WorkPackages::ActivitiesTab::Journals
       journals_form.hidden(name: :filter, value: @filter)
     end
 
-    def initialize(last_server_timestamp:, filter: :all)
+    def initialize(last_server_timestamp:, filter: WorkPackages::ActivitiesTab::Filters::ALL)
       super()
       @last_server_timestamp = last_server_timestamp
       @filter = filter

app/services/work_packages/activities_tab/filters.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+module WorkPackages
+  module ActivitiesTab
+    module Filters
+      ALL           = :all
+      ONLY_COMMENTS = :only_comments
+      ONLY_CHANGES  = :only_changes
+
+      VALUES = [ALL, ONLY_COMMENTS, ONLY_CHANGES].freeze
+
+      # Coerces an incoming filter param (string or symbol) to a known mode,
+      # falling back to ALL for anything unrecognised.
+      def self.cast(value)
+        VALUES.find { it.to_s == value.to_s } || ALL
+      end
+    end
+  end
+end

app/services/work_packages/activities_tab/paginator.rb

@@ -62,12 +62,14 @@ class WorkPackages::ActivitiesTab::Paginator
   include Pagy::Method
   include WorkPackages::ActivitiesTab::JournalSortingInquirable
 
+  Filters = WorkPackages::ActivitiesTab::Filters
+
   attr_reader :work_package, :params, :filter, :resolved_anchor
 
   def initialize(work_package, params = {})
     @work_package = work_package
     @params = params
-    @filter = params[:filter]&.to_sym || :all
+    @filter = Filters.cast(params[:filter])
     @resolved_anchor = nil
   end
 
@@ -90,53 +92,6 @@ class WorkPackages::ActivitiesTab::Paginator
 
   private
 
-  # The activity feed as a single Journal relation, newest-first: the work
-  # package's journals plus the journals written for its changesets — changesets
-  # are journalized, so each carries a journal timestamped with its committed_on.
-  def activities_scope(filter: self.filter)
-    scope = filtered_journals(filter)
-    scope = scope.or(changeset_journals) if include_changesets?(filter)
-    scope.reorder(created_at: :desc, id: :desc)
-  end
-
-  def filtered_journals(filter)
-    case filter
-    when :only_comments then apply_comments_only_filter(visible_journals)
-    when :only_changes then apply_changes_only_filter(visible_journals)
-    else visible_journals
-    end
-  end
-
-  def changeset_journals
-    Journal.where(journable_type: Changeset.name,
-                  journable_id: work_package.changesets.except(:order).select(:id))
-  end
-
-  # Most work packages have no changesets (revisions are a legacy feature), so
-  # the changeset leg is merged in only when one exists — keeping the common
-  # query scoped to the work package's own journals.
-  def include_changesets?(filter)
-    filter != :only_comments && work_package.changesets.exists?
-  end
-
-  def visible_journals
-    work_package.journals.internal_visible
-  end
-
-  # Coerced to a positive integer to match how pagy reads the same option:
-  # a non-positive value floors to one page, avoiding a ZeroDivisionError.
-  def limit
-    (params[:limit] || Pagy::DEFAULT[:limit]).to_i.clamp(1..)
-  end
-
-  def pagy_options
-    {
-      page: params[:page] || 1,
-      limit:,
-      request: { params: }
-    }.compact
-  end
-
   def parse_anchor
     anchor = params[:anchor] # e.g., "comment-78758" (without #)
     return unless anchor
@@ -150,11 +105,45 @@ class WorkPackages::ActivitiesTab::Paginator
   # An unresolvable anchor falls back to page 1; any params[:page] sent
   # alongside is ignored, since the anchor is the explicit navigation intent.
   def pagy_at_anchor(anchor_type, target_record_id)
-    scope = activities_scope(filter: :all)
+    scope = activities_scope(filter: Filters::ALL)
     page = page_for_anchor(scope, anchor_type, target_record_id) || 1
     pagy(:offset, scope, **pagy_options, page:)
   end
 
+  # The activity feed as a single Journal relation, newest-first: the work
+  # package's journals plus the journals written for its changesets — changesets
+  # are journalized, so each carries a journal timestamped with its committed_on.
+  def activities_scope(filter: self.filter)
+    scope = filtered_journals(filter)
+    scope = scope.or(changeset_journals) if include_changesets?(filter)
+    scope.reorder(created_at: :desc, id: :desc)
+  end
+
+  def pagy_options
+    {
+      page: params[:page] || 1,
+      limit:,
+      request: { params: }
+    }.compact
+  end
+
+  # Coerced to a positive integer to match how pagy reads the same option:
+  # a non-positive value floors to one page, avoiding a ZeroDivisionError.
+  def limit
+    (params[:limit] || Pagy::DEFAULT[:limit]).to_i.clamp(1..)
+  end
+
+  # 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))
+    wrapped = wrap_journals(journals.reject { changeset?(it) })
+
+    journals.filter_map { |journal| changeset?(journal) ? changesets[journal.journable_id] : wrapped[journal.id] }
+  end
+
   def page_for_anchor(scope, anchor_type, target_record_id)
     activity_at, anchor_id = locate_anchor(anchor_type, target_record_id)
     return nil unless activity_at && anchor_id
@@ -168,37 +157,24 @@ 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)
-    elsif anchor_type.activity?
-      locate_anchor_by_sequence_version(target_record_id)
+  def filtered_journals(filter)
+    case filter
+    when Filters::ONLY_COMMENTS then apply_comments_only_filter(visible_journals)
+    when Filters::ONLY_CHANGES then apply_changes_only_filter(visible_journals)
+    else visible_journals
     end
   end
 
-  def locate_anchor_by_sequence_version(sequence_version)
-    visible_journals
-      .with_sequence_version
-      .where(ranked: { sequence_version: sequence_version })
-      .pick(:created_at, :id)
+  def changeset_journals
+    Journal.where(journable_type: Changeset.name,
+                  journable_id: work_package.changesets.except(:order).select(:id))
   end
 
-  # 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))
-    wrapped = wrap_journals(journals.reject { changeset?(it) })
-
-    journals.filter_map { |journal| changeset?(journal) ? changesets[journal.journable_id] : wrapped[journal.id] }
+  # Most work packages have no changesets (revisions are a legacy feature), so
+  # the changeset leg is merged in only when one exists — keeping the common
+  # query scoped to the work package's own journals.
+  def include_changesets?(filter)
+    filter != Filters::ONLY_COMMENTS && work_package.changesets.exists?
   end
 
   def page_journals(page_relation)
@@ -207,14 +183,6 @@ class WorkPackages::ActivitiesTab::Paginator
       .to_a
   end
 
-  def wrap_journals(journals)
-    API::V3::Activities::ActivityEagerLoadingWrapper.wrap(journals).index_by(&:id)
-  end
-
-  def changeset?(journal)
-    journal.journable_type == Changeset.name
-  end
-
   def load_changesets(ids)
     return {} if ids.empty?
 
@@ -224,6 +192,29 @@ class WorkPackages::ActivitiesTab::Paginator
       .index_by(&:id)
   end
 
+  def wrap_journals(journals)
+    API::V3::Activities::ActivityEagerLoadingWrapper.wrap(journals).index_by(&:id)
+  end
+
+  def changeset?(journal)
+    journal.journable_type == Changeset.name
+  end
+
+  def locate_anchor(anchor_type, target_record_id)
+    if anchor_type.comment?
+      visible_journals.where(id: target_record_id).pick(:created_at, :id)
+    elsif anchor_type.activity?
+      locate_anchor_by_sequence_version(target_record_id)
+    end
+  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 apply_comments_only_filter(scope)
     scope.where.not(notes: [nil, ""])
   end
@@ -231,4 +222,15 @@ class WorkPackages::ActivitiesTab::Paginator
   def apply_changes_only_filter(scope)
     JournalChangesFilter.apply(scope)
   end
+
+  def visible_journals
+    work_package.journals.internal_visible
+  end
+
+  def locate_anchor_by_sequence_version(sequence_version)
+    visible_journals
+      .with_sequence_version
+      .where(ranked: { sequence_version: sequence_version })
+      .pick(:created_at, :id)
+  end
 end

spec/services/work_packages/activities_tab/filters_spec.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+require "spec_helper"
+
+RSpec.describe WorkPackages::ActivitiesTab::Filters do
+  describe ".cast" do
+    it "returns a known mode given as a symbol unchanged" do
+      expect(described_class.cast(:only_comments)).to eq(:only_comments)
+    end
+
+    it "coerces a known mode given as a string to its symbol" do
+      expect(described_class.cast("only_changes")).to eq(:only_changes)
+    end
+
+    it "falls back to ALL for an unknown value" do
+      expect(described_class.cast("bogus")).to eq(described_class::ALL)
+    end
+
+    it "falls back to ALL for a blank string" do
+      expect(described_class.cast("")).to eq(described_class::ALL)
+    end
+
+    it "falls back to ALL for nil" do
+      expect(described_class.cast(nil)).to eq(described_class::ALL)
+    end
+  end
+end

spec/services/work_packages/activities_tab/paginator_spec.rb

@@ -807,6 +807,27 @@ RSpec.describe WorkPackages::ActivitiesTab::Paginator, with_settings: { journal_
       end
     end
 
+    context "with an unrecognised filter value" do
+      let(:initial_journal) { work_package.journals.find_by(version: 1) }
+      let!(:journal_with_notes) do
+        create(:work_package_journal, user:, notes: "A comment", journable: work_package, version: 2)
+      end
+
+      before do
+        params[:filter] = "bogus"
+      end
+
+      it "normalises the filter to :all" do
+        expect(paginator.filter).to eq(:all)
+      end
+
+      it "returns the unfiltered feed" do
+        _pagy, records = paginator.call
+
+        expect(records.map(&:id)).to include(initial_journal.id, journal_with_notes.id)
+      end
+    end
+
     context "with filter and deep linking" do
       let!(:journal_with_notes) do
         create(:work_package_journal,