Merge branch '262860-create-shifts-table' into 'master'

Create IncidentManagement::OncallShift table and model

See merge request gitlab-org/gitlab!49423

changelogs/unreleased/262860-create-shifts-table.yml

+---
+title: Add table for tracking on-call shifts
+merge_request: 49423
+author:
+type: added

db/migrate/20201208210209_create_incident_management_oncall_shifts.rb

+# frozen_string_literal: true
+
+class CreateIncidentManagementOncallShifts < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    unless table_exists?(:incident_management_oncall_shifts)
+      with_lock_retries do
+        create_table :incident_management_oncall_shifts do |t|
+          t.references :rotation, null: false, foreign_key: { to_table: :incident_management_oncall_rotations, on_delete: :cascade }
+          t.references :participant, null: false, foreign_key: { to_table: :incident_management_oncall_participants, on_delete: :cascade }
+          t.datetime_with_timezone :starts_at, null: false
+          t.datetime_with_timezone :ends_at, null: false
+        end
+
+        execute <<~SQL
+          ALTER TABLE incident_management_oncall_shifts
+            ADD CONSTRAINT inc_mgmnt_no_overlapping_oncall_shifts
+            EXCLUDE USING gist
+            ( rotation_id WITH =,
+              tstzrange(starts_at, ends_at, '[)') WITH &&
+            )
+        SQL
+      end
+    end
+  end
+
+  def down
+    with_lock_retries do
+      drop_table :incident_management_oncall_shifts
+    end
+  end
+end

db/schema_migrations/20201208210209

+90b661656195e61c3b3ac43b8eebcdc06f462eb7e73a6201b2f2a8bc9dd519bf
\ No newline at end of file

db/structure.sql

@@ -13097,6 +13097,23 @@ CREATE SEQUENCE incident_management_oncall_schedules_id_seq
 
 ALTER SEQUENCE incident_management_oncall_schedules_id_seq OWNED BY incident_management_oncall_schedules.id;
 
