From e367a204ebf2f47e15868e0e33333f737b311dca Mon Sep 17 00:00:00 2001 From: Daniel Schierbeck Date: Wed, 5 Sep 2018 15:12:25 +0200 Subject: [PATCH 1/4] List statuses in release details --- app/models/changeset.rb | 2 +- app/views/changeset/_statuses.html.erb | 3 +++ app/views/releases/row_content.html.erb | 5 +++++ 3 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 app/views/changeset/_statuses.html.erb diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 10611d8e9a..35346917ef 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -2,7 +2,7 @@ class Changeset attr_reader :repo, :previous_commit, :commit BRANCH_TAGS = ["master", "develop"].freeze - ATTRIBUTE_TABS = %w[files commits pull_requests risks jira_issues].freeze + ATTRIBUTE_TABS = %w[files commits pull_requests statuses risks jira_issues].freeze def initialize(repo, previous_commit, commit) @repo = repo diff --git a/app/views/changeset/_statuses.html.erb b/app/views/changeset/_statuses.html.erb new file mode 100644 index 0000000000..55e95831d6 --- /dev/null +++ b/app/views/changeset/_statuses.html.erb @@ -0,0 +1,3 @@ +<% @release.github_status.statuses.each do |status| %> + <%= github_commit_status_icon(status.state) %> <%= status.context %>: <%= link_to status.description, status.url %>
+<% end %> diff --git a/app/views/releases/row_content.html.erb b/app/views/releases/row_content.html.erb index c879d3f139..e4843b4cff 100644 --- a/app/views/releases/row_content.html.erb +++ b/app/views/releases/row_content.html.erb @@ -4,6 +4,7 @@
  • Pull Requests
  • Commits
  • Files
    • +
  • Statuses
  • Risks
  • JIRA Issues
    • @@ -26,6 +27,10 @@ <%= render 'changeset/files', changeset: @changeset %> +
    + <%= render 'changeset/statuses', changeset: @changeset %> +
    +
    <%= render 'changeset/risks', changeset: @changeset, type: "release" %>
    From 1e3c297c2744187d7aa0195b9a7bfb6a09216e1f Mon Sep 17 00:00:00 2001 From: Daniel Schierbeck Date: Wed, 12 Sep 2018 16:10:36 +0200 Subject: [PATCH 2/4] Ignore a Brakeman warning This is safe enough. --- config/brakeman.ignore | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/config/brakeman.ignore b/config/brakeman.ignore index a868b3999f..cb67728780 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,7 +1,25 @@ { "ignored_warnings": [ - + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "50350274f9c62a91f27562722e2191833e489c5eb093c411425b2472b6b7dbf2", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/changeset/_statuses.html.erb", + "line": 2, + "link": "https://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to((Unresolved Model).new.description, (Unresolved Model).new.url)", + "render_path": [{"type":"controller","class":"ReleasesController","method":"show","line":10,"file":"app/controllers/releases_controller.rb"},{"type":"template","name":"releases/row_content","line":31,"file":"app/views/releases/row_content.html.erb"}], + "location": { + "type": "template", + "template": "changeset/_statuses" + }, + "user_input": "(Unresolved Model).new.url", + "confidence": "Weak", + "note": "" + } ], - "updated": "2018-01-12 08:38:04 -0800", - "brakeman_version": "4.1.1" + "updated": "2018-09-12 16:04:57 +0200", + "brakeman_version": "4.3.1" } From f80bd42cd279a4d6bf44976f6a4ba9b879a7a2d6 Mon Sep 17 00:00:00 2001 From: Daniel Schierbeck Date: Wed, 12 Sep 2018 16:18:03 +0200 Subject: [PATCH 3/4] Share a partial --- app/views/changeset/_statuses.html.erb | 2 +- app/views/releases/row_content.html.erb | 2 +- app/views/releases/show.html.erb | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/views/changeset/_statuses.html.erb b/app/views/changeset/_statuses.html.erb index 55e95831d6..2bcc00eaeb 100644 --- a/app/views/changeset/_statuses.html.erb +++ b/app/views/changeset/_statuses.html.erb @@ -1,3 +1,3 @@ -<% @release.github_status.statuses.each do |status| %> +<% github_status.statuses.each do |status| %> <%= github_commit_status_icon(status.state) %> <%= status.context %>: <%= link_to status.description, status.url %>
    <% end %> diff --git a/app/views/releases/row_content.html.erb b/app/views/releases/row_content.html.erb index e4843b4cff..ec5b04f072 100644 --- a/app/views/releases/row_content.html.erb +++ b/app/views/releases/row_content.html.erb @@ -28,7 +28,7 @@
    - <%= render 'changeset/statuses', changeset: @changeset %> + <%= render 'changeset/statuses', github_status: @release.github_status %>
    diff --git a/app/views/releases/show.html.erb b/app/views/releases/show.html.erb index dedfb181f6..2b2dafda87 100644 --- a/app/views/releases/show.html.erb +++ b/app/views/releases/show.html.erb @@ -18,9 +18,7 @@ Commit Status

    - <% @release.github_status.statuses.each do |status| %> - <%= github_commit_status_icon(status.state) %> <%= status.context %>: <%= link_to status.description, status.url %>
    - <% end %> + <%= render "changeset/statuses", github_status: @release.github_status %>

    <% if @changeset.empty? %> From 86519db76deb79a5898cb987be416e0729ad9315 Mon Sep 17 00:00:00 2001 From: Daniel Schierbeck Date: Wed, 12 Sep 2018 16:51:39 +0200 Subject: [PATCH 4/4] Refactor to allow getting statuses for a changeset --- app/models/changeset.rb | 4 ++++ app/models/github_status.rb | 28 +++++++++++++------------ app/models/release.rb | 2 +- app/views/changeset/_statuses.html.erb | 2 +- app/views/releases/row_content.html.erb | 2 +- app/views/releases/show.html.erb | 2 +- test/models/github_status_test.rb | 4 ++-- 7 files changed, 25 insertions(+), 19 deletions(-) diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 35346917ef..6a86bf35b9 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -34,6 +34,10 @@ def pull_requests @pull_requests ||= find_pull_requests end + def github_status + @github_status ||= GithubStatus.for_reference(repo, commit) + end + def risks? risky_pull_requests.any? end diff --git a/app/models/github_status.rb b/app/models/github_status.rb index 27a6828e6a..2d774e55c7 100644 --- a/app/models/github_status.rb +++ b/app/models/github_status.rb @@ -33,16 +33,7 @@ def initialize(state, statuses) @statuses = statuses end - def self.fetch(release) - repo = release.project.repository_path - ref = release.commit - - # Base the cache key on the Release, so that an update to it effectively - # clears the cache. - cache_key = [name, release] - - response = Rails.cache.read(cache_key) - + def self.for_reference(repo, ref) # Fetch the data if the cache returned nil. response ||= begin GITHUB.combined_status(repo, ref) @@ -57,12 +48,23 @@ def self.fetch(release) Status.new(context, statuses.max_by { |status| status.created_at.to_i }) end + new(response.state, statuses) + end + + def self.for_release(release) + # Base the cache key on the Release, so that an update to it effectively + # clears the cache. + cache_key = [name, release, "v2"] + + status = Rails.cache.read(cache_key) + status ||= for_reference(release.project.repository_path, release.commit) + # Don't cache pending statuses, since we expect updates soon. - unless statuses.any?(&:pending?) - Rails.cache.write(cache_key, response, expires_in: 1.hour) + unless status.statuses.any?(&:pending?) + Rails.cache.write(cache_key, status, expires_in: 1.hour) end - new(response.state, statuses) + status end def success? diff --git a/app/models/release.rb b/app/models/release.rb index cfd025a460..48f5c331c3 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -33,7 +33,7 @@ def to_param end def github_status - @github_status ||= GithubStatus.fetch(self) + @github_status ||= GithubStatus.for_release(self) end def self.find_by_param!(version) diff --git a/app/views/changeset/_statuses.html.erb b/app/views/changeset/_statuses.html.erb index 2bcc00eaeb..c3b590f913 100644 --- a/app/views/changeset/_statuses.html.erb +++ b/app/views/changeset/_statuses.html.erb @@ -1,3 +1,3 @@ -<% github_status.statuses.each do |status| %> +<% changeset.github_status.statuses.each do |status| %> <%= github_commit_status_icon(status.state) %> <%= status.context %>: <%= link_to status.description, status.url %>
    <% end %> diff --git a/app/views/releases/row_content.html.erb b/app/views/releases/row_content.html.erb index ec5b04f072..e4843b4cff 100644 --- a/app/views/releases/row_content.html.erb +++ b/app/views/releases/row_content.html.erb @@ -28,7 +28,7 @@
    - <%= render 'changeset/statuses', github_status: @release.github_status %> + <%= render 'changeset/statuses', changeset: @changeset %>
    diff --git a/app/views/releases/show.html.erb b/app/views/releases/show.html.erb index 2b2dafda87..f43a852fcf 100644 --- a/app/views/releases/show.html.erb +++ b/app/views/releases/show.html.erb @@ -18,7 +18,7 @@ Commit Status

    - <%= render "changeset/statuses", github_status: @release.github_status %> + <%= render "changeset/statuses", changeset: @changeset %>

    <% if @changeset.empty? %> diff --git a/test/models/github_status_test.rb b/test/models/github_status_test.rb index 20d5d6ac51..d0faa32be0 100644 --- a/test/models/github_status_test.rb +++ b/test/models/github_status_test.rb @@ -10,7 +10,7 @@ let(:release) { stub("release", project: project, commit: ref) } describe "#state" do - let(:status) { GithubStatus.fetch(release) } + let(:status) { GithubStatus.for_release(release) } it "returns `missing` if there's no response from Github" do stub_api({}, 401) @@ -24,7 +24,7 @@ end describe "#statuses" do - let(:status) { GithubStatus.fetch(release) } + let(:status) { GithubStatus.for_release(release) } it "returns a single status per context" do # The most recent status is used.