Commit 6a88b457 authored by Sean McGivern's avatar Sean McGivern

Merge branch '38096-splitmr-write-resource-milestone-events-pd' into 'master'

Create ResourceMilestoneEvents whenever milestones are changed

See merge request gitlab-org/gitlab!24780
parents a322913c ef481f13
# frozen_string_literal: true
module MilestoneEventable
extend ActiveSupport::Concern
included do
has_many :resource_milestone_events
end
end
# frozen_string_literal: true
module ResourceEventTools
extend ActiveSupport::Concern
included do
belongs_to :user
validates :user, presence: { unless: :importing? }, on: :create
validate :exactly_one_issuable
scope :created_after, ->(time) { where('created_at > ?', time) }
end
def exactly_one_issuable
issuable_count = self.class.issuable_attrs.count { |attr| self["#{attr}_id"] }
return true if issuable_count == 1
# if none of issuable IDs is set, check explicitly if nested issuable
# object is set, this is used during project import
if issuable_count == 0 && importing?
issuable_count = self.class.issuable_attrs.count { |attr| self.public_send(attr) } # rubocop:disable GitlabSecurity/PublicSend
return true if issuable_count == 1
end
errors.add(:base, "Exactly one of #{self.class.issuable_attrs.join(', ')} is required")
end
end
......@@ -15,6 +15,7 @@ class Issue < ApplicationRecord
include ThrottledTouch
include LabelEventable
include IgnorableColumns
include MilestoneEventable
DueDateStruct = Struct.new(:title, :name).freeze
NoDueDate = DueDateStruct.new('No Due Date', '0').freeze
......
......@@ -18,6 +18,7 @@ class MergeRequest < ApplicationRecord
include DeprecatedAssignee
include ShaAttribute
include IgnorableColumns
include MilestoneEventable
sha_attribute :squash_commit_sha
......
# frozen_string_literal: true
class MilestoneNote < ::Note
attr_accessor :resource_parent, :event, :milestone
def self.from_event(event, resource: nil, resource_parent: nil)
resource ||= event.resource
attrs = {
system: true,
author: event.user,
created_at: event.created_at,
noteable: resource,
milestone: event.milestone,
event: event,
system_note_metadata: ::SystemNoteMetadata.new(action: 'milestone'),
resource_parent: resource_parent
}
if resource_parent.is_a?(Project)
attrs[:project_id] = resource_parent.id
end
MilestoneNote.new(attrs)
end
def note
@note ||= note_text
end
def note_html
@note_html ||= Banzai::Renderer.cacheless_render_field(self, :note, { group: group, project: project })
end
def project
resource_parent if resource_parent.is_a?(Project)
end
def group
resource_parent if resource_parent.is_a?(Group)
end
private
def note_text(html: false)
format = milestone&.group_milestone? ? :name : :iid
milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}"
end
end
......@@ -4,20 +4,17 @@ class ResourceLabelEvent < ApplicationRecord
include Importable
include Gitlab::Utils::StrongMemoize
include CacheMarkdownField
include ResourceEventTools
cache_markdown_field :reference
belongs_to :user
belongs_to :issue
belongs_to :merge_request
belongs_to :label
scope :created_after, ->(time) { where('created_at > ?', time) }
scope :inc_relations, -> { includes(:label, :user) }
validates :user, presence: { unless: :importing? }, on: :create
validates :label, presence: { unless: :importing? }, on: :create
validate :exactly_one_issuable
after_save :expire_etag_cache
after_destroy :expire_etag_cache
......@@ -94,22 +91,6 @@ class ResourceLabelEvent < ApplicationRecord
end
end
def exactly_one_issuable
issuable_count = self.class.issuable_attrs.count { |attr| self["#{attr}_id"] }
return true if issuable_count == 1
# if none of issuable IDs is set, check explicitly if nested issuable
# object is set, this is used during project import
if issuable_count == 0 && importing?
issuable_count = self.class.issuable_attrs.count { |attr| self.public_send(attr) } # rubocop:disable GitlabSecurity/PublicSend
return true if issuable_count == 1
end
errors.add(:base, "Exactly one of #{self.class.issuable_attrs.join(', ')} is required")
end
def expire_etag_cache
issuable.expire_note_etag_cache
end
......
# frozen_string_literal: true
class ResourceMilestoneEvent < ApplicationRecord
include Gitlab::Utils::StrongMemoize
include Importable
include ResourceEventTools
belongs_to :issue
belongs_to :merge_request
belongs_to :milestone
scope :by_issue, ->(issue) { where(issue_id: issue.id) }
scope :by_merge_request, ->(merge_request) { where(merge_request_id: merge_request.id) }
enum action: {
add: 1,
remove: 2
}
# state is used for issue and merge request states.
enum state: Issue.available_states.merge(MergeRequest.available_states)
def self.issuable_attrs
%i(issue merge_request).freeze
end
def resource
issue || merge_request
end
end
......@@ -19,6 +19,7 @@ module Issuable
copy_resource_label_events
copy_resource_weight_events
copy_resource_milestone_events
end
private
......@@ -65,6 +66,23 @@ module Issuable
end
end
def copy_resource_milestone_events
entity_key = new_entity.class.name.underscore.foreign_key
copy_events(ResourceMilestoneEvent.table_name, original_entity.resource_milestone_events) do |event|
matching_destination_milestone = matching_milestone(event.milestone.title)
if matching_destination_milestone.present?
event.attributes
.except('id', 'reference', 'reference_html')
.merge(entity_key => new_entity.id,
'milestone_id' => matching_destination_milestone.id,
'action' => ResourceMilestoneEvent.actions[event.action],
'state' => ResourceMilestoneEvent.states[event.state])
end
end
end
def copy_events(table_name, events_to_copy)
events_to_copy.find_in_batches do |batch|
events = batch.map do |event|
......
......@@ -22,13 +22,17 @@ module Issuable
end
create_due_date_note if issuable.previous_changes.include?('due_date')
create_milestone_note if issuable.previous_changes.include?('milestone_id')
create_milestone_note if has_milestone_changes?
create_labels_note(old_labels) if old_labels && issuable.labels != old_labels
end
end
private
def has_milestone_changes?
issuable.previous_changes.include?('milestone_id')
end
def handle_time_tracking_note
if issuable.previous_changes.include?('time_estimate')
create_time_estimate_note
......@@ -95,8 +99,17 @@ module Issuable
end
def create_milestone_note
if milestone_changes_tracking_enabled?
# Creates a synthetic note
ResourceEvents::ChangeMilestoneService.new(resource: issuable, user: current_user).execute
else
SystemNoteService.change_milestone(issuable, issuable.project, current_user, issuable.milestone)
end
end
def milestone_changes_tracking_enabled?
::Feature.enabled?(:track_resource_milestone_change_events, issuable.project)
end
def create_due_date_note
SystemNoteService.change_due_date(issuable, issuable.project, current_user, issuable.due_date)
......
# frozen_string_literal: true
module ResourceEvents
class ChangeMilestoneService
attr_reader :resource, :user, :event_created_at, :resource_args
def initialize(resource:, user:, created_at: Time.now)
@resource = resource
@user = user
@event_created_at = created_at
@resource_args = {
user_id: user.id,
created_at: event_created_at
}
end
def execute
args = build_resource_args
action = if milestone.nil?
:remove
else
:add
end
record = args.merge(milestone_id: milestone&.id, action: ResourceMilestoneEvent.actions[action])
create_event(record)
end
private
def milestone
resource&.milestone
end
def create_event(record)
ResourceMilestoneEvent.create(record)
resource.expire_note_etag_cache
end
def build_resource_args
key = resource.class.name.underscore.foreign_key
resource_args.merge(key => resource.id, state: ResourceMilestoneEvent.states[resource.state])
end
end
end
......@@ -9,6 +9,11 @@ module ResourceEvents
class MergeIntoNotesService
include Gitlab::Utils::StrongMemoize
SYNTHETIC_NOTE_BUILDER_SERVICES = [
SyntheticLabelNotesBuilderService,
SyntheticMilestoneNotesBuilderService
].freeze
attr_reader :resource, :current_user, :params
def initialize(resource, current_user, params = {})
......@@ -24,7 +29,9 @@ module ResourceEvents
private
def synthetic_notes
SyntheticLabelNotesBuilderService.new(resource, current_user, params).execute
SYNTHETIC_NOTE_BUILDER_SERVICES.flat_map do |service|
service.new(resource, current_user, params).execute
end
end
end
end
......
# frozen_string_literal: true
# We store events about resource milestone changes in a separate table,
# but we still want to display notes about milestone changes
# as classic system notes in UI. This service generates "synthetic" notes for
# milestone event changes.
module ResourceEvents
class SyntheticMilestoneNotesBuilderService < BaseSyntheticNotesBuilderService
private
def synthetic_notes
return [] unless tracking_enabled?
milestone_change_events.map do |event|
MilestoneNote.from_event(event, resource: resource, resource_parent: resource_parent)
end
end
def milestone_change_events
return [] unless resource.respond_to?(:resource_milestone_events)
events = resource.resource_milestone_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
since_fetch_at(events)
end
def tracking_enabled?
::Feature.enabled?(:track_resource_milestone_change_events, resource.project)
end
end
end
---
title: Add possibility to track milestone changes on issues and merge requests
merge_request: 24780
author:
type: added
......@@ -34,7 +34,7 @@ describe 'New/edit issue', :js do
before do
# Using `allow_any_instance_of`/`and_wrap_original`, `original` would
# somehow refer to the very block we defined to _wrap_ that method, instead of
# the original method, resulting in infinite recurison when called.
# the original method, resulting in infinite recursion when called.
# This is likely a bug with helper modules included into dynamically generated view classes.
# To work around this, we have to hold on to and call to the original implementation manually.
original_issue_dropdown_options = FormHelper.instance_method(:assignees_dropdown_options)
......
......@@ -135,6 +135,10 @@ describe Projects::MilestonesController do
end
describe "#destroy" do
before do
stub_feature_flags(track_resource_milestone_change_events: false)
end
it "removes milestone" do
expect(issue.milestone_id).to eq(milestone.id)
......
# frozen_string_literal: true
FactoryBot.define do
factory :resource_milestone_event do
issue { merge_request.nil? ? create(:issue) : nil }
merge_request { nil }
milestone
action { :add }
state { :opened }
user { issue&.author || merge_request&.author || create(:user) }
end
end
......@@ -9,6 +9,7 @@ issues:
- notes
- resource_label_events
- resource_weight_events
- resource_milestone_events
- sentry_issue
- label_links
- labels
......@@ -109,6 +110,7 @@ merge_requests:
- milestone
- notes
- resource_label_events
- resource_milestone_events
- label_links
- labels
- last_edited_by
......
# frozen_string_literal: true
require 'spec_helper'
describe MilestoneNote do
describe '.from_event' do
let(:author) { create(:user) }
let(:project) { create(:project, :repository) }
let(:noteable) { create(:issue, author: author, project: project) }
let(:event) { create(:resource_milestone_event, issue: noteable) }
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
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ResourceMilestoneEvent, type: :model do
it_behaves_like 'a resource event'
it_behaves_like 'a resource event for issues'
it_behaves_like 'a resource event for merge requests'
it_behaves_like 'having unique enum values'
describe 'associations' do
it { is_expected.to belong_to(:milestone) }
end
describe 'validations' do
context 'when issue and merge_request are both nil' do
subject { build(described_class.name.underscore.to_sym, issue: nil, merge_request: nil) }
it { is_expected.not_to be_valid }
end
context 'when issue and merge_request are both set' do
subject { build(described_class.name.underscore.to_sym, issue: build(:issue), merge_request: build(:merge_request)) }
it { is_expected.not_to be_valid }
end
context 'when issue is set' do
subject { create(described_class.name.underscore.to_sym, issue: create(:issue), merge_request: nil) }
it { is_expected.to be_valid }
end
context 'when merge_request is set' do
subject { create(described_class.name.underscore.to_sym, issue: nil, merge_request: create(:merge_request)) }
it { is_expected.to be_valid }
end
end
describe 'states' do
[Issue, MergeRequest].each do |klass|
klass.available_states.each do |state|
it "supports state #{state.first} for #{klass.name.underscore}" do
model = create(klass.name.underscore, state: state[0])
key = model.class.name.underscore
event = build(described_class.name.underscore.to_sym, key => model, state: model.state)
expect(event.state).to eq(state[0])
end
end
end
end
end
......@@ -75,5 +75,47 @@ describe Issuable::Clone::AttributesRewriter do
expect(new_issue.reload.milestone).to eq(milestone)
end
context 'with existing milestone events' do
let!(:milestone1_project1) { create(:milestone, title: 'milestone1', project: project1) }
let!(:milestone2_project1) { create(:milestone, title: 'milestone2', project: project1) }
let!(:milestone3_project1) { create(:milestone, title: 'milestone3', project: project1) }
let!(:milestone1_project2) { create(:milestone, title: 'milestone1', project: project2) }
let!(:milestone2_project2) { create(:milestone, title: 'milestone2', project: project2) }
before do
original_issue.update(milestone: milestone2_project1)
create_event(milestone1_project1)
create_event(milestone2_project1)
create_event(milestone1_project1, 'remove')
create_event(milestone3_project1)
end
it 'copies existing resource milestone events' do
subject.execute
new_issue_milestone_events = new_issue.reload.resource_milestone_events
expect(new_issue_milestone_events.count).to eq(3)
expect_milestone_event(new_issue_milestone_events.first, milestone: milestone1_project2, action: 'add', state: 'opened')
expect_milestone_event(new_issue_milestone_events.second, milestone: milestone2_project2, action: 'add', state: 'opened')
expect_milestone_event(new_issue_milestone_events.third, milestone: milestone1_project2, action: 'remove', state: 'opened')
end
def create_event(milestone, action = 'add')
create(:resource_milestone_event, issue: original_issue, milestone: milestone, action: action)
end
def expect_milestone_event(event, expected_attrs)
expect(event.milestone_id).to eq(expected_attrs[:milestone].id)
expect(event.action).to eq(expected_attrs[:action])
expect(event.state).to eq(expected_attrs[:state])
expect(event.reference).to be_nil
expect(event.reference_html).to be_nil
end
end
end
end
......@@ -3,8 +3,9 @@
require 'spec_helper'
describe Issuable::CommonSystemNotesService do
let(:user) { create(:user) }
let(:project) { create(:project) }
let_it_be(:project) { create(:project) }
let_it_be(:user) { create(:user) }
let(:issuable) { create(:issue, project: project) }
context 'on issuable update' do
......@@ -35,6 +36,8 @@ describe Issuable::CommonSystemNotesService do
before do
milestone = create(:milestone, project: project)
issuable.milestone_id = milestone.id
stub_feature_flags(track_resource_milestone_change_events: false)
end
it_behaves_like 'system note creation', {}, 'changed milestone'
......@@ -97,14 +100,41 @@ describe Issuable::CommonSystemNotesService do
expect(event.user_id).to eq user.id
end
it 'creates a system note for milestone set' do
context 'when milestone change event tracking is disabled' do
before do
stub_feature_flags(track_resource_milestone_change_events: false)
issuable.milestone = create(:milestone, project: project)
issuable.save
end
it 'creates a system note for milestone set' do
expect { subject }.to change { issuable.notes.count }.from(0).to(1)
expect(issuable.notes.last.note).to match('changed milestone')
end
it 'does not create a milestone change event' do
expect { subject }.not_to change { ResourceMilestoneEvent.count }
end
end
context 'when milestone change event tracking is enabled' do
let_it_be(:milestone) { create(:milestone, project: project) }
let_it_be(:issuable) { create(:issue, project: project, milestone: milestone) }
before do
stub_feature_flags(track_resource_milestone_change_events: true)
end
it 'does not create a system note for milestone set' do
expect { subject }.not_to change { issuable.notes.count }
end
it 'creates a milestone change event' do
expect { subject }.to change { ResourceMilestoneEvent.count }.from(0).to(1)
end
end
it 'creates a system note for due_date set' do
issuable.due_date = Date.today
issuable.save
......
......@@ -385,6 +385,10 @@ describe Issues::UpdateService, :mailer do
end
context 'when the milestone is removed' do
before do
stub_feature_flags(track_resource_milestone_change_events: false)
end
let!(:non_subscriber) { create(:user) }
let!(:subscriber) do
......@@ -411,6 +415,10 @@ describe Issues::UpdateService, :mailer do
end
context 'when the milestone is changed' do
before do
stub_feature_flags(track_resource_milestone_change_events: false)
end
let!(:non_subscriber) { create(:user) }
let!(:subscriber) do
......
......@@ -367,6 +367,10 @@ describe MergeRequests::UpdateService, :mailer do
end
context 'when the milestone is removed' do
before do
stub_feature_flags(track_resource_milestone_change_events: false)
end
let!(:non_subscriber) { create(:user) }
let!(:subscriber) do
......@@ -393,6 +397,10 @@ describe MergeRequests::UpdateService, :mailer do
end
context 'when the milestone is changed' do
before do
stub_feature_flags(track_resource_milestone_change_events: false)
end
let!(:non_subscriber) { create(:user) }
let!(:subscriber) do
......
# frozen_string_literal: true
require 'spec_helper'
describe ResourceEvents::ChangeMilestoneService do
shared_examples 'milestone events creator' do
let_it_be(:user) { create(:user) }
let_it_be(:milestone) { create(:milestone) }
context 'when milestone is present' do
before do
resource.milestone = milestone
end
let(:service) { described_class.new(resource: resource, user: user, created_at: created_at_time) }
it 'creates the expected event record' do
expect { service.execute }.to change { ResourceMilestoneEvent.count }.from(0).to(1)
events = ResourceMilestoneEvent.all
expect(events.size).to eq(1)
expect_event_record(events.first, action: 'add', milestone: milestone, state: 'opened')
end
end
context 'when milestones is not present' do
before do
resource.milestone = nil
end
let(:service) { described_class.new(resource: resource, user: user, created_at: created_at_time) }
it 'creates the expected event records' do
expect { service.execute }.to change { ResourceMilestoneEvent.count }.from(0).to(1)
expect_event_record(ResourceMilestoneEvent.first, action: 'remove', milestone: nil, state: 'opened')
end
end
def expect_event_record(event, expected_attrs)
expect(event.action).to eq(expected_attrs[:action])
expect(event.state).to eq(expected_attrs[:state])
expect(event.user).to eq(user)
expect(event.issue).to eq(resource) if resource.is_a?(Issue)
expect(event.issue).to be_nil unless resource.is_a?(Issue)
expect(event.merge_request).to eq(resource) if resource.is_a?(MergeRequest)
expect(event.merge_request).to be_nil unless resource.is_a?(MergeRequest)
expect(event.milestone).to eq(expected_attrs[:milestone])
expect(event.created_at).to eq(created_at_time)
end
end
let_it_be(:merge_request) { create(:merge_request) }
let_it_be(:issue) { create(:issue) }
let!(:created_at_time) { Time.utc(2019, 12, 30) }
it_behaves_like 'milestone events creator' do
let(:resource) { issue }
end
it_behaves_like 'milestone events creator' do
let(:resource) { merge_request }
end
end
......@@ -50,6 +50,8 @@ RSpec.shared_examples 'move quick action' do
let(:bug) { create(:label, project: project, title: 'bug') }
let(:wontfix) { create(:label, project: project, title: 'wontfix') }
let!(:target_milestone) { create(:milestone, title: '1.0', project: target_project) }
before do
target_project.add_maintainer(user)
end
......
# frozen_string_literal: true
require 'spec_helper'
shared_examples 'a resource event' do
let_it_be(:user1) { create(:user) }
let_it_be(:user2) { create(:user) }
let_it_be(:issue1) { create(:issue, author: user1) }
let_it_be(:issue2) { create(:issue, author: user1) }
let_it_be(:issue3) { create(:issue, author: user2) }
describe 'validations' do
it { is_expected.not_to allow_value(nil).for(:user) }
end
describe 'associations' do
it { is_expected.to belong_to(:user) }
end
describe '.created_after' do
let!(:created_at1) { 1.day.ago }
let!(:created_at2) { 2.days.ago }
let!(:created_at3) { 3.days.ago }
let!(:event1) { create(described_class.name.underscore.to_sym, issue: issue1, created_at: created_at1) }
let!(:event2) { create(described_class.name.underscore.to_sym, issue: issue2, created_at: created_at2) }
let!(:event3) { create(described_class.name.underscore.to_sym, issue: issue2, created_at: created_at3) }
it 'returns the expected events' do
events = described_class.created_after(created_at3)
expect(events).to contain_exactly(event1, event2)
end
it 'returns no events if time is after last record time' do
events = described_class.created_after(1.minute.ago)
expect(events).to be_empty
end
end
end
shared_examples 'a resource event for issues' do
let_it_be(:user1) { create(:user) }
let_it_be(:user2) { create(:user) }
let_it_be(:issue1) { create(:issue, author: user1) }
let_it_be(:issue2) { create(:issue, author: user1) }
let_it_be(:issue3) { create(:issue, author: user2) }
describe 'associations' do
it { is_expected.to belong_to(:issue) }
end
describe '.by_issue' do
let_it_be(:event1) { create(described_class.name.underscore.to_sym, issue: issue1) }
let_it_be(:event2) { create(described_class.name.underscore.to_sym, issue: issue2) }
let_it_be(:event3) { create(described_class.name.underscore.to_sym, issue: issue1) }
it 'returns the expected records for an issue with events' do
events = described_class.by_issue(issue1)
expect(events).to contain_exactly(event1, event3)
end
it 'returns the expected records for an issue with no events' do
events = described_class.by_issue(issue3)
expect(events).to be_empty
end
end
end
shared_examples 'a resource event for merge requests' do
let_it_be(:user1) { create(:user) }
let_it_be(:user2) { create(:user) }
let_it_be(:merge_request1) { create(:merge_request, author: user1) }
let_it_be(:merge_request2) { create(:merge_request, author: user1) }
let_it_be(:merge_request3) { create(:merge_request, author: user2) }
describe 'associations' do
it { is_expected.to belong_to(:merge_request) }
end
describe '.by_merge_request' do
let_it_be(:event1) { create(described_class.name.underscore.to_sym, merge_request: merge_request1) }
let_it_be(:event2) { create(described_class.name.underscore.to_sym, merge_request: merge_request2) }
let_it_be(:event3) { create(described_class.name.underscore.to_sym, merge_request: merge_request1) }
it 'returns the expected records for an issue with events' do
events = described_class.by_merge_request(merge_request1)
expect(events).to contain_exactly(event1, event3)
end
it 'returns the expected records for an issue with no events' do
events = described_class.by_merge_request(merge_request3)
expect(events).to be_empty
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