Merge branch '414904-exclude-archived-projects-from-merge-requests' into 'master'

Allow excluding archived projects from merge request search

See merge request gitlab-org/gitlab!126024



Merged-by: Madelein van Niekerk <mvanniekerk@gitlab.com>
Co-authored-by: Siddharth Dungarwal <sdungarwal@gitlab.com>

config/feature_flags/development/search_merge_requests_hide_archived_projects.yml

+---
+name: search_merge_requests_hide_archived_projects
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126024
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/417595
+milestone: '16.3'
+type: development
+group: group::global search
+default_enabled: false

ee/app/models/ee/project.rb

@@ -155,6 +155,8 @@ def lock_for_confirmation!(id)
       elastic_index_dependant_association :issues, on_change: :visibility_level
       elastic_index_dependant_association :issues, on_change: :archived, depends_on_finished_migration: :add_archived_to_issues
       elastic_index_dependant_association :merge_requests, on_change: :visibility_level
+      elastic_index_dependant_association :merge_requests, on_change: :archived,
+        depends_on_finished_migration: :add_archived_to_merge_requests
       elastic_index_dependant_association :notes, on_change: :visibility_level
       elastic_index_dependant_association :milestones, on_change: :visibility_level
 

ee/elastic/docs/20230710142700_add_archived_to_merge_requests.yml

+---
+name: AddArchivedToMergeRequests
+version: '20230710142700'
+description: Adds archvied (boolean) to the merge request index mapping
+group: group::global search
+milestone: 16.3
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126024
+obsolete: false
+marked_obsolete_by_url:
+marked_obsolete_in_milestone:

ee/elastic/docs/20230711140500_backfill_archived_on_merge_requests.yml

+---
+name: BackfillArchivedOnMergeRequests
+version: '20230711140500'
+description: Backfills archived for merge request documents
+group: group::global search
+milestone: 16.3
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126024
+obsolete: false
+marked_obsolete_by_url:
+marked_obsolete_in_milestone:

ee/elastic/migrate/20230710142700_add_archived_to_merge_requests.rb

+# frozen_string_literal: true
+
+class AddArchivedToMergeRequests < Elastic::Migration
+  include Elastic::MigrationUpdateMappingsHelper
+
+  private
+
+  def index_name
+    MergeRequest.__elasticsearch__.index_name
+  end
+
+  def new_mappings
+    {
+      archived: {
+        type: 'boolean'
+      }
+    }
+  end
+end

ee/elastic/migrate/20230711140500_backfill_archived_on_merge_requests.rb

+# frozen_string_literal: true
+
+class BackfillArchivedOnMergeRequests < Elastic::Migration
+  include Elastic::MigrationBackfillHelper
+
+  batched!
+  batch_size 9_000
+  throttle_delay 1.minute
+
+  DOCUMENT_TYPE = MergeRequest
+  UPDATE_BATCH_SIZE = 100
+
+  private
+
+  def index_name
+    DOCUMENT_TYPE.__elasticsearch__.index_name
+  end
+
+  def field_name
+    'archived'
+  end
+end

ee/lib/elastic/latest/application_class_proxy.rb

@@ -241,6 +241,13 @@ def remove_fields_boost(fields)
         fields.map { |m| m.split('^').first }
       end
 
+      def archived_filter(query_hash)
+        exclude_archived_query = { term: { archived: { _name: context.name(:non_archived), value: false } } }
+        query_hash[:query][:bool][:filter] << exclude_archived_query
+
+        query_hash
+      end
+
       # Builds an elasticsearch query that will select projects the user is
       # granted access to.
       #

ee/lib/elastic/latest/issue_class_proxy.rb

@@ -165,11 +165,6 @@ def hidden_filter(query_hash)
         query_hash
       end
 
-      def archived_filter(query_hash)
-        query_hash[:query][:bool][:filter] << { term: { archived: { _name: context.name(:non_archived), value: false } } }
-        query_hash
-      end
-
       def label_ids_filter(query_hash, options)
         labels = [options[:labels]].flatten
         return query_hash unless labels.any?

