Merge branch 'remove-username-fallthru-on-bb-import-for-pr' into 'master'

Update BitBucket Server importer username mapping

See merge request gitlab-org/gitlab!66653

doc/user/project/import/bitbucket_server.md

@@ -78,10 +78,7 @@ the author's:
 - `slug`
 - `displayName`
 
-If the user is not found by any of these properties, the search falls back to the author's
-`email` address.
-
-Alternatively, if there is also no email address, the project creator is set as the author.
+If the user is not found by any of these properties, the project creator is set as the author.
 
 ##### Enable or disable User assignment by username
 

lib/gitlab/bitbucket_server_import/importer.rb

@@ -461,10 +461,14 @@ module Gitlab
       end
 
       def uid(rep_object)
-        find_user_id(by: :email, value: rep_object.author_email) unless Feature.enabled?(:bitbucket_server_user_mapping_by_username)
-
-        find_user_id(by: :username, value: rep_object.author_username) ||
+        # We want this explicit to only be username on the FF
+        # Otherwise, match email.
+        # There should be no default fall-through on username. Fall-through to import user
+        if Feature.enabled?(:bitbucket_server_user_mapping_by_username)
+          find_user_id(by: :username, value: rep_object.author_username)
+        else
           find_user_id(by: :email, value: rep_object.author_email)
+        end
       end
     end
   end

spec/lib/gitlab/bitbucket_server_import/importer_spec.rb

@@ -142,7 +142,7 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
           expect { subject.execute }.to change { MergeRequest.count }.by(1)
 
           merge_request = MergeRequest.first
-          expect(merge_request.author).to eq(pull_request_author)
+          expect(merge_request.author).to eq(expected_author)
         end
       end
 
@@ -151,7 +151,25 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
           stub_feature_flags(bitbucket_server_user_mapping_by_username: false)
         end
 
-        include_examples 'imports pull requests'
+        context 'when email is not present' do
+          before do
+            allow(pull_request).to receive(:author_email).and_return(nil)
+          end
+
+          let(:expected_author) { project_creator }
+
+          include_examples 'imports pull requests'
+        end
+
+        context 'when email is present' do
+          before do
+            allow(pull_request).to receive(:author_email).and_return(pull_request_author.email)
+          end
+
+          let(:expected_author) { pull_request_author }
+
+          include_examples 'imports pull requests'
+        end
       end
 
       context 'when bitbucket_server_user_mapping_by_username feature flag is enabled' do
@@ -159,19 +177,24 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
           stub_feature_flags(bitbucket_server_user_mapping_by_username: true)
         end
 
-        include_examples 'imports pull requests' do
-          context 'when username is not present' do
-            before do
-              allow(pull_request).to receive(:author_username).and_return(nil)
-            end
+        context 'when username is not present' do
+          before do
+            allow(pull_request).to receive(:author_username).and_return(nil)
+          end
 
-            it 'maps by email' do
-              expect { subject.execute }.to change { MergeRequest.count }.by(1)
+          let(:expected_author) { project_creator }
 
-              merge_request = MergeRequest.first
-              expect(merge_request.author).to eq(pull_request_author)
-            end
+          include_examples 'imports pull requests'
+        end
+
+        context 'when username is present' do
+          before do
+            allow(pull_request).to receive(:author_username).and_return(pull_request_author.username)
           end
+
+          let(:expected_author) { pull_request_author }
+
+          include_examples 'imports pull requests'
         end
       end
 
@@ -228,7 +251,23 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
             allow(subject.client).to receive(:activities).and_return([pr_comment])
           end
 
-          it 'maps by email' do
+          it 'defaults to import user' do
+            expect { subject.execute }.to change { MergeRequest.count }.by(1)
+
+            merge_request = MergeRequest.first
+            expect(merge_request.notes.count).to eq(1)
+            note = merge_request.notes.first
+            expect(note.author).to eq(project_creator)
+          end
+        end
+
+        context 'when username is present' do
+          before do
+            allow(pr_note).to receive(:author_username).and_return(note_author.username)
+            allow(subject.client).to receive(:activities).and_return([pr_comment])
+          end
+
+          it 'maps by username' do
             expect { subject.execute }.to change { MergeRequest.count }.by(1)
 
             merge_request = MergeRequest.first
@@ -384,13 +423,13 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
               allow(inline_note).to receive(:author_username).and_return(nil)
             end
 
-            it 'maps by email' do
+            it 'defaults to import user' do
               expect { subject.execute }.to change { MergeRequest.count }.by(1)
 
               notes = MergeRequest.first.notes.order(:id).to_a
 
-              expect(notes.first.author).to eq(inline_note_author)
-              expect(notes.last.author).to eq(reply_author)
+              expect(notes.first.author).to eq(project_creator)
+              expect(notes.last.author).to eq(project_creator)
             end
           end
         end