Merge branch...

Merge branch '228853-reviewer-roulette-default-to-previous-roulette-if-no-timezone-reviewer-maintainer-found' into 'master'

Fallback to random-based roulette when no one is picked otherwise

Closes #228853

See merge request gitlab-org/gitlab!36948

danger/roulette/Dangerfile

@@ -42,23 +42,29 @@ OPTIONAL_REVIEW_TEMPLATE = "%{role} review is optional for %{category}".freeze
 NOT_AVAILABLE_TEMPLATE = 'No %{role} available'.freeze
 TIMEZONE_EXPERIMENT = true
 
-def mr_author
-  roulette.team.find { |person| person.username == gitlab.mr_author }
+def note_for_spins_role(spins, role)
+  spins.each do |spin|
+    note = note_for_spin_role(spin, role)
+
+    return note if note
+  end
+
+  NOT_AVAILABLE_TEMPLATE % { role: role }
 end
 
-def note_for_category_role(spin, role)
+def note_for_spin_role(spin, role)
   if spin.optional_role == role
     return OPTIONAL_REVIEW_TEMPLATE % { role: role.capitalize, category: helper.label_for_category(spin.category) }
   end
 
-  spin.public_send(role)&.markdown_name(timezone_experiment: TIMEZONE_EXPERIMENT, author: mr_author) || NOT_AVAILABLE_TEMPLATE % { role: role } # rubocop:disable GitlabSecurity/PublicSend
+  spin.public_send(role)&.markdown_name(timezone_experiment: spin.timezone_experiment, author: roulette.team_mr_author) # rubocop:disable GitlabSecurity/PublicSend
 end
 
-def markdown_row_for_spin(spin)
-  reviewer_note = note_for_category_role(spin, :reviewer)
-  maintainer_note = note_for_category_role(spin, :maintainer)
+def markdown_row_for_spins(category, spins_array)
+  reviewer_note = note_for_spins_role(spins_array, :reviewer)
+  maintainer_note = note_for_spins_role(spins_array, :maintainer)
 
-  "| #{helper.label_for_category(spin.category)} | #{reviewer_note} | #{maintainer_note} |"
+  "| #{helper.label_for_category(category)} | #{reviewer_note} | #{maintainer_note} |"
 end
 
 changes = helper.changes_by_category
@@ -70,26 +76,20 @@ changes.delete(:docs)
 categories = changes.keys - [:unknown]
 
 # Ensure to spin for database reviewer/maintainer when ~database is applied (e.g. to review SQL queries)
-categories << :database if gitlab.mr_labels.include?('database') && !categories.include?(:database)
+categories << :database if helper.gitlab_helper&.mr_labels&.include?('database') && !categories.include?(:database)
 
 if changes.any?
   project = helper.project_name
-  branch_name = gitlab.mr_json['source_branch']
 
-  markdown(MESSAGE)
+  timezone_roulette_spins = roulette.spin(project, categories, timezone_experiment: true)
+  random_roulette_spins = roulette.spin(project, categories, timezone_experiment: false)
 
-  roulette_spins = roulette.spin(project, categories, branch_name, timezone_experiment: TIMEZONE_EXPERIMENT)
-  rows = roulette_spins.map do |spin|
-    # MR includes QA changes, but also other changes, and author isn't an SET
-    if spin.category == :qa && categories.size > 1 && mr_author && !mr_author.reviewer?(project, spin.category, [])
-      spin.optional_role = :maintainer
-    end
-
-    spin.optional_role = :maintainer if spin.category == :test
-
-    markdown_row_for_spin(spin)
+  rows = timezone_roulette_spins.map do |spin|
+    fallback_spin = random_roulette_spins.find { |random_roulette_spins| random_roulette_spins.category == spin.category }
+    markdown_row_for_spins(spin.category, [spin, fallback_spin])
   end
 
+  markdown(MESSAGE)
   markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty?
 
   unknown = changes.fetch(:unknown, [])

lib/gitlab/danger/helper.rb

@@ -53,7 +53,7 @@ module Gitlab
 
       def ee?
         # Support former project name for `dev` and support local Danger run
