Merge branch 'issue_928_group_boards' of gitlab.com:gitlab-org/gitlab-ee into...

Merge branch 'issue_928_group_boards' of gitlab.com:gitlab-org/gitlab-ee into issue_928_group_boards

app/controllers/boards/issues_controller.rb

@@ -65,7 +65,7 @@ module Boards
     end
 
     def move_params
-      params.permit(:board_id, :id, :from_list_id, :to_list_id, :move_before_iid, :move_after_iid)
+      params.permit(:board_id, :id, :from_list_id, :to_list_id, :move_before_id, :move_after_id)
     end
 
     def issue_params

app/models/concerns/relative_positioning.rb

@@ -10,8 +10,12 @@ module RelativePositioning
     after_save :save_positionable_neighbours
   end
 
+  def project_ids
+    project.id
+  end
+
   def max_relative_position
-    self.class.in_projects(project.id).maximum(:relative_position)
+    self.class.in_projects(project_ids).maximum(:relative_position)
   end
 
   def prev_relative_position
@@ -19,7 +23,7 @@ module RelativePositioning
 
     if self.relative_position
       prev_pos = self.class
-        .in_projects(project.id)
+        .in_projects(project_ids)
         .where('relative_position < ?', self.relative_position)
         .maximum(:relative_position)
     end
@@ -32,7 +36,7 @@ module RelativePositioning
 
     if self.relative_position
       next_pos = self.class
-        .in_projects(project.id)
+        .in_projects(project_ids)
         .where('relative_position > ?', self.relative_position)
         .minimum(:relative_position)
     end
@@ -59,7 +63,7 @@ module RelativePositioning
     pos_after = before.next_relative_position
 
     if before.shift_after?
-      issue_to_move = self.class.in_projects(project.id).find_by!(relative_position: pos_after)
+      issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_after)
       issue_to_move.move_after
       @positionable_neighbours = [issue_to_move]
 
@@ -74,7 +78,7 @@ module RelativePositioning
     pos_before = after.prev_relative_position
 
     if after.shift_before?
-      issue_to_move = self.class.in_projects(project.id).find_by!(relative_position: pos_before)
+      issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_before)
       issue_to_move.move_before
       @positionable_neighbours = [issue_to_move]
 

app/models/issue.rb

@@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord'
 
 class Issue < ActiveRecord::Base
   prepend EE::Issue
+  prepend EE::RelativePositioning
 
   include InternalId
   include Issuable

app/services/boards/issues/move_service.rb

@@ -44,7 +44,7 @@ module Boards
           )
         end
 
-        attrs[:move_between_iids] = move_between_iids if move_between_iids
+        attrs[:move_between_ids] = move_between_ids if move_between_ids
 
         attrs
       end
@@ -69,10 +69,10 @@ module Boards
         Array(label_ids).compact
       end
 
-      def move_between_iids
-        return unless params[:move_after_iid] || params[:move_before_iid]
+      def move_between_ids
+        return unless params[:move_after_id] || params[:move_before_id]
 
-        [params[:move_after_iid], params[:move_before_iid]]
+        [params[:move_after_id], params[:move_before_id]]
       end
     end
   end

app/services/issues/update_service.rb

@@ -3,7 +3,7 @@ module Issues
     include SpamCheckService
 
     def execute(issue)
-      handle_move_between_iids(issue)
+      handle_move_between_ids(issue)
       filter_spam_check_params
       change_issue_duplicate(issue)
       update(issue)
@@ -54,13 +54,13 @@ module Issues
       end
     end
 
-    def handle_move_between_iids(issue)
-      return unless params[:move_between_iids]
+    def handle_move_between_ids(issue)
+      return unless params[:move_between_ids]
 
-      after_iid, before_iid = params.delete(:move_between_iids)
+      after_id, before_id = params.delete(:move_between_ids)
 
