Introduce experimental timezone-based reviewers/maintainers suggestions

The goal is to reduce the mean time to review and mean time review to
merge for the GitLab project by using timezones for the the reviewer
selection process.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/216875.
Signed-off-by: Rémy Coutable <remy@rymai.me>

danger/roulette/Dangerfile

@@ -12,17 +12,19 @@ MARKDOWN
 
 CATEGORY_TABLE_HEADER = <<MARKDOWN
 
-To spread load more evenly across eligible reviewers, Danger has randomly picked
-a candidate for each review slot. Feel free to
+To spread load more evenly across eligible reviewers, Danger has picked a candidate for each
+review slot, based on their timezone. Feel free to
 [override these selections](https://about.gitlab.com/handbook/engineering/projects/#gitlab)
 if you think someone else would be better-suited, or the chosen person is unavailable.
 
 To read more on how to use the reviewer roulette, please take a look at the
 [Engineering workflow](https://about.gitlab.com/handbook/engineering/workflow/#basics)
 and [code review guidelines](https://docs.gitlab.com/ee/development/code_review.html).
+Please consider assigning a reviewer or maintainer who is a
+[domain expert](https://about.gitlab.com/handbook/engineering/projects/#gitlab) in the area of the merge request.
 
 Once you've decided who will review this merge request, mention them as you
-normally would! Danger does not (yet?) automatically notify them for you.
+normally would! Danger does not automatically notify them for you.
 
 | Category | Reviewer | Maintainer |
 | -------- | -------- | ---------- |
@@ -38,6 +40,7 @@ MARKDOWN
 
 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 }
@@ -48,7 +51,7 @@ def note_for_category_role(spin, role)
     return OPTIONAL_REVIEW_TEMPLATE % { role: role.capitalize, category: helper.label_for_category(spin.category) }
   end
 
-  spin.public_send(role)&.markdown_name || NOT_AVAILABLE_TEMPLATE % { role: role } # rubocop:disable GitlabSecurity/PublicSend
+  spin.public_send(role)&.markdown_name(timezone_experiment: TIMEZONE_EXPERIMENT, author: mr_author) || NOT_AVAILABLE_TEMPLATE % { role: role } # rubocop:disable GitlabSecurity/PublicSend
 end
 
 def markdown_row_for_spin(spin)
@@ -73,7 +76,9 @@ if changes.any?
   project = helper.project_name
   branch_name = gitlab.mr_json['source_branch']
 
-  roulette_spins = roulette.spin(project, categories, branch_name)
+  markdown(MESSAGE)
+
+  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.reviewer?(project, spin.category, [])
@@ -85,9 +90,8 @@ if changes.any?
     markdown_row_for_spin(spin)
   end
 
-  unknown = changes.fetch(:unknown, [])
-
-  markdown(MESSAGE)
   markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty?
+
+  unknown = changes.fetch(:unknown, [])
   markdown(UNKNOWN_FILES_MESSAGE + helper.markdown_list(unknown)) unless unknown.empty?
 end

lib/gitlab/danger/roulette.rb

@@ -6,6 +6,7 @@ module Gitlab
   module Danger
     module Roulette
       ROULETTE_DATA_URL = 'https://gitlab-org.gitlab.io/gitlab-roulette/roulette.json'
+      HOURS_WHEN_PERSON_CAN_BE_PICKED = (6..14).freeze
 
       Spin = Struct.new(:category, :reviewer, :maintainer, :optional_role)
 
@@ -13,7 +14,7 @@ module Gitlab
       # for each change category that a Merge Request contains.
       #
       # @return [Array<Spin>]
-      def spin(project, categories, branch_name)
+      def spin(project, categories, branch_name, timezone_experiment: false)
         team =
           begin
             project_team(project)
@@ -25,7 +26,7 @@ module Gitlab
         canonical_branch_name = canonical_branch_name(branch_name)
 
         spin_per_category = categories.each_with_object({}) do |category, memo|
-          memo[category] = spin_for_category(team, project, category, canonical_branch_name)
+          memo[category] = spin_for_category(team, project, category, canonical_branch_name, timezone_experiment: timezone_experiment)
         end
 
         spin_per_category.map do |category, spin|
@@ -79,9 +80,14 @@ module Gitlab
       # 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?))
+      def spin_for_person(people, random:, timezone_experiment: false)
+        shuffled_people = people.shuffle(random: random)
+
+        if timezone_experiment
+          shuffled_people.find(&method(:valid_person_with_timezone?))
+        else
+          shuffled_people.find(&method(:valid_person?))
+        end
       end
 
       private
@@ -89,7 +95,13 @@ module Gitlab
       # @param [Teammate] person
       # @return [Boolean]
       def valid_person?(person)
-        !mr_author?(person) && person.available && person.has_capacity
+        !mr_author?(person) && person.available
+      end
+
+      # @param [Teammate] person
+      # @return [Boolean]
+      def valid_person_with_timezone?(person)
+        valid_person?(person) && HOURS_WHEN_PERSON_CAN_BE_PICKED.cover?(person.local_hour)
       end
 
       # @param [Teammate] person
@@ -104,7 +116,7 @@ module Gitlab
         end
       end
 
-      def spin_for_category(team, project, category, branch_name)
+      def spin_for_category(team, project, category, branch_name, timezone_experiment: false)
         reviewers, traintainers, maintainers =
           %i[reviewer traintainer maintainer].map do |role|
             spin_role_for_category(team, role, project, category)
@@ -115,8 +127,8 @@ module Gitlab
 
         # Make traintainers have triple the chance to be picked as a reviewer
         random = new_random(branch_name)
-        reviewer = spin_for_person(reviewers + traintainers + traintainers, random: random)
-        maintainer = spin_for_person(maintainers, random: random)
+        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)
       end

lib/gitlab/danger/teammate.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module Danger
     class Teammate
-      attr_reader :username, :name, :markdown_name, :role, :projects, :available, :has_capacity
+      attr_reader :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 = {})
@@ -13,7 +13,7 @@ module Gitlab
         @role = options['role']
         @projects = options['projects']
         @available = options['available']
-        @has_capacity = options['has_capacity']
+        @tz_offset_hours = options['tz_offset_hours']
       end
 
       def in_project?(name)
@@ -34,8 +34,48 @@ module Gitlab
         has_capability?(project, category, :maintainer, labels)
       end
 
+      def markdown_name(timezone_experiment: false, author: nil)
+        return @markdown_name unless timezone_experiment
+
+        "#{@markdown_name} (#{utc_offset_text(author)})"
+      end
+
+      def local_hour
+        (Time.now.utc + tz_offset_hours * 3600).hour
+      end
+
+      protected
+
+      def floored_offset_hours
+        floored_offset = tz_offset_hours.floor(0)
+
+        floored_offset == tz_offset_hours ? floored_offset : tz_offset_hours
+      end
+
       private
 
+      def utc_offset_text(author = nil)
+        offset_text =
+          if floored_offset_hours >= 0
+            "UTC+#{floored_offset_hours}"
+          else
+            "UTC#{floored_offset_hours}"
+          end
+
+        return offset_text unless author
+
+        "#{offset_text}, #{offset_diff_compared_to_author(author)}"
+      end
+
+      def offset_diff_compared_to_author(author)
+        diff = floored_offset_hours - author.floored_offset_hours
+        return "same timezone as `@#{author.username}`" if diff.zero?
+
+        ahead_or_behind = diff < 0 ? 'behind' : 'ahead'
+
+        "#{diff.abs} hours #{ahead_or_behind} `@#{author.username}`"
+      end
+
       def has_capability?(project, category, kind, labels)
         case category
         when :test

spec/lib/gitlab/danger/roulette_spec.rb

@@ -2,10 +2,15 @@
 
 require 'fast_spec_helper'
 require 'webmock/rspec'
+require 'timecop'
 
 require 'gitlab/danger/roulette'
 
 RSpec.describe Gitlab::Danger::Roulette do
+  around do |example|
+    Timecop.freeze(Time.utc(2020, 06, 22, 10)) { example.run }
+  end
+
   let(:backend_maintainer) do
     {
       username: 'backend-maintainer',
@@ -13,7 +18,7 @@ RSpec.describe Gitlab::Danger::Roulette do
       role: 'Backend engineer',
       projects: { 'gitlab' => 'maintainer backend' },
       available: true,
-      has_capacity: true
+      tz_offset_hours: 2.0
     }
   end
   let(:frontend_reviewer) do
@@ -23,7 +28,7 @@ RSpec.describe Gitlab::Danger::Roulette do
       role: 'Frontend engineer',
       projects: { 'gitlab' => 'reviewer frontend' },
       available: true,
-      has_capacity: true
+      tz_offset_hours: 2.0
     }
   end
   let(:frontend_maintainer) do