-        %w[gitlab gitlab-ee].include?(ENV['CI_PROJECT_NAME']) || Dir.exist?('../../ee')
+        %w[gitlab gitlab-ee].include?(ENV['CI_PROJECT_NAME']) || Dir.exist?(File.expand_path('../../../ee', __dir__))
       end
 
       def gitlab_helper

lib/gitlab/danger/roulette.rb

 # frozen_string_literal: true
 
 require_relative 'teammate'
+require_relative 'request_helper'
 
 module Gitlab
   module Danger
@@ -12,45 +13,49 @@ module Gitlab
         database: false
       }.freeze
 
-      Spin = Struct.new(:category, :reviewer, :maintainer, :optional_role)
+      Spin = Struct.new(:category, :reviewer, :maintainer, :optional_role, :timezone_experiment)
+
+      def team_mr_author
+        team.find { |person| person.username == mr_author_username }
+      end
 
       # Assigns GitLab team members to be reviewer and maintainer
       # for each change category that a Merge Request contains.
       #
       # @return [Array<Spin>]
-      def spin(project, categories, branch_name, timezone_experiment: false)
-        team =
-          begin
-            project_team(project)
-          rescue => err
-            warn("Reviewer roulette failed to load team data: #{err.message}")
-            []
-          end
-
-        canonical_branch_name = canonical_branch_name(branch_name)
-
-        spin_per_category = categories.each_with_object({}) do |category, memo|
+      def spin(project, categories, timezone_experiment: false)
+        spins = categories.map do |category|
           including_timezone = INCLUDE_TIMEZONE_FOR_CATEGORY.fetch(category, timezone_experiment)
 
-          memo[category] = spin_for_category(team, project, category, canonical_branch_name, timezone_experiment: including_timezone)
+          spin_for_category(project, category, timezone_experiment: including_timezone)
         end
 
-        spin_per_category.map do |category, spin|
-          case category
+        backend_spin = spins.find { |spin| spin.category == :backend }
+
+        spins.each do |spin|
+          including_timezone = INCLUDE_TIMEZONE_FOR_CATEGORY.fetch(spin.category, timezone_experiment)
+          case spin.category
+          when :qa
+            # MR includes QA changes, but also other changes, and author isn't an SET
+            if categories.size > 1 && !team_mr_author&.reviewer?(project, spin.category, [])
+              spin.optional_role = :maintainer
+            end
           when :test
+            spin.optional_role = :maintainer
+
             if spin.reviewer.nil?
               # Fetch an already picked backend reviewer, or pick one otherwise
-              spin.reviewer = spin_per_category[:backend]&.reviewer || spin_for_category(team, project, :backend, canonical_branch_name).reviewer
+              spin.reviewer = backend_spin&.reviewer || spin_for_category(project, :backend, timezone_experiment: including_timezone).reviewer
             end
           when :engineering_productivity
             if spin.maintainer.nil?
               # Fetch an already picked backend maintainer, or pick one otherwise
-              spin.maintainer = spin_per_category[:backend]&.maintainer || spin_for_category(team, project, :backend, canonical_branch_name).maintainer
+              spin.maintainer = backend_spin&.maintainer || spin_for_category(project, :backend, timezone_experiment: including_timezone).maintainer
             end
           end
-
-          spin
         end
+
+        spins
       end
 
       # Looks up the current list of GitLab team members and parses it into a
@@ -73,14 +78,9 @@ module Gitlab
       # @return [Array<Teammate>]
       def project_team(project_name)
         team.select { |member| member.in_project?(project_name) }
-      end
-
-      def canonical_branch_name(branch_name)
-        branch_name.gsub(/^[ce]e-|-[ce]e$/, '')
-      end
-
-      def new_random(seed)
-        Random.new(Digest::MD5.hexdigest(seed).to_i(16))
+      rescue => err
+        warn("Reviewer roulette failed to load team data: #{err.message}")
+        []
       end
 
       # Known issue: If someone is rejected due to OOO, and then becomes not OOO, the
@@ -113,16 +113,35 @@ module Gitlab
       # @param [Teammate] person
       # @return [Boolean]
       def mr_author?(person)