-      issue_before = get_issue_if_allowed(issue.project, before_iid) if before_iid
-      issue_after = get_issue_if_allowed(issue.project, after_iid) if after_iid
+      issue_before = get_issue_if_allowed(before_id) if before_id
+      issue_after = get_issue_if_allowed(after_id) if after_id
 
       issue.move_between(issue_before, issue_after)
     end
@@ -76,8 +76,8 @@ module Issues
 
     private
 
-    def get_issue_if_allowed(project, iid)
-      issue = project.issues.find_by(iid: iid)
+    def get_issue_if_allowed(id)
+      issue = Issue.find(id)
       issue if can?(current_user, :update_issue, issue)
     end
 

ee/app/models/concerns/ee/relative_positioning.rb

+module EE
+  # Issue position on list boards should be relative to all group projects
+  module RelativePositioning
+    extend ActiveSupport::Concern
+
+    def board_group
+      @group ||= project.group
+    end
+
+    def has_group_boards?
+      board_group && board_group.boards.any?
+    end
+
+    def project_ids
+      if has_group_boards?
+        board_group.projects.pluck(:id)
+      else
+        super
+      end
+    end
+  end
+end

spec/controllers/boards/issues_controller_spec.rb

@@ -164,7 +164,7 @@ describe Boards::IssuesController do
 
       post :create, board_id: board.to_param,
                     list_id: list.to_param,
-                    issue: { title: title,  project_id: project.id},
+                    issue: { title: title,  project_id: project.id },
                     format: :json
     end
   end

spec/ee/spec/controllers/boards/issues_controller_spec.rb

@@ -163,7 +163,7 @@ describe Boards::IssuesController do
 
       post :create, board_id: board.to_param,
                     list_id: list.to_param,
-                    issue: { title: title, project_id: project_1.id},
+                    issue: { title: title, project_id: project_1.id },
                     format: :json
     end
   end

spec/ee/spec/models/ee/relative_positioning_spec.rb

