Merge branch '436320-map-by-username-gh' into 'master'

Move collaborators import to stage before pull requests import

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142732



Merged-by: Rodrigo Tomonari <rtomonari@gitlab.com>
Approved-by: Rodrigo Tomonari <rtomonari@gitlab.com>
Reviewed-by: George Koltsov <gkoltsov@gitlab.com>
Reviewed-by: Luke Duncalfe <lduncalfe@gitlab.com>
Co-authored-by: carlad-gl <cdrago@gitlab.com>

app/services/import/github_service.rb

@@ -142,7 +142,8 @@ def store_import_settings(project)
         .write(
           timeout_strategy: params[:timeout_strategy] || ProjectImportData::PESSIMISTIC_TIMEOUT,
           optional_stages: params[:optional_stages],
-          extended_events: Feature.enabled?(:github_import_extended_events, current_user)
+          extended_events: Feature.enabled?(:github_import_extended_events, current_user),
+          prioritize_collaborators: Feature.enabled?(:github_import_prioritize_collaborators, current_user)
         )
     end
   end

app/workers/gitlab/github_import/advance_stage_worker.rb

@@ -17,8 +17,9 @@ class AdvanceStageWorker # rubocop:disable Scalability/IdempotentWorker
 
       # The known importer stages and their corresponding Sidekiq workers.
       #
-      # Note: AdvanceStageWorker is not used for the repository, base_data, and pull_requests stages.
+      # Note: AdvanceStageWorker is not used to initiate the repository, base_data, and pull_requests stages.
       # They are included in the list for us to easily see all stage workers and the order in which they are executed.
+
       STAGES = {
         repository: Stage::ImportRepositoryWorker,
         base_data: Stage::ImportBaseDataWorker,

app/workers/gitlab/github_import/stage/import_base_data_worker.rb

@@ -26,7 +26,11 @@ def import(client, project)
             klass.new(project, client).execute
           end
 
-          ImportPullRequestsWorker.perform_async(project.id)
+          if import_settings(project).prioritize_collaborators?
+            ImportCollaboratorsWorker.perform_async(project.id)
+          else
+            ImportPullRequestsWorker.perform_async(project.id)
+          end
         end
       end
     end

app/workers/gitlab/github_import/stage/import_collaborators_worker.rb

@@ -47,9 +47,14 @@ def move_to_next_stage(project, waiters = {})
         end
 
         def next_stage(project)
-          return 'issues_and_diff_notes' if import_settings(project).extended_events?
+          if import_settings(project).prioritize_collaborators?
+            'pull_requests'
+          else
 
-          'pull_requests_merged_by'
+            return 'issues_and_diff_notes' if import_settings(project).extended_events?
+
+            'pull_requests_merged_by'
+          end
         end
       end
     end

app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb

@@ -26,11 +26,7 @@ def import(client, project)
             .new(project, client)
             .execute
 
-          AdvanceStageWorker.perform_async(
-            project.id,
-            { waiter.key => waiter.jobs_remaining },
-            'collaborators'
-          )
+          move_to_next_stage(project, { waiter.key => waiter.jobs_remaining })
         end
 
         private
@@ -45,6 +41,22 @@ def allocate_merge_requests_internal_id!(project, client)
 
           MergeRequest.track_target_project_iid!(project, last_github_pull_request[:number])
         end
+
+        def move_to_next_stage(project, waiters = {})
+          AdvanceStageWorker.perform_async(
+            project.id, waiters.deep_stringify_keys, next_stage(project)
+          )
+        end
+
+        def next_stage(project)
+          if import_settings(project).prioritize_collaborators?
+            return 'issues_and_diff_notes' if import_settings(project).extended_events?
+
+            'pull_requests_merged_by'
+          else
+            'collaborators'
+          end
+        end
       end
     end
   end

config/feature_flags/development/github_import_prioritize_collaborators.yml

+---
+name: github_import_prioritize_collaborators
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139410
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439742
+milestone: '16.9'
+type: development
+group: group::import and integrate
+default_enabled: false

lib/gitlab/github_import/settings.rb

@@ -67,7 +67,8 @@ def write(user_settings)
           data: {
             optional_stages: optional_stages,
             timeout_strategy: user_settings[:timeout_strategy],
-            extended_events: user_settings[:extended_events]
+            extended_events: user_settings[:extended_events],
+            prioritize_collaborators: user_settings[:prioritize_collaborators]
           },
           credentials: project.import_data&.credentials
         )
@@ -87,6 +88,10 @@ def extended_events?
         !!project.import_data&.data&.dig('extended_events')
       end
 
+      def prioritize_collaborators?
+        !!project.import_data&.data&.dig('prioritize_collaborators')
+      end
+
       private
 
       attr_reader :project

