Fix broken specs

Add 5000 file attachment limit to the publish
details code and fix broken specs.
Add limit to the docs.
Fix publish service spec.
Mock calls to other services in the publish spec.

doc/user/project/status_page/index.md

@@ -114,4 +114,8 @@ To change the incident status from `open` to `closed`, close the incident issue
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/205166) in GitLab 13.0.
 
-Starting with GitLab 13.0, any file that is uploaded to incident issue descriptions or comments will be published and unpublished to the status page s3 bucket, as part of the publication flow described in the [How it works](#how-it-works) section.
+Starting with GitLab 13.0, any file that is attached to incident issue descriptions or comments will be published and unpublished to the status page storage, as part of the publication flow described in the [How it works](#how-it-works) section.
+
+### Limit
+
+Only 5000 attachments per issue will be transferred to the status page.

ee/app/services/status_page/publish_details_service.rb

@@ -14,11 +14,9 @@ module StatusPage
       publish_json_response = publish_json(issue, user_notes)
       return publish_json_response if publish_json_response.error?
 
-      image_object_keys = publish_images(issue, user_notes)
+      publish_images(issue, user_notes)
 
-      success_payload = publish_json_response.payload.merge({ image_object_keys: image_object_keys })
-
-      success(success_payload)
+      success
     end
 
     # Publish Json
@@ -46,11 +44,13 @@ module StatusPage
     def publish_images(issue, user_notes)
       existing_image_keys = storage_client.list_object_keys(StatusPage::Storage.uploads_path(issue.iid))
       # Send all description images to s3
+      total_uploads = existing_image_keys.size
 
       publish_markdown_uploads(
         markdown_field: issue.description,
         issue_iid: issue.iid,
-        existing_image_keys: existing_image_keys
+        existing_image_keys: existing_image_keys,
+        total_uploads: total_uploads
       )
 
       # Send all comment images to s3
@@ -58,13 +58,16 @@ module StatusPage
         publish_markdown_uploads(
           markdown_field: user_note.note,
           issue_iid: issue.iid,
-          existing_image_keys: existing_image_keys
+          existing_image_keys: existing_image_keys,
+          total_uploads: total_uploads
         )
       end
     end
 
-    def publish_markdown_uploads(markdown_field:, issue_iid:, existing_image_keys:)
+    def publish_markdown_uploads(markdown_field:, issue_iid:, existing_image_keys:, total_uploads:)
       markdown_field.scan(FileUploader::MARKDOWN_PATTERN).map do |secret, file_name|
+        break if total_uploads >= StatusPage::Storage::MAX_IMAGE_UPLOADS
+
         key = StatusPage::Storage.upload_path(issue_iid, secret, file_name)
 
         next if existing_image_keys.include? key
@@ -72,6 +75,7 @@ module StatusPage
         uploader = UploaderFinder.new(@project, secret, file_name).execute
         uploader.open do |open_file|
           storage_client.multipart_upload(key, open_file)
+          total_uploads += 1
         end
       end
     end

ee/app/services/status_page/publish_list_service.rb

@@ -13,7 +13,7 @@ module StatusPage
     def process(issues)
       json = serialize(issues)
 
-      upload(object_key, json)
+      upload_json(object_key, json)
     end
 
     def serialize(issues)

ee/spec/services/status_page/publish_details_service_spec.rb

@@ -31,5 +31,110 @@ describe StatusPage::PublishDetailsService do
         expect(result.message).to eq('Missing object key')
       end
     end
+
+    context 'publishes image uploads' do
+      before do
+        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
+
+      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
+        end
+      end
+
+      context 'when in markdown' do
+        let(:upload_secret) { '734b8524a16d44eb0ff28a2c2e4ff3c0' }
+        let(:image_file_name) { 'tanuki.png'}
+        let(:upload_path) { "/uploads/#{upload_secret}/#{image_file_name}" }
+        let(:markdown_field) { "![tanuki](#{upload_path})" }
+        let(:status_page_upload_path) { StatusPage::Storage.upload_path(issue.iid, upload_secret, image_file_name) }
+        let(:user_notes) { [] }
+
+        let(:open_file) { instance_double(File, read: 'stubbed read') }
+        let(:uploader) { instance_double(FileUploader) }
+
+        before do
+          allow(uploader).to receive(:open).and_yield(open_file).twice
+
+          allow_next_instance_of(UploaderFinder) do |finder|
+            allow(finder).to receive(:execute).and_return(uploader)
+          end
+
+          allow(storage_client).to receive(:list_object_keys).and_return(Set[])
+          allow(storage_client).to receive(:upload_object)
+        end
+
+        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({})
+        end
+
+        context 'user notes uploads' do
+          let(:user_note) { instance_double(Note, note: markdown_field) }
+          let(:user_notes) { [user_note] }
+          let(:issue) { instance_double(Issue, notes: user_notes, description: '', iid: incident_id) }
+
+          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({})
+          end
+        end
+
+        context 'when exceeds upload limit' do
+          before do
+            stub_const("StatusPage::Storage::MAX_IMAGE_UPLOADS", 1)
+            allow(storage_client).to receive(:list_object_keys).and_return(Set[status_page_upload_path])
+          end
+
+          it 'publishes no images' do
+            expect(storage_client).not_to receive(:multipart_upload)
+
+            expect(result).to be_success
+            expect(result.payload).to eq({})
+          end
+        end
+
+        context 'when all images are in s3' do
+          before do
+            allow(storage_client).to receive(:list_object_keys).and_return(Set[status_page_upload_path])
+          end
+
+          it 'publishes no images' do
+            expect(storage_client).not_to receive(:multipart_upload)
+
+            expect(result).to be_success
+            expect(result.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) }
+
+          before do
+            allow(storage_client).to receive(:list_object_keys).and_return(Set[status_page_upload_path])
+          end
+
+          it 'publishes only new images' 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({})
+          end
+        end
+      end
+    end
   end
 end

ee/spec/services/status_page/publish_service_spec.rb

@@ -26,8 +26,8 @@ describe StatusPage::PublishService do
     describe 'publish details' do
       context 'when upload succeeds' do
         it 'uploads incident details and list' do
-          expect_to_upload_details(issue)
-          expect_to_upload_list
+          stub_service_called(StatusPage::PublishDetailsService, error?: false, success?: true)
+          stub_service_called(StatusPage::PublishListService, error?: false, success?: true)
 
           expect(result).to be_success
         end
@@ -47,7 +47,7 @@ describe StatusPage::PublishService do
 
       context 'when unpublish succeeds' do
         it 'unpublishes incident details and uploads incident list' do
-          expect_to_unpublish(error?: false)
+          stub_service_called(StatusPage::UnpublishDetailsService, error?: false, success?: true)
           expect_to_upload_list
 
           expect(result).to be_success
@@ -56,7 +56,7 @@ describe StatusPage::PublishService do
 
       context 'when unpublish service responses with error' do
         it 'returns the response' do
-          response = expect_to_unpublish(error?: true)
+          response = stub_service_called(StatusPage::UnpublishDetailsService, error?: true)
 
           expect(result).to be(response)
         end
@@ -65,11 +65,11 @@ describe StatusPage::PublishService do
 
     describe 'publish list' do
       context 'when upload fails' do
-        it 'returns error and skip list upload' do
-          expect_to_upload_details(issue)
-          expect_to_upload_list(status: 404)
+        it 'returns error response' do
+          stub_service_called(StatusPage::PublishDetailsService, error?: false, success?: true)
+          stub_service_called(StatusPage::PublishListService, error?: true, success?: false)
 
-          expect { result }.to raise_error(StatusPage::Storage::Error)
+          expect(result.error?).to be(true)
         end
       end
     end
@@ -95,9 +95,9 @@ describe StatusPage::PublishService do
 
   private
 
-  def expect_to_unpublish(**response_kwargs)
+  def stub_service_called(klass, **response_kwargs)
     service_response = double(**response_kwargs)
-    expect_next_instance_of(StatusPage::UnpublishDetailsService) do |service|
+    expect_next_instance_of(klass) do |service|
       expect(service).to receive(:execute).and_return(service_response)
     end
 
@@ -108,10 +108,6 @@ describe StatusPage::PublishService do
     stub_aws_request(:put, StatusPage::Storage.details_path(issue.iid), **kwargs)
   end
 
-  def expect_to_delete_details(issue, **kwargs)
-    stub_aws_request(:delete, StatusPage::Storage.details_path(issue.iid), **kwargs)
-  end
-
   def expect_to_upload_list(**kwargs)
     stub_aws_request(:put, StatusPage::Storage.list_path, **kwargs)
   end

ee/spec/support/shared_examples/status_page/publish_shared_examples.rb

@@ -77,90 +77,4 @@ RSpec.shared_examples 'publish incidents' do
       expect(result.message).to eq('Feature not available')
     end
   end
-
-  context 'publishes image uploads' do
-    before do
-      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
-
-    context 'when not in markdown' do
-      it 'publishes no images' do
-        expect(result).to be_success
-        expect(result.payload[:image_object_keys]).to eq([])
-      end
-    end
-
-    context 'when in markdown' do
-      let(:upload_secret) { '734b8524a16d44eb0ff28a2c2e4ff3c0' }
-      let(:image_file_name) { 'tanuki.png'}
-      let(:upload_path) { "/uploads/#{upload_secret}/#{image_file_name}" }
-      let(:markdown_field) { "![tanuki](#{upload_path})" }
-      let(:status_page_upload_path) { StatusPage::Storage.upload_path(issue.iid, upload_secret, image_file_name) }
-      let(:user_notes) { [] }
-
-      let(:open_file) { instance_double(File, read: 'stubbed read') }
-      let(:uploader) { instance_double(FileUploader) }
-
-      before do
-        allow(uploader).to receive(:open).and_yield(open_file).twice
-
-        allow_next_instance_of(UploadFinder) do |finder|
-          allow(finder).to receive(:execute).and_return(uploader)
-        end
-
-        allow(storage_client).to receive(:list_object_keys).and_return(Set[])
-        allow(storage_client).to receive(:upload_object)
-      end
-
-      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
-      end
-
-      context 'user notes uploads' do
-        let(:user_note) { instance_double(Note, note: markdown_field) }
-        let(:user_notes) { [user_note] }
-        let(:issue) { instance_double(Issue, notes: user_notes, description: '', iid: incident_id) }
-
-        it 'publishes images' do
-          expect(storage_client).to receive(:multipart_upload).with(status_page_upload_path, open_file).once
-
-          expect(result).to be_success
-        end
-      end
-
-      context 'when all images are in s3' do
-        before do
-          allow(storage_client).to receive(:list_object_keys).and_return(Set[status_page_upload_path])
-        end
-
-        it 'publishes no images' do
-          expect(storage_client).not_to receive(:multipart_upload)
-
-          expect(result).to be_success
-        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) }
-
-        before do
-          allow(storage_client).to receive(:list_object_keys).and_return(Set[status_page_upload_path])
-        end
-
-        it 'publishes only new images' 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
-        end
-      end
-    end
-  end
 end