diff --git a/.rubocop.yml b/.rubocop.yml index b951a30741b5..0f53d5b907d2 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -257,8 +257,10 @@ RSpec/ContextWording: - as - even - for + - given - having - if + - in - 'on' - to - unless diff --git a/app/models/work_package/validations.rb b/app/models/work_package/validations.rb index 7c9fce745f0a..94f1c37475ef 100644 --- a/app/models/work_package/validations.rb +++ b/app/models/work_package/validations.rb @@ -33,7 +33,7 @@ module WorkPackage::Validations validates :subject, :priority, :project, :type, :author, :status, presence: true validates :subject, length: { maximum: 255 } - validates :done_ratio, inclusion: { in: 0..100 } + validates :done_ratio, inclusion: { in: 0..100 }, allow_nil: true validates :estimated_hours, numericality: { allow_nil: true } validates :remaining_hours, numericality: { allow_nil: true, greater_than_or_equal_to: 0 } validates :derived_remaining_hours, numericality: { allow_nil: true, greater_than_or_equal_to: 0 } diff --git a/config/initializers/new_framework_defaults_7_0.rb b/config/initializers/new_framework_defaults_7_0.rb index 863a9f1dad5b..8a10e9e8eab1 100644 --- a/config/initializers/new_framework_defaults_7_0.rb +++ b/config/initializers/new_framework_defaults_7_0.rb @@ -90,7 +90,7 @@ # This default means that all columns will be referenced in INSERT queries # regardless of whether they have a default or not. # Previous versions had true. Rails 7.0+ default is false. -# Rails.application.config.active_record.partial_inserts = false +Rails.application.config.active_record.partial_inserts = false # https://guides.rubyonrails.org/configuring.html#config-action-controller-raise-on-open-redirects # Protect from open redirect attacks in `redirect_back_or_to` and `redirect_to`. diff --git a/db/migrate/20240307094432_change_done_ratio_default_value_to_null.rb b/db/migrate/20240307094432_change_done_ratio_default_value_to_null.rb new file mode 100644 index 000000000000..a8373662d445 --- /dev/null +++ b/db/migrate/20240307094432_change_done_ratio_default_value_to_null.rb @@ -0,0 +1,8 @@ +class ChangeDoneRatioDefaultValueToNull < ActiveRecord::Migration[7.1] + def change + change_column_null :work_packages, :done_ratio, true + change_column_default :work_packages, :done_ratio, from: 0, to: nil + change_column_null :work_package_journals, :done_ratio, true + change_column_default :work_package_journals, :done_ratio, from: 0, to: nil + end +end diff --git a/spec/models/mail_handler_spec.rb b/spec/models/mail_handler_spec.rb index 48b0ef901567..95687275eba4 100644 --- a/spec/models/mail_handler_spec.rb +++ b/spec/models/mail_handler_spec.rb @@ -1088,7 +1088,7 @@ .to be_nil expect(subject.done_ratio) - .to be 0 + .to be_nil expect(subject.priority) .to eql priority_low diff --git a/spec/models/work_package_spec.rb b/spec/models/work_package_spec.rb index e356a9c6fec3..0af23f558c4e 100644 --- a/spec/models/work_package_spec.rb +++ b/spec/models/work_package_spec.rb @@ -333,77 +333,57 @@ def stub_shared_versions(v = nil) is_closed: false, default_done_ratio: 0) end - shared_let(:work_package1) do + shared_let(:work_package_new) do create(:work_package, status: status_new) end - shared_let(:work_package2) do + shared_let(:work_package_assigned) do create(:work_package, - project: work_package1.project, + project: work_package_new.project, status: status_assigned, done_ratio: 30) end describe '#value' do - context 'work package field' do - before { allow(Setting).to receive(:work_package_done_ratio).and_return 'field' } - - context 'work package 1' do - subject { work_package1.done_ratio } - - it { is_expected.to eq(0) } - end - - context 'work package 2' do - subject { work_package2.done_ratio } - - it { is_expected.to eq(30) } + context 'for work-based mode', + with_settings: { work_package_done_ratio: 'field' } do + it 'returns the value from work package field' do + expect(work_package_new.done_ratio).to be_nil + expect(work_package_assigned.done_ratio).to eq(30) end end - context 'work package status' do - before { allow(Setting).to receive(:work_package_done_ratio).and_return 'status' } - - context 'work package 1' do - subject { work_package1.done_ratio } - - it { is_expected.to eq(50) } - end - - context 'work package 2' do - subject { work_package2.done_ratio } - - it { is_expected.to eq(0) } + context 'for status-based mode', + with_settings: { work_package_done_ratio: 'status' } do + it 'uses the % Complete value from the work package status' do + expect(work_package_new.done_ratio).to eq(status_new.default_done_ratio) + expect(work_package_assigned.done_ratio).to eq(status_assigned.default_done_ratio) end end end describe '#update_done_ratio_from_status' do - context 'work package field' do - before do - allow(Setting).to receive(:work_package_done_ratio).and_return 'field' - - work_package1.update_done_ratio_from_status - work_package2.update_done_ratio_from_status - end - + context 'for work-based mode', + with_settings: { work_package_done_ratio: 'field' } do it 'does not update the done ratio' do - expect(work_package1.done_ratio).to eq(0) - expect(work_package2.done_ratio).to eq(30) + expect { work_package_new.update_done_ratio_from_status } + .not_to change { work_package_new[:done_ratio] } + expect { work_package_assigned.update_done_ratio_from_status } + .not_to change { work_package_assigned[:done_ratio] } end end - context 'work package status' do - before do - allow(Setting).to receive(:work_package_done_ratio).and_return 'status' - - work_package1.update_done_ratio_from_status - work_package2.update_done_ratio_from_status - end + context 'for status-based mode', + with_settings: { work_package_done_ratio: 'status' } do + it 'updates the done ratio without saving it' do + expect { work_package_new.update_done_ratio_from_status } + .to change { work_package_new[:done_ratio] } + .from(nil).to(50) + expect { work_package_assigned.update_done_ratio_from_status } + .to change { work_package_assigned[:done_ratio] } + .from(30).to(0) - it 'updates the done ratio' do - expect(work_package1.done_ratio).to eq(50) - expect(work_package2.done_ratio).to eq(0) + expect(work_package_new).to have_changes_to_save 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 233fb9a7e03b..480d8fcee1d3 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -30,6 +30,10 @@ RSpec.describe WorkPackages::SetAttributesService, type: :model do + shared_let(:status_no_pct_complete) { create(:status, default_done_ratio: nil, name: 'no % complete') } + shared_let(:status_50_pct_complete) { create(:status, default_done_ratio: 50, name: '50% complete') } + shared_let(:status_70_pct_complete) { create(:status, default_done_ratio: 70, name: '70% complete') } + let(:today) { Time.zone.today } let(:user) { build_stubbed(:user) } let(:project) do @@ -39,7 +43,7 @@ p end let(:work_package) do - wp = build_stubbed(:work_package, project:) + wp = build_stubbed(:work_package, project:, status: status_no_pct_complete) wp.type = initial_type wp.send(:clear_changes_information) @@ -72,7 +76,7 @@ contract_class: mock_contract) end - shared_examples_for 'service call' do + shared_examples_for 'service call' do |description: nil| subject do allow(work_package) .to receive(:save) @@ -84,7 +88,7 @@ expect(subject).to be_success end - it 'sets the value' do + it description || 'sets the value' do next if !defined?(expected_attributes) || expected_attributes.blank? subject @@ -145,68 +149,112 @@ it_behaves_like 'service call' end - context 'for estimated_hours' do - context 'with the status being used for done_ratio computations', + describe 'deriving remaining work attribute (remaining_hours)' do + context 'in status-based mode', with_settings: { work_package_done_ratio: 'status' } do - context 'when the work package has estimated hours and remaining hours set along with a done ratio' do - let!(:status) { create(:status, default_done_ratio: 50) } - + context 'given a work package with work, remaining work, and status with % complete being set' do before do - work_package.status = status + work_package.status = status_50_pct_complete + work_package.done_ratio = work_package.status.default_done_ratio work_package.estimated_hours = 10.0 work_package.remaining_hours = 5.0 work_package.send(:clear_changes_information) end - context 'and estimated_hours are subsequently unset' do + context 'when work is unset' do let(:call_attributes) { { estimated_hours: nil } } - let(:expected_attributes) { { estimated_hours: nil, remaining_hours: nil, done_ratio: 50 } } + let(:expected_attributes) { { remaining_hours: nil } } - it_behaves_like 'service call' + it_behaves_like 'service call', description: 'unsets remaining work' end - context 'and estimated_hours are subsequently modified' do + context 'when work is modified' do let(:call_attributes) { { estimated_hours: 5.0 } } - let(:expected_attributes) { { estimated_hours: 5.0, remaining_hours: 2.5, done_ratio: 50 } } + let(:expected_attributes) { { remaining_hours: 2.5 } } - it_behaves_like 'service call' + it_behaves_like 'service call', description: 'recomputes remaining work accordingly' end - context 'and the status is subsequently changed' do - let!(:other_status) { create(:status, default_done_ratio: 70) } - let(:call_attributes) { {} } - let(:expected_attributes) { { estimated_hours: 10.0, remaining_hours: 3.0 } } + context 'when another status with a default % complete value is set' do + let(:call_attributes) { { status: status_70_pct_complete } } + let(:expected_attributes) { { remaining_hours: 3.0 } } - before do - work_package.status = other_status - end + it_behaves_like 'service call', + description: 'recomputes remaining work according to the % complete value of the new status' + end + + context 'when another status without any default % complete value is set', skip: 'TODO: not implemented yet' do + let(:call_attributes) { { status: status_no_pct_complete } } + let(:expected_attributes) { { remaining_hours: nil } } - it_behaves_like 'service call' + it_behaves_like 'service call', + description: 'unsets remaining work' + end + end + + context 'given a work package with work and remaining work unset, and a status with no % complete' do + before do + work_package.status = status_no_pct_complete + work_package.done_ratio = work_package.status.default_done_ratio + work_package.estimated_hours = nil + work_package.remaining_hours = nil + work_package.send(:clear_changes_information) + end + + context 'when another status with a default % complete value is set' do + let(:call_attributes) { { status: status_70_pct_complete } } + let(:expected_attributes) { { remaining_hours: nil } } + + it_behaves_like 'service call', + description: 'remaining work remains unset' + end + + context 'when work is set' do + let(:call_attributes) { { estimated_hours: 10.0 } } + let(:expected_attributes) { { remaining_hours: nil } } + + it_behaves_like 'service call', + description: 'remaining work remains unset' end end end end - context 'for done_ratio' do - context 'with the status being used for done_ratio computations', + describe 'deriving % complete attribute (done_ratio)' do + context 'in status-based mode', with_settings: { work_package_done_ratio: 'status' } do - let!(:status) { create(:status, default_done_ratio: 50) } - let(:call_attributes) { { estimated_hours: 10.0 } } - # remaining hours computed from estimated hours and the status's default done ratio - let(:expected_attributes) { { estimated_hours: 10.0, remaining_hours: 5.0, done_ratio: 50 } } + context 'given a work package with a status with 50% complete' do + before do + work_package.status = status_50_pct_complete + work_package.done_ratio = work_package.status.default_done_ratio + work_package.send(:clear_changes_information) + end - before do - work_package.status = status - end + context 'when another status with another % complete value is set' do + let(:call_attributes) { { status: status_70_pct_complete } } + let(:expected_attributes) { { done_ratio: 70 } } - it_behaves_like 'service call' + it_behaves_like 'service call', description: 'sets the % complete value to the status default % complete value' + end + + context 'when another status with no % complete value is set', skip: 'TODO: not implemented yet' do + let(:call_attributes) { { status: status_no_pct_complete } } + let(:expected_attributes) { { done_ratio: nil } } + + it_behaves_like 'service call', description: 'unsets the % complete value' + end + end end + ### + ### Below this line, it has not be rewritten / revised yet ### + ### + describe 'is "unset"' do context 'when estimated_hours is unset' do let(:call_attributes) { { estimated_hours: nil } } let(:expected_attributes) do - { estimated_hours: nil, done_ratio: 0 } + { estimated_hours: nil, done_ratio: nil } end it_behaves_like 'service call' @@ -215,7 +263,7 @@ context 'when remaining_hours is unset' do let(:call_attributes) { { remaining_hours: nil } } let(:expected_attributes) do - { remaining_hours: nil, done_ratio: 0 } + { remaining_hours: nil, done_ratio: nil } end it_behaves_like 'service call' @@ -224,7 +272,7 @@ context 'when both estimated_hours and remaining_hours are unset' do let(:call_attributes) { { estimated_hours: nil, remaining_hours: nil } } let(:expected_attributes) do - { estimated_hours: nil, remaining_hours: nil, done_ratio: 0 } + { estimated_hours: nil, remaining_hours: nil, done_ratio: nil } end it_behaves_like 'service call' diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index 2bcbea1c1f87..fca4e18c4175 100644 --- a/spec/services/work_packages/update_service_integration_spec.rb +++ b/spec/services/work_packages/update_service_integration_spec.rb @@ -495,7 +495,7 @@ # unchanged sibling1_work_package.reload expect(sibling1_work_package.done_ratio) - .to eq(0) + .to be_nil sibling2_work_package.reload expect(sibling2_work_package.done_ratio)