Merge branch '219023-jira-users-mapping' into 'master'

Match Jira users by email  and username/name

See merge request gitlab-org/gitlab!33883

app/models/user.rb

@@ -353,6 +353,9 @@ class User < ApplicationRecord
   scope :deactivated, -> { with_state(:deactivated).non_internal }
   scope :without_projects, -> { joins('LEFT JOIN project_authorizations ON users.id = project_authorizations.user_id').where(project_authorizations: { user_id: nil }) }
   scope :by_username, -> (usernames) { iwhere(username: Array(usernames).map(&:to_s)) }
+  scope :by_name, -> (names) { iwhere(name: Array(names)) }
+  scope :by_user_email, -> (emails) { iwhere(email: Array(emails)) }
+  scope :by_emails, -> (emails) { joins(:emails).where(emails: { email: Array(emails).map(&:downcase) }) }
   scope :for_todos, -> (todos) { where(id: todos.select(:user_id)) }
   scope :with_emails, -> { preload(:emails) }
   scope :with_dashboard, -> (dashboard) { where(dashboard: dashboard) }
@@ -517,17 +520,15 @@ class User < ApplicationRecord
     # @param emails [String, Array<String>] email addresses to check
     # @param confirmed [Boolean] Only return users where the email is confirmed
     def by_any_email(emails, confirmed: false)
-      emails = Array(emails).map(&:downcase)
-
-      from_users = where(email: emails)
+      from_users = by_user_email(emails)
       from_users = from_users.confirmed if confirmed
 
-      from_emails = joins(:emails).where(emails: { email: emails })
+      from_emails = by_emails(emails)
       from_emails = from_emails.confirmed.merge(Email.confirmed) if confirmed
 
       items = [from_users, from_emails]
 
-      user_ids = Gitlab::PrivateCommitEmail.user_ids_for_emails(emails)
+      user_ids = Gitlab::PrivateCommitEmail.user_ids_for_emails(Array(emails).map(&:downcase))
       items << where(id: user_ids) if user_ids.present?
 
       from_union(items)

app/services/jira_import/users_importer.rb

@@ -2,8 +2,6 @@
 
 module JiraImport
   class UsersImporter
-    attr_reader :user, :project, :start_at
-
     def initialize(user, project, start_at)
       @project = project
       @start_at = start_at
@@ -23,6 +21,8 @@ module JiraImport
 
     private
 
+    attr_reader :user, :project, :start_at
+
     def mapped_users
       users_mapper_service.execute
     end
@@ -44,9 +44,9 @@ module JiraImport
       # TODO: use deployment_type enum from jira service when https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37003 is merged
       case deployment_type.upcase
       when JiraService::DEPLOYMENT_TYPES[:server]
-        ServerUsersMapperService.new(project.jira_service, start_at)
+        ServerUsersMapperService.new(user, project, start_at)
       when JiraService::DEPLOYMENT_TYPES[:cloud]
-        CloudUsersMapperService.new(project.jira_service, start_at)
+        CloudUsersMapperService.new(user, project, start_at)
       else
         raise ArgumentError
       end

app/services/jira_import/users_mapper_service.rb

@@ -2,30 +2,37 @@
 
 module JiraImport
   class UsersMapperService
+    include Gitlab::Utils::StrongMemoize
+
     # MAX_USERS must match the pageSize value in app/assets/javascripts/jira_import/utils/constants.js
     MAX_USERS = 50
 
-    attr_reader :jira_service, :start_at
-
-    def initialize(jira_service, start_at)
-      @jira_service = jira_service
+    # The class is called from UsersImporter and small batches of users are expected
+    # In case the mapping of a big batch of users is expected to be passed here
+    # the implementation needs to change here and handles the matching in batches
+    def initialize(current_user, project, start_at)
+      @current_user = current_user
+      @project = project
+      @jira_service = project.jira_service
       @start_at = start_at
     end
 
     def execute
-      users.to_a.map do |jira_user|
+      jira_users.to_a.map do |jira_user|
         {
           jira_account_id: jira_user_id(jira_user),
           jira_display_name: jira_user_name(jira_user),
           jira_email: jira_user['emailAddress']
-        }.merge(match_user(jira_user))
+        }.merge(gitlab_id: find_gitlab_id(jira_user))
       end
     end
 
     private
 
-    def users
-      @users ||= client.get(url)
+    attr_reader :current_user, :project, :jira_service, :start_at
+
+    def jira_users
+      @jira_users ||= client.get(url)
     end
 
     def client
