Improve manifest feature after backend review

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

12 files changed, 57 insertions, 53 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 07089ada51d..f45fcd4d900 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -358,7 +358,7 @@ class ApplicationController < ActionController::Base
   end
 
   def manifest_import_enabled?
-    Gitlab::CurrentSettings.import_sources.include?('manifest')
+    Group.supports_nested_groups? && Gitlab::CurrentSettings.import_sources.include?('manifest')
   end
 
   # U2F (universal 2nd factor) devices need a unique identifier for the application
diff --git a/app/controllers/import/manifest_controller.rb b/app/controllers/import/manifest_controller.rb
index 42e661325cf..e5a719fa0df 100644
--- a/app/controllers/import/manifest_controller.rb
+++ b/app/controllers/import/manifest_controller.rb
@@ -27,8 +27,8 @@ class Import::ManifestController < Import::BaseController
     manifest = Gitlab::ManifestImport::Manifest.new(params[:manifest].tempfile)
 
     if manifest.valid?
-      session[:repositories] = manifest.projects
-      session[:group_id] = group.id
+      session[:manifest_import_repositories] = manifest.projects
+      session[:manifest_import_group_id] = group.id
 
       redirect_to status_import_manifest_path
     else
@@ -65,11 +65,11 @@ class Import::ManifestController < Import::BaseController
   end
 
   def group
-    @group ||= Group.find_by(id: session[:group_id])
+    @group ||= Group.find_by(id: session[:manifest_import_group_id])
   end
 
   def repositories
-    @repositories ||= session[:repositories]
+    @repositories ||= session[:manifest_import_repositories]
   end
 
   def find_jobs
diff --git a/app/views/import/_githubish_status.html.haml b/app/views/import/_githubish_status.html.haml
index 5e7be5cd37b..0f641352e99 100644
--- a/app/views/import/_githubish_status.html.haml
+++ b/app/views/import/_githubish_status.html.haml
@@ -21,23 +21,13 @@
         %th= _('Status')
     %tbody
       - @already_added_projects.each do |project|
-        %tr{ id: "project_#{project.id}", class: "#{project_status_css_class(project.import_status)}" }
+        %tr{ id: "project_#{project.id}", class: project_status_css_class(project.import_status) }
           %td
             = provider_project_link(provider, project.import_source)
           %td
             = link_to project.full_path, [project.namespace.becomes(Namespace), project]
           %td.job-status
-            - if project.import_status == 'finished'
-              %span
-                %i.fa.fa-check
-                = _('Done')
-            - elsif project.import_status == 'started'
-              %i.fa.fa-spinner.fa-spin
-              = _('Started')
-            - elsif project.import_status == 'failed'
-              = _('Failed')
-            - else
-              = project.human_import_status_name
+            = render 'import/project_status', project: project
 
       - @repos.each do |repo|
         %tr{ id: "repo_#{repo.id}", data: { qa: { repo_path: repo.full_name } } }
@@ -61,6 +51,6 @@
               = has_ci_cd_only_params? ? _('Connect') : _('Import')
               = icon("spinner spin", class: "loading-icon")
 
-.js-importer-status{ data: { jobs_import_path: "#{url_for([:jobs, :import, provider])}",
-                             import_path: "#{url_for([:import, provider])}",
-                             ci_cd_only: "#{has_ci_cd_only_params?}" } }
+.js-importer-status{ data: { jobs_import_path: url_for([:jobs, :import, provider]),
+                             import_path: url_for([:import, provider]),
+                             ci_cd_only: has_ci_cd_only_params? } }
diff --git a/app/views/import/_project_status.html.haml b/app/views/import/_project_status.html.haml
new file mode 100644
index 00000000000..280bcbc1e63
--- /dev/null
+++ b/app/views/import/_project_status.html.haml
@@ -0,0 +1,11 @@
+- case project.import_status
+- when 'finished'
+  = icon('check')
+  = _('Done')
+- when 'started'
+  = icon("spinner spin")
+  = _('Started')
+- when 'failed'
+  = _('Failed')
+- else
+  = project.human_import_status_name
diff --git a/app/views/import/manifest/_form.html.haml b/app/views/import/manifest/_form.html.haml
index 6c938255e65..763beb5958f 100644
--- a/app/views/import/manifest/_form.html.haml
+++ b/app/views/import/manifest/_form.html.haml
@@ -1,12 +1,5 @@
 = form_tag upload_import_manifest_path, multipart: true do
   .form-group
