diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2018-07-09 14:59:07 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2018-07-11 12:22:57 +0300 |
commit | 6743147b7d9f310fbf5afa520e19ae495fd4df33 (patch) | |
tree | 2ac26c5fd33ea635462d73b8925915dff8486694 | |
parent | e02efff63d2b065f95e4eeac28ba994d245e4505 (diff) | |
download | gitlab-ce-6743147b7d9f310fbf5afa520e19ae495fd4df33.tar.gz |
Improve manifest feature after backend review
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
-rw-r--r-- | app/controllers/application_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/import/manifest_controller.rb | 8 | ||||
-rw-r--r-- | app/views/import/_githubish_status.html.haml | 20 | ||||
-rw-r--r-- | app/views/import/_project_status.html.haml | 11 | ||||
-rw-r--r-- | app/views/import/manifest/_form.html.haml | 15 | ||||
-rw-r--r-- | app/views/import/manifest/status.html.haml | 19 | ||||
-rw-r--r-- | doc/user/project/import/img/manifest_upload.png | bin | 11940 -> 12079 bytes | |||
-rw-r--r-- | doc/user/project/import/manifest.md | 8 | ||||
-rw-r--r-- | lib/gitlab/manifest_import/manifest.rb | 11 | ||||
-rw-r--r-- | lib/gitlab/manifest_import/project_creator.rb | 3 | ||||
-rw-r--r-- | spec/features/import/manifest_import_spec.rb | 11 | ||||
-rw-r--r-- | spec/lib/gitlab/manifest_import/manifest_spec.rb | 2 |
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 Binary files differindex 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 |