ee/lib/elastic/latest/merge_request_class_proxy.rb

@@ -24,6 +24,7 @@ def elastic_search(query, options: {})
         context.name(:merge_request) do
           query_hash = context.name(:authorized) { project_ids_filter(query_hash, options) }
           query_hash = context.name(:match) { state_filter(query_hash, options) }
+          query_hash = context.name(:archived) { archived_filter(query_hash) } if archived_filter_applicable?(options)
           if hidden_filter_applicable?(options[:current_user])
             query_hash = context.name(:hidden) { hidden_filter(query_hash) }
           end
@@ -69,6 +70,12 @@ def hidden_filter_applicable?(user)
         Feature.enabled?(:hide_merge_requests_from_banned_users) && !user&.can_admin_all_resources?
       end
 
+      def archived_filter_applicable?(options)
+        Feature.enabled?(:search_merge_requests_hide_archived_projects) &&
+          ::Elastic::DataMigrationService.migration_has_finished?(:backfill_archived_on_merge_requests) &&
+          !options[:include_archived]
+      end
+
       def hidden_filter(query_hash)
         if ::Elastic::DataMigrationService.migration_has_finished?(:backfill_hidden_on_merge_requests)
           query_hash[:query][:bool][:filter] << { term: { hidden: { _name: context.name(:non_hidden), value: false } } }

ee/lib/elastic/latest/merge_request_config.rb

@@ -37,6 +37,7 @@ module MergeRequestConfig
         indexes :updated_at, type: :date
 
         indexes :hidden, type: :boolean
+        indexes :archived, type: :boolean
         indexes :visibility_level, type: :integer
         indexes :merge_requests_access_level, type: :integer
         indexes :upvotes, type: :integer

ee/lib/elastic/latest/merge_request_instance_proxy.rb

@@ -39,6 +39,10 @@ def as_indexed_json(options = {})
           data['hidden'] = target.author&.banned?
         end
 
+        if ::Elastic::DataMigrationService.migration_has_finished?(:add_archived_to_merge_requests)
+          data['archived'] = target.project.archived?
+        end
+
         data.merge(generic_attributes)
       end
 

ee/lib/gitlab/elastic/search_results.rb

@@ -286,7 +286,7 @@ def scope_options(scope)
         when :projects
           base_options.merge(filters.slice(:include_archived))
         when :merge_requests
-          base_options.merge(filters.slice(:order_by, :sort, :state))
+          base_options.merge(filters.slice(:order_by, :sort, :state, :include_archived))
         when :issues
           base_options.merge(filters.slice(:order_by, :sort, :confidential, :state, :labels, :include_archived))
         when :milestones

ee/spec/elastic/migrate/20230710142700_add_archived_to_merge_requests_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative 'migration_shared_examples'
+require File.expand_path('ee/elastic/migrate/20230710142700_add_archived_to_merge_requests.rb')
+
+RSpec.describe AddArchivedToMergeRequests, :elastic, :sidekiq_inline, feature_category: :global_search do
+  let(:version) { 20230710142700 }
+
+  include_examples 'migration adds mapping'
+end

ee/spec/elastic/migrate/20230711140500_backfill_archived_on_merge_requests_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative 'migration_shared_examples'
+require File.expand_path('ee/elastic/migrate/20230711140500_backfill_archived_on_merge_requests.rb')
+
+RSpec.describe BackfillArchivedOnMergeRequests, :elastic_delete_by_query, :sidekiq_inline, feature_category: :global_search do
+  let(:version) { 20230711140500 }
+
+  include_examples 'migration backfills fields' do
+    let_it_be(:project) { create(:project, archived: true) }
+    let(:objects) { create_list(:merge_request, 3, :unique_branches, target_project: project, source_project: project) }
+    let(:namespace) { project.namespace }
+    let(:expected_fields) { { archived: project.archived? } }
+
+    let(:expected_throttle_delay) { 1.minute }
+    let(:expected_batch_size) { 9000 }
+  end
+end

ee/spec/lib/elastic/latest/merge_request_class_proxy_spec.rb

