diff --git a/app/components/work_packages/activities_tab/journals/filter_and_sorting_component.html.erb b/app/components/work_packages/activities_tab/journals/filter_and_sorting_component.html.erb index 82896da1f4e..37128c731a4 100644 --- a/app/components/work_packages/activities_tab/journals/filter_and_sorting_component.html.erb +++ b/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", diff --git a/app/components/work_packages/activities_tab/journals/filter_and_sorting_component.rb b/app/components/work_packages/activities_tab/journals/filter_and_sorting_component.rb index bcaef00f737..b917c9a001c 100644 --- a/app/components/work_packages/activities_tab/journals/filter_and_sorting_component.rb +++ b/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 diff --git a/app/components/work_packages/activities_tab/journals/item_component.rb b/app/components/work_packages/activities_tab/journals/item_component.rb index c6ac51e53b4..2663545c11d 100644 --- a/app/components/work_packages/activities_tab/journals/item_component.rb +++ b/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? diff --git a/app/components/work_packages/activities_tab/journals/item_component/details.html.erb b/app/components/work_packages/activities_tab/journals/item_component/details.html.erb index 25cc575ff2b..a7f63b279a0 100644 --- a/app/components/work_packages/activities_tab/journals/item_component/details.html.erb +++ b/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) diff --git a/app/components/work_packages/activities_tab/journals/lazy_index_component.rb b/app/components/work_packages/activities_tab/journals/lazy_index_component.rb index 71a2c038f66..f4f6dbd7de1 100644 --- a/app/components/work_packages/activities_tab/journals/lazy_index_component.rb +++ b/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 diff --git a/app/components/work_packages/activities_tab/journals/page_component.rb b/app/components/work_packages/activities_tab/journals/page_component.rb index 86a4a612260..631e5556ef4 100644 --- a/app/components/work_packages/activities_tab/journals/page_component.rb +++ b/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 diff --git a/app/components/work_packages/activities_tab/journals/revision_component.rb b/app/components/work_packages/activities_tab/journals/revision_component.rb index ae19b73ddd8..f7de5a343ea 100644 --- a/app/components/work_packages/activities_tab/journals/revision_component.rb +++ b/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 diff --git a/app/components/work_packages/activities_tab/lazy_index_component.rb b/app/components/work_packages/activities_tab/lazy_index_component.rb index 23f9b20db8f..ccceb6a84d9 100644 --- a/app/components/work_packages/activities_tab/lazy_index_component.rb +++ b/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 diff --git a/app/controllers/work_packages/activities_tab_controller.rb b/app/controllers/work_packages/activities_tab_controller.rb index 150c41f4891..9fd3b16fe9a 100644 --- a/app/controllers/work_packages/activities_tab_controller.rb +++ b/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 diff --git a/app/forms/work_packages/activities_tab/journals/hidden_form.rb b/app/forms/work_packages/activities_tab/journals/hidden_form.rb index 448fa38e457..394282b71dd 100644 --- a/app/forms/work_packages/activities_tab/journals/hidden_form.rb +++ b/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 diff --git a/app/services/work_packages/activities_tab/filters.rb b/app/services/work_packages/activities_tab/filters.rb new file mode 100644 index 00000000000..0ab788df02f --- /dev/null +++ b/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 diff --git a/app/services/work_packages/activities_tab/paginator.rb b/app/services/work_packages/activities_tab/paginator.rb index 13f86816118..f0c3c7a7497 100644 --- a/app/services/work_packages/activities_tab/paginator.rb +++ b/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 diff --git a/spec/services/work_packages/activities_tab/filters_spec.rb b/spec/services/work_packages/activities_tab/filters_spec.rb new file mode 100644 index 00000000000..1eadb0dee16 --- /dev/null +++ b/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 diff --git a/spec/services/work_packages/activities_tab/paginator_spec.rb b/spec/services/work_packages/activities_tab/paginator_spec.rb index 860a2ba662b..af9428ce51a 100644 --- a/spec/services/work_packages/activities_tab/paginator_spec.rb +++ b/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,