Commit f0adf28ed917d8ebedc89c954e31b1f1f9e4b6fc
Exists in
spb-stable
and in
3 other branches
Merge branch 'event-create-service' into 'master'
EventCreateService class The goal is to collect all event creation logic in one place called EventCreateService. Because now its placed in observers, controllers, services etc
Showing
14 changed files
with
200 additions
and
144 deletions
Show diff stats
app/models/event.rb
| @@ -47,14 +47,6 @@ class Event < ActiveRecord::Base | @@ -47,14 +47,6 @@ class Event < ActiveRecord::Base | ||
| 47 | scope :in_projects, ->(project_ids) { where(project_id: project_ids).recent } | 47 | scope :in_projects, ->(project_ids) { where(project_id: project_ids).recent } |
| 48 | 48 | ||
| 49 | class << self | 49 | class << self |
| 50 | - def determine_action(record) | ||
| 51 | - if [Issue, MergeRequest].include? record.class | ||
| 52 | - Event::CREATED | ||
| 53 | - elsif record.kind_of? Note | ||
| 54 | - Event::COMMENTED | ||
| 55 | - end | ||
| 56 | - end | ||
| 57 | - | ||
| 58 | def create_ref_event(project, user, ref, action = 'add', prefix = 'refs/heads') | 50 | def create_ref_event(project, user, ref, action = 'add', prefix = 'refs/heads') |
| 59 | commit = project.repository.commit(ref.target) | 51 | commit = project.repository.commit(ref.target) |
| 60 | 52 |
app/observers/activity_observer.rb
| @@ -1,35 +0,0 @@ | @@ -1,35 +0,0 @@ | ||
| 1 | -class ActivityObserver < BaseObserver | ||
| 2 | - observe :issue, :note, :milestone | ||
| 3 | - | ||
| 4 | - def after_create(record) | ||
| 5 | - if record.kind_of?(Note) | ||
| 6 | - # Skip system notes, like status changes and cross-references. | ||
| 7 | - return true if record.system? | ||
| 8 | - | ||
| 9 | - # Skip wall notes to prevent spamming of dashboard | ||
| 10 | - return true if record.noteable_type.blank? | ||
| 11 | - end | ||
| 12 | - | ||
| 13 | - create_event(record, Event.determine_action(record)) if current_user | ||
| 14 | - end | ||
| 15 | - | ||
| 16 | - def after_close(record, transition) | ||
| 17 | - create_event(record, Event::CLOSED) | ||
| 18 | - end | ||
| 19 | - | ||
| 20 | - def after_reopen(record, transition) | ||
| 21 | - create_event(record, Event::REOPENED) | ||
| 22 | - end | ||
| 23 | - | ||
| 24 | - protected | ||
| 25 | - | ||
| 26 | - def create_event(record, status) | ||
| 27 | - Event.create( | ||
| 28 | - project: record.project, | ||
| 29 | - target_id: record.id, | ||
| 30 | - target_type: record.class.name, | ||
| 31 | - action: status, | ||
| 32 | - author_id: current_user.id | ||
| 33 | - ) | ||
| 34 | - end | ||
| 35 | -end |
app/observers/base_observer.rb
| @@ -3,6 +3,10 @@ class BaseObserver < ActiveRecord::Observer | @@ -3,6 +3,10 @@ class BaseObserver < ActiveRecord::Observer | ||
| 3 | NotificationService.new | 3 | NotificationService.new |
| 4 | end | 4 | end |
| 5 | 5 | ||
| 6 | + def event_service | ||
| 7 | + EventCreateService.new | ||
| 8 | + end | ||
| 9 | + | ||
| 6 | def log_info message | 10 | def log_info message |
| 7 | Gitlab::AppLogger.info message | 11 | Gitlab::AppLogger.info message |
| 8 | end | 12 | end |
app/observers/issue_observer.rb
| 1 | class IssueObserver < BaseObserver | 1 | class IssueObserver < BaseObserver |
| 2 | def after_create(issue) | 2 | def after_create(issue) |
| 3 | notification.new_issue(issue, current_user) | 3 | notification.new_issue(issue, current_user) |
| 4 | + event_service.open_issue(issue, current_user) | ||
| 4 | issue.create_cross_references!(issue.project, current_user) | 5 | issue.create_cross_references!(issue.project, current_user) |
| 5 | execute_hooks(issue) | 6 | execute_hooks(issue) |
| 6 | end | 7 | end |
| 7 | 8 | ||
| 8 | def after_close(issue, transition) | 9 | def after_close(issue, transition) |
| 9 | notification.close_issue(issue, current_user) | 10 | notification.close_issue(issue, current_user) |
| 11 | + event_service.close_issue(issue, current_user) | ||
| 10 | create_note(issue) | 12 | create_note(issue) |
| 11 | execute_hooks(issue) | 13 | execute_hooks(issue) |
| 12 | end | 14 | end |
| 13 | 15 | ||
| 14 | def after_reopen(issue, transition) | 16 | def after_reopen(issue, transition) |
| 17 | + event_service.reopen_issue(issue, current_user) | ||
| 15 | create_note(issue) | 18 | create_note(issue) |
| 16 | execute_hooks(issue) | 19 | execute_hooks(issue) |
| 17 | end | 20 | end |
app/observers/merge_request_observer.rb
| 1 | -class MergeRequestObserver < ActivityObserver | ||
| 2 | - observe :merge_request | ||
| 3 | - | 1 | +class MergeRequestObserver < BaseObserver |
| 4 | def after_create(merge_request) | 2 | def after_create(merge_request) |
| 5 | - if merge_request.author_id | ||
| 6 | - create_event(merge_request, Event.determine_action(merge_request)) | ||
| 7 | - end | ||
| 8 | - | 3 | + event_service.open_mr(merge_request, current_user) |
| 9 | notification.new_merge_request(merge_request, current_user) | 4 | notification.new_merge_request(merge_request, current_user) |
| 10 | merge_request.create_cross_references!(merge_request.project, current_user) | 5 | merge_request.create_cross_references!(merge_request.project, current_user) |
| 11 | execute_hooks(merge_request) | 6 | execute_hooks(merge_request) |
| 12 | end | 7 | end |
| 13 | 8 | ||
| 14 | def after_close(merge_request, transition) | 9 | def after_close(merge_request, transition) |
| 15 | - create_event(merge_request, Event::CLOSED) | 10 | + event_service.close_mr(merge_request, current_user) |
| 16 | notification.close_mr(merge_request, current_user) | 11 | notification.close_mr(merge_request, current_user) |
| 17 | create_note(merge_request) | 12 | create_note(merge_request) |
| 18 | execute_hooks(merge_request) | 13 | execute_hooks(merge_request) |
| 19 | end | 14 | end |
| 20 | 15 | ||
| 21 | def after_reopen(merge_request, transition) | 16 | def after_reopen(merge_request, transition) |
| 22 | - create_event(merge_request, Event::REOPENED) | 17 | + event_service.reopen_mr(merge_request, current_user) |
| 23 | create_note(merge_request) | 18 | create_note(merge_request) |
| 24 | execute_hooks(merge_request) | 19 | execute_hooks(merge_request) |
| 25 | merge_request.reload_code | 20 | merge_request.reload_code |
| @@ -33,16 +28,6 @@ class MergeRequestObserver < ActivityObserver | @@ -33,16 +28,6 @@ class MergeRequestObserver < ActivityObserver | ||
| 33 | execute_hooks(merge_request) | 28 | execute_hooks(merge_request) |
| 34 | end | 29 | end |
| 35 | 30 | ||
| 36 | - def create_event(record, status) | ||
| 37 | - Event.create( | ||
| 38 | - project: record.target_project, | ||
| 39 | - target_id: record.id, | ||
| 40 | - target_type: record.class.name, | ||
| 41 | - action: status, | ||
| 42 | - author_id: current_user.id | ||
| 43 | - ) | ||
| 44 | - end | ||
| 45 | - | ||
| 46 | private | 31 | private |
| 47 | 32 | ||
| 48 | # Create merge request note with service comment like 'Status changed to closed' | 33 | # Create merge request note with service comment like 'Status changed to closed' |
| @@ -0,0 +1,13 @@ | @@ -0,0 +1,13 @@ | ||
| 1 | +class MilestoneObserver < BaseObserver | ||
| 2 | + def after_create(milestone) | ||
| 3 | + event_service.open_milestone(milestone, current_user) | ||
| 4 | + end | ||
| 5 | + | ||
| 6 | + def after_close(milestone, transition) | ||
| 7 | + event_service.close_milestone(milestone, current_user) | ||
| 8 | + end | ||
| 9 | + | ||
| 10 | + def after_reopen(milestone, transition) | ||
| 11 | + event_service.reopen_milestone(milestone, current_user) | ||
| 12 | + end | ||
| 13 | +end |
app/observers/note_observer.rb
| @@ -2,6 +2,12 @@ class NoteObserver < BaseObserver | @@ -2,6 +2,12 @@ class NoteObserver < BaseObserver | ||
| 2 | def after_create(note) | 2 | def after_create(note) |
| 3 | notification.new_note(note) | 3 | notification.new_note(note) |
| 4 | 4 | ||
| 5 | + # Skip system notes, like status changes and cross-references. | ||
| 6 | + # Skip wall notes to prevent spamming of dashboard | ||
| 7 | + if note.noteable_type.present? && !note.system | ||
| 8 | + event_service.leave_note(note, current_user) | ||
| 9 | + end | ||
| 10 | + | ||
| 5 | unless note.system? | 11 | unless note.system? |
| 6 | # Create a cross-reference note if this Note contains GFM that names an | 12 | # Create a cross-reference note if this Note contains GFM that names an |
| 7 | # issue, merge request, or commit. | 13 | # issue, merge request, or commit. |
| @@ -0,0 +1,64 @@ | @@ -0,0 +1,64 @@ | ||
| 1 | +# EventCreateService class | ||
| 2 | +# | ||
| 3 | +# Used for creating events feed on dashboard after certain user action | ||
| 4 | +# | ||
| 5 | +# Ex. | ||
| 6 | +# EventCreateService.new.new_issue(issue, current_user) | ||
| 7 | +# | ||
| 8 | +class EventCreateService | ||
| 9 | + def open_issue(issue, current_user) | ||
| 10 | + create_event(issue, current_user, Event::CREATED) | ||
| 11 | + end | ||
| 12 | + | ||
| 13 | + def close_issue(issue, current_user) | ||
| 14 | + create_event(issue, current_user, Event::CLOSED) | ||
| 15 | + end | ||
| 16 | + | ||
| 17 | + def reopen_issue(issue, current_user) | ||
| 18 | + create_event(issue, current_user, Event::REOPENED) | ||
| 19 | + end | ||
| 20 | + | ||
| 21 | + def open_mr(merge_request, current_user) | ||
| 22 | + create_event(merge_request, current_user, Event::CREATED) | ||
| 23 | + end | ||
| 24 | + | ||
| 25 | + def close_mr(merge_request, current_user) | ||
| 26 | + create_event(merge_request, current_user, Event::CLOSED) | ||
| 27 | + end | ||
| 28 | + | ||
| 29 | + def reopen_mr(merge_request, current_user) | ||
| 30 | + create_event(merge_request, current_user, Event::REOPENED) | ||
| 31 | + end | ||
| 32 | + | ||
| 33 | + def merge_mr(merge_request, current_user) | ||
| 34 | + create_event(merge_request, current_user, Event::MERGED) | ||
| 35 | + end | ||
| 36 | + | ||
| 37 | + def open_milestone(milestone, current_user) | ||
| 38 | + create_event(milestone, current_user, Event::CREATED) | ||
| 39 | + end | ||
| 40 | + | ||
| 41 | + def close_milestone(milestone, current_user) | ||
| 42 | + create_event(milestone, current_user, Event::CLOSED) | ||
| 43 | + end | ||
| 44 | + | ||
| 45 | + def reopen_milestone(milestone, current_user) | ||
| 46 | + create_event(milestone, current_user, Event::REOPENED) | ||
| 47 | + end | ||
| 48 | + | ||
| 49 | + def leave_note(note, current_user) | ||
| 50 | + create_event(note, current_user, Event::COMMENTED) | ||
| 51 | + end | ||
| 52 | + | ||
| 53 | + private | ||
| 54 | + | ||
| 55 | + def create_event(record, current_user, status) | ||
| 56 | + Event.create( | ||
| 57 | + project: record.project, | ||
| 58 | + target_id: record.id, | ||
| 59 | + target_type: record.class.name, | ||
| 60 | + action: status, | ||
| 61 | + author_id: current_user.id | ||
| 62 | + ) | ||
| 63 | + end | ||
| 64 | +end |
app/services/merge_requests/base_merge_service.rb
| @@ -8,13 +8,7 @@ module MergeRequests | @@ -8,13 +8,7 @@ module MergeRequests | ||
| 8 | end | 8 | end |
| 9 | 9 | ||
| 10 | def create_merge_event(merge_request, current_user) | 10 | def create_merge_event(merge_request, current_user) |
| 11 | - Event.create( | ||
| 12 | - project: merge_request.target_project, | ||
| 13 | - target_id: merge_request.id, | ||
| 14 | - target_type: merge_request.class.name, | ||
| 15 | - action: Event::MERGED, | ||
| 16 | - author_id: current_user.id | ||
| 17 | - ) | 11 | + EventCreateService.new.merge_mr(merge_request, current_user) |
| 18 | end | 12 | end |
| 19 | 13 | ||
| 20 | def execute_project_hooks(merge_request) | 14 | def execute_project_hooks(merge_request) |
config/application.rb
| @@ -19,7 +19,7 @@ module Gitlab | @@ -19,7 +19,7 @@ module Gitlab | ||
| 19 | # config.plugins = [ :exception_notification, :ssl_requirement, :all ] | 19 | # config.plugins = [ :exception_notification, :ssl_requirement, :all ] |
| 20 | 20 | ||
| 21 | # Activate observers that should always be running. | 21 | # Activate observers that should always be running. |
| 22 | - config.active_record.observers = :activity_observer, | 22 | + config.active_record.observers = :milestone_observer, |
| 23 | :project_activity_cache_observer, | 23 | :project_activity_cache_observer, |
| 24 | :issue_observer, | 24 | :issue_observer, |
| 25 | :key_observer, | 25 | :key_observer, |
spec/observers/activity_observer_spec.rb
| @@ -1,61 +0,0 @@ | @@ -1,61 +0,0 @@ | ||
| 1 | -require 'spec_helper' | ||
| 2 | - | ||
| 3 | -describe ActivityObserver do | ||
| 4 | - let(:project) { create(:project) } | ||
| 5 | - | ||
| 6 | - before { Thread.current[:current_user] = create(:user) } | ||
| 7 | - | ||
| 8 | - def self.it_should_be_valid_event | ||
| 9 | - it { @event.should_not be_nil } | ||
| 10 | - it { @event.project.should == project } | ||
| 11 | - end | ||
| 12 | - | ||
| 13 | - describe "Issue created" do | ||
| 14 | - before do | ||
| 15 | - Issue.observers.enable :activity_observer do | ||
| 16 | - @issue = create(:issue, project: project) | ||
| 17 | - @event = Event.last | ||
| 18 | - end | ||
| 19 | - end | ||
| 20 | - | ||
| 21 | - it_should_be_valid_event | ||
| 22 | - it { @event.action.should == Event::CREATED } | ||
| 23 | - it { @event.target.should == @issue } | ||
| 24 | - end | ||
| 25 | - | ||
| 26 | - describe "Issue commented" do | ||
| 27 | - before do | ||
| 28 | - Note.observers.enable :activity_observer do | ||
| 29 | - @issue = create(:issue, project: project) | ||
| 30 | - @note = create(:note, noteable: @issue, project: project, author: @issue.author) | ||
| 31 | - @event = Event.last | ||
| 32 | - end | ||
| 33 | - end | ||
| 34 | - | ||
| 35 | - it_should_be_valid_event | ||
| 36 | - it { @event.action.should == Event::COMMENTED } | ||
| 37 | - it { @event.target.should == @note } | ||
| 38 | - end | ||
| 39 | - | ||
| 40 | - describe "Ignore system notes" do | ||
| 41 | - let(:author) { create(:user) } | ||
| 42 | - let!(:issue) { create(:issue, project: project) } | ||
| 43 | - let!(:other) { create(:issue) } | ||
| 44 | - | ||
| 45 | - it "should not create events for status change notes" do | ||
| 46 | - expect do | ||
| 47 | - Note.observers.enable :activity_observer do | ||
| 48 | - Note.create_status_change_note(issue, project, author, 'reopened', nil) | ||
| 49 | - end | ||
| 50 | - end.to_not change { Event.count } | ||
| 51 | - end | ||
| 52 | - | ||
| 53 | - it "should not create events for cross-reference notes" do | ||
| 54 | - expect do | ||
| 55 | - Note.observers.enable :activity_observer do | ||
| 56 | - Note.create_cross_reference_note(issue, other, author, issue.project) | ||
| 57 | - end | ||
| 58 | - end.to_not change { Event.count } | ||
| 59 | - end | ||
| 60 | - end | ||
| 61 | -end |
spec/requests/api/issues_spec.rb
| @@ -100,16 +100,4 @@ describe API::API do | @@ -100,16 +100,4 @@ describe API::API do | ||
| 100 | response.status.should == 405 | 100 | response.status.should == 405 |
| 101 | end | 101 | end |
| 102 | end | 102 | end |
| 103 | - | ||
| 104 | - describe "PUT /projects/:id/issues/:issue_id to test observer on close" do | ||
| 105 | - before { enable_observers } | ||
| 106 | - after { disable_observers } | ||
| 107 | - | ||
| 108 | - it "should create an activity event when an issue is closed" do | ||
| 109 | - Event.should_receive(:create) | ||
| 110 | - | ||
| 111 | - put api("/projects/#{project.id}/issues/#{issue.id}", user), | ||
| 112 | - state_event: "close" | ||
| 113 | - end | ||
| 114 | - end | ||
| 115 | end | 103 | end |
| @@ -0,0 +1,103 @@ | @@ -0,0 +1,103 @@ | ||
| 1 | +require 'spec_helper' | ||
| 2 | + | ||
| 3 | +describe EventCreateService do | ||
| 4 | + let(:service) { EventCreateService.new } | ||
| 5 | + | ||
| 6 | + describe 'Issues' do | ||
| 7 | + describe :open_issue do | ||
| 8 | + let(:issue) { create(:issue) } | ||
| 9 | + | ||
| 10 | + it { service.open_issue(issue, issue.author).should be_true } | ||
| 11 | + | ||
| 12 | + it "should create new event" do | ||
| 13 | + expect { service.open_issue(issue, issue.author) }.to change { Event.count } | ||
| 14 | + end | ||
| 15 | + end | ||
| 16 | + | ||
| 17 | + describe :close_issue do | ||
| 18 | + let(:issue) { create(:issue) } | ||
| 19 | + | ||
| 20 | + it { service.close_issue(issue, issue.author).should be_true } | ||
| 21 | + | ||
| 22 | + it "should create new event" do | ||
| 23 | + expect { service.close_issue(issue, issue.author) }.to change { Event.count } | ||
| 24 | + end | ||
| 25 | + end | ||
| 26 | + | ||
| 27 | + describe :reopen_issue do | ||
| 28 | + let(:issue) { create(:issue) } | ||
| 29 | + | ||
| 30 | + it { service.reopen_issue(issue, issue.author).should be_true } | ||
| 31 | + | ||
| 32 | + it "should create new event" do | ||
| 33 | + expect { service.reopen_issue(issue, issue.author) }.to change { Event.count } | ||
| 34 | + end | ||
| 35 | + end | ||
| 36 | + end | ||
| 37 | + | ||
| 38 | + describe 'Merge Requests' do | ||
| 39 | + describe :open_mr do | ||
| 40 | + let(:merge_request) { create(:merge_request) } | ||
| 41 | + | ||
| 42 | + it { service.open_mr(merge_request, merge_request.author).should be_true } | ||
| 43 | + | ||
| 44 | + it "should create new event" do | ||
| 45 | + expect { service.open_mr(merge_request, merge_request.author) }.to change { Event.count } | ||
| 46 | + end | ||
| 47 | + end | ||
| 48 | + | ||
| 49 | + describe :close_mr do | ||
| 50 | + let(:merge_request) { create(:merge_request) } | ||
| 51 | + | ||
| 52 | + it { service.close_mr(merge_request, merge_request.author).should be_true } | ||
| 53 | + | ||
| 54 | + it "should create new event" do | ||
| 55 | + expect { service.close_mr(merge_request, merge_request.author) }.to change { Event.count } | ||
| 56 | + end | ||
| 57 | + end | ||
| 58 | + | ||
| 59 | + describe :merge_mr do | ||
| 60 | + let(:merge_request) { create(:merge_request) } | ||
| 61 | + | ||
| 62 | + it { service.merge_mr(merge_request, merge_request.author).should be_true } | ||
| 63 | + | ||
| 64 | + it "should create new event" do | ||
| 65 | + expect { service.merge_mr(merge_request, merge_request.author) }.to change { Event.count } | ||
| 66 | + end | ||
| 67 | + end | ||
| 68 | + | ||
| 69 | + describe :reopen_mr do | ||
| 70 | + let(:merge_request) { create(:merge_request) } | ||
| 71 | + | ||
| 72 | + it { service.reopen_mr(merge_request, merge_request.author).should be_true } | ||
| 73 | + | ||
| 74 | + it "should create new event" do | ||
| 75 | + expect { service.reopen_mr(merge_request, merge_request.author) }.to change { Event.count } | ||
| 76 | + end | ||
| 77 | + end | ||
| 78 | + end | ||
| 79 | + | ||
| 80 | + describe 'Milestone' do | ||
| 81 | + let(:user) { create :user } | ||
| 82 | + | ||
| 83 | + describe :open_milestone do | ||
| 84 | + let(:milestone) { create(:milestone) } | ||
| 85 | + | ||
| 86 | + it { service.open_milestone(milestone, user).should be_true } | ||
| 87 | + | ||
| 88 | + it "should create new event" do | ||
| 89 | + expect { service.open_milestone(milestone, user) }.to change { Event.count } | ||
| 90 | + end | ||
| 91 | + end | ||
| 92 | + | ||
| 93 | + describe :close_mr do | ||
| 94 | + let(:milestone) { create(:milestone) } | ||
| 95 | + | ||
| 96 | + it { service.close_milestone(milestone, user).should be_true } | ||
| 97 | + | ||
| 98 | + it "should create new event" do | ||
| 99 | + expect { service.close_milestone(milestone, user) }.to change { Event.count } | ||
| 100 | + end | ||
| 101 | + end | ||
| 102 | + end | ||
| 103 | +end |
spec/support/test_env.rb
| @@ -90,7 +90,7 @@ module TestEnv | @@ -90,7 +90,7 @@ module TestEnv | ||
| 90 | size: 12.45 | 90 | size: 12.45 |
| 91 | ) | 91 | ) |
| 92 | 92 | ||
| 93 | - ActivityObserver.any_instance.stub( | 93 | + BaseObserver.any_instance.stub( |
| 94 | current_user: double("current_user", id: 1) | 94 | current_user: double("current_user", id: 1) |
| 95 | ) | 95 | ) |
| 96 | end | 96 | end |