Merge branch 'reduce-observers' into 'master'

Remove some observers

Merge branch 'reduce-observers' into 'master'
Remove some observers
Dmitriy Zaporozhets
2 parents 96f91c0f 9b6224f9
Showing 27 changed files with 214 additions and 335 deletions Show diff stats
app/controllers/projects/merge_requests_controller.rb
app/models/email.rb
app/models/key.rb
app/models/merge_request.rb
app/models/project.rb
app/observers/email_observer.rb
app/observers/key_observer.rb
app/observers/merge_request_observer.rb
app/services/merge_requests/base_service.rb
app/services/merge_requests/close_service.rb
app/services/merge_requests/create_service.rb
app/services/merge_requests/reopen_service.rb
app/services/merge_requests/update_service.rb
app/views/projects/merge_requests/_form.html.haml
config/application.rb
features/steps/dashboard/merge_requests.rb
features/support/env.rb
lib/api/merge_requests.rb
spec/finders/merge_requests_finder_spec.rb
spec/lib/gitlab/satellite/merge_action_spec.rb
@@ -76,10 +76,10 @@ class Projects::MergeRequestsController &lt; Projects::ApplicationController
   end
   def create
-    @merge_request = MergeRequest.new(params[:merge_request])
-    @merge_request.author = current_user
     @target_branches ||= []
-    if @merge_request.save
+    @merge_request = MergeRequests::CreateService.new(project, current_user, params[:merge_request]).execute
+
+    if @merge_request.valid?
       redirect_to [@merge_request.target_project, @merge_request], notice: 'Merge request was successfully created.'
     else
       @source_project = @merge_request.source_project
@@ -89,29 +89,9 @@ class Projects::MergeRequestsController &lt; Projects::ApplicationController
   end
   def update
-    # If we close MergeRequest we want to ignore validation
-    # so we can close broken one (Ex. fork project removed)
-    if params[:merge_request] == {"state_event"=>"close"}
-      @merge_request.allow_broken = true
-
-      if @merge_request.close
-        opts = { notice: 'Merge request was successfully closed.' }
-      else
-        opts = { alert: 'Failed to close merge request.' }
-      end
-
-      redirect_to [@merge_request.target_project, @merge_request], opts
-      return
-    end
-
-    # We dont allow change of source/target projects
-    # after merge request was created
-    params[:merge_request].delete(:source_project_id)
-    params[:merge_request].delete(:target_project_id)
-
-    if @merge_request.update_attributes(params[:merge_request])
-      @merge_request.reset_events_cache
+    @merge_request = MergeRequests::UpdateService.new(project, current_user, params[:merge_request]).execute(@merge_request)
+    if @merge_request.valid?
       respond_to do |format|
         format.js
         format.html do
@@ -13,14 +13,15 @@ class Email &lt; ActiveRecord::Base
   # Relations
   #
   belongs_to :user
-  
+
   #
   # Validations
   #
   validates :user_id, presence: true
   validates :email, presence: true, email: { strict_mode: true }, uniqueness: true
   validate :unique_email, if: ->(email) { email.email_changed? }
-  
+
+  after_create :notify
   before_validation :cleanup_email
   def cleanup_email
@@ -30,4 +31,8 @@ class Email &lt; ActiveRecord::Base
   def unique_email
     self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
   end
-end
 \ No newline at end of file
+
+  def notify
+    NotificationService.new.new_email(self)
+  end
+end
@@ -29,6 +29,10 @@ class Key &lt; ActiveRecord::Base
   delegate :name, :email, to: :user, prefix: true
+  after_create :add_to_shell
+  after_create :notify_user
+  after_destroy :remove_from_shell
+
   def strip_white_space
     self.key = key.strip unless key.blank?
   end
@@ -42,6 +46,26 @@ class Key &lt; ActiveRecord::Base
     "key-#{id}"
   end
+  def add_to_shell
+    GitlabShellWorker.perform_async(
+      :add_key,
+      shell_id,
+      key
+    )
+  end
+
+  def notify_user
+    NotificationService.new.new_key(self)
+  end
+
+  def remove_from_shell
+    GitlabShellWorker.perform_async(
+      :remove_key,
+      shell_id,
+      key,
+    )
+  end
+
   private
   def generate_fingerpint
@@ -97,6 +97,7 @@ class MergeRequest &lt; ActiveRecord::Base
   validates :target_project, presence: true
   validates :target_branch, presence: true
   validate :validate_branches
+  validate :validate_fork
   scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)", group_project_ids: group.project_ids) }
   scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))", team_project_ids: team.project_ids, team_member_ids: team.member_ids) }
@@ -125,6 +126,22 @@ class MergeRequest &lt; ActiveRecord::Base
     end
   end
+  def validate_fork
+    return true unless target_project && source_project
+
+    if target_project == source_project
+      true
+    else
+      # If source and target projects are different
+      # we should check if source project is actually a fork of target project
+      if source_project.forked_from?(target_project)
+        true
+      else
+        errors.add :base, "Source project is not a fork of target project"
+      end
+    end
+  end
+
   def update_merge_request_diff
     if source_branch_changed? || target_branch_changed?
       reload_code
@@ -552,4 +552,8 @@ class Project &lt; ActiveRecord::Base
     gitlab_shell.update_repository_head(self.path_with_namespace, branch)
     reload_default_branch
   end
+
+  def forked_from?(project)
+    forked? && project == forked_from_project
+  end
 end
@@ -1,5 +0,0 @@
-class EmailObserver < BaseObserver
-  def after_create(email)
-    notification.new_email(email)
-  end
-end
@@ -1,19 +0,0 @@
-class KeyObserver < BaseObserver
-  def after_create(key)
-    GitlabShellWorker.perform_async(
-      :add_key,
-      key.shell_id,
-      key.key
-    )
-
-    notification.new_key(key)
-  end
-
-  def after_destroy(key)
-    GitlabShellWorker.perform_async(
-      :remove_key,
-      key.shell_id,
-      key.key,
-    )
-  end
-end
@@ -1,43 +0,0 @@
-class MergeRequestObserver < BaseObserver
-  def after_create(merge_request)
-    event_service.open_mr(merge_request, current_user)
-    notification.new_merge_request(merge_request, current_user)
-    merge_request.create_cross_references!(merge_request.project, current_user)
-    execute_hooks(merge_request)
-  end
-
-  def after_close(merge_request, transition)
-    event_service.close_mr(merge_request, current_user)
-    notification.close_mr(merge_request, current_user)
-    create_note(merge_request)
-    execute_hooks(merge_request)
-  end
-
-  def after_reopen(merge_request, transition)
-    event_service.reopen_mr(merge_request, current_user)
-    create_note(merge_request)
-    execute_hooks(merge_request)
-    merge_request.reload_code
-    merge_request.mark_as_unchecked
-  end
-
-  def after_update(merge_request)
-    notification.reassigned_merge_request(merge_request, current_user) if merge_request.is_being_reassigned?
-
-    merge_request.notice_added_references(merge_request.project, current_user)
-    execute_hooks(merge_request)
-  end
-
-  private
-
-  # Create merge request note with service comment like 'Status changed to closed'
-  def create_note(merge_request)
-    Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
-  end
-
-  def execute_hooks(merge_request)
-    if merge_request.project
-      merge_request.project.execute_hooks(merge_request.to_hook_data, :merge_request_hooks)
-    end
-  end
-end
@@ -0,0 +1,16 @@
+module MergeRequests
+  class BaseService < ::BaseService
+
+    private
+
+    def create_note(merge_request)
+      Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
+    end
+
+    def execute_hooks(merge_request)
+      if merge_request.project
+        merge_request.project.execute_hooks(merge_request.to_hook_data, :merge_request_hooks)
+      end
+    end
+  end
+end
@@ -0,0 +1,18 @@
+module MergeRequests
+  class CloseService < MergeRequests::BaseService
+    def execute(merge_request, commit = nil)
+      # If we close MergeRequest we want to ignore validation
+      # so we can close broken one (Ex. fork project removed)
+      merge_request.allow_broken = true
+
+      if merge_request.close
+        event_service.close_mr(merge_request, current_user)
+        notification_service.close_mr(merge_request, current_user)
+        create_note(merge_request)
+        execute_hooks(merge_request)
+      end
+
+      merge_request
+    end
+  end
+end
@@ -0,0 +1,19 @@
+module MergeRequests
+  class CreateService < MergeRequests::BaseService
+    def execute
+      merge_request = MergeRequest.new(params)
+      merge_request.source_project = project
+      merge_request.target_project ||= project
+      merge_request.author = current_user
+
+      if merge_request.save
+        event_service.open_mr(merge_request, current_user)
+        notification_service.new_merge_request(merge_request, current_user)
+        merge_request.create_cross_references!(merge_request.project, current_user)
+        execute_hooks(merge_request)
+      end
+
+      merge_request
+    end
+  end
+end
@@ -0,0 +1,15 @@
+module MergeRequests
+  class ReopenService < MergeRequests::BaseService
+    def execute(merge_request)
+      if merge_request.reopen
+        event_service.reopen_mr(merge_request, current_user)
+        create_note(merge_request)
+        execute_hooks(merge_request)
+        merge_request.reload_code
+        merge_request.mark_as_unchecked
+      end
+
+      merge_request
+    end
+  end
+end
@@ -0,0 +1,37 @@
+require_relative 'base_service'
+require_relative 'reopen_service'
+require_relative 'close_service'
+
+module MergeRequests
+  class UpdateService < MergeRequests::BaseService
+    def execute(merge_request)
+      # We dont allow change of source/target projects
+      # after merge request was created
+      params.delete(:source_project_id)
+      params.delete(:target_project_id)
+
+      state = params.delete('state_event')
+
+      case state
+      when 'reopen'
+        MergeRequests::ReopenService.new(project, current_user, {}).execute(merge_request)
+      when 'close'
+        MergeRequests::CloseService.new(project, current_user, {}).execute(merge_request)
+      end
+
+      if params.present? && merge_request.update_attributes(params)
+        merge_request.reset_events_cache
+
+        if merge_request.previous_changes.include?('assignee_id')
+          notification_service.reassigned_merge_request(merge_request, current_user)
+          create_assignee_note(merge_request)
+        end
+
+        merge_request.notice_added_references(merge_request.project, current_user)
+        execute_hooks(merge_request)
+      end
+
+      merge_request
+    end
+  end
+end
@@ -33,7 +33,7 @@
       .col-sm-10
         .clearfix
           .pull-left
-            - projects =  @project.forked_from_project.nil? ? [@project] : [ @project,@project.forked_from_project]
+            - projects =  @project.forked_from_project.nil? ? [@project] : [@project, @project.forked_from_project]
             = f.select(:target_project_id, options_from_collection_for_select(projects, 'id', 'path_with_namespace', f.object.target_project_id), {}, { class: 'target_project select2 span3', disabled: @merge_request.persisted? })
           .pull-left
             &nbsp;
@@ -21,8 +21,6 @@ module Gitlab
     # Activate observers that should always be running.
     config.active_record.observers = :milestone_observer,
                                      :project_activity_cache_observer,
-                                     :key_observer,
-                                     :merge_request_observer,
                                      :note_observer,
                                      :project_observer,
                                      :system_hook_observer,
@@ -53,15 +53,15 @@ class DashboardMergeRequests &lt; Spinach::FeatureSteps
   end
   def assigned_merge_request
-    @assigned_merge_request ||= create :merge_request, assignee: current_user, target_project: project
+    @assigned_merge_request ||= create :merge_request, assignee: current_user, target_project: project, source_project: project
   end
   def authored_merge_request
-    @authored_merge_request ||= create :merge_request, author: current_user, target_project: project
+    @authored_merge_request ||= create :merge_request, source_branch: 'simple_merge_request', author: current_user, target_project: project, source_project: project
   end
   def other_merge_request
-    @other_merge_request ||= create :merge_request, target_project: project
+    @other_merge_request ||= create :merge_request, source_branch: '2_3_notes_fix', target_project: project, source_project: project
   end
   def project
@@ -52,6 +52,4 @@ Spinach.hooks.before_run do
   RSpec::Mocks::setup self
   include FactoryGirl::Syntax::Methods
-  MergeRequestObserver.any_instance.stub(current_user: create(:user))
 end
-
@@ -13,14 +13,6 @@ module API
           end
           not_found!
         end
-
-        def not_fork?(target_project_id, user_project)
-          target_project_id.nil? || target_project_id == user_project.id.to_s
-        end
-
-        def target_matches_fork(target_project_id,user_project)
-          user_project.forked? && user_project.forked_from_project.id.to_s == target_project_id
-        end
       end
       # List merge requests
@@ -70,29 +62,15 @@ module API
       #   POST /projects/:id/merge_requests
       #
       post ":id/merge_requests" do
-        set_current_user_for_thread do
-          authorize! :write_merge_request, user_project
-          required_attributes! [:source_branch, :target_branch, :title]
-          attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description]
-          merge_request = user_project.merge_requests.new(attrs)
-          merge_request.author = current_user
-          merge_request.source_project = user_project
-          target_project_id = attrs[:target_project_id]
-          if not_fork?(target_project_id, user_project)
-            merge_request.target_project = user_project
-          else
-            if target_matches_fork(target_project_id,user_project)
-              merge_request.target_project = Project.find_by(id: attrs[:target_project_id])
-            else
-              render_api_error!('(Bad Request) Specified target project that is not the source project, or the source fork of the project.', 400)
-            end
-          end
-
-          if merge_request.save
-            present merge_request, with: Entities::MergeRequest
-          else
-            handle_merge_request_errors! merge_request.errors
-          end
+        authorize! :write_merge_request, user_project
+        required_attributes! [:source_branch, :target_branch, :title]
+        attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description]
+        merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute
+
+        if merge_request.valid?
+          present merge_request, with: Entities::MergeRequest
+        else
+          handle_merge_request_errors! merge_request.errors
         end
       end
@@ -111,17 +89,15 @@ module API
       #   PUT /projects/:id/merge_request/:merge_request_id
       #
       put ":id/merge_request/:merge_request_id" do
-        set_current_user_for_thread do
-          attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event, :description]
-          merge_request = user_project.merge_requests.find(params[:merge_request_id])
-
-          authorize! :modify_merge_request, merge_request
+        attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event, :description]
+        merge_request = user_project.merge_requests.find(params[:merge_request_id])
+        authorize! :modify_merge_request, merge_request
+        merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, attrs).execute(merge_request)
-          if merge_request.update_attributes attrs
-            present merge_request, with: Entities::MergeRequest
-          else
-            handle_merge_request_errors! merge_request.errors
-          end
+        if merge_request.valid?
+          present merge_request, with: Entities::MergeRequest
+        else
+          handle_merge_request_errors! merge_request.errors
         end
       end
@@ -5,10 +5,10 @@ describe MergeRequestsFinder do
   let(:user2) { create :user }
   let(:project1) { create(:project) }
-  let(:project2) { create(:project) }
+  let(:project2) { create(:project, forked_from_project: project1) }
-  let!(:merge_request1) { create(:merge_request, :simple, author: user, source_project: project1, target_project: project2) }
-  let!(:merge_request2) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1) }
+  let!(:merge_request1) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1) }
+  let!(:merge_request2) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1, state: 'closed') }
   let!(:merge_request3) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project2) }
   before do
@@ -21,7 +21,7 @@ describe MergeRequestsFinder do
     it 'should filter by scope' do
       params = { scope: 'authored', state: 'opened' }
       merge_requests = MergeRequestsFinder.new.execute(user, params)
-      merge_requests.size.should == 3
+      merge_requests.size.should == 2
     end
     it 'should filter by project' do
@@ -13,7 +13,7 @@ describe &#39;Gitlab::Satellite::MergeAction&#39; do
   end
   let(:project) { create(:project, namespace: create(:group)) }
-  let(:fork_project) { create(:project, namespace: create(:group)) }
+  let(:fork_project) { create(:project, namespace: create(:group), forked_from_project: project) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:merge_request_fork) { create(:merge_request, source_project: fork_project, target_project: project) }
@@ -68,4 +68,18 @@ describe Key do
       build(:invalid_key).should_not be_valid
     end
   end
+
+  context 'callbacks' do
+    it 'should add new key to authorized_file' do
+      @key = build(:personal_key, id: 7)
+      GitlabShellWorker.should_receive(:perform_async).with(:add_key, @key.shell_id, @key.key)
+      @key.save
+    end
+
+    it 'should remove key from authorized_file' do
+      @key = create(:personal_key)
+      GitlabShellWorker.should_receive(:perform_async).with(:remove_key, @key.shell_id, @key.key)
+      @key.destroy
+    end
+  end
 end
@@ -209,7 +209,7 @@ describe Note do
     let(:project)    { create(:project) }
     let(:author)     { create(:user) }
     let(:issue)      { create(:issue, project: project) }
-    let(:mergereq)   { create(:merge_request, target_project: project) }
+    let(:mergereq)   { create(:merge_request, :simple, target_project: project, source_project: project) }
     let(:commit)     { project.repository.commit }
     # Test all of {issue, merge request, commit} in both the referenced and referencing
@@ -1,17 +0,0 @@
-require 'spec_helper'
-
-describe EmailObserver do
-  let(:email)      { create(:email) }
-
-  before { subject.stub(notification: double('NotificationService').as_null_object) }
-
-  subject { EmailObserver.instance }
-
-  describe '#after_create' do
-    it 'trigger notification to send emails' do
-      subject.should_receive(:notification)
-
-      subject.after_create(email)
-    end
-  end
-end
@@ -1,23 +0,0 @@
-require 'spec_helper'
-
-describe KeyObserver do
-  before do
-    @key = create(:personal_key)
-
-    @observer = KeyObserver.instance
-  end
-
-  context :after_create do
-    it do
-      GitlabShellWorker.should_receive(:perform_async).with(:add_key, @key.shell_id, @key.key)
-      @observer.after_create(@key)
-    end
-  end
-
-  context :after_destroy do
-    it do
-      GitlabShellWorker.should_receive(:perform_async).with(:remove_key, @key.shell_id, @key.key)
-      @observer.after_destroy(@key)
-    end
-  end
-end
@@ -1,131 +0,0 @@
-require 'spec_helper'
-
-describe MergeRequestObserver do
-  let(:some_user) { create :user }
-  let(:assignee) { create :user }
-  let(:author) { create :user }
-  let(:project) { create :project }
-  let(:mr_mock) { double(:merge_request, id: 42, assignee: assignee, author: author).as_null_object }
-  let(:assigned_mr) { create(:merge_request, assignee: assignee, author: author, source_project: project) }
-  let(:unassigned_mr) { create(:merge_request, author: author, source_project: project) }
-  let(:closed_assigned_mr) { create(:closed_merge_request, assignee: assignee, author: author, source_project: project) }
-  let(:closed_unassigned_mr) { create(:closed_merge_request, author: author, source_project: project) }
-
-  before { subject.stub(:current_user).and_return(some_user) }
-  before { subject.stub(notification: double('NotificationService').as_null_object) }
-  before { mr_mock.stub(:author_id) }
-  before { mr_mock.stub(:source_project) }
-  before { mr_mock.stub(:source_project) }
-  before { mr_mock.stub(:project) }
-  before { mr_mock.stub(:create_cross_references!).and_return(true) }
-  before { Repository.any_instance.stub(commit: nil) }
-
-  before(:each) { enable_observers }
-  after(:each) { disable_observers }
-
-  subject { MergeRequestObserver.instance }
-
-  describe '#after_create' do
-    it 'trigger notification service' do
-      subject.should_receive(:notification)
-      subject.after_create(mr_mock)
-    end
-
-    it 'creates cross-reference notes' do
-       project = create :project
-       mr_mock.stub(title: "this mr references !#{assigned_mr.id}", project: project)
-       mr_mock.should_receive(:create_cross_references!).with(project, some_user)
-
-       subject.after_create(mr_mock)
-    end
-  end
-
-  context '#after_update' do
-    before(:each) do
-      mr_mock.stub(:is_being_reassigned?).and_return(false)
-      mr_mock.stub(:notice_added_references)
-    end
-
-    it 'is called when a merge request is changed' do
-      changed = create(:merge_request, source_project: project)
-      subject.should_receive(:after_update)
-
-      MergeRequest.observers.enable :merge_request_observer do
-        changed.title = 'I changed'
-        changed.save
-      end
-    end
-
-    it 'checks for new references' do
-      mr_mock.should_receive(:notice_added_references)
-
-      subject.after_update(mr_mock)
-    end
-
-    context 'a notification' do
-      it 'is sent if the merge request is being reassigned' do
-        mr_mock.should_receive(:is_being_reassigned?).and_return(true)
-        subject.should_receive(:notification)
-
-        subject.after_update(mr_mock)
-      end
-
-      it 'is not sent if the merge request is not being reassigned' do
-        mr_mock.should_receive(:is_being_reassigned?).and_return(false)
-        subject.should_not_receive(:notification)
-
-        subject.after_update(mr_mock)
-      end
-    end
-  end
-
-  context '#after_close' do
-    context 'a status "closed"' do
-      it 'note is created if the merge request is being closed' do
-        Note.should_receive(:create_status_change_note).with(assigned_mr, assigned_mr.source_project, some_user, 'closed', nil)
-
-        assigned_mr.close
-      end
-
-      it 'notification is delivered only to author if the merge request is being closed' do
-        Note.should_receive(:create_status_change_note).with(unassigned_mr, unassigned_mr.source_project, some_user, 'closed', nil)
-
-        unassigned_mr.close
-      end
-    end
-  end
-
-  context '#after_reopen' do
-    context 'a status "reopened"' do
-      it 'note is created if the merge request is being reopened' do
-        Note.should_receive(:create_status_change_note).with(closed_assigned_mr, closed_assigned_mr.source_project, some_user, 'reopened', nil)
-
-        closed_assigned_mr.reopen
-      end
-
-      it 'notification is delivered only to author if the merge request is being reopened' do
-        Note.should_receive(:create_status_change_note).with(closed_unassigned_mr, closed_unassigned_mr.source_project, some_user, 'reopened', nil)
-
-        closed_unassigned_mr.reopen
-      end
-    end
-  end
-
-  describe "Merge Request created" do
-    def self.it_should_be_valid_event
-      it { @event.should_not be_nil }
-      it { @event.should_not be_nil }
-      it { @event.project.should == project }
-      it { @event.project.should == project }
-    end
-
-    before do
-      @merge_request = create(:merge_request, source_project: project, target_project: project)
-      @event = Event.last
-    end
-
-    it_should_be_valid_event
-    it { @event.action.should == Event::CREATED }
-    it { @event.target.should == @merge_request }
-  end
-end
@@ -6,7 +6,7 @@ describe API::API do
   after(:each) { ActiveRecord::Base.observers.disable(:user_observer) }
   let(:user) { create(:user) }
   let!(:project) {create(:project, creator_id: user.id, namespace: user.namespace) }
-  let!(:merge_request) { create(:merge_request, author: user, assignee: user, source_project: project, target_project: project, title: "Test") }
+  let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test") }
   let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
   before {
     project.team << [user, :reporters]
@@ -79,16 +79,12 @@ describe API::API do
     end
     context 'forked projects' do
-      let!(:user2) {create(:user)}
-      let!(:forked_project_link) { build(:forked_project_link) }
-      let!(:fork_project) {  create(:project, forked_project_link: forked_project_link,  namespace: user2.namespace, creator_id: user2.id)  }
-      let!(:unrelated_project) {  create(:project,  namespace: create(:user).namespace, creator_id: user2.id)  }
+      let!(:user2) { create(:user) }
+      let!(:fork_project) { create(:project, forked_from_project: project,  namespace: user2.namespace, creator_id: user2.id) }
+      let!(:unrelated_project) { create(:project,  namespace: create(:user).namespace, creator_id: user2.id) }
       before :each do |each|
         fork_project.team << [user2, :reporters]
-        forked_project_link.forked_from_project = project
-        forked_project_link.forked_to_project = fork_project
-        forked_project_link.save!
       end
       it "should return merge_request" do
@@ -127,16 +123,16 @@ describe API::API do
         response.status.should == 400
       end
-      it "should return 400 when target_branch is specified and not a forked project" do
+      it "should return 404 when target_branch is specified and not a forked project" do
         post api("/projects/#{project.id}/merge_requests", user),
         title: 'Test merge_request', target_branch: 'master', source_branch: 'stable', author: user, target_project_id: fork_project.id
-        response.status.should == 400
+        response.status.should == 404
       end
-      it "should return 400 when target_branch is specified and for a different fork" do
+      it "should return 404 when target_branch is specified and for a different fork" do
         post api("/projects/#{fork_project.id}/merge_requests", user2),
         title: 'Test merge_request', target_branch: 'master', source_branch: 'stable', author: user2, target_project_id: unrelated_project.id
-        response.status.should == 400
+        response.status.should == 404
       end
       it "should return 201 when target_branch is specified and for the same project" do
@@ -5,7 +5,7 @@ describe NotificationService do
   describe 'Keys' do
     describe :new_key do
-      let(:key) { create(:personal_key) }
+      let!(:key) { create(:personal_key) }
       it { notification.new_key(key).should be_true }
@@ -18,7 +18,7 @@ describe NotificationService do
   describe 'Email' do
     describe :new_email do
-      let(:email) { create(:email) }
+      let!(:email) { create(:email) }
       it { notification.new_email(email).should be_true }
	@@ -76,10 +76,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController		@@ -76,10 +76,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
76	end	76	end
77		77
78	def create	78	def create
79	- @merge_request = MergeRequest.new(params[:merge_request])
80	- @merge_request.author = current_user
81	@target_branches \|\|= []	79	@target_branches \|\|= []
82	- if @merge_request.save	80	+ @merge_request = MergeRequests::CreateService.new(project, current_user, params[:merge_request]).execute
		81	+
		82	+ if @merge_request.valid?
83	redirect_to [@merge_request.target_project, @merge_request], notice: 'Merge request was successfully created.'	83	redirect_to [@merge_request.target_project, @merge_request], notice: 'Merge request was successfully created.'
84	else	84	else
85	@source_project = @merge_request.source_project	85	@source_project = @merge_request.source_project
	@@ -89,29 +89,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController		@@ -89,29 +89,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
89	end	89	end
90		90
91	def update	91	def update
92	- # If we close MergeRequest we want to ignore validation
93	- # so we can close broken one (Ex. fork project removed)
94	- if params[:merge_request] == {"state_event"=>"close"}
95	- @merge_request.allow_broken = true
96	-
97	- if @merge_request.close
98	- opts = { notice: 'Merge request was successfully closed.' }
99	- else
100	- opts = { alert: 'Failed to close merge request.' }
101	- end
102	-
103	- redirect_to [@merge_request.target_project, @merge_request], opts
104	- return
105	- end
106	-
107	- # We dont allow change of source/target projects
108	- # after merge request was created
109	- params[:merge_request].delete(:source_project_id)
110	- params[:merge_request].delete(:target_project_id)
111	-
112	- if @merge_request.update_attributes(params[:merge_request])
113	- @merge_request.reset_events_cache	92	+ @merge_request = MergeRequests::UpdateService.new(project, current_user, params[:merge_request]).execute(@merge_request)
114		93
		94	+ if @merge_request.valid?
115	respond_to do \|format\|	95	respond_to do \|format\|
116	format.js	96	format.js
117	format.html do	97	format.html do
	@@ -29,6 +29,10 @@ class Key < ActiveRecord::Base		@@ -29,6 +29,10 @@ class Key < ActiveRecord::Base
29		29
30	delegate :name, :email, to: :user, prefix: true	30	delegate :name, :email, to: :user, prefix: true
31		31
		32	+ after_create :add_to_shell
		33	+ after_create :notify_user
		34	+ after_destroy :remove_from_shell
		35	+
32	def strip_white_space	36	def strip_white_space
33	self.key = key.strip unless key.blank?	37	self.key = key.strip unless key.blank?
34	end	38	end
	@@ -42,6 +46,26 @@ class Key < ActiveRecord::Base		@@ -42,6 +46,26 @@ class Key < ActiveRecord::Base
42	"key-#{id}"	46	"key-#{id}"
43	end	47	end
44		48
		49	+ def add_to_shell
		50	+ GitlabShellWorker.perform_async(
		51	+ :add_key,
		52	+ shell_id,
		53	+ key
		54	+ )
		55	+ end
		56	+
		57	+ def notify_user
		58	+ NotificationService.new.new_key(self)
		59	+ end
		60	+
		61	+ def remove_from_shell
		62	+ GitlabShellWorker.perform_async(
		63	+ :remove_key,
		64	+ shell_id,
		65	+ key,
		66	+ )
		67	+ end
		68	+
45	private	69	private
46		70
47	def generate_fingerpint	71	def generate_fingerpint
	@@ -97,6 +97,7 @@ class MergeRequest < ActiveRecord::Base		@@ -97,6 +97,7 @@ class MergeRequest < ActiveRecord::Base
97	validates :target_project, presence: true	97	validates :target_project, presence: true
98	validates :target_branch, presence: true	98	validates :target_branch, presence: true
99	validate :validate_branches	99	validate :validate_branches
		100	+ validate :validate_fork
100		101
101	scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)", group_project_ids: group.project_ids) }	102	scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)", group_project_ids: group.project_ids) }
102	scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))", team_project_ids: team.project_ids, team_member_ids: team.member_ids) }	103	scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))", team_project_ids: team.project_ids, team_member_ids: team.member_ids) }
	@@ -125,6 +126,22 @@ class MergeRequest < ActiveRecord::Base		@@ -125,6 +126,22 @@ class MergeRequest < ActiveRecord::Base
125	end	126	end
126	end	127	end
127		128
		129	+ def validate_fork
		130	+ return true unless target_project && source_project
		131	+
		132	+ if target_project == source_project
		133	+ true
		134	+ else
		135	+ # If source and target projects are different
		136	+ # we should check if source project is actually a fork of target project
		137	+ if source_project.forked_from?(target_project)
		138	+ true
		139	+ else
		140	+ errors.add :base, "Source project is not a fork of target project"
		141	+ end
		142	+ end
		143	+ end
		144	+
128	def update_merge_request_diff	145	def update_merge_request_diff
129	if source_branch_changed? \|\| target_branch_changed?	146	if source_branch_changed? \|\| target_branch_changed?
130	reload_code	147	reload_code
	@@ -552,4 +552,8 @@ class Project < ActiveRecord::Base		@@ -552,4 +552,8 @@ class Project < ActiveRecord::Base
552	gitlab_shell.update_repository_head(self.path_with_namespace, branch)	552	gitlab_shell.update_repository_head(self.path_with_namespace, branch)
553	reload_default_branch	553	reload_default_branch
554	end	554	end
		555	+
		556	+ def forked_from?(project)
		557	+ forked? && project == forked_from_project
		558	+ end
555	end	559	end
	@@ -1,5 +0,0 @@	@@ -1,5 +0,0 @@
1	-class EmailObserver < BaseObserver
2	- def after_create(email)
3	- notification.new_email(email)
4	- end
5	-end
	@@ -1,19 +0,0 @@	@@ -1,19 +0,0 @@
1	-class KeyObserver < BaseObserver
2	- def after_create(key)
3	- GitlabShellWorker.perform_async(
4	- :add_key,
5	- key.shell_id,
6	- key.key
7	- )
8	-
9	- notification.new_key(key)
10	- end
11	-
12	- def after_destroy(key)
13	- GitlabShellWorker.perform_async(
14	- :remove_key,
15	- key.shell_id,
16	- key.key,
17	- )
18	- end
19	-end
	@@ -1,43 +0,0 @@	@@ -1,43 +0,0 @@
1	-class MergeRequestObserver < BaseObserver
2	- def after_create(merge_request)
3	- event_service.open_mr(merge_request, current_user)
4	- notification.new_merge_request(merge_request, current_user)
5	- merge_request.create_cross_references!(merge_request.project, current_user)
6	- execute_hooks(merge_request)
7	- end
8	-
9	- def after_close(merge_request, transition)
10	- event_service.close_mr(merge_request, current_user)
11	- notification.close_mr(merge_request, current_user)
12	- create_note(merge_request)
13	- execute_hooks(merge_request)
14	- end
15	-
16	- def after_reopen(merge_request, transition)
17	- event_service.reopen_mr(merge_request, current_user)
18	- create_note(merge_request)
19	- execute_hooks(merge_request)
20	- merge_request.reload_code
21	- merge_request.mark_as_unchecked
22	- end
23	-
24	- def after_update(merge_request)
25	- notification.reassigned_merge_request(merge_request, current_user) if merge_request.is_being_reassigned?
26	-
27	- merge_request.notice_added_references(merge_request.project, current_user)
28	- execute_hooks(merge_request)
29	- end
30	-
31	- private
32	-
33	- # Create merge request note with service comment like 'Status changed to closed'
34	- def create_note(merge_request)
35	- Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
36	- end
37	-
38	- def execute_hooks(merge_request)
39	- if merge_request.project
40	- merge_request.project.execute_hooks(merge_request.to_hook_data, :merge_request_hooks)
41	- end
42	- end
43	-end
@@ -0,0 +1,16 @@		@@ -0,0 +1,16 @@
	1	+module MergeRequests
	2	+ class BaseService < ::BaseService
	3	+
	4	+ private
	5	+
	6	+ def create_note(merge_request)
	7	+ Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
	8	+ end
	9	+
	10	+ def execute_hooks(merge_request)
	11	+ if merge_request.project
	12	+ merge_request.project.execute_hooks(merge_request.to_hook_data, :merge_request_hooks)
	13	+ end
	14	+ end
	15	+ end
	16	+end
@@ -0,0 +1,18 @@		@@ -0,0 +1,18 @@
	1	+module MergeRequests
	2	+ class CloseService < MergeRequests::BaseService
	3	+ def execute(merge_request, commit = nil)
	4	+ # If we close MergeRequest we want to ignore validation
	5	+ # so we can close broken one (Ex. fork project removed)
	6	+ merge_request.allow_broken = true
	7	+
	8	+ if merge_request.close
	9	+ event_service.close_mr(merge_request, current_user)
	10	+ notification_service.close_mr(merge_request, current_user)
	11	+ create_note(merge_request)
	12	+ execute_hooks(merge_request)
	13	+ end
	14	+
	15	+ merge_request
	16	+ end
	17	+ end
	18	+end
@@ -0,0 +1,19 @@		@@ -0,0 +1,19 @@
	1	+module MergeRequests
	2	+ class CreateService < MergeRequests::BaseService
	3	+ def execute
	4	+ merge_request = MergeRequest.new(params)
	5	+ merge_request.source_project = project
	6	+ merge_request.target_project \|\|= project
	7	+ merge_request.author = current_user
	8	+
	9	+ if merge_request.save
	10	+ event_service.open_mr(merge_request, current_user)
	11	+ notification_service.new_merge_request(merge_request, current_user)
	12	+ merge_request.create_cross_references!(merge_request.project, current_user)
	13	+ execute_hooks(merge_request)
	14	+ end
	15	+
	16	+ merge_request
	17	+ end
	18	+ end
	19	+end