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..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 @@ -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/app/models/journal.rb b/app/models/journal.rb index 4d43339bdd80..9122ba7f0b99 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_ids + label_names 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..ed0e170095eb --- /dev/null +++ b/app/models/journal/caused_by_label_deletion.rb @@ -0,0 +1,49 @@ +# 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_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"] | 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 b0d8c915b887..6baeb53a3ba5 100644 --- a/app/services/journals/create_service.rb +++ b/app/services/journals/create_service.rb @@ -84,6 +84,7 @@ def aggregatable_predecessor(notes, internal, cause) def create_journal(notes, internal, cause) predecessor = aggregatable_predecessor(notes, internal, cause) + cause = aggregate_cause(predecessor, cause) log_journal_creation(predecessor) @@ -611,7 +612,13 @@ 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 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) 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/app/workers/labels/journalize_deletion_job.rb b/app/workers/labels/journalize_deletion_job.rb new file mode 100644 index 000000000000..a8addae8a1d3 --- /dev/null +++ b/app/workers/labels/journalize_deletion_job.rb @@ -0,0 +1,60 @@ +# 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. +# 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 + + 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_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) + end + end + + private + + def pending_ids(ids, label_id) + journalized = Journal + .where(journable_type: WorkPackage.name, journable_id: ids) + .where("journals.cause @> ?", { type: "label_deleted", label_ids: [label_id] }.to_json) + .pluck(:journable_id) + + ids - 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/config/locales/en.yml b/config/locales/en.yml index 7e436b902052..37c5d2a9cde5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3793,6 +3793,9 @@ en: header: "changes by %{author}" migrated: "from Jira" set_with_diff: "%{field} set (%{link})" + 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}" @@ -3846,6 +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:" 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..0abae175c0db 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,13 @@ def system_update_message html? ? message : Sanitize.fragment(message) end + def label_deleted_message + names = Array(cause["label_names"]) + label_names = names.map { |name| "'#{html? ? h(name) : name}'" }.join(", ") + + I18n.t("journals.cause_descriptions.label_deleted", count: names.size, label_names:) + 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/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/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 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/lib/open_project/journal_formatter/cause_spec.rb b/spec/lib/open_project/journal_formatter/cause_spec.rb index b7bc6fd85ba4..83eea3746b41 100644 --- a/spec/lib/open_project/journal_formatter/cause_spec.rb +++ b/spec/lib/open_project/journal_formatter/cause_spec.rb @@ -645,6 +645,50 @@ 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_ids: [42], label_names: ["Foo"]).to_hash } + + it do + expect(cause).to render_html_variant( + "Label removed: 'Foo' was deleted" + ) + end + + it do + 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 + subject(:cause) do + Journal::CausedByLabelDeletion.new(label_ids: [42], label_names: [""]).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 + + it "does not escape the label name in raw text output" do + expect(cause).to render_raw_variant(a_string_including("")) + end + end + end + context "when the change was caused by an import" do subject(:cause) 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..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,6 +1178,38 @@ def set_observed_in_versions(versions) end end + 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 + + 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"]) + expect(journable.journals.reload.last.cause) + .to eq(Journal::CausedByWorkPackageParentDeletion.new.to_hash) + 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/services/labels/delete_service_spec.rb b/spec/services/labels/delete_service_spec.rb index 6d700b1b19db..897ea87a1599 100644 --- a/spec/services/labels/delete_service_spec.rb +++ b/spec/services/labels/delete_service_spec.rb @@ -47,6 +47,29 @@ 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 "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 @@ -58,6 +81,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 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..14ac7547831d --- /dev/null +++ b/spec/workers/labels/journalize_deletion_job_spec.rb @@ -0,0 +1,128 @@ +# 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(:labels_on_work_packages) { [label] } + 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_ids" => [label_id], "label_names" => [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: labels_on_work_packages.map(&:id)).on_failure { raise it.message } + end + label.destroy! + end + + 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| + journal = work_package.reload.last_journal + expect(journal.user).to eq(admin) + expect(journal.cause).to eq(cause) + end + + 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 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_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 "adds nothing when the first label's job runs again after the merge" do + perform_for(other_label) + + expect { perform_for(label) }.not_to change { work_package.journals.count } + end + end +end