Merge branch '216875-use-timezones-for-danger-reviewer-roulette-implementation' into 'master'

Use timezones for Danger Reviewer Roulette implementation

Closes #216875

See merge request gitlab-org/gitlab!34862

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

doc/development/dangerbot.md

@@ -128,10 +128,18 @@ Here is a (non-exhaustive) list of the kinds of things Danger has been used for
 at GitLab so far:
 
 - Coding style
-- Database review workflow
-- Documentation review workflow
+- Database review
+- Documentation review
 - Merge request metrics
-- Reviewer roulette workflow
+- Reviewer roulette. Reviewers and maintainers are chosen based on:
+  - Their roles (backend, frontend, database, etc).
+  - Their availability:
+    - No "OOO"/"PTO"/"Parental Leave" in their GitLab or Slack status.
+    - No `:red_circle:`/`:palm_tree:`/`:beach:`/`:beach_umbrella:`/`:beach_with_umbrella:` emojis in GitLab or Slack status.
+  - [Experimental] Their timezone: people for which the local hour is between
+    6 AM and 2 PM are eligible to be picked. This is to ensure they have a good
+    chance to get to perform a review during their current work day. The experimentation is tracked in
+    [this issue](https://gitlab.com/gitlab-org/quality/team-tasks/-/issues/563)
 - Single codebase effort
 
 ## Limitations

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,6 +122,7 @@ RSpec.describe Gitlab::Danger::Roulette do
       allow(subject).to receive_message_chain(:gitlab, :mr_labels).and_return(mr_labels)
     end
 
+    context 'when timezone_experiment == false' do
       context 'when change contains backend category' do
         let(:categories) { [:backend] }
 
@@ -132,16 +139,6 @@ RSpec.describe Gitlab::Danger::Roulette do
             expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil))
           end
         end
-
-      context 'when teammate has no capacity' do
-        before do
-          backend_maintainer[:has_capacity] = 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 change contains frontend category' do
@@ -177,6 +174,29 @@ RSpec.describe Gitlab::Danger::Roulette do
       end
     end
 
+    context 'when timezone_experiment == true' do
+      let(:timezone_experiment) { true }
+
+      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 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
+
   describe '#team' do
     subject(:team) { roulette.team }
 
@@ -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
 
+    (-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)
+              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]
 
-    it 'excludes OOO persons' do
-      expect(subject.spin_for_person([ooo], random: Random.new)).to be_nil
+              selected = subject.spin_for_person(persons, random: Random.new, timezone_experiment: timezone_experiment)
+
+              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 mr.author' do
-      expect(subject.spin_for_person([author], 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 person with no capacity' do
-      expect(subject.spin_for_person([no_capacity], random: Random.new)).to be_nil
+    it 'excludes mr.author' do
+      expect(subject.spin_for_person([author], 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