Merge branch '3619-issuable_export_csv_worker-2' into 'master'

Refactor ExportCsvWorker to IssuableExportCsvWorker

See merge request gitlab-org/gitlab!42602

app/controllers/projects/issues_controller.rb

@@ -217,7 +217,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def export_csv
-    ExportCsvWorker.perform_async(current_user.id, project.id, finder_options.to_h) # rubocop:disable CodeReuse/Worker
+    IssuableExportCsvWorker.perform_async(:issue, current_user.id, project.id, finder_options.to_h) # rubocop:disable CodeReuse/Worker
 
     index_path = project_issues_path(project)
     message = _('Your CSV export has started. It will be emailed to %{email} when complete.') % { email: current_user.notification_email }

app/mailers/emails/issues.rb

@@ -93,7 +93,7 @@ module Emails
 
     def issues_csv_email(user, project, csv_data, export_status)
       @project = project
-      @issues_count = export_status.fetch(:rows_expected)
+      @count = export_status.fetch(:rows_expected)
       @written_count = export_status.fetch(:rows_written)
       @truncated = export_status.fetch(:truncated)
 

app/mailers/emails/merge_requests.rb

@@ -110,6 +110,20 @@ module Emails
       mail_answer_thread(@merge_request, merge_request_thread_options(mwps_set_by_user_id, recipient_id, reason))
     end
 
+    def merge_requests_csv_email(user, project, csv_data, export_status)
+      @project = project
+      @count = export_status.fetch(:rows_expected)
+      @written_count = export_status.fetch(:rows_written)
+      @truncated = export_status.fetch(:truncated)
+
+      filename = "#{project.full_path.parameterize}_merge_requests_#{Date.current.iso8601}.csv"
+      attachments[filename] = { content: csv_data, mime_type: 'text/csv' }
+      mail(to: user.notification_email_for(@project.group), subject: subject("Exported merge requests")) do |format|
+        format.html { render layout: 'mailer' }
+        format.text { render layout: 'mailer' }
+      end
+    end
+
     private
 
     def setup_merge_request_mail(merge_request_id, recipient_id, present: false)

app/services/merge_requests/export_csv_service.rb

@@ -8,7 +8,8 @@ module MergeRequests
     # Target attachment size before base64 encoding
     TARGET_FILESIZE = 15.megabytes
 
-    def initialize(merge_requests)
+    def initialize(merge_requests, project)
+      @project = project
       @merge_requests = merge_requests
     end
 
@@ -16,6 +17,10 @@ module MergeRequests
       csv_builder.render(TARGET_FILESIZE)
     end
 
+    def email(user)
+      Notify.merge_requests_csv_email(user, @project, csv_data, csv_builder.status).deliver_now
+    end
+
     private
 
     def csv_builder

app/views/notify/_issuable_csv_export.html.haml

+%p{ style: 'font-size:18px; text-align:center;  line-height:30px;' }
+  - project_link = link_to(@project.full_name, project_url(@project), style: "color:#3777b0; text-decoration:none; display:block;")
+  = _('Your CSV export of %{count} from project %{project_link} has been added to this email as an attachment.').html_safe % { count: pluralize(@written_count, type.to_s), project_link: project_link }
+  - if @truncated
+    %p
+      = _('This attachment has been truncated to avoid exceeding the maximum allowed attachment size of 15MB. %{written_count} of %{count} issues have been included. Consider re-exporting with a narrower selection of issues.') % { written_count: @written_count, count: @count }

app/views/notify/issues_csv_email.html.haml

