Merge pull request #1448 from alphagov/simplify-commits

Use commit history instead of compare API call

app/controllers/applications_controller.rb

@@ -19,14 +19,7 @@ def show
       format.json { render json: @application }
       format.html do
         @outstanding_dependency_pull_requests = @application.dependency_pull_requests[:total_count]
-
-        @tag_names_by_commit = @application.tag_names_by_commit
-
-        @commits = if @application.deployments.last_deploy_to(@application.live_environment)
-                     @application.undeployed_commits
-                   else
-                     @application.commits
-                   end
+        @commits = @application.commit_history
 
         @github_available = true
       rescue Octokit::TooManyRequests

app/models/application.rb

@@ -79,7 +79,41 @@ def dependency_pull_requests
   end
 
   def commits
-    Services.github.commits(repo_path)
+    @commits ||= Services.github.commits(repo_path, { per_page: 50 })
+  end
+
+  def commit_history
+    commit_older_than_live_environment = false
+    tags_by_commit = tag_names_by_commit
+
+    commits.filter_map do |commit|
+      unless commit_older_than_live_environment
+        tags = tags_by_commit.fetch(commit[:sha], [])
+        deployed_to = []
+
+        latest_deploys_by_environment.each do |environment, deployment|
+          if tags.include?(deployment.version) || deployment.commit_match?(commit[:sha])
+            commit_older_than_live_environment = true if environment == live_environment
+            deployed_to << deployment
+          end
+        end
+
+        {
+          deployed_to:,
+          tags:,
+          message: commit[:commit][:message].split(/\n/)[0],
+          author: commit.dig(:commit, :author, :name),
+          sha: commit[:sha],
+          github_url: "#{repo_url}/commit/#{commit[:sha]}",
+        }
+      end
+    end
+  end
+
+  def environment_on_default_branch(environment)
+    commit_history.any? do |commit|
+      commit[:deployed_to].map(&:environment).include?(environment)
+    end
   end
 
   def tag_names_by_commit
@@ -92,18 +126,6 @@ def tag_names_by_commit
     end
   end
 
-  def undeployed_commits
-    production_deployment = deployments.last_deploy_to(live_environment)
-
-    comparison = Services.github.compare(
-      repo_path,
-      production_deployment.version,
-      default_branch,
-    )
-    # The `compare` API shows commits in forward chronological order
-    comparison.commits.reverse + [comparison.base_commit]
-  end
-
   def live_environment
     "production"
   end

app/views/applications/show.html.erb

@@ -25,23 +25,19 @@
       <%= t.body do %>
         <% @commits.each do |commit| %>
           <%= t.row do %>
-            <% tags = @tag_names_by_commit.fetch(commit[:sha], []) %>
             <% commit_deployments = capture do %>
-              <% @application.latest_deploys_by_environment.each do |environment, deployment| %>
-                <% if tags.include?(deployment.version) || deployment.commit_match?(commit[:sha]) %>
-                  <% latest_deploy_on_default_branch << environment %>
-                  <p class="govuk-body-xs govuk-!-margin-bottom-1 release__commits-message">
-                    <span class="release__commits-label release__commits-label--<%= 'production' if deployment.to_live_environment? %>"><%= deployment.environment.humanize %></span>
-                    <span>at <%= time_tag(deployment.created_at, human_datetime(deployment.created_at)) %></span>
-                  </p>
-                <% end %>
+              <% commit[:deployed_to].each do |deployment| %>
+                <p class="govuk-body-xs govuk-!-margin-bottom-1 release__commits-message">
+                  <span class="release__commits-label release__commits-label--<%= 'production' if deployment.to_live_environment? %>"><%= deployment.environment.humanize %></span>
+                  <span>at <%= time_tag(deployment.created_at, human_datetime(deployment.created_at)) %></span>
+                </p>
               <% end %>
             <% end %>
 
             <%= t.cell commit_deployments || "" %>
 
             <% commit_tags = capture do %>
-              <% tags.each do |tag| %>
+              <% commit[:tags].each do |tag| %>
                 <a class="govuk-link govuk-body-s" href="<%= deploy_application_path(@application) %>?tag=<%= tag %>"><%= tag %></a>
               <% end %>
             <% end %>
@@ -50,17 +46,17 @@
 
             <% commit_message = capture do %>
               <p class="govuk-body-s govuk-!-margin-bottom-0">
-                <%= commit[:commit][:message].split(/\n/)[0] %>
-                <% if commit[:commit][:author] %>
+                <%= commit[:message] %>
+                <% if commit[:author] %>
                   <span class="release__commits-author">
