Fix specs and make tweaks

app/controllers/projects/merge_requests_controller.rb

@@ -100,39 +100,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     respond_to do |format|
       format.html { define_discussion_vars }
       format.json do
-        @merge_request_diff =
-          if params[:diff_id]
-            @merge_request.merge_request_diffs.viewable.find(params[:diff_id])
-          else
-            @merge_request.merge_request_diff
-          end
-
+        define_diff_vars
         define_diff_comment_vars
 
-        @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff
-        @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
-
-        if params[:start_sha].present?
-          @start_sha = params[:start_sha]
-          @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha }
-
-          unless @start_version
-            @start_sha = @merge_request_diff.head_commit_sha
-            @start_version = @merge_request_diff
-          end
-        end
-
         @environment = @merge_request.environments_for(current_user).last
 
-        @diff_notes_disabled = !@merge_request_diff.latest? || @start_sha
-
-        @diffs =
-          if @start_sha
-            @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
-          else
-            @merge_request_diff.diffs(diff_options)
-          end
-
         render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
       end
     end
@@ -144,16 +116,18 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def diff_for_path
     if params[:id]
       merge_request
+      define_diff_vars
       define_diff_comment_vars
     else
       build_merge_request
+      @diffs = @merge_request.diffs(diff_options)
       @diff_notes_disabled = true
       @grouped_diff_discussions = {}
     end
 
     define_commit_vars
 
-    render_diff_for_path(@merge_request.diffs(diff_options))
+    render_diff_for_path(@diffs)
   end
 
   def commits
@@ -590,12 +564,43 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
   end
 
+  def define_diff_vars
+    @merge_request_diff =
+      if params[:diff_id]
+        @merge_request.merge_request_diffs.viewable.find(params[:diff_id])
+      else
+        @merge_request.merge_request_diff
+      end
+
+    @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff
+    @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
+
+    if params[:start_sha].present?
+      @start_sha = params[:start_sha]
+      @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha }
+
+      unless @start_version
+        @start_sha = @merge_request_diff.head_commit_sha
+        @start_version = @merge_request_diff
+      end
+    end
+
+    @diffs =
+      if @start_sha
+        @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
+      else
+        @merge_request_diff.diffs(diff_options)
+      end
+  end
+
   def define_diff_comment_vars
     @new_diff_note_attrs = {
       noteable_type: 'MergeRequest',
       noteable_id: @merge_request.id
     }
 
+    @diff_notes_disabled = !@merge_request_diff.latest? || @start_sha
+
     @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
 
     @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@merge_request_diff.diff_refs)

app/helpers/notes_helper.rb

@@ -68,6 +68,8 @@ module NotesHelper
       elsif merge_request_diff = discussion.latest_merge_request_diff
         diff_id = merge_request_diff.id
       else
+        # If the discussion is not active, and we cannot find the latest
+        # merge request diff for this discussion, we return no path at all.
         return
       end
 

app/models/merge_request.rb

@@ -369,7 +369,7 @@ class MergeRequest < ActiveRecord::Base
 
   def merge_request_diff_for(diff_refs)
     @merge_request_diffs_by_diff_refs ||= Hash.new do |h, diff_refs|
-      h[diff_refs] = merge_request_diffs.viewable.select_without_diff.with_diff_refs(diff_refs).take
+      h[diff_refs] = merge_request_diffs.viewable.select_without_diff.find_by_diff_refs(diff_refs)
     end
 
     @merge_request_diffs_by_diff_refs[diff_refs]

app/models/merge_request_diff.rb

@@ -26,12 +26,15 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   scope :viewable, -> { without_state(:empty) }
-  scope :with_diff_refs, ->(diff_refs) { where(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha) }
 
   # All diff information is collected from repository after object is created.
   # It allows you to override variables like head_commit_sha before getting diff.
   after_create :save_git_content, unless: :importing?
 
+  def self.find_by_diff_refs(diff_refs)
+    where(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha)
+  end
+
   def self.select_without_diff
     select(column_names - ['st_diffs'])
   end

app/views/discussions/_discussion.html.haml

@@ -26,14 +26,15 @@
               - commit = discussion.noteable
               - if commit
                 commit
-                = link_to commit.short_id, discussion_diff_path(discussion), class: 'monospace'
+                = link_to commit.short_id, url, class: 'monospace'
               - else
                 a deleted commit
             - else
-              - unless discussion.active?
-                a now outdated portion of
               = conditional_link_to url.present?, url do
-                the diff
+                - if discussion.active?
+                  the diff
+                - else
+                  an outdated diff
 
             = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
           = render "discussions/headline", discussion: discussion

app/views/projects/merge_requests/show/_versions.html.haml

@@ -80,7 +80,7 @@
         - if @start_sha
           Comment creation is disabled because you're comparing two versions of this merge request.
         - else
-          Discussions on this old version of the merge request are displayed but comment creation has been disabled.
+          Discussions on this old version of the merge request are displayed but comment creation is disabled.
 
         .pull-right
           = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'

spec/features/merge_requests/merge_request_versions_spec.rb

@@ -37,7 +37,7 @@ feature 'Merge Request versions', js: true, feature: true do
     end
 
     it 'show the message about disabled comments' do
-      expect(page).to have_content 'Comments are disabled'
+      expect(page).to have_content 'comment creation is disabled'
     end
   end
 
@@ -66,7 +66,7 @@ feature 'Merge Request versions', js: true, feature: true do
     end
 
     it 'show the message about disabled comments' do
-      expect(page).to have_content 'Comments are disabled'
+      expect(page).to have_content 'Comment creation is disabled'
     end
 
     it 'show diff between new and old version' do