Merge branch 'jira-user-importer-prep' into 'master'

Refactor mapping users for jira import

See merge request gitlab-org/gitlab!30820

changelogs/unreleased/jira-user-importer-prep.yml

+---
+title: Fix mapping group membets as Jira issues authors/assignees
+merge_request: 30820
+author:
+type: fixed

lib/gitlab/jira_import/issue_serializer.rb

@@ -51,32 +51,16 @@ module Gitlab
         end
       end
 
-      def map_user_id(email)
-        return unless email
-
-        # We also include emails that are not yet confirmed
-        users = User.by_any_email(email).to_a
-
-        # this event should never happen but we should log it in case we have invalid data
-        log_user_mapping_message('Multiple users found for an email address', email) if users.count > 1
-
-        user = users.first
-
-        unless project.project_member(user)
-          log_user_mapping_message('Jira user not found', email)
-
-          return
-        end
-
-        user.id
+      def map_user_id(jira_user)
+        Gitlab::JiraImport::UserMapper.new(project, jira_user).execute&.id
       end
 
       def reporter
-        map_user_id(jira_issue&.reporter&.emailAddress) || import_owner_id
+        map_user_id(jira_issue.reporter&.attrs) || import_owner_id
       end
 
       def assignees
-        found_user_id = map_user_id(jira_issue&.assignee&.emailAddress)
+        found_user_id = map_user_id(jira_issue.assignee&.attrs)
 
         return unless found_user_id
 
@@ -95,15 +79,6 @@ module Gitlab
       def logger
         @logger ||= Gitlab::Import::Logger.build
       end
-
-      def log_user_mapping_message(message, email)
-        logger.info(
-          project_id: project.id,
-          project_path: project.full_path,
-          user_email: email,
-          message: message
-        )
-      end
     end
   end
 end

lib/gitlab/jira_import/user_mapper.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module JiraImport
+    class UserMapper
+      include ::Gitlab::Utils::StrongMemoize
+
+      def initialize(project, jira_user)
+        @project = project
+        @jira_user = jira_user
+      end
+
+      def execute
+        return unless jira_user
+
+        email = jira_user['emailAddress']
+
+        # We also include emails that are not yet confirmed
+        users = User.by_any_email(email).to_a
+
+        user = users.first
+
+        # this event should never happen but we should log it in case we have invalid data
+        log_user_mapping_message('Multiple users found for an email address', email) if users.count > 1
+
+        unless project.project_member(user) || project.group&.group_member(user)
+          log_user_mapping_message('Jira user not found', email)
+
+          return
+        end
+
+        user
+      end
+
+      private
+
+      attr_reader :project, :jira_user, :params
+
+      def log_user_mapping_message(message, email)
+        logger.info(
+          project_id: project.id,
+          project_path: project.full_path,
+          user_email: email,
+          message: message
+        )
+      end
+
+      def logger
+        @logger ||= Gitlab::Import::Logger.build
+      end
+    end
+  end
+end

spec/lib/gitlab/jira_import/issue_serializer_spec.rb

@@ -17,8 +17,8 @@ describe Gitlab::JiraImport::IssueSerializer do
     let(:description) { 'basic description' }
     let(:created_at) { '2020-01-01 20:00:00' }
     let(:updated_at) { '2020-01-10 20:00:00' }
-    let(:assignee) { double(displayName: 'Solver', emailAddress: 'assignee@example.com') }
-    let(:reporter) { double(displayName: 'Reporter', emailAddress: 'reporter@example.com') }
+    let(:assignee) { double(attrs: { 'displayName' => 'Solver', 'emailAddress' => 'assignee@example.com' }) }
+    let(:reporter) { double(attrs: { 'displayName' => 'Reporter', 'emailAddress' => 'reporter@example.com' }) }
     let(:jira_status) { 'new' }
 
     let(:parent_field) do
@@ -109,7 +109,7 @@ describe Gitlab::JiraImport::IssueSerializer do
       end
 
       context 'author' do
-        context 'when reporter maps to a GitLab user who is a project member' do
+        context 'when reporter maps to a valid GitLab user' do
           let!(:user) { create(:user, email: 'reporter@example.com') }
 
           it 'sets the issue author to the mapped user' do
