diff --git a/app/services/work_packages/activities_tab/paginator.rb b/app/services/work_packages/activities_tab/paginator.rb index f0c3c7a7497..305a1ccdfec 100644 --- a/app/services/work_packages/activities_tab/paginator.rb +++ b/app/services/work_packages/activities_tab/paginator.rb @@ -114,9 +114,7 @@ class WorkPackages::ActivitiesTab::Paginator # 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) + filtered_journals(filter).reorder(created_at: :desc, id: :desc) end def pagy_options @@ -160,8 +158,8 @@ class WorkPackages::ActivitiesTab::Paginator 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 + when Filters::ONLY_CHANGES then apply_changes_only_filter(with_changesets(visible_journals)) + else with_changesets(visible_journals) end end @@ -170,11 +168,12 @@ class WorkPackages::ActivitiesTab::Paginator 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 != Filters::ONLY_COMMENTS && work_package.changesets.exists? + # Most work packages have no changesets (revisions are a legacy feature). Merging + # the changeset leg in only when one exists keeps the common query scoped to the + # work package's own journals; always merging the empty leg pushes the planner + # onto a slower global scan. + def with_changesets(scope) + work_package.changesets.exists? ? scope.or(changeset_journals) : scope end def page_journals(page_relation) diff --git a/app/services/work_packages/activities_tab/paginator/journal_changes_filter.rb b/app/services/work_packages/activities_tab/paginator/journal_changes_filter.rb index 9eaa3b2f598..e8cc333ac87 100644 --- a/app/services/work_packages/activities_tab/paginator/journal_changes_filter.rb +++ b/app/services/work_packages/activities_tab/paginator/journal_changes_filter.rb @@ -38,32 +38,64 @@ # * Cause metadata (system-triggered changes) # * Attribute/data changes (compares work_package_journals columns with immediate predecessor) # -# This heuristic compares association records with the predecessor journal to detect actual changes, -# not just the presence of snapshot records. +# Each journal's immediate predecessor is resolved once, in a CTE named `journals` that +# shadows the table and exposes `predecessor_id` / `predecessor_data_id` columns. Every +# change-detection branch then reads those columns instead of re-seeking the predecessor. +# Changeset journals carry no detectable attribute history and are always included. class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter class << self - def apply(scope) - sql = <<~SQL.squish - version = 1 - OR (cause IS NOT NULL AND cause != '{}') + # @param membership [ActiveRecord::Relation] the activity feed's journals + # (visible work package journals, optionally unioned with changeset journals) + def apply(membership) + Journal + .with(journals: Arel.sql(enriched_journals_sql(membership))) + .where(OpenProject::SqlSanitization.sanitize(changes_condition_sql)) + end + + private + + # Enrich each journal row with its immediate predecessor's id and data_id. The + # predecessor is the highest version below the current one, located through the + # (journable_type, journable_id, version) index; versions are incremental but may + # have gaps, so the seek matches on `< version` rather than `version - 1`. A LEFT + # JOIN keeps initial (version = 1) journals, whose predecessor columns stay NULL. + def enriched_journals_sql(membership) + <<~SQL.squish + SELECT curr.*, + predecessor.id AS predecessor_id, + predecessor.data_id AS predecessor_data_id + FROM (#{membership.to_sql}) curr + LEFT JOIN LATERAL ( + SELECT p.id, p.data_id + FROM journals p + WHERE p.journable_id = curr.journable_id + AND p.journable_type = curr.journable_type + AND p.version < curr.version + ORDER BY p.version DESC + LIMIT 1 + ) predecessor ON TRUE + SQL + end + + def changes_condition_sql + <<~SQL.squish + journals.journable_type = '#{Changeset.name}' + OR journals.version = 1 + OR (journals.cause IS NOT NULL AND journals.cause != '{}') OR EXISTS (#{attribute_data_changes_condition_sql}) OR EXISTS (#{attachment_changes_condition_sql}) OR EXISTS (#{custom_field_changes_condition_sql}) OR EXISTS (#{file_link_changes_condition_sql}) SQL - - scope.where(OpenProject::SqlSanitization.sanitize(sql)) end - private - def attribute_data_changes_condition_sql <<~SQL.squish SELECT 1 - FROM #{predecessor_lateral_sql} predecessor - INNER JOIN work_package_journals pred_data ON predecessor.data_id = pred_data.id - INNER JOIN work_package_journals curr_data ON journals.data_id = curr_data.id - WHERE (#{data_changes_condition_sql}) + FROM work_package_journals pred_data + INNER JOIN work_package_journals curr_data ON curr_data.id = journals.data_id + WHERE pred_data.id = journals.predecessor_data_id + AND (#{data_changes_condition_sql}) SQL end @@ -95,25 +127,6 @@ class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter ) end - # The immediate predecessor journal, for comparison against the current one. - # Callers alias this as `predecessor`. Seeking the highest version below the - # current one through the (journable_type, journable_id, version) index keeps - # this a single-row lookup per journal; versions are incremental but may have - # gaps, so the seek matches on `< version` rather than `version - 1`. - def predecessor_lateral_sql - <<~SQL.squish - LATERAL ( - SELECT p.id, p.data_id - FROM journals p - WHERE p.journable_id = journals.journable_id - AND p.journable_type = journals.journable_type - AND p.version < journals.version - ORDER BY p.version DESC - LIMIT 1 - ) - SQL - end - def data_changes_condition_sql data_change_columns = Journal::WorkPackageJournal.column_names - ["id"] @@ -158,9 +171,8 @@ class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter <<~SQL.squish SELECT 1 FROM #{table} curr - LEFT JOIN #{predecessor_lateral_sql} predecessor ON TRUE LEFT JOIN #{table} pred - ON pred.journal_id = predecessor.id + ON pred.journal_id = journals.predecessor_id AND #{join_conditions} WHERE curr.journal_id = journals.id AND (#{where_clause}) @@ -176,13 +188,12 @@ class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter <<~SQL.squish SELECT 1 - FROM #{predecessor_lateral_sql} predecessor - INNER JOIN #{table} pred - ON pred.journal_id = predecessor.id + FROM #{table} pred LEFT JOIN #{table} curr ON curr.journal_id = journals.id AND #{join_conditions} - WHERE curr.id IS NULL + WHERE pred.journal_id = journals.predecessor_id + AND curr.id IS NULL SQL 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 af9428ce51a..771a5934906 100644 --- a/spec/services/work_packages/activities_tab/paginator_spec.rb +++ b/spec/services/work_packages/activities_tab/paginator_spec.rb @@ -529,6 +529,26 @@ RSpec.describe WorkPackages::ActivitiesTab::Paginator, with_settings: { journal_ expect(records.map(&:id)).to include(initial_journal.id) end + context "with changesets" do + let(:repository) { create(:repository_subversion, project:) } + let!(:changeset) do + create(:changeset, + repository:, + committed_on: 1.day.ago, + revision: "rev1") + end + + before do + work_package.changesets << changeset + end + + it "includes changesets (commits are activity, not attribute-diffable journals)" do + _pagy, records = paginator.call + + expect(records).to include(changeset) + end + end + context "with attribute changes" do let!(:journal_with_attribute_change) do work_package.subject = "Updated subject"