From beab626d4be92480d0821f09e9917bba31115ea7 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 11:05:21 +0300 Subject: [PATCH 01/12] Add a journal cause for labels deleted by an admin The label row is gone when the journals are written, so the cause carries the name alongside the id. See COMMS-1067. --- app/models/journal.rb | 3 ++ .../journal/caused_by_label_deletion.rb | 34 +++++++++++++++++++ config/locales/en.yml | 2 ++ lib/open_project/journal_formatter/cause.rb | 8 +++++ .../journal_formatter/cause_spec.rb | 26 ++++++++++++++ 5 files changed, 73 insertions(+) create mode 100644 app/models/journal/caused_by_label_deletion.rb diff --git a/app/models/journal.rb b/app/models/journal.rb index 4d43339bdd80..48505b351917 100644 --- a/app/models/journal.rb +++ b/app/models/journal.rb @@ -89,6 +89,8 @@ class Journal < ApplicationRecord status_name status_id status_changes + label_id + label_name meeting_id source_meeting_id message_id @@ -107,6 +109,7 @@ class Journal < ApplicationRecord default_attribute_written forum_message import + label_deleted progress_mode_changed_to_status_based status_changed system_update diff --git a/app/models/journal/caused_by_label_deletion.rb b/app/models/journal/caused_by_label_deletion.rb new file mode 100644 index 000000000000..47353d2af27b --- /dev/null +++ b/app/models/journal/caused_by_label_deletion.rb @@ -0,0 +1,34 @@ +# 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class Journal::CausedByLabelDeletion < CauseOfChange::Base + def initialize(label_id:, label_name:) + super("label_deleted", "label_id" => label_id, "label_name" => label_name) + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 7e436b902052..f9670c716ae7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3793,6 +3793,7 @@ en: header: "changes by %{author}" migrated: "from Jira" set_with_diff: "%{field} set (%{link})" + label_deleted: "because the label '%{label_name}' was deleted" meeting_cancelled: "(cancelled)" meeting_deleted: "(deleted)" meeting_series_backlog_html: "backlog for meeting series %{link}" @@ -3846,6 +3847,7 @@ en: default_attribute_written: "Read-only attributes written" forum_message_html: "Created from forum message %{message_information}" import: "Imported" + label_deleted: "Label removed" meeting_agenda_item_added_html: "Added to meeting %{meeting_title_information}" meeting_agenda_item_added_template_html: "Added to %{meeting_title_information}" meeting_agenda_item_discussed_html: "Discussed in meeting %{meeting_title_information}" diff --git a/lib/open_project/journal_formatter/cause.rb b/lib/open_project/journal_formatter/cause.rb index 3afa2b04fdb9..ae1f451bf218 100644 --- a/lib/open_project/journal_formatter/cause.rb +++ b/lib/open_project/journal_formatter/cause.rb @@ -101,6 +101,8 @@ def cause_description total_percent_complete_mode_changed_to_simple_average_message when "import" import_message + when "label_deleted" + label_deleted_message when "work_package_parent_deleted" I18n.t("journals.cause_descriptions.work_package_parent_deleted") else @@ -128,6 +130,12 @@ def system_update_message html? ? message : Sanitize.fragment(message) end + def label_deleted_message + label_name = html? ? h(cause["label_name"]) : cause["label_name"] + + I18n.t("journals.cause_descriptions.label_deleted", label_name:) + end + def working_days_changed_message(changed_dates) day_changes_messages = changed_dates["working_days"].collect do |day, working| working_day_change_message(day.to_i, working) diff --git a/spec/lib/open_project/journal_formatter/cause_spec.rb b/spec/lib/open_project/journal_formatter/cause_spec.rb index b7bc6fd85ba4..5f19bdb4df49 100644 --- a/spec/lib/open_project/journal_formatter/cause_spec.rb +++ b/spec/lib/open_project/journal_formatter/cause_spec.rb @@ -645,6 +645,32 @@ def render(cause, html:) end end + context "when the change was caused by the deletion of a label" do + subject(:cause) { Journal::CausedByLabelDeletion.new(label_id: 42, label_name: "Foo").to_hash } + + it do + expect(cause).to render_html_variant( + "Label removed because the label 'Foo' was deleted" + ) + end + + it do + expect(cause).to render_raw_variant("Label removed because the label 'Foo' was deleted") + end + + context "when the label name contains HTML (XSS prevention)" do + subject(:cause) do + Journal::CausedByLabelDeletion.new(label_id: 42, label_name: "").to_hash + end + + it "escapes the label name in HTML output" do + expect(cause).to render_html_variant( + a_string_including("<script>alert('x')</script>") + ) + end + end + end + context "when the change was caused by an import" do subject(:cause) do { From 098307df88e52d3542c678090090efa7e7a077b5 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 11:07:49 +0300 Subject: [PATCH 02/12] Journal work packages when an admin deletes their label Writes one journal per affected work package, authored by the admin, with the label name snapshotted into the cause since the label row is gone. The journals are written directly, so no notifications go out for them. See COMMS-1067. --- app/workers/labels/journalize_deletion_job.rb | 62 ++++++++++ .../labels/journalize_deletion_job_spec.rb | 108 ++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 app/workers/labels/journalize_deletion_job.rb create mode 100644 spec/workers/labels/journalize_deletion_job_spec.rb diff --git a/app/workers/labels/journalize_deletion_job.rb b/app/workers/labels/journalize_deletion_job.rb new file mode 100644 index 000000000000..286c14950161 --- /dev/null +++ b/app/workers/labels/journalize_deletion_job.rb @@ -0,0 +1,62 @@ +# 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +# Skips work packages that already carry the deletion journal, so a retried job adds no second entry. +# A journal written within the aggregation window merges into the admin's own preceding edit of that work package. +class Labels::JournalizeDeletionJob < ApplicationJob + queue_with_priority :below_normal + + def perform(work_package_ids:, label_id:, label_name:, user_id:) + user = User.find_by(id: user_id) || DeletedUser.first + cause = Journal::CausedByLabelDeletion.new(label_id:, label_name:) + + User.execute_as(user) do + pending_work_packages(work_package_ids, label_id).find_each(batch_size: 100) do |work_package| + journalize(work_package, user, cause) + end + end + end + + private + + def pending_work_packages(ids, label_id) + already_journalized = Journal + .where(journable_type: WorkPackage.name) + .where("journals.cause @> ?", { type: "label_deleted", label_id: }.to_json) + .select(:journable_id) + + WorkPackage.where(id: ids).where.not(id: already_journalized) + end + + def journalize(work_package, user, cause) + OpenProject::Mutex.with_advisory_lock_transaction(work_package) do + Journals::CreateService.new(work_package, user).call(cause:) + end + end +end diff --git a/spec/workers/labels/journalize_deletion_job_spec.rb b/spec/workers/labels/journalize_deletion_job_spec.rb new file mode 100644 index 000000000000..c3632682239e --- /dev/null +++ b/spec/workers/labels/journalize_deletion_job_spec.rb @@ -0,0 +1,108 @@ +# 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Labels::JournalizeDeletionJob, type: :job, with_settings: { journal_aggregation_time_minutes: 0 } do + shared_let(:admin) { create(:admin) } + + let(:label) { create(:label, name: "Deprecated") } + let(:label_id) { label.id } + let(:label_name) { label.name } + let(:work_packages) do + create_list(:work_package, 2, journals: { 10.minutes.ago => { user: admin } }) + end + let(:user_id) { admin.id } + let(:cause) do + { "type" => "label_deleted", "label_id" => label_id, "label_name" => label_name } + end + + def perform + described_class.perform_now(work_package_ids: work_packages.map(&:id), label_id:, label_name:, user_id:) + end + + before do + work_packages.each do |work_package| + WorkPackages::UpdateService.new(user: admin, model: work_package).call(label_ids: [label.id]).on_failure { raise it.message } + end + label.destroy! + end + + it "writes one journal per work package, authored by the admin, carrying the cause" do + expect { perform }.to change(Journal, :count).by(2) + + work_packages.each do |work_package| + journal = work_package.reload.last_journal + expect(journal.user).to eq(admin) + expect(journal.cause).to eq(cause) + end + end + + it "records the label as removed and renders it as a deleted label" do + perform + + journal = work_packages.first.reload.last_journal + old_ids, new_ids = journal.details["labels"].map { it.to_s.split(",").map(&:to_i) } + expect(old_ids).to include(label_id) + expect(new_ids).not_to include(label_id) + expect(journal.render_detail(["labels", journal.details["labels"]], html: false)) + .to include(I18n.t(:label_deleted_label)) + end + + it "adds no further journal when run again with the same arguments" do + perform + + expect { perform }.not_to change(Journal, :count) + end + + context "when the admin no longer exists" do + let(:user_id) { 0 } + + it "authors the journals as the deleted user" do + perform + + expect(work_packages.first.reload.last_journal.user).to eq(DeletedUser.first) + end + end + + context "when the admin edited the work package within the aggregation window", + with_settings: { journal_aggregation_time_minutes: 5 } do + let(:work_packages) { [create(:work_package, journals: { 10.minutes.ago => { user: admin } })] } + + before do + WorkPackages::UpdateService.new(user: admin, model: work_packages.first).call(subject: "x") + end + + it "merges the cause into that preceding journal" do + expect { perform }.not_to change { work_packages.first.journals.count } + + expect(work_packages.first.reload.last_journal.cause_type).to eq("label_deleted") + end + end +end From 535e9d87e49b71a5253c4c87873e5c641162466f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 11:09:28 +0300 Subject: [PATCH 03/12] Record the removal on each work package when a label is deleted The delete service collects the labeled work packages before the label goes and hands them to the journaling job once the deletion is committed. See COMMS-1067. --- app/services/labels/delete_service.rb | 24 +++++++++++++++++++++ config/locales/en.yml | 2 +- spec/features/admin/labels/manage_spec.rb | 1 + spec/services/labels/delete_service_spec.rb | 12 +++++++++++ 4 files changed, 38 insertions(+), 1 deletion(-) diff --git a/app/services/labels/delete_service.rb b/app/services/labels/delete_service.rb index ce804fe88d90..75593794fb1f 100644 --- a/app/services/labels/delete_service.rb +++ b/app/services/labels/delete_service.rb @@ -29,5 +29,29 @@ module Labels class DeleteService < ::BaseServices::Delete + protected + + def persist(service_result) + @labeled_work_package_ids = labeled_work_package_ids + super + end + + def after_persist(service_result) + enqueue_journalization if @labeled_work_package_ids.any? + super + end + + private + + def labeled_work_package_ids + Labeling.where(label_id: model.id, labelable_type: WorkPackage.name).pluck(:labelable_id) + end + + def enqueue_journalization + Labels::JournalizeDeletionJob.perform_later(work_package_ids: @labeled_work_package_ids, + label_id: model.id, + label_name: model.name, + user_id: user.id) + end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index f9670c716ae7..e5b7b3be8635 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1905,7 +1905,7 @@ en: labels: delete_dialog_component: confirmation: "I understand that this action is not reversible" - description_html: "This will remove the label %{name} from all work packages in all projects." + description_html: "This will remove the label %{name} from all work packages in all projects. The removal is recorded in each work package's activity." heading: "Delete this label?" title: "Delete this label?" dialog_component: diff --git a/spec/features/admin/labels/manage_spec.rb b/spec/features/admin/labels/manage_spec.rb index 98e83dba80ee..51c579371288 100644 --- a/spec/features/admin/labels/manage_spec.rb +++ b/spec/features/admin/labels/manage_spec.rb @@ -114,6 +114,7 @@ def expect_row(label, usage:) end expect_flash(message: "Successful deletion.") + expect(Labels::JournalizeDeletionJob).to have_been_enqueued expect(page).to have_no_test_selector(row_selector(label)) expect(Label.where(id: label.id)).not_to exist expect(Labeling.where(label_id: label.id)).not_to exist diff --git a/spec/services/labels/delete_service_spec.rb b/spec/services/labels/delete_service_spec.rb index 6d700b1b19db..e27cdee9364c 100644 --- a/spec/services/labels/delete_service_spec.rb +++ b/spec/services/labels/delete_service_spec.rb @@ -47,6 +47,17 @@ expect(result).to be_success expect(Label.where(id: label.id)).not_to exist expect(Labeling.where(id: labeling.id)).not_to exist + expect(Labels::JournalizeDeletionJob) + .to have_been_enqueued.with(work_package_ids: [labeling.labelable_id], + label_id: label.id, + label_name: label.name, + user_id: admin.id) + end + + it "enqueues nothing for an unused label" do + instance.call + + expect(Labels::JournalizeDeletionJob).not_to have_been_enqueued end context "with a non-admin user" do @@ -58,6 +69,7 @@ expect(result).to be_failure expect(result.errors.symbols_for(:base)).to include(:error_unauthorized) expect(Label.where(id: label.id)).to exist + expect(Labels::JournalizeDeletionJob).not_to have_been_enqueued end end end From 995026e43069b3a8399e998be49dc756f3931483 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 11:10:50 +0300 Subject: [PATCH 04/12] Pin the raw rendering of a deleted label's name in the cause spec The XSS example only asserted the HTML variant; raw text must keep the name verbatim. --- spec/lib/open_project/journal_formatter/cause_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/lib/open_project/journal_formatter/cause_spec.rb b/spec/lib/open_project/journal_formatter/cause_spec.rb index 5f19bdb4df49..79a5b39d16f8 100644 --- a/spec/lib/open_project/journal_formatter/cause_spec.rb +++ b/spec/lib/open_project/journal_formatter/cause_spec.rb @@ -668,6 +668,10 @@ def render(cause, html:) a_string_including("<script>alert('x')</script>") ) end + + it "does not escape the label name in raw text output" do + expect(cause).to render_raw_variant(a_string_including("")) + end end end From df7ae0504c7cd7d9aa452b24be2ba5cb87ac06d7 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 11:15:30 +0300 Subject: [PATCH 05/12] Check once which work packages already carry the label deletion journal The guard subquery was re-run for every batch and could only scan the whole journals table; one indexed lookup over the given work packages replaces it. --- app/workers/labels/journalize_deletion_job.rb | 18 ++++++++---------- spec/services/labels/delete_service_spec.rb | 12 ++++++++++++ .../labels/journalize_deletion_job_spec.rb | 6 +----- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/app/workers/labels/journalize_deletion_job.rb b/app/workers/labels/journalize_deletion_job.rb index 286c14950161..9603733d2cc0 100644 --- a/app/workers/labels/journalize_deletion_job.rb +++ b/app/workers/labels/journalize_deletion_job.rb @@ -36,22 +36,20 @@ def perform(work_package_ids:, label_id:, label_name:, user_id:) user = User.find_by(id: user_id) || DeletedUser.first cause = Journal::CausedByLabelDeletion.new(label_id:, label_name:) - User.execute_as(user) do - pending_work_packages(work_package_ids, label_id).find_each(batch_size: 100) do |work_package| - journalize(work_package, user, cause) - end + WorkPackage.where(id: pending_ids(work_package_ids, label_id)).find_each(batch_size: 100) do |work_package| + journalize(work_package, user, cause) end end private - def pending_work_packages(ids, label_id) - already_journalized = Journal - .where(journable_type: WorkPackage.name) - .where("journals.cause @> ?", { type: "label_deleted", label_id: }.to_json) - .select(:journable_id) + def pending_ids(ids, label_id) + journalized = Journal + .where(journable_type: WorkPackage.name, journable_id: ids) + .where("journals.cause @> ?", { type: "label_deleted", label_id: }.to_json) + .pluck(:journable_id) - WorkPackage.where(id: ids).where.not(id: already_journalized) + ids - journalized end def journalize(work_package, user, cause) diff --git a/spec/services/labels/delete_service_spec.rb b/spec/services/labels/delete_service_spec.rb index e27cdee9364c..897ea87a1599 100644 --- a/spec/services/labels/delete_service_spec.rb +++ b/spec/services/labels/delete_service_spec.rb @@ -60,6 +60,18 @@ expect(Labels::JournalizeDeletionJob).not_to have_been_enqueued end + context "when the label cannot be destroyed" do + before do + create(:labeling, label:) + allow(label).to receive(:destroy).and_return(false) + end + + it "fails and enqueues nothing" do + expect(instance.call).to be_failure + expect(Labels::JournalizeDeletionJob).not_to have_been_enqueued + end + end + context "with a non-admin user" do let(:instance) { described_class.new(user: create(:user), model: label) } diff --git a/spec/workers/labels/journalize_deletion_job_spec.rb b/spec/workers/labels/journalize_deletion_job_spec.rb index c3632682239e..67df93f97476 100644 --- a/spec/workers/labels/journalize_deletion_job_spec.rb +++ b/spec/workers/labels/journalize_deletion_job_spec.rb @@ -54,7 +54,7 @@ def perform label.destroy! end - it "writes one journal per work package, authored by the admin, carrying the cause" do + it "writes one journal per work package, authored by the admin, recording the label as removed" do expect { perform }.to change(Journal, :count).by(2) work_packages.each do |work_package| @@ -62,10 +62,6 @@ def perform expect(journal.user).to eq(admin) expect(journal.cause).to eq(cause) end - end - - it "records the label as removed and renders it as a deleted label" do - perform journal = work_packages.first.reload.last_journal old_ids, new_ids = journal.details["labels"].map { it.to_s.split(",").map(&:to_i) } From 2fd941b51b957edd5e601a81d43904a4a4432855 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 11:37:51 +0300 Subject: [PATCH 06/12] Keep the label delete dialog copy as designed The epic defines the dialog text without a note about journaling. --- config/locales/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index e5b7b3be8635..f9670c716ae7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1905,7 +1905,7 @@ en: labels: delete_dialog_component: confirmation: "I understand that this action is not reversible" - description_html: "This will remove the label %{name} from all work packages in all projects. The removal is recorded in each work package's activity." + description_html: "This will remove the label %{name} from all work packages in all projects." heading: "Delete this label?" title: "Delete this label?" dialog_component: From 69475195e3ef815ce557eab8a322046db56da37e Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 16:22:05 +0300 Subject: [PATCH 07/12] Let a label deletion cause aggregate with the previous one A cause may now declare that it extends the predecessor's cause; the aggregated journal then carries the merged cause. Deleting several labels in a row leaves one entry per work package listing all of them. See COMMS-1067. --- app/models/journal.rb | 4 +-- .../journal/caused_by_label_deletion.rb | 13 ++++++-- app/services/journals/create_service.rb | 5 +++- app/workers/labels/journalize_deletion_job.rb | 4 +-- lib/open_project/journal_formatter/cause.rb | 2 +- .../journal_formatter/cause_spec.rb | 4 +-- .../work_package_acts_as_journalized_spec.rb | 30 +++++++++++++++++++ .../labels/journalize_deletion_job_spec.rb | 2 +- 8 files changed, 53 insertions(+), 11 deletions(-) diff --git a/app/models/journal.rb b/app/models/journal.rb index 48505b351917..9122ba7f0b99 100644 --- a/app/models/journal.rb +++ b/app/models/journal.rb @@ -89,8 +89,8 @@ class Journal < ApplicationRecord status_name status_id status_changes - label_id - label_name + label_ids + label_names meeting_id source_meeting_id message_id diff --git a/app/models/journal/caused_by_label_deletion.rb b/app/models/journal/caused_by_label_deletion.rb index 47353d2af27b..806f38d74ab5 100644 --- a/app/models/journal/caused_by_label_deletion.rb +++ b/app/models/journal/caused_by_label_deletion.rb @@ -28,7 +28,16 @@ #++ class Journal::CausedByLabelDeletion < CauseOfChange::Base - def initialize(label_id:, label_name:) - super("label_deleted", "label_id" => label_id, "label_name" => label_name) + def initialize(label_ids:, label_names:) + super("label_deleted", "label_ids" => label_ids, "label_names" => label_names) + end + + def aggregatable_with?(predecessor_cause) + predecessor_cause["type"] == type + end + + def aggregate_with(predecessor_cause) + self.class.new(label_ids: predecessor_cause["label_ids"] | additional_attributes["label_ids"], + label_names: predecessor_cause["label_names"] | additional_attributes["label_names"]) end end diff --git a/app/services/journals/create_service.rb b/app/services/journals/create_service.rb index b0d8c915b887..b0a56c7e4033 100644 --- a/app/services/journals/create_service.rb +++ b/app/services/journals/create_service.rb @@ -84,6 +84,9 @@ def aggregatable_predecessor(notes, internal, cause) def create_journal(notes, internal, cause) predecessor = aggregatable_predecessor(notes, internal, cause) + if predecessor && !predecessor.cause.empty? && cause.respond_to?(:aggregate_with) + cause = cause.aggregate_with(predecessor.cause) + end log_journal_creation(predecessor) @@ -611,7 +614,7 @@ def same_user?(predecessor) end def only_one_or_same_cause?(predecessor, cause) - predecessor.cause.empty? || predecessor.cause == cause + predecessor.cause.empty? || predecessor.cause == cause || cause.try(:aggregatable_with?, predecessor.cause) end def only_one_note?(predecessor, notes) diff --git a/app/workers/labels/journalize_deletion_job.rb b/app/workers/labels/journalize_deletion_job.rb index 9603733d2cc0..e4f744caeada 100644 --- a/app/workers/labels/journalize_deletion_job.rb +++ b/app/workers/labels/journalize_deletion_job.rb @@ -34,7 +34,7 @@ class Labels::JournalizeDeletionJob < ApplicationJob def perform(work_package_ids:, label_id:, label_name:, user_id:) user = User.find_by(id: user_id) || DeletedUser.first - cause = Journal::CausedByLabelDeletion.new(label_id:, label_name:) + cause = Journal::CausedByLabelDeletion.new(label_ids: [label_id], label_names: [label_name]) WorkPackage.where(id: pending_ids(work_package_ids, label_id)).find_each(batch_size: 100) do |work_package| journalize(work_package, user, cause) @@ -46,7 +46,7 @@ def perform(work_package_ids:, label_id:, label_name:, user_id:) def pending_ids(ids, label_id) journalized = Journal .where(journable_type: WorkPackage.name, journable_id: ids) - .where("journals.cause @> ?", { type: "label_deleted", label_id: }.to_json) + .where("journals.cause @> ?", { type: "label_deleted", label_ids: [label_id] }.to_json) .pluck(:journable_id) ids - journalized diff --git a/lib/open_project/journal_formatter/cause.rb b/lib/open_project/journal_formatter/cause.rb index ae1f451bf218..ea54be1da827 100644 --- a/lib/open_project/journal_formatter/cause.rb +++ b/lib/open_project/journal_formatter/cause.rb @@ -131,7 +131,7 @@ def system_update_message end def label_deleted_message - label_name = html? ? h(cause["label_name"]) : cause["label_name"] + label_name = html? ? h(cause["label_names"].first) : cause["label_names"].first I18n.t("journals.cause_descriptions.label_deleted", label_name:) end diff --git a/spec/lib/open_project/journal_formatter/cause_spec.rb b/spec/lib/open_project/journal_formatter/cause_spec.rb index 79a5b39d16f8..8eb3bb5945a7 100644 --- a/spec/lib/open_project/journal_formatter/cause_spec.rb +++ b/spec/lib/open_project/journal_formatter/cause_spec.rb @@ -646,7 +646,7 @@ def render(cause, html:) end context "when the change was caused by the deletion of a label" do - subject(:cause) { Journal::CausedByLabelDeletion.new(label_id: 42, label_name: "Foo").to_hash } + subject(:cause) { Journal::CausedByLabelDeletion.new(label_ids: [42], label_names: ["Foo"]).to_hash } it do expect(cause).to render_html_variant( @@ -660,7 +660,7 @@ def render(cause, html:) context "when the label name contains HTML (XSS prevention)" do subject(:cause) do - Journal::CausedByLabelDeletion.new(label_id: 42, label_name: "").to_hash + Journal::CausedByLabelDeletion.new(label_ids: [42], label_names: [""]).to_hash end it "escapes the label name in HTML output" do diff --git a/spec/models/work_package/work_package_acts_as_journalized_spec.rb b/spec/models/work_package/work_package_acts_as_journalized_spec.rb index e03f40996e7e..70b3e0c2d752 100644 --- a/spec/models/work_package/work_package_acts_as_journalized_spec.rb +++ b/spec/models/work_package/work_package_acts_as_journalized_spec.rb @@ -1178,6 +1178,36 @@ def set_observed_in_versions(versions) end end + context "on label deletion causes within aggregation time", with_settings: { journal_aggregation_time_minutes: 5 } do + shared_let(:journable) do + create(:work_package, journals: { 10.minutes.ago => { user: } }) + end + + def journalize(cause) + Journals::CreateService.new(journable, user).call(cause:) + end + + before do + journalize(Journal::CausedByLabelDeletion.new(label_ids: [1], label_names: ["A"])) + end + + it "aggregates consecutive label deletions into one journal listing every label" do + expect { journalize(Journal::CausedByLabelDeletion.new(label_ids: [2], label_names: ["B"])) } + .not_to change { journable.journals.count } + + expect(journable.journals.reload.last.cause) + .to eq("type" => "label_deleted", "label_ids" => [1, 2], "label_names" => %w[A B]) + end + + it "does not aggregate an unrelated cause into the label deletion journal" do + expect { journalize(Journal::CausedByWorkPackageParentDeletion.new) } + .to change { journable.journals.count }.by(1) + + expect(journable.journals.reload[-2].cause) + .to eq("type" => "label_deleted", "label_ids" => [1], "label_names" => ["A"]) + end + end + context "on a comment within aggregation time after a journal with a meeting cause" do let(:meeting_cause) { { "type" => "meeting_agenda_item_added", "meeting_id" => 42 } } diff --git a/spec/workers/labels/journalize_deletion_job_spec.rb b/spec/workers/labels/journalize_deletion_job_spec.rb index 67df93f97476..7007aae5ef57 100644 --- a/spec/workers/labels/journalize_deletion_job_spec.rb +++ b/spec/workers/labels/journalize_deletion_job_spec.rb @@ -40,7 +40,7 @@ end let(:user_id) { admin.id } let(:cause) do - { "type" => "label_deleted", "label_id" => label_id, "label_name" => label_name } + { "type" => "label_deleted", "label_ids" => [label_id], "label_names" => [label_name] } end def perform From ab6966db019ec06cce1ca560408f86fbd0900067 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 16:24:30 +0300 Subject: [PATCH 08/12] Word the label deletion entry as "Label removed: ... was deleted" Lists every label of an aggregated deletion and pluralises the verb. See COMMS-1067. --- config/locales/en.yml | 6 ++++-- lib/open_project/journal_formatter/cause.rb | 5 +++-- .../journal_formatter/cause_spec.rb | 18 ++++++++++++++++-- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index f9670c716ae7..37c5d2a9cde5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3793,7 +3793,9 @@ en: header: "changes by %{author}" migrated: "from Jira" set_with_diff: "%{field} set (%{link})" - label_deleted: "because the label '%{label_name}' was deleted" + label_deleted: + one: "%{label_names} was deleted" + other: "%{label_names} were deleted" meeting_cancelled: "(cancelled)" meeting_deleted: "(deleted)" meeting_series_backlog_html: "backlog for meeting series %{link}" @@ -3847,7 +3849,7 @@ en: default_attribute_written: "Read-only attributes written" forum_message_html: "Created from forum message %{message_information}" import: "Imported" - label_deleted: "Label removed" + label_deleted: "Label removed:" meeting_agenda_item_added_html: "Added to meeting %{meeting_title_information}" meeting_agenda_item_added_template_html: "Added to %{meeting_title_information}" meeting_agenda_item_discussed_html: "Discussed in meeting %{meeting_title_information}" diff --git a/lib/open_project/journal_formatter/cause.rb b/lib/open_project/journal_formatter/cause.rb index ea54be1da827..0abae175c0db 100644 --- a/lib/open_project/journal_formatter/cause.rb +++ b/lib/open_project/journal_formatter/cause.rb @@ -131,9 +131,10 @@ def system_update_message end def label_deleted_message - label_name = html? ? h(cause["label_names"].first) : cause["label_names"].first + names = Array(cause["label_names"]) + label_names = names.map { |name| "'#{html? ? h(name) : name}'" }.join(", ") - I18n.t("journals.cause_descriptions.label_deleted", label_name:) + I18n.t("journals.cause_descriptions.label_deleted", count: names.size, label_names:) end def working_days_changed_message(changed_dates) diff --git a/spec/lib/open_project/journal_formatter/cause_spec.rb b/spec/lib/open_project/journal_formatter/cause_spec.rb index 8eb3bb5945a7..83eea3746b41 100644 --- a/spec/lib/open_project/journal_formatter/cause_spec.rb +++ b/spec/lib/open_project/journal_formatter/cause_spec.rb @@ -650,12 +650,26 @@ def render(cause, html:) it do expect(cause).to render_html_variant( - "Label removed because the label 'Foo' was deleted" + "Label removed: 'Foo' was deleted" ) end it do - expect(cause).to render_raw_variant("Label removed because the label 'Foo' was deleted") + expect(cause).to render_raw_variant("Label removed: 'Foo' was deleted") + end + + context "when several labels were deleted" do + subject(:cause) do + Journal::CausedByLabelDeletion.new( + label_ids: [1, 2, 3], label_names: %w[Frontend Backend Middleware] + ).to_hash + end + + it "lists every label name and pluralises the verb" do + expect(cause).to render_html_variant( + "Label removed: 'Frontend', 'Backend', 'Middleware' were deleted" + ) + end end context "when the label name contains HTML (XSS prevention)" do From 9e8e8614077a68efef1d29c8de74ee6fef379ffc Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 16:27:12 +0300 Subject: [PATCH 09/12] Cover consecutive label deletions merging into one journal entry See COMMS-1067. --- app/workers/labels/journalize_deletion_job.rb | 2 +- .../labels/journalize_deletion_job_spec.rb | 38 +++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/app/workers/labels/journalize_deletion_job.rb b/app/workers/labels/journalize_deletion_job.rb index e4f744caeada..a8addae8a1d3 100644 --- a/app/workers/labels/journalize_deletion_job.rb +++ b/app/workers/labels/journalize_deletion_job.rb @@ -28,7 +28,7 @@ #++ # Skips work packages that already carry the deletion journal, so a retried job adds no second entry. -# A journal written within the aggregation window merges into the admin's own preceding edit of that work package. +# Deletions by the same admin inside the aggregation window merge into one entry listing every label. class Labels::JournalizeDeletionJob < ApplicationJob queue_with_priority :below_normal diff --git a/spec/workers/labels/journalize_deletion_job_spec.rb b/spec/workers/labels/journalize_deletion_job_spec.rb index 7007aae5ef57..14ac7547831d 100644 --- a/spec/workers/labels/journalize_deletion_job_spec.rb +++ b/spec/workers/labels/journalize_deletion_job_spec.rb @@ -35,6 +35,7 @@ let(:label) { create(:label, name: "Deprecated") } let(:label_id) { label.id } let(:label_name) { label.name } + let(:labels_on_work_packages) { [label] } let(:work_packages) do create_list(:work_package, 2, journals: { 10.minutes.ago => { user: admin } }) end @@ -49,7 +50,7 @@ def perform before do work_packages.each do |work_package| - WorkPackages::UpdateService.new(user: admin, model: work_package).call(label_ids: [label.id]).on_failure { raise it.message } + WorkPackages::UpdateService.new(user: admin, model: work_package).call(label_ids: labels_on_work_packages.map(&:id)).on_failure { raise it.message } end label.destroy! end @@ -87,18 +88,41 @@ def perform end end - context "when the admin edited the work package within the aggregation window", - with_settings: { journal_aggregation_time_minutes: 5 } do + context "when the same admin deletes another label right after" do let(:work_packages) { [create(:work_package, journals: { 10.minutes.ago => { user: admin } })] } + let(:other_label) { create(:label, name: "Obsolete") } + let(:labels_on_work_packages) { [label, other_label] } + let(:work_package) { work_packages.first } + + def perform_for(deleted_label) + described_class.perform_now(work_package_ids: [work_package.id], label_id: deleted_label.id, + label_name: deleted_label.name, user_id:) + end before do - WorkPackages::UpdateService.new(user: admin, model: work_packages.first).call(subject: "x") + WorkPackages::UpdateService.new(user: admin, model: work_package).call(subject: "x").on_failure { raise it.message } + with_settings(journal_aggregation_time_minutes: 5) + perform_for(label) + other_label.destroy! + end + + it "merges both labels into one journal entry" do + expect { perform_for(other_label) }.not_to change { work_package.journals.count } + + journal = work_package.reload.last_journal + expect(journal.cause).to eq( + "type" => "label_deleted", + "label_ids" => [label.id, other_label.id], + "label_names" => [label.name, other_label.name] + ) + new_ids = journal.details["labels"].last.to_s.split(",").map(&:to_i) + expect(new_ids).not_to include(label.id, other_label.id) end - it "merges the cause into that preceding journal" do - expect { perform }.not_to change { work_packages.first.journals.count } + it "adds nothing when the first label's job runs again after the merge" do + perform_for(other_label) - expect(work_packages.first.reload.last_journal.cause_type).to eq("label_deleted") + expect { perform_for(label) }.not_to change { work_package.journals.count } end end end From b201ee8fc1df32ae5e336b9a3aea4ba51260835e Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 16:29:37 +0300 Subject: [PATCH 10/12] Give causes a default for aggregating with their predecessor Keeps the journal service free of duck-typing checks. --- app/models/journal/caused_by_label_deletion.rb | 10 ++++++++-- app/services/journals/create_service.rb | 4 +--- .../acts_as_journalized/lib/cause_of_change/base.rb | 8 ++++++++ .../work_package_acts_as_journalized_spec.rb | 4 +++- 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/app/models/journal/caused_by_label_deletion.rb b/app/models/journal/caused_by_label_deletion.rb index 806f38d74ab5..ed0e170095eb 100644 --- a/app/models/journal/caused_by_label_deletion.rb +++ b/app/models/journal/caused_by_label_deletion.rb @@ -37,7 +37,13 @@ def aggregatable_with?(predecessor_cause) end def aggregate_with(predecessor_cause) - self.class.new(label_ids: predecessor_cause["label_ids"] | additional_attributes["label_ids"], - label_names: predecessor_cause["label_names"] | additional_attributes["label_names"]) + self.class.new(label_ids: predecessor_cause["label_ids"] | label_ids, + label_names: predecessor_cause["label_names"] | label_names) end + + private + + def label_ids = additional_attributes["label_ids"] + + def label_names = additional_attributes["label_names"] end diff --git a/app/services/journals/create_service.rb b/app/services/journals/create_service.rb index b0a56c7e4033..18c927ae68e6 100644 --- a/app/services/journals/create_service.rb +++ b/app/services/journals/create_service.rb @@ -84,9 +84,7 @@ def aggregatable_predecessor(notes, internal, cause) def create_journal(notes, internal, cause) predecessor = aggregatable_predecessor(notes, internal, cause) - if predecessor && !predecessor.cause.empty? && cause.respond_to?(:aggregate_with) - cause = cause.aggregate_with(predecessor.cause) - end + cause = cause.aggregate_with(predecessor.cause) if predecessor && cause.try(:aggregatable_with?, predecessor.cause) log_journal_creation(predecessor) diff --git a/lib_static/plugins/acts_as_journalized/lib/cause_of_change/base.rb b/lib_static/plugins/acts_as_journalized/lib/cause_of_change/base.rb index ba34c78f96e9..30c20d61c669 100644 --- a/lib_static/plugins/acts_as_journalized/lib/cause_of_change/base.rb +++ b/lib_static/plugins/acts_as_journalized/lib/cause_of_change/base.rb @@ -47,5 +47,13 @@ def blank? def ==(other) to_hash == other.to_hash end + + def aggregatable_with?(_predecessor_cause) + false + end + + def aggregate_with(_predecessor_cause) + self + end end end diff --git a/spec/models/work_package/work_package_acts_as_journalized_spec.rb b/spec/models/work_package/work_package_acts_as_journalized_spec.rb index 70b3e0c2d752..1dc5f972ecdc 100644 --- a/spec/models/work_package/work_package_acts_as_journalized_spec.rb +++ b/spec/models/work_package/work_package_acts_as_journalized_spec.rb @@ -1178,7 +1178,7 @@ def set_observed_in_versions(versions) end end - context "on label deletion causes within aggregation time", with_settings: { journal_aggregation_time_minutes: 5 } do + context "on consecutive label deletions within aggregation time", with_settings: { journal_aggregation_time_minutes: 5 } do shared_let(:journable) do create(:work_package, journals: { 10.minutes.ago => { user: } }) end @@ -1205,6 +1205,8 @@ def journalize(cause) expect(journable.journals.reload[-2].cause) .to eq("type" => "label_deleted", "label_ids" => [1], "label_names" => ["A"]) + expect(journable.journals.reload.last.cause) + .to eq(Journal::CausedByWorkPackageParentDeletion.new.to_hash) end end From a1fc50e6170f635ac2a35e1174b527f51e56c822 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 16:30:07 +0300 Subject: [PATCH 11/12] Hide the labels detail on entries written for a label deletion The cause line already names the removed labels. See COMMS-1067. --- .../journals/item_component/details.rb | 8 ++++++- .../journals/item_component/details_spec.rb | 23 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/app/components/work_packages/activities_tab/journals/item_component/details.rb b/app/components/work_packages/activities_tab/journals/item_component/details.rb index 723bba06c9c7..65ec45ac8902 100644 --- a/app/components/work_packages/activities_tab/journals/item_component/details.rb +++ b/app/components/work_packages/activities_tab/journals/item_component/details.rb @@ -61,7 +61,13 @@ def journal_details # A formatter may render a change as nothing, so what is worth heading is # what comes out of them rather than what went in. def rendered_details - @rendered_details ||= journal_details.filter_map { |detail| journal.render_detail(detail).presence } + @rendered_details ||= journal_details + .reject { |attribute, _| hidden_by_cause?(attribute) } + .filter_map { |detail| journal.render_detail(detail).presence } + end + + def hidden_by_cause?(attribute) + attribute.to_s == "labels" && journal.cause["type"] == "label_deleted" end def has_details? diff --git a/spec/components/work_packages/activities_tab/journals/item_component/details_spec.rb b/spec/components/work_packages/activities_tab/journals/item_component/details_spec.rb index 75a3dab5a773..f9a1e3f237a9 100644 --- a/spec/components/work_packages/activities_tab/journals/item_component/details_spec.rb +++ b/spec/components/work_packages/activities_tab/journals/item_component/details_spec.rb @@ -75,6 +75,29 @@ end end + context "with a labels change" do + let(:details) { { "labels" => ["1", nil] } } + let(:rendered) { { ["labels", ["1", nil]] => "Labels deleted A" } } + + it "renders the labels line" do + expect(page).to have_text("Labels deleted A") + end + + context "when written for a label deletion" do + let(:cause) { Journal::CausedByLabelDeletion.new(label_ids: [1], label_names: ["A"]).to_hash } + let(:journal) { build_stubbed(:work_package_journal, journable: work_package, user:, version: 2, cause:) } + let(:details) { { "labels" => ["1", nil], cause: [nil, cause] } } + let(:rendered) do + { ["labels", ["1", nil]] => "Labels deleted A", [:cause, [nil, cause]] => "Label removed: 'A' was deleted" } + end + + it "renders the cause but not the labels line", :aggregate_failures do + expect(page).to have_text("Label removed: 'A' was deleted") + expect(page).to have_no_text("Labels deleted A") + end + end + end + # A formatter that renders nothing leaves the entry with no change to show, so # a header would announce an author and a time above an empty entry. context "when every detail renders blank" do From f88a01a4919a67031ea197acd2b6ee8699938926 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 21 Oct 2026 16:33:03 +0300 Subject: [PATCH 12/12] Read the cause type through its accessor and name the aggregation step Use the store accessor for the cause type and extract the predecessor cause aggregation into a named method. --- .../activities_tab/journals/item_component/details.rb | 2 +- app/services/journals/create_service.rb | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/components/work_packages/activities_tab/journals/item_component/details.rb b/app/components/work_packages/activities_tab/journals/item_component/details.rb index 65ec45ac8902..46e0213d3f52 100644 --- a/app/components/work_packages/activities_tab/journals/item_component/details.rb +++ b/app/components/work_packages/activities_tab/journals/item_component/details.rb @@ -67,7 +67,7 @@ def rendered_details end def hidden_by_cause?(attribute) - attribute.to_s == "labels" && journal.cause["type"] == "label_deleted" + attribute.to_s == "labels" && journal.cause_type == "label_deleted" end def has_details? diff --git a/app/services/journals/create_service.rb b/app/services/journals/create_service.rb index 18c927ae68e6..6baeb53a3ba5 100644 --- a/app/services/journals/create_service.rb +++ b/app/services/journals/create_service.rb @@ -84,7 +84,7 @@ def aggregatable_predecessor(notes, internal, cause) def create_journal(notes, internal, cause) predecessor = aggregatable_predecessor(notes, internal, cause) - cause = cause.aggregate_with(predecessor.cause) if predecessor && cause.try(:aggregatable_with?, predecessor.cause) + cause = aggregate_cause(predecessor, cause) log_journal_creation(predecessor) @@ -615,6 +615,12 @@ def only_one_or_same_cause?(predecessor, cause) predecessor.cause.empty? || predecessor.cause == cause || cause.try(:aggregatable_with?, predecessor.cause) end + def aggregate_cause(predecessor, cause) + return cause unless predecessor && cause.try(:aggregatable_with?, predecessor.cause) + + cause.aggregate_with(predecessor.cause) + end + def only_one_note?(predecessor, notes) predecessor.notes.empty? || notes.empty? end