+require 'spec_helper'
+
+describe EE::RelativePositioning do
+  let(:group) { create(:group) }
+  let!(:board) { create(:board, group: group) }
+  let(:project) { create(:project, namespace: group) }
+  let(:project1) { create(:project, namespace: group) }
+  let(:issue) { create(:issue, project: project) }
+  let(:issue1) { create(:issue, project: project1) }
+  let(:new_issue) { create(:issue, project: project1) }
+
+  before do
+    [issue, issue1].each do |issue|
+      issue.move_to_end && issue.save
+    end
+  end
+
+  describe '#max_relative_position' do
+    it 'returns maximum position' do
+      expect(issue.max_relative_position).to eq issue1.relative_position
+    end
+  end
+
+  describe '#prev_relative_position' do
+    it 'returns previous position if there is an issue above' do
+      expect(issue1.prev_relative_position).to eq issue.relative_position
+    end
+
+    it 'returns nil if there is no issue above' do
+      expect(issue.prev_relative_position).to eq nil
+    end
+  end
+
+  describe '#next_relative_position' do
+    it 'returns next position if there is an issue below' do
+      expect(issue.next_relative_position).to eq issue1.relative_position
+    end
+
+    it 'returns nil if there is no issue below' do
+      expect(issue1.next_relative_position).to eq nil
+    end
+  end
+
+  describe '#move_before' do
+    it 'moves issue before' do
+      [issue1, issue].each(&:move_to_end)
+
+      issue.move_before(issue1)
+
+      expect(issue.relative_position).to be < issue1.relative_position
+    end
+  end
+
+  describe '#move_after' do
+    it 'moves issue after' do
+      [issue, issue1].each(&:move_to_end)
+
+      issue.move_after(issue1)
+
+      expect(issue.relative_position).to be > issue1.relative_position
+    end
+  end
+
+  describe '#move_to_end' do
+    it 'moves issue to the end' do
+      new_issue.move_to_end
+
+      expect(new_issue.relative_position).to be > issue1.relative_position
+    end
+  end
+
+  describe '#shift_after?' do
+    it 'returns true' do
+      issue.update(relative_position: issue1.relative_position - 1)
+
+      expect(issue.shift_after?).to be_truthy
+    end
+
+    it 'returns false' do
+      issue.update(relative_position: issue1.relative_position - 2)
+
+      expect(issue.shift_after?).to be_falsey
+    end
+  end
+
+  describe '#shift_before?' do
+    it 'returns true' do
+      issue.update(relative_position: issue1.relative_position + 1)
+
+      expect(issue.shift_before?).to be_truthy
+    end
+
+    it 'returns false' do
+      issue.update(relative_position: issue1.relative_position + 2)
+
+      expect(issue.shift_before?).to be_falsey
+    end
+  end
+
+  describe '#move_between' do
+    it 'positions issue between two other' do
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to be > issue.relative_position
+      expect(new_issue.relative_position).to be < issue1.relative_position
+    end
+
+    it 'positions issue between on top' do
+      new_issue.move_between(nil, issue)
+
+      expect(new_issue.relative_position).to be < issue.relative_position
+    end
+
+    it 'positions issue between to end' do
+      new_issue.move_between(issue1, nil)
+
+      expect(new_issue.relative_position).to be > issue1.relative_position
+    end
+
+    it 'positions issues even when after and before positions are the same' do
+      issue1.update relative_position: issue.relative_position
+
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to be > issue.relative_position
+      expect(issue.relative_position).to be < issue1.relative_position
+    end
+
+    it 'positions issues between other two if distance is 1' do
+      issue1.update relative_position: issue.relative_position + 1
+
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to be > issue.relative_position
+      expect(issue.relative_position).to be < issue1.relative_position
+    end
+
+    it 'positions issue in the middle of other two if distance is big enough' do
+      issue.update relative_position: 6000
+      issue1.update relative_position: 10000
+
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to eq(8000)
+    end
+
+    it 'positions issue closer to the middle if we are at the very top' do
+      issue1.update relative_position: 6000
+
+      new_issue.move_between(nil, issue1)
+
+      expect(new_issue.relative_position).to eq(6000 - RelativePositioning::IDEAL_DISTANCE)
+    end
+
+    it 'positions issue closer to the middle if we are at the very bottom' do
+      issue.update relative_position: 6000
+      issue1.update relative_position: nil
+
+      new_issue.move_between(issue, nil)
+
+      expect(new_issue.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE)
+    end
+
+    it 'positions issue in the middle of other two if distance is not big enough' do
+      issue.update relative_position: 100
+      issue1.update relative_position: 400
+
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to eq(250)
+    end
+
+    it 'positions issue in the middle of other two is there is no place' do
+      issue.update relative_position: 100
+      issue1.update relative_position: 101
+
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to be_between(issue.relative_position, issue1.relative_position)
+    end
+
+    it 'uses rebalancing if there is no place' do
+      issue.update relative_position: 100
+      issue1.update relative_position: 101
+      issue2 = create(:issue, relative_position: 102, project: project)
+      new_issue.update relative_position: 103
+
+      new_issue.move_between(issue1, issue2)
+      new_issue.save!
+
+      expect(new_issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+      expect(issue.reload.relative_position).not_to eq(100)
+    end
+
+    it 'positions issue right if we pass none-sequential parameters' do
+      issue.update relative_position: 99
+      issue1.update relative_position: 101
+      issue2 = create(:issue, relative_position: 102, project: project)
+      new_issue.update relative_position: 103
+
+      new_issue.move_between(issue, issue2)
+      new_issue.save!
+
+      expect(new_issue.relative_position).to be(100)
+    end
+  end
+end

spec/services/boards/issues/move_service_spec.rb

@@ -98,7 +98,7 @@ describe Boards::Issues::MoveService do
           issue.move_to_end && issue.save!
         end
 
-        params.merge!(move_after_iid: issue1.iid, move_before_iid: issue2.iid)
+        params.merge!(move_after_id: issue1.id, move_before_id: issue2.id)
 
         described_class.new(project, user, params).execute(issue)