Merge branch '24121_extract_yet_another_users_finder' into 'master'

Extract AutocompleteController#users into finder

Closes #24121

See merge request gitlab-org/gitlab-ce!13778

4 files changed, 169 insertions, 40 deletions

diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb
index dfc8bd0ba81..10e8e54f402 100644
--- a/app/controllers/autocomplete_controller.rb
+++ b/app/controllers/autocomplete_controller.rb
@@ -3,31 +3,10 @@ class AutocompleteController < ApplicationController
 
   skip_before_action :authenticate_user!, only: [:users, :award_emojis]
   before_action :load_project, only: [:users]
-  before_action :find_users, only: [:users]
+  before_action :load_group, only: [:users]
 
   def users
-    @users ||= User.none
-    @users = @users.active
-    @users = @users.reorder(:name)
-    @users = @users.search(params[:search]) if params[:search].present?
-    @users = @users.where.not(id: params[:skip_users]) if params[:skip_users].present?
-    @users = @users.page(params[:page]).per(params[:per_page])
-
-    if params[:todo_filter].present? && current_user
-      @users = @users.todo_authors(current_user.id, params[:todo_state_filter])
-    end
-
-    if params[:search].blank?
-      # Include current user if available to filter by "Me"
-      if params[:current_user].present? && current_user
-        @users = [current_user, *@users].uniq
-      end
-
-      if params[:author_id].present? && current_user
-        author = User.find_by_id(params[:author_id])
-        @users = [author, *@users].uniq if author
-      end
-    end
+    @users = AutocompleteUsersFinder.new(params: params, current_user: current_user, project: @project, group: @group).execute
 
     render json: @users, only: [:name, :username, :id], methods: [:avatar_url]
   end
@@ -60,26 +39,14 @@ class AutocompleteController < ApplicationController
 
   private
 
-  def find_users
-    @users =
-      if @project
-        user_ids = @project.team.users.pluck(:id)
-
-        if params[:author_id].present?
-          user_ids << params[:author_id]
-        end
-
-        User.where(id: user_ids)
-      elsif params[:group_id].present?
+  def load_group
+    @group ||= begin
+      if @project.blank? && params[:group_id].present?
         group = Group.find(params[:group_id])
         return render_404 unless can?(current_user, :read_group, group)
-
-        group.users
-      elsif current_user
-        User.all
-      else
-        User.none
+        group
       end
+    end
   end
 
   def load_project
