Addresses backend review comments52499-mc-make-diffs-requests-and-rendering-smarter

- Changes 'commit' to diffable, hopefully to make it compatible with
other diffs
- Renames diffs_per_paths to diffs_per_batch as it's more descriptive
- Abstract EnvironmentFinder method so it can be reused across different
controller actions.
- Additionally make all the files in batches render as expanded and load
the notes (if any).
- Makes batch_size an attribute of FileCollection::Batch
- Renames argument on decorate_batch!

6 files changed, 42 insertions, 21 deletions

diff --git a/app/assets/javascripts/commit_page_loader.js b/app/assets/javascripts/commit_page_loader.js
index 39064c6e4fb..75bba01b75f 100644
--- a/app/assets/javascripts/commit_page_loader.js
+++ b/app/assets/javascripts/commit_page_loader.js
@@ -2,6 +2,8 @@ import $ from 'jquery';
 
 import { isScrolledToBottom } from '~/lib/utils/scroll_utils';
 import axios from './lib/utils/axios_utils';
+import syntaxHighlight from './syntax_highlight';
+import FilesCommentButton from './files_comment_button';
 
 export default class CommitPageLoader {
   constructor() {
@@ -19,7 +21,7 @@ export default class CommitPageLoader {
   }
 
   callDiffForPaths() {
-    const url = `/${gon.project_full_path}/commit/${gon.commit_sha}/diff_for_paths`;
+    const url = `/${gon.project_full_path}/commit/${gon.commit_sha}/diffs_per_batch`;
     const divID = `.next-batch-section[data-next-batch="${this.currentBatch}"]`;
 
     return axios
@@ -31,7 +33,16 @@ export default class CommitPageLoader {
       .then(({ data }) => {
         if (data.html) {
           $(divID).html(data.html);
+          syntaxHighlight($(divID));
+          this.bindDiffFiles();
         }
       });
   }
+
+  bindDiffFiles() {
+    const divID = `.next-batch-section[data-next-batch="${this.currentBatch}"] .diff-file`;
+    const $batchOfFiles = $(divID);
+
+    FilesCommentButton.init($batchOfFiles);
+  }
 }
diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb
index 04cc8ae1569..d58305e6452 100644
--- a/app/controllers/concerns/diff_for_path.rb
+++ b/app/controllers/concerns/diff_for_path.rb
@@ -13,7 +13,7 @@ module DiffForPath
     render json: { html: view_to_html_string('projects/diffs/_content', diff_file: diff_file) }
   end
 
-  def render_diff_for_paths(commit, project, environment, batch_number)
+  def render_diff_per_batch(commit, project, environment, batch_number)
     options = diff_options.merge(batch_number: batch_number)
 
     file_collection = Gitlab::Diff::FileCollection::Batch.new(commit,
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index f66bfe90538..3ca23bd1a2a 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -17,6 +17,7 @@ class Projects::CommitController < Projects::ApplicationController
   before_action :define_commit_vars, only: [:show, :diff_for_path, :pipelines, :merge_requests]
   before_action :define_note_vars, only: [:show, :diff_for_path]
   before_action :authorize_edit_tree!, only: [:revert, :cherry_pick]
+  before_action :environment, except: [:branches, :revert, :cherry_pick]
   before_action :define_gon_variables, only: [:show]
 
   BRANCH_SEARCH_LIMIT = 1000
@@ -41,11 +42,13 @@ class Projects::CommitController < Projects::ApplicationController
     render_diff_for_path(@commit.diffs(diff_options))
   end
 
-  def diff_for_paths
+  def diffs_per_batch
     batch_number = params[:batch_number].to_i
 
-    render_diff_for_paths(
-      commit, project, environment_for_batch_diff, batch_number)
+    @grouped_diff_discussions = commit.grouped_diff_discussions
+
+    render_diff_per_batch(
+      commit, project, environment, batch_number)
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
@@ -201,10 +204,6 @@ class Projects::CommitController < Projects::ApplicationController
     @commit_params = { commit: @commit }
   end
 
-  def environment_for_batch_diff
-    @environment ||= EnvironmentsFinder.new(@project, current_user, commit: @commit).execute.last
-  end
-
   def define_gon_variables
     gon.push(commit_gon_variables)
   end
@@ -217,4 +216,8 @@ class Projects::CommitController < Projects::ApplicationController
       commit_sha: params[:id]
     }
   end
+
+  def environment
+    @environment ||= EnvironmentsFinder.new(@project, current_user, commit: @commit).execute.last
+  end
 end
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 5e1cbe2ed07..3d42debd141 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -178,7 +178,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           post :cherry_pick
           get :diff_for_path
           get :merge_requests
-          get :diff_for_paths
+          get :diffs_per_batch
         end
       end
 
diff --git a/lib/gitlab/diff/file_collection/batch.rb b/lib/gitlab/diff/file_collection/batch.rb
index f113c5a2fc0..f1622facd78 100644
--- a/lib/gitlab/diff/file_collection/batch.rb
+++ b/lib/gitlab/diff/file_collection/batch.rb
@@ -4,13 +4,20 @@ module Gitlab
   module Diff
     module FileCollection
       class Batch < Base
-        def initialize(commit, diff_options:)
+        def initialize(diffable, diff_options:)
           @batch_number = diff_options.fetch(:batch_number, 0)
+          @batch_size = if @batch_number.zero?
+                          ::Commit::INITIAL_FILES_BATCH
+                        else
+                          ::Commit::FILES_PER_BATCH
+                        end
 
-          super(commit,
-                project: commit.project,
+          diff_options[:expanded] = true unless @batch_number.zero?
+
+          super(diffable,
+                project: diffable.project,
                 diff_options: diff_options,
-                diff_refs: commit.diff_refs)
+                diff_refs: diffable.diff_refs)
         end
 
         def diff_files
@@ -19,21 +26,21 @@ module Gitlab
 
         private
 
-        attr_reader :batch_number
+        attr_reader :batch_number, :batch_size
 
         def initial_diff_file
-          if @batch_number.zero?
+          if batch_number.zero?
             0
           else
-            last_diff_file - (::Commit::FILES_PER_BATCH - 1)
+            last_diff_file - (batch_size - 1)
           end
         end
 
         def last_diff_file
           if @batch_number.zero?
-            ::Commit::INITIAL_FILES_BATCH
+            batch_size
           else
-            ::Commit::INITIAL_FILES_BATCH + (::Commit::FILES_PER_BATCH * @batch_number)
+            batch_size + (batch_size * batch_number)
           end
         end
       end
diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb
index a0b0d943522..4264195167a 100644
--- a/lib/gitlab/git/diff_collection.rb
+++ b/lib/gitlab/git/diff_collection.rb
@@ -87,12 +87,12 @@ module Gitlab
         collection
       end
 
-      def decorate_batch!(start_diff, end_diff)
+      def decorate_batch!(start_diff_index, end_diff_index)
         collection = each_with_index do |element, i|
           @array[i] = yield(element)
         end
 
-        @array = @array[start_diff..end_diff]
+        @array = @array[start_diff_index..end_diff_index]
         collection
       end