diff --git a/app/contracts/model_contract.rb b/app/contracts/model_contract.rb index 19a47d8488..127ac6afc2 100644 --- a/app/contracts/model_contract.rb +++ b/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. diff --git a/app/contracts/projects/copy_contract.rb b/app/contracts/projects/copy_contract.rb new file mode 100644 index 0000000000..0bbdad6531 --- /dev/null +++ b/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 diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 409def4367..41b833cbc3 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/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 diff --git a/app/models/project/copy.rb b/app/models/project/copy.rb index a6baf49e57..9805aec3ea 100644 --- a/app/models/project/copy.rb +++ b/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 diff --git a/app/services/base_services/base_contracted.rb b/app/services/base_services/base_contracted.rb index 037bcbe0ae..b32e451bb4 100644 --- a/app/services/base_services/base_contracted.rb +++ b/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 diff --git a/app/services/base_services/delete.rb b/app/services/base_services/delete.rb index c574ca6213..3203f1cd0f 100644 --- a/app/services/base_services/delete.rb +++ b/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) diff --git a/app/services/base_services/set_attributes.rb b/app/services/base_services/set_attributes.rb index 4ee9a14d6e..2113bb696a 100644 --- a/app/services/base_services/set_attributes.rb +++ b/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, diff --git a/app/services/concerns/contracted.rb b/app/services/concerns/contracted.rb index 0a297d26ed..c5f5fb1064 100644 --- a/app/services/concerns/contracted.rb +++ b/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 diff --git a/app/workers/copy_project_job.rb b/app/workers/copy_project_job.rb index b81dca88e9..83ded2cff5 100644 --- a/app/workers/copy_project_job.rb +++ b/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 diff --git a/spec/workers/copy_project_job_spec.rb b/spec/workers/copy_project_job_spec.rb index 7a7286c814..bb452ae05c 100644 --- a/spec/workers/copy_project_job_spec.rb +++ b/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)