+CREATE TABLE incident_management_oncall_shifts (
+    id bigint NOT NULL,
+    rotation_id bigint NOT NULL,
+    participant_id bigint NOT NULL,
+    starts_at timestamp with time zone NOT NULL,
+    ends_at timestamp with time zone NOT NULL
+);
+
+CREATE SEQUENCE incident_management_oncall_shifts_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE incident_management_oncall_shifts_id_seq OWNED BY incident_management_oncall_shifts.id;
+
 CREATE TABLE index_statuses (
     id integer NOT NULL,
     project_id integer NOT NULL,
@@ -18456,6 +18473,8 @@ ALTER TABLE ONLY incident_management_oncall_rotations ALTER COLUMN id SET DEFAUL
 
 ALTER TABLE ONLY incident_management_oncall_schedules ALTER COLUMN id SET DEFAULT nextval('incident_management_oncall_schedules_id_seq'::regclass);
 
+ALTER TABLE ONLY incident_management_oncall_shifts ALTER COLUMN id SET DEFAULT nextval('incident_management_oncall_shifts_id_seq'::regclass);
+
 ALTER TABLE ONLY index_statuses ALTER COLUMN id SET DEFAULT nextval('index_statuses_id_seq'::regclass);
 
 ALTER TABLE ONLY insights ALTER COLUMN id SET DEFAULT nextval('insights_id_seq'::regclass);
@@ -19672,6 +19691,9 @@ ALTER TABLE ONLY import_export_uploads
 ALTER TABLE ONLY import_failures
     ADD CONSTRAINT import_failures_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY incident_management_oncall_shifts
+    ADD CONSTRAINT inc_mgmnt_no_overlapping_oncall_shifts EXCLUDE USING gist (rotation_id WITH =, tstzrange(starts_at, ends_at, '[)'::text) WITH &&);
+
 ALTER TABLE ONLY incident_management_oncall_participants
     ADD CONSTRAINT incident_management_oncall_participants_pkey PRIMARY KEY (id);
 
@@ -19681,6 +19703,9 @@ ALTER TABLE ONLY incident_management_oncall_rotations
 ALTER TABLE ONLY incident_management_oncall_schedules
     ADD CONSTRAINT incident_management_oncall_schedules_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY incident_management_oncall_shifts
+    ADD CONSTRAINT incident_management_oncall_shifts_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY index_statuses
     ADD CONSTRAINT index_statuses_pkey PRIMARY KEY (id);
 
@@ -21640,6 +21665,10 @@ CREATE UNIQUE INDEX index_inc_mgmnt_oncall_rotations_on_oncall_schedule_id_and_n
 
 CREATE INDEX index_incident_management_oncall_schedules_on_project_id ON incident_management_oncall_schedules USING btree (project_id);
 
+CREATE INDEX index_incident_management_oncall_shifts_on_participant_id ON incident_management_oncall_shifts USING btree (participant_id);
+
+CREATE INDEX index_incident_management_oncall_shifts_on_rotation_id ON incident_management_oncall_shifts USING btree (rotation_id);
+
 CREATE UNIQUE INDEX index_index_statuses_on_project_id ON index_statuses USING btree (project_id);
 
 CREATE INDEX index_insights_on_namespace_id ON insights USING btree (namespace_id);
@@ -25197,6 +25226,9 @@ ALTER TABLE ONLY user_callouts
 ALTER TABLE ONLY vulnerability_feedback
     ADD CONSTRAINT fk_rails_debd54e456 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY incident_management_oncall_shifts
+    ADD CONSTRAINT fk_rails_df4feb286a FOREIGN KEY (rotation_id) REFERENCES incident_management_oncall_rotations(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY analytics_cycle_analytics_group_stages
     ADD CONSTRAINT fk_rails_dfb37c880d FOREIGN KEY (end_event_label_id) REFERENCES labels(id) ON DELETE CASCADE;
 
@@ -25308,6 +25340,9 @@ ALTER TABLE ONLY board_group_recent_visits
 ALTER TABLE ONLY resource_state_events
     ADD CONSTRAINT fk_rails_f5827a7ccd FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY incident_management_oncall_shifts
+    ADD CONSTRAINT fk_rails_f6eef06841 FOREIGN KEY (participant_id) REFERENCES incident_management_oncall_participants(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY design_user_mentions
     ADD CONSTRAINT fk_rails_f7075a53c1 FOREIGN KEY (design_id) REFERENCES design_management_designs(id) ON DELETE CASCADE;
 

ee/app/models/incident_management/oncall_participant.rb

@@ -2,8 +2,6 @@
 
 module IncidentManagement
   class OncallParticipant < ApplicationRecord
-    include BulkInsertSafe
-
     self.table_name = 'incident_management_oncall_participants'
 
     enum color_palette: Enums::DataVisualizationPalette.colors
@@ -11,7 +9,11 @@ module IncidentManagement
 
     belongs_to :rotation, class_name: 'OncallRotation', foreign_key: :oncall_rotation_id
     belongs_to :user, class_name: 'User', foreign_key: :user_id
+    has_many :shifts, class_name: 'OncallShift', inverse_of: :participant, foreign_key: :participant_id
 
+    # Uniqueness validations added here should be duplicated
+    # in IncidentManagement::OncallRotation::CreateService
+    # as bulk insertion skips validations
     validates :rotation, presence: true
     validates :color_palette, presence: true
     validates :color_weight, presence: true

ee/app/models/incident_management/oncall_rotation.rb

@@ -15,6 +15,7 @@ module IncidentManagement
     belongs_to :schedule, class_name: 'OncallSchedule', inverse_of: 'rotations', foreign_key: 'oncall_schedule_id'
     has_many :participants, class_name: 'OncallParticipant', inverse_of: :rotation
     has_many :users, through: :participants
+    has_many :shifts, class_name: 'OncallShift', inverse_of: :rotation, foreign_key: :rotation_id
 
     validates :name, presence: true, uniqueness: { scope: :oncall_schedule_id }, length: { maximum: NAME_LENGTH }
     validates :starts_at, presence: true

ee/app/models/incident_management/oncall_shift.rb

+# frozen_string_literal: true
+
+module IncidentManagement
+  class OncallShift < ApplicationRecord
+    self.table_name = 'incident_management_oncall_shifts'
+
+    belongs_to :rotation, class_name: 'OncallRotation', inverse_of: :shifts, foreign_key: :rotation_id
+    belongs_to :participant, class_name: 'OncallParticipant', inverse_of: :shifts, foreign_key: :participant_id
+
+    validates :rotation, presence: true
+    validates :participant, presence: true
+    validates :starts_at, presence: true
+    validates :ends_at, presence: true
+    validate :timeframes_do_not_overlap, if: :rotation
+
+    scope :for_timeframe, -> (starts_at, ends_at) do
+      where("tstzrange(starts_at, ends_at, '[)') && tstzrange(?, ?, '[)')", starts_at, ends_at)
+    end
+
+    private
+
+    def timeframes_do_not_overlap
+      return unless rotation.shifts.where.not(id: id).for_timeframe(starts_at, ends_at).exists?
+
+      errors.add(:base, 'Shift timeframe cannot overlap with other existing shifts')
+    end
+  end
+end

ee/app/services/incident_management/oncall_rotations/create_service.rb

@@ -21,38 +21,33 @@ module IncidentManagement
         @schedule = schedule
         @project = project
         @current_user = current_user
-        @params = params
+        @rotation_params = params.except(:participants)
+        @participants_params = Array(params[:participants])
       end
 
       def execute
         return error_no_license unless available?
         return error_no_permissions unless allowed?
+        return error_too_many_participants if participants_params.size > MAXIMUM_PARTICIPANTS
+        return error_duplicate_participants if duplicated_users?
 
-        participant_params = Array(params[:participants])
+        OncallRotation.transaction do
+          oncall_rotation = schedule.rotations.create(rotation_params)
+          break error_in_validation(oncall_rotation) unless oncall_rotation.persisted?
 
-        return error_too_many_participants if participant_params.size > MAXIMUM_PARTICIPANTS
+          participants = participants_for(oncall_rotation)
+          first_invalid_participant = participants.find(&:invalid?)
+          break error_in_validation(first_invalid_participant) if first_invalid_participant
 
-        oncall_rotation = schedule.rotations.create(params.except(:participants))
+          insert_participants(participants)
 
-        return error_in_create(oncall_rotation) unless oncall_rotation.persisted?
-
-        new_participants = Array(participant_params).map do |participant|
-          OncallParticipant.new(
-            rotation: oncall_rotation,
-            user: participant[:user],
-            color_palette: participant[:color_palette],
-            color_weight: participant[:color_weight]
-          )
+          success(oncall_rotation)
         end
-
-        OncallParticipant.bulk_insert!(new_participants)
-
-        success(oncall_rotation)
       end
 
       private
 
-      attr_reader :schedule, :project, :current_user, :params, :participants
+      attr_reader :schedule, :project, :current_user, :rotation_params, :participants_params
 
       def allowed?
         Ability.allowed?(current_user, :admin_incident_management_oncall_schedule, project)
@@ -62,6 +57,43 @@ module IncidentManagement
         ::Gitlab::IncidentManagement.oncall_schedules_available?(project)
       end
 
+      def duplicated_users?
+        users = participants_params.map { |participant| participant[:user] }
+
+        users != users.uniq
+      end
+
+      def participants_for(rotation)
+        participants_params.map do |participant|
+          OncallParticipant.new(
+            rotation: rotation,
+            user: participant[:user],
+            color_palette: participant[:color_palette],
+            color_weight: participant[:color_weight]
+          )
+        end
+      end
+
+      def participant_rows(participants)
+        participants.map do |participant|
+          {
+            oncall_rotation_id: participant.oncall_rotation_id,
+            user_id: participant.user_id,
+            color_palette: OncallParticipant.color_palettes[participant.color_palette],
+            color_weight: OncallParticipant.color_weights[participant.color_weight]
+          }
+        end
+      end
+
+      # BulkInsertSafe cannot be used here while OncallParticipant
+      # has a has_many association. https://gitlab.com/gitlab-org/gitlab/-/issues/247718
+      # We still want to bulk insert to avoid up to MAXIMUM_PARTICIPANTS
+      # consecutive insertions, but .insert_all
+      # does not include validations. Warning!
+      def insert_participants(participants)
+        OncallParticipant.insert_all(participant_rows(participants))
+      end
+
       def error(message)
         ServiceResponse.error(message: message)
       end
@@ -71,19 +103,23 @@ module IncidentManagement
       end
 
       def error_too_many_participants
-        error("A maximum of #{MAXIMUM_PARTICIPANTS} participants can be added")
+        error(_('A maximum of %{count} participants can be added') % { count: MAXIMUM_PARTICIPANTS })
+      end
+
+      def error_duplicate_participants
+        error(_('A user can only participate in a rotation once'))
       end
 
       def error_no_permissions
-        error('You have insufficient permissions to create an on-call rotation for this project')
+        error(_('You have insufficient permissions to create an on-call rotation for this project'))
       end
 
       def error_no_license
-        error('Your license does not support on-call rotations')
+        error(_('Your license does not support on-call rotations'))
       end
 
-      def error_in_create(oncall_rotation)
-        error(oncall_rotation.errors.full_messages.to_sentence)
+      def error_in_validation(object)
+        error(object.errors.full_messages.to_sentence)
       end
     end
   end

ee/spec/factories/incident_management/oncall_participant.rb

@@ -6,5 +6,11 @@ FactoryBot.define do
     association :user, factory: :user
     color_palette { IncidentManagement::OncallParticipant.color_palettes.first.first }
     color_weight { IncidentManagement::OncallParticipant.color_weights.first.first }
+
+    trait :with_developer_access do
+      after(:build) do |participant, _|
+        participant.rotation.project.add_developer(participant.user)
+      end
+    end
   end
 end

ee/spec/factories/incident_management/oncall_shifts.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :incident_management_oncall_shift, class: 'IncidentManagement::OncallShift' do
+    association :participant, factory: :incident_management_oncall_participant
+    rotation { participant.rotation }
+    starts_at { 5.days.ago }
+    ends_at { 2.days.from_now }
+  end
+end

ee/spec/graphql/mutations/incident_management/oncall_rotation/create_spec.rb

@@ -71,25 +71,6 @@ RSpec.describe Mutations::IncidentManagement::OncallRotation::Create do
           end
         end
 
-        context 'user does not have access to the project' do
-          before do
-            other_user = create(:user)
-            args.merge!(
-              participants: [
-                {
-                  username: other_user.username,
-                  color_weight: ::IncidentManagement::OncallParticipant.color_weights['50'],
-                  color_palette: ::IncidentManagement::OncallParticipant.color_palettes[:blue]
-                }
-              ]
-            )
-          end
-
-          it 'raises an error' do
-            expect { resolve }.to raise_error(Gitlab::Graphql::Errors::ArgumentError, /User does not have access to the project/)
-          end
-        end
-
         context 'project path incorrect' do
           before do
             args[:project_path] = "something/incorrect"

ee/spec/models/incident_management/oncall_participant_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe IncidentManagement::OncallParticipant do
   describe '.associations' do
     it { is_expected.to belong_to(:rotation) }
     it { is_expected.to belong_to(:user) }
+    it { is_expected.to have_many(:shifts) }
   end
 
   describe '.validations' do

ee/spec/models/incident_management/oncall_rotation_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe IncidentManagement::OncallRotation do
     it { is_expected.to belong_to(:schedule) }
     it { is_expected.to have_many(:participants) }
     it { is_expected.to have_many(:users).through(:participants) }
+    it { is_expected.to have_many(:shifts) }
   end
 
   describe '.validations' do

ee/spec/models/incident_management/oncall_shift_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IncidentManagement::OncallShift do
+  let_it_be(:participant) { create(:incident_management_oncall_participant, :with_developer_access) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:rotation) }
+    it { is_expected.to belong_to(:participant) }
+  end
+
+  describe 'validations' do
+    subject { build(:incident_management_oncall_shift) }
+
+    it { is_expected.to validate_presence_of(:starts_at) }
+    it { is_expected.to validate_presence_of(:ends_at) }
+    it { is_expected.to validate_presence_of(:rotation) }
+    it { is_expected.to validate_presence_of(:participant) }
+
+    describe 'for timeframe' do
+      let_it_be(:shift_start) { Time.current }
+      let_it_be(:shift_end) { shift_start + 1.day }
+      let_it_be(:existing_shift) { create_shift(shift_start, shift_end, participant) }
+
+      subject { build_shift(starts_at, ends_at, participant) }
+
+      context 'when the new shift does not conflict' do
+        let(:starts_at) { shift_end }
+        let(:ends_at) { shift_end + 5.hours }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'when the new shift conflicts' do
+        let(:starts_at) { shift_start + 5.hours }
+        let(:ends_at) { shift_end + 5.hours }
+
+        specify do
+          expect(subject).to be_invalid
+          expect(subject.errors.full_messages.to_sentence).to eq('Shift timeframe cannot overlap with other existing shifts')
+        end
+      end
+    end
+  end
+
+  describe 'scopes' do
+    describe '.for_timeframe' do
+      let_it_be(:monday) { Time.current.next_week(:monday) }
+      let_it_be(:tuesday) { monday + 1.day }
+      let_it_be(:wednesday) { tuesday + 1.day }
+      let_it_be(:thursday) { wednesday + 1.day }
+      let_it_be(:friday) { thursday + 1.day }
+      let_it_be(:saturday) { friday + 1.day }
+      let_it_be(:sunday) { saturday + 1.day }
+
+      # Using multiple participants in different rotations
+      # to be able to simultaneously save shifts which would
+      # conflict if they were part of the same rotation
+      let_it_be(:participant2) { create(:incident_management_oncall_participant, :with_developer_access) }
+      let_it_be(:participant3) { create(:incident_management_oncall_participant, :with_developer_access) }
+
+      # First rotation
+      let_it_be(:mon_to_tue) { create_shift(monday, tuesday, participant) }
+      let_it_be(:tue_to_wed) { create_shift(tuesday, wednesday, participant) }
+      let_it_be(:wed_to_thu) { create_shift(wednesday, thursday, participant) }
+      let_it_be(:thu_to_fri) { create_shift(thursday, friday, participant) }
+      let_it_be(:fri_to_sat) { create_shift(friday, saturday, participant) }
+      let_it_be(:sat_to_sun) { create_shift(saturday, sunday, participant) }
+
+      # Second rotation
+      let_it_be(:mon_to_thu) { create_shift(monday, thursday, participant2) }
+      let_it_be(:fri_to_sun) { create_shift(friday, sunday, participant2) }
+
+      # Third rotation
+      let_it_be(:tue_to_sun) { create_shift(wednesday, sunday, participant3) }
+
+      subject(:shifts) { described_class.for_timeframe(wednesday, saturday) }
+
+      it 'includes shifts which cover the timeframe' do
+        expect(shifts).to contain_exactly(
+          mon_to_thu, # Overlaps start time
+          wed_to_thu, # Coinciding start times
+          thu_to_fri, # Completely contained
+          fri_to_sat, # Coinciding end times
+          fri_to_sun, # Overlapping end time
+          tue_to_sun # Covers entire timeframe
+        )
+        # Excluded shifts:
+        # mon_to_tue - Completely before timeframe
+        # tue_to_wed - Ends as timeframe starts
+        # sat_to_sun - Starts as timeframe ends
+      end
+    end
+  end
+
+  private
+
+  def create_shift(starts_at, ends_at, participant)
+    create(:incident_management_oncall_shift, starts_at: starts_at, ends_at: ends_at, participant: participant, rotation: participant.rotation)
+  end
+
+  def build_shift(starts_at, ends_at, participant)
+    build(:incident_management_oncall_shift, starts_at: starts_at, ends_at: ends_at, participant: participant, rotation: participant.rotation)
+  end
+end

ee/spec/services/incident_management/oncall_rotations/create_service_spec.rb

@@ -85,15 +85,55 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
       end
     end
 
+    context 'participants do not have access to the project' do
+      let(:participants) do
+        [
+          {
+            user: create(:user),
+            color_palette: 'blue',
+            color_weight: '500'
+          }
+        ]
+      end
+
+      it_behaves_like 'error response', 'User does not have access to the project'
+    end
+
+    context 'participant is included multiple times' do
+      let(:participants) do
+        [
+          {
+            user: current_user,
+            color_palette: 'blue',
+            color_weight: '500'
+          },
+          {
+            user: current_user,
+            color_palette: 'magenta',
+            color_weight: '500'
+          }
+        ]
+      end
+
+      it_behaves_like 'error response', 'A user can only participate in a rotation once'
+    end
+
     context 'with valid params' do
-      it 'successfully creates an on-call rotation' do
+      it 'successfully creates an on-call rotation with participants' do
         expect(execute).to be_success
 
-        oncall_schedule = execute.payload[:oncall_rotation]
-        expect(oncall_schedule).to be_a(::IncidentManagement::OncallRotation)
-        expect(oncall_schedule.name).to eq('On-call rotation')
-        expect(oncall_schedule.length).to eq(1)
-        expect(oncall_schedule.length_unit).to eq('days')
+        oncall_rotation = execute.payload[:oncall_rotation]
+        expect(oncall_rotation).to be_a(::IncidentManagement::OncallRotation)
+        expect(oncall_rotation.name).to eq('On-call rotation')
+        expect(oncall_rotation.length).to eq(1)
+        expect(oncall_rotation.length_unit).to eq('days')
+
+        expect(oncall_rotation.participants.length).to eq(1)
+        expect(oncall_rotation.participants.first).to have_attributes(
+          **participants.first,
+          rotation: oncall_rotation,
+          persisted?: true
+        )
       end
     end
   end

locale/gitlab.pot

@@ -1268,6 +1268,9 @@ msgstr ""
 msgid "A group represents your organization in GitLab. Groups allow you to manage users and collaborate across multiple projects."
 msgstr ""
 
+msgid "A maximum of %{count} participants can be added"
+msgstr ""
+
 msgid "A member of the abuse team will review your report as soon as possible."
 msgstr ""
 
@@ -1331,6 +1334,9 @@ msgstr ""
 msgid "A subscription will trigger a new pipeline on the default branch of this project when a pipeline successfully completes for a new tag on the %{default_branch_docs} of the subscribed project."
 msgstr ""
 
+msgid "A user can only participate in a rotation once"
+msgstr ""
+
 msgid "A user with write access to the source branch selected this option"
 msgstr ""
 
@@ -31978,6 +31984,9 @@ msgstr ""
 msgid "You have insufficient permissions to create an HTTP integration for this project"
 msgstr ""
 
+msgid "You have insufficient permissions to create an on-call rotation for this project"
+msgstr ""
+
 msgid "You have insufficient permissions to create an on-call schedule for this project"
 msgstr ""
 
@@ -32362,6 +32371,9 @@ msgstr ""
 msgid "Your issues will be imported in the background. Once finished, you'll get a confirmation email."
 msgstr ""
 
+msgid "Your license does not support on-call rotations"
+msgstr ""
+
 msgid "Your license does not support on-call schedules"
 msgstr ""