Have the publish attachments service not inherit

The attachment service does not need everything from the
base service so this removes the inheritance
Make total_uploads and instance variable
add comment to publish service

ee/app/services/status_page/publication_service_helpers.rb

+# frozen_string_literal: true
+
+module StatusPage
+  module PublicationServiceHelpers
+    include Gitlab::Utils::StrongMemoize
+
+    def error(message, payload = {})
+      ServiceResponse.error(message: message, payload: payload)
+    end
+
+    def success(payload = {})
+      ServiceResponse.success(payload: payload)
+    end
+  end
+end

ee/app/services/status_page/publish_attachments_service.rb

@@ -2,62 +2,75 @@
 
 module StatusPage
   # Publishes Attachments from incident comments and descriptions to s3
-  # Should only be called from publish details
-  class PublishAttachmentsService < PublishBaseService
-    private
+  # Should only be called from publish details or a service that inherits from the publish_base_service
+  class PublishAttachmentsService
+    include StatusPage::PublicationServiceHelpers
+
+    def initialize(project:, issue:, user_notes:, storage_client:)
+      @project = project
+      @issue = issue
+      @user_notes = user_notes
+      @storage_client = storage_client
+      @total_uploads = existing_keys.size
+    end
 
-    def process(issue, user_notes)
-      total_uploads = existing_keys(issue).size
-      publish_description_attachments(issue, total_uploads)
-      publish_user_note_attachments(issue, total_uploads, user_notes)
+    def execute
+      publish_description_attachments
+      publish_user_note_attachments
 
       success
     end
 
-    def publish_description_attachments(issue, total_uploads)
+    private
+
+    attr_reader :project, :issue, :user_notes, :storage_client
+
+    def publish_description_attachments
       publish_markdown_uploads(
         markdown_field: issue.description,
-        issue_iid: issue.iid,
-        total_uploads: total_uploads
+        issue_iid: issue.iid
       )
     end
 
-    def publish_user_note_attachments(issue, total_uploads, user_notes)
+    def publish_user_note_attachments
       user_notes.each do |user_note|
         publish_markdown_uploads(
           markdown_field: user_note.note,
-          issue_iid: issue.iid,
-          total_uploads: total_uploads
+          issue_iid: issue.iid
         )
       end
     end
 
-    def existing_keys(issue = nil)
-      strong_memoize(:existing_keys) do
-        storage_client.list_object_keys(
-          uploads_path(issue)
-        )
-      end
-    end
-
-    def uploads_path(issue)
-      StatusPage::Storage.uploads_path(issue.iid)
-    end
-
-    def publish_markdown_uploads(markdown_field:, issue_iid:, total_uploads:)
+    def publish_markdown_uploads(markdown_field:, issue_iid:)
       markdown_field.scan(FileUploader::MARKDOWN_PATTERN).map do |secret, file_name|
-        break if total_uploads >= StatusPage::Storage::MAX_UPLOADS
+        break if @total_uploads >= StatusPage::Storage::MAX_UPLOADS
 
         key = StatusPage::Storage.upload_path(issue_iid, secret, file_name)
-
         next if existing_keys.include? key
 
-        uploader = UploaderFinder.new(@project, secret, file_name).execute
-        uploader.open do |open_file|
-          storage_client.multipart_upload(key, open_file)
-          total_uploads += 1
-        end
+        # uploader behaves like a file with an 'open' method
+        file = UploaderFinder.new(project, secret, file_name).execute
+
+        upload_file(key, file)
       end
     end
+
+    def upload_file(key, file)
+      file.open do |open_file|
+        # Send files to s3 storage in parts (hanles large files)
+        storage_client.multipart_upload(key, open_file)
+        @total_uploads += 1
+      end
+    end
+
+    def existing_keys
+      strong_memoize(:existing_keys) do
+        storage_client.list_object_keys(uploads_path)
+      end
+    end
+
+    def uploads_path
+      StatusPage::Storage.uploads_path(issue.iid)
+    end
   end
 end

ee/app/services/status_page/publish_base_service.rb

@@ -2,7 +2,7 @@
 
 module StatusPage
   class PublishBaseService
-    include Gitlab::Utils::StrongMemoize
+    include StatusPage::PublicationServiceHelpers
 
     def initialize(project:)
       @project = project
@@ -71,10 +71,6 @@ module StatusPage
         .valid?
     end
 
-    def error(message, payload = {})
-      ServiceResponse.error(message: message, payload: payload)
-    end
-
     def error_limit_exceeded(key)
       error("Failed to upload #{key}: Limit exceeded")
     end
@@ -86,9 +82,5 @@ module StatusPage
     def error_no_storage_client
       error('No storage client available. Is the status page setting activated?')
     end
-
-    def success(payload = {})
-      ServiceResponse.success(payload: payload)
-    end
   end
 end

ee/app/services/status_page/publish_details_service.rb

