Merge branch '322459-github-import-does-not-show-all-available-namespaces' into 'master'

Resolve "GitHub import does not show all available namespaces"

See merge request gitlab-org/gitlab!68270

app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue

@@ -32,7 +32,7 @@ export default {
   },
 
   computed: {
-    ...mapState(['filter', 'repositories', 'namespaces', 'defaultTargetNamespace']),
+    ...mapState(['filter', 'repositories', 'namespaces', 'defaultTargetNamespace', 'pageInfo']),
     ...mapGetters([
       'isLoading',
       'isImportingAnyRepo',
@@ -43,7 +43,7 @@ export default {
     ]),
 
     pagePaginationStateKey() {
-      return `${this.filter}-${this.repositories.length}`;
+      return `${this.filter}-${this.repositories.length}-${this.pageInfo.page}`;
     },
 
     availableNamespaces() {

app/assets/javascripts/import_entities/import_projects/store/actions.js

@@ -53,7 +53,6 @@ const importAll = ({ state, dispatch }) => {
 
 const fetchReposFactory = ({ reposPath = isRequired() }) => ({ state, commit }) => {
   const nextPage = state.pageInfo.page + 1;
-  commit(types.SET_PAGE, nextPage);
   commit(types.REQUEST_REPOS);
 
   const { provider, filter } = state;
@@ -67,11 +66,10 @@ const fetchReposFactory = ({ reposPath = isRequired() }) => ({ state, commit })
       }),
     )
     .then(({ data }) => {
+      commit(types.SET_PAGE, nextPage);
       commit(types.RECEIVE_REPOS_SUCCESS, convertObjectPropsToCamelCase(data, { deep: true }));
     })
     .catch((e) => {
-      commit(types.SET_PAGE, nextPage - 1);
-
       if (hasRedirectInError(e)) {
         redirectToUrlInError(e);
       } else if (tooManyRequests(e)) {

app/assets/javascripts/import_entities/import_projects/store/mutations.js

@@ -9,7 +9,7 @@ const makeNewImportedProject = (importedProject) => ({
     sanitizedName: importedProject.name,
     providerLink: importedProject.providerLink,
   },
-  importedProject,
+  importedProject: { ...importedProject },
 });
 
 const makeNewIncompatibleProject = (project) => ({
@@ -63,15 +63,16 @@ export default {
         factory: makeNewIncompatibleProject,
       });
 
-      state.repositories = [
-        ...newImportedProjects,
-        ...state.repositories,
-        ...repositories.providerRepos.map((project) => ({
+      const existingProjects = [...newImportedProjects, ...state.repositories];
+      const existingProjectNames = new Set(existingProjects.map((p) => p.importSource.fullName));
+      const newProjects = repositories.providerRepos
+        .filter((project) => !existingProjectNames.has(project.fullName))
+        .map((project) => ({
           importSource: project,
           importedProject: null,
-        })),
-        ...newIncompatibleProjects,
-      ];
+        }));
+
+      state.repositories = [...existingProjects, ...newProjects, ...newIncompatibleProjects];
 
       if (incompatibleRepos.length === 0 && repositories.providerRepos.length === 0) {
         state.pageInfo.page -= 1;

app/controllers/import/bitbucket_controller.rb

@@ -62,14 +62,10 @@ class Import::BitbucketController < Import::BaseController
 
   protected
 
-  # rubocop: disable CodeReuse/ActiveRecord
   override :importable_repos
   def importable_repos
-    already_added_projects_names = already_added_projects.map(&:import_source)
-
-    bitbucket_repos.reject { |repo| already_added_projects_names.include?(repo.full_name) || !repo.valid? }
+    bitbucket_repos.filter { |repo| repo.valid? }
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   override :incompatible_repos
   def incompatible_repos

app/controllers/import/bitbucket_server_controller.rb

@@ -62,16 +62,10 @@ class Import::BitbucketServerController < Import::BaseController
 
   protected
 
-  # rubocop: disable CodeReuse/ActiveRecord
   override :importable_repos
   def importable_repos
-    # Use the import URL to filter beyond what BaseService#find_already_added_projects
-    already_added_projects = filter_added_projects('bitbucket_server', bitbucket_repos.map(&:browse_url))
-    already_added_projects_names = already_added_projects.map(&:import_source)
-
-    bitbucket_repos.reject { |repo| already_added_projects_names.include?(repo.browse_url) || !repo.valid? }
+    bitbucket_repos.filter { |repo| repo.valid? }
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   override :incompatible_repos
   def incompatible_repos
@@ -90,12 +84,6 @@ class Import::BitbucketServerController < Import::BaseController
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def filter_added_projects(import_type, import_sources)
-    current_user.created_projects.where(import_type: import_type, import_source: import_sources).with_import_state
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
   def client
     @client ||= BitbucketServer::Client.new(credentials)
   end

app/controllers/import/fogbugz_controller.rb

@@ -74,16 +74,10 @@ class Import::FogbugzController < Import::BaseController
 
   protected
 
-  # rubocop: disable CodeReuse/ActiveRecord
   override :importable_repos
   def importable_repos
-    repos = client.repos
-
-    already_added_projects_names = already_added_projects.map(&:import_source)
-
-    repos.reject { |repo| already_added_projects_names.include? repo.name }
+    client.repos
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   override :incompatible_repos
   def incompatible_repos

app/controllers/import/github_controller.rb

@@ -64,9 +64,7 @@ class Import::GithubController < Import::BaseController
   # rubocop: disable CodeReuse/ActiveRecord
   override :importable_repos
   def importable_repos
-    already_added_projects_names = already_added_projects.pluck(:import_source)
-
-    client_repos.reject { |repo| already_added_projects_names.include?(repo.full_name) }
+    client_repos.to_a
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

app/controllers/import/gitlab_controller.rb

@@ -39,16 +39,10 @@ class Import::GitlabController < Import::BaseController
 
   protected
 
-  # rubocop: disable CodeReuse/ActiveRecord
   override :importable_repos
   def importable_repos
-    repos = client.projects(starting_page: 1, page_limit: MAX_PROJECT_PAGES, per_page: PER_PAGE_PROJECTS)
-
-    already_added_projects_names = already_added_projects.map(&:import_source)
-
-    repos.reject { |repo| already_added_projects_names.include? repo["path_with_namespace"] }
+    client.projects(starting_page: 1, page_limit: MAX_PROJECT_PAGES, per_page: PER_PAGE_PROJECTS)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   override :incompatible_repos
   def incompatible_repos

app/controllers/import/manifest_controller.rb

@@ -41,7 +41,7 @@ class Import::ManifestController < Import::BaseController
   end
 
   def create
-    repository = repositories.find do |project|
+    repository = importable_repos.find do |project|
       project[:id] == params[:repo_id].to_i
     end
 
@@ -56,14 +56,10 @@ class Import::ManifestController < Import::BaseController
 
   protected
 
-  # rubocop: disable CodeReuse/ActiveRecord
   override :importable_repos
   def importable_repos
-    already_added_projects_names = already_added_projects.pluck(:import_url)
-
-    repositories.reject { |repo| already_added_projects_names.include?(repo[:url]) }
+    @importable_repos ||= manifest_import_metadata.repositories
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   override :incompatible_repos
   def incompatible_repos
@@ -88,7 +84,7 @@ class Import::ManifestController < Import::BaseController
   private
 
   def ensure_import_vars
-    unless group && repositories.present?
+    unless group && importable_repos.present?
       redirect_to(new_import_manifest_path)
     end
   end
@@ -103,10 +99,6 @@ class Import::ManifestController < Import::BaseController
     @manifest_import_status ||= Gitlab::ManifestImport::Metadata.new(current_user, fallback: session)
   end
 
-  def repositories
-    @repositories ||= manifest_import_metadata.repositories
-  end
-
   def find_jobs
     find_already_added_projects.to_json(only: [:id], methods: [:import_status])
   end

spec/controllers/import/manifest_controller_spec.rb

@@ -75,16 +75,6 @@ RSpec.describe Import::ManifestController, :clean_gitlab_redis_shared_state do
         expect(json_response.dig("provider_repos", 0, "id")).to eq(repo1[:id])
         expect(json_response.dig("provider_repos", 1, "id")).to eq(repo2[:id])
       end
-
-      it "does not show already added project" do
-        project = create(:project, import_type: 'manifest', namespace: user.namespace, import_status: :finished, import_url: repo1[:url])
-
-        get :status, format: :json
-
-        expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
-        expect(json_response.dig("provider_repos").length).to eq(1)
-        expect(json_response.dig("provider_repos", 0, "id")).not_to eq(repo1[:id])
-      end
     end
 
     context 'when the data is stored via Gitlab::ManifestImport::Metadata' do

spec/frontend/import_entities/import_projects/store/actions_spec.js

@@ -85,7 +85,7 @@ describe('import_projects store actions', () => {
 
     afterEach(() => mock.restore());
 
-    it('commits SET_PAGE, REQUEST_REPOS, RECEIVE_REPOS_SUCCESS mutations on a successful request', () => {
+    it('commits REQUEST_REPOS, SET_PAGE, RECEIVE_REPOS_SUCCESS mutations on a successful request', () => {
       mock.onGet(MOCK_ENDPOINT).reply(200, payload);
 
       return testAction(
@@ -93,8 +93,8 @@ describe('import_projects store actions', () => {
         null,
         localState,
         [
-          { type: SET_PAGE, payload: 1 },
           { type: REQUEST_REPOS },
+          { type: SET_PAGE, payload: 1 },
           {
             type: RECEIVE_REPOS_SUCCESS,
             payload: convertObjectPropsToCamelCase(payload, { deep: true }),
@@ -104,19 +104,14 @@ describe('import_projects store actions', () => {
       );
     });
 
-    it('commits SET_PAGE, REQUEST_REPOS, RECEIVE_REPOS_ERROR and SET_PAGE again mutations on an unsuccessful request', () => {
+    it('commits REQUEST_REPOS, RECEIVE_REPOS_ERROR mutations on an unsuccessful request', () => {
       mock.onGet(MOCK_ENDPOINT).reply(500);
 
       return testAction(
         fetchRepos,
         null,
         localState,
-        [
-          { type: SET_PAGE, payload: 1 },
-          { type: REQUEST_REPOS },
-          { type: SET_PAGE, payload: 0 },
-          { type: RECEIVE_REPOS_ERROR },
-        ],
+        [{ type: REQUEST_REPOS }, { type: RECEIVE_REPOS_ERROR }],
         [],
       );
     });
@@ -135,7 +130,7 @@ describe('import_projects store actions', () => {
       expect(requestedUrl).toBe(`${MOCK_ENDPOINT}?page=${localStateWithPage.pageInfo.page + 1}`);
     });
 
-    it('correctly updates current page on an unsuccessful request', () => {
+    it('correctly keeps current page on an unsuccessful request', () => {
       mock.onGet(MOCK_ENDPOINT).reply(500);
       const CURRENT_PAGE = 5;
 
@@ -143,10 +138,7 @@ describe('import_projects store actions', () => {
         fetchRepos,
         null,
         { ...localState, pageInfo: { page: CURRENT_PAGE } },
-        expect.arrayContaining([
-          { type: SET_PAGE, payload: CURRENT_PAGE + 1 },
-          { type: SET_PAGE, payload: CURRENT_PAGE },
-        ]),
+        expect.arrayContaining([]),
         [],
       );
     });
@@ -159,12 +151,7 @@ describe('import_projects store actions', () => {
           fetchRepos,
           null,
           { ...localState, filter: 'filter' },
-          [
-            { type: SET_PAGE, payload: 1 },
-            { type: REQUEST_REPOS },
-            { type: SET_PAGE, payload: 0 },
-            { type: RECEIVE_REPOS_ERROR },
-          ],
+          [{ type: REQUEST_REPOS }, { type: RECEIVE_REPOS_ERROR }],
           [],
         );
 
@@ -183,8 +170,8 @@ describe('import_projects store actions', () => {
           null,
           { ...localState, filter: 'filter' },
           [
-            { type: SET_PAGE, payload: 1 },
             { type: REQUEST_REPOS },
+            { type: SET_PAGE, payload: 1 },
             {
               type: RECEIVE_REPOS_SUCCESS,
               payload: convertObjectPropsToCamelCase(payload, { deep: true }),

spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb

@@ -82,16 +82,6 @@ RSpec.shared_examples 'a GitHub-ish import controller: GET status' do
     expect(json_response.dig("provider_repos", 1, "id")).to eq(org_repo.id)
   end
 
-  it "does not show already added project" do
-    project = create(:project, import_type: provider, namespace: user.namespace, import_status: :finished, import_source: 'asd/vim')
-    stub_client(repos: [repo], orgs: [], each_page: [OpenStruct.new(objects: [repo])].to_enum)
-
-    get :status, format: :json
-
-    expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
-    expect(json_response.dig("provider_repos")).to eq([])
-  end
-
   it "touches the etag cache store" do
     stub_client(repos: [], orgs: [], each_page: [])
 

spec/support/shared_examples/controllers/import_controller_status_shared_examples.rb

@@ -19,14 +19,4 @@ RSpec.shared_examples 'import controller status' do
     expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
     expect(json_response.dig("provider_repos", 0, "id")).to eq(repo_id)
   end
-
-  it "does not show already added project" do
-    project = create(:project, import_type: provider_name, namespace: user.namespace, import_status: :finished, import_source: import_source)
-    stub_client(client_repos_field => [repo])
-
-    get :status, format: :json
-
-    expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
-    expect(json_response.dig("provider_repos")).to eq([])
-  end
 end