@@ -33,7 +38,7 @@ RSpec.describe Gitlab::Danger::Roulette do
       role: 'Frontend engineer',
       projects: { 'gitlab' => "maintainer frontend" },
       available: true,
-      has_capacity: true
+      tz_offset_hours: 2.0
     }
   end
   let(:software_engineer_in_test) do
@@ -46,7 +51,7 @@ RSpec.describe Gitlab::Danger::Roulette do
         'gitlab-qa' => 'maintainer'
       },
       available: true,
-      has_capacity: true
+      tz_offset_hours: 2.0
     }
   end
   let(:engineering_productivity_reviewer) do
@@ -56,7 +61,7 @@ RSpec.describe Gitlab::Danger::Roulette do
       role: 'Engineering Productivity',
       projects: { 'gitlab' => 'reviewer backend' },
       available: true,
-      has_capacity: true
+      tz_offset_hours: 2.0
     }
   end
 
@@ -102,13 +107,14 @@ RSpec.describe Gitlab::Danger::Roulette do
     let!(:branch_name) { 'a-branch' }
     let!(:mr_labels) { ['backend', 'devops::create'] }
     let!(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa') }
+    let(:timezone_experiment) { false }
     let(:spins) do
-      # Stub the request at the latest time so that we can modify the raw data, e.g. available and has_capacity fields.
+      # Stub the request at the latest time so that we can modify the raw data, e.g. available fields.
       WebMock
         .stub_request(:get, described_class::ROULETTE_DATA_URL)
         .to_return(body: teammate_json)
 
-      subject.spin(project, categories, branch_name)
+      subject.spin(project, categories, branch_name, timezone_experiment: timezone_experiment)
     end
 
     before do
@@ -116,63 +122,77 @@ RSpec.describe Gitlab::Danger::Roulette do
       allow(subject).to receive_message_chain(:gitlab, :mr_labels).and_return(mr_labels)
     end
 
-    context 'when change contains backend category' do
-      let(:categories) { [:backend] }
+    context 'when timezone_experiment == false' do
+      context 'when change contains backend category' do
+        let(:categories) { [:backend] }
 
-      it 'assigns backend reviewer and maintainer' do
-        expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
-      end
-
-      context 'when teammate is not available' do
-        before do
-          backend_maintainer[:available] = false
+        it 'assigns backend reviewer and maintainer' do
+          expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
         end
 
-        it 'assigns backend reviewer and no maintainer' do
-          expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil))
+        context 'when teammate is not available' do
+          before do
+            backend_maintainer[:available] = false
+          end
+
+          it 'assigns backend reviewer and no maintainer' do
+            expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil))
+          end
         end
       end
 