@@ -39,7 +39,12 @@ module StatusPage
     end
 
     def publish_attachments(issue, user_notes)
-      StatusPage::PublishAttachmentsService.new(project: @project).execute(issue, user_notes)
+      StatusPage::PublishAttachmentsService.new(
+        project: @project,
+        issue: issue,
+        user_notes: user_notes,
+        storage_client: storage_client
+      ).execute
     end
   end
 end

ee/app/services/status_page/publish_service.rb

@@ -6,6 +6,7 @@ module StatusPage
   #
   # Use this service for publishing an incident to CDN synchronously.
   # To publish asynchronously use +StatusPage::TriggerPublishService+ instead.
+  # Called within an async job so as to run out of out of band from web requests
   #
   # This services calls:
   # * StatusPage::PublishDetailsService

ee/lib/status_page/storage/s3_client.rb

@@ -70,7 +70,7 @@ module StatusPage
       # key: s3 key at which file is stored
       # file: An open file or file-like io object
       def multipart_upload(key, file)
-        StatusPage::Storage::S3Client.new(
+        StatusPage::Storage::S3MultipartUpload.new(
           client: client, bucket_name: bucket_name, key: key, open_file: file
         ).call
       end

ee/lib/status_page/storage/s3_multipart_upload.rb

@@ -30,6 +30,7 @@ module StatusPage
             complete_upload(upload_id, parts)
             # Rescue on Exception since even on keyboard inturrupt we want to abort the upload and re-raise
             # abort clears the already uploaded parts so that they do not cost the bucket owner
+            # The status page bucket lifecycle policy will clear out any parts if this fails without an exception (power failures etc.)
           rescue Exception => e # rubocop:disable Lint/RescueException
             abort_upload(upload_id)
             raise e

ee/spec/services/status_page/publish_attachments_service_spec.rb

@@ -3,33 +3,33 @@
 require 'spec_helper'
 
 describe StatusPage::PublishAttachmentsService do
-  let_it_be(:project, refind: true) { create(:project) }
-  let(:markdown_field) { 'Hello World' }
-  let(:user_notes) { [] }
-  let(:incident_id) { 1 }
-  let(:issue) { instance_double(Issue, notes: user_notes, description: markdown_field, iid: incident_id) }
-  let(:key) { StatusPage::Storage.details_path(incident_id) }
-  let(:content) { { id: incident_id } }
-  let(:storage_client) { instance_double(StatusPage::Storage::S3Client) }
-  let(:status_page_setting_enabled) { true }
-  let(:status_page_setting) do
-    instance_double(StatusPage::ProjectSetting, enabled?: status_page_setting_enabled,
-                    storage_client: storage_client)
+  RSpec.shared_context 'second file' do
+    # Setup second file
+    let(:upload_secret_2) { '9cb61a79ce884d5b681dd42728d3c159' }
+    let(:image_file_name_2) { 'tanuki_2.png' }
+    let(:upload_path_2) { "/uploads/#{upload_secret_2}/#{image_file_name_2}" }
+    let(:markdown_field) { "![tanuki](#{upload_path}) and ![tanuki_2](#{upload_path_2})" }
+    let(:status_page_upload_path_2) { StatusPage::Storage.upload_path(issue.iid, upload_secret_2, image_file_name_2) }
   end
 
-  let(:service) { described_class.new(project: project) }
+  describe '#execute' do
+    let_it_be(:project, refind: true) { create(:project) }
+    let(:markdown_field) { 'Hello World' }
+    let(:user_notes) { [] }
+    let(:incident_id) { 1 }
+    let(:issue) { instance_double(Issue, notes: user_notes, description: markdown_field, iid: incident_id) }
+    let(:key) { StatusPage::Storage.details_path(incident_id) }
+    let(:content) { { id: incident_id } }
+    let(:storage_client) { instance_double(StatusPage::Storage::S3Client) }
 
-  subject(:result) { service.execute(issue, user_notes) }
+    let(:service) { described_class.new(project: project, issue: issue, user_notes: user_notes, storage_client: storage_client) }
 
-  before do
-    allow(project).to receive(:status_page_setting)
-      .and_return(status_page_setting)
-  end
+    subject { service.execute }
+
+    include_context 'stub status page enabled'
 
-  describe '#execute' do
     context 'publishes file attachments' do
       before do
-        stub_licensed_features(status_page: true)
         allow(storage_client).to receive(:upload_object).with("data/incident/1.json", "{\"id\":1}")
         allow(storage_client).to receive(:list_object_keys).and_return(Set.new)
       end
@@ -37,8 +37,8 @@ describe StatusPage::PublishAttachmentsService do
       context 'when not in markdown' do
         it 'publishes no images' do
           expect(storage_client).not_to receive(:multipart_upload)
-          expect(result.payload).to eq({})
-          expect(result).to be_success
+          expect(subject.payload).to eq({})
+          expect(subject).to be_success
         end
       end
 
@@ -67,8 +67,8 @@ describe StatusPage::PublishAttachmentsService do
         it 'publishes description images' do
           expect(storage_client).to receive(:multipart_upload).with(status_page_upload_path, open_file).once
 
-          expect(result).to be_success
-          expect(result.payload).to eq({})
+          expect(subject).to be_success
+          expect(subject.payload).to eq({})
         end
 
         context 'user notes uploads' do
@@ -79,22 +79,24 @@ describe StatusPage::PublishAttachmentsService do
           it 'publishes images' do
             expect(storage_client).to receive(:multipart_upload).with(status_page_upload_path, open_file).once
 
-            expect(result).to be_success
-            expect(result.payload).to eq({})
+            expect(subject).to be_success
+            expect(subject.payload).to eq({})
           end
         end
 
         context 'when exceeds upload limit' do
+          include_context 'second file'
+
           before do
-            stub_const("StatusPage::Storage::MAX_UPLOADS", 1)
-            allow(storage_client).to receive(:list_object_keys).and_return(Set[status_page_upload_path])
+            stub_const("StatusPage::Storage::MAX_UPLOADS", 2)
+            allow(storage_client).to receive(:list_object_keys).and_return(Set['existing_key'])
           end
 
           it 'publishes no images' do
-            expect(storage_client).not_to receive(:multipart_upload)
+            expect(storage_client).to receive(:multipart_upload).once
 
-            expect(result).to be_success
-            expect(result.payload).to eq({})
+            expect(subject).to be_success
+            expect(subject.payload).to eq({})
           end
         end
 
@@ -106,17 +108,13 @@ describe StatusPage::PublishAttachmentsService do
           it 'publishes no images' do
             expect(storage_client).not_to receive(:multipart_upload)
 
-            expect(result).to be_success
-            expect(result.payload).to eq({})
+            expect(subject).to be_success
+            expect(subject.payload).to eq({})
           end
         end
 
         context 'when images are already in s3' do
-          let(:upload_secret_2) { '9cb61a79ce884d5b681dd42728d3c159' }
-          let(:image_file_name_2) { 'tanuki_2.png' }
-          let(:upload_path_2) { "/uploads/#{upload_secret_2}/#{image_file_name_2}" }
-          let(:markdown_field) { "![tanuki](#{upload_path}) and ![tanuki_2](#{upload_path_2})" }
-          let(:status_page_upload_path_2) { StatusPage::Storage.upload_path(issue.iid, upload_secret_2, image_file_name_2) }
+          include_context 'second file'
 
           before do
             allow(storage_client).to receive(:list_object_keys).and_return(Set[status_page_upload_path])
@@ -126,8 +124,8 @@ describe StatusPage::PublishAttachmentsService do
             expect(storage_client).to receive(:multipart_upload).with(status_page_upload_path_2, open_file).once
             expect(storage_client).not_to receive(:multipart_upload).with(status_page_upload_path, open_file)
 
-            expect(result).to be_success
-            expect(result.payload).to eq({})
+            expect(subject).to be_success
+            expect(subject.payload).to eq({})
           end
         end
       end

ee/spec/services/status_page/publish_details_service_spec.rb

@@ -4,10 +4,9 @@ require 'spec_helper'
 
 describe StatusPage::PublishDetailsService do
   let_it_be(:project, refind: true) { create(:project) }
-  let(:markdown_field) { 'Hello World' }
   let(:user_notes) { [] }
   let(:incident_id) { 1 }
-  let(:issue) { instance_double(Issue, notes: user_notes, description: markdown_field, iid: incident_id) }
+  let(:issue) { instance_double(Issue, notes: user_notes, description: 'Incident Occuring', iid: incident_id) }
   let(:key) { StatusPage::Storage.details_path(incident_id) }
   let(:content) { { id: incident_id } }
 
@@ -35,9 +34,10 @@ describe StatusPage::PublishDetailsService do
     context 'publishes attachments' do
       it 'sends attachements to s3' do
         allow(storage_client).to receive(:upload_object)
+        allow(storage_client).to receive(:list_object_keys).and_return([])
 
         expect_next_instance_of(StatusPage::PublishAttachmentsService) do |service|
-          expect(service).to receive(:execute).with(issue, user_notes)
+          expect(service).to receive(:execute)
         end
 
         subject

ee/spec/support/shared_contexts/status_page/status_page_enabled_context.rb

@@ -12,3 +12,20 @@ RSpec.shared_context 'status page enabled' do
     end
   end
 end
+
+RSpec.shared_context 'stub status page enabled' do
+  let(:status_page_setting_enabled) { true }
+  let(:status_page_setting) do
+    instance_double(
+      StatusPage::ProjectSetting,
+      enabled?: status_page_setting_enabled,
+      storage_client: storage_client
+    )
+  end
+
+  before do
+    stub_licensed_features(status_page: true)
+    allow(project).to receive(:status_page_setting)
+    .and_return(status_page_setting)
+  end
+end