Merge branch 'positioning-ca-stages' into 'master'

Positioning cycle analytics stages

See merge request gitlab-org/gitlab!18794

app/models/analytics/cycle_analytics/project_stage.rb

@@ -10,6 +10,14 @@ module Analytics
 
       alias_attribute :parent, :project
       alias_attribute :parent_id, :project_id
+
+      def self.relative_positioning_query_base(stage)
+        where(project_id: stage.project_id)
+      end
+
+      def self.relative_positioning_parent_column
+        :project_id
+      end
     end
   end
 end

app/models/concerns/analytics/cycle_analytics/stage.rb

@@ -4,6 +4,7 @@ module Analytics
   module CycleAnalytics
     module Stage
       extend ActiveSupport::Concern
+      include RelativePositioning
 
       included do
         validates :name, presence: true
@@ -17,6 +18,7 @@ module Analytics
 
         alias_attribute :custom_stage?, :custom
         scope :default_stages, -> { where(custom: false) }
+        scope :ordered, -> { order(:relative_position, :id) }
       end
 
       def parent=(_)
@@ -58,6 +60,10 @@ module Analytics
           end_event_identifier.to_s.eql?(stage_params[:end_event_identifier].to_s)
       end
 
+      def find_with_same_parent!(id)
+        parent.cycle_analytics_stages.find(id)
+      end
+
       private
 
       def validate_stage_event_pairs

ee/app/controllers/analytics/cycle_analytics/stages_controller.rb

@@ -88,15 +88,15 @@ module Analytics
       end
 
       def create_service
-        Stages::CreateService.new(parent: @group, current_user: current_user, params: params.permit(:name, :start_event_identifier, :end_event_identifier))
+        Stages::CreateService.new(parent: @group, current_user: current_user, params: create_params)
       end
 
       def update_service
-        Stages::UpdateService.new(parent: @group, current_user: current_user, params: params.permit(:name, :start_event_identifier, :end_event_identifier, :id))
+        Stages::UpdateService.new(parent: @group, current_user: current_user, params: update_params)
       end
 
       def delete_service
-        Stages::DeleteService.new(parent: @group, current_user: current_user, params: params.permit(:id))
+        Stages::DeleteService.new(parent: @group, current_user: current_user, params: delete_params)
       end
 
       def render_stage_service_result(result)
@@ -107,6 +107,18 @@ module Analytics
           render json: { message: result.message, errors: result.payload[:errors] }, status: result.http_status
         end
       end
+
+      def update_params
+        params.permit(:name, :start_event_identifier, :end_event_identifier, :id, :move_after_id, :move_before_id)
+      end
+
+      def create_params
+        params.permit(:name, :start_event_identifier, :end_event_identifier)
+      end
+
+      def delete_params
+        params.permit(:id)
+      end
     end
   end
 end

ee/app/models/analytics/cycle_analytics/group_stage.rb

@@ -10,6 +10,14 @@ module Analytics
 
       alias_attribute :parent, :group
       alias_attribute :parent_id, :group_id
+
+      def self.relative_positioning_query_base(stage)
+        where(group_id: stage.group_id)
+      end
+
+      def self.relative_positioning_parent_column
+        :group_id
+      end
     end
   end
 end

ee/app/services/analytics/cycle_analytics/stages/list_service.rb

@@ -17,7 +17,7 @@ module Analytics
         end
 
         def persisted_stages
-          parent.cycle_analytics_stages
+          parent.cycle_analytics_stages.ordered
         end
       end
     end

ee/app/services/analytics/cycle_analytics/stages/update_service.rb

@@ -17,6 +17,7 @@ module Analytics
             persist_default_stages!
 
             @stage = find_stage
+            handle_position_change
             @stage.assign_attributes(filtered_params)
 
             raise ActiveRecord::Rollback unless @stage.valid?
@@ -53,6 +54,19 @@ module Analytics
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord
+
+        def handle_position_change
+          move_before_id = params.delete(:move_before_id)
+          move_after_id = params.delete(:move_after_id)
+
+          if move_before_id
+            before_stage = @stage.find_with_same_parent!(move_before_id)
+            @stage.move_before(before_stage)
+          elsif move_after_id
+            after_stage = @stage.find_with_same_parent!(move_after_id)
+            @stage.move_after(after_stage)
+          end
+        end
       end
     end
   end

ee/spec/controllers/analytics/cycle_analytics/stages_controller_spec.rb

@@ -89,7 +89,7 @@ describe Analytics::CycleAnalytics::StagesController do
   end
 
   describe 'PUT `update`' do
-    let(:stage) { create(:cycle_analytics_group_stage, parent: group) }
+    let(:stage) { create(:cycle_analytics_group_stage, parent: group, relative_position: 15) }
     subject { put :update, params: params.merge(id: stage.id) }
 
     include_examples 'group permission check on the controller level'
@@ -117,6 +117,19 @@ describe Analytics::CycleAnalytics::StagesController do
 
         expect(stage.name).to eq(params[:name])
       end
