Merge branch 'extract-vulnerable-projects-finder' into 'master'

Extract a VulnerableProjectsFinder

See merge request gitlab-org/gitlab!23725

ee/app/controllers/groups/security/vulnerable_projects_controller.rb

@@ -6,12 +6,18 @@ class Groups::Security::VulnerableProjectsController < Groups::ApplicationContro
   alias_method :vulnerable, :group
 
   def index
-    projects = group.vulnerable_projects.non_archived.without_deleted.with_route
+    vulnerable_projects = ::Security::VulnerableProjectsFinder.new(projects).execute
 
-    vulnerable_projects = projects.map do |project|
+    presented_projects = vulnerable_projects.map do |project|
       ::Security::VulnerableProjectPresenter.new(project)
     end
 
-    render json: VulnerableProjectSerializer.new.represent(vulnerable_projects)
+    render json: VulnerableProjectSerializer.new.represent(presented_projects)
+  end
+
+  private
+
+  def projects
+    ::Project.for_group_and_its_subgroups(group).non_archived.without_deleted.with_route
   end
 end

ee/app/finders/security/vulnerable_projects_finder.rb

+# frozen_string_literal: true
+
+module Security
+  class VulnerableProjectsFinder
+    def initialize(projects)
+      @projects = projects
+    end
+
+    def execute
+      projects.where("EXISTS(?)", vulnerabilities) # rubocop:disable CodeReuse/ActiveRecord
+    end
+
+    private
+
+    attr_reader :projects
+
+    def vulnerabilities
+      ::Vulnerabilities::Occurrence
+        .select(1)
+        .undismissed
+        .scoped_project
+    end
+  end
+end

ee/app/models/ee/group.rb

@@ -150,15 +150,6 @@ module EE
       ip_restrictions.map(&:range).join(",")
     end
 
-    def vulnerable_projects
-      vulnerabilities = ::Vulnerabilities::Occurrence
-        .select(1)
-        .undismissed
-        .where('vulnerability_occurrences.project_id = projects.id')
-
-      ::Project.for_group_and_its_subgroups(self).where("EXISTS(?)", vulnerabilities)
-    end
-
     def human_ldap_access
       ::Gitlab::Access.options_with_owner.key(ldap_access)
     end

ee/spec/controllers/groups/security/vulnerable_projects_controller_spec.rb

@@ -43,6 +43,18 @@ describe Groups::Security::VulnerableProjectsController do
       expect(json_response.first['critical_vulnerability_count']).to eq(2)
     end
 
+    it 'includes projects in subgroups' do
+      subgroup = create(:group, parent: group)
+      project = create(:project, namespace: subgroup)
+      create(:vulnerabilities_occurrence, project: project)
+
+      subject
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response.count).to be(1)
+      expect(json_response.first['id']).to eq(project.id)
+    end
+
     it 'does not include archived or deleted projects' do
       archived_project = create(:project, :archived, namespace: group)
       deleted_project = create(:project, namespace: group, pending_delete: true)

ee/spec/factories/vulnerabilities/occurrences.rb

@@ -66,5 +66,11 @@ FactoryBot.define do
                project_fingerprint: finding.project_fingerprint)
       end
     end
+
+    ::Vulnerabilities::Occurrence::REPORT_TYPES.keys.each do |security_report_type|
+      trait security_report_type do
+        report_type { security_report_type }
+      end
+    end
   end
 end

ee/spec/finders/security/vulnerable_projects_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Security::VulnerableProjectsFinder do
+  describe '#execute' do
+    let(:projects) { Project.all }
+    let!(:safe_project) { create(:project) }
+    let(:vulnerable_project) { create(:project) }
+    let!(:vulnerability) { create(:vulnerabilities_occurrence, project: vulnerable_project) }
+
+    subject { described_class.new(projects).execute }
+
+    it 'returns the projects that have vulnerabilities from the collection of projects given to it' do
+      expect(subject).to contain_exactly(vulnerable_project)
+    end
+
+    it 'does not include projects that only have dismissed vulnerabilities' do
+      create(:vulnerabilities_occurrence, :dismissed, project: safe_project)
+
+      expect(subject).to contain_exactly(vulnerable_project)
+    end
+
+    it 'only uses 1 query' do
+      another_project = create(:project)
+      create(:vulnerabilities_occurrence, :dismissed, project: another_project)
+
+      expect { subject }.not_to exceed_query_limit(1)
+
+      expect(subject).to contain_exactly(vulnerable_project)
+    end
+  end
+end

ee/spec/models/group_spec.rb

@@ -261,60 +261,6 @@ describe Group do
     end
   end
 
-  describe '#vulnerable_projects' do
-    it "fetches the group's projects that have vulnerabilities" do
-      vulnerable_project = create(:project, namespace: group)
-      _safe_project = create(:project, namespace: group)
-      create(:vulnerabilities_occurrence, project: vulnerable_project)
-
-      vulnerable_projects = group.vulnerable_projects
-
-      expect(vulnerable_projects.count).to be(1)
-      expect(vulnerable_projects.first).to eq(vulnerable_project)
-    end
-
-    it 'includes projects in subgroups' do
-      subgroup = create(:group, parent: group)
-      project = create(:project, namespace: subgroup)
-      create(:vulnerabilities_occurrence, project: project)
-
-      vulnerable_projects = group.vulnerable_projects
-
-      expect(vulnerable_projects.count).to be(1)
-      expect(vulnerable_projects.first).to eq(project)
-    end
-
-    it 'does not include projects that only have dismissed vulnerabilities' do
-      project = create(:project, namespace: group)
-      vulnerability = create(:vulnerabilities_occurrence, report_type: :dast, project: project)
-      create(
-        :vulnerability_feedback,
-        category: :dast,
-        feedback_type: :dismissal,
-        project: project,
-        project_fingerprint: vulnerability.project_fingerprint
-      )
-
-      vulnerable_projects = group.vulnerable_projects
-
-      expect(vulnerable_projects).to be_empty
-    end
-
-    it 'only uses 1 query' do
-      project_one = create(:project, namespace: group)
-      project_two = create(:project, namespace: group)
-      create(:vulnerabilities_occurrence, project: project_one)
-      dismissed_vulnerability = create(:vulnerabilities_occurrence, project: project_two)
-      create(
-        :vulnerability_feedback,
-        project_fingerprint: dismissed_vulnerability.project_fingerprint,
-        feedback_type: :dismissal
-      )
-
-      expect { group.vulnerable_projects }.not_to exceed_query_limit(1)
-    end
-  end
-
   describe '#mark_ldap_sync_as_failed' do
     it 'sets the state to failed' do
       group.start_ldap_sync