Some updates after last code review.

9 files changed, 67 insertions, 52 deletions

diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 1818947eb70..4d7b1b80ded 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -13,16 +13,10 @@ module CreatesCommit
     result = service.new(@tree_edit_project, current_user, commit_params).execute
 
     if result[:status] == :success
-      flash[:notice] = success_notice || "Your changes have been successfully committed."
-
-      if create_merge_request?
-        success_path = merge_request_exists? ? existent_merge_request_path : new_merge_request_path
-        target = different_project? ? "project" : "branch"
-        flash[:notice] << " You can now submit a merge request to get this change into the original #{target}."
-      end
+      update_flash_notice(success_notice)
 
       respond_to do |format|
-        format.html { redirect_to success_path }
+        format.html { redirect_to final_success_path(success_path) }
         format.json { render json: { message: "success", filePath: success_path } }
       end
     else
@@ -41,14 +35,32 @@ module CreatesCommit
   end
 
   def authorize_edit_tree!
-    return if can?(current_user, :push_code, project)
-    return if current_user && current_user.already_forked?(project)
+    return if can_collaborate_with_project?
 
     access_denied!
   end
 
   private
 
+  def update_flash_notice(success_notice)
+    flash[:notice] = success_notice || "Your changes have been successfully committed."
+
+    if create_merge_request?
+      if merge_request_exists?
+        flash[:notice] = nil
+      else
+        target = different_project? ? "project" : "branch"
+        flash[:notice] << " You can now submit a merge request to get this change into the original #{target}."
+      end
+    end
+  end
+
+  def final_success_path(success_path)
+    return success_path unless create_merge_request?
+
+    merge_request_exists? ? existing_merge_request_path : new_merge_request_path
+  end
+
   def new_merge_request_path
     new_namespace_project_merge_request_path(
       @mr_source_project.namespace,
@@ -62,15 +74,15 @@ module CreatesCommit
     )
   end
 
-  def existent_merge_request_path
+  def existing_merge_request_path
     namespace_project_merge_request_path(@mr_target_project.namespace, @mr_target_project, @merge_request)
   end
 
   def merge_request_exists?
-    @merge_request = @mr_target_project.merge_requests.opened.where(
-                       source_branch: @mr_source_branch,
-                       target_branch: @mr_target_branch
-                     ).first
+    return @merge_request if defined?(@merge_request)
+
+    @merge_request = @mr_target_project.merge_requests.opened.find_by(
+                       source_branch: @mr_source_branch, target_branch: @mr_target_branch)
   end
 
   def different_project?
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index a326bc58215..e436aed1240 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -3,6 +3,7 @@ class Projects::ApplicationController < ApplicationController
   before_action :repository
   layout 'project'
 
+  helper_method :can_collaborate_with_project?
   def authenticate_user!
     # Restrict access to Projects area only
     # for non-signed users
@@ -36,4 +37,11 @@ class Projects::ApplicationController < ApplicationController
   def builds_enabled
     return render_404 unless @project.builds_enabled?
   end
+
+  def can_collaborate_with_project?(project = nil)
+    project ||= @project
+
+    can?(current_user, :push_code, project) ||
+      (current_user && current_user.already_forked?(project))
+  end
 end
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index aeb4bcabc7d..b37bdaf5f07 100644
--- a/app/helpers/commits_helper.rb
+++ b/app/helpers/commits_helper.rb
@@ -123,11 +123,28 @@ module CommitsHelper
     )
   end
 
-  def can_collaborate_with_project?(project = nil)
-    project ||= @project
-
-    can?(current_user, :push_code, project) ||
-      (current_user && current_user.already_forked?(project))
+  def revert_commit_link(show_modal_condition, continue_to_path)
+    if show_modal_condition
+      link_to('Revert', '#modal-revert-commit',
+        'data-target' => '#modal-revert-commit',
+        'data-toggle' => 'modal',
+        class: 'btn btn-grouped btn-close',
+        title: 'Create merge request to revert commit'
+      )
+    else
+      continue_params = {
+        to: continue_to_path,
+        notice: edit_in_new_fork_notice + ' Try to revert this commit again.',
+        notice_now: edit_in_new_fork_notice_now
+      }
+      fork_path = namespace_project_forks_path(@project.namespace, @project,
+                    namespace_key: current_user.namespace.id,
+                    continue: continue_params
+                  )
+
+      link_to 'Revert', fork_path, class: 'btn btn-grouped btn-close', method: :post,
+        title: 'Create merge request to revert commit'
+    end
   end
 
   protected
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index b4858b999d5..f5d90b7791d 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -96,7 +96,6 @@ module MergeRequestsHelper
   def can_update_merge_request?
     project ||= @project
 
-    can?(current_user, :update_merge_request, project) ||
-      (current_user && current_user.already_forked?(project))
+    can_collaborate_with_project?(project)
   end
 end
diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb
index 2ad7c80dae0..4920ca5af6e 100644
--- a/app/helpers/tree_helper.rb
+++ b/app/helpers/tree_helper.rb
@@ -56,8 +56,7 @@ module TreeHelper
 
     return false unless on_top_of_branch?(project, ref)
 
-    can?(current_user, :push_code, project) ||
-      (current_user && current_user.already_forked?(project))
+    can_collaborate_with_project?(project)
   end
 
   def tree_edit_branch(project = @project, ref = @ref)
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 25007c7f15a..3c0e042e2ed 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -220,17 +220,17 @@ class Commit
   end
 
   def revert_message
-    "Revert \"#{safe_message.lines.first.chomp}\"".truncate(80) + "\n\nReverts #{to_reference}"
+    "Revert \"#{title}\"".truncate(80) + "\n\nReverts #{sha}"
   end
 
-  def is_a_merge_commit?
+  def merge_commit?
     parents.size > 1
   end
 
   def merged_merge_request
     return @merged_merge_request if defined?(@merged_merge_request)
 
-    @merged_merge_request = is_a_merge_commit? && MergeRequest.where(merge_commit_sha: id).first
+    @merged_merge_request = merge_commit? && MergeRequest.find_by(merge_commit_sha: id)
   end
 
   private
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 27e3da24559..b570afaa32c 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -626,7 +626,7 @@ class Repository
     source_sha    = find_branch(base_branch).target
     target_branch = create_mr ? commit.revert_branch_name : base_branch
     args          = [commit.id, source_sha]
-    args          << { mainline: 1 } if commit.is_a_merge_commit?
+    args          << { mainline: 1 } if commit.merge_commit?
 
     # Temporary branch exists and contains the revert commit
     return true if create_mr && find_branch(target_branch)
@@ -638,14 +638,14 @@ class Repository
 
     commit_with_hooks(user, target_branch) do |ref|
       committer = user_to_committer(user)
-      source_sha = Rugged::Commit.create(rugged, {
+      source_sha = Rugged::Commit.create(rugged,
         message: commit.revert_message,
         author: committer,
         committer: committer,
         tree: revert_index.write_tree(rugged),
         parents: [rugged.lookup(source_sha)],
         update_ref: ref
-      })
+      )
     end
   end
 
diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml
index 3dc0fefe407..95d6465adfc 100644
--- a/app/views/projects/commit/_commit_box.html.haml
+++ b/app/views/projects/commit/_commit_box.html.haml
@@ -16,17 +16,7 @@
     = link_to namespace_project_tree_path(@project.namespace, @project, @commit), class: "btn btn-grouped" do
       = icon('files-o')
       Browse Files
-    - if can_collaborate_with_project?
-      = link_to '#modal-revert-commit', { 'data-target' => '#modal-revert-commit', 'data-toggle' => 'modal', class: 'btn btn-grouped btn-close'} do
-        Revert
-    - else
-      - continue_params = { to:         namespace_project_commit_path(@project.namespace, @project, @commit.id),
-                            notice:     edit_in_new_fork_notice,
-                            notice_now: edit_in_new_fork_notice_now }
-      - fork_path = namespace_project_forks_path(@project.namespace, @project, namespace_key:  current_user.namespace.id,
-                                                                              continue:       continue_params)
-      = link_to fork_path, { class: 'btn btn-grouped', method: :post } do
-        Revert
+    = revert_commit_link(can_collaborate_with_project?, namespace_project_commit_path(@project.namespace, @project, @commit.id))
   %div
 
 %p
diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml
index a6aac25ca8c..32c9b19d8c2 100644
--- a/app/views/projects/merge_requests/show/_mr_title.html.haml
+++ b/app/views/projects/merge_requests/show/_mr_title.html.haml
@@ -20,14 +20,4 @@
         = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: 'btn btn-nr btn-grouped btn-reopen reopen-mr-link', title: "Reopen merge request"
 
     - if @merge_request.merged? && @merge_request.merge_commit_sha.present?
-      - if can_update_merge_request?
-        = link_to '#modal-revert-commit', { 'data-target' => '#modal-revert-commit', 'data-toggle' => 'modal', class: 'btn btn-grouped btn-close'} do
-          Revert
-      - else
-        - continue_params = { to:         namespace_project_merge_request_path(@project.namespace, @project, @merge_request),
-                              notice:     edit_in_new_fork_notice,
-                              notice_now: edit_in_new_fork_notice_now }
-        - fork_path = namespace_project_forks_path(@project.namespace, @project, namespace_key:  current_user.namespace.id,
-                                                                                 continue:       continue_params)
-        = link_to fork_path, { class: 'btn btn-grouped btn-close', method: :post } do
-          Revert
+      = revert_commit_link(can_update_merge_request?, namespace_project_merge_request_path(@project.namespace, @project, @merge_request))