From 0e69b25854a0f4e09ba336a28deab80551b576b6 Mon Sep 17 00:00:00 2001
From: Peter Hegman <phegman@gitlab.com>
Date: Tue, 28 Jun 2022 22:28:17 +0000
Subject: [PATCH] Refactor import group name and path field to Vue
To improve UX and remove HAML/jQuery
---
.../_import_group_from_file_panel.html.haml | 63 +++++-----
.../_group_name_and_path_fields.html.haml | 2 +-
.../groups/import_export/import_file_spec.rb | 112 ++++++++++++++++--
3 files changed, 136 insertions(+), 41 deletions(-)
diff --git a/app/views/groups/_import_group_from_file_panel.html.haml b/app/views/groups/_import_group_from_file_panel.html.haml
index 04170c30a2074..2e524e6567954 100644
--- a/app/views/groups/_import_group_from_file_panel.html.haml
+++ b/app/views/groups/_import_group_from_file_panel.html.haml
@@ -2,46 +2,49 @@
- group_path = root_url
- group_path << parent.full_path + '/' if parent
-= form_with url: import_gitlab_group_path, class: 'group-form gl-show-field-errors', multipart: true do |f|
+= form_for '', url: import_gitlab_group_path, namespace: ('import_group' if Feature::enabled?(:group_name_path_vue, current_user)), class: 'group-form gl-show-field-errors', multipart: true do |f|
.gl-border-l-solid.gl-border-r-solid.gl-border-gray-100.gl-border-1.gl-p-5
%h4
= _('Import group from file')
= render Pajamas::AlertComponent.new(variant: :warning,
+ alert_options: { class: 'gl-mb-5' },
dismissible: false) do |c|
= c.body do
- docs_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: help_page_path('user/group/import/index.md') }
- link_end = '</a>'.html_safe
= s_('GroupsNew|This feature is deprecated and replaced by %{docs_link_start}group migration%{docs_link_end}.').html_safe % { docs_link_start: docs_link_start, docs_link_end: link_end }
-
- .form-group.gl-display-flex.gl-flex-direction-column.gl-mt-5
- = f.label :name, _('New group name'), for: 'import_group_name'
- = f.text_field :name, placeholder: s_('GroupsNew|My Awesome Group'), class: 'js-autofill-group-name gl-form-input col-xs-12 col-sm-8',
- required: true,
- title: _('Please fill in a descriptive name for your group.'),
- autofocus: true,
- id: 'import_group_name'
-
- .form-group.gl-display-flex.gl-flex-direction-column
- = f.label :import_group_path, _('New group URL'), for: 'import_group_path'
- .input-group.gl-field-error-anchor.col-xs-12.col-sm-8.gl-p-0
- .group-root-path.input-group-prepend.has-tooltip{ title: group_path, :'data-placement' => 'bottom' }
- .input-group-text
- %span
- = root_url
- - if parent
- %strong= parent.full_path + '/'
- = f.hidden_field :parent_id, value: parent&.id
- = f.text_field :path, placeholder: 'my-awesome-group', class: 'form-control js-validate-group-path js-autofill-group-path',
- id: 'import_group_path',
+ - if Feature::enabled?(:group_name_path_vue, current_user)
+ = render 'shared/groups/group_name_and_path_fields', f: f
+ - else
+ .form-group.gl-display-flex.gl-flex-direction-column.gl-mt-5
+ = f.label :name, _('New group name'), for: 'import_group_name'
+ = f.text_field :name, placeholder: s_('GroupsNew|My Awesome Group'), class: 'js-autofill-group-name gl-form-input col-xs-12 col-sm-8',
required: true,
- pattern: Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX_JS,
- title: group_url_error_message,
- maxlength: ::Namespace::URL_MAX_LENGTH,
- "data-bind-in" => "#{'create_chat_team' if Gitlab.config.mattermost.enabled}"
- %p.validation-error.gl-field-error.field-validation.hide
- = _("Group path is already taken. We've suggested one that is available.")
- %p.validation-success.gl-field-success.field-validation.hide= _('Group path is available.')
- %p.validation-pending.gl-field-error-ignore.field-validation.hide= _('Checking group path availability...')
+ title: _('Please fill in a descriptive name for your group.'),
+ autofocus: true,
+ id: 'import_group_name'
+
+ .form-group.gl-display-flex.gl-flex-direction-column
+ = f.label :import_group_path, _('New group URL'), for: 'import_group_path'
+ .input-group.gl-field-error-anchor.col-xs-12.col-sm-8.gl-p-0
+ .group-root-path.input-group-prepend.has-tooltip{ title: group_path, :'data-placement' => 'bottom' }
+ .input-group-text
+ %span
+ = root_url
+ - if parent
+ %strong= parent.full_path + '/'
+ = f.hidden_field :parent_id, value: parent&.id
+ = f.text_field :path, placeholder: 'my-awesome-group', class: 'form-control js-validate-group-path js-autofill-group-path',
+ id: 'import_group_path',
+ required: true,
+ pattern: Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX_JS,
+ title: group_url_error_message,
+ maxlength: ::Namespace::URL_MAX_LENGTH,
+ "data-bind-in" => "#{'create_chat_team' if Gitlab.config.mattermost.enabled}"
+ %p.validation-error.gl-field-error.field-validation.hide
+ = _("Group path is already taken. We've suggested one that is available.")
+ %p.validation-success.gl-field-success.field-validation.hide= _('Group path is available.')
+ %p.validation-pending.gl-field-error-ignore.field-validation.hide= _('Checking group path availability...')
.form-group
= f.label :file, s_('GroupsNew|Upload file')
.gl-font-weight-normal
diff --git a/app/views/shared/groups/_group_name_and_path_fields.html.haml b/app/views/shared/groups/_group_name_and_path_fields.html.haml
index 709130a47d3b6..634b8448535d5 100644
--- a/app/views/shared/groups/_group_name_and_path_fields.html.haml
+++ b/app/views/shared/groups/_group_name_and_path_fields.html.haml
@@ -1,5 +1,5 @@
.js-group-name-and-path{ data: group_name_and_path_app_data(@group) }
= f.hidden_field :name, data: { js_name: 'name' }
= f.hidden_field :path, maxlength: ::Namespace::URL_MAX_LENGTH, pattern: Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX_JS, data: { js_name: 'path' }
- = f.hidden_field :parent_id, data: { js_name: 'parentId' }
+ = f.hidden_field :parent_id, value: @group.parent&.id, data: { js_name: 'parentId' }
= f.hidden_field :id, data: { js_name: 'groupId' }
diff --git a/spec/features/groups/import_export/import_file_spec.rb b/spec/features/groups/import_export/import_file_spec.rb
index 3d23451feef7d..4147b4982df19 100644
--- a/spec/features/groups/import_export/import_file_spec.rb
+++ b/spec/features/groups/import_export/import_file_spec.rb
@@ -20,6 +20,94 @@
FileUtils.rm_rf(import_path, secure: true)
end
+ context 'when `group_name_path_vue` feature flag is disabled' do
+ before do
+ stub_feature_flags(group_name_path_vue: false)
+ end
+
+ context 'when the user uploads a valid export file' do
+ let(:file) { File.join(Rails.root, 'spec', %w[fixtures group_export.tar.gz]) }
+
+ context 'when using the pre-filled path', :sidekiq_inline do
+ it 'successfully imports the group' do
+ group_name = 'Test Group Import'
+
+ visit new_group_path
+ click_link 'Import group'
+
+ fill_in :import_group_name, with: group_name
+
+ expect(page).to have_content 'Import group from file'
+ attach_file(file) do
+ find('.js-filepicker-button').click
+ end
+
+ expect { click_on 'Import' }.to change { Group.count }.by 1
+
+ group = Group.find_by(name: group_name)
+
+ expect(group).not_to be_nil
+ expect(group.description).to eq 'A voluptate non sequi temporibus quam at.'
+ expect(group.path).to eq 'test-group-import'
+ expect(group.import_state.status).to eq GroupImportState.state_machine.states[:finished].value
+ end
+ end
+
+ context 'when modifying the pre-filled path' do
+ it 'successfully imports the group' do
+ visit new_group_path
+ click_link 'Import group'
+
+ fill_in :import_group_name, with: 'Test Group Import'
+
+ fill_in :import_group_path, with: 'custom-path'
+ attach_file(file) do
+ find('.js-filepicker-button').click
+ end
+
+ expect { click_on 'Import' }.to change { Group.count }.by 1
+
+ group = Group.find_by(name: 'Test Group Import')
+ expect(group.path).to eq 'custom-path'
+ end
+ end
+
+ context 'when the path is already taken' do
+ before do
+ create(:group, path: 'test-group-import')
+ end
+
+ it 'suggests a unique path' do
+ visit new_group_path
+ click_link 'Import group'
+
+ fill_in :import_group_path, with: 'test-group-import'
+ expect(page).to have_content "Group path is already taken. We've suggested one that is available."
+ end
+ end
+ end
+
+ context 'when the user uploads an invalid export file' do
+ let(:file) { File.join(Rails.root, 'spec', %w[fixtures big-image.png]) }
+
+ it 'displays an error', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/343995' do
+ visit new_group_path
+ click_link 'Import group'
+
+ fill_in :import_group_name, with: 'Test Group Import'
+ attach_file(file) do
+ find('.js-filepicker-button').click
+ end
+
+ expect { click_on 'Import' }.not_to change { Group.count }
+
+ page.within('.flash-container') do
+ expect(page).to have_content('Unable to process group import file')
+ end
+ end
+ end
+ end
+
context 'when the user uploads a valid export file' do
let(:file) { File.join(Rails.root, 'spec', %w[fixtures group_export.tar.gz]) }
@@ -30,7 +118,7 @@
visit new_group_path
click_link 'Import group'
- fill_in :import_group_name, with: group_name
+ fill_in s_('Groups|Group name'), with: group_name
expect(page).to have_content 'Import group from file'
attach_file(file) do
@@ -41,10 +129,12 @@
group = Group.find_by(name: group_name)
- expect(group).not_to be_nil
- expect(group.description).to eq 'A voluptate non sequi temporibus quam at.'
- expect(group.path).to eq 'test-group-import'
- expect(group.import_state.status).to eq GroupImportState.state_machine.states[:finished].value
+ aggregate_failures do
+ expect(group).not_to be_nil
+ expect(group.description).to eq 'A voluptate non sequi temporibus quam at.'
+ expect(group.path).to eq 'test-group-import'
+ expect(group.import_state.status).to eq GroupImportState.state_machine.states[:finished].value
+ end
end
end
@@ -53,9 +143,9 @@
visit new_group_path
click_link 'Import group'
- fill_in :import_group_name, with: 'Test Group Import'
+ fill_in s_('Groups|Group name'), with: 'Test Group Import'
- fill_in :import_group_path, with: 'custom-path'
+ fill_in s_('Groups|Group URL'), with: 'custom-path'
attach_file(file) do
find('.js-filepicker-button').click
end
@@ -76,8 +166,10 @@
visit new_group_path
click_link 'Import group'
- fill_in :import_group_path, with: 'test-group-import'
- expect(page).to have_content "Group path is already taken. We've suggested one that is available."
+ fill_in s_('Groups|Group URL'), with: 'test-group-import'
+ expect(page).to have_content s_(
+ 'Groups|Group path is unavailable. Path has been replaced with a suggested available path.'
+ )
end
end
end
@@ -89,7 +181,7 @@
visit new_group_path
click_link 'Import group'
- fill_in :import_group_name, with: 'Test Group Import'
+ fill_in s_('Groups|Group name'), with: 'Test Group Import'
attach_file(file) do
find('.js-filepicker-button').click
end
--
GitLab