allow copying projects without parent project reference

app/contracts/model_contract.rb

@@ -96,11 +96,13 @@ class ModelContract < Reform::Contract
   end
 
   attr_reader :user
+  attr_accessor :options
 
-  def initialize(model, user)
+  def initialize(model, user, options: {})
     super(model)
 
     @user = user
+    @options = options
   end
 
   # we want to add a validation error whenever someone sets a property that we don't know.

app/contracts/projects/copy_contract.rb

@@ -0,0 +1,37 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
+#
+# 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 doc/COPYRIGHT.rdoc for more details.
+#++
+
+module Projects
+  class CopyContract < BaseContract
+    private
+
+    def validate_user_allowed_to_manage
+      errors.add :base, :error_unauthorized unless user.allowed_to?(:copy_projects, options[:copied_from])
+    end
+  end
+end

app/contracts/work_packages/base_contract.rb

@@ -119,8 +119,8 @@ module WorkPackages
     validate :validate_category
     validate :validate_estimated_hours
 
-    def initialize(work_package, user)
-      super(work_package, user)
+    def initialize(work_package, user, options: {})
+      super
 
       @can = WorkPackagePolicy.new(user)
     end

app/models/project/copy.rb

@@ -84,6 +84,7 @@ module Project::Copy
       project.wiki.pages.each do |page|
         # Skip pages without content
         next if page.content.nil?
+
         new_wiki_content = WikiContent.new(page.content.attributes.dup.except('id', 'page_id', 'updated_at'))
         new_wiki_page = WikiPage.new(page.attributes.dup.except('id', 'wiki_id', 'created_on', 'parent_id'))
         new_wiki_page.content = new_wiki_content

app/services/base_services/base_contracted.rb

@@ -35,9 +35,10 @@ module BaseServices
 
     attr_reader :user
 
-    def initialize(user:, contract_class: nil)
+    def initialize(user:, contract_class: nil, contract_options: {})
       @user = user
       self.contract_class = contract_class || default_contract_class
+      self.contract_options = contract_options
     end
 
     def call(params = nil)
@@ -63,7 +64,7 @@ module BaseServices
     end
 
     def validate_contract(call)
-      success, errors = validate(model, user)
+      success, errors = validate(model, user, options: contract_options)
 
       unless success
         call.success = false

app/services/base_services/delete.rb

@@ -30,9 +30,9 @@ module BaseServices
   class Delete < BaseContracted
     attr_accessor :model
 
-    def initialize(user:, model:, contract_class: nil)
+    def initialize(user:, model:, contract_class: nil, contract_options: {})
       self.model = model
-      super(user: user, contract_class: contract_class)
+      super(user: user, contract_class: contract_class, contract_options: {})
     end
 
     def persist(service_result)

app/services/base_services/set_attributes.rb

@@ -32,7 +32,7 @@ module BaseServices
   class SetAttributes
     include Concerns::Contracted
 
-    def initialize(user:, model:, contract_class:)
+    def initialize(user:, model:, contract_class:, contract_options: {})
       self.user = user
       self.model = model
 
@@ -43,9 +43,10 @@ module BaseServices
       model.extend(Mixins::ChangedBySystem)
 
       self.contract_class = contract_class
+      self.contract_options = contract_options
     end
 
-    def call(params)
+    def call(params, contract_options: {})
       set_attributes(params)
 
       validate_and_result
@@ -68,7 +69,7 @@ module BaseServices
     end
 
     def validate_and_result
