Merge branch 'cached-mr-commit-partials' into 'master'

Cache the commit partials in MRs [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!61617

app/helpers/commits_helper.rb

@@ -148,6 +148,27 @@ module CommitsHelper
     end
   end
 
+  # This is used to calculate a cache key for the app/views/projects/commits/_commit.html.haml
+  # partial. It takes some of the same parameters as used in the partial and will hash the
+  # current pipeline status.
+  #
+  # This includes a keyed hash for values that can be nil, to prevent invalid cache entries
+  # being served if the order should change in future.
+  def commit_partial_cache_key(commit, ref:, merge_request:, request:)
+    [
+      commit,
+      commit.author,
+      ref,
+      {
+        merge_request: merge_request,
+        pipeline_status: hashed_pipeline_status(commit, ref),
+        xhr: request.xhr?,
+        controller: controller.controller_path,
+        path: @path # referred to in #link_to_browse_code
+      }
+    ]
+  end
+
   protected
 
   # Private: Returns a link to a person. If the person has a matching user and
@@ -221,4 +242,14 @@ module CommitsHelper
       project_commit_path(project, commit)
     end
   end
+
+  private
+
+  def hashed_pipeline_status(commit, ref)
+    status = commit.status_for(ref)
+
+    return if status.nil?
+
+    Digest::SHA1.hexdigest(status.to_s)
+  end
 end

app/views/projects/commits/_commit.html.haml

@@ -3,22 +3,24 @@
   - `assets/javascripts/diffs/components/commit_item.vue`
 
   EXCEPTION WARNING - see above `.vue` file for de-sync drift
--#-----------------------------------------------------------------
-- view_details  = local_assigns.fetch(:view_details, false)
-- merge_request = local_assigns.fetch(:merge_request, nil)
-- project       = local_assigns.fetch(:project) { merge_request&.project }
-- ref           = local_assigns.fetch(:ref) { merge_request&.source_branch }
-- commit        = commit.present(current_user: current_user)
-- commit_status = commit.status_for(ref)
-- collapsible = local_assigns.fetch(:collapsible, true)
-- link_data_attrs = local_assigns.fetch(:link_data_attrs, {})
-
-- link = commit_path(project, commit, merge_request: merge_request)
 
+  WARNING: When introducing new content here, please consider what
+           changes may need to be made in the cache keys used to
+           wrap this view, found in
+           CommitsHelper#commit_partial_cache_key
+-#-----------------------------------------------------------------
+- view_details      = local_assigns.fetch(:view_details, false)
+- merge_request     = local_assigns.fetch(:merge_request, nil)
+- project           = local_assigns.fetch(:project) { merge_request&.project }
+- ref               = local_assigns.fetch(:ref) { merge_request&.source_branch }
+- commit            = commit.present(current_user: current_user)
+- commit_status     = commit.status_for(ref)
+- collapsible       = local_assigns.fetch(:collapsible, true)
+- link_data_attrs   = local_assigns.fetch(:link_data_attrs, {})
+- link              = commit_path(project, commit, merge_request: merge_request)
 - show_project_name = local_assigns.fetch(:show_project_name, false)
 
 %li{ class: ["commit flex-row", ("js-toggle-container" if collapsible)], id: "commit-#{commit.short_id}" }
-
   .avatar-cell.d-none.d-sm-block
     = author_avatar(commit, size: 40, has_tooltip: false)
 

app/views/projects/commits/_commits.html.haml

@@ -3,8 +3,8 @@
 - ref = local_assigns.fetch(:ref) { merge_request&.source_branch }
 - can_update_merge_request = can?(current_user, :update_merge_request, @merge_request)
 
-- commits = @commits
-- context_commits = @context_commits
+- commits = @commits&.map { |commit| commit.present(current_user: current_user) }
+- context_commits = @context_commits&.map { |commit| commit.present(current_user: current_user) }
 - hidden = @hidden_commit_count
 
 - commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, daily_commits|
@@ -14,7 +14,10 @@
 
   %li.commits-row{ data: { day: day } }
     %ul.content-list.commit-list.flex-list
-      = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }
+      - if Feature.enabled?(:cached_commits, project, default_enabled: :yaml)
+        = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, merge_request: merge_request, request: request) }
+      - else
+        = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }
 
 - if context_commits.present?
   %li.commit-header.js-commit-header
@@ -25,7 +28,10 @@
 
   %li.commits-row
     %ul.content-list.commit-list.flex-list
-      = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }
+      - if Feature.enabled?(:cached_commits, project, default_enabled: :yaml)
+        = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, merge_request: merge_request, request: request) }
+      - else
+        = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }
 
 - if hidden > 0
   %li.gl-alert.gl-alert-warning

config/feature_flags/development/cached_commits.yml

+---
+name: cached_commits
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61617
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330968
+milestone: '13.12'
+type: development
+group: group::source code
+default_enabled: false

spec/helpers/commits_helper_spec.rb

@@ -289,4 +289,38 @@ RSpec.describe CommitsHelper do
       }
     end
   end
+
+  describe "#commit_partial_cache_key" do
+    subject { helper.commit_partial_cache_key(commit, ref: ref, merge_request: merge_request, request: request) }
+
+    let(:commit) { create(:commit).present(current_user: user) }
+    let(:commit_status) { create(:commit_status) }
+    let(:user) { create(:user) }
+    let(:ref) { "master" }
+    let(:merge_request) { nil }
+    let(:request) { double(xhr?: true) }
+    let(:current_path) { "test" }
+
+    before do
+      expect(commit).to receive(:status_for).with(ref).and_return(commit_status)
+      assign(:path, current_path)
+    end
+
+    it { is_expected.to be_an(Array) }
+    it { is_expected.to include(commit) }
+    it { is_expected.to include(commit.author) }
+    it { is_expected.to include(ref) }
+
+    it do
+      is_expected.to include(
+        {
+          merge_request: merge_request,
+          pipeline_status: Digest::SHA1.hexdigest(commit_status.to_s),
+          xhr: true,
+          controller: "commits",
+          path: current_path
+        }
+      )
+    end
+  end
 end