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

Merge branch '38096-create-resource-weight-events-table-pd' into 'master'

Add weight change tracking

See merge request gitlab-org/gitlab!21515
parents 9bb91e4f a05f762c
# frozen_string_literal: true
class ResourceWeightEvent < ApplicationRecord
include Gitlab::Utils::StrongMemoize
validates :user, presence: true
validates :issue, presence: true
belongs_to :user
belongs_to :issue
scope :by_issue, ->(issue) { where(issue_id: issue.id) }
scope :created_after, ->(time) { where('created_at > ?', time) }
def discussion_id(resource = nil)
strong_memoize(:discussion_id) do
Digest::SHA1.hexdigest(discussion_id_key.join("-"))
end
end
private
def discussion_id_key
[self.class.name, created_at, user_id]
end
end
# frozen_string_literal: true
# We store events about issuable label changes and weight changes in a separate
# table (not as other system notes), but we still want to display notes about
# label changes and weight changes as classic system notes in UI. This service
# generates "synthetic" notes for label event changes.
module ResourceEvents
class BaseSyntheticNotesBuilderService
include Gitlab::Utils::StrongMemoize
attr_reader :resource, :current_user, :params
def initialize(resource, current_user, params = {})
@resource = resource
@current_user = current_user
@params = params
end
def execute
synthetic_notes
end
private
def since_fetch_at(events)
return events unless params[:last_fetched_at].present?
last_fetched_at = Time.at(params.fetch(:last_fetched_at).to_i)
events.created_after(last_fetched_at - NotesFinder::FETCH_OVERLAP)
end
def resource_parent
strong_memoize(:resource_parent) do
resource.project || resource.group
end
end
end
end
# frozen_string_literal: true
# We store events about issuable label changes in a separate table (not as
# other system notes), but we still want to display notes about label changes
# as classic system notes in UI. This service generates "synthetic" notes for
# label event changes and merges them with classic notes and sorts them by
# creation time.
# We store events about issuable label changes and weight changes in separate tables (not as
# other system notes), but we still want to display notes about label and weight changes
# as classic system notes in UI. This service merges synthetic label and weight notes
# with classic notes and sorts them by creation time.
module ResourceEvents
class MergeIntoNotesService
......@@ -19,39 +18,15 @@ module ResourceEvents
end
def execute(notes = [])
(notes + label_notes).sort_by { |n| n.created_at }
(notes + synthetic_notes).sort_by { |n| n.created_at }
end
private
def label_notes
label_events_by_discussion_id.map do |discussion_id, events|
LabelNote.from_events(events, resource: resource, resource_parent: resource_parent)
end
end
# rubocop: disable CodeReuse/ActiveRecord
def label_events_by_discussion_id
return [] unless resource.respond_to?(:resource_label_events)
events = resource.resource_label_events.includes(:label, user: :status)
events = since_fetch_at(events)
events.group_by { |event| event.discussion_id }
end
# rubocop: enable CodeReuse/ActiveRecord
def since_fetch_at(events)
return events unless params[:last_fetched_at].present?
last_fetched_at = Time.at(params.fetch(:last_fetched_at).to_i)
events.created_after(last_fetched_at - NotesFinder::FETCH_OVERLAP)
end
def resource_parent
strong_memoize(:resource_parent) do
resource.project || resource.group
end
def synthetic_notes
SyntheticLabelNotesBuilderService.new(resource, current_user, params).execute
end
end
end
ResourceEvents::MergeIntoNotesService.prepend_if_ee('EE::ResourceEvents::MergeIntoNotesService')
# frozen_string_literal: true
# We store events about issuable label changes in a separate table (not as
# other system notes), but we still want to display notes about label changes
# as classic system notes in UI. This service generates "synthetic" notes for
# label event changes.
module ResourceEvents
class SyntheticLabelNotesBuilderService < BaseSyntheticNotesBuilderService
private
def synthetic_notes
label_events_by_discussion_id.map do |discussion_id, events|
LabelNote.from_events(events, resource: resource, resource_parent: resource_parent)
end
end
def label_events_by_discussion_id
return [] unless resource.respond_to?(:resource_label_events)
events = resource.resource_label_events.includes(:label, user: :status) # rubocop: disable CodeReuse/ActiveRecord
events = since_fetch_at(events)
events.group_by { |event| event.discussion_id }
end
end
end
# frozen_string_literal: true
class CreateResourceWeightEvent < ActiveRecord::Migration[5.2]
DOWNTIME = false
def change
create_table :resource_weight_events do |t|
t.references :user, null: false, foreign_key: { on_delete: :nullify },
index: { name: 'index_resource_weight_events_on_user_id' }
t.references :issue, null: false, foreign_key: { on_delete: :cascade },
index: false
t.integer :weight
t.datetime_with_timezone :created_at, null: false
t.index [:issue_id, :weight], name: 'index_resource_weight_events_on_issue_id_and_weight'
end
end
end
......@@ -3605,6 +3605,15 @@ ActiveRecord::Schema.define(version: 2019_12_18_225624) do
t.index ["user_id"], name: "index_resource_label_events_on_user_id"
end
create_table "resource_weight_events", force: :cascade do |t|
t.bigint "user_id", null: false
t.bigint "issue_id", null: false
t.integer "weight"
t.datetime_with_timezone "created_at", null: false
t.index ["issue_id", "weight"], name: "index_resource_weight_events_on_issue_id_and_weight"
t.index ["user_id"], name: "index_resource_weight_events_on_user_id"
end
create_table "reviews", force: :cascade do |t|
t.integer "author_id"
t.integer "merge_request_id", null: false
......@@ -4745,6 +4754,8 @@ ActiveRecord::Schema.define(version: 2019_12_18_225624) do
add_foreign_key "resource_label_events", "labels", on_delete: :nullify
add_foreign_key "resource_label_events", "merge_requests", on_delete: :cascade
add_foreign_key "resource_label_events", "users", on_delete: :nullify
add_foreign_key "resource_weight_events", "issues", on_delete: :cascade
add_foreign_key "resource_weight_events", "users", on_delete: :nullify
add_foreign_key "reviews", "merge_requests", on_delete: :cascade
add_foreign_key "reviews", "projects", on_delete: :cascade
add_foreign_key "reviews", "users", column: "author_id", on_delete: :nullify
......
# frozen_string_literal: true
module EE
module WeightEventable
extend ActiveSupport::Concern
included do
has_many :resource_weight_events
end
end
end
......@@ -13,6 +13,7 @@ module EE
include Elastic::ApplicationVersionedSearch
include UsageStatistics
include WeightEventable
scope :order_weight_desc, -> { reorder ::Gitlab::Database.nulls_last_order('weight', 'DESC') }
scope :order_weight_asc, -> { reorder ::Gitlab::Database.nulls_last_order('weight') }
......
# frozen_string_literal: true
module EE
class WeightNote < ::Note
attr_accessor :resource_parent, :event
def self.from_event(event, resource: nil, resource_parent: nil)
resource ||= event.issue
attrs = {
system: true,
author: event.user,
created_at: event.created_at,
noteable: resource,
event: event,
system_note_metadata: ::SystemNoteMetadata.new(action: 'weight'),
resource_parent: resource_parent
}
if resource_parent.is_a?(Project)
attrs[:project_id] = resource_parent.id
end
WeightNote.new(attrs)
end
def note
@note ||= note_text
end
def note_html
@note_html ||= "<p dir=\"auto\">#{note_text(html: true)}</p>"
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)
weight_text = html ? "<strong>#{event.weight}</strong>" : event.weight
event.weight ? "changed weight to #{weight_text}" : 'removed the weight'
end
end
end
......@@ -11,7 +11,7 @@ module EE
super
ActiveRecord::Base.no_touching do
handle_weight_change_note
handle_weight_change
handle_date_change_note if is_update
end
end
......@@ -28,14 +28,18 @@ module EE
end
end
def handle_weight_change_note
if issuable.previous_changes.include?('weight')
create_weight_change_note
def handle_weight_change
return unless issuable.previous_changes.include?('weight')
if weight_changes_tracking_enabled?
EE::ResourceEvents::ChangeWeightService.new([issuable], current_user, Time.now).execute
else
::SystemNoteService.change_weight_note(issuable, issuable.project, current_user)
end
end
def create_weight_change_note
::SystemNoteService.change_weight_note(issuable, issuable.project, current_user)
def weight_changes_tracking_enabled?
!issuable.is_a?(Epic) && ::Feature.enabled?(:track_issue_weight_change_events, issuable.project)
end
end
end
......
# frozen_string_literal: true
module EE
module ResourceEvents
class ChangeWeightService
attr_reader :resources, :user, :event_created_at
def initialize(resources, user, created_at)
@resources = resources
@user = user
@event_created_at = created_at
end
def execute
unless resource_weight_changes.empty?
::Gitlab::Database.bulk_insert(ResourceWeightEvent.table_name, resource_weight_changes)
resources.each(&:expire_note_etag_cache)
end
end
private
def resource_weight_changes
@weight_changes ||= resources.map do |resource|
changes = []
base_data = { user_id: user.id, issue_id: resource.id }
changes << base_data.merge({ weight: previous_weight(resource), created_at: resource.updated_at }) if first_weight_event?(resource)
changes << base_data.merge({ weight: resource.weight, created_at: event_created_at })
end.flatten
end
def previous_weight(resource)
resource.previous_changes['weight']&.first
end
def first_weight_event?(resource)
previous_weight(resource) && ResourceWeightEvent.by_issue(resource).none?
end
end
end
end
# frozen_string_literal: true
module EE
module ResourceEvents
module MergeIntoNotesService
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
private
override :synthetic_notes
def synthetic_notes
super + SyntheticWeightNotesBuilderService.new(resource, current_user, params).execute
end
end
end
end
# frozen_string_literal: true
# We store events about issue weight changes in a separate table,
# but we still want to display notes about weight changes
# as classic system notes in UI. This service generates "synthetic" notes for
# weight event changes.
module EE
module ResourceEvents
class SyntheticWeightNotesBuilderService < ::ResourceEvents::BaseSyntheticNotesBuilderService
private
def synthetic_notes
weight_change_events.map do |event|
WeightNote.from_event(event, resource: resource, resource_parent: resource_parent)
end
end
def weight_change_events
return [] unless resource.respond_to?(:resource_weight_events)
events = resource.resource_weight_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
since_fetch_at(events)
end
end
end
end
---
title: Track resource weight changes
merge_request: 21515
author:
type: added
......@@ -116,6 +116,7 @@ describe Issue do
it { is_expected.to have_many(:vulnerability_links).class_name('Vulnerabilities::IssueLink').inverse_of(:issue) }
it { is_expected.to have_many(:related_vulnerabilities).through(:vulnerability_links).source(:vulnerability) }
it { is_expected.to belong_to(:promoted_to_epic).class_name('Epic') }
it { is_expected.to have_many(:resource_weight_events) }
describe 'versions.most_recent' do
it 'returns the most recent version' do
......
......@@ -345,10 +345,20 @@ describe API::Issues, :mailer do
expect(json_response['message']['weight']).to be_present
end
it 'adds a note when the weight is changed' do
it 'creates a ResourceWeightEvent' do
expect do
put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
end.to change { ResourceWeightEvent.count }.by(1)
end
it 'does not create a system note' do
expect do
put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
end.to change { Note.count }.by(1)
end.not_to change { Note.count }
end
it 'adds a note when the weight is changed' do
put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
expect(response).to have_gitlab_http_status(200)
expect(json_response['weight']).to eq(9)
......@@ -367,6 +377,24 @@ describe API::Issues, :mailer do
expect(issue.reload.read_attribute(:weight)).to be_nil
end
end
context 'when issue weight tracking feature flag is not active' do
before do
stub_feature_flags(track_issue_weight_change_events: false)
end
it 'does not create a ResourceWeightEvent' do
expect do
put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
end.not_to change { ResourceWeightEvent.count }
end
it 'creates a system note' do
expect do
put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
end.to change { Note.count }.by(1)
end
end
end
describe 'PUT /projects/:id/issues/:issue_id to update epic' do
......
......@@ -8,7 +8,20 @@ describe Issuable::CommonSystemNotesService do
let(:issuable) { create(:issue) }
context 'on issuable update' do
it_behaves_like 'system note creation', { weight: 5 }, 'changed weight to **5**'
context 'when weight is changed' do
before do
issuable.update!(weight: 5)
end
it 'creates a resource label event' do
described_class.new(project, user).execute(issuable, old_labels: [])
event = issuable.reload.resource_weight_events.last
expect(event).not_to be_nil
expect(event.weight).to eq 5
expect(event.user_id).to eq user.id
end
end
context 'when issuable is an epic' do
let(:timestamp) { Time.now }
......@@ -35,12 +48,48 @@ describe Issuable::CommonSystemNotesService do
subject { described_class.new(project, user).execute(issuable, old_labels: [], is_update: false) }
it 'creates a system note for weight' do
before do
issuable.weight = 5
issuable.save
end
it 'does not create a common system note for weight' do
expect { subject }.not_to change { issuable.notes.count }
end
context 'when resource weight event tracking is enabled' do
before do
stub_feature_flags(track_issue_weight_change_events: true)
end
it 'creates a resource weight event' do
subject
expect { subject }.to change { issuable.notes.count }.from(0).to(1)
expect(issuable.notes.last.note).to match('changed weight')
event = issuable.resource_weight_events.last
expect(event.weight).to eq(5)
expect(event.user_id).to eq(user.id)
end
it 'does not create a system note' do
expect { subject }.not_to change { Note.count }
end
end
context 'when resource weight event tracking is disabled' do
before do
stub_feature_flags(track_issue_weight_change_events: false)
end
it 'does not created a resource weight event' do
expect { subject }.not_to change { ResourceWeightEvent.count }
end
it 'does create a system note' do
expect { subject }.to change { Note.count }.from(0).to(1)
expect(Note.first.note).to eq('changed weight to **5**')
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe EE::ResourceEvents::ChangeWeightService do
let_it_be(:user) { create(:user) }
let(:issue) { create(:issue, weight: 3) }
let(:created_at_time) { Time.new(2019, 1, 1, 12, 30, 48) }
subject { described_class.new([issue], user, created_at_time).execute }
before do
ResourceWeightEvent.new(issue: issue, user: user).save!
end
it 'creates the expected event record' do
expect { subject }.to change { ResourceWeightEvent.count }.by(1)
record = ResourceWeightEvent.last
expect_event_record(record, weight: 3, created_at: created_at_time)
end
context 'when weight is nil' do
let(:issue) { create(:issue, weight: nil) }
it 'creates an event record' do
expect { subject }.to change { ResourceWeightEvent.count }.by(1)
record = ResourceWeightEvent.last
expect_event_record(record, weight: nil, created_at: created_at_time)
end
end
context 'when there is no existing weight event record' do
before do
ResourceWeightEvent.delete_all
issue.update(weight: 5)
end
it 'creates the expected event records' do
expect { subject }.to change { ResourceWeightEvent.count }.by(2)
record = ResourceWeightEvent.first
expect_event_record(record, weight: 3, created_at: issue.reload.updated_at)
record = ResourceWeightEvent.last
expect_event_record(record, weight: 5, created_at: created_at_time)
end
end
def expect_event_record(record, weight:, created_at:)
expect(record.issue).to eq(issue)
expect(record.user).to eq(user)
expect(record.weight).to eq(weight)
expect(record.created_at).to eq(created_at)
end
describe 'bulk issue weight updates' do
let(:issues) { create_list(:issue, 3, weight: 1) }
before do
issues.each { |issue| issue.update!(weight: 3) }
end
it 'bulk insert weight changes' do
expect do
described_class.new(issues, user, created_at_time).execute
end.to change { ResourceWeightEvent.count }.by(6)
end
it 'calls first_weight_event? once per resource' do
service = described_class.new(issues, user, created_at_time)
allow(service).to receive(:first_weight_event?).exactly(3).times
service.execute
end
end
end
......@@ -3,13 +3,19 @@
require 'spec_helper'
describe ResourceEvents::MergeIntoNotesService do
def create_event(params)
def create_label_event(params)
event_params = { action: :add, label: label, issue: resource,
user: user }
create(:resource_label_event, event_params.merge(params))
end
def create_weight_event(params, weight = resource.weight)
event_params = { issue: resource, user: user }
create(:resource_weight_event, event_params.merge(params))
end
set(:project) { create(:project) }
set(:user) { create(:user) }
set(:resource) { create(:issue, project: project) }
......@@ -26,15 +32,15 @@ describe ResourceEvents::MergeIntoNotesService do
it 'squashes events with same time and author into single note but scoped labels are separated' do
user2 = create(:user)
create_event(created_at: time)
create_event(created_at: time, label: label2, action: :remove)
create_event(created_at: time, label: scoped_label_group1_1, action: :remove)
create_event(created_at: time, label: scoped_label_group1_2, action: :add)
create_event(created_at: time, label: scoped_label_group2_2, action: :remove)
create_event(created_at: time, label: scoped_label_group2_1, action: :add)
create_event(created_at: time, label: scoped_label_group3_1, action: :add)
create_event(created_at: time, user: user2)
create_event(created_at: 1.day.ago, label: label2)
create_label_event(created_at: time)
create_label_event(created_at: time, label: label2, action: :remove)
create_label_event(created_at: time, label: scoped_label_group1_1, action: :remove)
create_label_event(created_at: time, label: scoped_label_group1_2, action: :add)
create_label_event(created_at: time, label: scoped_label_group2_2, action: :remove)
create_label_event(created_at: time, label: scoped_label_group2_1, action: :add)
create_label_event(created_at: time, label: scoped_label_group3_1, action: :add)
create_label_event(created_at: time, user: user2)
create_label_event(created_at: 1.day.ago, label: label2)
notes = described_class.new(resource, user).execute
......@@ -56,10 +62,10 @@ describe ResourceEvents::MergeIntoNotesService do
context 'scoped labels' do
context 'when all labels are automatically removed' do
it 'adds "automatically removed" message' do
create_event(created_at: time, label: scoped_label_group1_1, action: :add)
create_event(created_at: time, label: scoped_label_group1_2, action: :remove)
create_event(created_at: time, label: scoped_label_group2_1, action: :add)
create_event(created_at: time, label: scoped_label_group2_2, action: :remove)
create_label_event(created_at: time, label: scoped_label_group1_1, action: :add)
create_label_event(created_at: time, label: scoped_label_group1_2, action: :remove)
create_label_event(created_at: time, label: scoped_label_group2_1, action: :add)
create_label_event(created_at: time, label: scoped_label_group2_2, action: :remove)
note = described_class.new(resource, user).execute.first.note
......@@ -72,9 +78,9 @@ describe ResourceEvents::MergeIntoNotesService do
context 'when any of the labels is manually removed' do
it 'adds "removed" message' do
create_event(created_at: time, label: scoped_label_group1_1, action: :add)
create_event(created_at: time, label: scoped_label_group1_2, action: :remove)
create_event(created_at: time, label: scoped_label_group2_1, action: :remove)
create_label_event(created_at: time, label: scoped_label_group1_1, action: :add)
create_label_event(created_at: time, label: scoped_label_group1_2, action: :remove)
create_label_event(created_at: time, label: scoped_label_group2_1, action: :remove)
note = described_class.new(resource, user).execute.first.note
......@@ -85,5 +91,21 @@ describe ResourceEvents::MergeIntoNotesService do
end
end
end
context 'with weight events' do
it 'includes the expected notes' do
create_weight_event(created_at: time, weight: 3)
create_weight_event(created_at: time, weight: 1)
create_weight_event(created_at: time, weight: 5)
notes = described_class.new(resource, user).execute
expect(notes.size).to eq(3)
expect(notes[0].note).to eq('changed weight to 3')
expect(notes[1].note).to eq('changed weight to 1')
expect(notes[2].note).to eq('changed weight to 5')
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe EE::ResourceEvents::SyntheticWeightNotesBuilderService do
describe '#execute' do
let!(:user) { create(:user) }
let!(:issue) { create(:issue, author: user) }
let!(:event1) { create(:resource_weight_event, issue: issue) }
let!(:event2) { create(:resource_weight_event, issue: issue) }
let!(:event3) { create(:resource_weight_event, issue: issue) }
it 'returns the expected synthetic notes' do
notes = EE::ResourceEvents::SyntheticWeightNotesBuilderService.new(issue, user).execute
expect(notes.size).to eq(3)
end
end
end
# frozen_string_literal: true
FactoryBot.define do
factory :resource_weight_event do
issue { create(:issue) }
user { issue&.author || create(:user) }
end
end
......@@ -8,6 +8,7 @@ issues:
- milestone
- notes
- resource_label_events
- resource_weight_events
- sentry_issue
- label_links
- labels
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe ResourceWeightEvent, type: :model 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) }
it { is_expected.not_to allow_value(nil).for(:issue) }
it { is_expected.to allow_value(nil).for(:weight) }
end
describe 'associations' do
it { is_expected.to belong_to(:user) }
it { is_expected.to belong_to(:issue) }
end
describe '.by_issue' do
let_it_be(:event1) { create(:resource_weight_event, issue: issue1) }
let_it_be(:event2) { create(:resource_weight_event, issue: issue2) }
let_it_be(:event3) { create(:resource_weight_event, issue: issue1) }
it 'returns the expected records for an issue with events' do
events = ResourceWeightEvent.by_issue(issue1)
expect(events).to contain_exactly(event1, event3)
end
it 'returns the expected records for an issue with no events' do
events = ResourceWeightEvent.by_issue(issue3)
expect(events).to be_empty
end
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(:resource_weight_event, issue: issue1, created_at: created_at1) }
let!(:event2) { create(:resource_weight_event, issue: issue2, created_at: created_at2) }
let!(:event3) { create(:resource_weight_event, issue: issue2, created_at: created_at3) }
it 'returns the expected events' do
events = ResourceWeightEvent.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 = ResourceWeightEvent.created_after(1.minute.ago)
expect(events).to be_empty
end
end
describe '#discussion_id' do
let_it_be(:event) { create(:resource_weight_event, issue: issue1, created_at: Time.utc(2019, 12, 30)) }
it 'returns the expected id' do
allow(Digest::SHA1).to receive(:hexdigest)
.with("ResourceWeightEvent-2019-12-30 00:00:00 UTC-#{user1.id}")
.and_return('73d167c478')
expect(event.discussion_id).to eq('73d167c478')
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ResourceEvents::SyntheticLabelNotesBuilderService do
describe '#execute' do
let!(:user) { create(:user) }
let!(:issue) { create(:issue, author: user) }
let!(:event1) { create(:resource_label_event, issue: issue) }
let!(:event2) { create(:resource_label_event, issue: issue) }
let!(:event3) { create(:resource_label_event, issue: issue) }
it 'returns the expected synthetic notes' do
notes = ResourceEvents::SyntheticLabelNotesBuilderService.new(issue, user).execute
expect(notes.size).to eq(3)
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