Merge branch...

Merge branch '199089-sometimes-you-cannot-find-the-group-in-the-autofill-dropdown-for-elasticsearch-indexed-groups' into 'master'

Improve the ElasticSearch settings autocomplete results

See merge request gitlab-org/gitlab!29043

app/finders/autocomplete/routes_finder.rb

+# frozen_string_literal: true
+
+module Autocomplete
+  # Finder that returns a list of routes that match on the `path` attribute.
+  class RoutesFinder
+    attr_reader :current_user, :search
+
+    LIMIT = 20
+
+    def initialize(current_user, params = {})
+      @current_user = current_user
+      @search = params[:search]
+    end
+
+    def execute
+      return [] if @search.blank?
+
+      Route
+        .for_routable(routables)
+        .sort_by_path_length
+        .fuzzy_search(@search, [:path])
+        .limit(LIMIT) # rubocop: disable CodeReuse/ActiveRecord
+    end
+
+    private
+
+    def routables
+      raise NotImplementedError
+    end
+
+    class NamespacesOnly < self
+      def routables
+        return Namespace.all if current_user.admin?
+
+        current_user.namespaces
+      end
+    end
+
+    class ProjectsOnly < self
+      def routables
+        return Project.all if current_user.admin?
+
+        current_user.projects
+      end
+    end
+  end
+end

app/models/route.rb

@@ -2,9 +2,9 @@
 
 class Route < ApplicationRecord
   include CaseSensitivity
+  include Gitlab::SQL::Pattern
 
   belongs_to :source, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
-
   validates :source, presence: true
 
   validates :path,
@@ -19,6 +19,8 @@ class Route < ApplicationRecord
   after_update :rename_descendants
 
   scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") }
+  scope :for_routable, -> (routable) { where(source: routable) }
+  scope :sort_by_path_length, -> { order('LENGTH(routes.path)', :path) }
 
   def rename_descendants
     return unless saved_change_to_path? || saved_change_to_name?

app/serializers/route_entity.rb

+# frozen_string_literal: true
+
+class RouteEntity < Grape::Entity
+  expose :id
+  expose :source_id
+  expose :source_type
+  expose :path
+end

app/serializers/route_serializer.rb

+# frozen_string_literal: true
+
+class RouteSerializer < BaseSerializer
+  entity RouteEntity
+end

config/routes.rb

@@ -53,6 +53,8 @@ Rails.application.routes.draw do
 
   Gitlab.ee do
     get '/autocomplete/project_groups' => 'autocomplete#project_groups'
+    get '/autocomplete/project_routes' => 'autocomplete#project_routes'
+    get '/autocomplete/namespace_routes' => 'autocomplete#namespace_routes'
   end
 
   # Sign up

db/migrate/20200408133211_add_index_on_route_path_trigram.rb

+# frozen_string_literal: true
+
+class AddIndexOnRoutePathTrigram < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_routes_on_path_trigram'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :routes, :path, name: INDEX_NAME, using: :gin, opclass: { path: :gin_trgm_ops }
+  end
+
+  def down
+    remove_concurrent_index_by_name(:routes, INDEX_NAME)
+  end
+end

db/structure.sql

@@ -8566,6 +8566,8 @@ CREATE UNIQUE INDEX design_management_designs_versions_uniqueness ON public.desi
 
 CREATE INDEX design_user_mentions_on_design_id_and_note_id_index ON public.design_user_mentions USING btree (design_id, note_id);
 
+CREATE INDEX dev_index_route_on_path_trigram ON public.routes USING gin (path public.gin_trgm_ops);
+
 CREATE UNIQUE INDEX epic_user_mentions_on_epic_id_and_note_id_index ON public.epic_user_mentions USING btree (epic_id, note_id);
 
 CREATE UNIQUE INDEX epic_user_mentions_on_epic_id_index ON public.epic_user_mentions USING btree (epic_id) WHERE (note_id IS NULL);
@@ -10116,6 +10118,8 @@ CREATE UNIQUE INDEX index_routes_on_path ON public.routes USING btree (path);
 
 CREATE INDEX index_routes_on_path_text_pattern_ops ON public.routes USING btree (path varchar_pattern_ops);
 
+CREATE INDEX index_routes_on_path_trigram ON public.routes USING gin (path public.gin_trgm_ops);
+
 CREATE UNIQUE INDEX index_routes_on_source_type_and_source_id ON public.routes USING btree (source_type, source_id);
 
 CREATE INDEX index_saml_providers_on_group_id ON public.saml_providers USING btree (group_id);
@@ -13158,6 +13162,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200407094005
 20200407094923
 20200408110856
+20200408133211
 20200408153842
 20200408175424
 20200409211607

ee/app/assets/javascripts/pages/admin/application_settings/integrations/index.js

 import 'select2/select2';
 import $ from 'jquery';
 import { s__ } from '~/locale';
