Merge branch '61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility' into 'master'

Review roulette excludes mr_author

Closes #61157

See merge request gitlab-org/gitlab-ce!28886

changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml

+---
+title: Excludes MR author from Review roulette
+merge_request: 28886
+author: Jacopo Beschi @jacopo-beschi
+type: fixed

danger/gitlab_ui_wg/Dangerfile

+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)

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, [])
 

danger/single_codebase/Dangerfile

-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?

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

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)

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

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