-      success, errors = validate(model, user)
+      success, errors = validate(model, user, options: contract_options)
 
       ServiceResult.new(success: success,
                         errors: errors,

app/services/concerns/contracted.rb

@@ -33,6 +33,7 @@ module Concerns::Contracted
 
   included do
     attr_reader :contract_class
+    attr_accessor :contract_options
 
     def contract_class=(cls)
       unless cls <= ::ModelContract
@@ -44,12 +45,12 @@ module Concerns::Contracted
 
     private
 
-    def instantiate_contract(object, user)
-      contract_class.new(object, user)
+    def instantiate_contract(object, user, options: {})
+      contract_class.new(object, user, options: options)
     end
 
-    def validate_and_save(object, user)
-      validate_and_yield(object, user) do
+    def validate_and_save(object, user, options: {})
+      validate_and_yield(object, user, options: options) do
         object.save
       end
     end
@@ -57,8 +58,8 @@ module Concerns::Contracted
     ##
     # Call the given block and assume object is erroneous if
     # it does not return truthy
-    def validate_and_yield(object, user)
-      contract = instantiate_contract(object, user)
+    def validate_and_yield(object, user, options: {})
+      contract = instantiate_contract(object, user, options: options)
 
       if !contract.validate
         [false, contract.errors]
@@ -69,8 +70,8 @@ module Concerns::Contracted
       end
     end
 
-    def validate(object, user)
-      validate_and_yield(object, user) do
+    def validate(object, user, options: {})
+      validate_and_yield(object, user, options: options) do
         # No object validation is necessary at this point
         # as object.valid? is already called in the contract
         true

app/workers/copy_project_job.rb

@@ -40,8 +40,11 @@ class CopyProjectJob < ApplicationJob
               :associations_to_copy,
               :send_mails
 
-  def perform(user_id:, source_project_id:, target_project_params:,
-                 associations_to_copy:, send_mails: false)
+  def perform(user_id:,
+              source_project_id:,
+              target_project_params:,
+              associations_to_copy:,
+              send_mails: false)
     # Needs refactoring after moving to activejob
 
     @user_id               = user_id
@@ -54,10 +57,7 @@ class CopyProjectJob < ApplicationJob
     @target_project_name = target_project_params[:name]
 
     @target_project, @errors = with_locale_for(user) do
-      create_project_copy(source_project,
-                          target_project_params,
-                          associations_to_copy,
-                          send_mails)
+      create_project_copy
     end
 
     if target_project
@@ -81,41 +81,22 @@ class CopyProjectJob < ApplicationJob
     @project ||= Project.find source_project_id
   end
 
-  def create_project_copy(source_project,
-                          target_project_params,
-                          associations_to_copy,
-                          send_mails)
-    target_project = nil
-    errors         = []
+  def create_project_copy
+    errors = []
 
     ProjectMailer.with_deliveries(send_mails) do
-      target_project = Project.copy_attributes(source_project)
-
-      service_call = Projects::SetAttributesService
-                     .new(user: user,
-                          model: target_project,
-                          contract_class: Projects::CreateContract)
-                     .call(target_project_params)
+      service_call = copy_project_attributes
+      target_project = service_call.result
 
       if service_call.success? && target_project.save
-        target_project.copy_associations(source_project, only: associations_to_copy)
-
-        # Project was created
-        # But some objects might not have been copied due to validation failures
-        error_objects = (target_project.compiled_errors.flatten + [target_project.errors]).flatten
-        error_objects.each do |error_object|
-          base = error_object.instance_variable_get(:@base)
-          error_prefix = base.is_a?(Project) ? '' : "#{base.class.model_name.human} '#{base}': "
-
-          error_object.full_messages.flatten.each do |error|
-            errors << error_prefix + error
-          end
-        end
+        errors = copy_project_associations(target_project)
       else
-        errors         = service_call.errors.merge(target_project.errors).full_messages
+        errors = service_call.errors.merge(target_project.errors).full_messages
         target_project = nil
         logger.error("Copying project fails with validation errors: #{errors.join("\n")}")
       end
+
+      return target_project, errors
     end
   rescue ActiveRecord::RecordNotFound => e
     logger.error("Entity missing: #{e.message} #{e.backtrace.join("\n")}")
@@ -127,11 +108,62 @@ class CopyProjectJob < ApplicationJob
       logger.error('Encountered an errors while trying to copy related objects for '\
                    "project '#{source_project_id}': #{errors.inspect}")
     end
-
-    return target_project, errors
   end
 
   def logger
     Rails.logger
   end
+
+  def copy_project_attributes
+    target_project = Project.copy_attributes(source_project)
+
+    cleanup_target_project_attributes(target_project)
+    cleanup_target_project_params
+
+    Projects::SetAttributesService
+      .new(user: user,
+           model: target_project,
+           contract_class: Projects::CopyContract,
+           contract_options: { copied_from: source_project })
+      .call(target_project_params)
+  end
+
+  def cleanup_target_project_params
+    if (parent_id = target_project_params["parent_id"]) && (parent = Project.find_by(id: parent_id))
+      target_project_params.delete("parent_id") unless user.allowed_to?(:add_subprojects, parent)
+    end
+  end
+
+  def cleanup_target_project_attributes(target_project)
+    if target_project.parent
+      target_project.parent = nil unless user.allowed_to?(:add_subprojects, target_project.parent)
+    end
+  end
+
+  def copy_project_associations(target_project)
+    target_project.copy_associations(source_project, only: associations_to_copy)
+    errors = []
+
+    # Project was created
+    # But some objects might not have been copied due to validation failures
+    error_objects = project_errors(target_project)
+    error_objects.each do |error_object|
+      error_prefix = error_prefix_for(error_object)
+
+      error_object.full_messages.flatten.each do |error|
+        errors << error_prefix + error
+      end
+    end
+
+    errors
+  end
+
+  def project_errors(project)
+    (project.compiled_errors.flatten + [project.errors]).flatten
+  end
+
+  def error_prefix_for(error_object)
+    base = error_object.instance_variable_get(:@base)
+    base.is_a?(Project) ? '' : "#{base.class.model_name.human} '#{base}': "
+  end
 end

spec/workers/copy_project_job_spec.rb

@@ -127,7 +127,6 @@ describe CopyProjectJob, type: :model do
                     associations_to_copy: [:work_packages]
       end
     end
-    let(:params) { {name: 'Copy', identifier: 'copy'} }
 
     before do
       allow(User).to receive(:current).and_return(admin)
@@ -193,28 +192,40 @@ describe CopyProjectJob, type: :model do
     end
 
     describe 'subproject' do
-      let(:params) { {name: 'Copy', identifier: 'copy', parent_id: project.id} }
-      let(:subproject) { FactoryBot.create(:project, parent: project) }
+      let(:params) { { name: 'Copy', identifier: 'copy', parent_id: project.id } }
+      let(:subproject) do
+        FactoryBot.create(:project, parent: project).tap do |p|
+          FactoryBot.create(:member,
+                            principal: user,
+                            roles: [role],
+                            project: p)
+        end
+      end
+
+      subject { Project.find_by(identifier: 'copy') }
 
-      describe 'invalid parent' do
+      describe 'user without add_subprojects permission in parent' do
         include_context 'copy project' do
           let(:project_to_copy) { subproject }
         end
 
-        it "creates no new project" do
-          expect(Project.all).to match_array([project, subproject])
+        it 'copies the project without the parent being set' do
+          expect(subject).not_to be_nil
+          expect(subject.parent).to be_nil
+
+          expect(subproject.reload.enabled_module_names).not_to be_empty
         end
 
-        it "notifies the user of the failure" do
+        it "notifies the user of the success" do
           mail = ActionMailer::Base.deliveries
-            .find { |m| m.message_id.start_with? "openproject.project-#{user.id}-#{subproject.id}" }
+                   .find { |m| m.message_id.start_with? "openproject.project-#{user.id}-#{subject.id}" }
 
           expect(mail).to be_present
-          expect(mail.subject).to eq "Cannot copy project #{subproject.name}"
+          expect(mail.subject).to eq "Created project #{subject.name}"
         end
       end
 
-      describe 'valid parent' do
+      describe 'user with add_subprojects permission in parent' do
         let(:role_add_subproject) { FactoryBot.create(:role, permissions: [:add_subprojects]) }
         let(:member_add_subproject) do
           FactoryBot.create(:member,
@@ -231,8 +242,6 @@ describe CopyProjectJob, type: :model do
           let(:project_to_copy) { subproject }
         end
 
-        subject { Project.find_by(identifier: 'copy') }
-
         it 'copies the project' do
           expect(subject).not_to be_nil
           expect(subject.parent).to eql(project)