From c48573961c163c031046b4478711ab1bd9296c6e Mon Sep 17 00:00:00 2001 From: Dombi Attila <83396+dombesz@users.noreply.github.com> Date: Fri, 22 Nov 2024 18:58:43 +0200 Subject: [PATCH] Ensure the Project::LifeCycleStep and Definition type column is readonly and it cannot be changed into an invalid state. The validation added will ensure that if we initialised a Project::Stage object, then we cannot change it's type to "Project::Gate". This could lead to bypassing certain child model validations. --- app/models/project/gate.rb | 2 ++ app/models/project/life_cycle_step.rb | 2 +- app/models/project/life_cycle_step_definition.rb | 2 ++ app/models/project/stage.rb | 2 ++ config/locales/en.yml | 2 ++ spec/models/project/gate_spec.rb | 9 +++++---- spec/models/project/life_cycle_step_definition_spec.rb | 6 ++++++ spec/models/project/life_cycle_step_spec.rb | 1 + spec/models/project/stage_spec.rb | 1 + 9 files changed, 22 insertions(+), 5 deletions(-) diff --git a/app/models/project/gate.rb b/app/models/project/gate.rb index f83dc6ce756d..46ee6fffdb74 100644 --- a/app/models/project/gate.rb +++ b/app/models/project/gate.rb @@ -29,6 +29,8 @@ class Project::Gate < Project::LifeCycleStep alias_attribute :date, :start_date + # This ensures the type cannot be changed after initialising the class. + validates :type, inclusion: { in: %w[Project::Gate], message: :must_be_a_gate } validates :date, presence: true validate :end_date_not_allowed diff --git a/app/models/project/life_cycle_step.rb b/app/models/project/life_cycle_step.rb index dc317a69bc88..dfa4b11efda9 100644 --- a/app/models/project/life_cycle_step.rb +++ b/app/models/project/life_cycle_step.rb @@ -33,7 +33,7 @@ class Project::LifeCycleStep < ApplicationRecord class_name: "Project::LifeCycleStepDefinition" has_many :work_packages, inverse_of: :project_life_cycle_step, dependent: :nullify - attr_readonly :definition_id + attr_readonly :definition_id, :type validates :type, inclusion: { in: %w[Project::Stage Project::Gate], message: :must_be_a_stage_or_gate } diff --git a/app/models/project/life_cycle_step_definition.rb b/app/models/project/life_cycle_step_definition.rb index 767483d8fcef..d511cc284cc4 100644 --- a/app/models/project/life_cycle_step_definition.rb +++ b/app/models/project/life_cycle_step_definition.rb @@ -38,6 +38,8 @@ class Project::LifeCycleStepDefinition < ApplicationRecord validates :name, presence: true validates :type, inclusion: { in: %w[Project::StageDefinition Project::GateDefinition], message: :must_be_a_stage_or_gate } + attr_readonly :type + acts_as_list def initialize(*args) diff --git a/app/models/project/stage.rb b/app/models/project/stage.rb index 4e5f8ef92a4a..b49ca7787b71 100644 --- a/app/models/project/stage.rb +++ b/app/models/project/stage.rb @@ -27,5 +27,7 @@ #++ class Project::Stage < Project::LifeCycleStep + # This ensures the type cannot be changed after initialising the class. + validates :type, inclusion: { in: %w[Project::Stage], message: :must_be_a_stage } validates :start_date, :end_date, presence: true end diff --git a/config/locales/en.yml b/config/locales/en.yml index 34fec6cbe6f0..f316ac0db441 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1118,6 +1118,8 @@ en: attributes: type: must_be_a_stage_or_gate: "must be either Project::Stage or Project::Gate" + must_be_a_stage: "must be a Project::Stage" + must_be_a_gate: "must be a Project::Gate" project/gate: attributes: base: diff --git a/spec/models/project/gate_spec.rb b/spec/models/project/gate_spec.rb index 874a4314ee00..f634c63c1836 100644 --- a/spec/models/project/gate_spec.rb +++ b/spec/models/project/gate_spec.rb @@ -34,17 +34,18 @@ describe "validations" do it { is_expected.to validate_presence_of(:date) } + it { is_expected.to validate_inclusion_of(:type).in_array(["Project::Gate"]).with_message(:must_be_a_gate) } it "is invalid if `end_date` is present" do - gate_with_end_date = build(:project_gate, end_date: Time.zone.today) + subject.end_date = Time.zone.today - expect(gate_with_end_date).not_to be_valid - expect(gate_with_end_date.errors[:base]) + expect(subject).not_to be_valid + expect(subject.errors[:base]) .to include("Cannot assign `end_date` to a Project::Gate") end it "is valid if `end_date` is not present" do - valid_gate = build(:project_gate) + valid_gate = build(:project_gate, end_date: nil) expect(valid_gate).to be_valid end end diff --git a/spec/models/project/life_cycle_step_definition_spec.rb b/spec/models/project/life_cycle_step_definition_spec.rb index 7ccdf6399780..6251817bb2ef 100644 --- a/spec/models/project/life_cycle_step_definition_spec.rb +++ b/spec/models/project/life_cycle_step_definition_spec.rb @@ -33,6 +33,12 @@ expect { described_class.new }.to raise_error(NotImplementedError) end + context "with a Project::StageDefinition" do + subject { create :project_stage_definition } + + it { is_expected.to have_readonly_attribute(:type) } + end + # For more specs see: # - spec/support/shared/project_life_cycle_helpers.rb # - spec/models/project/gate_definition_spec.rb diff --git a/spec/models/project/life_cycle_step_spec.rb b/spec/models/project/life_cycle_step_spec.rb index 1c47422612d5..03722a2cb66a 100644 --- a/spec/models/project/life_cycle_step_spec.rb +++ b/spec/models/project/life_cycle_step_spec.rb @@ -37,6 +37,7 @@ subject { build :project_gate } it { is_expected.to have_readonly_attribute(:definition_id) } + it { is_expected.to have_readonly_attribute(:type) } end # For more specs see: diff --git a/spec/models/project/stage_spec.rb b/spec/models/project/stage_spec.rb index 2ee72a827edf..6fd727a34b27 100644 --- a/spec/models/project/stage_spec.rb +++ b/spec/models/project/stage_spec.rb @@ -35,6 +35,7 @@ describe "validations" do it { is_expected.to validate_presence_of(:start_date) } it { is_expected.to validate_presence_of(:end_date) } + it { is_expected.to validate_inclusion_of(:type).in_array(["Project::Stage"]).with_message(:must_be_a_stage) } it "is valid when `start_date` and `end_date` are present" do valid_stage = build(:project_stage)