From cef127e10778a21756c00c4226592f32f15a6c1f Mon Sep 17 00:00:00 2001 From: Jacopo Date: Thu, 30 May 2019 12:50:40 +0200 Subject: Excludes MR author from Review roulette Excludes MR author from gitlab_ui and single_codebase Review roulette results. --- ...ouldn-t-include-the-author-as-a-possibility.yml | 5 +++ danger/gitlab_ui_wg/Dangerfile | 29 +++++++++------ danger/roulette/Dangerfile | 8 ++-- danger/single_codebase/Dangerfile | 14 +++---- lib/gitlab/danger/helper.rb | 4 ++ lib/gitlab/danger/roulette.rb | 20 +++++----- spec/lib/gitlab/danger/helper_spec.rb | 10 +++++ spec/lib/gitlab/danger/roulette_spec.rb | 43 ++++++++++++++++++++++ 8 files changed, 100 insertions(+), 33 deletions(-) create mode 100644 changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml diff --git a/changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml b/changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml new file mode 100644 index 00000000000..8d1a38b3db5 --- /dev/null +++ b/changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml @@ -0,0 +1,5 @@ +--- +title: Excludes MR author from Review roulette +merge_request: 28886 +author: Jacopo Beschi @jacopo-beschi +type: fixed diff --git a/danger/gitlab_ui_wg/Dangerfile b/danger/gitlab_ui_wg/Dangerfile index 02d94fa5ab7..672b1deecb3 100644 --- a/danger/gitlab_ui_wg/Dangerfile +++ b/danger/gitlab_ui_wg/Dangerfile @@ -1,29 +1,36 @@ +FRONTEND_MAINTAINERS = %w[filipa iamphill psimyn sarahghp mishunov].freeze +UX_MAINTAINERS = %w[tauriedavis rverissimo].freeze +NO_REVIEWER = 'No reviewer available'.freeze + def mention_single_codebase_approvers - frontend_maintainers = %w(@filipa @iamphill @psimyn @sarahghp @mishunov) - ux_maintainers = %w(@tauriedavis @rverissimo) + canonical_branch_name = + roulette.canonical_branch_name(gitlab.mr_json['source_branch']) + + random = roulette.new_random(canonical_branch_name) + + frontend_maintainers = helper.new_teammates(FRONTEND_MAINTAINERS) + ux_maintainers = helper.new_teammates(UX_MAINTAINERS) rows = [] - users = [] if gitlab.mr_labels.include?('frontend') - frontend_maintainer = frontend_maintainers.sample + frontend_maintainer = + roulette.spin_for_person(frontend_maintainers, random: random) - rows << "| ~frontend | `#{frontend_maintainer}`" - users << frontend_maintainer + rows << "| ~frontend | #{frontend_maintainer&.markdown_name || NO_REVIEWER}" end if gitlab.mr_labels.include?('UX') - ux_maintainers = ux_maintainers.sample + ux_maintainers = + roulette.spin_for_person(ux_maintainers, random: random) - rows << "| ~UX | `#{ux_maintainers}`" - users << ux_maintainers + rows << "| ~UX | #{ux_maintainers&.markdown_name || NO_REVIEWER}" end if rows.empty? backup_maintainer = frontend_maintainers.sample - rows << "| ~frontend / ~UX | `#{backup_maintainer}`" - users << backup_maintainer + rows << "| ~frontend / ~UX | #{backup_maintainer.markdown_name}" end markdown(<<~MARKDOWN.strip) diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile index a0f1447e76a..6718e218233 100644 --- a/danger/roulette/Dangerfile +++ b/danger/roulette/Dangerfile @@ -31,6 +31,9 @@ Please consider creating a merge request to for them. MARKDOWN +NO_REVIEWER = 'No reviewer available'.freeze +NO_MAINTAINER = 'No maintainer available'.freeze + def spin_for_category(team, project, category, branch_name) random = roulette.new_random(branch_name) labels = gitlab.mr_labels @@ -49,7 +52,7 @@ def spin_for_category(team, project, category, branch_name) reviewer = roulette.spin_for_person(reviewers + traintainers + traintainers, random: random) maintainer = roulette.spin_for_person(maintainers, random: random) - "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name} | #{maintainer&.markdown_name} |" + "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name || NO_REVIEWER} | #{maintainer&.markdown_name || NO_MAINTAINER} |" end def build_list(items) @@ -85,9 +88,6 @@ if changes.any? && !gitlab.mr_labels.include?('single codebase') && !gitlab.mr_l [] end - # Exclude the MR author from the team for selection purposes - team.delete_if { |teammate| teammate.username == gitlab.mr_author } - project = helper.project_name unknown = changes.fetch(:unknown, []) diff --git a/danger/single_codebase/Dangerfile b/danger/single_codebase/Dangerfile index d1f538bec7f..f371a42e9b1 100644 --- a/danger/single_codebase/Dangerfile +++ b/danger/single_codebase/Dangerfile @@ -1,6 +1,6 @@ -def new_teammates(usernames) - usernames.map { |u| ::Gitlab::Danger::Teammate.new('username' => u) } -end +FRONTEND_MAINTAINERS = %w[filipa iamphill].freeze +BACKEND_MAINTAINERS = %w[rspeicher rymai yorickpeterse godfat].freeze +NO_REVIEWER = 'No reviewer available'.freeze def mention_single_codebase_approvers canonical_branch_name = @@ -8,8 +8,8 @@ def mention_single_codebase_approvers random = roulette.new_random(canonical_branch_name) - frontend_maintainers = new_teammates(%w[filipa iamphill]) - backend_maintainers = new_teammates(%w[rspeicher rymai yorickpeterse godfat]) + frontend_maintainers = helper.new_teammates(FRONTEND_MAINTAINERS) + backend_maintainers = helper.new_teammates(BACKEND_MAINTAINERS) rows = [] @@ -17,14 +17,14 @@ def mention_single_codebase_approvers frontend_maintainer = roulette.spin_for_person(frontend_maintainers, random: random) - rows << "| ~frontend | #{frontend_maintainer.markdown_name}" + rows << "| ~frontend | #{frontend_maintainer&.markdown_name || NO_REVIEWER}" end if gitlab.mr_labels.include?('backend') backend_maintainer = roulette.spin_for_person(backend_maintainers, random: random) - rows << "| ~backend | #{backend_maintainer.markdown_name}" + rows << "| ~backend | #{backend_maintainer&.markdown_name || NO_REVIEWER}" end if rows.empty? diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb index 7a0fb419f8e..1ecf4a12db7 100644 --- a/lib/gitlab/danger/helper.rb +++ b/lib/gitlab/danger/helper.rb @@ -124,6 +124,10 @@ module Gitlab %r{\.(md|txt)\z} => :none, # To reinstate roulette for documentation, set to `:docs`. %r{\.js\z} => :frontend }.freeze + + def new_teammates(usernames) + usernames.map { |u| Gitlab::Danger::Teammate.new('username' => u) } + end end end end diff --git a/lib/gitlab/danger/roulette.rb b/lib/gitlab/danger/roulette.rb index 062eda38ee4..25de0a87c9d 100644 --- a/lib/gitlab/danger/roulette.rb +++ b/lib/gitlab/danger/roulette.rb @@ -45,21 +45,19 @@ module Gitlab # Known issue: If someone is rejected due to OOO, and then becomes not OOO, the # selection will change on next spin def spin_for_person(people, random:) - person = nil - people = people.dup - - people.size.times do - person = people.sample(random: random) - - break person unless out_of_office?(person) + people.shuffle(random: random) + .find(&method(:valid_person?)) + end - people -= [person] - end + private - person + def valid_person?(person) + !mr_author?(person) && !out_of_office?(person) end - private + def mr_author?(person) + person.username == gitlab.mr_author + end def out_of_office?(person) username = CGI.escape(person.username) diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb index f7642182a17..22e52901758 100644 --- a/spec/lib/gitlab/danger/helper_spec.rb +++ b/spec/lib/gitlab/danger/helper_spec.rb @@ -202,4 +202,14 @@ describe Gitlab::Danger::Helper do it { is_expected.to eq(expected_label) } end end + + describe '#new_teammates' do + it 'returns an array of Teammate' do + usernames = %w[filipa iamphil] + + teammates = helper.new_teammates(usernames) + + expect(teammates.map(&:username)).to eq(usernames) + end + end end diff --git a/spec/lib/gitlab/danger/roulette_spec.rb b/spec/lib/gitlab/danger/roulette_spec.rb index 40dce0c5378..121c5d8ecd9 100644 --- a/spec/lib/gitlab/danger/roulette_spec.rb +++ b/spec/lib/gitlab/danger/roulette_spec.rb @@ -98,4 +98,47 @@ describe Gitlab::Danger::Roulette do is_expected.to contain_exactly(ce_teammate_matcher) end end + + describe '#spin_for_person' do + let(:person1) { Gitlab::Danger::Teammate.new('username' => 'rymai') } + let(:person2) { Gitlab::Danger::Teammate.new('username' => 'godfat') } + let(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa') } + let(:ooo) { Gitlab::Danger::Teammate.new('username' => 'jacopo-beschi') } + + before do + stub_person_message(person1, 'making GitLab magic') + stub_person_message(person2, 'making GitLab magic') + stub_person_message(ooo, 'OOO till 15th') + # we don't stub Filipa, as she is the author and + # we should not fire request checking for her + + allow(subject).to receive_message_chain(:gitlab, :mr_author).and_return(author.username) + end + + it 'returns a random person' do + persons = [person1, person2] + + selected = subject.spin_for_person(persons, random: Random.new) + + expect(selected.username).to be_in(persons.map(&:username)) + end + + it 'excludes OOO persons' do + expect(subject.spin_for_person([ooo], random: Random.new)).to be_nil + end + + it 'excludes mr.author' do + expect(subject.spin_for_person([author], random: Random.new)).to be_nil + end + + private + + def stub_person_message(person, message) + body = { message: message }.to_json + + WebMock + .stub_request(:get, "https://gitlab.com/api/v4/users/#{person.username}/status") + .to_return(body: body) + end + end end -- cgit v1.2.3