-    = label_tag :manifest, class: 'label-light' do
-      = _('Manifest')
-    = file_field_tag :manifest, class: 'form-control-file', required: true
-    .form-text.text-muted
-      = _('Import multiple repositories by uploading a manifest file.')
-
-  .form-group
     = label_tag :group_id, nil, class: 'label-light' do
       = _('Group')
     .input-group
@@ -17,6 +10,14 @@
     .form-text.text-muted
       = _('Choose the top-level group for your repository imports.')
 
+  .form-group
+    = label_tag :manifest, class: 'label-light' do
+      = _('Manifest')
+    = file_field_tag :manifest, class: 'form-control-file', required: true
+    .form-text.text-muted
+      = _('Import multiple repositories by uploading a manifest file.')
+      = link_to icon('question-circle'), help_page_path('user/project/import/manifest')
+
   .append-bottom-10
     = submit_tag _('List available repositories'), class: 'btn btn-success'
     = link_to _('Cancel'), new_project_path, class: 'btn btn-cancel'
diff --git a/app/views/import/manifest/status.html.haml b/app/views/import/manifest/status.html.haml
index c8065d90103..5b2e1005398 100644
--- a/app/views/import/manifest/status.html.haml
+++ b/app/views/import/manifest/status.html.haml
@@ -19,23 +19,13 @@
         %th= _('Status')
     %tbody
       - @already_added_projects.each do |project|
-        %tr{ id: "project_#{project.id}", class: "#{project_status_css_class(project.import_status)}" }
+        %tr{ id: "project_#{project.id}", class: project_status_css_class(project.import_status) }
           %td
             = project.import_url
           %td
             = link_to_project project
           %td.job-status
-            - if project.import_status == 'finished'
-              %span
-                = icon('check')
-                = _('Done')
-            - elsif project.import_status == 'started'
-              = icon("spinner spin")
-              = _('Started')
-            - elsif project.import_status == 'failed'
-              = _('Failed')
-            - else
-              = project.human_import_status_name
+            = render 'import/project_status', project: project
 
       - @pending_repositories.each do |repository|
         %tr{ id: "repo_#{repository[:id]}" }
@@ -48,6 +38,5 @@
               = _('Import')
               = icon("spinner spin", class: "loading-icon")
 
-.js-importer-status{ data: { jobs_import_path: "#{url_for([:jobs, :import, provider])}",
-                             import_path: "#{url_for([:import, provider])}",
-                             ci_cd_only: "#{has_ci_cd_only_params?}" } }
+.js-importer-status{ data: { jobs_import_path: url_for([:jobs, :import, provider]),
+                             import_path: url_for([:import, provider]) } }
diff --git a/doc/user/project/import/img/manifest_upload.png b/doc/user/project/import/img/manifest_upload.png
index 1925c3349ed..d6bf4b157dd 100644
--- a/doc/user/project/import/img/manifest_upload.png
+++ b/doc/user/project/import/img/manifest_upload.png
diff --git a/doc/user/project/import/manifest.md b/doc/user/project/import/manifest.md
index 5ec64225098..2233a310bc7 100644
--- a/doc/user/project/import/manifest.md
+++ b/doc/user/project/import/manifest.md
@@ -1,7 +1,11 @@
 # Import multiple repositories by uploading a manifest file
 
-GitLab allows you to import all the required git repositories based on the 
-Android repository manifest file.
+GitLab allows you to import all the required git repositories 
+based a manifest file like the one used by the Android repository.
+
+
+>**Note:** 
+This feature requires [subgroups](user/group/subgroups/index.md) to be supported by your database. 
 
 You can do it by following next steps: 
 
