Merge pull request #6281 from bgeuken/missing_reviews_of_staging_proj…

…ects

Missing reviews of staging projects

src/api/app/models/staging/staging_project.rb

@@ -35,21 +35,26 @@ def missing_reviews
       return @missing_reviews if @missing_reviews
 
       @missing_reviews = []
-      attribs = [:by_group, :by_user, :by_project, :by_package]
-
-      (requests_to_review + staged_requests).uniq.each do |request|
-        request.reviews.each do |review|
-          next if review.state == :accepted || review.by_project == name
+      attribs = [:by_group, :by_user, :by_package, :by_project]
+
+      staged_requests.includes(:reviews).find_each do |request|
+        request.reviews.where.not(state: :accepted).find_each do |review|
+          # We skip reviews for the staging project since these reviews are used
+          # by the openSUSE release tools _after_ the overall_state switched to
+          # 'accepted'.
+          next if review.by_project == name
           # FIXME: this loop (and the inner if) would not be needed
           # if every review only has one valid by_xxx.
           # I'm keeping it to mimic the python implementation.
           # Instead, we could have something like
           # who = review.by_group || review.by_user || review.by_project || review.by_package
           attribs.each do |att|
             who = review.send(att)
-            if who
-              @missing_reviews << { id: review.id, request: request.number, state: review.state.to_s, package: request.first_target_package, by: who }
-            end
+            next unless who
+
+            @missing_reviews << { id: review.id, request: request.number, state: review.state.to_s, package: request.first_target_package, by: who }
+            # No need to duplicate reviews
+            break
           end
         end
       end

src/api/spec/models/staging/staging_project_spec.rb

@@ -32,6 +32,37 @@
     status_report.update(uuid: '1234')
   end
 
+  describe '#missing_reviews' do
+    let(:other_user) { create(:confirmed_user) }
+    let(:other_package) { create(:package) }
+    let(:group) { create(:group) }
+    let!(:review_1) { create(:review, by_user:    other_user,            bs_request: submit_request) }
+    let!(:review_2) { create(:review, by_group:   group,                 bs_request: submit_request) }
+    let!(:review_3) { create(:review, by_project: other_package.project, bs_request: submit_request) }
+    let!(:review_4) { create(:review, by_package: other_package,         by_project: other_package.project, bs_request: submit_request) }
+
+    subject { staging_project.missing_reviews }
+
+    it 'contains all open reviews of staged requests' do
+      # rubocop:disable Style/BracesAroundHashParameters
+      expect(subject).to contain_exactly(
+        { id: review_1.id, request: submit_request.number, state: 'new', package: target_package.name, by: other_user.login },
+        { id: review_2.id, request: submit_request.number, state: 'new', package: target_package.name, by: group.title },
+        { id: review_3.id, request: submit_request.number, state: 'new', package: target_package.name, by: other_package.project.name },
+        { id: review_4.id, request: submit_request.number, state: 'new', package: target_package.name, by: other_package.name }
+      )
+      # rubocop:enable Style/BracesAroundHashParameters
+    end
+
+    context 'when there is an accepted review' do
+      before do
+        review_2.update(state: 'accepted')
+      end
+
+      it { expect(subject.map { |review| review[:id] }).not_to include(review_2.id) }
+    end
+  end
+
   describe '#staging_identifier' do
     before do
       staging_project.update(name: 'openSUSE_41:Staging:myStagingProject')