Merge branch 'pl-status-page-track-subtransaction' into 'master'

Status Page: Track incident issue without subtransaction

See merge request gitlab-org/gitlab!68776

ee/app/models/status_page/published_incident.rb

@@ -8,8 +8,22 @@ module StatusPage
     belongs_to :issue, inverse_of: :status_page_published_incident
     validates :issue, presence: true
 
+    # NOTE: This method is not atomic and might raise
+    # +ActiveRecord::RecordNotUnique+ in case of a duplicate published issue.
+    #
+    # Previously, we've used +safe_find_or_create_by+ to circumvent this fact
+    # but it uses subtransactions under the hood which is problematic in nested
+    # transactions.
+    #
+    # See https://gitlab.com/groups/gitlab-org/-/epics/6540 for more context.
+    #
+    # In the rare event of +ActiveRecord::RecordNotUnique+ users end up seeing
+    # a meaningful error message. This behaviour is acceptable and that's why
+    # switched to unsafe method +find_or_create_by+.
+    #
+    # @raise ActiveRecord::RecordNotUnique
     def self.track(issue)
-      safe_find_or_create_by(issue: issue)
+      find_or_create_by!(issue: issue)
     end
 
     def self.untrack(issue)

ee/spec/models/status_page/published_incident_spec.rb

@@ -3,6 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe StatusPage::PublishedIncident do
+  let_it_be_with_reload(:issue) { create(:issue) }
+
+  before do
+    # prefill association cache
+    issue&.status_page_published_incident
+  end
+
   describe 'associations' do
     it { is_expected.to belong_to(:issue).inverse_of(:status_page_published_incident) }
   end
@@ -12,11 +19,10 @@ RSpec.describe StatusPage::PublishedIncident do
   end
 
   describe '.track' do
-    let_it_be(:issue) { create(:issue) }
-
     subject { described_class.track(issue) }
 
     it { is_expected.to be_a(described_class) }
+    it { is_expected.to eq(issue.status_page_published_incident) }
     specify { expect(subject.issue).to eq issue }
     specify { expect { subject }.to change { described_class.count }.by(1) }
 
@@ -26,14 +32,31 @@ RSpec.describe StatusPage::PublishedIncident do
       end
 
       it { is_expected.to be_a(described_class) }
+      it { is_expected.to eq(issue.status_page_published_incident) }
       specify { expect(subject.issue).to eq issue }
       specify { expect { subject }.not_to change { described_class.count } }
     end
+
+    context 'when issue is new record' do
+      let(:issue) { build(:issue) }
+
+      it { is_expected.to be_a(described_class) }
+      it { is_expected.to eq(issue.status_page_published_incident) }
+      specify { expect(subject.issue).to eq issue }
+      specify { expect { subject }.to change { described_class.count }.by(1) }
+    end
+
+    context 'when issue is nil' do
+      let(:issue) { nil }
+
+      specify do
+        expect { subject }
+          .to raise_error(ActiveRecord::RecordInvalid, /Issue can't be blank/)
+      end
+    end
   end
 
   describe '.untrack' do
-    let_it_be(:issue) { create(:issue) }
-
     subject { described_class.untrack(issue) }
 
     context 'when the incident is not yet tracked' do