@@ -119,15 +119,7 @@ describe Gitlab::JiraImport::IssueSerializer do
           end
         end
 
-        context 'when reporter maps to a GitLab user who is not a project member' do
-          let!(:user) { create(:user, email: 'reporter@example.com') }
-
-          it 'defaults the issue author to project creator' do
-            expect(subject[:author_id]).to eq(current_user.id)
-          end
-        end
-
-        context 'when reporter does not map to a GitLab user' do
+        context 'when reporter does not map to a valid Gitlab user' do
           it 'defaults the issue author to project creator' do
             expect(subject[:author_id]).to eq(current_user.id)
           end
@@ -142,7 +134,7 @@ describe Gitlab::JiraImport::IssueSerializer do
         end
 
         context 'when reporter field is missing email address' do
-          let(:reporter) { double(name: 'Reporter', emailAddress: nil) }
+          let(:reporter) { double(attrs: { 'displayName' => 'Reporter' }) }
 
           it 'defaults the issue author to project creator' do
             expect(subject[:author_id]).to eq(current_user.id)
@@ -151,7 +143,7 @@ describe Gitlab::JiraImport::IssueSerializer do
       end
 
       context 'assignee' do
-        context 'when assignee maps to a GitLab user who is a project member' do
+        context 'when assignee maps to a valid GitLab user' do
           let!(:user) { create(:user, email: 'assignee@example.com') }
 
           it 'sets the issue assignees to the mapped user' do
@@ -161,15 +153,7 @@ describe Gitlab::JiraImport::IssueSerializer do
           end
         end
 
-        context 'when assignee maps to a GitLab user who is not a project member' do
-          let!(:user) { create(:user, email: 'assignee@example.com') }
-
-          it 'leaves the assignee empty' do
-            expect(subject[:assignee_ids]).to be_nil
-          end
-        end
-
-        context 'when assignee does not map to a GitLab user' do
+        context 'when assignee does not map to a valid GitLab user' do
           it 'leaves the assignee empty' do
             expect(subject[:assignee_ids]).to be_nil
           end
@@ -184,7 +168,7 @@ describe Gitlab::JiraImport::IssueSerializer do
         end
 
         context 'when assginee field is missing email address' do
-          let(:assignee) { double(name: 'Assignee', emailAddress: nil) }
+          let(:assignee) { double(attrs: { 'displayName' => 'Reporter' }) }
 
           it 'leaves the assignee empty' do
             expect(subject[:assignee_ids]).to be_nil

spec/lib/gitlab/jira_import/user_mapper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::JiraImport::UserMapper do
+  let_it_be(:group)   { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:user)    { create(:user, email: 'user@example.com') }
+  let_it_be(:email)   { create(:email, user: user, email: 'second_email@example.com', confirmed_at: nil) }
+
+  let(:jira_user) { { 'acountId' => '1a2b', 'emailAddress' => 'user@example.com' } }
+
+  describe '#execute' do
+    subject { described_class.new(project, jira_user).execute }
+
+    context 'when jira_user is nil' do
+      let(:jira_user) { nil }
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'when Gitlab user is not found by email' do
+      let(:jira_user) { { 'acountId' => '1a2b', 'emailAddress' => 'other@example.com' } }
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'when jira_user emailAddress is nil' do
+      let(:jira_user) { { 'acountId' => '1a2b', 'emailAddress' => nil } }
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'when jira_user emailAddress key is missing' do
+      let(:jira_user) { { 'acountId' => '1a2b' } }
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'when found user is not a project member' do
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'when found user is a project member' do
+      it 'returns the found user' do
+        project.add_developer(user)
+
+        expect(subject).to eq(user)
+      end
+    end
+
+    context 'when user found by unconfirmd secondary address is a project member' do
+      let(:jira_user) { { 'acountId' => '1a2b', 'emailAddress' => 'second_email@example.com' } }
+
+      it 'returns the found user' do
+        project.add_developer(user)
+
+        expect(subject).to eq(user)
+      end
+    end
+
+    context 'when user is a group member' do
+      it 'returns the found user' do
+        group.add_developer(user)
+
+        expect(subject).to eq(user)
+      end
+    end
+  end
+end