-%p{ style: 'font-size:18px; text-align:center;  line-height:30px;' }
-  - project_link = link_to(@project.full_name, project_url(@project), style: "color:#3777b0; text-decoration:none; display:block;")
-  = _('Your CSV export of %{issues_count} from project %{project_link} has been added to this email as an attachment.').html_safe % { issues_count: pluralize(@written_count, 'issue'), project_link: project_link }
-  - if @truncated
-    %p
-      = _('This attachment has been truncated to avoid exceeding the maximum allowed attachment size of 15MB. %{written_count} of %{issues_count} issues have been included. Consider re-exporting with a narrower selection of issues.') % { written_count: @written_count, issues_count: @issues_count }
+= render 'issuable_csv_export', type: :issue

app/views/notify/merge_requests_csv_email.html.haml

+= render 'issuable_csv_export', type: :merge_request

app/views/notify/merge_requests_csv_email.text.erb

+<%= _('Your CSV export of %{written_count} from project %{project_name} (%{project_url}) has been added to this email as an attachment.') %  { written_count: pluralize(@written_count, 'merge request'), project_name: @project.full_name, project_url: project_url(@project) } %>
+
+<% if @truncated %>
+  <%= _('This attachment has been truncated to avoid exceeding the maximum allowed attachment size of 15MB. %{written_count} of %{merge_requests_count} issues have been included. Consider re-exporting with a narrower selection of issues.') % { written_count: @written_count, merge_requests_count: @merge_requests_count} %>
+<% end %>

app/workers/all_queues.yml

@@ -1571,6 +1571,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: issuable_export_csv
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :cpu
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: issue_placement
   :feature_category: :issue_tracking
   :has_external_dependencies:

app/workers/export_csv_worker.rb

@@ -15,8 +15,6 @@ class ExportCsvWorker # rubocop:disable Scalability/IdempotentWorker
     params[:project_id] = project_id
     params.delete(:sort)
 
-    issues = IssuesFinder.new(@current_user, params).execute
-
-    Issues::ExportCsvService.new(issues, @project).email(@current_user)
+    IssuableExportCsvWorker.perform_async(:issue, @current_user.id, @project.id, params)
   end
 end

app/workers/issuable_export_csv_worker.rb

+# frozen_string_literal: true
+
+class IssuableExportCsvWorker # rubocop:disable Scalability/IdempotentWorker
+  include ApplicationWorker
+
+  feature_category :issue_tracking
+  worker_resource_boundary :cpu
+  loggable_arguments 2
+
+  PERMITTED_TYPES = [:merge_request, :issue].freeze
+
+  def perform(type, current_user_id, project_id, params)
+    @type = type.to_sym
+    check_permitted_type!
+    process_params!(params, project_id)
+
+    @current_user = User.find(current_user_id)
+    @project = Project.find(project_id)
+    @service = service(find_objects(params))
+
+    @service.email(@current_user)
+  end
+
+  private
+
+  def find_objects(params)
+    case @type
+    when :issue
+      IssuesFinder.new(@current_user, params).execute
+    when :merge_request
+      MergeRequestsFinder.new(@current_user, params).execute
+    end
+  end
+
+  def service(issuables)
+    case @type
+    when :issue
+      Issues::ExportCsvService.new(issuables, @project)
+    when :merge_request
+      MergeRequests::ExportCsvService.new(issuables, @project)
+    end
+  end
+
+  def process_params!(params, project_id)
+    params.symbolize_keys!
+    params[:project_id] = project_id
+    params.delete(:sort)
+  end
+
+  def check_permitted_type!
+    raise ArgumentError, "type parameter must be :issue or :merge_request, it was #{@type}" unless PERMITTED_TYPES.include?(@type)
+  end
+end

config/sidekiq_queues.yml

@@ -152,6 +152,8 @@
   - 2
 - - irker
   - 1
+- - issuable_export_csv
+  - 1
 - - issue_placement
   - 2
 - - issue_rebalancing

locale/gitlab.pot

@@ -26720,9 +26720,15 @@ msgstr ""
 msgid "This application will be able to:"
 msgstr ""
 
