From e2698d5d7455d91fa94f9bbf1fc838f8cb142700 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 31 Dec 2018 16:39:43 +0800 Subject: 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 +- app/views/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 -- cgit v1.2.1