diff options
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 |