Commit ef481f13 authored by Patrick Derichs's avatar Patrick Derichs

Create ResourceMilestoneEvents whenever milestones are changed

Use default system note content for MilestoneNote

Also update specs and fixed one typo.

Remove milestones_of method

Fix display of synthetic milestone notes

Fix display of synthetic milestone notes with direct banzai render call

Cleanup and simplify options hash

Extract ResourceEventTools module

Move some scopes and validations to resource events module

Add changelog entry
parent 434b7047
# 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,7 +99,16 @@ module Issuable
end
def create_milestone_note
SystemNoteService.change_milestone(issuable, issuable.project, current_user, issuable.milestone)
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
......
# 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,12 +100,39 @@ describe Issuable::CommonSystemNotesService do
expect(event.user_id).to eq user.id
end
it 'creates a system note for milestone set' do
issuable.milestone = create(:milestone, project: project)
issuable.save
context 'when milestone change event tracking is disabled' do
before do
stub_feature_flags(track_resource_milestone_change_events: false)
expect { subject }.to change { issuable.notes.count }.from(0).to(1)
expect(issuable.notes.last.note).to match('changed milestone')
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
......
......@@ -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