+
+      context 'when positioning parameter is given' do
+        before do
+          params[:move_before_id] = create(:cycle_analytics_group_stage, parent: group, relative_position: 10).id
+        end
+
+        it 'moves the stage before the last place' do
+          subject
+
+          before_last = group.cycle_analytics_stages.ordered[-2]
+          expect(before_last.id).to eq(stage.id)
+        end
+      end
     end
 
     include_context 'when invalid stage parameters are given'

ee/spec/models/analytics/cycle_analytics/group_stage_spec.rb

@@ -11,4 +11,12 @@ describe Analytics::CycleAnalytics::GroupStage do
     let(:parent) { create(:group) }
     let(:parent_name) { :group }
   end
+
+  context 'relative positioning' do
+    it_behaves_like 'a class that supports relative positioning' do
+      let(:group) { create(:group) }
+      let(:factory) { :cycle_analytics_group_stage }
+      let(:default_params) { { group: group } }
+    end
+  end
 end

ee/spec/services/analytics/cycle_analytics/stages/list_service_spec.rb

@@ -28,11 +28,12 @@ describe Analytics::CycleAnalytics::Stages::ListService do
   end
 
   context 'when there are persisted stages' do
-    let!(:stage1) { create(:cycle_analytics_group_stage, parent: group) }
-    let!(:stage2) { create(:cycle_analytics_group_stage, parent: group) }
+    let_it_be(:stage1) { create(:cycle_analytics_group_stage, parent: group, relative_position: 2) }
+    let_it_be(:stage2) { create(:cycle_analytics_group_stage, parent: group, relative_position: 3) }
+    let_it_be(:stage3) { create(:cycle_analytics_group_stage, parent: group, relative_position: 1) }
 
-    it 'returns the persisted stages' do
-      expect(stages).to contain_exactly(stage1, stage2)
+    it 'returns the persisted stages in order' do
+      expect(stages).to eq([stage3, stage1, stage2])
     end
   end
 end

ee/spec/services/analytics/cycle_analytics/stages/update_service_spec.rb

@@ -7,6 +7,7 @@ describe Analytics::CycleAnalytics::Stages::UpdateService do
   let_it_be(:user, refind: true) { create(:user) }
   let(:default_stages) { Gitlab::Analytics::CycleAnalytics::DefaultStages.all }
   let(:params) { {} }
+  let(:persisted_stages) { group.reload.cycle_analytics_stages.ordered }
 
   subject { described_class.new(parent: group, params: params, current_user: user).execute }
 
@@ -41,8 +42,6 @@ describe Analytics::CycleAnalytics::Stages::UpdateService do
     end
 
     context 'when the first update happens on a default stage' do
-      let(:persisted_stages) { group.reload.cycle_analytics_stages }
-
       it { expect(subject).to be_success }
 
       it 'persists all default stages' do
@@ -112,4 +111,63 @@ describe Analytics::CycleAnalytics::Stages::UpdateService do
       it { expect(subject.payload[:errors].keys).to eq([:name]) }
     end
   end
+
+  context 'when positioning a stage' do
+    let!(:first_stage) { create(:cycle_analytics_group_stage, group: group, relative_position: 10) }
+    let!(:middle_stage) { create(:cycle_analytics_group_stage, group: group, relative_position: 11) }
+    let!(:last_stage) { create(:cycle_analytics_group_stage, group: group, relative_position: 12) }
+
+    context 'when moving the stage down' do
+      before do
+        params[:id] = first_stage.id
+        params[:move_after_id] = last_stage.id
+      end
+
+      it 'changes the stage positions correctly' do
+        subject
+
+        expect(persisted_stages.last(3)).to eq([middle_stage, last_stage, first_stage])
+      end
+    end
+
+    context 'when moving the stage to the middle' do
+      before do
+        params[:id] = last_stage.id
+        params[:move_before_id] = middle_stage.id
+      end
+
+      it 'changes the stage positions correctly' do
+        subject
+
+        expect(persisted_stages.last(3)).to eq([first_stage, last_stage, middle_stage])
+      end
+    end
+
+    context 'when bogus `move_before_id` is given' do
+      before do
+        params[:id] = last_stage.id
+        params[:move_before_id] = -1
+      end
+
+      it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+    end
+
+    context 'when bogus `move_after_id` is given' do
+      before do
+        params[:id] = last_stage.id
+        params[:move_after_id] = -1
+      end
+
+      it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+    end
+
+    context 'when `move_before_id` points to a stage within a different group' do
+      before do
+        params[:id] = last_stage.id
+        params[:move_before_id] = create(:cycle_analytics_group_stage, group: create(:group)).id
+      end
+
+      it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+    end
+  end
 end

spec/models/analytics/cycle_analytics/project_stage_spec.rb

@@ -16,8 +16,16 @@ describe Analytics::CycleAnalytics::ProjectStage do
     end
   end
 
-  it_behaves_like "cycle analytics stage" do
+  it_behaves_like 'cycle analytics stage' do
     let(:parent) { create(:project) }
     let(:parent_name) { :project }
   end
+
+  context 'relative positioning' do
+    it_behaves_like 'a class that supports relative positioning' do
+      let(:project) { create(:project) }
+      let(:factory) { :cycle_analytics_project_stage }
+      let(:default_params) { { project: project } }
+    end
+  end
 end