@@ -52,6 +52,37 @@
           it 'does not include merge_requests from the banned authors' do
             expect(elasticsearch_hit_ids(result)).to match_array([active_user_mr.id])
           end
+
+          it 'has the correct named queries' do
+            result.response
+
+            assert_named_queries(
+              'merge_request:match:search_terms',
+              'merge_request:archived:non_archived'
+            )
+          end
+
+          context 'when search_issues_hide_archived_projects is disabled' do
+            before do
+              stub_feature_flags(search_merge_requests_hide_archived_projects: false)
+            end
+
+            it 'does not have a filter for archived' do
+              result.response
+
+              assert_named_queries('merge_request:match:search_terms', without: ['merge_request:archived:non_archived'])
+            end
+          end
+
+          context 'when include_archived is set' do
+            let(:options) { { include_archived: true } }
+
+            it 'does not have a filter for archived' do
+              result.response
+
+              assert_named_queries('merge_request:match:search_terms', without: ['merge_request:archived:non_archived'])
+            end
+          end
         end
 
         context 'when current_user is anonymous' do

ee/spec/lib/gitlab/elastic/group_search_results_spec.rb

@@ -38,17 +38,23 @@
 
   context 'merge_requests search', :sidekiq_inline do
     let!(:project) { create(:project, :public, group: group) }
+    let_it_be(:unarchived_project) { create(:project, :public, group: group) }
+    let_it_be(:archived_project) { create(:project, :public, :archived, group: group) }
     let!(:opened_result) { create(:merge_request, :opened, source_project: project, title: 'foo opened') }
     let!(:closed_result) { create(:merge_request, :closed, source_project: project, title: 'foo closed') }
+    let!(:unarchived_result) { create(:merge_request, source_project: unarchived_project, title: 'foo unarchived') }
+    let!(:archived_result) { create(:merge_request, source_project: archived_project, title: 'foo archived') }
 
     let(:query) { 'foo' }
     let(:scope) { 'merge_requests' }
 
-    include_examples 'search results filtered by state' do
-      before do
-        ensure_elasticsearch_index!
-      end
+    before do
+      set_elasticsearch_migration_to(:backfill_archived_on_merge_requests, including: true)
+      ensure_elasticsearch_index!
     end
+
+    include_examples 'search results filtered by state'
+    include_examples 'search results filtered by archived', 'search_merge_requests_hide_archived_projects'
   end
 
   context 'blobs' do
@@ -58,12 +64,12 @@
   end
 
   context 'for projects' do
-    let!(:unarchived_project) { create(:project, :public, group: group) }
-    let!(:archived_project) { create(:project, :archived, :public, group: group) }
+    let!(:unarchived_result) { create(:project, :public, group: group) }
+    let!(:archived_result) { create(:project, :archived, :public, group: group) }
 
     let(:scope) { 'projects' }
 
-    it_behaves_like 'search results filtered by archived' do
+    it_behaves_like 'search results filtered by archived', 'search_projects_hide_archived' do
       before do
         ensure_elasticsearch_index!
       end

ee/spec/lib/gitlab/elastic/search_results_spec.rb

@@ -401,12 +401,12 @@
 
       context 'for projects' do
         let_it_be(:group) { create(:group) }
-        let!(:unarchived_project) { create(:project, :public, group: group) }
-        let!(:archived_project) { create(:project, :archived, :public, group: group) }
+        let!(:unarchived_result) { create(:project, :public, group: group) }
+        let!(:archived_result) { create(:project, :archived, :public, group: group) }
+        let(:scope) { 'projects' }
+        let(:results) { described_class.new(user, '*', [unarchived_result.id, archived_result.id], filters: filters) }
 
-        let(:results) { described_class.new(user, '*', [unarchived_project.id, archived_project.id], filters: filters) }
-
-        it_behaves_like 'search results filtered by archived' do
+        it_behaves_like 'search results filtered by archived', 'search_projects_hide_archived' do
           before do
             ensure_elasticsearch_index!
           end
@@ -802,16 +802,24 @@
 
     context 'filtering' do
       let!(:project) { create(:project, :public) }