diff --git a/lib/gitlab/manifest_import/manifest.rb b/lib/gitlab/manifest_import/manifest.rb
index 7b1e6a22c3a..4d6034fb956 100644
--- a/lib/gitlab/manifest_import/manifest.rb
+++ b/lib/gitlab/manifest_import/manifest.rb
@@ -1,4 +1,4 @@
-# Class to parse manifest file to import multiple projects at once
+# Class to parse manifest file and build a list of repositories for import
 #
 # <manifest>
 #   <remote review="https://android-review.googlesource.com/" />
@@ -11,17 +11,16 @@
 #    For example, you can't have projects with 'foo' and 'foo/bar' paths.
 # 2. Remote must be present with review attribute so GitLab knows
 #    where to fetch source code
-# 3. For each nested keyword in path a corresponding group will be created.
-#    For example if a path is 'foo/bar' then GitLab will create a group 'foo'
-#    and a project 'bar' in it.
 module Gitlab
   module ManifestImport
     class Manifest
       attr_reader :parsed_xml, :errors
 
       def initialize(file)
-        @parsed_xml = File.open(file) { |f| Nokogiri::XML(f) }
+        @parsed_xml = Nokogiri::XML(file) { |config| config.strict }
         @errors = []
+      rescue Nokogiri::XML::SyntaxError
+        @errors = ['The uploaded file is not a valid XML file.']
       end
 
       def projects
@@ -36,6 +35,8 @@ module Gitlab
       end
 
       def valid?
+        return false if @errors.any?
+
         unless validate_remote
           @errors << 'Make sure a <remote> tag is present and is valid.'
         end
diff --git a/lib/gitlab/manifest_import/project_creator.rb b/lib/gitlab/manifest_import/project_creator.rb
index 9ccd32c3a3b..b5967c93735 100644
--- a/lib/gitlab/manifest_import/project_creator.rb
+++ b/lib/gitlab/manifest_import/project_creator.rb
@@ -12,8 +12,7 @@ module Gitlab
       def execute
         group_full_path, _, project_path = repository[:path].rpartition('/')
         group_full_path = File.join(destination.full_path, group_full_path) if destination
-        group = Group.find_by_full_path(group_full_path) ||
-          create_group_with_parents(group_full_path)
+        group = create_group_with_parents(group_full_path)
 
         params = {
           import_url: repository[:url],
diff --git a/spec/features/import/manifest_import_spec.rb b/spec/features/import/manifest_import_spec.rb
index 06d0f2b1857..e381d073804 100644
--- a/spec/features/import/manifest_import_spec.rb
+++ b/spec/features/import/manifest_import_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-feature 'Import multiple repositories by uploading a manifest file', :js, :postgresql do
+describe 'Import multiple repositories by uploading a manifest file', :js, :postgresql do
   include Select2Helper
 
   let(:user) { create(:admin) }
@@ -36,6 +36,15 @@ feature 'Import multiple repositories by uploading a manifest file', :js, :postg
     end
   end
 
+  it 'renders an error if invalid file was provided' do
+    visit new_import_manifest_path
+
+    attach_file('manifest', Rails.root.join('spec/fixtures/banana_sample.gif'))
+    click_on 'List available repositories'
+
+    expect(page).to have_content 'The uploaded file is not a valid XML file.'
+  end
+
   def first_row
     page.all('table.import-jobs tbody tr')[0]
   end
diff --git a/spec/lib/gitlab/manifest_import/manifest_spec.rb b/spec/lib/gitlab/manifest_import/manifest_spec.rb
index a0b7c3f65db..ab305fb2316 100644
--- a/spec/lib/gitlab/manifest_import/manifest_spec.rb
+++ b/spec/lib/gitlab/manifest_import/manifest_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Gitlab::ManifestImport::Manifest, :postgresql do
-  let(:file) { Rails.root.join('spec/fixtures/aosp_manifest.xml') }
+  let(:file) { File.open(Rails.root.join('spec/fixtures/aosp_manifest.xml')) }
   let(:manifest) { described_class.new(file) }
 
   describe '#valid?' do