+msgid "This attachment has been truncated to avoid exceeding the maximum allowed attachment size of 15MB. %{written_count} of %{count} issues have been included. Consider re-exporting with a narrower selection of issues."
+msgstr ""
+
 msgid "This attachment has been truncated to avoid exceeding the maximum allowed attachment size of 15MB. %{written_count} of %{issues_count} issues have been included. Consider re-exporting with a narrower selection of issues."
 msgstr ""
 
+msgid "This attachment has been truncated to avoid exceeding the maximum allowed attachment size of 15MB. %{written_count} of %{merge_requests_count} issues have been included. Consider re-exporting with a narrower selection of issues."
+msgstr ""
+
 msgid "This block is self-referential"
 msgstr ""
 
@@ -30369,7 +30375,7 @@ msgstr ""
 msgid "Your CSV export has started. It will be emailed to %{email} when complete."
 msgstr ""
 
-msgid "Your CSV export of %{issues_count} from project %{project_link} has been added to this email as an attachment."
+msgid "Your CSV export of %{count} from project %{project_link} has been added to this email as an attachment."
 msgstr ""
 
 msgid "Your CSV export of %{written_count} from project %{project_name} (%{project_url}) has been added to this email as an attachment."

spec/controllers/projects/issues_controller_spec.rb

@@ -1642,7 +1642,7 @@ RSpec.describe Projects::IssuesController do
       end
 
       it 'allows CSV export' do
-        expect(ExportCsvWorker).to receive(:perform_async).with(viewer.id, project.id, anything)
+        expect(IssuableExportCsvWorker).to receive(:perform_async).with(:issue, viewer.id, project.id, anything)
 
         request_csv
 
@@ -1657,7 +1657,7 @@ RSpec.describe Projects::IssuesController do
       it 'redirects to the sign in page' do
         request_csv
 
-        expect(ExportCsvWorker).not_to receive(:perform_async)
+        expect(IssuableExportCsvWorker).not_to receive(:perform_async)
         expect(response).to redirect_to(new_user_session_path)
       end
     end

spec/features/issues/csv_spec.rb

@@ -31,13 +31,13 @@ RSpec.describe 'Issues csv' do
   end
 
   it 'triggers an email export' do
-    expect(ExportCsvWorker).to receive(:perform_async).with(user.id, project.id, hash_including("project_id" => project.id))
+    expect(IssuableExportCsvWorker).to receive(:perform_async).with(:issue, user.id, project.id, hash_including("project_id" => project.id))
 
     request_csv
   end
 
   it "doesn't send request params to ExportCsvWorker" do
-    expect(ExportCsvWorker).to receive(:perform_async).with(anything, anything, hash_excluding("controller" => anything, "action" => anything))
+    expect(IssuableExportCsvWorker).to receive(:perform_async).with(:issue, anything, anything, hash_excluding("controller" => anything, "action" => anything))
 
     request_csv
   end

spec/mailers/emails/merge_requests_spec.rb

@@ -33,4 +33,37 @@ RSpec.describe Emails::MergeRequests do
       expect(subject).to have_content current_user.name
     end
   end
+
+  describe '#merge_requests_csv_email' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project) }
+    let(:merge_requests) { create_list(:merge_request, 10) }
+    let(:export_status) do
+      {
+        rows_expected: 10,
+        rows_written: 10,
+        truncated: false
+      }
+    end
+
+    let(:csv_data) { MergeRequests::ExportCsvService.new(MergeRequest.all, project).csv_data }
+
+    subject { Notify.merge_requests_csv_email(user, project, csv_data, export_status) }
+
+    it { expect(subject.subject).to eq("#{project.name} | Exported merge requests") }
+    it { expect(subject.to).to contain_exactly(user.notification_email_for(project.group)) }
+    it { expect(subject).to have_content('Your CSV export of 10 merge requests from project')}
+
+    context 'when truncated' do
+      let(:export_status) do
+        {
+            rows_expected: 10,
+            rows_written: 10,
+            truncated: true
+        }
+      end
+
+      it { expect(subject).to have_content('This attachment has been truncated to avoid exceeding the maximum allowed attachment size of 15MB.') }
+    end
+  end
 end