+      let_it_be(:unarchived_project) { create(:project, :public) }
+      let_it_be(:archived_project) { create(:project, :public, :archived) }
       let!(:opened_result) { create(:merge_request, :opened, source_project: project, title: 'foo opened') }
       let!(:closed_result) { create(:merge_request, :closed, source_project: project, title: 'foo closed') }
+      let!(:unarchived_result) { create(:merge_request, source_project: unarchived_project, title: 'foo unarchived') }
+      let!(:archived_result) { create(:merge_request, source_project: archived_project, title: 'foo archived') }
+      let(:scope) { 'merge_requests' }
+      let(:project_ids) { [project.id, unarchived_project.id, archived_project.id] }
 
-      let(:results) { described_class.new(user, 'foo', [project.id], filters: filters) }
+      let(:results) { described_class.new(user, 'foo', project_ids, filters: filters) }
 
-      include_examples 'search results filtered by state' do
-        before do
-          ensure_elasticsearch_index!
-        end
+      before do
+        set_elasticsearch_migration_to(:backfill_archived_on_merge_requests, including: true)
+        ensure_elasticsearch_index!
       end
+
+      include_examples 'search results filtered by state'
+      include_examples 'search results filtered by archived', 'search_merge_requests_hide_archived_projects'
     end
 
     context 'ordering' do

ee/spec/models/concerns/elastic/merge_request_spec.rb

@@ -90,6 +90,8 @@
         'merge_requests_access_level' => ProjectFeature::ENABLED,
         'visibility_level' => Gitlab::VisibilityLevel::INTERNAL,
         'project_id' => merge_request.target_project.id,
+        'hidden' => merge_request.author.banned?,
+        'archived' => merge_request.target_project.archived?,
         'hashed_root_namespace_id' => merge_request.target_project.namespace.hashed_root_namespace_id })
     end
 
@@ -99,11 +101,15 @@
 
     it 'does not include hidden if add_hidden_to_merge_requests is not finished' do
       set_elasticsearch_migration_to :add_hidden_to_merge_requests, including: false
-      expect(merge_request.__elasticsearch__.as_indexed_json).to eq(expected_hash)
+      expect(merge_request.__elasticsearch__.as_indexed_json).to eq(expected_hash.except('hidden', 'archived'))
+    end
+
+    it 'does not include archived if add_archived_to_merge_requests is not finished' do
+      set_elasticsearch_migration_to :add_archived_to_merge_requests, including: false
+      expect(merge_request.__elasticsearch__.as_indexed_json).to eq(expected_hash.except('archived'))
     end
 
     it 'returns json with all needed elements' do
-      expected_hash['hidden'] = merge_request.author.banned?
       expect(merge_request.__elasticsearch__.as_indexed_json).to eq(expected_hash)
     end
   end

ee/spec/support/elastic_query_name_inspector.rb

@@ -15,4 +15,8 @@ def inspect(query)
   def has_named_query?(*expected_names)
     @names.superset?(expected_names.to_set)
   end
+
+  def excludes_named_query?(*unexpected_names)
+    unexpected_names.all? { |name| @names.exclude?(name) }
+  end
 end

ee/spec/support/helpers/elasticsearch_helpers.rb

 # frozen_string_literal: true
 
 module ElasticsearchHelpers
-  def assert_named_queries(*expected_names)
+  def assert_named_queries(*expected_names, without: [])
     es_host = Gitlab::CurrentSettings.elasticsearch_url.first
     search_uri = %r{#{es_host}/[\w-]+/_search}
 
@@ -15,12 +15,14 @@ def assert_named_queries(*expected_names)
 
       inspector.inspect(query)
       inspector.has_named_query?(*expected_names)
+      inspector.excludes_named_query?(*without)
     rescue ::JSON::ParserError
       false
     end
 
     a_named_query = a_request(:post, search_uri).with(&ensure_names_present)
-    message = "Expected a query with the following names: #{expected_names.inspect}"
+    message = "Expected a query with the names #{expected_names.inspect}"
+    message << " and without the names #{without.inspect}" if without.any?
     expect(a_named_query).to have_been_made.at_least_once, message
   end