spec/lib/gitlab/github_import/settings_spec.rb

@@ -137,4 +137,24 @@
       expect(settings.extended_events?).to eq(false)
     end
   end
+
+  describe '#prioritize_collaborators?' do
+    it 'when prioritize_collaborators is set to true' do
+      project.build_or_assign_import_data(data: { prioritize_collaborators: true })
+
+      expect(settings.prioritize_collaborators?).to eq(true)
+    end
+
+    it 'when prioritize_collaborators is set to false' do
+      project.build_or_assign_import_data(data: { prioritize_collaborators: false })
+
+      expect(settings.prioritize_collaborators?).to eq(false)
+    end
+
+    it 'when prioritize_collaborators is not present' do
+      project.build_or_assign_import_data(data: {})
+
+      expect(settings.prioritize_collaborators?).to eq(false)
+    end
+  end
 end

spec/services/import/github_service_spec.rb

@@ -33,7 +33,8 @@
       .with(
         extended_events: true,
         optional_stages: optional_stages,
-        timeout_strategy: timeout_strategy
+        timeout_strategy: timeout_strategy,
+        prioritize_collaborators: true
       )
   end
 
@@ -94,7 +95,8 @@
           .to have_received(:write)
           .with(optional_stages: nil,
             extended_events: true,
-            timeout_strategy: timeout_strategy
+            timeout_strategy: timeout_strategy,
+            prioritize_collaborators: true
           )
         expect_snowplow_event(
           category: 'Import::GithubService',
@@ -120,7 +122,8 @@
           .with(
             optional_stages: nil,
             extended_events: true,
-            timeout_strategy: timeout_strategy
+            timeout_strategy: timeout_strategy,
+            prioritize_collaborators: true
           )
         expect_snowplow_event(
           category: 'Import::GithubService',
@@ -153,7 +156,8 @@
             .with(
               optional_stages: nil,
               extended_events: true,
-              timeout_strategy: timeout_strategy
+              timeout_strategy: timeout_strategy,
+              prioritize_collaborators: true
             )
           expect_snowplow_event(
             category: 'Import::GithubService',
@@ -190,7 +194,8 @@
           .with(
             optional_stages: optional_stages,
             extended_events: true,
-            timeout_strategy: timeout_strategy
+            timeout_strategy: timeout_strategy,
+            prioritize_collaborators: true
           )
       end
     end
@@ -206,7 +211,8 @@
           .with(
             optional_stages: optional_stages,
             extended_events: true,
-            timeout_strategy: timeout_strategy
+            timeout_strategy: timeout_strategy,
+            prioritize_collaborators: true
           )
       end
     end
@@ -220,12 +226,13 @@
           .with(
             optional_stages: optional_stages,
             extended_events: true,
-            timeout_strategy: timeout_strategy
+            timeout_strategy: timeout_strategy,
+            prioritize_collaborators: true
           )
       end
     end
 
-    context 'when `github_import_extended_events`` feature flag is disabled' do
+    context 'when `github_import_extended_events` feature flag is disabled' do
       before do
         stub_feature_flags(github_import_extended_events: false)
       end
@@ -238,6 +245,20 @@
         subject.execute(access_params, :github)
       end
     end
+
+    context 'when `prioritize_collaborators` feature flag is disabled' do
+      before do
+        stub_feature_flags(github_import_prioritize_collaborators: false)
+      end
+
+      it 'saves prioritize_collaborators to import_data' do
+        expect(settings)
+          .to receive(:write)
+          .with(a_hash_including(prioritize_collaborators: false))
+
+        subject.execute(access_params, :github)
+      end
+    end
   end
 
   context 'when import source is disabled' do

spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb

@@ -14,6 +14,10 @@
 
   describe '#import' do
     it 'imports the base data of a project' do
+      allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting|
+        allow(setting).to receive(:prioritize_collaborators?).and_return(true)
+      end
+
       described_class::IMPORTERS.each do |klass|
         expect(klass)
           .to receive(:new)
@@ -23,11 +27,34 @@
         expect(importer).to receive(:execute)
       end
 
-      expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker)
+      expect(Gitlab::GithubImport::Stage::ImportCollaboratorsWorker)
         .to receive(:perform_async)
         .with(project.id)
 
       worker.import(client, project)
     end
+
+    context 'when the prioritize_collaborators feature flag is disabled' do
+      it 'imports the base data of a project' do
+        allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting|
+          allow(setting).to receive(:prioritize_collaborators?).and_return(false)
+        end
+
+        described_class::IMPORTERS.each do |klass|
+          expect(klass)
+            .to receive(:new)
+            .with(project, client)
+            .and_return(importer)
+
+          expect(importer).to receive(:execute)
+        end
+
+        expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker)
+          .to receive(:perform_async)
+          .with(project.id)
+
+        worker.import(client, project)
+      end
+    end
   end
 end

spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb

@@ -21,6 +21,9 @@
       settings.write({ optional_stages: { collaborators_import: stage_enabled } })
       allow(client).to receive(:repository).with(project.import_source)
         .and_return({ permissions: { push: push_rights_granted } })
+      allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting|
+        allow(setting).to receive(:prioritize_collaborators?).and_return(true)
+      end
     end
 
     context 'when user has push access for this repo' do
@@ -35,7 +38,7 @@
 
         expect(Gitlab::GithubImport::AdvanceStageWorker)
           .to receive(:perform_async)
-          .with(project.id, { '123' => 2 }, 'pull_requests_merged_by')
+          .with(project.id, { '123' => 2 }, 'pull_requests')
 
         worker.import(client, project)
       end
@@ -49,7 +52,7 @@
 
         expect(Gitlab::GithubImport::AdvanceStageWorker)
           .to receive(:perform_async)
-          .with(project.id, {}, 'pull_requests_merged_by')
+          .with(project.id, {}, 'pull_requests')
 
         worker.import(client, project)
       end
@@ -63,7 +66,29 @@
 
         expect(Gitlab::GithubImport::AdvanceStageWorker)
           .to receive(:perform_async)
-          .with(project.id, {}, 'pull_requests_merged_by')
+          .with(project.id, {}, 'pull_requests')
+
+        worker.import(client, project)
+      end
+    end
+
+    context 'when prioritize_collaborators import setting is disabled' do
+      it 'imports all collaborators and advances to the correct next stage' do
+        allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting|
+          allow(setting).to receive(:prioritize_collaborators?).and_return(false)
+        end
+
+        waiter = Gitlab::JobWaiter.new(2, '123')
+
+        expect(Gitlab::GithubImport::Importer::CollaboratorsImporter)
+          .to receive(:new)
+          .with(project, client)
+          .and_return(importer)
+        expect(importer).to receive(:execute).and_return(waiter)
+
+        expect(Gitlab::GithubImport::AdvanceStageWorker)
+          .to receive(:perform_async)
+          .with(project.id, { '123' => 2 }, 'pull_requests_merged_by')
 
         worker.import(client, project)
       end

spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb

@@ -14,6 +14,10 @@
   it_behaves_like Gitlab::GithubImport::StageMethods
 
   describe '#import' do
+    before do
+      project.build_or_assign_import_data(data: { prioritize_collaborators: true })
+    end
+
     context 'with pull requests' do
       it 'imports all the pull requests and allocates internal iids' do
         waiter = Gitlab::JobWaiter.new(2, '123')
@@ -35,7 +39,7 @@
 
         expect(Gitlab::GithubImport::AdvanceStageWorker)
           .to receive(:perform_async)
-          .with(project.id, { '123' => 2 }, 'collaborators')
+          .with(project.id, { '123' => 2 }, 'pull_requests_merged_by')
 
         expect(MergeRequest).to receive(:track_target_project_iid!)
 
@@ -64,7 +68,7 @@
 
         expect(Gitlab::GithubImport::AdvanceStageWorker)
           .to receive(:perform_async)
-          .with(project.id, { '123' => 2 }, 'collaborators')
+          .with(project.id, { '123' => 2 }, 'pull_requests_merged_by')
 
         expect(MergeRequest).not_to receive(:track_target_project_iid!)
 
@@ -93,5 +97,38 @@
         worker.import(client, project)
       end
     end
+
+    context 'when prioritize_collaborators import setting is disabled' do
+      before do
+        project.build_or_assign_import_data(data: { prioritize_collaborators: false })
+      end
+
+      it 'advances to the correct next stage' do
+        waiter = Gitlab::JobWaiter.new(2, '123')
+
+        expect(Gitlab::GithubImport::Importer::PullRequestsImporter)
+          .to receive(:new)
+          .with(project, client)
+          .and_return(importer)
+
+        expect(importer)
+          .to receive(:execute)
+          .and_return(waiter)
+
+        expect(InternalId).to receive(:exists?).and_return(false)
+
+        expect(client).to receive(:each_object).with(
+          :pulls, project.import_source, options
+        ).and_return([{ number: 4 }].each)
+
+        expect(Gitlab::GithubImport::AdvanceStageWorker)
+          .to receive(:perform_async)
+          .with(project.id, { '123' => 2 }, 'collaborators')
+
+        expect(MergeRequest).to receive(:track_target_project_iid!)
+
+        worker.import(client, project)
+      end
+    end
   end
 end