From 876ab436fabf2f44e2a6912262f980256b7c9736 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Tue, 4 Dec 2018 16:42:27 +0800 Subject: [PATCH 1/6] Add Import CSV Frontend Added button and modal to accept CSV file for uploading --- app/assets/stylesheets/framework/modal.scss | 38 ++++++++++++++++++ .../stylesheets/framework/variables.scss | 1 + app/assets/stylesheets/pages/issues.scss | 8 ++++ app/controllers/projects/issues_controller.rb | 13 +++++-- app/views/projects/issues/_import_export.svg | 1 + app/views/projects/issues/_nav_btns.html.haml | 39 ++++++++++++++----- .../issues/import_csv/_button.html.haml | 9 +++++ .../issues/import_csv/_modal.html.haml | 25 ++++++++++++ app/views/projects/issues/index.html.haml | 5 +-- .../shared/empty_states/_issues.html.haml | 13 ++++++- .../shared/issuable/_feed_buttons.html.haml | 4 +- app/views/shared/issuable/_filter.html.haml | 32 --------------- config/routes/project.rb | 1 + 13 files changed, 137 insertions(+), 52 deletions(-) create mode 100644 app/views/projects/issues/_import_export.svg create mode 100644 app/views/projects/issues/import_csv/_button.html.haml create mode 100644 app/views/projects/issues/import_csv/_modal.html.haml delete mode 100644 app/views/shared/issuable/_filter.html.haml diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index 46d40ea7aa5..ace46e32b18 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/app/assets/stylesheets/framework/modal.scss @@ -101,3 +101,41 @@ body.modal-open { margin: 0; } } + +.issues-import-modal, +.issues-export-modal { + .modal-header { + justify-content: flex-start; + + .import-export-svg-container { + flex-grow: 1; + height: 56px; + padding: $gl-btn-padding $gl-btn-padding 0; + + > svg { + float: right; + height: 100%; + } + } + } + + .modal-body { + padding: 0; + + .modal-subheader { + justify-content: flex-start; + align-items: center; + border-bottom: 1px solid $modal-border-color; + padding: 14px; + } + + .modal-text { + padding: $gl-padding-24 $gl-padding; + min-height: $modal-body-height; + } + } + + .checkmark { + color: $green-400; + } +} diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index d92d81b2cb5..242977e8543 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -656,6 +656,7 @@ $border-color-settings: #e1e1e1; Modals */ $modal-body-height: 134px; +$modal-border-color: #e9ecef; $priority-label-empty-state-width: 114px; diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index bb6b6f84849..6c847fc0d53 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/app/assets/stylesheets/pages/issues.scss @@ -155,6 +155,14 @@ ul.related-merge-requests > li { } } +.issues-nav-controls { + font-size: 0; + + .btn-group:empty { + display: none; + } +} + .issuable-email-modal-btn { padding: 0; color: $blue-600; diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 5ed46fc0545..ae48a02f623 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -10,7 +10,7 @@ class Projects::IssuesController < Projects::ApplicationController include SpammableActions def self.issue_except_actions - %i[index calendar new create bulk_update] + %i[index calendar new create bulk_update import_csv] end def self.set_issuables_index_only_actions @@ -155,11 +155,11 @@ class Projects::IssuesController < Projects::ApplicationController def can_create_branch can_create = current_user && can?(current_user, :push_code, @project) && - @issue.can_be_worked_on? + issue.can_be_worked_on? respond_to do |format| format.json do - render json: { can_create_branch: can_create, suggested_branch_name: @issue.suggested_branch_name } + render json: { can_create_branch: can_create, suggested_branch_name: issue.suggested_branch_name } end end end @@ -175,6 +175,13 @@ class Projects::IssuesController < Projects::ApplicationController end end + def import_csv + redirect_to( + project_issues_path(project), + notice: _("Your issues are being imported. Once finished, you'll get a confirmation email.") + ) + end + protected # rubocop: disable CodeReuse/ActiveRecord diff --git a/app/views/projects/issues/_import_export.svg b/app/views/projects/issues/_import_export.svg new file mode 100644 index 00000000000..53c35d12f57 --- /dev/null +++ b/app/views/projects/issues/_import_export.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/app/views/projects/issues/_nav_btns.html.haml b/app/views/projects/issues/_nav_btns.html.haml index e4a0d4b8479..4a4b8a9fcad 100644 --- a/app/views/projects/issues/_nav_btns.html.haml +++ b/app/views/projects/issues/_nav_btns.html.haml @@ -1,11 +1,30 @@ -= render 'shared/issuable/feed_buttons' +- show_feed_buttons = local_assigns.fetch(:show_feed_buttons, true) +- show_import_button = local_assigns.fetch(:show_import_button, true) && Feature.enabled?(:issues_import_csv) +- show_export_button = local_assigns.fetch(:show_export_button, true) -- if @can_bulk_update - = button_tag "Edit issues", class: "btn btn-default append-right-10 js-bulk-update-toggle" -- if show_new_issue_link?(@project) - = link_to "New issue", new_project_issue_path(@project, - issue: { assignee_id: finder.assignee.try(:id), - milestone_id: finder.milestones.first.try(:id) }), - class: "btn btn-success", - title: "New issue", - id: "new_issue_link" +.nav-controls.issues-nav-controls + - if show_feed_buttons + = render 'shared/issuable/feed_buttons' + + .btn-group.append-right-10< + - if show_export_button + = render_if_exists 'projects/issues/export_csv/button' + + - if show_import_button + = render 'projects/issues/import_csv/button' + + - if @can_bulk_update + = button_tag _("Edit issues"), class: "btn btn-default append-right-10 js-bulk-update-toggle" + - if show_new_issue_link?(@project) + = link_to _("New issue"), new_project_issue_path(@project, + issue: { assignee_id: finder.assignee.try(:id), + milestone_id: finder.milestones.first.try(:id) }), + class: "btn btn-success", + title: _("New issue"), + id: "new_issue_link" + +- if show_export_button + = render_if_exists 'projects/issues/export_csv/modal' + +- if show_import_button + = render 'projects/issues/import_csv/modal' diff --git a/app/views/projects/issues/import_csv/_button.html.haml b/app/views/projects/issues/import_csv/_button.html.haml new file mode 100644 index 00000000000..acc2c50294f --- /dev/null +++ b/app/views/projects/issues/import_csv/_button.html.haml @@ -0,0 +1,9 @@ +- type = local_assigns.fetch(:type, :icon) + +%button.csv-import-button.btn{ title: _('Import CSV'), class: ('has-tooltip' if type == :icon), + data: { toggle: 'modal', target: '.issues-import-modal' } } + - if type == :icon + = sprite_icon('upload') + - else + = _('Import CSV') + diff --git a/app/views/projects/issues/import_csv/_modal.html.haml b/app/views/projects/issues/import_csv/_modal.html.haml new file mode 100644 index 00000000000..e8576a2f17d --- /dev/null +++ b/app/views/projects/issues/import_csv/_modal.html.haml @@ -0,0 +1,25 @@ +.issues-import-modal.modal + .modal-dialog + .modal-content + = form_tag [:import_csv, @project.namespace.becomes(Namespace), @project, :issues], multipart: true do + .modal-header + %h3 + = _('Import issues') + .import-export-svg-container + = render 'projects/issues/import_export.svg' + %a.close{ href: '#', 'data-dismiss' => 'modal' } × + .modal-body + .modal-text + %p + = _("Your issues will be imported in the background. Once finished, you'll get a confirmation email.") + .form-group + = label_tag :file, _('Upload CSV File'), class: 'label-bold' + %div + = file_field_tag :file, accept: '.csv', required: true + %p.text-secondary + = _('It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected.') + %p.text-secondary + = _('The maximum file size allowed is %{size}.') % {size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes)} + .modal-footer + %button{ type: 'submit', class: 'btn btn-success', title: _('Import issues') } + = _('Import issues') diff --git a/app/views/projects/issues/index.html.haml b/app/views/projects/issues/index.html.haml index 1e7737aeb97..39e9e9171cf 100644 --- a/app/views/projects/issues/index.html.haml +++ b/app/views/projects/issues/index.html.haml @@ -11,8 +11,7 @@ %div{ class: (container_class) } .top-area = render 'shared/issuable/nav', type: :issues - .nav-controls - = render "projects/issues/nav_btns" + = render "projects/issues/nav_btns" = render 'shared/issuable/search_bar', type: :issues - if @can_bulk_update @@ -23,4 +22,4 @@ - if new_issue_email = render 'projects/issuable_by_email', email: new_issue_email, issuable_type: 'issue' - else - = render 'shared/empty_states/issues', button_path: new_project_issue_path(@project) + = render 'shared/empty_states/issues', button_path: new_project_issue_path(@project), show_import_button: true diff --git a/app/views/shared/empty_states/_issues.html.haml b/app/views/shared/empty_states/_issues.html.haml index 0ddc56dc6c3..76f6a294cb3 100644 --- a/app/views/shared/empty_states/_issues.html.haml +++ b/app/views/shared/empty_states/_issues.html.haml @@ -1,5 +1,6 @@ - button_path = local_assigns.fetch(:button_path, false) - project_select_button = local_assigns.fetch(:project_select_button, false) +- show_import_button = local_assigns.fetch(:show_import_button, false) - has_button = button_path || project_select_button .row.empty-state @@ -21,12 +22,20 @@ - if has_button .text-center - if project_select_button - = render 'shared/new_project_item_select', path: 'issues/new', label: 'New issue', type: :issues, with_feature_enabled: 'issues' + = render 'shared/new_project_item_select', path: 'issues/new', label: _('New issue'), type: :issues, with_feature_enabled: 'issues' - else - = link_to 'New issue', button_path, class: 'btn btn-success', title: 'New issue', id: 'new_issue_link' + = link_to _('New issue'), button_path, class: 'btn btn-success', title: _('New issue'), id: 'new_issue_link' + + - if show_import_button + = render 'projects/issues/import_csv/button', type: :text + - else %h4.text-center= _("There are no issues to show") %p = _("The Issue Tracker is the place to add things that need to be improved or solved in a project. You can register or sign in to create issues for this project.") .text-center = link_to _('Register / Sign In'), new_user_session_path, class: 'btn btn-success' + +- if show_import_button + = render 'projects/issues/import_csv/modal' + diff --git a/app/views/shared/issuable/_feed_buttons.html.haml b/app/views/shared/issuable/_feed_buttons.html.haml index d4834090413..83f60fa6fe2 100644 --- a/app/views/shared/issuable/_feed_buttons.html.haml +++ b/app/views/shared/issuable/_feed_buttons.html.haml @@ -1,4 +1,4 @@ -= link_to safe_params.merge(rss_url_options), class: 'btn has-tooltip', data: { container: 'body' }, title: 'Subscribe to RSS feed' do += link_to safe_params.merge(rss_url_options), class: 'btn has-tooltip', data: { container: 'body' }, title: _('Subscribe to RSS feed') do = icon('rss') -= link_to safe_params.merge(calendar_url_options), class: 'btn has-tooltip', data: { container: 'body' }, title: 'Subscribe to calendar' do += link_to safe_params.merge(calendar_url_options), class: 'btn has-tooltip', data: { container: 'body' }, title: _('Subscribe to calendar') do = custom_icon('icon_calendar') diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml deleted file mode 100644 index 2ca4657851c..00000000000 --- a/app/views/shared/issuable/_filter.html.haml +++ /dev/null @@ -1,32 +0,0 @@ -.issues-filters - .issues-details-filters.row-content-block.second-block - = form_tag page_filter_path(without: [:assignee_id, :author_id, :milestone_title, :label_name, :search]), method: :get, class: 'filter-form js-filter-form' do - - if params[:search].present? - = hidden_field_tag :search, params[:search] - .issues-other-filters - .filter-item.inline - - if params[:author_id].present? - = hidden_field_tag(:author_id, params[:author_id]) - = dropdown_tag(user_dropdown_label(params[:author_id], "Author"), options: { toggle_class: "js-user-search js-filter-submit js-author-search", title: "Filter by author", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable dropdown-menu-author js-filter-submit", - placeholder: "Search authors", data: { any_user: "Any Author", first_user: current_user&.username, current_user: true, project_id: @project&.id, group_id: @group&.id, selected: params[:author_id], field_name: "author_id", default_label: "Author" } }) - - .filter-item.inline - - if params[:assignee_id].present? - = hidden_field_tag(:assignee_id, params[:assignee_id]) - = dropdown_tag(user_dropdown_label(params[:assignee_id], "Assignee"), options: { toggle_class: "js-user-search js-filter-submit js-assignee-search", title: "Filter by assignee", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable dropdown-menu-assignee js-filter-submit", - placeholder: "Search assignee", data: { any_user: "Any Assignee", first_user: current_user&.username, null_user: true, current_user: true, project_id: @project&.id, group_id: @group&.id, selected: params[:assignee_id], field_name: "assignee_id", default_label: "Assignee" } }) - - .filter-item.inline.milestone-filter - = render "shared/issuable/milestone_dropdown", selected: finder.milestones.try(:first), name: :milestone_title, show_any: true, show_upcoming: true, show_started: true - - .filter-item.inline.labels-filter - = render "shared/issuable/label_dropdown", selected: selected_labels, use_id: false, selected_toggle: params[:label_name], data_options: { field_name: "label_name[]" } - - - unless @no_filters_set - .float-right - = render 'shared/issuable/sort_dropdown' - - - has_labels = @labels && @labels.any? - .row-content-block.second-block.filtered-labels{ class: ("hidden" unless has_labels) } - - if has_labels - = render 'shared/labels_row', labels: @labels diff --git a/config/routes/project.rb b/config/routes/project.rb index f50bf5ab76f..cf5a57300cf 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -361,6 +361,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do end collection do post :bulk_update + post :import_csv end end From 3c026971149c95f076b8c50a52ddbfed139d5b20 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Thu, 6 Dec 2018 08:51:30 +0800 Subject: [PATCH 2/6] Import CSV Backend Process CSV uploads async using a worker then email results --- app/controllers/projects/issues_controller.rb | 21 ++++-- app/mailers/emails/issues.rb | 11 ++++ app/mailers/previews/notify_preview.rb | 4 ++ app/policies/project_policy.rb | 1 + app/services/issues/import_csv_service.rb | 65 ++++++++++++++++++ app/services/upload_service.rb | 8 ++- .../notify/import_issues_csv_email.html.haml | 18 +++++ .../notify/import_issues_csv_email.text.erb | 11 ++++ app/views/projects/issues/_nav_btns.html.haml | 2 +- .../issues/import_csv/_modal.html.haml | 4 +- app/workers/all_queues.yml | 1 + app/workers/import_issues_csv_worker.rb | 16 +++++ config/sidekiq_queues.yml | 1 + locale/gitlab.pot | 33 ++++++++++ .../projects/issues_controller_spec.rb | 66 +++++++++++++++++++ spec/fixtures/csv_comma.csv | 4 ++ spec/fixtures/csv_semicolon.csv | 5 ++ spec/fixtures/csv_tab.csv | 4 ++ spec/mailers/emails/issues_spec.rb | 33 ++++++++++ .../issues/import_csv_service_spec.rb | 64 ++++++++++++++++++ spec/workers/import_issues_csv_worker_spec.rb | 21 ++++++ 21 files changed, 381 insertions(+), 12 deletions(-) create mode 100644 app/services/issues/import_csv_service.rb create mode 100644 app/views/notify/import_issues_csv_email.html.haml create mode 100644 app/views/notify/import_issues_csv_email.text.erb create mode 100644 app/workers/import_issues_csv_worker.rb create mode 100644 spec/fixtures/csv_comma.csv create mode 100644 spec/fixtures/csv_semicolon.csv create mode 100644 spec/fixtures/csv_tab.csv create mode 100644 spec/mailers/emails/issues_spec.rb create mode 100644 spec/services/issues/import_csv_service_spec.rb create mode 100644 spec/workers/import_issues_csv_worker_spec.rb diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index ae48a02f623..f88eb9e0322 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -155,11 +155,11 @@ class Projects::IssuesController < Projects::ApplicationController def can_create_branch can_create = current_user && can?(current_user, :push_code, @project) && - issue.can_be_worked_on? + @issue.can_be_worked_on? respond_to do |format| format.json do - render json: { can_create_branch: can_create, suggested_branch_name: issue.suggested_branch_name } + render json: { can_create_branch: can_create, suggested_branch_name: @issue.suggested_branch_name } end end end @@ -176,10 +176,19 @@ class Projects::IssuesController < Projects::ApplicationController end def import_csv - redirect_to( - project_issues_path(project), - notice: _("Your issues are being imported. Once finished, you'll get a confirmation email.") - ) + return render_404 unless Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, project) + + service = UploadService.new(project, params[:file]) + + if service.execute + ImportIssuesCsvWorker.perform_async(current_user.id, project.id, service.uploader.upload.id) + + flash[:notice] = _("Your issues are being imported. Once finished, you'll get a confirmation email.") + else + flash[:alert] = _("File upload error.") + end + + redirect_to project_issues_path(project) end protected diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 370e6d2f90b..654ae211310 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -77,6 +77,17 @@ module Emails mail_answer_thread(issue, issue_thread_options(updated_by_user.id, recipient.id, reason)) end + def import_issues_csv_email(user_id, project_id, results) + @user = User.find(user_id) + @project = Project.find(project_id) + @results = results + + mail(to: @user.notification_email, subject: subject('Imported issues')) do |format| + format.html { render layout: 'mailer' } + format.text { render layout: 'mailer' } + end + end + private def setup_issue_mail(issue_id, recipient_id) diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 2ac4610967d..80e0a17c312 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -76,6 +76,10 @@ class NotifyPreview < ActionMailer::Preview Notify.changed_milestone_issue_email(user.id, issue.id, milestone, user.id) end + def import_issues_csv_email + Notify.import_issues_csv_email(user, project, { success: 3, errors: [5, 6, 7], valid_file: true }) + end + def closed_merge_request_email Notify.closed_merge_request_email(user.id, issue.id, user.id).message end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 3146f26bed5..0026a88b972 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -223,6 +223,7 @@ class ProjectPolicy < BasePolicy rule { ~request_access_enabled }.prevent :request_access rule { can?(:developer_access) }.policy do + enable :import_issues enable :admin_merge_request enable :admin_milestone enable :update_merge_request diff --git a/app/services/issues/import_csv_service.rb b/app/services/issues/import_csv_service.rb new file mode 100644 index 00000000000..411000a7066 --- /dev/null +++ b/app/services/issues/import_csv_service.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Issues + class ImportCsvService + def initialize(user, project, upload) + @user = user + @project = project + @uploader = upload.build_uploader + @results = { success: 0, errors: [], valid_file: true } + end + + def execute + # Cache remote file locally for processing + @uploader.cache_stored_file! unless @uploader.file_storage? + + process_csv + email_results_to_user + + cleanup_cache unless @uploader.file_storage? + + @results + end + + private + + def process_csv + CSV.foreach(@uploader.file.path, col_sep: detect_col_sep, headers: true).with_index(2) do |row, line_no| + issue = Issues::CreateService.new(@project, @user, title: row[0], description: row[1]).execute + + if issue.persisted? + @results[:success] += 1 + else + @results[:errors].push(line_no) + end + end + rescue ArgumentError, CSV::MalformedCSVError + @results[:valid_file] = false + end + + def email_results_to_user + Notify.import_issues_csv_email(@user.id, @project.id, @results).deliver_now + end + + def detect_col_sep + header = File.open(@uploader.file.path, &:readline) + + if header.include?(",") + "," + elsif header.include?(";") + ";" + elsif header.include?("\t") + "\t" + else + raise CSV::MalformedCSVError + end + end + + def cleanup_cache + cached_file_path = @uploader.file.cache_path + + File.delete(cached_file_path) + Dir.delete(File.dirname(cached_file_path)) + end + end +end diff --git a/app/services/upload_service.rb b/app/services/upload_service.rb index 39909ee4f82..47903eb48c3 100644 --- a/app/services/upload_service.rb +++ b/app/services/upload_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class UploadService + attr_accessor :uploader + def initialize(model, file, uploader_class = FileUploader, **uploader_context) @model, @file, @uploader_class, @uploader_context = model, file, uploader_class, uploader_context end @@ -8,10 +10,10 @@ class UploadService def execute return nil unless @file && @file.size <= max_attachment_size - uploader = @uploader_class.new(@model, nil, @uploader_context) - uploader.store!(@file) + @uploader = @uploader_class.new(@model, nil, @uploader_context) + @uploader.store!(@file) - uploader.to_h + @uploader.to_h end private diff --git a/app/views/notify/import_issues_csv_email.html.haml b/app/views/notify/import_issues_csv_email.html.haml new file mode 100644 index 00000000000..a6a797a5fb7 --- /dev/null +++ b/app/views/notify/import_issues_csv_email.html.haml @@ -0,0 +1,18 @@ +- text_style = 'font-size:16px; text-align:center; line-height:30px;' + +%p{ style: text_style } + Your CSV import for project + %a{ href: project_url(@project), style: "color:#3777b0; text-decoration:none;" } + = @project.full_name + has been completed. + +%p{ style: text_style } + #{pluralize(@results[:success], 'issue')} imported successfully. + +- if @results[:errors].present? + %p{ style: text_style } + Errors found on line #{'number'.pluralize(@results[:errors].size)}: #{@results[:errors].join(', ')}. + +- unless @results[:valid_file] + %p{ style: text_style } + Error parsing CSV file. diff --git a/app/views/notify/import_issues_csv_email.text.erb b/app/views/notify/import_issues_csv_email.text.erb new file mode 100644 index 00000000000..54a1762c4ec --- /dev/null +++ b/app/views/notify/import_issues_csv_email.text.erb @@ -0,0 +1,11 @@ +Your CSV import for project <%= @project.full_name %> (<%= project_url(@project) %>) has been completed. + +<%= pluralize(@results[:success], 'issue') %> imported successfully. + +<% if @results[:errors].present? %> +Errors found on line <%= 'number'.pluralize(@results[:errors].size) %>: <%= @results[:errors].join(', ') %>. +<% end %> + +<% unless @results[:valid_file] %> +Error parsing CSV file. +<% end %> diff --git a/app/views/projects/issues/_nav_btns.html.haml b/app/views/projects/issues/_nav_btns.html.haml index 4a4b8a9fcad..fd6559e37ba 100644 --- a/app/views/projects/issues/_nav_btns.html.haml +++ b/app/views/projects/issues/_nav_btns.html.haml @@ -1,5 +1,5 @@ - show_feed_buttons = local_assigns.fetch(:show_feed_buttons, true) -- show_import_button = local_assigns.fetch(:show_import_button, true) && Feature.enabled?(:issues_import_csv) +- show_import_button = local_assigns.fetch(:show_import_button, true) && Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, @project) - show_export_button = local_assigns.fetch(:show_export_button, true) .nav-controls.issues-nav-controls diff --git a/app/views/projects/issues/import_csv/_modal.html.haml b/app/views/projects/issues/import_csv/_modal.html.haml index e8576a2f17d..03a6c79ff74 100644 --- a/app/views/projects/issues/import_csv/_modal.html.haml +++ b/app/views/projects/issues/import_csv/_modal.html.haml @@ -13,9 +13,9 @@ %p = _("Your issues will be imported in the background. Once finished, you'll get a confirmation email.") .form-group - = label_tag :file, _('Upload CSV File'), class: 'label-bold' + = label_tag :file, _('Upload CSV file'), class: 'label-bold' %div - = file_field_tag :file, accept: '.csv', required: true + = file_field_tag :file, accept: 'text/csv', required: true %p.text-secondary = _('It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected.') %p.text-secondary diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index d3cf21db335..223ddc80c88 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -140,3 +140,4 @@ - detect_repository_languages - repository_cleanup - delete_stored_files +- import_issues_csv diff --git a/app/workers/import_issues_csv_worker.rb b/app/workers/import_issues_csv_worker.rb new file mode 100644 index 00000000000..5d8c02a0605 --- /dev/null +++ b/app/workers/import_issues_csv_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class ImportIssuesCsvWorker + include ApplicationWorker + + def perform(current_user_id, project_id, upload_id) + @user = User.find(current_user_id) + @project = Project.find(project_id) + @upload = Upload.find(upload_id) + + importer = Issues::ImportCsvService.new(@user, @project, @upload) + importer.execute + + @upload.destroy + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 3ee32678f34..3e8c218052d 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -85,3 +85,4 @@ - [repository_cleanup, 1] - [delete_stored_files, 1] - [remote_mirror_notification, 2] + - [import_issues_csv, 2] diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1bb94c5a61a..4a77e09bed9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2683,6 +2683,9 @@ msgstr "" msgid "Edit identity for %{user_name}" msgstr "" +msgid "Edit issues" +msgstr "" + msgid "Email" msgstr "" @@ -3067,6 +3070,9 @@ msgstr "" msgid "File templates" msgstr "" +msgid "File upload error." +msgstr "" + msgid "Files" msgstr "" @@ -3573,6 +3579,9 @@ msgstr "" msgid "Import" msgstr "" +msgid "Import CSV" +msgstr "" + msgid "Import Projects from Gitea" msgstr "" @@ -3591,6 +3600,9 @@ msgstr "" msgid "Import in progress" msgstr "" +msgid "Import issues" +msgstr "" + msgid "Import multiple repositories by uploading a manifest file." msgstr "" @@ -3729,6 +3741,9 @@ msgstr "" msgid "Issues, merge requests, pushes and comments." msgstr "" +msgid "It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected." +msgstr "" + msgid "It's you" msgstr "" @@ -6440,6 +6455,12 @@ msgstr "" msgid "Subscribe at project level" msgstr "" +msgid "Subscribe to RSS feed" +msgstr "" + +msgid "Subscribe to calendar" +msgstr "" + msgid "Subscribed" msgstr "" @@ -6602,6 +6623,9 @@ msgstr "" msgid "The maximum file size allowed is %{max_attachment_size}mb" msgstr "" +msgid "The maximum file size allowed is %{size}." +msgstr "" + msgid "The maximum file size allowed is 200KB." msgstr "" @@ -7284,6 +7308,9 @@ msgstr "" msgid "Upload GoogleCodeProjectHosting.json here:" msgstr "" +msgid "Upload CSV file" +msgstr "" + msgid "Upload New File" msgstr "" @@ -7830,6 +7857,12 @@ msgstr "" msgid "Your groups" msgstr "" +msgid "Your issues are being imported. Once finished, you'll get a confirmation email." +msgstr "" + +msgid "Your issues will be imported in the background. Once finished, you'll get a confirmation email." +msgstr "" + msgid "Your name" msgstr "" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index a239ac16c0d..df21dc7bc85 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1026,6 +1026,72 @@ describe Projects::IssuesController do end end + describe 'POST #import_csv' do + let(:project) { create(:project, :public) } + let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } + + context 'feature disabled' do + it 'returns 404' do + sign_in(user) + project.add_maintainer(user) + + stub_feature_flags(issues_import_csv: false) + + import_csv + + expect(response).to have_gitlab_http_status :not_found + end + end + + context 'unauthorized' do + it 'returns 404 for guests' do + sign_out(:user) + + import_csv + + expect(response).to have_gitlab_http_status :not_found + end + + it 'returns 404 for project members with reporter role' do + sign_in(user) + project.add_reporter(user) + + import_csv + + expect(response).to have_gitlab_http_status :not_found + end + end + + context 'authorized' do + before do + sign_in(user) + project.add_developer(user) + end + + it "returns 302 for project members with developer role" do + import_csv + + expect(flash[:notice]).to include('Your issues are being imported') + expect(response).to redirect_to(project_issues_path(project)) + end + + it "shows error when upload fails" do + allow_any_instance_of(UploadService).to receive(:execute).and_return(nil) + + import_csv + + expect(flash[:alert]).to include('File upload error.') + expect(response).to redirect_to(project_issues_path(project)) + end + end + + def import_csv + post :import_csv, namespace_id: project.namespace.to_param, + project_id: project.to_param, + file: file + end + end + describe 'GET #discussions' do let!(:discussion) { create(:discussion_note_on_issue, noteable: issue, project: issue.project) } context 'when authenticated' do diff --git a/spec/fixtures/csv_comma.csv b/spec/fixtures/csv_comma.csv new file mode 100644 index 00000000000..e477a27d243 --- /dev/null +++ b/spec/fixtures/csv_comma.csv @@ -0,0 +1,4 @@ +title,description +Issue in 中文,Test description +"Hello","World" +"Title with quote""",Description diff --git a/spec/fixtures/csv_semicolon.csv b/spec/fixtures/csv_semicolon.csv new file mode 100644 index 00000000000..679797489e2 --- /dev/null +++ b/spec/fixtures/csv_semicolon.csv @@ -0,0 +1,5 @@ +title;description +Issue in 中文;Test description +Title with, comma;"Description" + +"Hello";"World" diff --git a/spec/fixtures/csv_tab.csv b/spec/fixtures/csv_tab.csv new file mode 100644 index 00000000000..f801794ea9c --- /dev/null +++ b/spec/fixtures/csv_tab.csv @@ -0,0 +1,4 @@ +title description +Issue in 中文 Test description + "Error Row" +"Hello" "World" diff --git a/spec/mailers/emails/issues_spec.rb b/spec/mailers/emails/issues_spec.rb new file mode 100644 index 00000000000..68c87803b78 --- /dev/null +++ b/spec/mailers/emails/issues_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'email_spec' + +describe Emails::Issues do + include EmailSpec::Matchers + + describe "#import_issues_csv_email" do + let(:user) { create(:user) } + let(:project) { create(:project) } + + subject { Notify.import_issues_csv_email(user.id, project.id, @results) } + + it "shows number of successful issues imported" do + @results = { success: 165, errors: [], valid_file: true } + + expect(subject).to have_body_text "165 issues imported" + end + + it "shows error when file is invalid" do + @results = { success: 0, errors: [], valid_file: false } + + expect(subject).to have_body_text "Error parsing CSV" + end + + it "shows line numbers with errors" do + @results = { success: 0, errors: [23, 34, 58], valid_file: false } + + expect(subject).to have_body_text "23, 34, 58" + end + end +end diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb new file mode 100644 index 00000000000..f5e58bc9a9e --- /dev/null +++ b/spec/services/issues/import_csv_service_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Issues::ImportCsvService do + let(:project) { create(:project) } + let(:user) { create(:user) } + + subject do + uploader = FileUploader.new(project) + uploader.store!(file) + + described_class.new(user, project, uploader.upload).execute + end + + describe '#execute' do + context 'invalid file' do + let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } + + it 'returns invalid file error' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(0) + expect(subject[:valid_file]).to eq(false) + end + end + + context 'comma delimited file' do + let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } + + it 'imports CSV without errors' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(3) + expect(subject[:errors]).to eq([]) + expect(subject[:valid_file]).to eq(true) + end + end + + context 'tab delimited file with error row' do + let(:file) { fixture_file_upload('spec/fixtures/csv_tab.csv') } + + it 'imports CSV with some error rows' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(2) + expect(subject[:errors]).to eq([3]) + expect(subject[:valid_file]).to eq(true) + end + end + + context 'semicolon delimited file with CRLF' do + let(:file) { fixture_file_upload('spec/fixtures/csv_semicolon.csv') } + + it 'imports CSV with a blank row' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(3) + expect(subject[:errors]).to eq([4]) + expect(subject[:valid_file]).to eq(true) + end + end + end +end diff --git a/spec/workers/import_issues_csv_worker_spec.rb b/spec/workers/import_issues_csv_worker_spec.rb new file mode 100644 index 00000000000..89370c4890d --- /dev/null +++ b/spec/workers/import_issues_csv_worker_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ImportIssuesCsvWorker do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:upload) { create(:upload) } + + let(:worker) { described_class.new } + + describe '#perform' do + it 'calls #execute on Issues::ImportCsvService and destroys upload' do + expect_any_instance_of(Issues::ImportCsvService).to receive(:execute).and_return({ success: 5, errors: [], valid_file: true }) + + worker.perform(user.id, project.id, upload.id) + + expect { upload.reload }.to raise_error ActiveRecord::RecordNotFound + end + end +end From ccbc45559b7bedd1d76c1840bbdfba3cce542af7 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Fri, 7 Dec 2018 21:19:03 +0800 Subject: [PATCH 3/6] Add CSV Import documentation Document importing of CSVs with format information and sample data --- .../unreleased/49231-import-issues-csv.yml | 5 ++ doc/user/project/issues/csv_import.md | 45 ++++++++++++++++++ .../project/issues/img/import_csv_button.png | Bin 0 -> 4342 bytes doc/user/project/issues/index.md | 9 ++++ 4 files changed, 59 insertions(+) create mode 100644 changelogs/unreleased/49231-import-issues-csv.yml create mode 100644 doc/user/project/issues/csv_import.md create mode 100644 doc/user/project/issues/img/import_csv_button.png diff --git a/changelogs/unreleased/49231-import-issues-csv.yml b/changelogs/unreleased/49231-import-issues-csv.yml new file mode 100644 index 00000000000..c10bd8143b2 --- /dev/null +++ b/changelogs/unreleased/49231-import-issues-csv.yml @@ -0,0 +1,5 @@ +--- +title: Add importing of issues from CSV file +merge_request: 23532 +author: +type: added diff --git a/doc/user/project/issues/csv_import.md b/doc/user/project/issues/csv_import.md new file mode 100644 index 00000000000..001e0d303e9 --- /dev/null +++ b/doc/user/project/issues/csv_import.md @@ -0,0 +1,45 @@ +# Importing Issues from CSV + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23532) in GitLab 11.7. + +Issues can be imported by uploading a CSV file. The file will be processed in the background and a notification email +will be sent to you once the import is completed. + +> **Note:** A permission level of `Developer` or higher is required to import issues. + +## CSV File Format + +### Header row + +CSV files must contain a header row with at least two columns: `title` and `description`, in that order. + +### Column separator + +The column separator is automatically detected from the header row. + +Supported separator characters are: commas (`,`), semicolons (`;`), and tabs (`\t`). + +### Row separator + +Lines ending in either `CRLF` or `LF` are supported. + +### Quote character + +The double-quote (`"`) character is used to quote fields so you can use the column separator within a field. To insert +a double-quote (`"`) within a quoted field, use two double-quote characters in succession, i.e. `""`. + +### Data rows + +After the header row, succeeding rows must follow the same column order. The issue title is required while the +description is optional. + +The user uploading the CSV file will be set as the author of the imported issues. + +## Sample Data + +```csv +title,description +My Issue Title,My Issue Description +Another Title,"A description, with a comma" +"One More Title","One More Description" +``` diff --git a/doc/user/project/issues/img/import_csv_button.png b/doc/user/project/issues/img/import_csv_button.png new file mode 100644 index 0000000000000000000000000000000000000000..ab100a9575087d5808d47e14b12e511423e45544 GIT binary patch literal 4342 zcmbVQX*d*Y+vcINrKzT|Rz{`CUdb9UM#fIr2~SjLgfW9`iJ8d~M)t8}m@t$jTgW3XJ;K9vSP9)L;^E-<>yNpyr5=mD&StZBcXvgLtwcmb=I7_v*49>5R%Vul zK?!gW2$YbLASWj`H#g_w<8#QZW3mnoI>-@z8|>kH1{>e8psA?|27?K7rt919EOzg; zYf8<{&Fsm9Y%~gWD0E9gfGKUUnU@nM@{|y}&vcW37H1 z8m> z(UQOyVQA5k$9J@^tI5lUQ}_EfSRJhOsGJY%G%g~4EqGDZdl2db*7uE8J2mY#Zw|Hm$?C0+G z_VzM+UAlWq{)SsugsF$pP3tLU*VrO!cN$YQwzf5tQPIg_)O3); zJv=-_i=7pe?cnnI5|mGSTOY(eR#~C)D>Cstd7)ahPY?)1MZ2?^nVE31(MLb+P^j|a z4twJF(DK~prv;x;(ZyskdGl-DCN0&hC26uTE+gDePxWfq&d!H9W>DRNL*8V|=ojs< z&KGZ5{bDM=O~230%|-g>h`vu3Eyekuf@Vk1LwShoW@oh$E0MyxMltmIp5AB0jg;yS zYTi{Hjm349#O3KoqOWyc7d5S+i&#~YnUqxZI^vqKe`vLpy}m~7SGL<2y|lJtWvG32 zk`kFlrq8{;?GC_AYa;F;?&rO^kGcB*?UEJ&Ro(c|g{G^rT z*T0auJOj-*C!hS!kTb;VQMJ;VUoA+Zj+Ji4ZwG(u_{FqFOW)V?^-xv4*{wDb4!?7m zG*C?YX5sOUtLlCA<6#2KqVuFjj1^?Z;*7)Gb9sJ)z6XZM>fmbW*2h}fG7ft@CbNNa zrSW%8Sxul{aG9@10}NI$Bb~!<(_Do_6Omw`!bOq@%w`1VHE5u zNk$Rw4%6j~`id=M9?HH8S0E7Mft$wADK25xvl4$Q%_kSH$-`bvT5P*LO1CH)%Tjo_ zjH-9MI_=mi0c>w2JU#V_>5S!DV7LWj>E!5#Aq9@Hb&Brab3V!!M-g_=!ewBP9S4kS zJTp*;awy>ZniPmo!>#MTXinsoNcIjK`XITc77&_$xT%WO#(Ii05DTSp^#3iUNJ?_kDqZcUz#er1St4T*b{0bPn7=y$ zQ8ID7kiM+nW9WAjR2=N8wrC@L#(tp% z{8haloPUQ5V48jn?HZZ&EX`KDbW35hBub%pn?nq#&; zDhdmT88c*K@wEE6%RDBlV<%32yS=e&!|43kTSCSIc9lHsg~+)E8212COt}7U-vzMk zto)2v0fUb-k>bb&z40)xpDN3kO8u%Zl=xKvwvb^^nqTOch+X;m>ZtlB2_RpQOG>5D zMSKlK=00&sdveGeOJ4<@XgKx1O6XCK^<1_BdRc6{8Zogn{4PL z9S2Xq+(!rzj5jT$Q8QM$B<0)15@^klad-n=`Gs+Cr&o2uS1Hg6aF3o50+yW@Ak=&~ z<&cxyO<%B0dE6VSx3sM(gm`P9hpshU5L)HWN4m>vzI$20%WL9R8&^|H(cutes6l@t z`At4`#*x|_M#>le%oPo40=3fU355d}ZiCAxJ-5{6*`eS5>2IBXF}jix#&T@?OLADH z6Lyx;Y-i^B-~i3%^}tJEq1kkY(sXuCnLqMxakjW-cyj*(OzXLplHaJc5&7~B4m%lD z%wiT;U%8maC zC-iu^Tiy=;jh4LVhYCX+JyL`Qb1vDeE;Lcw@8!v_oSBc}`FH+uy%eI{BQC`E>-WR3 zv-?7-XPpy-PApg{R3X)0j(R#J;=lhZzdNfArhXc`^wD3o(opU<{;Lh(=n^yAM!V(m|{!W6( zyJx*L1wuEcY4Bp7zs`UB`mHgU|9cOQwnZO1df2aiv)VpsQ=uNPu4%)+{~g-*>_}&| zJy_r;v@88ggAPS7V4MHc%3d|T^GQ-C4v>f+hkm{QJ{1&;fk$DoqA`v_vlsY$PZ4rG zVZ@b`#_JC~gk$m)Rm>ZJIu<%Hs!^(tS!08jhz02~3r1fO44~#3VA>ARIgQHC%)B+C(r;qxj8sg^= zYCbrdbQ3@UAa9y~QnVCbH760LMWFWj+-`u}AD;)zk+>~=W~@n?9Fn^qIEx zw0wSAp^Qd*J*TV`Y+Y+7U!u{U*=n2K1Xg*6P>ipNFOL>KaWxm!`y3f$Du8|1FK0s{ zPT@=o$`Ds80|`9}*k!TxAcx?xpQBZRFr-AMF3eng@gk$`)-`l%zlsM0NA!Hdt(znX zp9Xk*^$#W<%F&_ASxIEoGSo=YKb!qngR$s1+_j8`1*9@Fh1`Pu5irxoo2 zG7xL`>FP9C{0n#?s`nS$9HAS6ECz}W3Lf`(fveKh9V3F7G`zn%TtkVJT?94@<1j0{ z7(Gs-Y3HmN{t3RRm|J*JGw(kT&WaB99SdGOLAWj6@oU-^085T=Bb&Okv75qj`)>he^kyK^!w;?TXzBc|8Jm8OZo?7SHrw(J@;EG z)8kBsy8xgXE>N82bMm%IzSq?TZ5;labFB~cNWnK%ZjDT_zeL(}wrOpns=Ima(I1^$ z5w{3Zx~hsI>as#8wcKcdnHJS~dAY+jXlK94Oc5W>ojODL)1CKZyus>$U9Jh|*_;+8 zm+SQF&@O3y6|Zqsi}Ut!sqTavTMNICMw@*0?mh(%{_($n*fbLQS~Pdb4v|O%U*8)1 zq_p4Z9cIT+{n6h|16(x{7C~#>+ju5(2|6z-vXV3-hD>`hYt?OlJCJp3#RLqVPOmo1 z@cq;_oV6kC?)pgWu{$T#`g=^A*Rh8tdRkNdRVp@lC6=1cvyu%mBz51R11%1;Pq6^*b=X1%|y&!*8 z>~;itQ?s95o;1Ai3Pq4Kt25K2F(WAfJ@k(m0>|ATh-o{%F-u;m)VEhCT_awZ@>dIb zCNxjTyAvABJYB0eMg4_0jb#EfE@JnPF)qxzXq!Z=V8n%FFmMqGXL4I&>vL~VYC=5~ z-J&pB6&E$z8bEN|QCKs4A9*{(G#y&Zo$i6)E^s^@hV=_P5}3$&x)pR?Y&Nn7$Q!tR z`N*e@7WjMk-6s;s8vgs*by;yT6^v}lpN#K1%BOdsG=;*JC_QPIS%3&X5SQ5I>Q=RWZB?-}cR7kD+?+Ab0mJ?->7 z$F@RrJ=V((u?p;>C2Xyx^Jz=qB3z8H$HXN-tu%@7CzM{>ku<5^GEk5c@1AdVla}ih z%R=*PD4K8heukE7jBe6xQ{1#|X-ij=2*e;4%2}pSamhOWNaf9+6 zmIH2N{rea^w#d00VD$)G+y}Lc!WO9?vInq5Vjbl%$1b}e55FWhI1KbmVWm))h<^Z< CNv7!l literal 0 HcmV?d00001 diff --git a/doc/user/project/issues/index.md b/doc/user/project/issues/index.md index 200b3a642a1..40a1f60c4ab 100644 --- a/doc/user/project/issues/index.md +++ b/doc/user/project/issues/index.md @@ -142,6 +142,15 @@ to find out more about this feature. With [GitLab Starter](https://about.gitlab.com/pricing/), you can also create various boards per project with [Multiple Issue Boards](https://docs.gitlab.com/ee/user/project/issue_board.html#multiple-issue-boards). +### Import Issues from CSV + +From the project-level issues list, you can find the import button near the "Edit issues" button in the upper-right +side. + +![Import CSV button](img/import_csv_button.png) + +Learn more about [importing issues from CSV](csv_import.md) + ### External Issue Tracker Alternatively to GitLab's built-in Issue Tracker, you can also use an [external From 63e9969ca3ac57839b78d9cc44bcf32bc9a45248 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 31 Dec 2018 11:23:50 +0800 Subject: [PATCH 4/6] Refactor upload service to return uploader Also changes old calls to the service --- app/controllers/concerns/uploads_actions.rb | 2 +- app/controllers/projects/issues_controller.rb | 10 +++++----- app/services/upload_service.rb | 8 +++----- lib/api/projects.rb | 2 +- lib/gitlab/email/attachment_uploader.rb | 2 +- lib/gitlab/import_export/uploads_manager.rb | 2 +- spec/services/upload_service_spec.rb | 2 +- 7 files changed, 13 insertions(+), 15 deletions(-) diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 0eea0cdd50f..5992ad5f1e1 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -7,7 +7,7 @@ module UploadsActions UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze def create - link_to_file = UploadService.new(model, params[:file], uploader_class).execute + link_to_file = UploadService.new(model, params[:file], uploader_class).execute.to_h respond_to do |format| if link_to_file diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index f88eb9e0322..21688e54481 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -37,6 +37,8 @@ class Projects::IssuesController < Projects::ApplicationController # Allow create a new branch and empty WIP merge request from current issue before_action :authorize_create_merge_request_from!, only: [:create_merge_request] + before_action :authorize_import_issues!, only: [:import_csv] + before_action :set_suggested_issues_feature_flags, only: [:new] respond_to :html @@ -176,12 +178,10 @@ class Projects::IssuesController < Projects::ApplicationController end def import_csv - return render_404 unless Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, project) + return render_404 unless Feature.enabled?(:issues_import_csv) - service = UploadService.new(project, params[:file]) - - if service.execute - ImportIssuesCsvWorker.perform_async(current_user.id, project.id, service.uploader.upload.id) + if uploader = UploadService.new(project, params[:file]).execute + ImportIssuesCsvWorker.perform_async(current_user.id, project.id, uploader.upload.id) flash[:notice] = _("Your issues are being imported. Once finished, you'll get a confirmation email.") else diff --git a/app/services/upload_service.rb b/app/services/upload_service.rb index 47903eb48c3..41ca95b3b6f 100644 --- a/app/services/upload_service.rb +++ b/app/services/upload_service.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class UploadService - attr_accessor :uploader - def initialize(model, file, uploader_class = FileUploader, **uploader_context) @model, @file, @uploader_class, @uploader_context = model, file, uploader_class, uploader_context end @@ -10,10 +8,10 @@ class UploadService def execute return nil unless @file && @file.size <= max_attachment_size - @uploader = @uploader_class.new(@model, nil, @uploader_context) - @uploader.store!(@file) + uploader = @uploader_class.new(@model, nil, @uploader_context) + uploader.store!(@file) - @uploader.to_h + uploader end private diff --git a/lib/api/projects.rb b/lib/api/projects.rb index f5d21d8923f..9f3a1699146 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -475,7 +475,7 @@ module API requires :file, type: File, desc: 'The file to be uploaded' end post ":id/uploads" do - UploadService.new(user_project, params[:file]).execute + UploadService.new(user_project, params[:file]).execute.to_h end desc 'Get the users list of a project' do diff --git a/lib/gitlab/email/attachment_uploader.rb b/lib/gitlab/email/attachment_uploader.rb index a826519b2dd..bffea697f9a 100644 --- a/lib/gitlab/email/attachment_uploader.rb +++ b/lib/gitlab/email/attachment_uploader.rb @@ -23,7 +23,7 @@ module Gitlab content_type: attachment.content_type } - link = UploadService.new(project, file).execute + link = UploadService.new(project, file).execute.to_h attachments << link if link ensure tmp.close! diff --git a/lib/gitlab/import_export/uploads_manager.rb b/lib/gitlab/import_export/uploads_manager.rb index 474e9d45566..e232198150a 100644 --- a/lib/gitlab/import_export/uploads_manager.rb +++ b/lib/gitlab/import_export/uploads_manager.rb @@ -40,7 +40,7 @@ module Gitlab def add_upload(upload) uploader_context = FileUploader.extract_dynamic_path(upload).named_captures.symbolize_keys - UploadService.new(@project, File.open(upload, 'r'), FileUploader, uploader_context).execute + UploadService.new(@project, File.open(upload, 'r'), FileUploader, uploader_context).execute.to_h end def copy_project_uploads diff --git a/spec/services/upload_service_spec.rb b/spec/services/upload_service_spec.rb index 9b232a52efa..57382c4bc8d 100644 --- a/spec/services/upload_service_spec.rb +++ b/spec/services/upload_service_spec.rb @@ -68,6 +68,6 @@ describe UploadService do end def upload_file(project, file) - described_class.new(project, file, FileUploader).execute + described_class.new(project, file, FileUploader).execute.to_h end end From e2698d5d7455d91fa94f9bbf1fc838f8cb142700 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 31 Dec 2018 16:39:43 +0800 Subject: [PATCH 5/6] Improve email messages Also refactored cleanup view to use the same localized string --- app/controllers/concerns/uploads_actions.rb | 4 ++-- app/policies/project_policy.rb | 3 ++- app/services/issues/import_csv_service.rb | 6 +++--- app/views/notify/import_issues_csv_email.html.haml | 10 +++++----- app/views/notify/import_issues_csv_email.text.erb | 10 +++++----- app/views/projects/cleanup/_show.html.haml | 2 +- .../projects/issues/import_csv/_modal.html.haml | 5 ++--- locale/gitlab.pot | 3 --- spec/services/issues/import_csv_service_spec.rb | 14 +++++++------- spec/services/upload_service_spec.rb | 2 +- 10 files changed, 28 insertions(+), 31 deletions(-) diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 5992ad5f1e1..3771c97fdc8 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -7,12 +7,12 @@ module UploadsActions UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze def create - link_to_file = UploadService.new(model, params[:file], uploader_class).execute.to_h + link_to_file = UploadService.new(model, params[:file], uploader_class).execute respond_to do |format| if link_to_file format.json do - render json: { link: link_to_file } + render json: { link: link_to_file.to_h } end else format.json do diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 0026a88b972..8e256b766df 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -222,8 +222,9 @@ class ProjectPolicy < BasePolicy rule { owner | admin | guest | group_member }.prevent :request_access rule { ~request_access_enabled }.prevent :request_access + rule { developer & can?(:create_issue) }.enable :import_issues + rule { can?(:developer_access) }.policy do - enable :import_issues enable :admin_merge_request enable :admin_milestone enable :update_merge_request diff --git a/app/services/issues/import_csv_service.rb b/app/services/issues/import_csv_service.rb index 411000a7066..7fa2ecc3afd 100644 --- a/app/services/issues/import_csv_service.rb +++ b/app/services/issues/import_csv_service.rb @@ -6,7 +6,7 @@ module Issues @user = user @project = project @uploader = upload.build_uploader - @results = { success: 0, errors: [], valid_file: true } + @results = { success: 0, error_lines: [], parse_error: false } end def execute @@ -30,11 +30,11 @@ module Issues if issue.persisted? @results[:success] += 1 else - @results[:errors].push(line_no) + @results[:error_lines].push(line_no) end end rescue ArgumentError, CSV::MalformedCSVError - @results[:valid_file] = false + @results[:parse_error] = true end def email_results_to_user diff --git a/app/views/notify/import_issues_csv_email.html.haml b/app/views/notify/import_issues_csv_email.html.haml index a6a797a5fb7..f30d2b5f078 100644 --- a/app/views/notify/import_issues_csv_email.html.haml +++ b/app/views/notify/import_issues_csv_email.html.haml @@ -7,12 +7,12 @@ has been completed. %p{ style: text_style } - #{pluralize(@results[:success], 'issue')} imported successfully. + #{pluralize(@results[:success], 'issue')} imported. -- if @results[:errors].present? +- if @results[:error_lines].present? %p{ style: text_style } - Errors found on line #{'number'.pluralize(@results[:errors].size)}: #{@results[:errors].join(', ')}. + Errors found on line #{'number'.pluralize(@results[:error_lines].size)}: #{@results[:error_lines].join(', ')}. Please check if these lines have an issue title. -- unless @results[:valid_file] +- if @results[:parse_error] %p{ style: text_style } - Error parsing CSV file. + Error parsing CSV file. Please make sure it has the correct format: a delimited text file that uses a comma to separate values. diff --git a/app/views/notify/import_issues_csv_email.text.erb b/app/views/notify/import_issues_csv_email.text.erb index 54a1762c4ec..1117f90714d 100644 --- a/app/views/notify/import_issues_csv_email.text.erb +++ b/app/views/notify/import_issues_csv_email.text.erb @@ -1,11 +1,11 @@ Your CSV import for project <%= @project.full_name %> (<%= project_url(@project) %>) has been completed. -<%= pluralize(@results[:success], 'issue') %> imported successfully. +<%= pluralize(@results[:success], 'issue') %> imported. -<% if @results[:errors].present? %> -Errors found on line <%= 'number'.pluralize(@results[:errors].size) %>: <%= @results[:errors].join(', ') %>. +<% if @results[:error_lines].present? %> +Errors found on line <%= 'number'.pluralize(@results[:error_lines].size) %>: <%= @results[:error_lines].join(', ') %>. Please check if these lines have an issue title. <% end %> -<% unless @results[:valid_file] %> -Error parsing CSV file. +<% if @results[:parse_error] %> +Error parsing CSV file. Please make sure it has the correct format: a delimited text file that uses a comma to separate values. <% end %> diff --git a/app/views/projects/cleanup/_show.html.haml b/app/views/projects/cleanup/_show.html.haml index cecc139b183..888be4ee282 100644 --- a/app/views/projects/cleanup/_show.html.haml +++ b/app/views/projects/cleanup/_show.html.haml @@ -24,6 +24,6 @@ = _("No file selected") = f.file_field :bfg_object_map, accept: 'text/plain', class: "hidden js-object-map-input", required: true .form-text.text-muted - = _("The maximum file size allowed is %{max_attachment_size}mb") % { max_attachment_size: Gitlab::CurrentSettings.max_attachment_size } + = _("The maximum file size allowed is %{size}.") % { size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes) } = f.submit _('Start cleanup'), class: 'btn btn-success' diff --git a/app/views/projects/issues/import_csv/_modal.html.haml b/app/views/projects/issues/import_csv/_modal.html.haml index 03a6c79ff74..bef34c93e63 100644 --- a/app/views/projects/issues/import_csv/_modal.html.haml +++ b/app/views/projects/issues/import_csv/_modal.html.haml @@ -1,7 +1,7 @@ .issues-import-modal.modal .modal-dialog .modal-content - = form_tag [:import_csv, @project.namespace.becomes(Namespace), @project, :issues], multipart: true do + = form_tag import_csv_namespace_project_issues_path, multipart: true do .modal-header %h3 = _('Import issues') @@ -18,8 +18,7 @@ = file_field_tag :file, accept: 'text/csv', required: true %p.text-secondary = _('It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected.') - %p.text-secondary - = _('The maximum file size allowed is %{size}.') % {size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes)} + = _('The maximum file size allowed is %{size}.') % { size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes) } .modal-footer %button{ type: 'submit', class: 'btn btn-success', title: _('Import issues') } = _('Import issues') diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4a77e09bed9..3be3779d7b6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -6620,9 +6620,6 @@ msgstr "" msgid "The issue stage shows the time it takes from creating an issue to assigning the issue to a milestone, or add the issue to a list on your Issue Board. Begin creating issues to see data for this stage." msgstr "" -msgid "The maximum file size allowed is %{max_attachment_size}mb" -msgstr "" - msgid "The maximum file size allowed is %{size}." msgstr "" diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb index f5e58bc9a9e..848dc2c2cd2 100644 --- a/spec/services/issues/import_csv_service_spec.rb +++ b/spec/services/issues/import_csv_service_spec.rb @@ -21,7 +21,7 @@ describe Issues::ImportCsvService do expect_any_instance_of(Notify).to receive(:import_issues_csv_email) expect(subject[:success]).to eq(0) - expect(subject[:valid_file]).to eq(false) + expect(subject[:parse_error]).to eq(true) end end @@ -32,8 +32,8 @@ describe Issues::ImportCsvService do expect_any_instance_of(Notify).to receive(:import_issues_csv_email) expect(subject[:success]).to eq(3) - expect(subject[:errors]).to eq([]) - expect(subject[:valid_file]).to eq(true) + expect(subject[:error_lines]).to eq([]) + expect(subject[:parse_error]).to eq(false) end end @@ -44,8 +44,8 @@ describe Issues::ImportCsvService do expect_any_instance_of(Notify).to receive(:import_issues_csv_email) expect(subject[:success]).to eq(2) - expect(subject[:errors]).to eq([3]) - expect(subject[:valid_file]).to eq(true) + expect(subject[:error_lines]).to eq([3]) + expect(subject[:parse_error]).to eq(false) end end @@ -56,8 +56,8 @@ describe Issues::ImportCsvService do expect_any_instance_of(Notify).to receive(:import_issues_csv_email) expect(subject[:success]).to eq(3) - expect(subject[:errors]).to eq([4]) - expect(subject[:valid_file]).to eq(true) + expect(subject[:error_lines]).to eq([4]) + expect(subject[:parse_error]).to eq(false) end end end diff --git a/spec/services/upload_service_spec.rb b/spec/services/upload_service_spec.rb index 57382c4bc8d..4a809d5bf18 100644 --- a/spec/services/upload_service_spec.rb +++ b/spec/services/upload_service_spec.rb @@ -63,7 +63,7 @@ describe UploadService do @link_to_file = upload_file(@project, txt) end - it { expect(@link_to_file).to eq(nil) } + it { expect(@link_to_file).to eq({}) } end end From f54290de751e365be0928c66bb75fd106bb7aa88 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Thu, 3 Jan 2019 13:17:07 +0800 Subject: [PATCH 6/6] Remove caching of CSV file Load whole file in memory to simplify code --- app/controllers/concerns/uploads_actions.rb | 6 ++--- app/policies/project_policy.rb | 2 +- app/services/issues/import_csv_service.rb | 24 +++++-------------- .../issues/import_csv/_modal.html.haml | 2 +- .../shared/empty_states/_issues.html.haml | 2 +- app/workers/import_issues_csv_worker.rb | 6 ++++- lib/gitlab/email/attachment_uploader.rb | 4 ++-- spec/mailers/emails/issues_spec.rb | 6 ++--- .../issues/import_csv_service_spec.rb | 2 +- 9 files changed, 23 insertions(+), 31 deletions(-) diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 3771c97fdc8..c114e16edf8 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -7,12 +7,12 @@ module UploadsActions UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze def create - link_to_file = UploadService.new(model, params[:file], uploader_class).execute + uploader = UploadService.new(model, params[:file], uploader_class).execute respond_to do |format| - if link_to_file + if uploader format.json do - render json: { link: link_to_file.to_h } + render json: { link: uploader.to_h } end else format.json do diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 8e256b766df..d70417e710e 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -222,7 +222,7 @@ class ProjectPolicy < BasePolicy rule { owner | admin | guest | group_member }.prevent :request_access rule { ~request_access_enabled }.prevent :request_access - rule { developer & can?(:create_issue) }.enable :import_issues + rule { can?(:developer_access) & can?(:create_issue) }.enable :import_issues rule { can?(:developer_access) }.policy do enable :admin_merge_request diff --git a/app/services/issues/import_csv_service.rb b/app/services/issues/import_csv_service.rb index 7fa2ecc3afd..ef08fafa7cc 100644 --- a/app/services/issues/import_csv_service.rb +++ b/app/services/issues/import_csv_service.rb @@ -2,29 +2,26 @@ module Issues class ImportCsvService - def initialize(user, project, upload) + def initialize(user, project, csv_io) @user = user @project = project - @uploader = upload.build_uploader + @csv_io = csv_io @results = { success: 0, error_lines: [], parse_error: false } end def execute - # Cache remote file locally for processing - @uploader.cache_stored_file! unless @uploader.file_storage? - process_csv email_results_to_user - cleanup_cache unless @uploader.file_storage? - @results end private def process_csv - CSV.foreach(@uploader.file.path, col_sep: detect_col_sep, headers: true).with_index(2) do |row, line_no| + csv_data = @csv_io.open(&:read).force_encoding(Encoding::UTF_8) + + CSV.new(csv_data, col_sep: detect_col_sep(csv_data.lines.first), headers: true).each.with_index(2) do |row, line_no| issue = Issues::CreateService.new(@project, @user, title: row[0], description: row[1]).execute if issue.persisted? @@ -41,9 +38,7 @@ module Issues Notify.import_issues_csv_email(@user.id, @project.id, @results).deliver_now end - def detect_col_sep - header = File.open(@uploader.file.path, &:readline) - + def detect_col_sep(header) if header.include?(",") "," elsif header.include?(";") @@ -54,12 +49,5 @@ module Issues raise CSV::MalformedCSVError end end - - def cleanup_cache - cached_file_path = @uploader.file.cache_path - - File.delete(cached_file_path) - Dir.delete(File.dirname(cached_file_path)) - end end end diff --git a/app/views/projects/issues/import_csv/_modal.html.haml b/app/views/projects/issues/import_csv/_modal.html.haml index bef34c93e63..18768307341 100644 --- a/app/views/projects/issues/import_csv/_modal.html.haml +++ b/app/views/projects/issues/import_csv/_modal.html.haml @@ -15,7 +15,7 @@ .form-group = label_tag :file, _('Upload CSV file'), class: 'label-bold' %div - = file_field_tag :file, accept: 'text/csv', required: true + = file_field_tag :file, accept: '.csv,text/csv', required: true %p.text-secondary = _('It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected.') = _('The maximum file size allowed is %{size}.') % { size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes) } diff --git a/app/views/shared/empty_states/_issues.html.haml b/app/views/shared/empty_states/_issues.html.haml index 76f6a294cb3..0434860dec4 100644 --- a/app/views/shared/empty_states/_issues.html.haml +++ b/app/views/shared/empty_states/_issues.html.haml @@ -1,6 +1,6 @@ - button_path = local_assigns.fetch(:button_path, false) - project_select_button = local_assigns.fetch(:project_select_button, false) -- show_import_button = local_assigns.fetch(:show_import_button, false) +- show_import_button = local_assigns.fetch(:show_import_button, false) && Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, @project) - has_button = button_path || project_select_button .row.empty-state diff --git a/app/workers/import_issues_csv_worker.rb b/app/workers/import_issues_csv_worker.rb index 5d8c02a0605..d44fdfec8ae 100644 --- a/app/workers/import_issues_csv_worker.rb +++ b/app/workers/import_issues_csv_worker.rb @@ -3,12 +3,16 @@ class ImportIssuesCsvWorker include ApplicationWorker + sidekiq_retries_exhausted do |job| + Upload.find(job['args'][2]).destroy + end + def perform(current_user_id, project_id, upload_id) @user = User.find(current_user_id) @project = Project.find(project_id) @upload = Upload.find(upload_id) - importer = Issues::ImportCsvService.new(@user, @project, @upload) + importer = Issues::ImportCsvService.new(@user, @project, @upload.build_uploader) importer.execute @upload.destroy diff --git a/lib/gitlab/email/attachment_uploader.rb b/lib/gitlab/email/attachment_uploader.rb index bffea697f9a..3323ce60158 100644 --- a/lib/gitlab/email/attachment_uploader.rb +++ b/lib/gitlab/email/attachment_uploader.rb @@ -23,8 +23,8 @@ module Gitlab content_type: attachment.content_type } - link = UploadService.new(project, file).execute.to_h - attachments << link if link + uploader = UploadService.new(project, file).execute + attachments << uploader.to_h if uploader ensure tmp.close! end diff --git a/spec/mailers/emails/issues_spec.rb b/spec/mailers/emails/issues_spec.rb index 68c87803b78..09253cf8003 100644 --- a/spec/mailers/emails/issues_spec.rb +++ b/spec/mailers/emails/issues_spec.rb @@ -13,19 +13,19 @@ describe Emails::Issues do subject { Notify.import_issues_csv_email(user.id, project.id, @results) } it "shows number of successful issues imported" do - @results = { success: 165, errors: [], valid_file: true } + @results = { success: 165, error_lines: [], parse_error: false } expect(subject).to have_body_text "165 issues imported" end it "shows error when file is invalid" do - @results = { success: 0, errors: [], valid_file: false } + @results = { success: 0, error_lines: [], parse_error: true } expect(subject).to have_body_text "Error parsing CSV" end it "shows line numbers with errors" do - @results = { success: 0, errors: [23, 34, 58], valid_file: false } + @results = { success: 0, error_lines: [23, 34, 58], parse_error: false } expect(subject).to have_body_text "23, 34, 58" end diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb index 848dc2c2cd2..516a1137319 100644 --- a/spec/services/issues/import_csv_service_spec.rb +++ b/spec/services/issues/import_csv_service_spec.rb @@ -10,7 +10,7 @@ describe Issues::ImportCsvService do uploader = FileUploader.new(project) uploader.store!(file) - described_class.new(user, project, uploader.upload).execute + described_class.new(user, project, uploader).execute end describe '#execute' do