Address reviewer comments

- make tests more precise
- remove owner_name from serializing

app/controllers/import/bitbucket_controller.rb

@@ -24,9 +24,7 @@ class Import::BitbucketController < Import::BaseController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def status
-    if Feature.enabled?(:new_import_ui)
-      return super
-    end
+    return super if Feature.enabled?(:new_import_ui)
 
     bitbucket_client = Bitbucket::Client.new(credentials)
     repos = bitbucket_client.repos(filter: sanitized_filter_param)

app/controllers/import/bitbucket_server_controller.rb

@@ -9,6 +9,8 @@ class Import::BitbucketServerController < Import::BaseController
   before_action :bitbucket_auth, except: [:new, :configure]
   before_action :validate_import_params, only: [:create]
 
+  rescue_from BitbucketServer::Connection::ConnectionError, with: :bitbucket_connection_error
+
   # As a basic sanity check to prevent URL injection, restrict project
   # repository input and repository slugs to allowed characters. For Bitbucket:
   #
@@ -61,14 +63,7 @@ class Import::BitbucketServerController < Import::BaseController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def status
-    if Feature.enabled?(:new_import_ui)
-      begin
-        return super
-      rescue BitbucketServer::Connection::ConnectionError => error
-        render json: { error: _("Unable to connect to server: %{error}") % { error: error } }, status: :unprocessable_entity
-        return
-      end
-    end
+    return super if Feature.enabled?(:new_import_ui)
 
     @collection = client.repos(page_offset: page_offset, limit: limit_per_page, filter: sanitized_filter_param)
     @repos, @incompatible_repos = @collection.partition { |repo| repo.valid? }
@@ -78,10 +73,6 @@ class Import::BitbucketServerController < Import::BaseController
     already_added_projects_names = @already_added_projects.pluck(:import_source)
 
     @repos.reject! { |repo| already_added_projects_names.include?(repo.browse_url) }
-  rescue BitbucketServer::Connection::ConnectionError => error
-    flash[:alert] = _("Unable to connect to server: %{error}") % { error: error }
-    clear_session_data
-    redirect_to new_import_bitbucket_server_path
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -200,4 +191,17 @@ class Import::BitbucketServerController < Import::BaseController
   def sanitized_filter_param
     sanitize(params[:filter])
   end
+
+  def bitbucket_connection_error(error)
+    respond_to do |format|
+      format.json do
+        render json: { error: _("Unable to connect to server: %{error}") % { error: error } }, status: :unprocessable_entity
+      end
+      format.html do
+        flash[:alert] = _("Unable to connect to server: %{error}") % { error: error }
+        clear_session_data
+        redirect_to new_import_bitbucket_server_path
+      end
+    end
+  end
 end

app/controllers/import/fogbugz_controller.rb

@@ -50,9 +50,7 @@ class Import::FogbugzController < Import::BaseController
       return redirect_to new_import_fogbugz_path
     end
 
-    if Feature.enabled?(:new_import_ui)
-      return super
-    end
+    return super if Feature.enabled?(:new_import_ui)
 
     @repos = client.repos
 

app/serializers/import/base_provider_repo_entity.rb

@@ -3,7 +3,6 @@
 class Import::BaseProviderRepoEntity < Grape::Entity
   expose :id
   expose :full_name
-  expose :owner_name
   expose :sanitized_name
   expose :provider_link
 end

app/serializers/import/bitbucket_provider_repo_entity.rb

@@ -7,10 +7,6 @@ class Import::BitbucketProviderRepoEntity < Import::BaseProviderRepoEntity
     repo.full_name
   end
 
-  expose :owner_name, override: true do |repo|
-    repo.owner
-  end
-
   expose :sanitized_name, override: true do |repo|
     repo.name.gsub(/[^\s\w.-]/, '')
   end

app/serializers/import/bitbucket_server_provider_repo_entity.rb

 # frozen_string_literal: true
 
 class Import::BitbucketServerProviderRepoEntity < Import::BitbucketProviderRepoEntity
-  expose :owner_name, override: true do
-    ''
-  end
-
   expose :provider_link, override: true do |repo, options|
     repo.browse_url
   end

app/serializers/import/fogbugz_provider_repo_entity.rb

@@ -7,10 +7,6 @@ class Import::FogbugzProviderRepoEntity < Import::BaseProviderRepoEntity
     repo.name
   end
 
-  expose :owner_name, override: true do
-    ''
-  end
-
   expose :sanitized_name, override: true do |repo|
     repo.safe_name
   end

app/serializers/import/githubish_provider_repo_entity.rb

@@ -3,10 +3,6 @@
 class Import::GithubishProviderRepoEntity < Import::BaseProviderRepoEntity
   include ImportHelper
 
-  expose :owner_name, override: true do |provider_repo, options|
-    owner_name(provider_repo, options[:provider])
-  end
-
   expose :sanitized_name, override: true do |provider_repo|
     sanitize_project_name(provider_repo[:name])
   end

spec/controllers/import/bitbucket_controller_spec.rb

@@ -56,7 +56,7 @@ RSpec.describe Import::BitbucketController do
 
   describe "GET status" do
     before do
-      @repo = double(name: 'vim', slug: 'vim', owner: 'asd', full_name: 'asd/vim', clone_url: "http://test.host/demo/url.git", "valid?" => true)
+      @repo = double(name: 'vim', slug: 'vim', owner: 'asd', full_name: 'asd/vim', clone_url: 'http://test.host/demo/url.git', 'valid?' => true)
       assign_session_tokens
       stub_feature_flags(new_import_ui: false)
     end

spec/serializers/import/bitbucket_provider_repo_entity_spec.rb

