diff --git a/app/models/journal.rb b/app/models/journal.rb index 82ec32d35325..004e2ea97d25 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_default_attribute_written.rb b/app/models/journal/caused_by_default_attribute_written.rb new file mode 100644 index 000000000000..08a5ff5ec107 --- /dev/null +++ b/app/models/journal/caused_by_default_attribute_written.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::CausedByDefaultAttributeWritten < CauseOfChange::Base + def initialize + super("default_attribute_written") + end +end 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..6af862bef728 --- /dev/null +++ b/app/models/journal/caused_by_label_deletion.rb @@ -0,0 +1,53 @@ +# 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 + + # Labels deleted in quick succession by the same user are merged into the same cause + def merge_into(predecessor_cause) + return unless label_deletion?(predecessor_cause) + + merged = names_by_id(predecessor_cause).merge(names_by_id(to_hash)) + + self.class.new(label_ids: merged.keys, label_names: merged.values) + end + + private + + def label_deletion?(cause) + cause["type"] == type + end + + def names_by_id(cause) + cause["label_ids"].zip(cause["label_names"]).to_h + end +end diff --git a/app/services/journals/create_service.rb b/app/services/journals/create_service.rb index 9e6916315248..e1ea2775faac 100644 --- a/app/services/journals/create_service.rb +++ b/app/services/journals/create_service.rb @@ -70,7 +70,7 @@ def call(notes: "", internal: false, cause: CauseOfChange::NoCause.new) # # * The journalizing happens within the configured aggregation time # * The journalizing is carried out by the same user - # * The cause is identical + # * The cause is identical or extends the predecessor's cause # * ONLY one note exists between the predecessor and the journal to be created # * The predecessor and the journal to be created have the same restriction # @@ -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.mergeable_into?(predecessor.cause) + end + + def aggregate_cause(predecessor, cause) + return cause unless predecessor + + cause.merge_into(predecessor.cause) || 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..8cba8f627294 100644 --- a/app/services/labels/delete_service.rb +++ b/app/services/labels/delete_service.rb @@ -29,5 +29,24 @@ module Labels class DeleteService < ::BaseServices::Delete + protected + + def persist(service_result) + ids = labeled_work_package_ids + super.tap { |result| enqueue_journalization(ids) if result.success? && ids.any? } + end + + private + + def labeled_work_package_ids + Labeling.where(label_id: model.id, labelable_type: WorkPackage.name).pluck(:labelable_id) + end + + def enqueue_journalization(work_package_ids) + Labels::JournalizeDeletionJob.perform_later(work_package_ids:, + label_id: model.id, + label_name: model.name, + user_id: user.id) + end end end diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 7759957adfe3..3376e7c80a51 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -549,8 +549,6 @@ def parent_due_later_than_start? def set_cause_for_readonly_attributes return unless work_package.changes.keys.intersect?(%w(created_at updated_at author)) - work_package.journal_cause = { - "type" => "default_attribute_written" - } + work_package.journal_cause = Journal::CausedByDefaultAttributeWritten.new 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..12404b64bf36 --- /dev/null +++ b/app/workers/labels/journalize_deletion_job.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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +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: work_package_ids).find_each(batch_size: 100) do |work_package| + journalize(work_package, user, cause, label_id) + end + end + + private + + def journalize(work_package, user, cause, label_id) + OpenProject::Mutex.with_advisory_lock_transaction(work_package) do + next if journalized?(work_package, label_id) + + Journals::CreateService.new(work_package, user).call(cause:) + end + end + + def journalized?(work_package, label_id) + work_package.journals.exists?(["journals.cause @> ?", { type: "label_deleted", label_ids: [label_id] }.to_json]) + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 7f32aae2c3dd..ea6535f5138a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3917,6 +3917,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}" @@ -3971,6 +3974,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..76ee1adaf58f 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,15 @@ def system_update_message html? ? message : Sanitize.fragment(message) end + def label_deleted_message + names = cause["label_names"] + return if names.blank? + + 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/open_project/journal_formatter/labels.rb b/lib/open_project/journal_formatter/labels.rb index ce89795284fa..0bee462915e1 100644 --- a/lib/open_project/journal_formatter/labels.rb +++ b/lib/open_project/journal_formatter/labels.rb @@ -29,8 +29,23 @@ #++ class OpenProject::JournalFormatter::Labels < OpenProject::JournalFormatter::JoinedAssociation + def render(key, values, options = { html: true }) + return if only_deleted_labels_removed?(values) + + super + end + private + def only_deleted_labels_removed?(values) + return false unless @journal.cause_type == "label_deleted" + + old_ids, new_ids = values.map { |value| value.to_s.split(",").map(&:to_i) } + added = new_ids - old_ids + removed = old_ids - new_ids + added.empty? && removed.to_set == Array(@journal.cause_label_ids).to_set(&:to_i) + end + def name_or_placeholder(object) return I18n.t(:label_deleted_label) if object.nil? 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..fd28a5bc2f7c 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 merge_into(_predecessor_cause) + nil + end + + def mergeable_into?(predecessor_cause) + !merge_into(predecessor_cause).nil? + 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..de4bf66cdf73 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]] => nil, [: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/api/v3/activities/activity_representer_rendering_spec.rb b/spec/lib/api/v3/activities/activity_representer_rendering_spec.rb index 0496773f1f6c..47d8cf63a862 100644 --- a/spec/lib/api/v3/activities/activity_representer_rendering_spec.rb +++ b/spec/lib/api/v3/activities/activity_representer_rendering_spec.rb @@ -160,6 +160,24 @@ expect(subject).to have_json_path("details/#{x}/html") end end + + context "when a label deletion cause already explains the removed label" do + let(:label) { build_stubbed(:label) } + let(:journal) do + build_stubbed(:work_package_journal, + version: 2, + notes: "", + user: other_user, + cause: Journal::CausedByLabelDeletion.new(label_ids: [label.id], + label_names: [label.name]).to_hash).tap do |journal| + allow(journal).to receive(:get_changes).and_return("labels" => [label.id.to_s, nil]) + end + end + + it "omits the labels detail" do + expect(subject).to have_json_size(0).at_path("details") + end + end end end diff --git a/spec/lib/open_project/journal_formatter/cause_spec.rb b/spec/lib/open_project/journal_formatter/cause_spec.rb index 2c44cb50a5f7..6bd70a55d645 100644 --- a/spec/lib/open_project/journal_formatter/cause_spec.rb +++ b/spec/lib/open_project/journal_formatter/cause_spec.rb @@ -696,6 +696,58 @@ 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 pluralizes the verb" do + expect(cause).to render_html_variant( + "Label removed: 'Frontend', 'Backend', 'Middleware' were deleted" + ) + end + end + + context "when no label names are recorded" do + subject(:cause) { Journal::CausedByLabelDeletion.new(label_ids: [], label_names: []).to_hash } + + it "renders only the caption" do + expect(cause).to render_raw_variant("Label removed: ") + 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/lib/open_project/journal_formatter/labels_spec.rb b/spec/lib/open_project/journal_formatter/labels_spec.rb index 8f528ac37b8e..87282eaf116f 100644 --- a/spec/lib/open_project/journal_formatter/labels_spec.rb +++ b/spec/lib/open_project/journal_formatter/labels_spec.rb @@ -88,6 +88,36 @@ end end + context "when the journal was written for a label deletion" do + let(:cause) { Journal::CausedByLabelDeletion.new(label_ids: [label.id], label_names: [label.name]).to_hash } + let(:journal) { build_stubbed(:work_package_journal, journable: work_package, cause:) } + + it "renders nothing when only that label was removed" do + expect(instance.render(:labels, [label.id.to_s, nil])).to be_nil + end + + it "renders the change when another label was removed as well" do + expect(instance.render(:labels, ["#{label.id},#{other_label.id}", nil])) + .to eq(I18n.t(:text_journal_deleted, label: label_text, old: "A, B")) + end + + it "renders the change when another label was added as well" do + expect(instance.render(:labels, [label.id.to_s, other_label.id.to_s])) + .to eq(I18n.t(:text_journal_changed_plain, + label: label_text, + linebreak: nil, + old: "A", + new: "B")) + end + end + + context "when the journal has no cause and the same label was removed" do + it "renders the change" do + expect(instance.render(:labels, [label.id.to_s, nil])) + .to eq(I18n.t(:text_journal_deleted, label: label_text, old: "A")) + end + end + context "with html: false" do it "renders plain text" do expect(instance.render(:labels, [label.id.to_s, other_label.id.to_s], html: false)) 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..7216763eea0d 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 @@ -1098,10 +1098,7 @@ def set_observed_in_versions(versions) include_examples "journaled values for", new_values_set: { - "journal_cause" => { - "type" => "The good cause", - "some_reference" => 42 - } + "journal_cause" => CauseOfChange::Base.new("The good cause", "some_reference" => 42) }, expected_values: {}, expected_cause: { @@ -1115,16 +1112,16 @@ def set_observed_in_versions(versions) shared_let(:journable) do create(:work_package, journals: { - 4.minutes.ago => { user:, cause: "XYZ" } + 4.minutes.ago => { user:, cause: { "type" => "import" } } }) end include_examples "journaled values for", new_values_set: { - "journal_cause" => "ABC" + "journal_cause" => CauseOfChange::Base.new("system_update") }, expected_values: {}, - expected_cause: "ABC", + expected_cause: { "type" => "system_update" }, expect_new_journal: true end @@ -1134,20 +1131,20 @@ def set_observed_in_versions(versions) subject: "Initial subject", journals: { 10.minutes.ago => { user: }, - 4.minutes.ago => { user:, cause: "ABC" } + 4.minutes.ago => { user:, cause: { "type" => "system_update" } } }) end # Adding the change to subject here to show that the whole change is aggregated include_examples "journaled values for", new_values_set: { - "journal_cause" => "ABC", + "journal_cause" => CauseOfChange::Base.new("system_update"), "subject" => "Changed subject" }, expected_values: { "subject" => ["Initial subject", "Changed subject"] }, - expected_cause: "ABC", + expected_cause: { "type" => "system_update" }, expect_new_journal: false end @@ -1157,7 +1154,7 @@ def set_observed_in_versions(versions) subject: "Initial subject", journals: { 10.minutes.ago => { user: }, - 4.minutes.ago => { user:, cause: "ABC" } + 4.minutes.ago => { user:, cause: { "type" => "system_update" } } }) end @@ -1174,7 +1171,46 @@ def set_observed_in_versions(versions) journable.subject = "Changed subject" journable.save! - expect(journable.journals.reload[-2].cause).to eq("ABC") + expect(journable.journals.reload[-2].cause).to eq("type" => "system_update") + 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 "keeps one name per deleted label when labels share a name" do + journalize(Journal::CausedByLabelDeletion.new(label_ids: [2], label_names: ["A"])) + + expect(journable.journals.reload.last.cause) + .to eq("type" => "label_deleted", "label_ids" => [1, 2], "label_names" => %w[A A]) + 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 @@ -1219,10 +1255,7 @@ def set_observed_in_versions(versions) new_values_set: { "subject" => "Changed subject", "journal_notes" => "Some notes", - "journal_cause" => { - "type" => "The good cause", - "some_reference" => 42 - } + "journal_cause" => CauseOfChange::Base.new("The good cause", "some_reference" => 42) }, expected_values: { "subject" => ["Initial subject", "Changed subject"] @@ -1249,10 +1282,7 @@ def set_observed_in_versions(versions) new_values_set: { "subject" => "Changed subject", "journal_notes" => "Some notes", - "journal_cause" => { - "type" => "The good cause", - "some_reference" => 42 - } + "journal_cause" => CauseOfChange::Base.new("The good cause", "some_reference" => 42) }, expected_values: { "subject" => ["Initial subject", "Changed subject"] @@ -1279,10 +1309,7 @@ def set_observed_in_versions(versions) new_values_set: { "subject" => "Changed subject", "journal_notes" => "Some notes", - "journal_cause" => { - "type" => "The good cause", - "some_reference" => 42 - } + "journal_cause" => CauseOfChange::Base.new("The good cause", "some_reference" => 42) }, expected_values: { "subject" => ["Initial subject", "Changed subject"] @@ -1344,10 +1371,10 @@ def set_observed_in_versions(versions) include_examples "journaled values for", new_values_set: { - "journal_cause" => "ABC" + "journal_cause" => CauseOfChange::Base.new("system_update") }, expected_values: {}, - expected_cause: "ABC", + expected_cause: { "type" => "system_update" }, expect_new_journal: true end @@ -1363,10 +1390,10 @@ def set_observed_in_versions(versions) include_examples "journaled values for", new_values_set: { - "journal_cause" => "ABC" + "journal_cause" => CauseOfChange::Base.new("system_update") }, expected_values: {}, - expected_cause: "ABC", + expected_cause: { "type" => "system_update" }, expect_new_journal: true, expect_journable_update_at_changed: false end 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/services/work_packages/set_attributes_service_spec.rb b/spec/services/work_packages/set_attributes_service_spec.rb index f7847451e191..9f460fadbf97 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -140,6 +140,16 @@ it_behaves_like "service call" end + context "when writing a read-only attribute via attributes" do + let(:call_attributes) { { created_at: 2.days.ago } } + + it "journals the write as a default attribute written cause" do + instance.call(call_attributes) + + expect(work_package.journal_cause).to be_an_instance_of(Journal::CausedByDefaultAttributeWritten) + end + end + # Scenarios specified in https://community.openproject.org/wp/40749 # Just checking that everything is correctly wired up. All other scenarios tested in: # - spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb 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..c8237f2fbc98 --- /dev/null +++ b/spec/workers/labels/journalize_deletion_job_spec.rb @@ -0,0 +1,124 @@ +# 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 and rendering only 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 + + 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 be_nil + 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 within the aggregation time", + with_settings: { journal_aggregation_time_minutes: 5 } 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 } + 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 } + + expect(work_package.reload.last_journal.cause).to eq( + "type" => "label_deleted", + "label_ids" => [label.id, other_label.id], + "label_names" => [label.name, other_label.name] + ) + end + + it "adds no journal 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