From f9bb48c63df71475ad0335812474448f684ac10b Mon Sep 17 00:00:00 2001
From: Winnie Hellmann <winnie@gitlab.com>
Date: Wed, 16 Oct 2019 06:30:29 +0000
Subject: [PATCH] Move out_of_office? from reviewer roulette to Teammate class
---
Dangerfile | 1 +
lib/gitlab/danger/request_helper.rb | 23 +++++++++
lib/gitlab/danger/roulette.rb | 33 +++----------
lib/gitlab/danger/teammate.rb | 14 ++++++
spec/lib/gitlab/danger/teammate_spec.rb | 66 ++++++++++++++++++++++++-
5 files changed, 109 insertions(+), 28 deletions(-)
create mode 100644 lib/gitlab/danger/request_helper.rb
diff --git a/Dangerfile b/Dangerfile
index 228190cd530a1..b65a9074078a0 100644
--- a/Dangerfile
+++ b/Dangerfile
@@ -1,6 +1,7 @@
# frozen_string_literal: true
require_relative 'lib/gitlab_danger'
+require_relative 'lib/gitlab/danger/request_helper'
danger.import_plugin('danger/plugins/helper.rb')
danger.import_plugin('danger/plugins/roulette.rb')
diff --git a/lib/gitlab/danger/request_helper.rb b/lib/gitlab/danger/request_helper.rb
new file mode 100644
index 0000000000000..06da4ed9ad36b
--- /dev/null
+++ b/lib/gitlab/danger/request_helper.rb
@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require 'net/http'
+require 'json'
+
+module Gitlab
+ module Danger
+ module RequestHelper
+ HTTPError = Class.new(RuntimeError)
+
+ # @param [String] url
+ def self.http_get_json(url)
+ rsp = Net::HTTP.get_response(URI.parse(url))
+
+ unless rsp.is_a?(Net::HTTPOK)
+ raise HTTPError, "Failed to read #{url}: #{rsp.code} #{rsp.message}"
+ end
+
+ JSON.parse(rsp.body)
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/danger/roulette.rb b/lib/gitlab/danger/roulette.rb
index 25de0a87c9dfa..0700a72c918cd 100644
--- a/lib/gitlab/danger/roulette.rb
+++ b/lib/gitlab/danger/roulette.rb
@@ -1,16 +1,11 @@
# frozen_string_literal: true
-require 'net/http'
-require 'json'
-require 'cgi'
-
require_relative 'teammate'
module Gitlab
module Danger
module Roulette
ROULETTE_DATA_URL = 'https://about.gitlab.com/roulette.json'
- HTTPError = Class.new(RuntimeError)
# Looks up the current list of GitLab team members and parses it into a
# useful form
@@ -19,7 +14,7 @@ module Roulette
def team
@team ||=
begin
- data = http_get_json(ROULETTE_DATA_URL)
+ data = Gitlab::Danger::RequestHelper.http_get_json(ROULETTE_DATA_URL)
data.map { |hash| ::Gitlab::Danger::Teammate.new(hash) }
rescue JSON::ParserError
raise "Failed to parse JSON response from #{ROULETTE_DATA_URL}"
@@ -44,6 +39,7 @@ def new_random(seed)
# Known issue: If someone is rejected due to OOO, and then becomes not OOO, the
# selection will change on next spin
+ # @param [Array<Teammate>] people
def spin_for_person(people, random:)
people.shuffle(random: random)
.find(&method(:valid_person?))
@@ -51,32 +47,17 @@ def spin_for_person(people, random:)
private
+ # @param [Teammate] person
+ # @return [Boolean]
def valid_person?(person)
- !mr_author?(person) && !out_of_office?(person)
+ !mr_author?(person) && !person.out_of_office?
end
+ # @param [Teammate] person
+ # @return [Boolean]
def mr_author?(person)
person.username == gitlab.mr_author
end
-
- def out_of_office?(person)
- username = CGI.escape(person.username)
- api_endpoint = "https://gitlab.com/api/v4/users/#{username}/status"
- response = http_get_json(api_endpoint)
- response["message"]&.match?(/OOO/i)
- rescue HTTPError, JSON::ParserError
- false # this is no worse than not checking for OOO
- end
-
- def http_get_json(url)
- rsp = Net::HTTP.get_response(URI.parse(url))
-
- unless rsp.is_a?(Net::HTTPSuccess)
- raise HTTPError, "Failed to read #{url}: #{rsp.code} #{rsp.message}"
- end
-
- JSON.parse(rsp.body)
- end
end
end
end
diff --git a/lib/gitlab/danger/teammate.rb b/lib/gitlab/danger/teammate.rb
index 4ad66f61c2b30..c0a2d909f69be 100644
--- a/lib/gitlab/danger/teammate.rb
+++ b/lib/gitlab/danger/teammate.rb
@@ -1,5 +1,7 @@
# frozen_string_literal: true
+require 'cgi'
+
module Gitlab
module Danger
class Teammate
@@ -34,6 +36,18 @@ def maintainer?(project, category, labels)
has_capability?(project, category, :maintainer, labels)
end
+ def status
+ api_endpoint = "https://gitlab.com/api/v4/users/#{CGI.escape(username)}/status"
+ @status ||= Gitlab::Danger::RequestHelper.http_get_json(api_endpoint)
+ rescue Gitlab::Danger::RequestHelper::HTTPError, JSON::ParserError
+ nil # better no status than a crashing Danger
+ end
+
+ # @return [Boolean]
+ def out_of_office?
+ status&.dig("message")&.match?(/OOO/i) || false
+ end
+
private
def has_capability?(project, category, kind, labels)
diff --git a/spec/lib/gitlab/danger/teammate_spec.rb b/spec/lib/gitlab/danger/teammate_spec.rb
index ca036390bde09..36486cbbc7d0d 100644
--- a/spec/lib/gitlab/danger/teammate_spec.rb
+++ b/spec/lib/gitlab/danger/teammate_spec.rb
@@ -2,11 +2,13 @@
require 'fast_spec_helper'
+require 'rspec-parameterized'
+
require 'gitlab/danger/teammate'
describe Gitlab::Danger::Teammate do
- subject { described_class.new(options) }
- let(:options) { { 'projects' => projects, 'role' => role } }
+ subject { described_class.new(options.stringify_keys) }
+ let(:options) { { username: 'luigi', projects: projects, role: role } }
let(:projects) { { project => capabilities } }
let(:role) { 'Engineer, Manage' }
let(:labels) { [] }
@@ -95,4 +97,64 @@
expect(subject.maintainer?(project, :frontend, labels)).to be_falsey
end
end
+
+ describe '#status' do
+ let(:capabilities) { ['dish washing'] }
+
+ context 'with empty cache' do
+ context 'for successful request' do
+ it 'returns the response' do
+ mock_status = double(does_not: 'matter')
+ expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
+ .and_return(mock_status)
+
+ expect(subject.status).to be mock_status
+ end
+ end
+
+ context 'for failing request' do
+ it 'returns nil' do
+ expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
+ .and_raise(Gitlab::Danger::RequestHelper::HTTPError.new)
+
+ expect(subject.status).to be nil
+ end
+ end
+ end
+
+ context 'with filled cache' do
+ it 'returns the cached response' do
+ mock_status = double(does_not: 'matter')
+ expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
+ .and_return(mock_status)
+ subject.status
+
+ expect(Gitlab::Danger::RequestHelper).not_to receive(:http_get_json)
+ expect(subject.status).to be mock_status
+ end
+ end
+ end
+
+ describe '#out_of_office?' do
+ using RSpec::Parameterized::TableSyntax
+
+ let(:capabilities) { ['dry head'] }
+
+ where(:status, :result) do
+ nil | false
+ {} | false
+ { message: 'dear reader' } | false
+ { message: 'OOO: massage' } | true
+ { message: 'love it SOOO much' } | true
+ end
+
+ with_them do
+ before do
+ expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
+ .and_return(status&.stringify_keys)
+ end
+
+ it { expect(subject.out_of_office?).to be result }
+ end
+ end
end
--
GitLab