-        person.username == gitlab.mr_author
+        person.username == mr_author_username
+      end
+
+      def mr_author_username
+        helper.gitlab_helper&.mr_author || `whoami`
+      end
+
+      def mr_source_branch
+        return `git rev-parse --abbrev-ref HEAD` unless helper.gitlab_helper&.mr_json
+
+        helper.gitlab_helper.mr_json['source_branch']
+      end
+
+      def mr_labels
+        helper.gitlab_helper&.mr_labels || []
+      end
+
+      def new_random(seed)
+        Random.new(Digest::MD5.hexdigest(seed).to_i(16))
       end
 
       def spin_role_for_category(team, role, project, category)
         team.select do |member|
-          member.public_send("#{role}?", project, category, gitlab.mr_labels) # rubocop:disable GitlabSecurity/PublicSend
+          member.public_send("#{role}?", project, category, mr_labels) # rubocop:disable GitlabSecurity/PublicSend
         end
       end
 
-      def spin_for_category(team, project, category, branch_name, timezone_experiment: false)
+      def spin_for_category(project, category, timezone_experiment: false)
+        team = project_team(project)
         reviewers, traintainers, maintainers =
           %i[reviewer traintainer maintainer].map do |role|
             spin_role_for_category(team, role, project, category)
@@ -132,11 +151,11 @@ module Gitlab
         # https://gitlab.com/gitlab-org/gitlab/issues/26723
 
         # Make traintainers have triple the chance to be picked as a reviewer
-        random = new_random(branch_name)
+        random = new_random(mr_source_branch)
         reviewer = spin_for_person(reviewers + traintainers + traintainers, random: random, timezone_experiment: timezone_experiment)
         maintainer = spin_for_person(maintainers, random: random, timezone_experiment: timezone_experiment)
 
-        Spin.new(category, reviewer, maintainer)
+        Spin.new(category, reviewer, maintainer, false, timezone_experiment)
       end
     end
   end

lib/gitlab/danger/teammate.rb

@@ -3,10 +3,11 @@
 module Gitlab
   module Danger
     class Teammate
-      attr_reader :username, :name, :role, :projects, :available, :tz_offset_hours
+      attr_reader :options, :username, :name, :role, :projects, :available, :tz_offset_hours
 
       # The options data are produced by https://gitlab.com/gitlab-org/gitlab-roulette/-/blob/master/lib/team_member.rb
       def initialize(options = {})
+        @options = options
         @username = options['username']
         @name = options['name']
         @markdown_name = options['markdown_name']
@@ -16,6 +17,16 @@ module Gitlab
         @tz_offset_hours = options['tz_offset_hours']
       end
 
+      def to_h
+        options
+      end
+
+      def ==(other)
+        return false unless other.respond_to?(:username)
+
+        other.username == username
+      end
+
       def in_project?(name)
         projects&.has_key?(name)
       end

spec/lib/gitlab/danger/helper_spec.rb

@@ -98,21 +98,21 @@ RSpec.describe Gitlab::Danger::Helper do
 
     it 'delegates to CHANGELOG-EE.md existence if CI_PROJECT_NAME is set to something else' do
       stub_env('CI_PROJECT_NAME', 'something else')
-      expect(Dir).to receive(:exist?).with('../../ee') { true }
+      expect(Dir).to receive(:exist?).with(File.expand_path('../../../../ee', __dir__)) { true }
 
       is_expected.to be_truthy
     end
 
     it 'returns true if ee exists' do
       stub_env('CI_PROJECT_NAME', nil)
-      expect(Dir).to receive(:exist?).with('../../ee') { true }
+      expect(Dir).to receive(:exist?).with(File.expand_path('../../../../ee', __dir__)) { true }
 
       is_expected.to be_truthy
     end
 
     it "returns false if ee doesn't exist" do
       stub_env('CI_PROJECT_NAME', nil)
-      expect(Dir).to receive(:exist?).with('../../ee') { false }
+      expect(Dir).to receive(:exist?).with(File.expand_path('../../../../ee', __dir__)) { false }
 
       is_expected.to be_falsy
     end

spec/lib/gitlab/danger/teammate_spec.rb

 # frozen_string_literal: true
 
-require 'fast_spec_helper'
-
 require 'timecop'
 require 'rspec-parameterized'
 
