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 b25cc3443efffdb21ded233903536d4d6c1e5574..dd8b2665b1d2e3877da3c83dc60722fde69bc111 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 {

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 2dfd87ed9049b1a504a42c0cd1d3fd93a265c81e..2a4211925680705e0bcfbaed17139a7022a5ea23 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 {

+ :author="mr.metrics.mergedBy" + :date-title="mr.metrics.mergedAt" + :date-readable="mr.metrics.readableMergedAt" /> { 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 2714e8294fa83e6d2f68c4c1e6c1a0ece7b21373..909724696305778f42de6f426bbad5a2a7f1f999 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 1ad7c2d8efa198ab94b5db7a6ffb8cd1bcea91a6..ca29c9fee3217d353655f01f436020f9c4b5f041 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 0000000000000000000000000000000000000000..dfe3b31f1c0370610cc3ae205613df7256d681be --- /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 0000000000000000000000000000000000000000..97e089c5cb888ff5a49704c1febb0cd58f342300 --- /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 c9deeb45c1a2d3bacbe8641eac2b0570e5f7999b..5ced000cdb638865b9fbd42dc57bd54ca9a63ad3 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 9353d5c3c8ad3998d08d4efddc54dbee417797ac..02ff783973904fbe0fba616398a022237c9d4c77 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 df94617a19ea0c8c854c8f7008ec8b7df6bdf135..d8ebd46faab0eea8f5445824f75b9b26fa9ba38e 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 be8d9c191257eafab6ef1003ed456001be816784..981c9c27325972b5d6d2e0e7919b9820d73d847d 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 6fe8ab5a3f6808f8d94794fcd30e38520eeb4e33..08ea7314bb32b6eee2169eb44acdc9a01b92ab42 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 9ee71ea9c5d68f049e90c53210379b60163d597a..6021600e09c34fd7a37be4276d358d8529780e18 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 6428d9daabaaaecdff6472b1c1e78033d0c0c3cf..5532795ab022e38aeabd0b900e2e51f4c1c75fc4 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 bb54597c96742a1d4a4019cc1e9c9db6c3620df2..0000000000000000000000000000000000000000 --- 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 a5924a8589c44ff9fbf9bb9646f8daddc9df4b89..9bbe815c11514113a7537939fb0135afcabd9bcd 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 08267d6e6a043a602814245b9549afa4d2c929ee..b9bfbb11511486ce9bd13f6fa6969765d14927b2 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 2a59bc4594af47ba5d3efbfd7bf0dc4023030992..4d12de3eccec75ff29a74249225eece1a75f023d 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 8f2c5df5907d0907af163e5f61694af78f51d7dc..70957431942ad947b3d102265dee8ac1b9ca3882 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 94f31ff139c78933a1ecf28d2db07754a7d1f914..a44d63e5f9f679773e0c2319771b17529779827f 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 0000000000000000000000000000000000000000..b5fb999381dfc55125c48aa76563241fd6046182 --- /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