@@ -5,13 +5,13 @@ require 'spec_helper'
 describe Import::BitbucketProviderRepoEntity do
   let(:repo_data) do
     {
-      "name" => "repo_name",
-      "full_name" => "owner/repo_name",
-      "links" => {
-        "clone" => [
+      'name' => 'repo_name',
+      'full_name' => 'owner/repo_name',
+      'links' => {
+        'clone' => [
           {
-            "href" => "https://bitbucket.org/owner/repo_name",
-            "name" => "https"
+            'href' => 'https://bitbucket.org/owner/repo_name',
+            'name' => 'https'
           }
         ]
       }
@@ -21,5 +21,14 @@ describe Import::BitbucketProviderRepoEntity do
 
   subject { described_class.new(repo).as_json }
 
-  it_behaves_like 'exposes required fields for import entity'
+  it_behaves_like 'exposes required fields for import entity' do
+    let(:expected_values) do
+      {
+        id: 'owner/repo_name',
+        full_name: 'owner/repo_name',
+        sanitized_name: 'repo_name',
+        provider_link: 'https://bitbucket.org/owner/repo_name'
+      }
+    end
+  end
 end

spec/serializers/import/bitbucket_server_provider_repo_entity_spec.rb

@@ -5,15 +5,15 @@ require 'spec_helper'
 describe Import::BitbucketServerProviderRepoEntity do
   let(:repo_data) do
     {
-      "name" => "demo",
-      "project" => {
-        "name" => "demo"
+      'name' => 'test',
+      'project' => {
+        'name' => 'demo'
       },
-      "links" => {
-        "self" => [
+      'links' => {
+        'self' => [
           {
-            "href" => "http://local.bitbucket.server/demo/demo.git",
-            "name" => "http"
+            'href' => 'http://local.bitbucket.server/demo/test.git',
+            'name' => 'http'
           }
         ]
       }
@@ -23,5 +23,14 @@ describe Import::BitbucketServerProviderRepoEntity do
 
   subject { described_class.new(repo).as_json }
 
-  it_behaves_like 'exposes required fields for import entity'
+  it_behaves_like 'exposes required fields for import entity' do
+    let(:expected_values) do
+      {
+        id: 'demo/test',
+        full_name: 'demo/test',
+        sanitized_name: 'test',
+        provider_link: 'http://local.bitbucket.server/demo/test.git'
+      }
+    end
+  end
 end

spec/serializers/import/fogbugz_provider_repo_entity_spec.rb

@@ -3,15 +3,25 @@
 require 'spec_helper'
 
 describe Import::FogbugzProviderRepoEntity do
+  let(:provider_url) { 'https://demo.fogbugz.com/' }
   let(:repo_data) do
     {
-      "ixProject" => "foo",
-      "sProject" => "demo"
+      'ixProject' => 'foo',
+      'sProject' => 'demo'
     }
   end
   let(:repo) { Gitlab::FogbugzImport::Repository.new(repo_data) }
 
-  subject { described_class.new(repo).as_json }
+  subject { described_class.represent(repo, { provider_url: provider_url }).as_json }
 
-  it_behaves_like 'exposes required fields for import entity'
+  it_behaves_like 'exposes required fields for import entity' do
+    let(:expected_values) do
+      {
+        id: 'foo',
+        full_name: 'demo',
+        sanitized_name: 'demo',
+        provider_link: 'https://demo.fogbugz.com/demo'
+      }
+    end
+  end
 end

spec/serializers/import/githubish_provider_repo_entity_spec.rb

@@ -3,16 +3,25 @@
 require 'spec_helper'
 
 describe Import::GithubishProviderRepoEntity do
+  let(:provider_url) { 'https://github.com/' }
   let(:repo) do
     {
       id: 1,
       full_name: 'full/name',
-      name: 'name',
-      owner: { login: 'owner' }
+      name: 'name'
     }
   end
 
-  subject { described_class.new(repo).as_json }
+  subject { described_class.represent(repo, { provider_url: provider_url }).as_json }
 
-  it_behaves_like 'exposes required fields for import entity'
+  it_behaves_like 'exposes required fields for import entity' do
+    let(:expected_values) do
+      {
+        id: 1,
+        full_name: 'full/name',
+        sanitized_name: 'name',
+        provider_link: 'https://github.com/full/name'
+      }
+    end
+  end
 end

spec/serializers/import/provider_repo_serializer_spec.rb

 # frozen_string_literal: true
 
 require 'spec_helper'
+
 describe Import::ProviderRepoSerializer do
   using RSpec::Parameterized::TableSyntax
 

spec/support/shared_examples/serializers/import/import_entity_shared_examples.rb

@@ -2,24 +2,20 @@
 
 RSpec.shared_examples 'exposes required fields for import entity' do
   describe 'exposes required fields' do
-    it 'exposes id' do
-      expect(subject).to include(:id)
+    it 'correctly exposes id' do
+      expect(subject[:id]).to eql(expected_values[:id])
     end
 
-    it 'exposes full name' do
-      expect(subject).to include(:full_name)
+    it 'correctly exposes full name' do
+      expect(subject[:full_name]).to eql(expected_values[:full_name])
     end
 
-    it 'exposes owner name' do
-      expect(subject).to include(:owner_name)
+    it 'correctly exposes sanitized name' do
+      expect(subject[:sanitized_name]).to eql(expected_values[:sanitized_name])
     end
 
-    it 'exposes sanitized name' do
-      expect(subject).to include(:sanitized_name)
-    end
-
-    it 'exposes provider link' do
-      expect(subject).to include(:provider_link)
+    it 'correctly exposes provider link' do
+      expect(subject[:provider_link]).to eql(expected_values[:provider_link])
     end
   end
 end