@@ -10,16 +8,16 @@ require 'gitlab/danger/teammate'
 RSpec.describe Gitlab::Danger::Teammate do
   using RSpec::Parameterized::TableSyntax
 
-  subject { described_class.new(options.stringify_keys) }
+  subject { described_class.new(options) }
 
   let(:tz_offset_hours) { 2.0 }
   let(:options) do
     {
-      username: 'luigi',
-      projects: projects,
-      role: role,
-      markdown_name: '[Luigi](https://gitlab.com/luigi) (`@luigi`)',
-      tz_offset_hours: tz_offset_hours
+      'username' => 'luigi',
+      'projects' => projects,
+      'role' => role,
+      'markdown_name' => '[Luigi](https://gitlab.com/luigi) (`@luigi`)',
+      'tz_offset_hours' => tz_offset_hours
     }
   end
   let(:capabilities) { ['reviewer backend'] }
@@ -28,6 +26,26 @@ RSpec.describe Gitlab::Danger::Teammate do
   let(:labels) { [] }
   let(:project) { double }
 
+  describe '#==' do
+    it 'compares Teammate username' do
+      joe1 = described_class.new('username' => 'joe', 'projects' => projects)
+      joe2 = described_class.new('username' => 'joe', 'projects' => [])
+      jane1 = described_class.new('username' => 'jane', 'projects' => projects)
+      jane2 = described_class.new('username' => 'jane', 'projects' => [])
+
+      expect(joe1).to eq(joe2)
+      expect(jane1).to eq(jane2)
+      expect(jane1).not_to eq(nil)
+      expect(described_class.new('username' => nil)).not_to eq(nil)
+    end
+  end
+
+  describe '#to_h' do
+    it 'returns the given options' do
+      expect(subject.to_h).to eq(options)
+    end
+  end
+
   context 'when having multiple capabilities' do
     let(:capabilities) { ['reviewer backend', 'maintainer frontend', 'trainee_maintainer qa'] }
 
@@ -153,21 +171,21 @@ RSpec.describe Gitlab::Danger::Teammate do
   describe '#markdown_name' do
     context 'when timezone_experiment == false' do
       it 'returns markdown name as-is' do
-        expect(subject.markdown_name).to eq(options[:markdown_name])
-        expect(subject.markdown_name(timezone_experiment: false)).to eq(options[:markdown_name])
+        expect(subject.markdown_name).to eq(options['markdown_name'])
+        expect(subject.markdown_name(timezone_experiment: false)).to eq(options['markdown_name'])
       end
     end
 
     context 'when timezone_experiment == true' do
       it 'returns markdown name with timezone info' do
-        expect(subject.markdown_name(timezone_experiment: true)).to eq("#{options[:markdown_name]} (UTC+2)")
+        expect(subject.markdown_name(timezone_experiment: true)).to eq("#{options['markdown_name']} (UTC+2)")
       end
 
       context 'when offset is 1.5' do
         let(:tz_offset_hours) { 1.5 }
 
         it 'returns markdown name with timezone info, not truncated' do
-          expect(subject.markdown_name(timezone_experiment: true)).to eq("#{options[:markdown_name]} (UTC+1.5)")
+          expect(subject.markdown_name(timezone_experiment: true)).to eq("#{options['markdown_name']} (UTC+1.5)")
         end
       end
 
@@ -185,12 +203,12 @@ RSpec.describe Gitlab::Danger::Teammate do
 
         with_them do
           it 'returns markdown name with timezone info' do
-            author = described_class.new(options.merge(username: 'mario', tz_offset_hours: author_offset).stringify_keys)
+            author = described_class.new(options.merge('username' => 'mario', 'tz_offset_hours' => author_offset))
 
             floored_offset_hours = subject.__send__(:floored_offset_hours)
             utc_offset = floored_offset_hours >= 0 ? "+#{floored_offset_hours}" : floored_offset_hours
 
-            expect(subject.markdown_name(timezone_experiment: true, author: author)).to eq("#{options[:markdown_name]} (UTC#{utc_offset}, #{diff_text})")
+            expect(subject.markdown_name(timezone_experiment: true, author: author)).to eq("#{options['markdown_name']} (UTC#{utc_offset}, #{diff_text})")
           end
         end
       end