@@ -44,10 +51,33 @@ module JiraImport
       raise NotImplementedError
     end
 
-    # TODO: Matching user by email and displayName will be done as the part
-    # of follow-up issue: https://gitlab.com/gitlab-org/gitlab/-/issues/219023
-    def match_user(jira_user)
-      { gitlab_id: nil, gitlab_username: nil, gitlab_name: nil }
+    def matched_users
+      strong_memoize(:matched_users) do
+        jira_emails = jira_users.map { |u| u['emailAddress']&.downcase }.compact
+        jira_names = jira_users.map { |u| jira_user_name(u)&.downcase }.compact
+
+        relations = []
+        relations << User.by_username(jira_names).select("users.id, users.name, users.username, users.email as user_email")
+        relations << User.by_name(jira_names).select("users.id, users.name, users.username, users.email as user_email")
+        relations << User.by_user_email(jira_emails).select("users.id, users.name, users.username, users.email as user_email")
+        relations << User.by_emails(jira_emails).select("users.id, users.name, users.username, emails.email as user_email")
+
+        User.from_union(relations).id_in(project_member_ids).select("users.id as user_id, users.name as name, users.username as username, user_email")
+      end
+    end
+
+    def find_gitlab_id(jira_user)
+      user = matched_users.find do |matched_user|
+        matched_user.user_email&.downcase == jira_user['emailAddress']&.downcase ||
+          matched_user.name&.downcase == jira_user_name(jira_user)&.downcase ||
+          matched_user.username&.downcase == jira_user_name(jira_user)&.downcase
+      end
+
+      user&.user_id
+    end
+
+    def project_member_ids
+      @project_member_ids ||= MembersFinder.new(project, current_user).execute.select(:user_id)
     end
   end
 end

changelogs/unreleased/219023-jira-users-mapping.yml

+---
+title: Match Jira users by email, username or name on jira issues import
+merge_request: 33883
+author:
+type: changed

spec/services/jira_import/cloud_users_mapper_service_spec.rb

@@ -5,15 +5,44 @@ require 'spec_helper'
 RSpec.describe JiraImport::CloudUsersMapperService do
   let(:start_at) { 7 }
   let(:url) { "/rest/api/2/users?maxResults=50&startAt=#{start_at}" }
+
+  let_it_be(:user_1) { create(:user, username: 'randomuser', name: 'USER-name1', email: 'uji@example.com') }
+  let_it_be(:user_2) { create(:user, username: 'username-2') }
+  let_it_be(:user_5) { create(:user, username: 'username-5') }
+  let_it_be(:user_4) { create(:user, email: 'user-4@example.com') }
+  let_it_be(:user_6) { create(:user, email: 'user-6@example.com') }
+  let_it_be(:user_7) { create(:user, username: 'username-7') }
+  let_it_be(:user_8) do
+    create(:user).tap { |user| create(:email, user: user, email: 'user8_email@example.com') }
+  end
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:group)        { create(:group) }
+  let_it_be(:project)      { create(:project, group: group) }
+
   let(:jira_users) do
     [
-      { 'accountId' => 'abcd', 'displayName' => 'user1' },
-      { 'accountId' => 'efg' },
-      { 'accountId' => 'hij', 'displayName' => 'user3', 'emailAddress' => 'user3@example.com' }
+      { 'accountId' => 'abcd', 'displayName' => 'User-Name1' }, # matched by name
+      { 'accountId' => 'efg', 'displayName' => 'username-2' }, # matcher by username
+      { 'accountId' => 'hij' }, # no match
+      { 'accountId' => '123', 'displayName' => 'user-4', 'emailAddress' => 'user-4@example.com' }, # matched by email
+      { 'accountId' => '456', 'displayName' => 'username5foo', 'emailAddress' => 'user-5@example.com' }, # no match
+      { 'accountId' => '789', 'displayName' => 'user-6', 'emailAddress' => 'user-6@example.com' }, # matched by email, no project member
+      { 'accountId' => 'xyz', 'displayName' => 'username-7', 'emailAddress' => 'user-7@example.com' }, # matched by username, no project member
+      { 'accountId' => 'vhk', 'displayName' => 'user-8', 'emailAddress' => 'user8_email@example.com' }, # matched by secondary email
+      { 'accountId' => 'uji', 'displayName' => 'user-9', 'emailAddress' => 'uji@example.com' } # matched by email, same as user_1
     ]
   end
 
   describe '#execute' do
+    before do
+      project.add_developer(current_user)
+      project.add_developer(user_1)
+      project.add_developer(user_2)
+      group.add_developer(user_4)
+      group.add_guest(user_8)
+    end
+
     it_behaves_like 'mapping jira users'
   end
 end

