summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorOswaldo Ferreira <oswaldo@gitlab.com>2017-12-07 15:41:30 -0200
committerOswaldo Ferreira <oswaldo@gitlab.com>2018-01-02 17:45:25 -0200
commitbf8c20729baffbf55605484230ff33680d72139b (patch)
tree70d9bea9ea2a37dc8cd8c37cda396fcf246db72b
parent2cbb2d0eceaed0f31c92d4eed8932e98f4f74559 (diff)
downloadgitlab-ce-bf8c20729baffbf55605484230ff33680d72139b.tar.gz
Cache merged and closed events data in merge_request_metrics table
-rw-r--r--app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js6
-rw-r--r--app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js6
-rw-r--r--app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js42
-rw-r--r--app/models/concerns/issuable.rb11
-rw-r--r--app/models/issue.rb5
-rw-r--r--app/models/merge_request/metrics.rb10
-rw-r--r--app/serializers/event_entity.rb4
-rw-r--r--app/serializers/merge_request_metrics_entity.rb6
-rw-r--r--app/serializers/merge_request_widget_entity.rb32
-rw-r--r--app/services/event_create_service.rb2
-rw-r--r--app/services/merge_request_metrics_service.rb19
-rw-r--r--app/services/merge_requests/base_service.rb4
-rw-r--r--app/services/merge_requests/close_service.rb13
-rw-r--r--app/services/merge_requests/post_merge_service.rb11
-rw-r--r--app/services/merge_requests/reopen_service.rb13
-rw-r--r--changelogs/unreleased/osw-introduce-merge-request-statistics.yml5
-rw-r--r--db/migrate/20171127151038_add_events_related_columns_to_merge_request_metrics.rb37
-rw-r--r--db/post_migrate/20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb68
-rw-r--r--db/schema.rb5
-rw-r--r--lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data.rb135
-rw-r--r--spec/fixtures/api/schemas/entities/merge_request_metrics.json21
-rw-r--r--spec/fixtures/api/schemas/entities/merge_request_widget.json6
-rw-r--r--spec/fixtures/api/schemas/entities/user.json17
-rw-r--r--spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js13
-rw-r--r--spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js11
-rw-r--r--spec/javascripts/vue_mr_widget/mock_data.js9
-rw-r--r--spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb124
-rw-r--r--spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb24
-rw-r--r--spec/models/issue_spec.rb26
-rw-r--r--spec/models/merge_request/metrics_spec.rb15
-rw-r--r--spec/models/merge_request_spec.rb19
-rw-r--r--spec/requests/api/notes_spec.rb2
-rw-r--r--spec/requests/api/project_milestones_spec.rb2
-rw-r--r--spec/requests/api/v3/milestones_spec.rb2
-rw-r--r--spec/requests/api/v3/notes_spec.rb2
-rw-r--r--spec/serializers/event_entity_spec.rb13
-rw-r--r--spec/serializers/merge_request_widget_entity_spec.rb74
-rw-r--r--spec/services/create_deployment_service_spec.rb2
-rw-r--r--spec/services/merge_requests/close_service_spec.rb13
-rw-r--r--spec/services/merge_requests/post_merge_service_spec.rb13
-rw-r--r--spec/services/merge_requests/reopen_service_spec.rb13
-rw-r--r--spec/services/update_merge_request_metrics_service_spec.rb42
42 files changed, 799 insertions, 98 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js
index b25cc3443ef..dd8b2665b1d 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js
@@ -16,9 +16,9 @@ export default {
<div class="media-body">
<mr-widget-author-and-time
actionText="Closed by"
- :author="mr.closedEvent.author"
- :dateTitle="mr.closedEvent.updatedAt"
- :dateReadable="mr.closedEvent.formattedUpdatedAt"
+ :author="mr.metrics.closedBy"
+ :dateTitle="mr.metrics.closedAt"
+ :dateReadable="mr.metrics.readableClosedAt"
/>
<section class="mr-info-list">
<p>
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js
index 2dfd87ed904..2a421192568 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js
@@ -68,9 +68,9 @@ export default {
<div class="space-children">
<mr-widget-author-and-time
actionText="Merged by"
- :author="mr.mergedEvent.author"
- :date-title="mr.mergedEvent.updatedAt"
- :date-readable="mr.mergedEvent.formattedUpdatedAt" />
+ :author="mr.metrics.mergedBy"
+ :date-title="mr.metrics.mergedAt"
+ :date-readable="mr.metrics.readableMergedAt" />
<a
v-if="mr.canRevertInCurrentMR"
v-tooltip
diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
index 93d31a2a684..3b0672cb936 100644
--- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
+++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
@@ -39,9 +39,8 @@ export default class MergeRequestStore {
}
this.updatedAt = data.updated_at;
- this.mergedEvent = MergeRequestStore.getEventObject(data.merge_event);
- this.closedEvent = MergeRequestStore.getEventObject(data.closed_event);
- this.setToMWPSBy = MergeRequestStore.getAuthorObject({ author: data.merge_user || {} });
+ this.metrics = MergeRequestStore.buildMetrics(data.metrics);
+ this.setToMWPSBy = MergeRequestStore.formatUserObject(data.merge_user || {});
this.mergeUserId = data.merge_user_id;
this.currentUserId = gon.current_user_id;
this.sourceBranchPath = data.source_branch_path;
@@ -125,43 +124,38 @@ export default class MergeRequestStore {
return this.state === stateKey.nothingToMerge;
}
- static getEventObject(event) {
+ static buildMetrics(metrics) {
return {
- author: MergeRequestStore.getAuthorObject(event),
- updatedAt: formatDate(MergeRequestStore.getEventUpdatedAtDate(event)),
- formattedUpdatedAt: MergeRequestStore.getEventDate(event),
+ mergedBy: MergeRequestStore.formatUserObject(metrics.merged_by),
+ closedBy: MergeRequestStore.formatUserObject(metrics.closed_by),
+ mergedAt: formatDate(metrics.merged_at),
+ closedAt: formatDate(metrics.closed_at),
+ readableMergedAt: MergeRequestStore.getReadableDate(metrics.merged_at),
+ readableClosedAt: MergeRequestStore.getReadableDate(metrics.closed_at),
};
}
- static getAuthorObject(event) {
- if (!event) {
+ static formatUserObject(user) {
+ if (!user) {
return {};
}
return {
- name: event.author.name || '',
- username: event.author.username || '',
- webUrl: event.author.web_url || '',
- avatarUrl: event.author.avatar_url || '',
+ name: user.name || '',
+ username: user.username || '',
+ webUrl: user.web_url || '',
+ avatarUrl: user.avatar_url || '',
};
}
- static getEventUpdatedAtDate(event) {
- if (!event) {
+ static getReadableDate(date) {
+ if (!date) {
return '';
}
- return event.updated_at;
- }
-
- static getEventDate(event) {
const timeagoInstance = new Timeago();
- if (!event) {
- return '';
- }
-
- return timeagoInstance.format(MergeRequestStore.getEventUpdatedAtDate(event));
+ return timeagoInstance.format(date);
}
}
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 5ca4a7086cb..4251561a0a0 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -96,7 +96,7 @@ module Issuable
strip_attributes :title
- after_save :record_metrics, unless: :imported?
+ after_save :ensure_metrics, unless: :imported?
# We want to use optimistic lock for cases when only title or description are involved
# http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html
@@ -335,11 +335,6 @@ module Issuable
false
end
- def record_metrics
- metrics = self.metrics || create_metrics
- metrics.record!
- end
-
##
# Override in issuable specialization
#
@@ -347,6 +342,10 @@ module Issuable
false
end
+ def ensure_metrics
+ self.metrics || create_metrics
+ end
+
##
# Overriden in MergeRequest
#
diff --git a/app/models/issue.rb b/app/models/issue.rb
index dc64888b6fc..4eafc1316d6 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -276,6 +276,11 @@ class Issue < ActiveRecord::Base
private
+ def ensure_metrics
+ super
+ metrics.record!
+ end
+
# Returns `true` if the given User can read the current Issue.
#
# This method duplicates the same check of issue_policy.rb
diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb
index cdc408738be..9e660eccd86 100644
--- a/app/models/merge_request/metrics.rb
+++ b/app/models/merge_request/metrics.rb
@@ -1,12 +1,6 @@
class MergeRequest::Metrics < ActiveRecord::Base
belongs_to :merge_request
belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
-
- def record!
- if merge_request.merged? && self.merged_at.blank?
- self.merged_at = Time.now
- end
-
- self.save
- end
+ belongs_to :latest_closed_by, class_name: 'User'
+ belongs_to :merged_by, class_name: 'User'
end
diff --git a/app/serializers/event_entity.rb b/app/serializers/event_entity.rb
deleted file mode 100644
index 935d67a4f37..00000000000
--- a/app/serializers/event_entity.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-class EventEntity < Grape::Entity
- expose :author, using: UserEntity
- expose :updated_at
-end
diff --git a/app/serializers/merge_request_metrics_entity.rb b/app/serializers/merge_request_metrics_entity.rb
new file mode 100644
index 00000000000..3548107ac16
--- /dev/null
+++ b/app/serializers/merge_request_metrics_entity.rb
@@ -0,0 +1,6 @@
+class MergeRequestMetricsEntity < Grape::Entity
+ expose :latest_closed_at, as: :closed_at
+ expose :merged_at
+ expose :latest_closed_by, as: :closed_by, using: UserEntity
+ expose :merged_by, using: UserEntity
+end
diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb
index f8e59b2ffd7..83ea36adcc1 100644
--- a/app/serializers/merge_request_widget_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -17,9 +17,11 @@ class MergeRequestWidgetEntity < IssuableEntity
merge_request.project.merge_requests_ff_only_enabled
end
- # Events
- expose :merge_event, using: EventEntity
- expose :closed_event, using: EventEntity
+ expose :metrics do |merge_request|
+ metrics = build_metrics(merge_request)
+
+ MergeRequestMetricsEntity.new(metrics).as_json
+ end
# User entities
expose :merge_user, using: UserEntity
@@ -178,4 +180,28 @@ class MergeRequestWidgetEntity < IssuableEntity
@presenters ||= {}
@presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user)
end
+
+ # Once SchedulePopulateMergeRequestMetricsWithEventsData fully runs,
+ # we can remove this method and just serialize MergeRequest#metrics
+ # instead. See https://gitlab.com/gitlab-org/gitlab-ce/issues/41587
+ def build_metrics(merge_request)
+ # There's no need to query and serialize metrics data for merge requests that are not
+ # merged or closed.
+ case merge_request.state
+ when 'merged'
+ merge_request.metrics&.merged_by_id ? merge_request.metrics : build_metrics_from_events(merge_request)
+ when 'closed'
+ merge_request.metrics&.latest_closed_by_id ? merge_request.metrics : build_metrics_from_events(merge_request)
+ end
+ end
+
+ def build_metrics_from_events(merge_request)
+ closed_event = merge_request.closed_event
+ merge_event = merge_request.merge_event
+
+ MergeRequest::Metrics.new(latest_closed_at: closed_event&.updated_at,
+ latest_closed_by: closed_event&.author,
+ merged_at: merge_event&.updated_at,
+ merged_by: merge_event&.author)
+ end
end
diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb
index 6328d567a07..44dc90b3462 100644
--- a/app/services/event_create_service.rb
+++ b/app/services/event_create_service.rb
@@ -103,6 +103,6 @@ class EventCreateService
author_id: current_user.id
)
- Event.create(attributes)
+ Event.create!(attributes)
end
end
diff --git a/app/services/merge_request_metrics_service.rb b/app/services/merge_request_metrics_service.rb
new file mode 100644
index 00000000000..9248de14a53
--- /dev/null
+++ b/app/services/merge_request_metrics_service.rb
@@ -0,0 +1,19 @@
+class MergeRequestMetricsService
+ delegate :update!, to: :@merge_request_metrics
+
+ def initialize(merge_request_metrics)
+ @merge_request_metrics = merge_request_metrics
+ end
+
+ def merge(event)
+ update!(merged_by_id: event.author_id, merged_at: event.created_at)
+ end
+
+ def close(event)
+ update!(latest_closed_by_id: event.author_id, latest_closed_at: event.created_at)
+ end
+
+ def reopen
+ update!(latest_closed_by_id: nil, latest_closed_at: nil)
+ end
+end
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 6b32d65a74b..20a2b50d3de 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -24,6 +24,10 @@ module MergeRequests
private
+ def merge_request_metrics_service(merge_request)
+ MergeRequestMetricsService.new(merge_request.metrics)
+ end
+
def create_assignee_note(merge_request)
SystemNoteService.change_assignee(
merge_request, merge_request.project, current_user, merge_request.assignee)
diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb
index 40213c99014..f727ec002e7 100644
--- a/app/services/merge_requests/close_service.rb
+++ b/app/services/merge_requests/close_service.rb
@@ -8,7 +8,7 @@ module MergeRequests
merge_request.allow_broken = true
if merge_request.close
- event_service.close_mr(merge_request, current_user)
+ create_event(merge_request)
create_note(merge_request)
notification_service.close_mr(merge_request, current_user)
todo_service.close_merge_request(merge_request, current_user)
@@ -19,5 +19,16 @@ module MergeRequests
merge_request
end
+
+ private
+
+ def create_event(merge_request)
+ # Making sure MergeRequest::Metrics updates are in sync with
+ # Event creation.
+ Event.transaction do
+ close_event = event_service.close_mr(merge_request, current_user)
+ merge_request_metrics_service(merge_request).close(close_event)
+ end
+ end
end
end
diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index b1d6bac4d4a..c78e78afcd1 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/app/services/merge_requests/post_merge_service.rb
@@ -9,7 +9,7 @@ module MergeRequests
close_issues(merge_request)
todo_service.merge_merge_request(merge_request, current_user)
merge_request.mark_as_merged
- create_merge_event(merge_request, current_user)
+ create_event(merge_request)
create_note(merge_request)
notification_service.merge_mr(merge_request, current_user)
execute_hooks(merge_request, 'merge')
@@ -34,5 +34,14 @@ module MergeRequests
def create_merge_event(merge_request, current_user)
EventCreateService.new.merge_mr(merge_request, current_user)
end
+
+ def create_event(merge_request)
+ # Making sure MergeRequest::Metrics updates are in sync with
+ # Event creation.
+ Event.transaction do
+ merge_event = create_merge_event(merge_request, current_user)
+ merge_request_metrics_service(merge_request).merge(merge_event)
+ end
+ end
end
end
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index c599a90f9fe..120677a7149 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -4,7 +4,7 @@ module MergeRequests
return merge_request unless can?(current_user, :update_merge_request, merge_request)
if merge_request.reopen
- event_service.reopen_mr(merge_request, current_user)
+ create_event(merge_request)
create_note(merge_request, 'reopened')
notification_service.reopen_mr(merge_request, current_user)
execute_hooks(merge_request, 'reopen')
@@ -16,5 +16,16 @@ module MergeRequests
merge_request
end
+
+ private
+
+ def create_event(merge_request)
+ # Making sure MergeRequest::Metrics updates are in sync with
+ # Event creation.
+ Event.transaction do
+ event_service.reopen_mr(merge_request, current_user)
+ merge_request_metrics_service(merge_request).reopen
+ end
+ end
end
end
diff --git a/changelogs/unreleased/osw-introduce-merge-request-statistics.yml b/changelogs/unreleased/osw-introduce-merge-request-statistics.yml
new file mode 100644
index 00000000000..fed7c2141fb
--- /dev/null
+++ b/changelogs/unreleased/osw-introduce-merge-request-statistics.yml
@@ -0,0 +1,5 @@
+---
+title: Cache merged and closed events data in merge_request_metrics table
+merge_request:
+author:
+type: performance
diff --git a/db/migrate/20171127151038_add_events_related_columns_to_merge_request_metrics.rb b/db/migrate/20171127151038_add_events_related_columns_to_merge_request_metrics.rb
new file mode 100644
index 00000000000..18af697cf88
--- /dev/null
+++ b/db/migrate/20171127151038_add_events_related_columns_to_merge_request_metrics.rb
@@ -0,0 +1,37 @@
+class AddEventsRelatedColumnsToMergeRequestMetrics < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ change_table :merge_request_metrics do |t|
+ t.references :merged_by, references: :users
+ t.references :latest_closed_by, references: :users
+ end
+
+ add_column :merge_request_metrics, :latest_closed_at, :datetime_with_timezone
+
+ add_concurrent_foreign_key :merge_request_metrics, :users,
+ column: :merged_by_id,
+ on_delete: :nullify
+
+ add_concurrent_foreign_key :merge_request_metrics, :users,
+ column: :latest_closed_by_id,
+ on_delete: :nullify
+ end
+
+ def down
+ if foreign_keys_for(:merge_request_metrics, :merged_by_id).any?
+ remove_foreign_key :merge_request_metrics, column: :merged_by_id
+ end
+
+ if foreign_keys_for(:merge_request_metrics, :latest_closed_by_id).any?
+ remove_foreign_key :merge_request_metrics, column: :latest_closed_by_id
+ end
+
+ remove_columns :merge_request_metrics,
+ :merged_by_id, :latest_closed_by_id, :latest_closed_at
+ end
+end
diff --git a/db/post_migrate/20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb b/db/post_migrate/20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb
new file mode 100644
index 00000000000..547cc68e10e
--- /dev/null
+++ b/db/post_migrate/20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb
@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+# rubocop:disable GitlabSecurity/SqlInjection
+
+class SchedulePopulateMergeRequestMetricsWithEventsData < ActiveRecord::Migration
+ DOWNTIME = false
+ BATCH_SIZE = 10_000
+ MIGRATION = 'PopulateMergeRequestMetricsWithEventsData'
+
+ disable_ddl_transaction!
+
+ class MergeRequest < ActiveRecord::Base
+ self.table_name = 'merge_requests'
+
+ include ::EachBatch
+ end
+
+ def up
+ merge_requests = MergeRequest.where("id IN (#{updatable_merge_requests_union_sql})").reorder(:id)
+
+ say 'Scheduling `PopulateMergeRequestMetricsWithEventsData` jobs'
+ # It will update around 4_000_000 records in batches of 10_000 merge
+ # requests (running between 10 minutes) and should take around 66 hours to complete.
+ # Apparently, production PostgreSQL is able to vacuum 10k-20k dead_tuples by
+ # minute, and at maximum, each of these jobs should UPDATE 20k records.
+ #
+ # More information about the updates in `PopulateMergeRequestMetricsWithEventsData` class.
+ #
+ merge_requests.each_batch(of: BATCH_SIZE) do |relation, index|
+ range = relation.pluck('MIN(id)', 'MAX(id)').first
+
+ BackgroundMigrationWorker.perform_in(index * 10.minutes, MIGRATION, range)
+ end
+ end
+
+ def down
+ execute "update merge_request_metrics set latest_closed_at = null"
+ execute "update merge_request_metrics set latest_closed_by_id = null"
+ execute "update merge_request_metrics set merged_by_id = null"
+ end
+
+ private
+
+ # On staging:
+ # Planning time: 0.682 ms
+ # Execution time: 22033.158 ms
+ #
+ def updatable_merge_requests_union_sql
+ metrics_not_exists_clause =
+ 'NOT EXISTS (SELECT 1 FROM merge_request_metrics WHERE merge_request_metrics.merge_request_id = merge_requests.id)'
+
+ without_metrics_data = <<-SQL.strip_heredoc
+ merge_request_metrics.merged_by_id IS NULL OR
+ merge_request_metrics.latest_closed_by_id IS NULL OR
+ merge_request_metrics.latest_closed_at IS NULL
+ SQL
+
+ mrs_without_metrics_record = MergeRequest
+ .where(metrics_not_exists_clause)
+ .select(:id)
+
+ mrs_without_events_data = MergeRequest
+ .joins('INNER JOIN merge_request_metrics ON merge_requests.id = merge_request_metrics.merge_request_id')
+ .where(without_metrics_data)
+ .select(:id)
+
+ Gitlab::SQL::Union.new([mrs_without_metrics_record, mrs_without_events_data]).to_sql
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 42715d5e1e8..9d3b5db117e 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1056,6 +1056,9 @@ ActiveRecord::Schema.define(version: 20171220191323) do
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "pipeline_id"
+ t.integer "merged_by_id"
+ t.integer "latest_closed_by_id"
+ t.datetime_with_timezone "latest_closed_at"
end
add_index "merge_request_metrics", ["first_deployed_to_production_at"], name: "index_merge_request_metrics_on_first_deployed_to_production_at", using: :btree
@@ -1995,6 +1998,8 @@ ActiveRecord::Schema.define(version: 20171220191323) do
add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
+ add_foreign_key "merge_request_metrics", "users", column: "latest_closed_by_id", name: "fk_ae440388cc", on_delete: :nullify
+ add_foreign_key "merge_request_metrics", "users", column: "merged_by_id", name: "fk_7f28d925f3", on_delete: :nullify
add_foreign_key "merge_requests", "ci_pipelines", column: "head_pipeline_id", name: "fk_fd82eae0b9", on_delete: :nullify
add_foreign_key "merge_requests", "merge_request_diffs", column: "latest_merge_request_diff_id", name: "fk_06067f5644", on_delete: :nullify
add_foreign_key "merge_requests", "milestones", name: "fk_6a5165a692", on_delete: :nullify
diff --git a/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data.rb b/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data.rb
new file mode 100644
index 00000000000..8a901a9bf39
--- /dev/null
+++ b/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data.rb
@@ -0,0 +1,135 @@
+# frozen_string_literal: true
+# rubocop:disable Metrics/LineLength
+# rubocop:disable Metrics/MethodLength
+# rubocop:disable Metrics/ClassLength
+# rubocop:disable Style/Documentation
+
+module Gitlab
+ module BackgroundMigration
+ class PopulateMergeRequestMetricsWithEventsData
+ def perform(min_merge_request_id, max_merge_request_id)
+ insert_metrics_for_range(min_merge_request_id, max_merge_request_id)
+ update_metrics_with_events_data(min_merge_request_id, max_merge_request_id)
+ end
+
+ # Inserts merge_request_metrics records for merge_requests without it for
+ # a given merge request batch.
+ def insert_metrics_for_range(min, max)
+ metrics_not_exists_clause =
+ <<-SQL.strip_heredoc
+ NOT EXISTS (SELECT 1 FROM merge_request_metrics
+ WHERE merge_request_metrics.merge_request_id = merge_requests.id)
+ SQL
+
+ MergeRequest.where(metrics_not_exists_clause).where(id: min..max).each_batch do |batch|
+ select_sql = batch.select(:id, :created_at, :updated_at).to_sql
+
+ execute("INSERT INTO merge_request_metrics (merge_request_id, created_at, updated_at) #{select_sql}")
+ end
+ end
+
+ def update_metrics_with_events_data(min, max)
+ if Gitlab::Database.postgresql?
+ # Uses WITH syntax in order to update merged and closed events with a single UPDATE.
+ # WITH is not supported by MySQL.
+ update_events_for_range(min, max)
+ else
+ update_merged_events_for_range(min, max)
+ update_closed_events_for_range(min, max)
+ end
+ end
+
+ private
+
+ # Updates merge_request_metrics latest_closed_at, latest_closed_by_id and merged_by_id
+ # based on the latest event records on events table for a given merge request batch.
+ def update_events_for_range(min, max)
+ sql = <<-SQL.strip_heredoc
+ WITH events_for_update AS (
+ SELECT DISTINCT ON (target_id, action) target_id, action, author_id, updated_at
+ FROM events
+ WHERE target_id BETWEEN #{min} AND #{max}
+ AND target_type = 'MergeRequest'
+ AND action IN (#{Event::CLOSED},#{Event::MERGED})
+ ORDER BY target_id, action, id DESC
+ )
+ UPDATE merge_request_metrics met
+ SET latest_closed_at = latest_closed.updated_at,
+ latest_closed_by_id = latest_closed.author_id,
+ merged_by_id = latest_merged.author_id
+ FROM (SELECT * FROM events_for_update WHERE action = #{Event::CLOSED}) AS latest_closed
+ FULL OUTER JOIN
+ (SELECT * FROM events_for_update WHERE action = #{Event::MERGED}) AS latest_merged
+ USING (target_id)
+ WHERE target_id = merge_request_id;
+ SQL
+
+ execute(sql)
+ end
+
+ # Updates merge_request_metrics latest_closed_at, latest_closed_by_id based on the latest closed
+ # records on events table for a given merge request batch.
+ def update_closed_events_for_range(min, max)
+ sql =
+ <<-SQL.strip_heredoc
+ UPDATE merge_request_metrics metrics,
+ (#{select_events(min, max, Event::CLOSED)}) closed_events
+ SET metrics.latest_closed_by_id = closed_events.author_id,
+ metrics.latest_closed_at = closed_events.updated_at #{where_matches_closed_events};
+ SQL
+
+ execute(sql)
+ end
+
+ # Updates merge_request_metrics merged_by_id based on the latest merged
+ # records on events table for a given merge request batch.
+ def update_merged_events_for_range(min, max)
+ sql =
+ <<-SQL.strip_heredoc
+ UPDATE merge_request_metrics metrics,
+ (#{select_events(min, max, Event::MERGED)}) merged_events
+ SET metrics.merged_by_id = merged_events.author_id #{where_matches_merged_events};
+ SQL
+
+ execute(sql)
+ end
+
+ def execute(sql)
+ @connection ||= ActiveRecord::Base.connection
+ @connection.execute(sql)
+ end
+
+ def select_events(min, max, action)
+ select_max_event_id = <<-SQL.strip_heredoc
+ SELECT max(id)
+ FROM events
+ WHERE action = #{action}
+ AND target_type = 'MergeRequest'
+ AND target_id BETWEEN #{min} AND #{max}
+ GROUP BY target_id
+ SQL
+
+ <<-SQL.strip_heredoc
+ SELECT author_id, updated_at, target_id
+ FROM events
+ WHERE id IN(#{select_max_event_id})
+ SQL
+ end
+
+ def where_matches_closed_events
+ <<-SQL.strip_heredoc
+ WHERE metrics.merge_request_id = closed_events.target_id
+ AND metrics.latest_closed_at IS NULL
+ AND metrics.latest_closed_by_id IS NULL
+ SQL
+ end
+
+ def where_matches_merged_events
+ <<-SQL.strip_heredoc
+ WHERE metrics.merge_request_id = merged_events.target_id
+ AND metrics.merged_by_id IS NULL
+ SQL
+ end
+ end
+ end
+end
diff --git a/spec/fixtures/api/schemas/entities/merge_request_metrics.json b/spec/fixtures/api/schemas/entities/merge_request_metrics.json
new file mode 100644
index 00000000000..3fa767f85df
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/merge_request_metrics.json
@@ -0,0 +1,21 @@
+{
+ "type": "object",
+ "required": ["closed_at", "merged_at", "closed_by", "merged_by"],
+ "properties" : {
+ "closed_at": { "type": ["datetime", "null"] },
+ "merged_at": { "type": ["datetime", "null"] },
+ "closed_by": {
+ "oneOf": [
+ { "type": "null" },
+ { "$ref": "user.json" }
+ ]
+ },
+ "merged_by": {
+ "oneOf": [
+ { "type": "null" },
+ { "$ref": "user.json" }
+ ]
+ }
+ },
+ "additionalProperties": false
+}
diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json
index 342890c3dee..dad415e7d81 100644
--- a/spec/fixtures/api/schemas/entities/merge_request_widget.json
+++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json
@@ -31,8 +31,10 @@
"source_project_id": { "type": "integer" },
"target_branch": { "type": "string" },
"target_project_id": { "type": "integer" },
- "merge_event": { "type": ["object", "null"] },
- "closed_event": { "type": ["object", "null"] },
+ "metrics": {
+ "type": "object",
+ "$ref": "merge_request_metrics.json"
+ },
"author": { "type": ["object", "null"] },
"merge_user": { "type": ["object", "null"] },
"diff_head_sha": { "type": ["string", "null"] },
diff --git a/spec/fixtures/api/schemas/entities/user.json b/spec/fixtures/api/schemas/entities/user.json
new file mode 100644
index 00000000000..6482e0eedd2
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/user.json
@@ -0,0 +1,17 @@
+{
+ "type": "object",
+ "required": [
+ "id",
+ "state",
+ "avatar_url",
+ "web_url",
+ "path"
+ ],
+ "properties": {
+ "id": { "type": "integer" },
+ "state": { "type": "string" },
+ "avatar_url": { "type": "string" },
+ "web_url": { "type": "string" },
+ "path": { "type": "string" }
+ }
+}
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js
index d23b558f4ea..1bf97bbf093 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js
@@ -4,13 +4,16 @@ import closedComponent from '~/vue_merge_request_widget/components/states/mr_wid
const mr = {
targetBranch: 'good-branch',
targetBranchPath: '/good-branch',
- closedEvent: {
- author: {
+ metrics: {
+ mergedBy: {},
+ mergedAt: 'mergedUpdatedAt',
+ closedBy: {
name: 'Fatih Acet',
username: 'fatihacet',
},
- updatedAt: 'closedEventUpdatedAt',
- formattedUpdatedAt: '',
+ closedAt: 'closedEventUpdatedAt',
+ readableMergedAt: '',
+ readableClosedAt: '',
},
updatedAt: 'mrUpdatedAt',
closedAt: '1 day ago',
@@ -56,7 +59,7 @@ describe('MRWidgetClosed', () => {
it('should have correct elements', () => {
expect(el.querySelector('h4').textContent).toContain('Closed by');
- expect(el.querySelector('h4').textContent).toContain(mr.closedEvent.author.name);
+ expect(el.querySelector('h4').textContent).toContain(mr.metrics.closedBy.name);
expect(el.textContent).toContain('The changes were not merged into');
expect(el.querySelector('.label-branch').getAttribute('href')).toEqual(mr.targetBranchPath);
expect(el.querySelector('.label-branch').textContent).toContain(mr.targetBranch);
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js
index 2714e8294fa..90972469630 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js
@@ -14,10 +14,13 @@ const createComponent = () => {
canRevertInCurrentMR: true,
canRemoveSourceBranch: true,
sourceBranchRemoved: true,
- mergedEvent: {
- author: {},
- updatedAt: 'mergedUpdatedAt',
- formattedUpdatedAt: '',
+ metrics: {
+ mergedBy: {},
+ mergedAt: 'mergedUpdatedAt',
+ readableMergedAt: '',
+ closedBy: {},
+ closedAt: 'mergedUpdatedAt',
+ readableClosedAt: '',
},
updatedAt: 'mrUpdatedAt',
targetBranch,
diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js
index 1ad7c2d8efa..ca29c9fee32 100644
--- a/spec/javascripts/vue_mr_widget/mock_data.js
+++ b/spec/javascripts/vue_mr_widget/mock_data.js
@@ -33,8 +33,8 @@ export default {
"source_project_id": 19,
"target_branch": "master",
"target_project_id": 19,
- "merge_event": {
- "author": {
+ "metrics": {
+ "merged_by": {
"name": "Administrator",
"username": "root",
"id": 1,
@@ -42,9 +42,10 @@ export default {
"avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
- "updated_at": "2017-04-07T15:39:25.696Z"
+ "merged_at": "2017-04-07T15:39:25.696Z",
+ "closed_by": null,
+ "closed_at": null
},
- "closed_event": null,
"author": {
"name": "Administrator",
"username": "root",
diff --git a/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb b/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb
new file mode 100644
index 00000000000..dfe3b31f1c0
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb
@@ -0,0 +1,124 @@
+require 'rails_helper'
+
+describe Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData, :migration, schema: 20171128214150 do
+ describe '#perform' do
+ let(:mr_with_event) { create(:merge_request) }
+ let!(:merged_event) { create(:event, :merged, target: mr_with_event) }
+ let!(:closed_event) { create(:event, :closed, target: mr_with_event) }
+
+ before do
+ # Make sure no metrics are created and kept through after_* callbacks.
+ mr_with_event.metrics.destroy!
+ end
+
+ it 'inserts metrics and updates closed and merged events' do
+ subject.perform(mr_with_event.id, mr_with_event.id)
+
+ mr_with_event.reload
+
+ expect(mr_with_event.metrics).to have_attributes(latest_closed_by_id: closed_event.author_id,
+ merged_by_id: merged_event.author_id)
+ expect(mr_with_event.metrics.latest_closed_at.to_s).to eq(closed_event.updated_at.to_s)
+ end
+ end
+
+ describe '#insert_metrics_for_range' do
+ let!(:mrs_without_metrics) { create_list(:merge_request, 3) }
+ let!(:mrs_with_metrics) { create_list(:merge_request, 2) }
+
+ before do
+ # Make sure no metrics are created and kept through after_* callbacks.
+ mrs_without_metrics.each { |m| m.metrics.destroy! }
+ end
+
+ it 'inserts merge_request_metrics for merge_requests without one' do
+ expect { subject.insert_metrics_for_range(MergeRequest.first.id, MergeRequest.last.id) }
+ .to change(MergeRequest::Metrics, :count).from(2).to(5)
+
+ mrs_without_metrics.each do |mr_without_metrics|
+ expect(mr_without_metrics.reload.metrics).to be_present
+ end
+ end
+
+ it 'does not inserts merge_request_metrics for MRs out of given range' do
+ expect { subject.insert_metrics_for_range(mrs_with_metrics.first.id, mrs_with_metrics.last.id) }
+ .not_to change(MergeRequest::Metrics, :count).from(2)
+ end
+ end
+
+ describe '#update_metrics_with_events_data' do
+ context 'closed events data update' do
+ let(:users) { create_list(:user, 3) }
+ let(:mrs_with_event) { create_list(:merge_request, 3) }
+
+ before do
+ create_list(:event, 2, :closed, author: users.first, target: mrs_with_event.first)
+ create_list(:event, 3, :closed, author: users.second, target: mrs_with_event.second)
+ create(:event, :closed, author: users.third, target: mrs_with_event.third)
+ end
+
+ it 'migrates multiple MR metrics with closed event data' do
+ mr_without_event = create(:merge_request)
+ create(:event, :merged)
+
+ subject.update_metrics_with_events_data(mrs_with_event.first.id, mrs_with_event.last.id)
+
+ mrs_with_event.each do |mr_with_event|
+ latest_event = Event.where(action: 3, target: mr_with_event).last
+
+ mr_with_event.metrics.reload
+
+ expect(mr_with_event.metrics.latest_closed_by).to eq(latest_event.author)
+ expect(mr_with_event.metrics.latest_closed_at.to_s).to eq(latest_event.updated_at.to_s)
+ end
+
+ expect(mr_without_event.metrics.reload).to have_attributes(latest_closed_by_id: nil,
+ latest_closed_at: nil)
+ end
+
+ it 'does not updates metrics out of given range' do
+ out_of_range_mr = create(:merge_request)
+ create(:event, :closed, author: users.last, target: out_of_range_mr)
+
+ expect { subject.perform(mrs_with_event.first.id, mrs_with_event.second.id) }
+ .not_to change { out_of_range_mr.metrics.reload.merged_by }
+ .from(nil)
+ end
+ end
+
+ context 'merged events data update' do
+ let(:users) { create_list(:user, 3) }
+ let(:mrs_with_event) { create_list(:merge_request, 3) }
+
+ before do
+ create_list(:event, 2, :merged, author: users.first, target: mrs_with_event.first)
+ create_list(:event, 3, :merged, author: users.second, target: mrs_with_event.second)
+ create(:event, :merged, author: users.third, target: mrs_with_event.third)
+ end
+
+ it 'migrates multiple MR metrics with merged event data' do
+ mr_without_event = create(:merge_request)
+ create(:event, :merged)
+
+ subject.update_metrics_with_events_data(mrs_with_event.first.id, mrs_with_event.last.id)
+
+ mrs_with_event.each do |mr_with_event|
+ latest_event = Event.where(action: Event::MERGED, target: mr_with_event).last
+
+ expect(mr_with_event.metrics.reload.merged_by).to eq(latest_event.author)
+ end
+
+ expect(mr_without_event.metrics.reload).to have_attributes(merged_by_id: nil)
+ end
+
+ it 'does not updates metrics out of given range' do
+ out_of_range_mr = create(:merge_request)
+ create(:event, :merged, author: users.last, target: out_of_range_mr)
+
+ expect { subject.perform(mrs_with_event.first.id, mrs_with_event.second.id) }
+ .not_to change { out_of_range_mr.metrics.reload.merged_by }
+ .from(nil)
+ end
+ end
+ end
+end
diff --git a/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb b/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb
new file mode 100644
index 00000000000..97e089c5cb8
--- /dev/null
+++ b/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb')
+
+describe SchedulePopulateMergeRequestMetricsWithEventsData, :migration, :sidekiq do
+ let!(:mrs) { create_list(:merge_request, 3) }
+
+ it 'correctly schedules background migrations' do
+ stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+ Sidekiq::Testing.fake! do
+ Timecop.freeze do
+ migrate!
+
+ expect(described_class::MIGRATION)
+ .to be_scheduled_migration(10.minutes, mrs.first.id, mrs.second.id)
+
+ expect(described_class::MIGRATION)
+ .to be_scheduled_migration(20.minutes, mrs.third.id, mrs.third.id)
+
+ expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+ end
+ end
+ end
+end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index c9deeb45c1a..5ced000cdb6 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -23,6 +23,32 @@ describe Issue do
it { is_expected.to have_db_index(:deleted_at) }
end
+ describe 'callbacks' do
+ describe '#ensure_metrics' do
+ it 'creates metrics after saving' do
+ issue = create(:issue)
+
+ expect(issue.metrics).to be_persisted
+ expect(Issue::Metrics.count).to eq(1)
+ end
+
+ it 'does not create duplicate metrics for an issue' do
+ issue = create(:issue)
+
+ issue.close!
+
+ expect(issue.metrics).to be_persisted
+ expect(Issue::Metrics.count).to eq(1)
+ end
+
+ it 'records current metrics' do
+ expect_any_instance_of(Issue::Metrics).to receive(:record!)
+
+ create(:issue)
+ end
+ end
+ end
+
describe '#order_by_position_and_priority' do
let(:project) { create :project }
let(:p1) { create(:label, title: 'P1', project: project, priority: 1) }
diff --git a/spec/models/merge_request/metrics_spec.rb b/spec/models/merge_request/metrics_spec.rb
index 9353d5c3c8a..02ff7839739 100644
--- a/spec/models/merge_request/metrics_spec.rb
+++ b/spec/models/merge_request/metrics_spec.rb
@@ -1,16 +1,11 @@
require 'spec_helper'
describe MergeRequest::Metrics do
- subject { create(:merge_request) }
+ subject { described_class.new }
- describe "when recording the default set of metrics on merge request save" do
- it "records the merge time" do
- time = Time.now
- Timecop.freeze(time) { subject.mark_as_merged }
- metrics = subject.metrics
-
- expect(metrics).to be_present
- expect(metrics.merged_at).to be_like_time(time)
- end
+ describe 'associations' do
+ it { is_expected.to belong_to(:merge_request) }
+ it { is_expected.to belong_to(:latest_closed_by).class_name('User') }
+ it { is_expected.to belong_to(:merged_by).class_name('User') }
end
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index df94617a19e..d8ebd46faab 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -65,6 +65,25 @@ describe MergeRequest do
end
end
+ describe 'callbacks' do
+ describe '#ensure_merge_request_metrics' do
+ it 'creates metrics after saving' do
+ merge_request = create(:merge_request)
+
+ expect(merge_request.metrics).to be_persisted
+ expect(MergeRequest::Metrics.count).to eq(1)
+ end
+
+ it 'does not duplicate metrics for a merge request' do
+ merge_request = create(:merge_request)
+
+ merge_request.mark_as_merged!
+
+ expect(MergeRequest::Metrics.count).to eq(1)
+ end
+ end
+ end
+
describe 'respond to' do
it { is_expected.to respond_to(:unchecked?) }
it { is_expected.to respond_to(:can_be_merged?) }
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index be8d9c19125..981c9c27325 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -464,7 +464,7 @@ describe API::Notes do
describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
it "creates an activity event when an issue note is created" do
- expect(Event).to receive(:create)
+ expect(Event).to receive(:create!)
post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!'
end
diff --git a/spec/requests/api/project_milestones_spec.rb b/spec/requests/api/project_milestones_spec.rb
index 6fe8ab5a3f6..08ea7314bb3 100644
--- a/spec/requests/api/project_milestones_spec.rb
+++ b/spec/requests/api/project_milestones_spec.rb
@@ -16,7 +16,7 @@ describe API::ProjectMilestones do
describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do
it 'creates an activity event when an milestone is closed' do
- expect(Event).to receive(:create)
+ expect(Event).to receive(:create!)
put api("/projects/#{project.id}/milestones/#{milestone.id}", user),
state_event: 'close'
diff --git a/spec/requests/api/v3/milestones_spec.rb b/spec/requests/api/v3/milestones_spec.rb
index 9ee71ea9c5d..6021600e09c 100644
--- a/spec/requests/api/v3/milestones_spec.rb
+++ b/spec/requests/api/v3/milestones_spec.rb
@@ -161,7 +161,7 @@ describe API::V3::Milestones do
describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do
it 'creates an activity event when an milestone is closed' do
- expect(Event).to receive(:create)
+ expect(Event).to receive(:create!)
put v3_api("/projects/#{project.id}/milestones/#{milestone.id}", user),
state_event: 'close'
diff --git a/spec/requests/api/v3/notes_spec.rb b/spec/requests/api/v3/notes_spec.rb
index 6428d9daaba..5532795ab02 100644
--- a/spec/requests/api/v3/notes_spec.rb
+++ b/spec/requests/api/v3/notes_spec.rb
@@ -302,7 +302,7 @@ describe API::V3::Notes do
describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
it "creates an activity event when an issue note is created" do
- expect(Event).to receive(:create)
+ expect(Event).to receive(:create!)
post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
end
diff --git a/spec/serializers/event_entity_spec.rb b/spec/serializers/event_entity_spec.rb
deleted file mode 100644
index bb54597c967..00000000000
--- a/spec/serializers/event_entity_spec.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-require 'spec_helper'
-
-describe EventEntity do
- subject { described_class.represent(create(:event)).as_json }
-
- it 'exposes author' do
- expect(subject).to include(:author)
- end
-
- it 'exposes core elements of event' do
- expect(subject).to include(:updated_at)
- end
-end
diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb
index a5924a8589c..9bbe815c115 100644
--- a/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/spec/serializers/merge_request_widget_entity_spec.rb
@@ -35,6 +35,80 @@ describe MergeRequestWidgetEntity do
end
end
+ describe 'metrics' do
+ context 'when metrics record exists with merged data' do
+ before do
+ resource.mark_as_merged!
+ resource.metrics.update!(merged_by: user)
+ end
+
+ it 'matches merge request metrics schema' do
+ expect(subject[:metrics].with_indifferent_access)
+ .to match_schema('entities/merge_request_metrics')
+ end
+
+ it 'returns values from metrics record' do
+ expect(subject.dig(:metrics, :merged_by, :id))
+ .to eq(resource.metrics.merged_by_id)
+ end
+ end
+
+ context 'when metrics record exists with closed data' do
+ before do
+ resource.close!
+ resource.metrics.update!(latest_closed_by: user)
+ end
+
+ it 'matches merge request metrics schema' do
+ expect(subject[:metrics].with_indifferent_access)
+ .to match_schema('entities/merge_request_metrics')
+ end
+
+ it 'returns values from metrics record' do
+ expect(subject.dig(:metrics, :closed_by, :id))
+ .to eq(resource.metrics.latest_closed_by_id)
+ end
+ end
+
+ context 'when metrics does not exists' do
+ before do
+ resource.metrics.destroy!
+ resource.reload
+ end
+
+ context 'when events exists' do
+ let!(:closed_event) { create(:event, :closed, project: project, target: resource) }
+ let!(:merge_event) { create(:event, :merged, project: project, target: resource) }
+
+ it 'matches merge request metrics schema' do
+ expect(subject[:metrics].with_indifferent_access)
+ .to match_schema('entities/merge_request_metrics')
+ end
+
+ it 'returns values from events record' do
+ expect(subject.dig(:metrics, :merged_by, :id))
+ .to eq(merge_event.author_id)
+
+ expect(subject.dig(:metrics, :closed_by, :id))
+ .to eq(closed_event.author_id)
+
+ expect(subject.dig(:metrics, :merged_at).to_s)
+ .to eq(merge_event.updated_at.to_s)
+
+ expect(subject.dig(:metrics, :closed_at).to_s)
+ .to eq(closed_event.updated_at.to_s)
+ end
+ end
+
+ context 'when events does not exists' do
+ it 'matches merge request metrics schema' do
+ expect(subject[:metrics].with_indifferent_access)
+ .to match_schema('entities/merge_request_metrics')
+ end
+ end
+ end
+ end
+
it 'has email_patches_path' do
expect(subject[:email_patches_path])
.to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.patch")
diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb
index 08267d6e6a0..b9bfbb11511 100644
--- a/spec/services/create_deployment_service_spec.rb
+++ b/spec/services/create_deployment_service_spec.rb
@@ -266,7 +266,7 @@ describe CreateDeploymentService do
context "while updating the 'first_deployed_to_production_at' time" do
before do
- merge_request.mark_as_merged
+ merge_request.metrics.update!(merged_at: Time.now)
end
context "for merge requests merged before the current deploy" do
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb
index 2a59bc4594a..4d12de3ecce 100644
--- a/spec/services/merge_requests/close_service_spec.rb
+++ b/spec/services/merge_requests/close_service_spec.rb
@@ -52,6 +52,19 @@ describe MergeRequests::CloseService do
end
end
+ it 'updates metrics' do
+ metrics = merge_request.metrics
+ metrics_service = double(MergeRequestMetricsService)
+ allow(MergeRequestMetricsService)
+ .to receive(:new)
+ .with(metrics)
+ .and_return(metrics_service)
+
+ expect(metrics_service).to receive(:close)
+
+ described_class.new(project, user, {}).execute(merge_request)
+ end
+
it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do
service = described_class.new(project, user, {})
diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb
index 8f2c5df5907..70957431942 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/spec/services/merge_requests/post_merge_service_spec.rb
@@ -22,5 +22,18 @@ describe MergeRequests::PostMergeService do
expect { service.execute(merge_request) }
.to change { project.open_merge_requests_count }.from(1).to(0)
end
+
+ it 'updates metrics' do
+ metrics = merge_request.metrics
+ metrics_service = double(MergeRequestMetricsService)
+ allow(MergeRequestMetricsService)
+ .to receive(:new)
+ .with(metrics)
+ .and_return(metrics_service)
+
+ expect(metrics_service).to receive(:merge)
+
+ described_class.new(project, user, {}).execute(merge_request)
+ end
end
end
diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb
index 94f31ff139c..a44d63e5f9f 100644
--- a/spec/services/merge_requests/reopen_service_spec.rb
+++ b/spec/services/merge_requests/reopen_service_spec.rb
@@ -47,6 +47,19 @@ describe MergeRequests::ReopenService do
end
end
+ it 'updates metrics' do
+ metrics = merge_request.metrics
+ service = double(MergeRequestMetricsService)
+ allow(MergeRequestMetricsService)
+ .to receive(:new)
+ .with(metrics)
+ .and_return(service)
+
+ expect(service).to receive(:reopen)
+
+ described_class.new(project, user, {}).execute(merge_request)
+ end
+
it 'refreshes the number of open merge requests for a valid MR' do
service = described_class.new(project, user, {})
diff --git a/spec/services/update_merge_request_metrics_service_spec.rb b/spec/services/update_merge_request_metrics_service_spec.rb
new file mode 100644
index 00000000000..b5fb999381d
--- /dev/null
+++ b/spec/services/update_merge_request_metrics_service_spec.rb
@@ -0,0 +1,42 @@
+require 'rails_helper'
+
+describe MergeRequestMetricsService do
+ let(:metrics) { create(:merge_request).metrics }
+
+ describe '#merge' do
+ it 'updates metrics' do
+ user = create(:user)
+ service = described_class.new(metrics)
+ event = double(Event, author_id: user.id, created_at: Time.now)
+
+ service.merge(event)
+
+ expect(metrics.merged_by).to eq(user)
+ expect(metrics.merged_at).to eq(event.created_at)
+ end
+ end
+
+ describe '#close' do
+ it 'updates metrics' do
+ user = create(:user)
+ service = described_class.new(metrics)
+ event = double(Event, author_id: user.id, created_at: Time.now)
+
+ service.close(event)
+
+ expect(metrics.latest_closed_by).to eq(user)
+ expect(metrics.latest_closed_at).to eq(event.created_at)
+ end
+ end
+
+ describe '#reopen' do
+ it 'updates metrics' do
+ service = described_class.new(metrics)
+
+ service.reopen
+
+ expect(metrics.latest_closed_by).to be_nil
+ expect(metrics.latest_closed_at).to be_nil
+ end
+ end
+end