Merge branch '37694-pullrequestimporter-should-not-use-deprecated-assignee_id-column' into 'master'

Remove deprecated assignee_id from PullRequestImporter

See merge request gitlab-org/gitlab!22144

lib/gitlab/bitbucket_import/importer.rb

@@ -182,7 +182,6 @@ module Gitlab
             target_branch_sha: target_branch_sha,
             state: pull_request.state,
             author_id: gitlab_user_id(project, pull_request.author),
-            assignee_id: nil,
             created_at: pull_request.created_at,
             updated_at: pull_request.updated_at
           )

lib/gitlab/bitbucket_server_import/importer.rb

@@ -211,7 +211,6 @@ module Gitlab
           target_branch_sha: pull_request.target_branch_sha,
           state_id: MergeRequest.available_states[pull_request.state],
           author_id: author_id,
-          assignee_id: nil,
           created_at: pull_request.created_at,
           updated_at: pull_request.updated_at
         }

lib/gitlab/github_import/importer/pull_request_importer.rb

@@ -27,6 +27,7 @@ module Gitlab
           mr, already_exists = create_merge_request
 
           if mr
+            set_merge_request_assignees(mr)
             insert_git_data(mr, already_exists)
             issuable_finder.cache_database_id(mr.id)
           end
@@ -57,7 +58,6 @@ module Gitlab
             state_id: ::MergeRequest.available_states[pull_request.state],
             milestone_id: milestone_finder.id_for(pull_request),
             author_id: author_id,
-            assignee_id: user_finder.assignee_id_for(pull_request),
             created_at: pull_request.created_at,
             updated_at: pull_request.updated_at
           }
@@ -65,6 +65,10 @@ module Gitlab
           create_merge_request_without_hooks(project, attributes, pull_request.iid)
         end
 
+        def set_merge_request_assignees(merge_request)
+          merge_request.assignee_ids = [user_finder.assignee_id_for(pull_request)]
+        end
+
         def insert_git_data(merge_request, already_exists)
           insert_or_replace_git_data(merge_request, pull_request.source_branch_sha, pull_request.target_branch_sha, already_exists)
           # We need to create the branch after the merge request is

spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb

@@ -49,6 +49,10 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
         .to receive(:create_merge_request)
         .and_return([mr, false])
 
+      expect(importer)
+        .to receive(:set_merge_request_assignees)
+        .with(mr)
+
       expect(importer)
         .to receive(:insert_git_data)
         .with(mr, false)
@@ -75,11 +79,6 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
           .to receive(:author_id_for)
           .with(pull_request)
           .and_return([user.id, true])
-
-        allow(importer.user_finder)
-          .to receive(:assignee_id_for)
-          .with(pull_request)
-          .and_return(user.id)
       end
 
       it 'imports the pull request with the pull request author as the merge request author' do
@@ -97,7 +96,6 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
               state_id: 3,
               milestone_id: milestone.id,
               author_id: user.id,
-              assignee_id: user.id,
               created_at: created_at,
               updated_at: updated_at
             },
@@ -114,20 +112,72 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
         expect(mr).to be_instance_of(MergeRequest)
         expect(exists).to eq(false)
       end
+
+      context 'when the source and target branch are identical' do
+        before do
+          allow(pull_request).to receive_messages(
+            source_repository_id: pull_request.target_repository_id,
+            source_branch: 'master'
+          )
+        end
+
+        it 'uses a generated source branch name for the merge request' do
+          expect(importer)
+            .to receive(:insert_and_return_id)
+            .with(
+              {
+                iid: 42,
+                title: 'My Pull Request',
+                description: 'This is my pull request',
+                source_project_id: project.id,
+                target_project_id: project.id,
+                source_branch: 'master-42',
+                target_branch: 'master',
+                state_id: 3,
+                milestone_id: milestone.id,
+                author_id: user.id,
+                created_at: created_at,
+                updated_at: updated_at
+              },
+              project.merge_requests
+            )
+            .and_call_original
+
+          importer.create_merge_request
+        end
+      end
+
+      context 'when the import fails due to a foreign key error' do
+        it 'does not raise any errors' do
+          expect(importer)
+            .to receive(:insert_and_return_id)
+            .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key')
+
+          expect { importer.create_merge_request }.not_to raise_error
+        end
+      end
+
+      context 'when the merge request already exists' do
+        it 'returns the existing merge request' do
+          mr1, exists1 = importer.create_merge_request
+          mr2, exists2 = importer.create_merge_request
+
+          expect(mr2).to eq(mr1)
+          expect(exists1).to eq(false)
+          expect(exists2).to eq(true)
+        end
+      end
     end
 
     context 'when the author could not be found' do