-                    <%= commit[:commit][:author][:name] %>
+                    <%= commit[:author] %>
                   </span>
                 <% end %>
               </p>
             <% end %>
 
             <%= t.cell commit_message %>
-            <%= t.cell link_to(commit[:sha][0..8], "#{@application.repo_url}/commit/#{commit[:sha]}", target: "_blank", class: "release__commit-hash govuk-link govuk-body-s") %>
+            <%= t.cell link_to(commit[:sha][0..8], commit[:github_url], target: "_blank", class: "release__commit-hash govuk-link govuk-body-s") %>
           <% end %>
         <% end %>
       <% end %>
@@ -110,8 +106,8 @@
   <%= t.body do %>
     <% @application.latest_deploys_by_environment.each do |environment, deployment| %>
       <% not_on_default_branch = capture do %>
-        <% unless latest_deploy_on_default_branch.include?(environment) %>
-            <span class="release__badge release__badge--orange">Not on default branch</span>
+        <% unless @application.environment_on_default_branch(environment)  %>
+          <span class="release__badge release__badge--orange">Not on default branch</span>
         <% end %>
       <% end %>
       <%= t.row do %>

test/functional/applications_controller_test.rb

@@ -90,7 +90,7 @@ class ApplicationsControllerTest < ActionController::TestCase
       stub_request(:get, "http://docs.publishing.service.gov.uk/apps.json").to_return(status: 200, body: "", headers: {})
       @app = FactoryBot.create(:application)
       stub_request(:get, "https://api.github.com/repos/#{@app.repo_path}/tags").to_return(body: [])
-      stub_request(:get, "https://api.github.com/repos/#{@app.repo_path}/commits").to_return(body: [])
+      stub_request(:get, "https://api.github.com/repos/#{@app.repo_path}/commits").with(query: { "per_page": "50" }).to_return(body: [])
 
       Octokit::Client.any_instance.stubs(:search_issues)
         .with("repo:#{@app.repo_path} is:pr state:open label:dependencies")
@@ -146,13 +146,9 @@ class ApplicationsControllerTest < ActionController::TestCase
         FactoryBot.create(:deployment, application: @app, environment: "staging", version:, deployed_sha: @deployed_sha)
         FactoryBot.create(:deployment, application: @app, environment: "integration", version: @manual_deploy)
 
-        Octokit::Client.any_instance.stubs(:compare)
-          .with(@app.repo_path, version, @app.default_branch)
-          .returns(stub(
-                     "comparison",
-                     commits: [@first_commit],
-                     base_commit: @base_commit,
-                   ))
+        Octokit::Client.any_instance.stubs(:commits)
+          .with(@app.repo_path, { per_page: 50 })
+          .returns([@first_commit, @base_commit])
       end
 
       should "show 'not on default branch' status" do
@@ -169,13 +165,9 @@ class ApplicationsControllerTest < ActionController::TestCase
         @base_commit = stub_commit
         stub_request(:get, "http://docs.publishing.service.gov.uk/apps.json").to_return(status: 200, body: "", headers: {})
         FactoryBot.create(:deployment, application: @app, version:, deployed_sha: @first_commit[:sha])
-        Octokit::Client.any_instance.stubs(:compare)
-          .with(@app.repo_path, version, @app.default_branch)
-          .returns(stub(
-                     "comparison",
-                     commits: [@first_commit, @second_commit],
-                     base_commit: @base_commit,
-                   ))
+        Octokit::Client.any_instance.stubs(:commits)
+          .with(@app.repo_path, { per_page: 50 })
+          .returns([@second_commit, @first_commit, @base_commit])
       end
 
       should "show the application" do
@@ -198,7 +190,7 @@ class ApplicationsControllerTest < ActionController::TestCase
       should "include the base commit" do
         get :show, params: { id: @app.id }
 
-        assert_equal @base_commit[:sha], assigns[:commits].last[:sha]
+        assert_equal @first_commit[:sha], assigns[:commits].last[:sha]
       end
     end
 

test/integration/global_status_notes_test.rb

@@ -70,7 +70,7 @@ def stub_deploy_and_release_page_api_requests_for(application, tag)
           },
         },
       ])
-    stub_request(:get, "https://api.github.com/repos/#{application.repo_path}/commits").to_return(body:
+    stub_request(:get, "https://api.github.com/repos/#{application.repo_path}/commits?per_page=50").to_return(body:
       [
         {
           "sha": "1234567890",