Merge branch 'feature/16770_rework_validation' into feature/16770_work_package_api_patch_for_subject

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

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
 

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

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