-      context 'when teammate has no capacity' do
-        before do
-          backend_maintainer[:has_capacity] = false
-        end
+      context 'when change contains frontend category' do
+        let(:categories) { [:frontend] }
 
-        it 'assigns backend reviewer and no maintainer' do
-          expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil))
+        it 'assigns frontend reviewer and maintainer' do
+          expect(spins).to contain_exactly(matching_spin(:frontend, reviewer: frontend_reviewer, maintainer: frontend_maintainer))
         end
       end
-    end
 
-    context 'when change contains frontend category' do
-      let(:categories) { [:frontend] }
+      context 'when change contains QA category' do
+        let(:categories) { [:qa] }
 
-      it 'assigns frontend reviewer and maintainer' do
-        expect(spins).to contain_exactly(matching_spin(:frontend, reviewer: frontend_reviewer, maintainer: frontend_maintainer))
+        it 'assigns QA reviewer' do
+          expect(spins).to contain_exactly(matching_spin(:qa, reviewer: software_engineer_in_test))
+        end
       end
-    end
 
-    context 'when change contains QA category' do
-      let(:categories) { [:qa] }
+      context 'when change contains Engineering Productivity category' do
+        let(:categories) { [:engineering_productivity] }
 
-      it 'assigns QA reviewer' do
-        expect(spins).to contain_exactly(matching_spin(:qa, reviewer: software_engineer_in_test))
+        it 'assigns Engineering Productivity reviewer and fallback to backend maintainer' do
+          expect(spins).to contain_exactly(matching_spin(:engineering_productivity, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
+        end
       end
-    end
 
-    context 'when change contains Engineering Productivity category' do
-      let(:categories) { [:engineering_productivity] }
+      context 'when change contains test category' do
+        let(:categories) { [:test] }
 
-      it 'assigns Engineering Productivity reviewer and fallback to backend maintainer' do
-        expect(spins).to contain_exactly(matching_spin(:engineering_productivity, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
+        it 'assigns corresponding SET' do
+          expect(spins).to contain_exactly(matching_spin(:test, reviewer: software_engineer_in_test))
+        end
       end
     end
 
-    context 'when change contains test category' do
-      let(:categories) { [:test] }
+    context 'when timezone_experiment == true' do
+      let(:timezone_experiment) { true }
+
+      context 'when change contains backend category' do
+        let(:categories) { [:backend] }
 
-      it 'assigns corresponding SET' do
-        expect(spins).to contain_exactly(matching_spin(:test, reviewer: software_engineer_in_test))
+        it 'assigns backend reviewer and maintainer' do
+          expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
+        end
+
+        context 'when teammate is not in a good timezone' do
+          before do
+            backend_maintainer[:tz_offset_hours] = 5.0
+          end
+
+          it 'assigns backend reviewer and no maintainer' do
+            expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil))
+          end
+        end
       end
     end
   end
@@ -244,34 +264,83 @@ RSpec.describe Gitlab::Danger::Roulette do
   end
 
   describe '#spin_for_person' do
-    let(:person1) { Gitlab::Danger::Teammate.new('username' => 'rymai', 'available' => true, 'has_capacity' => true) }
-    let(:person2) { Gitlab::Danger::Teammate.new('username' => 'godfat', 'available' => true, 'has_capacity' => true) }
-    let(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa', 'available' => true, 'has_capacity' => true) }
-    let(:ooo) { Gitlab::Danger::Teammate.new('username' => 'jacopo-beschi', 'available' => false, 'has_capacity' => true) }
-    let(:no_capacity) { Gitlab::Danger::Teammate.new('username' => 'uncharged', 'available' => true, 'has_capacity' => false) }
+    let(:person_tz_offset_hours) { 0.0 }
+    let(:person1) do
+      Gitlab::Danger::Teammate.new(
+        'username' => 'rymai',
+        'available' => true,
+        'tz_offset_hours' => person_tz_offset_hours
+      )
+    end
+    let(:person2) do
+      Gitlab::Danger::Teammate.new(
+        'username' => 'godfat',
+        'available' => true,
+        'tz_offset_hours' => person_tz_offset_hours)
+    end
+    let(:author) do
+      Gitlab::Danger::Teammate.new(
+        'username' => 'filipa',
+        'available' => true,
+        'tz_offset_hours' => 0.0)
+    end
+    let(:unavailable) do
+      Gitlab::Danger::Teammate.new(
+        'username' => 'jacopo-beschi',
+        'available' => false,
+        'tz_offset_hours' => 0.0)
+    end
 
     before do
       allow(subject).to receive_message_chain(:gitlab, :mr_author).and_return(author.username)
     end
 
-    it 'returns a random person' do
-      persons = [person1, person2]
+    (-4..4).each do |utc_offset|
+      context "when local hour for person is #{10 + utc_offset} (offset: #{utc_offset})" do
+        let(:person_tz_offset_hours) { utc_offset }
+
+        [false, true].each do |timezone_experiment|
+          context "with timezone_experiment == #{timezone_experiment}" do
+            it 'returns a random person' do
+              persons = [person1, person2]
+
+              selected = subject.spin_for_person(persons, random: Random.new, timezone_experiment: timezone_experiment)
+
+              expect(selected.username).to be_in(persons.map(&:username))
+            end
+          end
+        end
+      end
+    end
+
+    ((-12..-5).to_a + (5..12).to_a).each do |utc_offset|
+      context "when local hour for person is #{10 + utc_offset} (offset: #{utc_offset})" do
+        let(:person_tz_offset_hours) { utc_offset }
+
+        [false, true].each do |timezone_experiment|
+          context "with timezone_experiment == #{timezone_experiment}" do
+            it 'returns a random person or nil' do
+              persons = [person1, person2]
 
-      selected = subject.spin_for_person(persons, random: Random.new)
+              selected = subject.spin_for_person(persons, random: Random.new, timezone_experiment: timezone_experiment)
 
-      expect(selected.username).to be_in(persons.map(&:username))
+              if timezone_experiment
+                expect(selected).to be_nil
+              else
+                expect(selected.username).to be_in(persons.map(&:username))
+              end
+            end
+          end
+        end
+      end
     end
 
-    it 'excludes OOO persons' do
-      expect(subject.spin_for_person([ooo], random: Random.new)).to be_nil
+    it 'excludes unavailable persons' do
+      expect(subject.spin_for_person([unavailable], 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
-
-    it 'excludes person with no capacity' do
-      expect(subject.spin_for_person([no_capacity], random: Random.new)).to be_nil
-    end
   end
 end

spec/lib/gitlab/danger/teammate_spec.rb

@@ -2,14 +2,27 @@
 
 require 'fast_spec_helper'
 
+require 'timecop'
 require 'rspec-parameterized'
 
 require 'gitlab/danger/teammate'
 
 RSpec.describe Gitlab::Danger::Teammate do
+  using RSpec::Parameterized::TableSyntax
+
   subject { described_class.new(options.stringify_keys) }
 
-  let(:options) { { username: 'luigi', projects: projects, role: role } }
+  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
+    }
+  end
+  let(:capabilities) { ['reviewer backend'] }
   let(:projects) { { project => capabilities } }
   let(:role) { 'Engineer, Manage' }
   let(:labels) { [] }
@@ -114,4 +127,71 @@ RSpec.describe Gitlab::Danger::Teammate do
       expect(subject.maintainer?(project, :frontend, labels)).to be_falsey
     end
   end
+
+  describe '#local_hour' do
+    around do |example|
+      Timecop.freeze(Time.utc(2020, 6, 23, 10)) { example.run }
+    end
+
+    context 'when author is given' do
+      where(:tz_offset_hours, :expected_local_hour) do
+        -12 | 22
+        -10 | 0
+        2 | 12
+        4 | 14
+        12 | 22
+      end
+
+      with_them do
+        it 'returns the correct local_hour' do
+          expect(subject.local_hour).to eq(expected_local_hour)
+        end
+      end
+    end
+  end
+
+  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])
+      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)")
+      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)")
+        end
+      end
+
+      context 'when author is given' do
+        where(:tz_offset_hours, :author_offset, :diff_text) do
+          -12 | -10 | "2 hours behind `@mario`"
+          -10 | -12 | "2 hours ahead `@mario`"
+          -10 | 2 | "12 hours behind `@mario`"
+          2 | 4 | "2 hours behind `@mario`"
+          4 | 2 | "2 hours ahead `@mario`"
+          2 | 2 | "same timezone as `@mario`"
+        end
+
+        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)
+
+            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})")
+          end
+        end
+      end
+    end
+  end
 end