Merge pull request #4258 from bgeuken/review_refactoring_

[frontend] DRY request code by moving it to review model

src/api/app/models/bs_request.rb

@@ -412,10 +412,7 @@ def is_reviewer?(user)
   def obsolete_reviews(opts)
     return false unless opts[:by_user] || opts[:by_group] || opts[:by_project] || opts[:by_package]
     reviews.each do |review|
-      if review.by_user && review.by_user == opts[:by_user] ||
-         review.by_group && review.by_group == opts[:by_group] ||
-         review.by_project && review.by_project == opts[:by_project] ||
-         review.by_package && review.by_package == opts[:by_package]
+      if review.reviewable_by?(opts)
         logger.debug "Obsoleting review #{review.id}"
         review.state = :obsoleted
         review.save

src/api/app/models/review.rb

@@ -224,6 +224,13 @@ def create_notification(params = {})
     Event::ReviewWanted.create params
   end
 
+  def reviewable_by?(opts)
+    by_user && by_user == opts[:by_user] ||
+      by_group && by_group == opts[:by_group] ||
+      by_project && by_project == opts[:by_project] ||
+      by_package && by_package == opts[:by_package]
+  end
+
   private
 
   # The authoritative storage are the by_ attributes as even when a record (project, package ...) got deleted

src/api/spec/models/review_spec.rb

@@ -401,4 +401,30 @@
       end
     end
   end
+
+  describe '#reviewable_by?' do
+    let(:other_user)    { create(:user, login: 'bob') }
+    let(:other_group)   { create(:group, title: 'my_group') }
+    let(:other_project) { create(:project_with_package, name: 'doc:things', package_name: 'less') }
+    let(:other_package) { other_project.packages.first }
+
+    let(:review_by_user)    { create(:review, by_user:    user.login) }
+    let(:review_by_group)   { create(:review, by_group:   group.title) }
+    let(:review_by_project) { create(:review, by_project: project.name) }
+    let(:review_by_package) { create(:review, by_project: project.name, by_package: package.name) }
+
+    it 'returns true if review configuration matches provided hash' do
+      expect(review_by_user.reviewable_by?({    by_user:    user.login   })).to be true
+      expect(review_by_group.reviewable_by?({   by_group:   group.title  })).to be true
+      expect(review_by_project.reviewable_by?({ by_project: project.name })).to be true
+      expect(review_by_package.reviewable_by?({ by_package: package.name })).to be true
+    end
+
+    it 'returns false if review configuration does not match provided hash' do
+      expect(review_by_user.reviewable_by?({    by_user:    other_user.login   })).to be_falsy
+      expect(review_by_group.reviewable_by?({   by_group:   other_group.title  })).to be_falsy
+      expect(review_by_project.reviewable_by?({ by_project: other_project.name })).to be_falsy
+      expect(review_by_package.reviewable_by?({ by_package: other_package.name })).to be_falsy
+    end
+  end
 end