-import Api from '~/api';
 import PersistentUserCallout from '~/persistent_user_callout';
 
 const onLimitCheckboxChange = (checked, $limitByNamespaces, $limitByProjects) => {
@@ -11,14 +10,14 @@ const onLimitCheckboxChange = (checked, $limitByNamespaces, $limitByProjects) =>
   $limitByProjects.toggleClass('hidden', !checked);
 };
 
-const getDropdownConfig = (placeholder, apiPath, textProp) => ({
+const getDropdownConfig = (placeholder, url) => ({
   placeholder,
   multiple: true,
   initSelection($el, callback) {
     callback($el.data('selected'));
   },
   ajax: {
-    url: Api.buildUrl(apiPath),
+    url,
     dataType: 'JSON',
     quietMillis: 250,
     data(search) {
@@ -29,8 +28,8 @@ const getDropdownConfig = (placeholder, apiPath, textProp) => ({
     results(data) {
       return {
         results: data.map(entity => ({
-          id: entity.id,
-          text: entity[textProp],
+          id: entity.source_id,
+          text: entity.path,
         })),
       };
     },
@@ -59,8 +58,7 @@ document.addEventListener('DOMContentLoaded', () => {
     .select2(
       getDropdownConfig(
         s__('Elastic|None. Select namespaces to index.'),
-        Api.namespacesPath,
-        'full_path',
+        '/autocomplete/namespace_routes.json',
       ),
     );
 
@@ -69,8 +67,7 @@ document.addEventListener('DOMContentLoaded', () => {
     .select2(
       getDropdownConfig(
         s__('Elastic|None. Select projects to index.'),
-        Api.projectsPath,
-        'name_with_namespace',
+        '/autocomplete/project_routes.json',
       ),
     );
 });

ee/app/controllers/ee/autocomplete_controller.rb

@@ -9,5 +9,21 @@ module EE
 
       render json: InvitedGroupSerializer.new.represent(groups)
     end
+
+    def project_routes
+      routes = ::Autocomplete::RoutesFinder::ProjectsOnly
+                 .new(current_user, params)
+                 .execute
+
+      render json: RouteSerializer.new.represent(routes)
+    end
+
+    def namespace_routes
+      routes = ::Autocomplete::RoutesFinder::NamespacesOnly
+                 .new(current_user, params)
+                 .execute
+
+      render json: RouteSerializer.new.represent(routes)
+    end
   end
 end

ee/app/helpers/ee/application_settings_helper.rb

@@ -56,7 +56,7 @@ module EE
     end
 
     def elasticsearch_objects_options(objects)
-      objects.map { |g| { id: g.id, text: g.full_name } }
+      objects.map { |g| { id: g.id, text: g.full_path } }
     end
 
     # The admin UI cannot handle so many namespaces so we just hide it. We

ee/app/models/elasticsearch_indexed_namespace.rb

@@ -19,7 +19,7 @@ class ElasticsearchIndexedNamespace < ApplicationRecord
   end
 
   def self.limited(ignore_descendants: false)
-    namespaces = Namespace.where(id: target_ids)
+    namespaces = Namespace.with_route.where(id: target_ids)
 
     return namespaces if ignore_descendants
 

ee/app/models/elasticsearch_indexed_project.rb

@@ -15,9 +15,9 @@ class ElasticsearchIndexedProject < ApplicationRecord
   end
 
   def self.limited(ignore_namespaces: false)
-    return Project.where(id: target_ids) if ignore_namespaces
+    return Project.inc_routes.where(id: target_ids) if ignore_namespaces
 
-    Project.from_union(
+    Project.inc_routes.from_union(
       [
         Project.where(namespace_id: ElasticsearchIndexedNamespace.limited.select(:id)),
         Project.where(id: target_ids)

ee/changelogs/unreleased/199089-sometimes-you-cannot-find-the-group-in-the-autofill-dropdown-fo.yml

+---
+title: Fix missing autocomplete results in the ElasticSearch admin UI.
+merge_request: 29043
+author: mbergeron
+type: fixed

ee/spec/controllers/autocomplete_controller_spec.rb

@@ -57,7 +57,7 @@ describe AutocompleteController do
     end
   end
 
-  context "groups" do
+  context 'groups' do
     let(:matching_group) { create(:group) }
     let(:non_matching_group) { create(:group) }
     let(:user2) { create(:user) }
@@ -97,4 +97,133 @@ describe AutocompleteController do
       it { expect(response).to be_not_found }
     end
   end
+
+  shared_examples 'has expected results' do
+    it 'returns the matching routes', :aggregate_failures do
+      expect(json_response).to be_kind_of(Array)
+      expect(json_response.size).to eq(expected_results.length)
+
+      json_response.each do |result|
+        expect(expected_results).to include(result.values_at('source_id', 'source_type'))
+      end
+    end
+  end
+
+  context 'GET project_routes' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:projects) { create_list(:project, 3, group: group) }
+
+    before do
+      sign_in(user)
+      get(:project_routes, params: { search: search })
+    end
+
+    context 'as admin' do
+      let(:user) { create(:admin) }
+
+      describe "while searching for a project by namespace" do
+        let(:search) { group.path }
+        let!(:expected_results) { group.projects.map { |p| [p.id, 'Project'] }}
+
+        include_examples 'has expected results'
+      end
+
+      describe "while searching for a project by path" do
+        let(:search) { projects.first.path }
+        let!(:expected_results) { [[projects.first.id, 'Project']] }
+
+        include_examples 'has expected results'
+      end
+    end
+
+    context 'as project owner' do
+      let(:user) { project.owner }
+      let!(:expected_results) { [[project.id, 'Project']] }
+
+      context "while searching for a project by namespace" do
+        let(:search) { user.namespace.path }
+
+        include_examples 'has expected results'
+      end
+
+      context "while searching for a project by path" do
+        let(:search) { project.path }
+
+        include_examples 'has expected results'
+      end
+    end
+
+    context 'while searching for nothing' do
+      let(:search) { nil }
+      let(:expected_results) { [] }
+
+      include_examples 'has expected results'
+    end
+  end
+
+  context 'GET namespace_routes' do
+    let_it_be(:groups) { create_list(:group, 3, :private) }
+    let_it_be(:users) { create_list(:user, 3) }
+
+    before do
+      sign_in(user)
+      get(:namespace_routes, params: { search: search })
+    end
+
+    context 'as admin' do
+      let(:user) { create(:admin) }
+
+      describe "while searching for a namespace by group path" do
+        let(:search) { 'group' }
+        let!(:expected_results) do
+          Group.all.map { |g| [g.id, 'Namespace'] }
+        end
+
+        include_examples 'has expected results'
+      end
+
+      describe "while searching for a namespace by user path" do
+        let(:search) { 'user' }
+        let!(:expected_results) do
+          User.all.map { |u| [u.namespace.id, 'Namespace'] }
+        end
+
+        include_examples 'has expected results'
+      end
+    end
+
+    context 'as a user' do
+      let(:search) { user.namespace.path }
+
+      context "while searching for a namespace by path" do
+        let!(:expected_results) { [[user.namespace.id, 'Namespace']] }
+
+        include_examples 'has expected results'
+      end
+    end
+
+    context 'as group member' do
+      let_it_be(:group_developer) do
+        groups.first.add_developer(users.first)
+
+        users.first
+      end
+
+      let(:search) { groups.first.path }
+      let(:user) { group_developer }
+
+      context "while searching for a namespace by path" do
+        let!(:expected_results) { [[groups.first.id, 'Namespace']] }
+
+        include_examples 'has expected results'
+      end
+    end
+
+    context 'while searching for nothing' do
+      let(:search) { nil }
+      let(:expected_results) { [] }
+
+      include_examples 'has expected results'
+    end
+  end
 end

ee/spec/features/admin/admin_settings_spec.rb

@@ -102,7 +102,7 @@ describe 'Admin updates EE-only settings' do
         expect(page).to have_content('Namespaces to index')
         expect(page).to have_content('Projects to index')
 
-        fill_in 'Namespaces to index', with: namespace.name
+        fill_in 'Namespaces to index', with: namespace.path
         wait_for_requests
       end
 
@@ -113,12 +113,12 @@ describe 'Admin updates EE-only settings' do
       page.within('.as-elasticsearch') do
         find('.js-limit-namespaces .select2-choices input[type=text]').native.send_keys(:enter)
 
-        fill_in 'Projects to index', with: project.name
+        fill_in 'Projects to index', with: project.path
         wait_for_requests
       end
 
       page.within('#select2-drop') do
-        expect(page).to have_content(project.full_name)
+        expect(page).to have_content(project.full_path)
       end
 
       page.within('.as-elasticsearch') do
@@ -148,14 +148,14 @@ describe 'Admin updates EE-only settings' do
 
         expect(page).to have_content('Namespaces to index')
         expect(page).to have_content('Projects to index')
-        expect(page).to have_content(namespace.full_name)
-        expect(page).to have_content(project.full_name)
+        expect(page).to have_content(namespace.full_path)
+        expect(page).to have_content(project.full_path)
 
         find('.js-limit-namespaces .select2-search-choice-close').click
         find('.js-limit-projects .select2-search-choice-close').click
 
-        expect(page).not_to have_content(namespace.full_name)
-        expect(page).not_to have_content(project.full_name)
+        expect(page).not_to have_content(namespace.full_path)
+        expect(page).not_to have_content(project.full_path)
 
         click_button 'Save changes'
       end