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 <rageltman [at] sempervictus>

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
 

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

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

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

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