Merge pull request #8005 from vpereira/update_review_validations

Convert custom validations into Rails validations

src/api/app/models/review.rb

@@ -21,7 +21,14 @@ class NotFoundError < APIError
   validates :reviewer, length: { maximum: 250 }
   validates :reason, length: { maximum: 65_534 }
 
-  validate :check_initial, on: [:create]
+  validates :user, presence: true, if: :by_user?
+  validates :group, presence: true, if: :by_group?
+  validates :project, presence: true, if: :by_project?, on: :create
+  validates :package, presence: true, if: :by_package?, on: :create
+  validates :by_project, presence: true, if: :by_package?, on: :create
+
+  validate :review_assignment
+
   # Validate the review is not assigned to a review which is already assigned to this review
   validate :validate_non_symmetric_assignment
   validate :validate_not_self_assigned
@@ -57,6 +64,11 @@ class NotFoundError < APIError
   before_validation :set_reviewable_association
   after_commit :update_cache
 
+  def review_assignment
+    errors.add(:unknown, 'no reviewer defined') unless by_user || by_group || by_project
+    errors.add(:base, 'it is not allowed to have more than one reviewer entity: by_user, by_group, by_project') if invalid_reviewers?
+  end
+
   def validate_non_symmetric_assignment
     return unless review_assigned_from && review_assigned_from == review_assigned_to
 
@@ -111,38 +123,6 @@ def assigned_reviewer
     self[:reviewer] || by_user || by_group || by_project || by_package
   end
 
-  def check_initial
-    # Validates the existence of references.
-    # NOTE: they can disappear later and the review should be still
-    #       usable to some degree (can be showed at least)
-    #       But it must not be possible to create one with broken references
-    unless by_user || by_group || by_project
-      errors.add(:unknown, 'no reviewer defined')
-    end
-
-    if validate_reviewer_fields
-      errors.add(:base, 'it is not allowed to have more than one reviewer entity: by_user, by_group, by_project, by_package')
-    end
-
-    errors.add(:by_user, "#{by_user} not found") if by_user && !user
-
-    errors.add(:by_group, "#{by_group} not found") if by_group && !group
-
-    if by_project && !project
-      # must be a local project or we can't ask
-      errors.add(:by_project, "#{by_project} not found")
-    end
-
-    if by_package && !by_project
-      errors.add(:unknown, 'by_package defined, but missing by_project')
-    end
-    return unless by_package && !package
-
-    # must be a local package. maybe we should rewrite in case the
-    # package comes via local project link...
-    errors.add(:by_package, "#{by_project}/#{by_package} not found")
-  end
-
   def self.new_from_xml_hash(hash)
     r = Review.new
 
@@ -288,7 +268,8 @@ def set_reviewable_association
     self.group = Group.find_by(title: by_group)
   end
 
-  def validate_reviewer_fields
+  # A review can be by one and only one of following options: by_user, by_group or by_project
+  def invalid_reviewers?
     (by_user && (by_group || by_project || by_package)) || (by_group && (by_project || by_package))
   end
 

src/api/spec/models/bs_request_spec.rb

@@ -127,7 +127,7 @@
     it 'fails with reasonable error' do
       expect { request.addreview(by_user: 'NOEXIST') }.to raise_error do |exception|
         expect(exception).to be_a(BsRequest::InvalidReview)
-        expect(exception.message.to_s).to eq('Review invalid: By user NOEXIST not found')
+        expect(exception.message.to_s).to eq("Review invalid: User can't be blank")
       end
     end
   end

src/api/spec/models/review_spec.rb

@@ -35,15 +35,15 @@
       [:by_group, :by_project, :by_package].each do |reviewable|
         review = Review.create(:by_user => user.login, reviewable => 'not-existent-reviewable')
         expect(review.errors.messages[:base]).
-          to eq(['it is not allowed to have more than one reviewer entity: by_user, by_group, by_project, by_package'])
+          to eq(['it is not allowed to have more than one reviewer entity: by_user, by_group, by_project'])
       end
     end
 
     it 'is not allowed to specify by_group and any other reviewable' do
       [:by_project, :by_package].each do |reviewable|
         review = Review.create(:by_group => group.title, reviewable => 'not-existent-reviewable')
         expect(review.errors.messages[:base]).
-          to eq(['it is not allowed to have more than one reviewer entity: by_user, by_group, by_project, by_package'])
+          to eq(['it is not allowed to have more than one reviewer entity: by_user, by_group, by_project'])
       end
     end
   end
@@ -89,6 +89,18 @@
         expect(review.project).to eq(project)
         expect(review.by_project).to eq(project.name)
       end
+
+      it 'sets package and project associations when by_package and by_project object exists. remove package. Review should be invalid' do
+        User.session = user
+        review = create(:review, by_project: project.name, by_package: package.name)
+        expect(review.package).to eq(package)
+        expect(review.by_package).to eq(package.name)
+        expect(review.project).to eq(project)
+        expect(review.by_project).to eq(project.name)
+        package.destroy
+        expect(review).to be_valid
+        expect(review.errors.messages[:package]).not_to include('can\'t be blank')
+      end
     end
 
     context 'with invalid attributes' do
@@ -128,7 +140,8 @@
       it 'does not set package association when by_project parameter is missing' do
         review = Review.new(by_package: package.name)
         expect(review.package).to eq(nil)
-        expect(review.valid?).to eq(false)
+        expect(review).not_to be_valid
+        expect(review.errors.messages[:package]).to include('can\'t be blank')
       end
     end
   end