spec/services/merge_requests/export_csv_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe MergeRequests::ExportCsvService do
   let_it_be(:merge_request) { create(:merge_request) }
   let(:csv) { CSV.parse(subject.csv_data, headers: true).first }
 
-  subject { described_class.new(MergeRequest.where(id: merge_request.id)) }
+  subject { described_class.new(MergeRequest.where(id: merge_request.id), merge_request.project) }
 
   describe 'csv_data' do
     it 'contains the correct information', :aggregate_failures do

spec/workers/export_csv_worker_spec.rb

@@ -10,25 +10,9 @@ RSpec.describe ExportCsvWorker do
     described_class.new.perform(user.id, project.id, params)
   end
 
-  it 'emails a CSV' do
-    expect {perform}.to change(ActionMailer::Base.deliveries, :size).by(1)
-  end
-
-  it 'ensures that project_id is passed to issues_finder' do
-    expect(IssuesFinder).to receive(:new).with(anything, hash_including(project_id: project.id)).and_call_original
+  it 'delegates call to IssuableExportCsvWorker' do
+    expect(IssuableExportCsvWorker).to receive(:perform_async).with(:issue, user.id, project.id, anything)
 
     perform
   end
-
-  it 'removes sort parameter' do
-    expect(IssuesFinder).to receive(:new).with(anything, hash_not_including(:sort)).and_call_original
-
-    perform
-  end
-
-  it 'converts controller string keys to symbol keys for IssuesFinder' do
-    expect(IssuesFinder).to receive(:new).with(anything, hash_including(test_key: true)).and_call_original
-
-    perform('test_key' => true)
-  end
 end

spec/workers/issuable_export_csv_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IssuableExportCsvWorker do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, creator: user) }
+  let(:params) { {} }
+
+  subject { described_class.new.perform(issuable_type, user.id, project.id, params) }
+
+  context 'when issuable type is Issue' do
+    let(:issuable_type) { :issue }
+
+    it 'emails a CSV' do
+      expect { subject }.to change(ActionMailer::Base.deliveries, :size).by(1)
+    end
+
+    it 'ensures that project_id is passed to issues_finder' do
+      expect(IssuesFinder).to receive(:new).with(anything, hash_including(project_id: project.id)).and_call_original
+
+      subject
+    end
+
+    it 'removes sort parameter' do
+      expect(IssuesFinder).to receive(:new).with(anything, hash_not_including(:sort)).and_call_original
+
+      subject
+    end
+
+    it 'calls the issue export service' do
+      expect(Issues::ExportCsvService).to receive(:new).once.and_call_original
+
+      subject
+    end
+
+    context 'with params' do
+      let(:params) { { 'test_key' => true } }
+
+      it 'converts controller string keys to symbol keys for IssuesFinder' do
+        expect(IssuesFinder).to receive(:new).with(user, hash_including(test_key: true)).and_call_original
+
+        subject
+      end
+    end
+  end
+
+  context 'when issuable type is MergeRequest' do
+    let(:issuable_type) { :merge_request }
+
+    it 'emails a CSV' do
+      expect { subject }.to change(ActionMailer::Base.deliveries, :size).by(1)
+    end
+
+    it 'calls the MR export service' do
+      expect(MergeRequests::ExportCsvService).to receive(:new).with(anything, project).once.and_call_original
+
+      subject
+    end
+
+    it 'calls the MergeRequest finder' do
+      expect(MergeRequestsFinder).to receive(:new).once.and_call_original
+
+      subject
+    end
+  end
+
+  context 'when issuable type is User' do
+    let(:issuable_type) { :user }
+
+    it { expect { subject }.to raise_error(ArgumentError) }
+  end
+end