Commit a3787b4c authored by Igor Drozdov's avatar Igor Drozdov

Merge branch 'create-state-events-pd' into 'master'

Store reason for resource state events

See merge request gitlab-org/gitlab!32924
parents ec8fc1d0 193d9dc5
......@@ -6,6 +6,8 @@ class ResourceStateEvent < ResourceEvent
validate :exactly_one_issuable
belongs_to :source_merge_request, class_name: 'MergeRequest', foreign_key: :source_merge_request_id
# state is used for issue and merge request states.
enum state: Issue.available_states.merge(MergeRequest.available_states).merge(reopened: 5)
......
# frozen_string_literal: true
class StateNote < SyntheticNote
include Gitlab::Utils::StrongMemoize
def self.from_event(event, resource: nil, resource_parent: nil)
attrs = note_attributes(event.state, event, resource, resource_parent)
attrs = note_attributes(action_by(event), event, resource, resource_parent)
StateNote.new(attrs)
end
def note_html
@note_html ||= "<p dir=\"auto\">#{note_text(html: true)}</p>"
@note_html ||= Banzai::Renderer.cacheless_render_field(self, :note, { group: group, project: project })
end
private
def note_text(html: false)
event.state
if event.state == 'closed'
if event.close_after_error_tracking_resolve
return 'resolved the corresponding error and closed the issue.'
end
if event.close_auto_resolve_prometheus_alert
return 'automatically closed this issue because the alert resolved.'
end
end
body = event.state.dup
body << " via #{event_source.gfm_reference(project)}" if event_source
body
end
def event_source
strong_memoize(:event_source) do
if event.source_commit
project&.commit(event.source_commit)
else
event.source_merge_request
end
end
end
def self.action_by(event)
event.state == 'reopened' ? 'opened' : event.state
end
end
......@@ -3,20 +3,18 @@
class SyntheticNote < Note
attr_accessor :resource_parent, :event
self.abstract_class = true
def self.note_attributes(action, event, resource, resource_parent)
resource ||= event.resource
attrs = {
system: true,
author: event.user,
created_at: event.created_at,
discussion_id: event.discussion_id,
noteable: resource,
event: event,
system_note_metadata: ::SystemNoteMetadata.new(action: action),
resource_parent: resource_parent
system: true,
author: event.user,
created_at: event.created_at,
discussion_id: event.discussion_id,
noteable: resource,
event: event,
system_note_metadata: ::SystemNoteMetadata.new(action: action),
resource_parent: resource_parent
}
if resource_parent.is_a?(Project)
......
......@@ -11,44 +11,30 @@ class EventCreateService
IllegalActionError = Class.new(StandardError)
def open_issue(issue, current_user)
create_resource_event(issue, current_user, :opened)
create_record_event(issue, current_user, :created)
end
def close_issue(issue, current_user)
create_resource_event(issue, current_user, :closed)
create_record_event(issue, current_user, :closed)
end
def reopen_issue(issue, current_user)
create_resource_event(issue, current_user, :reopened)
create_record_event(issue, current_user, :reopened)
end
def open_mr(merge_request, current_user)
create_resource_event(merge_request, current_user, :opened)
create_record_event(merge_request, current_user, :created)
end
def close_mr(merge_request, current_user)
create_resource_event(merge_request, current_user, :closed)
create_record_event(merge_request, current_user, :closed)
end
def reopen_mr(merge_request, current_user)
create_resource_event(merge_request, current_user, :reopened)
create_record_event(merge_request, current_user, :reopened)
end
def merge_mr(merge_request, current_user)
create_resource_event(merge_request, current_user, :merged)
create_record_event(merge_request, current_user, :merged)
end
......@@ -220,18 +206,6 @@ class EventCreateService
{ resource_parent_attr => resource_parent.id }
end
def create_resource_event(issuable, current_user, status)
return unless state_change_tracking_enabled?(issuable)
ResourceEvents::ChangeStateService.new(resource: issuable, user: current_user)
.execute(status)
end
def state_change_tracking_enabled?(issuable)
issuable&.respond_to?(:resource_state_events) &&
::Feature.enabled?(:track_resource_state_change_events, issuable&.project)
end
end
EventCreateService.prepend_if_ee('EE::EventCreateService')
......@@ -8,12 +8,18 @@ module ResourceEvents
@user, @resource = user, resource
end
def execute(state)
def execute(params)
@params = params
ResourceStateEvent.create(
user: user,
issue: issue,
merge_request: merge_request,
source_commit: commit_id_of(mentionable_source),
source_merge_request_id: merge_request_id_of(mentionable_source),
state: ResourceStateEvent.states[state],
close_after_error_tracking_resolve: close_after_error_tracking_resolve,
close_auto_resolve_prometheus_alert: close_auto_resolve_prometheus_alert,
created_at: Time.zone.now)
resource.expire_note_etag_cache
......@@ -21,6 +27,36 @@ module ResourceEvents
private
attr_reader :params
def close_auto_resolve_prometheus_alert
params[:close_auto_resolve_prometheus_alert] || false
end
def close_after_error_tracking_resolve
params[:close_after_error_tracking_resolve] || false
end
def state
params[:status]
end
def mentionable_source
params[:mentionable_source]
end
def commit_id_of(mentionable_source)
return unless mentionable_source.is_a?(Commit)
mentionable_source.id[0...40]
end
def merge_request_id_of(mentionable_source)
return unless mentionable_source.is_a?(MergeRequest)
mentionable_source.id
end
def issue
return unless resource.is_a?(Issue)
......
......@@ -228,7 +228,9 @@ module SystemNotes
# A state event which results in a synthetic note will be
# created by EventCreateService if change event tracking
# is enabled.
unless state_change_tracking_enabled?
if state_change_tracking_enabled?
create_resource_state_event(status: status, mentionable_source: source)
else
create_note(NoteSummary.new(noteable, project, author, body, action: action))
end
end
......@@ -288,15 +290,23 @@ module SystemNotes
end
def close_after_error_tracking_resolve
body = _('resolved the corresponding error and closed the issue.')
if state_change_tracking_enabled?
create_resource_state_event(status: 'closed', close_after_error_tracking_resolve: true)
else
body = 'resolved the corresponding error and closed the issue.'
create_note(NoteSummary.new(noteable, project, author, body, action: 'closed'))
create_note(NoteSummary.new(noteable, project, author, body, action: 'closed'))
end
end
def auto_resolve_prometheus_alert
body = 'automatically closed this issue because the alert resolved.'
if state_change_tracking_enabled?
create_resource_state_event(status: 'closed', close_auto_resolve_prometheus_alert: true)
else
body = 'automatically closed this issue because the alert resolved.'
create_note(NoteSummary.new(noteable, project, author, body, action: 'closed'))
create_note(NoteSummary.new(noteable, project, author, body, action: 'closed'))
end
end
private
......@@ -324,6 +334,11 @@ module SystemNotes
note_text =~ /\A#{cross_reference_note_prefix}/i
end
def create_resource_state_event(params)
ResourceEvents::ChangeStateService.new(resource: noteable, user: author)
.execute(params)
end
def state_change_tracking_enabled?
noteable.respond_to?(:resource_state_events) &&
::Feature.enabled?(:track_resource_state_change_events, noteable.project)
......
---
title: Add source to resource state events
merge_request: 32924
author:
type: other
# frozen_string_literal: true
class AddSourceToResourceStateEvent < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
unless column_exists?(:resource_state_events, :source_commit)
add_column :resource_state_events, :source_commit, :text
end
add_text_limit :resource_state_events, :source_commit, 40
end
def down
remove_column :resource_state_events, :source_commit
end
end
# frozen_string_literal: true
class AddClosedByFieldsToResourceStateEvents < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def up
add_column :resource_state_events, :close_after_error_tracking_resolve, :boolean, default: false, null: false
add_column :resource_state_events, :close_auto_resolve_prometheus_alert, :boolean, default: false, null: false
end
def down
remove_column :resource_state_events, :close_auto_resolve_prometheus_alert, :boolean
remove_column :resource_state_events, :close_after_error_tracking_resolve, :boolean
end
end
# frozen_string_literal: true
class AddSourceMergeRequestIdToResourceStateEvents < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
INDEX_NAME = 'index_resource_state_events_on_source_merge_request_id'
DOWNTIME = false
disable_ddl_transaction!
def up
unless column_exists?(:resource_state_events, :source_merge_request_id)
add_column :resource_state_events, :source_merge_request_id, :bigint
end
unless index_exists?(:resource_state_events, :source_merge_request_id, name: INDEX_NAME)
add_index :resource_state_events, :source_merge_request_id, name: INDEX_NAME # rubocop: disable Migration/AddIndex
end
unless foreign_key_exists?(:resource_state_events, :merge_requests, column: :source_merge_request_id)
with_lock_retries do
add_foreign_key :resource_state_events, :merge_requests, column: :source_merge_request_id, on_delete: :nullify # rubocop:disable Migration/AddConcurrentForeignKey
end
end
end
def down
with_lock_retries do
remove_column :resource_state_events, :source_merge_request_id
end
end
end
......@@ -14855,6 +14855,11 @@ CREATE TABLE public.resource_state_events (
created_at timestamp with time zone NOT NULL,
state smallint NOT NULL,
epic_id integer,
source_commit text,
close_after_error_tracking_resolve boolean DEFAULT false NOT NULL,
close_auto_resolve_prometheus_alert boolean DEFAULT false NOT NULL,
source_merge_request_id bigint,
CONSTRAINT check_f0bcfaa3a2 CHECK ((char_length(source_commit) <= 40)),
CONSTRAINT state_events_must_belong_to_issue_or_merge_request_or_epic CHECK ((((issue_id <> NULL::bigint) AND (merge_request_id IS NULL) AND (epic_id IS NULL)) OR ((issue_id IS NULL) AND (merge_request_id <> NULL::bigint) AND (epic_id IS NULL)) OR ((issue_id IS NULL) AND (merge_request_id IS NULL) AND (epic_id <> NULL::integer))))
);
......@@ -20154,6 +20159,8 @@ CREATE INDEX index_resource_state_events_on_issue_id_and_created_at ON public.re
CREATE INDEX index_resource_state_events_on_merge_request_id ON public.resource_state_events USING btree (merge_request_id);
CREATE INDEX index_resource_state_events_on_source_merge_request_id ON public.resource_state_events USING btree (source_merge_request_id);
CREATE INDEX index_resource_state_events_on_user_id ON public.resource_state_events USING btree (user_id);
CREATE INDEX index_resource_weight_events_on_issue_id_and_created_at ON public.resource_weight_events USING btree (issue_id, created_at);
......@@ -22059,6 +22066,9 @@ ALTER TABLE ONLY public.operations_scopes
ALTER TABLE ONLY public.milestone_releases
ADD CONSTRAINT fk_rails_7ae0756a2d FOREIGN KEY (milestone_id) REFERENCES public.milestones(id) ON DELETE CASCADE;
ALTER TABLE ONLY public.resource_state_events
ADD CONSTRAINT fk_rails_7ddc5f7457 FOREIGN KEY (source_merge_request_id) REFERENCES public.merge_requests(id) ON DELETE SET NULL;
ALTER TABLE ONLY public.application_settings
ADD CONSTRAINT fk_rails_7e112a9599 FOREIGN KEY (instance_administration_project_id) REFERENCES public.projects(id) ON DELETE SET NULL;
......@@ -23624,6 +23634,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200521225346
20200522205606
20200522235146
20200524104346
20200525114553
20200525121014
20200525144525
......@@ -23682,6 +23693,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200615111857
20200615121217
20200615123055
20200615141554
20200615193524
20200615232735
20200615234047
......@@ -23711,6 +23723,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200622235737
20200623000148
20200623000320
20200623073431
20200623090030
20200623121135
20200623141217
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe WeightNote do
let(:author) { create(:user) }
let(:project) { create(:project, :repository) }
let(:noteable) { create(:issue, author: author, project: project) }
let(:event) { create(:resource_weight_event, issue: noteable) }
subject { described_class.from_event(event, resource: noteable, resource_parent: project) }
it_behaves_like 'a synthetic note', 'weight'
end
......@@ -28508,9 +28508,6 @@ msgstr[1] ""
msgid "reset it."
msgstr ""
msgid "resolved the corresponding error and closed the issue."
msgstr ""
msgid "revised"
msgstr ""
......
......@@ -11,9 +11,7 @@ RSpec.describe MilestoneNote do
subject { described_class.from_event(event, resource: noteable, resource_parent: project) }
it_behaves_like 'a system note', exclude_project: true do
let(:action) { 'milestone' }
end
it_behaves_like 'a synthetic note', 'milestone'
context 'with a remove milestone event' do
let(:milestone) { create(:milestone) }
......
......@@ -10,18 +10,62 @@ RSpec.describe StateNote do
ResourceStateEvent.states.each do |state, _value|
context "with event state #{state}" do
let_it_be(:event) { create(:resource_state_event, issue: noteable, state: state, created_at: '2020-02-05') }
let(:event) { create(:resource_state_event, issue: noteable, state: state, created_at: '2020-02-05') }
subject { described_class.from_event(event, resource: noteable, resource_parent: project) }
it_behaves_like 'a system note', exclude_project: true do
let(:action) { state.to_s }
it_behaves_like 'a synthetic note', state == 'reopened' ? 'opened' : state
it 'contains the expected values' do
expect(subject.author).to eq(author)
expect(subject.created_at).to eq(event.created_at)
expect(subject.note).to eq(state)
end
end
end
context 'with a mentionable source' do
subject { described_class.from_event(event, resource: noteable, resource_parent: project) }
context 'with a commit' do
let(:commit) { create(:commit, project: project) }
let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', source_commit: commit.id) }
it 'contains the expected values' do
expect(subject.author).to eq(author)
expect(subject.created_at).to eq(subject.created_at)
expect(subject.note).to eq("closed via commit #{commit.id}")
end
end
context 'with a merge request' do
let(:merge_request) { create(:merge_request, source_project: project) }
let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', source_merge_request: merge_request) }
it 'contains the expected values' do
expect(subject.author).to eq(author)
expect(subject.created_at).to eq(event.created_at)
expect(subject.note).to eq("closed via merge request !#{merge_request.iid}")
end
end
context 'when closed by error tracking' do
let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', close_after_error_tracking_resolve: true) }
it 'contains the expected values' do
expect(subject.author).to eq(author)
expect(subject.created_at).to eq(event.created_at)
expect(subject.note).to eq('resolved the corresponding error and closed the issue.')
end
end
context 'when closed by promotheus alert' do
let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', close_auto_resolve_prometheus_alert: true) }
it 'contains the expected values' do
expect(subject.author).to eq(author)
expect(subject.created_at).to eq(event.created_at)
expect(subject.note_html).to eq("<p dir=\"auto\">#{state}</p>")
expect(subject.note).to eq('automatically closed this issue because the alert resolved.')
end
end
end
......
......@@ -117,19 +117,29 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
expect { execute }.to change { alert.reload.resolved? }.to(true)
end
context 'existing issue' do
let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: parsed_alert.gitlab_fingerprint) }
it 'closes the issue' do
issue = alert.issue
expect { execute }
.to change { issue.reload.state }
.from('opened')
.to('closed')
[true, false].each do |state_tracking_enabled|
context 'existing issue' do
before do
stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
end
let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: parsed_alert.gitlab_fingerprint) }
it 'closes the issue' do
issue = alert.issue
expect { execute }
.to change { issue.reload.state }
.from('opened')
.to('closed')
end
if state_tracking_enabled
specify { expect { execute }.to change(ResourceStateEvent, :count).by(1) }
else
specify { expect { execute }.to change(Note, :count).by(1) }
end
end
specify { expect { execute }.to change(Note, :count).by(1) }
end
end
......
......@@ -16,7 +16,6 @@ RSpec.describe EventCreateService do
it "creates new event" do
expect { service.open_issue(issue, issue.author) }.to change { Event.count }
expect { service.open_issue(issue, issue.author) }.to change { ResourceStateEvent.count }
end
end
......@@ -27,7 +26,6 @@ RSpec.describe EventCreateService do
it "creates new event" do
expect { service.close_issue(issue, issue.author) }.to change { Event.count }
expect { service.close_issue(issue, issue.author) }.to change { ResourceStateEvent.count }
end
end
......@@ -38,7 +36,6 @@ RSpec.describe EventCreateService do
it "creates new event" do
expect { service.reopen_issue(issue, issue.author) }.to change { Event.count }
expect { service.reopen_issue(issue, issue.author) }.to change { ResourceStateEvent.count }
end
end
end
......@@ -51,7 +48,6 @@ RSpec.describe EventCreateService do
it "creates new event" do
expect { service.open_mr(merge_request, merge_request.author) }.to change { Event.count }
expect { service.open_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
end
end
......@@ -62,7 +58,6 @@ RSpec.describe EventCreateService do
it "creates new event" do
expect { service.close_mr(merge_request, merge_request.author) }.to change { Event.count }
expect { service.close_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
end
end
......@@ -73,7 +68,6 @@ RSpec.describe EventCreateService do
it "creates new event" do
expect { service.merge_mr(merge_request, merge_request.author) }.to change { Event.count }
expect { service.merge_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
end
end
......@@ -84,7 +78,6 @@ RSpec.describe EventCreateService do
it "creates new event" do
expect { service.reopen_mr(merge_request, merge_request.author) }.to change { Event.count }
expect { service.reopen_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
end
end
......
......@@ -8,32 +8,89 @@ RSpec.describe ResourceEvents::ChangeStateService do
let(:issue) { create(:issue, project: project) }
let(:merge_request) { create(:merge_request, source_project: project) }
let(:source_commit) { create(:commit, project: project) }
let(:source_merge_request) { create(:merge_request, source_project: project, target_project: project, target_branch: 'foo') }
describe '#execute' do
context 'when resource is an issue' do
%w[opened reopened closed locked].each do |state|
it "creates the expected event if issue has #{state} state" do
described_class.new(user: user, resource: issue).execute(state)
shared_examples 'a state event' do
%w[opened reopened closed locked].each do |state|
it "creates the expected event if resource has #{state} state" do
described_class.new(user: user, resource: resource).execute(status: state, mentionable_source: source)
event = resource.resource_state_events.last
event = issue.resource_state_events.last
expect(event.issue).to eq(issue)
if resource.is_a?(Issue)
expect(event.issue).to eq(resource)
expect(event.merge_request).to be_nil
expect(event.state).to eq(state)
elsif resource.is_a?(MergeRequest)
expect(event.issue).to be_nil
expect(event.merge_request).to eq(resource)
end
expect(event.state).to eq(state)
expect_event_source(event, source)
end
end
end
context 'when resource is a merge request' do
%w[opened reopened closed locked merged].each do |state|
it "creates the expected event if merge request has #{state} state" do
described_class.new(user: user, resource: merge_request).execute(state)
describe '#execute' do
context 'when resource is an Issue' do
context 'when no source is given' do
it_behaves_like 'a state event' do
let(:resource) { issue }
let(:source) { nil }
end
end
event = merge_request.resource_state_events.last
expect(event.issue).to be_nil
expect(event.merge_request).to eq(merge_request)
expect(event.state).to eq(state)
context 'when source commit is given' do
it_behaves_like 'a state event' do
let(:resource) { issue }
let(:source) { source_commit }
end
end
context 'when source merge request is given' do
it_behaves_like 'a state event' do
let(:resource) { issue }
let(:source) { source_merge_request }
end
end
end
context 'when resource is a MergeRequest' do
context 'when no source is given' do
it_behaves_like 'a state event' do
let(:resource) { merge_request }
let(:source) { nil }
end
end
context 'when source commit is given' do
it_behaves_like 'a state event' do
let(:resource) { merge_request }
let(:source) { source_commit }
end
end
context 'when source merge request is given' do
it_behaves_like 'a state event' do
let(:resource) { merge_request }
let(:source) { source_merge_request }
end
end
end
end
def expect_event_source(event, source)
if source.is_a?(MergeRequest)
expect(event.source_commit).to be_nil
expect(event.source_merge_request).to eq(source)
elsif source.is_a?(Commit)
expect(event.source_commit).to eq(source.id)
expect(event.source_merge_request).to be_nil
else
expect(event.source_merge_request).to be_nil
expect(event.source_commit).to be_nil
end
end
end
......@@ -161,7 +161,9 @@ RSpec.describe ::SystemNotes::IssuablesService do
let(:status) { 'reopened' }
let(:source) { nil }
it { is_expected.to be_nil }
it 'does not change note count' do
expect { subject }.not_to change { Note.count }
end
end
context 'with status reopened' do
......@@ -660,25 +662,67 @@ RSpec.describe ::SystemNotes::IssuablesService do
describe '#close_after_error_tracking_resolve' do
subject { service.close_after_error_tracking_resolve }
it_behaves_like 'a system note' do
let(:action) { 'closed' }
context 'when state tracking is enabled' do
before do
stub_feature_flags(track_resource_state_change_events: true)
end
it 'creates the expected state event' do
subject
event = ResourceStateEvent.last
expect(event.close_after_error_tracking_resolve).to eq(true)
expect(event.state).to eq('closed')
end
end
it 'creates the expected system note' do
expect(subject.note)
context 'when state tracking is disabled' do
before do
stub_feature_flags(track_resource_state_change_events: false)
end
it_behaves_like 'a system note' do
let(:action) { 'closed' }
end
it 'creates the expected system note' do
expect(subject.note)
.to eq('resolved the corresponding error and closed the issue.')
end
end
end
describe '#auto_resolve_prometheus_alert' do
subject { service.auto_resolve_prometheus_alert }
it_behaves_like 'a system note' do
let(:action) { 'closed' }
context 'when state tracking is enabled' do
before do
stub_feature_flags(track_resource_state_change_events: true)
end
it 'creates the expected state event' do
subject
event = ResourceStateEvent.last
expect(event.close_auto_resolve_prometheus_alert).to eq(true)
expect(event.state).to eq('closed')
end
end
it 'creates the expected system note' do
expect(subject.note).to eq('automatically closed this issue because the alert resolved.')
context 'when state tracking is disabled' do
before do
stub_feature_flags(track_resource_state_change_events: false)
end
it_behaves_like 'a system note' do
let(:action) { 'closed' }
end
it 'creates the expected system note' do
expect(subject.note).to eq('automatically closed this issue because the alert resolved.')
end
end
end
end
# frozen_string_literal: true
RSpec.shared_examples 'a synthetic note' do |action|
it_behaves_like 'a system note', exclude_project: true do
let(:action) { action }
end
describe '#discussion_id' do
before do
allow(event).to receive(:discussion_id).and_return('foobar42')
end
it 'returns the expected discussion id' do
expect(subject.discussion_id(nil)).to eq('foobar42')
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment