Use MergeRequest services in API and controllers

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/controllers/projects/merge_requests_controller.rb

@@ -76,10 +76,10 @@ class Projects::MergeRequestsController < 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 < 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

app/models/merge_request.rb

@@ -97,6 +97,7 @@ class MergeRequest < 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,20 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def validate_fork
+    if target_projet == 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

app/models/project.rb

@@ -552,4 +552,8 @@ class Project < 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

app/views/projects/merge_requests/_form.html.haml

@@ -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
              

lib/api/merge_requests.rb

@@ -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
+        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.save
-            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
 
@@ -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])
+        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)
 
-          authorize! :modify_merge_request, 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