Move flter code to finder

Change log entry

Change shadowed variable name

Review comments

Missed change

Remove duplicate check

Add wrong remove

Review comments

Adding test case

app/finders/group_projects_finder.rb

@@ -19,6 +19,9 @@
 #     personal: boolean
 #     search: string
 #     non_archived: boolean
+#     with_issues_enabled: boolean
+#     with_merge_requests_enabled: boolean
+#     min_access_level: int
 #
 class GroupProjectsFinder < ProjectsFinder
   DEFAULT_PROJECTS_LIMIT = 100
@@ -42,6 +45,12 @@ class GroupProjectsFinder < ProjectsFinder
 
   private
 
+  def filter_projects(collection)
+    projects = super
+    projects = by_feature_availability(projects)
+    projects
+  end
+
   def limit(collection)
     limit = options[:limit]
 
@@ -49,35 +58,37 @@ class GroupProjectsFinder < ProjectsFinder
   end
 
   def init_collection
-    projects = if current_user
-                 collection_with_user
-               else
-                 collection_without_user
-               end
+    projects =
+      if only_shared?
+        [shared_projects]
+      elsif only_owned?
+        [owned_projects]
+      else
+        [owned_projects, shared_projects]
+      end
+
+    projects.map! do |project_relation|
+      filter_by_visibility(project_relation)
+    end
 
     union(projects)
   end
 
-  def collection_with_user
-    if only_shared?
-      [shared_projects.public_or_visible_to_user(current_user)]
-    elsif only_owned?
-      [owned_projects.public_or_visible_to_user(current_user)]
-    else
-      [
-        owned_projects.public_or_visible_to_user(current_user),
-        shared_projects.public_or_visible_to_user(current_user)
-      ]
-    end
+  def by_feature_availability(projects)
+    projects = projects.with_issues_available_for_user(current_user) if params[:with_issues_enabled].present?
+    projects = projects.with_merge_requests_available_for_user(current_user) if params[:with_merge_requests_enabled].present?
+    projects
   end
 
-  def collection_without_user
-    if only_shared?
-      [shared_projects.public_only]
-    elsif only_owned?
-      [owned_projects.public_only]
+  def filter_by_visibility(relation)
+    if current_user
+      if min_access_level?
+        relation.visible_to_user_and_access_level(current_user, params[:min_access_level])
+      else
+        relation.public_or_visible_to_user(current_user)
+      end
     else
-      [shared_projects.public_only, owned_projects.public_only]
+      relation.public_only
     end
   end
 

changelogs/unreleased/grp-finder-code-refactor.yml

+---
+title: Move filter code into finder
+merge_request: 34470
+author: Ravishankar
+type: other

lib/api/groups.rb

@@ -76,9 +76,6 @@ module API
           params: project_finder_params,
           options: finder_options
         ).execute
-        projects = projects.with_issues_available_for_user(current_user) if params[:with_issues_enabled]
-        projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
-        projects = projects.visible_to_user_and_access_level(current_user, params[:min_access_level]) if params[:min_access_level]
         projects = reorder_projects(projects)
         paginate(projects)
       end

lib/api/helpers.rb

@@ -528,6 +528,8 @@ module API
 
     def project_finder_params_ce
       finder_params = project_finder_params_visibility_ce
+      finder_params[:with_issues_enabled] = true if params[:with_issues_enabled].present?
+      finder_params[:with_merge_requests_enabled] = true if params[:with_merge_requests_enabled].present?
       finder_params[:without_deleted] = true
       finder_params[:search] = params[:search] if params[:search]
       finder_params[:search_namespaces] = true if params[:search_namespaces].present?

spec/finders/group_projects_finder_spec.rb

@@ -46,6 +46,18 @@ RSpec.describe GroupProjectsFinder do
       context 'without subgroups projects' do
         it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, private_project, public_project]) }
       end
+
+      context "with min access level" do
+        let!(:shared_project_4) { create(:project, :internal, path: '8') }
+
+        before do
+          shared_project_4.project_group_links.create(group_access: Gitlab::Access::REPORTER, group: group)
+        end
+
+        let(:params) { { min_access_level: Gitlab::Access::MAINTAINER } }
+
+        it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, private_project, public_project]) }
+      end
     end
   end
 
@@ -171,6 +183,38 @@ RSpec.describe GroupProjectsFinder do
     end
   end
 
+  describe 'feature availability' do
+    let!(:project_with_issues_disabled) { create(:project, :issues_disabled, :internal, path: '9') }
+    let!(:project_with_merge_request_disabled) { create(:project, :merge_requests_disabled, :internal, path: '10') }
+
+    before do
+      project_with_issues_disabled.project_group_links.create!(group_access: Gitlab::Access::REPORTER, group: group)
+      project_with_merge_request_disabled.project_group_links.create!(group_access: Gitlab::Access::REPORTER, group: group)
+    end
+
+    context 'without issues and merge request enabled' do
+      it { is_expected.to match_array([public_project, shared_project_1, shared_project_3, project_with_issues_disabled, project_with_merge_request_disabled]) }
+    end
+
+    context 'with issues enabled' do
+      let(:params) { { with_issues_enabled: true } }
+
+      it { is_expected.to match_array([public_project, shared_project_1, shared_project_3, project_with_merge_request_disabled]) }
+    end
+
+    context 'with merge request enabled' do
+      let(:params) { { with_merge_requests_enabled: true } }
+
+      it { is_expected.to match_array([public_project, shared_project_1, shared_project_3, project_with_issues_disabled]) }
+    end
+
+    context 'with issues and merge request enabled' do
+      let(:params) { { with_merge_requests_enabled: true, with_issues_enabled: true } }
+
+      it { is_expected.to match_array([public_project, shared_project_1, shared_project_3]) }
+    end
+  end
+
   describe 'limiting' do
     context 'without limiting' do
       it 'returns all projects' do