From e0ff976427b3777cd0c420f4310925229be0b5dd Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Thu, 30 Nov 2023 17:06:07 +0100 Subject: [PATCH 1/3] Remove dead code The code was not working properly: `property` is a string, it was compared against a symbol, so `remaining_time` and `version` were always considered writable. It is a check in the contract which prevents the version from being changed, and a check at the api level which prevents the remaining time from being changed. This code can be considered dead since it has not worked since more than 6 years (fd66a376e) and nobody noticed or complained. --- .../lib/open_project/backlogs/engine.rb | 1 - .../specific_work_package_schema_patch.rb | 57 -------- .../specific_work_package_schema_spec.rb | 130 ------------------ 3 files changed, 188 deletions(-) delete mode 100644 modules/backlogs/lib/open_project/backlogs/patches/specific_work_package_schema_patch.rb delete mode 100644 modules/backlogs/spec/api/work_packages/schema/specific_work_package_schema_spec.rb diff --git a/modules/backlogs/lib/open_project/backlogs/engine.rb b/modules/backlogs/lib/open_project/backlogs/engine.rb index 74210dbd8407..b3f57372c7b1 100644 --- a/modules/backlogs/lib/open_project/backlogs/engine.rb +++ b/modules/backlogs/lib/open_project/backlogs/engine.rb @@ -132,7 +132,6 @@ def self.settings VersionsController Version] - patch_with_namespace :API, :V3, :WorkPackages, :Schema, :SpecificWorkPackageSchema patch_with_namespace :WorkPackages, :UpdateAncestors, :Loader patch_with_namespace :BasicData, :SettingSeeder patch_with_namespace :DemoData, :ProjectSeeder diff --git a/modules/backlogs/lib/open_project/backlogs/patches/specific_work_package_schema_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/specific_work_package_schema_patch.rb deleted file mode 100644 index 1cb60f9fdb01..000000000000 --- a/modules/backlogs/lib/open_project/backlogs/patches/specific_work_package_schema_patch.rb +++ /dev/null @@ -1,57 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2023 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, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module OpenProject::Backlogs::Patches::SpecificWorkPackageSchemaPatch - def self.included(base) - base.class_eval do - prepend InstanceMethods - extend ClassMethods - end - end - - module ClassMethods - end - - module InstanceMethods - def writable?(property) - if property == :remaining_time && !@work_package.leaf? - false - elsif version_with_backlogs_parent(property) - false - else - super - end - end - - private - - def version_with_backlogs_parent(property) - property == :version && @work_package.is_task? && WorkPackage.find(@work_package.parent_id).in_backlogs_type? - end - end -end diff --git a/modules/backlogs/spec/api/work_packages/schema/specific_work_package_schema_spec.rb b/modules/backlogs/spec/api/work_packages/schema/specific_work_package_schema_spec.rb deleted file mode 100644 index 797b1137a284..000000000000 --- a/modules/backlogs/spec/api/work_packages/schema/specific_work_package_schema_spec.rb +++ /dev/null @@ -1,130 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2023 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, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -require 'spec_helper' - -RSpec.describe API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do - let(:project) { build(:project) } - let(:type) { build(:type) } - let(:work_package) { build(:work_package, project:, type:) } - let(:current_user) { build_stubbed(:user) } - - before do - mock_permissions_for(current_user, &:allow_everything) - - login_as(current_user) - end - - shared_examples_for 'with parent which is a BACKLOGS type' do |writable| - let(:parent) { create(:work_package, type: type_task) } - - before do - work_package.parent_id = parent.id - work_package.save! - end - - it "is #{'not' unless writable} writable" do - if writable - expect(subject).to be_writable(:version) - else - expect(subject).not_to be_writable(:version) - end - end - end - - shared_examples_for 'with parent which is not a BACKLOGS type' do - let(:parent) { create(:work_package, type: type_feature) } - - before do - work_package.parent_id = parent.id - work_package.save! - end - - it "is writable" do - expect(subject).to be_writable(:version) - end - end - - describe '#writable? for remaining_hours' do - subject { described_class.new(work_package:) } - - context 'work_package is a leaf' do - before do - allow(work_package).to receive(:leaf?).and_return(true) - end - - it 'is writable' do - expect(subject).to be_writable(:remaining_hours) - end - end - - context 'work_package is no leaf' do - before do - allow(work_package).to receive(:leaf?).and_return(false) - end - - it 'is not writable' do - expect(subject).not_to be_writable(:remaining_hours) - end - end - end - - describe '#version_writable?' do - subject { described_class.new(work_package:) } - - let(:type_task) { create(:type_task) } - let(:type_feature) { create(:type_feature) } - - before do - allow(WorkPackage).to receive(:backlogs_types).and_return([type_task.id]) - allow(work_package).to receive(:backlogs_enabled?).and_return(true) - end - - describe 'work_package is a task' do - before do - allow(work_package) - .to receive(:is_task?) - .and_return(true) - end - - it_behaves_like 'with parent which is a BACKLOGS type', false - it_behaves_like 'with parent which is not a BACKLOGS type' - end - - describe 'work_package is no task' do - before do - allow(work_package) - .to receive(:is_task?) - .and_return(false) - end - - it_behaves_like 'with parent which is a BACKLOGS type', true - it_behaves_like 'with parent which is not a BACKLOGS type' - end - end -end From 72d396789c7540ce8ffab1a6b89bd0a2824faf05 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Thu, 30 Nov 2023 17:29:01 +0100 Subject: [PATCH 2/3] [50958] Make remaining work editable even when not leaf --- modules/backlogs/lib/open_project/backlogs/engine.rb | 6 +++--- .../spec/contracts/work_packages/update_contract_spec.rb | 4 +--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/modules/backlogs/lib/open_project/backlogs/engine.rb b/modules/backlogs/lib/open_project/backlogs/engine.rb index b3f57372c7b1..1ba1a1b30e89 100644 --- a/modules/backlogs/lib/open_project/backlogs/engine.rb +++ b/modules/backlogs/lib/open_project/backlogs/engine.rb @@ -27,8 +27,8 @@ #++ require 'open_project/plugins' -require_relative './patches/api/work_package_representer' -require_relative './patches/api/work_package_schema_representer' +require_relative 'patches/api/work_package_representer' +require_relative 'patches/api/work_package_schema_representer' module OpenProject::Backlogs class Engine < ::Rails::Engine @@ -171,7 +171,7 @@ def self.settings &::OpenProject::Backlogs::Patches::API::WorkPackageSchemaRepresenter.extension) add_api_attribute on: :work_package, ar_name: :story_points - add_api_attribute on: :work_package, ar_name: :remaining_hours, writable: ->(*) { model.leaf? } + add_api_attribute on: :work_package, ar_name: :remaining_hours add_api_path :backlogs_type do |id| # There is no api endpoint for this url diff --git a/modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb b/modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb index 05d0cbaac164..2b878b1ad340 100644 --- a/modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb +++ b/modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb @@ -105,9 +105,7 @@ context 'has changed' do let(:changed_values) { ['remaining_hours'] } - it('is invalid') do - expect(contract.errors.symbols_for(:remaining_hours)).to match_array([:error_readonly]) - end + it('is valid') { expect(contract.errors.empty?).to be true } end end end From 34bc774683245f76cfcfbde90cc175d0d7b95af8 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 4 Dec 2023 15:17:08 +0100 Subject: [PATCH 3/3] [50958] Stop copying estimated_hours to remaining_hours and vice versa It was limited to backlogs Task only, and was only updating if not set. This is not desired anymore. Ideally, we'd like to eventually have them always kept in sync, and for all work package types. --- .../backlogs/patches/work_package_patch.rb | 11 ----- modules/backlogs/spec/models/task_spec.rb | 45 ------------------- 2 files changed, 56 deletions(-) diff --git a/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb index 77e28aaa4703..a1d5c29ad131 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb +++ b/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb @@ -33,8 +33,6 @@ module OpenProject::Backlogs::Patches::WorkPackagePatch prepend InstanceMethods extend ClassMethods - before_validation :backlogs_before_validation, if: -> { backlogs_enabled? } - register_journal_formatted_fields(:fraction, 'remaining_hours') register_journal_formatted_fields(:fraction, 'derived_remaining_hours') register_journal_formatted_fields(:decimal, 'story_points') @@ -130,15 +128,6 @@ def backlogs_enabled? def in_backlogs_type? backlogs_enabled? && WorkPackage.backlogs_types.include?(type.try(:id)) end - - private - - def backlogs_before_validation - if type_id == Task.type - self.estimated_hours = remaining_hours if estimated_hours.blank? && remaining_hours.present? - self.remaining_hours = estimated_hours if remaining_hours.blank? && estimated_hours.present? - end - end end end diff --git a/modules/backlogs/spec/models/task_spec.rb b/modules/backlogs/spec/models/task_spec.rb index b0316f5175c3..38b6b77a373e 100644 --- a/modules/backlogs/spec/models/task_spec.rb +++ b/modules/backlogs/spec/models/task_spec.rb @@ -101,49 +101,4 @@ end end end - - describe 'copying remaining_hours to estimated_hours and vice versa' do - context 'providing only remaining_hours' do - before do - task.remaining_hours = 3 - - task.save! - end - - it 'copies to estimated_hours' do - expect(task.estimated_hours) - .to eql task.remaining_hours - end - end - - context 'providing only estimated_hours' do - before do - task.estimated_hours = 3 - - task.save! - end - - it 'copies to estimated_hours' do - expect(task.remaining_hours) - .to eql task.estimated_hours - end - end - - context 'providing estimated_hours and remaining_hours' do - before do - task.estimated_hours = 3 - task.remaining_hours = 5 - - task.save! - end - - it 'leaves the values unchanged' do - expect(task.remaining_hours) - .to be 5.0 - - expect(task.estimated_hours) - .to be 3.0 - end - end - end end