spec/services/jira_import/server_users_mapper_service_spec.rb

@@ -5,15 +5,44 @@ require 'spec_helper'
 RSpec.describe JiraImport::ServerUsersMapperService do
   let(:start_at) { 7 }
   let(:url) { "/rest/api/2/user/search?username=''&maxResults=50&startAt=#{start_at}" }
+
+  let_it_be(:user_1) { create(:user, username: 'randomuser', name: 'USER-name1', email: 'uji@example.com') }
+  let_it_be(:user_2) { create(:user, username: 'username-2') }
+  let_it_be(:user_5) { create(:user, username: 'username-5') }
+  let_it_be(:user_4) { create(:user, email: 'user-4@example.com') }
+  let_it_be(:user_6) { create(:user, email: 'user-6@example.com') }
+  let_it_be(:user_7) { create(:user, username: 'username-7') }
+  let_it_be(:user_8) do
+    create(:user).tap { |user| create(:email, user: user, email: 'user8_email@example.com') }
+  end
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:group)        { create(:group) }
+  let_it_be(:project)      { create(:project, group: group) }
+
   let(:jira_users) do
     [
-      { 'key' => 'abcd', 'name' => 'user1' },
-      { 'key' => 'efg' },
-      { 'key' => 'hij', 'name' => 'user3', 'emailAddress' => 'user3@example.com' }
+      { 'key' => 'abcd', 'name' => 'User-Name1' }, # matched by name
+      { 'key' => 'efg', 'name' => 'username-2' }, # matcher by username
+      { 'key' => 'hij' }, # no match
+      { 'key' => '123', 'name' => 'user-4', 'emailAddress' => 'user-4@example.com' }, # matched by email
+      { 'key' => '456', 'name' => 'username5foo', 'emailAddress' => 'user-5@example.com' }, # no match
+      { 'key' => '789', 'name' => 'user-6', 'emailAddress' => 'user-6@example.com' }, # matched by email, no project member
+      { 'key' => 'xyz', 'name' => 'username-7', 'emailAddress' => 'user-7@example.com' }, # matched by username, no project member
+      { 'key' => 'vhk', 'name' => 'user-8', 'emailAddress' => 'user8_email@example.com' }, # matched by secondary email
+      { 'key' => 'uji', 'name' => 'user-9', 'emailAddress' => 'uji@example.com' } # matched by email, same as user_1
     ]
   end
 
   describe '#execute' do
+    before do
+      project.add_developer(current_user)
+      project.add_developer(user_1)
+      project.add_developer(user_2)
+      group.add_developer(user_4)
+      group.add_guest(user_8)
+    end
+
     it_behaves_like 'mapping jira users'
   end
 end

spec/services/jira_import/users_importer_spec.rb

@@ -6,7 +6,8 @@ RSpec.describe JiraImport::UsersImporter do
   include JiraServiceHelper
 
   let_it_be(:user) { create(:user) }
-  let_it_be(:project, reload: true) { create(:project) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project, reload: true) { create(:project, group: group) }
   let_it_be(:start_at) { 7 }
 
   let(:importer) { described_class.new(user, project, start_at) }
@@ -18,19 +19,15 @@ RSpec.describe JiraImport::UsersImporter do
       [
         {
           jira_account_id: 'acc1',
-          jira_display_name: 'user1',
+          jira_display_name: 'user-name1',
           jira_email: 'sample@jira.com',
-          gitlab_id: nil,
-          gitlab_username: nil,
-          gitlab_name: nil
+          gitlab_id: project_member.id
         },
         {
           jira_account_id: 'acc2',
-          jira_display_name: 'user2',
+          jira_display_name: 'user-name2',
           jira_email: nil,
-          gitlab_id: nil,
-          gitlab_username: nil,
-          gitlab_name: nil
+          gitlab_id: group_member.id
         }
       ]
     end
@@ -69,13 +66,22 @@ RSpec.describe JiraImport::UsersImporter do
           context 'when jira client returns an empty array' do
             let(:jira_users) { [] }
 
-            it 'retturns nil payload' do
+            it 'returns nil payload' do
               expect(subject.success?).to be_truthy
               expect(subject.payload).to be_empty
             end
           end
 
           context 'when jira client returns an results' do
