explicitly handle deleted project phases

Author: ulferts

config/locales/en.yml

@@ -1673,12 +1673,13 @@ en:
       work_package: "Work packages"
     project_phase:
       activated: "activated"
-      deactivated: "deactivated"
       added_date: "set to %{date}"
       changed_date: "changed from %{from} to %{to}"
-      removed_date: "date deleted %{date}"
-      phase_and_one_gate: "%{phase_message}. %{gate_message}"
+      deactivated: "deactivated"
+      deleted_project_phase: "Deleted project phase"
       phase_and_both_gates: "%{phase_message}. %{start_gate_message}, and %{finish_gate_message}"
+      phase_and_one_gate: "%{phase_message}. %{gate_message}"
+      removed_date: "date deleted %{date}"
 
   # common attributes of all models
   attributes:

lib/open_project/journal_formatter/project_phase_definition.rb

@@ -32,16 +32,21 @@ class OpenProject::JournalFormatter::ProjectPhaseDefinition < JournalFormatter::
   private
 
   def associated_object_name(object)
-    if phase_active?(object)
+    if object.nil?
+      I18n.t(:"activity.project_phase.deleted_project_phase")
+    elsif phase_active?(object)
       super
     else
       "#{super} (#{I18n.t(:label_inactive)})"
     end
   end
 
   def phase_active?(definition)
-    phase = @journal.journable.project.phases.detect { |phase| phase.definition_id == definition.id } if definition
-
-    phase&.active
+    @journal
+      .journable
+      .project
+      .phases
+      .detect { |phase| phase.definition_id == definition.id }
+      &.active
   end
 end

spec/lib/open_project/journal_formatter/project_phase_definition_spec.rb

@@ -34,13 +34,17 @@
   describe "#render" do
     let(:project_phase) { build_stubbed(:project_phase) }
     let(:other_project_phase) { build_stubbed(:project_phase) }
-    let(:project_phases) { [project_phase, other_project_phase] }
+    let(:project_phases) { [project_phase, other_project_phase].compact }
     let(:project) { build_stubbed(:project, phases: project_phases) }
     let(:work_package) { build_stubbed(:work_package, project:) }
     let(:journal) { build_stubbed(:work_package_journal, journable: work_package) }
     let(:instance) { described_class.new(journal) }
 
     before do
+      allow(Project::PhaseDefinition)
+        .to receive(:find_by)
+              .and_return(nil)
+
       project_phases.each do |phase|
         allow(Project::PhaseDefinition)
           .to receive(:find_by)
@@ -173,5 +177,88 @@
 
       it_behaves_like "renders project phase definition change"
     end
+
+    context "when setting a phase whose definition is deleted" do
+      let(:old_value) { nil }
+      let(:new_value) { -1.to_s }
+      let(:expected) do
+        I18n.t(:text_journal_set_to,
+               label: "<strong>Project phase</strong>",
+               value: "<i>#{I18n.t(:"activity.project_phase.deleted_project_phase")}</i>")
+      end
+
+      it_behaves_like "renders project phase definition change"
+    end
+
+    context "when changing between two phases whose definition is deleted" do
+      let(:old_value) { -1.to_s }
+      let(:new_value) { -2.to_s }
+      let(:expected) do
+        I18n.t(:text_journal_changed_plain,
+               label: "<strong>Project phase</strong>",
+               linebreak: nil,
+               old: "<i>#{I18n.t(:"activity.project_phase.deleted_project_phase")}</i>",
+               new: "<i>#{I18n.t(:"activity.project_phase.deleted_project_phase")}</i>")
+      end
+
+      it_behaves_like "renders project phase definition change"
+    end
+
+    context "when deleting a phase whose definition is deleted" do
+      let(:old_value) { -1.to_s }
+      let(:new_value) { nil }
+      let(:expected) do
+        I18n.t(:text_journal_deleted,
+               label: "<strong>Project phase</strong>",
+               old: "<strike><i>#{I18n.t(:"activity.project_phase.deleted_project_phase")}</i></strike>")
+      end
+
+      it_behaves_like "renders project phase definition change"
+    end
+
+    context "when changing between an active and an inactive phase" do
+      let(:project_phase) { build_stubbed(:project_phase, active: true) }
+      let(:other_project_phase) { build_stubbed(:project_phase, active: false) }
+      let(:old_value) { other_project_phase.definition_id.to_s }
+      let(:new_value) { project_phase.definition_id.to_s }
+      let(:expected) do
+        I18n.t(:text_journal_changed_plain,
+               label: "<strong>Project phase</strong>",
+               linebreak: nil,
+               old: "<i>#{other_project_phase.name} (Inactive)</i>",
+               new: "<i>#{project_phase.name}</i>")
+      end
+
+      it_behaves_like "renders project phase definition change"
+    end
+
+    context "when changing between an active and a phase not configured in the project" do
+      let(:project) { build_stubbed(:project, phases: [other_project_phase]) }
+      let(:old_value) { other_project_phase.definition_id.to_s }
+      let(:new_value) { project_phase.definition_id.to_s }
+      let(:expected) do
+        I18n.t(:text_journal_changed_plain,
+               label: "<strong>Project phase</strong>",
+               linebreak: nil,
+               old: "<i>#{other_project_phase.name}</i>",
+               new: "<i>#{project_phase.name} (Inactive)</i>")
+      end
+
+      it_behaves_like "renders project phase definition change"
+    end
+
+    context "when changing between an active and a deleted phase" do
+      let(:old_value) { other_project_phase.definition_id.to_s }
+      let(:new_value) { -1.to_s }
+      let(:expected) do
+        I18n.t(:text_journal_changed_plain,
+               label: "<strong>Project phase</strong>",
+               linebreak: nil,
+               old: "<i>#{other_project_phase.name}</i>",
+               new: "<i>#{I18n.t(:"activity.project_phase.deleted_project_phase")}</i>")
+      end
+
+      it_behaves_like "renders project phase definition change"
+    end
   end
 end