From e58df37e29ada43ee7187a123a50910d61697cf5 Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 26 Nov 2020 10:49:10 +0100 Subject: [PATCH] Fix/copy budgets (#8856) * Fix broken call to #super in Budget#copy_from The #copy_from method in Budget appears to assume that superclasses will have the same method available to them as called on the new Budget object to which the copy is going. This is not the case. Resolve the issue by assigning duplicated attributes from the source object directly to self.attributes prior to copying the actual budget items. Tested in Chef/Vagrant test environment. Deployed to SemperVictus pre-production OP instance successfully. * Resolve budget child item copy confusion The #copy_from method does not account for child models duplicated from another Budget holding a unique constraint validation on the :budget_id attribute. The Budget itself also appears to inherit the source Budget's ID, creating additional problems. Address by unfolding the sub-object copy procedure and clean up the relevant attributes on #dup ensuring the appropriate relations are created when the object is assigned (after the #dup block). Tested in Chef/Vagrant workflow. * Rewrite for readability DRY the code, leaving it as an instance method for the time being. TODO for upstream: Convert to class method and update controller in the MVC (per GH comment). Write tests/spec and verify results of the original fix and final revision for release branches. * use whitelist approach for copying budgets * extract method Co-authored-by: RageLtMan --- .../app/controllers/budgets_controller.rb | 9 +- modules/budgets/app/models/budget.rb | 60 +++++++++---- .../spec/features/budgets/copy_budget_spec.rb | 89 +++++++++++++++++++ .../budgets/spec/support/pages/budget_form.rb | 9 ++ .../budgets/spec/support/pages/edit_budget.rb | 6 ++ 5 files changed, 151 insertions(+), 22 deletions(-) create mode 100644 modules/budgets/spec/features/budgets/copy_budget_spec.rb diff --git a/modules/budgets/app/controllers/budgets_controller.rb b/modules/budgets/app/controllers/budgets_controller.rb index c35c74f69e..e9f7e3d41e 100644 --- a/modules/budgets/app/controllers/budgets_controller.rb +++ b/modules/budgets/app/controllers/budgets_controller.rb @@ -84,11 +84,12 @@ class BudgetsController < ApplicationController def copy source = Budget.find(params[:id].to_i) - @budget = Budget.new - if source - @budget.copy_from(source) - end + @budget = if source + Budget.new_copy(source) + else + Budget.new + end @budget.fixed_date ||= Date.today diff --git a/modules/budgets/app/models/budget.rb b/modules/budgets/app/models/budget.rb index d379083a87..a81994b024 100644 --- a/modules/budgets/app/models/budget.rb +++ b/modules/budgets/app/models/budget.rb @@ -63,10 +63,48 @@ class Budget < ApplicationRecord Budget.replace_author_with_deleted_user user end - def self.visible(user) - includes(:project) - .references(:projects) - .merge(Project.allowed_to(user, :view_budgets)) + class << self + def visible(user) + includes(:project) + .references(:projects) + .merge(Project.allowed_to(user, :view_budgets)) + end + + # TODO: Extract into copy service + def new_copy(source) + copy = new(copy_attributes(source)) + + copy_budget_items(source, copy, items: :labor_budget_items) + copy_budget_items(source, copy, items: :material_budget_items) + + copy + end + + def replace_author_with_deleted_user(user) + substitute = DeletedUser.first + + where(author_id: user.id).update_all(author_id: substitute.id) + end + + protected + + def copy_attributes(source) + source.attributes.slice('project_id', 'subject', 'description', 'fixed_date').merge('author' => User.current) + end + + def copy_budget_items(source, sink, items:) + raise ArgumentError unless %i(labor_budget_items material_budget_items).include? items + + source.send(items).each do |bi| + to_slice = if items == :material_budget_items + %w(units cost_type_id comments amount) + else + %w(hours user_id comments amount) + end + + sink.send(items).build(bi.attributes.slice(*to_slice).merge('budget' => sink)) + end + end end def initialize(attributes = nil) @@ -74,14 +112,6 @@ class Budget < ApplicationRecord self.author = User.current if new_record? end - def copy_from(arg) - budget = (arg.is_a?(Budget) ? arg : self.class.find(arg)) - attrs = budget.attributes.dup - super(attrs) - self.labor_budget_items = budget.labor_budget_items.map(&:dup) - self.material_budget_items = budget.material_budget_items.map(&:dup) - end - def budget material_budget + labor_budget end @@ -105,12 +135,6 @@ class Budget < ApplicationRecord 'budget' end - def self.replace_author_with_deleted_user(user) - substitute = DeletedUser.first - - where(author_id: user.id).update_all(author_id: substitute.id) - end - def to_s subject end diff --git a/modules/budgets/spec/features/budgets/copy_budget_spec.rb b/modules/budgets/spec/features/budgets/copy_budget_spec.rb new file mode 100644 index 0000000000..6b25a20dc2 --- /dev/null +++ b/modules/budgets/spec/features/budgets/copy_budget_spec.rb @@ -0,0 +1,89 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2020 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb') + +describe 'Copying a budget', type: :feature, js: true do + let(:project) { FactoryBot.create :project, enabled_module_names: %i[budgets costs] } + let(:current_user) do + FactoryBot.create :user, + member_in_project: project, + member_with_permissions: %i(view_budgets edit_budgets view_hourly_rates view_cost_rates) + end + let(:original_author) { FactoryBot.create :user } + let(:budget_subject) { "A budget subject" } + let(:budget_description) { "A budget description" } + let!(:budget) do + FactoryBot.create :budget, + subject: budget_subject, + description: budget_description, + author: original_author, + project: project + end + let!(:cost_type) do + FactoryBot.create :cost_type, name: 'Post-war', unit: 'cap', unit_plural: 'caps' + end + let!(:cost_type_rate) { FactoryBot.create :cost_rate, cost_type: cost_type, rate: 50.0 } + let!(:default_hourly_rate) { FactoryBot.create :default_hourly_rate, user: original_author, rate: 25.0 } + let!(:material_budget_item) do + FactoryBot.create :material_budget_item, + units: 3, + cost_type: cost_type, + budget: budget + end + + let!(:labor_budget_item) do + FactoryBot.create :labor_budget_item, + hours: 5, + user: original_author, + budget: budget + end + let(:budget_page) { Pages::EditBudget.new budget.id } + + before do + login_as(current_user) + end + + it 'copies all the items of the budget under the name of the copying user' do + budget_page.visit! + + budget_page.click_copy + + budget_page.expect_subject(budget_subject) + + budget_page.expect_planned_costs!(type: :labor, row: 1, expected: '125.00 EUR') + budget_page.expect_planned_costs!(type: :material, row: 1, expected: '150.00 EUR') + + click_button 'Create' + + budget_page.expect_notification message: 'Successful creation.' + + expect(page) + .to have_selector('.author', text: current_user.name) + end +end diff --git a/modules/budgets/spec/support/pages/budget_form.rb b/modules/budgets/spec/support/pages/budget_form.rb index e811ec2bef..26da55599e 100644 --- a/modules/budgets/spec/support/pages/budget_form.rb +++ b/modules/budgets/spec/support/pages/budget_form.rb @@ -121,6 +121,11 @@ module Pages end end + def expect_subject(subject) + expect(page) + .to have_field("Subject", with: subject) + end + def unit_costs_at(num_row) unit_costs_container.all('tbody td.currency')[num_row - 1] end @@ -176,5 +181,9 @@ module Pages def labor_rows @labor_rows ||= 0 end + + def notification_type + :rails + end end end diff --git a/modules/budgets/spec/support/pages/edit_budget.rb b/modules/budgets/spec/support/pages/edit_budget.rb index 458dfc2a59..1065c3439c 100644 --- a/modules/budgets/spec/support/pages/edit_budget.rb +++ b/modules/budgets/spec/support/pages/edit_budget.rb @@ -39,6 +39,12 @@ module Pages @budget_id = budget_id end + def click_copy + within '.toolbar-items' do + click_link 'Copy' + end + end + def path "/budgets/#{budget_id}" end