diff --git a/lib/api/v3/work_packages/work_package_model.rb b/lib/api/v3/work_packages/work_package_model.rb index 6228cbf393..ad5c782166 100644 --- a/lib/api/v3/work_packages/work_package_model.rb +++ b/lib/api/v3/work_packages/work_package_model.rb @@ -158,15 +158,30 @@ module API validates_presence_of :subject, :project_id, :type, :author, :status validates_length_of :subject, maximum: 255 - validate :validate_parent_constraint + validate :milestone_constraint + validate :user_allowed_to_access_parent private - def validate_parent_constraint - if model.parent - errors.add :parent_id, :cannot_be_milestone if model.parent.is_milestone? - end - end + def milestone_constraint + errors.add :parent_id, :cannot_be_milestone if model.parent && model.parent.is_milestone? + end + + def user_allowed_to_access_parent + errors.add(:parent_id, error_message('parent_id.does_not_exist')) if parent_changed? && !parent_visible? + end + + def parent_changed? + parent_id != model.parent_id + end + + def parent_visible? + !parent_id || ::WorkPackage.visible(User.current).exists?(parent_id) + end + + def error_message(path) + I18n.t("activerecord.errors.models.work_package.attributes.#{path}") + end end end end diff --git a/lib/api/v3/work_packages/work_packages_api.rb b/lib/api/v3/work_packages/work_packages_api.rb index 604a0c2bc3..299e10104d 100644 --- a/lib/api/v3/work_packages/work_packages_api.rb +++ b/lib/api/v3/work_packages/work_packages_api.rb @@ -55,22 +55,10 @@ module API attributes.delete_if { |key, _| VALID_UPDATE_ATTRIBUTES.include? key } end - def check_parent_update - attributes = JSON.parse(env['api.request.input']) - - authorize(:manage_subtasks, context: @work_package.project) if attributes.include? 'parentId' - - parent_id = attributes['parentId'].blank? ? nil : attributes['parentId'].to_i - - if parent_id && !WorkPackage.visible(current_user).exists?(parent_id) - @work_package.errors.add(:parent_id, :not_a_valid_parent) - fail ::API::Errors::Validation.new(@work_package) - end - end - def work_package_attributes - attributes = JSON.parse(env['api.request.input']) - attributes.delete_if { |key, _| VALID_REQUEST_ATTRIBUTES.include? key } + @work_package_attributes ||= JSON.parse(env['api.request.input']).tap do |attributes| + attributes.delete_if { |key, _| VALID_REQUEST_ATTRIBUTES.include? key } + end end end @@ -87,15 +75,14 @@ module API patch do authorize(:edit_work_packages, context: @work_package.project) + authorize(:manage_subtasks, context: @work_package.project) if work_package_attributes.has_key? 'parentId' check_work_package_attributes # fails if request contains read-only attributes - check_parent_update # fails if parent update is invalid @representer.from_json(work_package_attributes.to_json) - @representer.represented.sync - if @representer.represented.model.valid? && @representer.represented.save + if @representer.represented.valid? && @representer.represented.sync && @representer.represented.save @representer else - fail ::API::Errors::Validation.new(@representer.represented.model) + fail ::API::Errors::Validation.new(@representer.represented) end end diff --git a/spec/requests/api/v3/support/response_examples.rb b/spec/requests/api/v3/support/response_examples.rb index cfc76b270d..97af52cec5 100644 --- a/spec/requests/api/v3/support/response_examples.rb +++ b/spec/requests/api/v3/support/response_examples.rb @@ -124,7 +124,7 @@ end shared_examples_for 'multiple errors of the same type with messages' do |expected_messages| let(:errors) { JSON.parse(last_response.body)['_embedded']['errors'] } - let(:messages) { errors.each_with_object([]) { |error, l| l << error['_embedded']['messages'] }.compact } + let(:messages) { errors.each_with_object([]) { |error, l| l << error['message'] }.compact } it { expect(messages).to match_array(Array(expected_messages)) } end diff --git a/spec/requests/api/v3/work_package_resource_spec.rb b/spec/requests/api/v3/work_package_resource_spec.rb index 974162d73b..2e84412441 100644 --- a/spec/requests/api/v3/work_package_resource_spec.rb +++ b/spec/requests/api/v3/work_package_resource_spec.rb @@ -234,7 +234,7 @@ h4. things we like let(:invisible_parent) { FactoryGirl.create(:work_package) } let(:params) { valid_params.merge(parentId: invisible_parent.id) } - it { expect(WorkPackage.visible(current_user).exists?(invisible_parent.id)).to be_false } + it { expect(WorkPackage.visible(current_user).exists?(invisible_parent.id)).to be_falsey } it { expect(response.status).to eq(422) } end @@ -320,19 +320,22 @@ h4. things we like end context 'multiple invalid attributes' do - # TODO Add another invalid parameter - # At the moment only subject and parent id are writable but validated - # at different places. Thus, we need to wait until this is harmonized - # or other attributes become writable. - let(:params) { valid_params.tap { |h| h[:subject] = '' } } + let(:invisible_parent) { FactoryGirl.create(:work_package) } + let(:params) { valid_params } + let(:params) do + valid_params.tap { |h| h[:subject] = '' } + .merge(parentId: invisible_parent.id) + end + + before { role.add_permission!(:manage_subtasks) } include_context 'patch request' - # it_behaves_like 'multiple errors', 422, 'multiple fields violated their constraints.' + it_behaves_like 'multiple errors', 422, 'Multiple fields violated their constraints.' - # it_behaves_like 'multiple errors of the same type', 2, 'PropertyConstraintViolation' + it_behaves_like 'multiple errors of the same type', 2, 'PropertyConstraintViolation' - # it_behaves_like 'multiple errors of the same type with messages', ['error1', 'error2'] + it_behaves_like 'multiple errors of the same type with messages', ['Subject can\'t be blank', 'Parent does not exist'] end end end