-      it 'imports the pull request with the project creator as the merge request author' do
+      before do
         allow(importer.user_finder)
           .to receive(:author_id_for)
           .with(pull_request)
           .and_return([project.creator_id, false])
+      end
 
-        allow(importer.user_finder)
-          .to receive(:assignee_id_for)
-          .with(pull_request)
-          .and_return(user.id)
-
+      it 'imports the pull request with the project creator as the merge request author' do
         expect(importer)
           .to receive(:insert_and_return_id)
           .with(
@@ -142,7 +192,6 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
               state_id: 3,
               milestone_id: milestone.id,
               author_id: project.creator_id,
-              assignee_id: user.id,
               created_at: created_at,
               updated_at: updated_at
             },
@@ -153,93 +202,33 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
         importer.create_merge_request
       end
     end
+  end
 
-    context 'when the source and target branch are identical' do
-      it 'uses a generated source branch name for the merge request' do
-        allow(importer.user_finder)
-          .to receive(:author_id_for)
-          .with(pull_request)
-          .and_return([user.id, true])
-
-        allow(importer.user_finder)
-          .to receive(:assignee_id_for)
-          .with(pull_request)
-          .and_return(user.id)
-
-        allow(pull_request)
-          .to receive(:source_repository_id)
-          .and_return(pull_request.target_repository_id)
-
-        allow(pull_request)
-          .to receive(:source_branch)
-          .and_return('master')
+  describe '#set_merge_request_assignees' do
+    let_it_be(:merge_request) { create(:merge_request) }
 
-        expect(importer)
-          .to receive(:insert_and_return_id)
-          .with(
-            {
-              iid: 42,
-              title: 'My Pull Request',
-              description: 'This is my pull request',
-              source_project_id: project.id,
-              target_project_id: project.id,
-              source_branch: 'master-42',
-              target_branch: 'master',
-              state_id: 3,
-              milestone_id: milestone.id,
-              author_id: user.id,
-              assignee_id: user.id,
-              created_at: created_at,
-              updated_at: updated_at
-            },
-            project.merge_requests
-          )
-          .and_call_original
+    before do
+      allow(importer.user_finder)
+        .to receive(:assignee_id_for)
+        .with(pull_request)
+        .and_return(user_id)
 
-        importer.create_merge_request
-      end
+      importer.set_merge_request_assignees(merge_request)
     end
 
-    context 'when the import fails due to a foreign key error' do
-      it 'does not raise any errors' do
-        allow(importer.user_finder)
-          .to receive(:author_id_for)
-          .with(pull_request)
-          .and_return([user.id, true])
-
-        allow(importer.user_finder)
-          .to receive(:assignee_id_for)
-          .with(pull_request)
-          .and_return(user.id)
-
-        expect(importer)
-          .to receive(:insert_and_return_id)
-          .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key')
+    context 'when pull request has an assignee' do
+      let(:user_id) { user.id }
 
-        expect { importer.create_merge_request }.not_to raise_error
+      it 'sets merge request assignees' do
+        expect(merge_request.assignee_ids).to eq [user.id]
       end
     end
 
-    context 'when the merge request already exists' do
-      before do
-        allow(importer.user_finder)
-          .to receive(:author_id_for)
-          .with(pull_request)
-          .and_return([user.id, true])
-
-        allow(importer.user_finder)
-          .to receive(:assignee_id_for)
-          .with(pull_request)
-          .and_return(user.id)
-      end
-
-      it 'returns the existing merge request' do
-        mr1, exists1 = importer.create_merge_request
-        mr2, exists2 = importer.create_merge_request
+    context 'when pull request does not have any assignees' do
+      let(:user_id) { nil }
 
-        expect(mr2).to eq(mr1)
-        expect(exists1).to eq(false)
-        expect(exists2).to eq(true)
+      it 'does not set merge request assignees' do
+        expect(merge_request.assignee_ids).to eq []
       end
     end
   end
@@ -255,11 +244,6 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
         .to receive(:author_id_for)
         .with(pull_request)
         .and_return([user.id, true])
-
-      allow(importer.user_finder)
-        .to receive(:assignee_id_for)
-        .with(pull_request)
-        .and_return(user.id)
     end
 
     it 'does not create the source branch if merge request is merged' do

spec/lib/gitlab/import/merge_request_helpers_spec.rb

@@ -19,8 +19,7 @@ describe Gitlab::Import::MergeRequestHelpers, type: :helper do
         source_branch: 'master-42',
         target_branch: 'master',
         state_id: 3,
-        author_id: user.id,
-        assignee_id: user.id
+        author_id: user.id
       }
     end