diff --git a/app/finders/autocomplete_users_finder.rb b/app/finders/autocomplete_users_finder.rb
new file mode 100644
index 00000000000..b8f52e31926
--- /dev/null
+++ b/app/finders/autocomplete_users_finder.rb
@@ -0,0 +1,60 @@
+class AutocompleteUsersFinder
+  attr_reader :current_user, :project, :group, :search, :skip_users,
+              :page, :per_page, :author_id, :params
+
+  def initialize(params:, current_user:, project:, group:)
+    @current_user = current_user
+    @project = project
+    @group = group
+    @search = params[:search]
+    @skip_users = params[:skip_users]
+    @page = params[:page]
+    @per_page = params[:per_page]
+    @author_id = params[:author_id]
+    @params = params
+  end
+
+  def execute
+    items = find_users
+    items = items.active
+    items = items.reorder(:name)
+    items = items.search(search) if search.present?
+    items = items.where.not(id: skip_users) if skip_users.present?
+    items = items.page(page).per(per_page)
+
+    if params[:todo_filter].present? && current_user
+      items = items.todo_authors(current_user.id, params[:todo_state_filter])
+    end
+
+    if search.blank?
+      # Include current user if available to filter by "Me"
+      if params[:current_user].present? && current_user
+        items = [current_user, *items].uniq
+      end
+
+      if author_id.present? && current_user
+        author = User.find_by_id(author_id)
+        items = [author, *items].uniq if author
+      end
+    end
+
+    items
+  end
+
+  private
+
+  def find_users
+    return users_from_project if project
+    return group.users if group
+    return User.all if current_user
+
+    User.none
+  end
+
+  def users_from_project
+    user_ids = project.team.users.pluck(:id)
+    user_ids << author_id if author_id.present?
+
+    User.where(id: user_ids)
+  end
+end
diff --git a/changelogs/unreleased/24121_extract_yet_another_users_finder.yml b/changelogs/unreleased/24121_extract_yet_another_users_finder.yml
new file mode 100644
index 00000000000..e43e97303e2
--- /dev/null
+++ b/changelogs/unreleased/24121_extract_yet_another_users_finder.yml
@@ -0,0 +1,5 @@
+---
+title: Extract AutocompleteController#users into finder
+merge_request: 13778
+author: Maxim Rydkin, Mayra Cabrera
+type: other
diff --git a/spec/finders/autocomplete_users_finder_spec.rb b/spec/finders/autocomplete_users_finder_spec.rb
new file mode 100644
index 00000000000..684af74d750
--- /dev/null
+++ b/spec/finders/autocomplete_users_finder_spec.rb
@@ -0,0 +1,97 @@
+require 'spec_helper'
+
+describe AutocompleteUsersFinder do
+  describe '#execute' do
+    let!(:user1) { create(:user, username: 'johndoe') }
+    let!(:user2) { create(:user, :blocked, username: 'notsorandom') }
+    let!(:external_user) { create(:user, :external) }
+    let!(:omniauth_user) { create(:omniauth_user, provider: 'twitter', extern_uid: '123456') }
+    let(:current_user) { create(:user) }
+    let(:params) { {} }
+
+    let(:project) { nil }
+    let(:group) { nil }
+
+    subject { described_class.new(params: params, current_user: current_user, project: project, group: group).execute.to_a }
+
+    context 'when current_user not passed or nil' do
+      let(:current_user) { nil }
+
+      it { is_expected.to match_array([]) }
+    end
+
+    context 'when project passed' do
+      let(:project) { create(:project) }
+
+      it { is_expected.to match_array([project.owner]) }
+
+      context 'when author_id passed' do
+        let(:params) { { author_id: user2.id } }
+
+        it { is_expected.to match_array([project.owner, user2]) }
+      end
+    end
+
+    context 'when group passed and project not passed' do
+      let(:group) { create(:group, :public) }
+
+      before do
+        group.add_users([user1], GroupMember::DEVELOPER)
+      end
+
+      it { is_expected.to match_array([user1]) }
+    end
+
+    it { is_expected.to match_array([user1, external_user, omniauth_user, current_user]) }
+
+    context 'when filtered by search' do
+      let(:params) { { search: 'johndoe' } }
+
+      it { is_expected.to match_array([user1]) }
+    end
+
+    context 'when filtered by skip_users' do
+      let(:params) { { skip_users: [omniauth_user.id, current_user.id] } }
+
+      it { is_expected.to match_array([user1, external_user]) }
+    end
+
+    context 'when todos exist' do
+      let!(:pending_todo1) { create(:todo, user: current_user, author: user1, state: :pending) }
+      let!(:pending_todo2) { create(:todo, user: external_user, author: omniauth_user, state: :pending) }
+      let!(:done_todo1) { create(:todo, user: current_user, author: external_user, state: :done) }
+      let!(:done_todo2) { create(:todo, user: user1, author: external_user, state: :done) }
+
+      context 'when filtered by todo_filter without todo_state_filter' do
+        let(:params) { { todo_filter: true } }
+
+        it { is_expected.to match_array([]) }
+      end
+
+      context 'when filtered by todo_filter with pending todo_state_filter' do
+        let(:params) { { todo_filter: true, todo_state_filter: 'pending' } }
+
+        it { is_expected.to match_array([user1]) }
+      end
+
+      context 'when filtered by todo_filter with done todo_state_filter' do
+        let(:params) { { todo_filter: true, todo_state_filter: 'done' } }
+
+        it { is_expected.to match_array([external_user]) }
+      end
+    end
+
+    context 'when filtered by current_user' do
+      let(:current_user) { user2 }
+      let(:params) { { current_user: true } }
+
+      it { is_expected.to match_array([user2, user1, external_user, omniauth_user]) }
+    end
+
+    context 'when filtered by author_id' do
+      let(:params) { { author_id: user2.id } }
+
+      it { is_expected.to match_array([user2, user1, external_user, omniauth_user, current_user]) }
+    end
+  end
+end