Commit 193d9dc5 authored by Patrick Derichs's avatar Patrick Derichs Committed by Igor Drozdov

Store reason for resource state events

Also update specs and add needed fields for source of state change to
resource_state_events table.
parent f37dd23d
......@@ -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
......@@ -14853,6 +14853,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))))
);
......@@ -20149,6 +20154,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);
......@@ -22051,6 +22058,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;
......@@ -23616,6 +23626,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200521225346
20200522205606
20200522235146
20200524104346
20200525114553
20200525121014
20200525144525
......@@ -23674,6 +23685,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200615111857
20200615121217
20200615123055
20200615141554
20200615193524
20200615232735
20200615234047
......@@ -23702,6 +23714,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200622235737
20200623000148
20200623000320
20200623073431
20200623090030
20200623121135
20200623141544
......
# 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
......@@ -28424,9 +28424,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