+            let_it_be(:project_member) { create(:user, email: 'sample@jira.com') }
+            let_it_be(:group_member) { create(:user, name: 'user-name2') }
+            let_it_be(:other_user) { create(:user) }
+
+            before do
+              project.add_developer(project_member)
+              group.add_developer(group_member)
+            end
+
             it 'returns the mapped users' do
               expect(subject.success?).to be_truthy
               expect(subject.payload).to eq(mapped_users)
@@ -90,8 +96,8 @@ RSpec.describe JiraImport::UsersImporter do
       let(:url) { "/rest/api/2/user/search?username=''&maxResults=50&startAt=#{start_at}" }
       let(:jira_users) do
         [
-          { 'key' => 'acc1', 'name' => 'user1', 'emailAddress' => 'sample@jira.com' },
-          { 'key' => 'acc2', 'name' => 'user2' }
+          { 'key' => 'acc1', 'name' => 'user-name1', 'emailAddress' => 'sample@jira.com' },
+          { 'key' => 'acc2', 'name' => 'user-name2' }
         ]
       end
 
@@ -110,8 +116,8 @@ RSpec.describe JiraImport::UsersImporter do
       let(:url) { "/rest/api/2/users?maxResults=50&startAt=#{start_at}" }
       let(:jira_users) do
         [
-          { 'accountId' => 'acc1', 'displayName' => 'user1', 'emailAddress' => 'sample@jira.com' },
-          { 'accountId' => 'acc2', 'displayName' => 'user2' }
+          { 'accountId' => 'acc1', 'displayName' => 'user-name1', 'emailAddress' => 'sample@jira.com' },
+          { 'accountId' => 'acc2', 'displayName' => 'user-name2' }
         ]
       end
 

spec/support/shared_examples/services/jira_import/user_mapper_services_shared_examples.rb

@@ -3,7 +3,6 @@
 RSpec.shared_examples 'mapping jira users' do
   let(:client) { double }
 
-  let_it_be(:project) { create(:project) }
   let_it_be(:jira_service) { create(:jira_service, project: project, active: true) }
 
   before do
@@ -11,7 +10,7 @@ RSpec.shared_examples 'mapping jira users' do
     allow(client).to receive(:get).with(url).and_return(jira_users)
   end
 
-  subject { described_class.new(jira_service, start_at) }
+  subject { described_class.new(current_user, project, start_at) }
 
   context 'jira_users is nil' do
     let(:jira_users) { nil }
@@ -22,18 +21,27 @@ RSpec.shared_examples 'mapping jira users' do
   end
 
   context 'when jira_users is present' do
-    # TODO: now we only create an array in a proper format
-    # mapping is tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/219023
     let(:mapped_users) do
       [
-        { jira_account_id: 'abcd', jira_display_name: 'user1', jira_email: nil, gitlab_id: nil, gitlab_username: nil, gitlab_name: nil },
-        { jira_account_id: 'efg', jira_display_name: nil, jira_email: nil, gitlab_id: nil, gitlab_username: nil, gitlab_name: nil },
-        { jira_account_id: 'hij', jira_display_name: 'user3', jira_email: 'user3@example.com', gitlab_id: nil, gitlab_username: nil, gitlab_name: nil }
+        { jira_account_id: 'abcd', jira_display_name: 'User-Name1', jira_email: nil, gitlab_id: user_1.id },
+        { jira_account_id: 'efg', jira_display_name: 'username-2', jira_email: nil, gitlab_id: user_2.id },
+        { jira_account_id: 'hij', jira_display_name: nil, jira_email: nil, gitlab_id: nil },
+        { jira_account_id: '123', jira_display_name: 'user-4', jira_email: 'user-4@example.com', gitlab_id: user_4.id },
+        { jira_account_id: '456', jira_display_name: 'username5foo', jira_email: 'user-5@example.com', gitlab_id: nil },
+        { jira_account_id: '789', jira_display_name: 'user-6', jira_email: 'user-6@example.com', gitlab_id: nil },
+        { jira_account_id: 'xyz', jira_display_name: 'username-7', jira_email: 'user-7@example.com', gitlab_id: nil },
+        { jira_account_id: 'vhk', jira_display_name: 'user-8', jira_email: 'user8_email@example.com', gitlab_id: user_8.id },
+        { jira_account_id: 'uji', jira_display_name: 'user-9', jira_email: 'uji@example.com', gitlab_id: user_1.id }
       ]
     end
 
     it 'returns users mapped to Gitlab' do
       expect(subject.execute).to eq(mapped_users)
     end
+
+    # 1 query for getting matched users, 3 queries for MembersFinder
+    it 'runs only 4 queries' do
+      expect { subject }.not_to exceed_query_limit(4)
+    end
   end
 end