diff options
author | Valery Sizov <valery@gitlab.com> | 2017-03-03 20:10:07 +0200 |
---|---|---|
committer | Valery Sizov <valery@gitlab.com> | 2017-03-03 20:23:33 +0200 |
commit | 32538def144f88a68c5cdfbe7cb7cb2866bce932 (patch) | |
tree | 77b71c3be17d359e92b5da1de44c6bbfe34f7fd4 | |
parent | 39db04bb17aa4ddb02290e4681d394190adb7e3c (diff) | |
download | gitlab-ce-32538def144f88a68c5cdfbe7cb7cb2866bce932.tar.gz |
[Issue sorting on board] Addressing review issues
-rw-r--r-- | app/models/concerns/relative_positioning.rb | 7 | ||||
-rw-r--r-- | app/services/boards/issues/move_service.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 | ||||
-rw-r--r-- | spec/services/boards/issues/list_service_spec.rb | 26 | ||||
-rw-r--r-- | spec/services/boards/issues/move_service_spec.rb | 4 |
5 files changed, 12 insertions, 32 deletions
diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index e4c3426b0ca..4cb7048acda 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -114,6 +114,13 @@ module RelativePositioning private + # This method takes two integer values (positions) and + # calculates some random position between them. The range is huge as + # the maximum integer value is 2147483647. Ideally, the calculated value would be + # exactly between those terminating values, but this will introduce possibility of a race condition + # so two or more issues can get the same value, we want to avoid that and we also want to avoid + # using a lock here. If we have two issues with distance more than one thousand, we are OK. + # Given the huge range of possible values that integer can fit we shoud never face a problem. def position_between(pos_before, pos_after) pos_before ||= MIN_POSITION pos_after ||= MAX_POSITION diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb index a946a42a116..2a9981ab884 100644 --- a/app/services/boards/issues/move_service.rb +++ b/app/services/boards/issues/move_service.rb @@ -68,11 +68,9 @@ module Boards end def move_between_iids - move_after_iid = params[:move_after_iid] - move_before_iid = params[:move_before_iid] - return unless move_after_iid || move_before_iid + return unless params[:move_after_iid] || params[:move_before_iid] - [move_after_iid, move_before_iid] + [params[:move_after_iid], params[:move_before_iid]] end end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index c5ac702d831..261c1f8447a 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -21,6 +21,7 @@ Issue: - milestone_id - weight - time_estimate +- relative_position Event: - id - target_type diff --git a/spec/services/boards/issues/list_service_spec.rb b/spec/services/boards/issues/list_service_spec.rb index 305278843f5..01baedc4761 100644 --- a/spec/services/boards/issues/list_service_spec.rb +++ b/spec/services/boards/issues/list_service_spec.rb @@ -43,32 +43,6 @@ describe Boards::Issues::ListService, services: true do described_class.new(project, user, params).execute end - context 'sets default order to priority' do - it 'returns opened issues when list id is missing' do - params = { board_id: board.id } - - issues = described_class.new(project, user, params).execute - - expect(issues).to eq [opened_issue2, reopened_issue1, opened_issue1] - end - - it 'returns closed issues when listing issues from Done' do - params = { board_id: board.id, id: done.id } - - issues = described_class.new(project, user, params).execute - - expect(issues).to eq [closed_issue4, closed_issue2, closed_issue3, closed_issue1] - end - - it 'returns opened issues that have label list applied when listing issues from a label list' do - params = { board_id: board.id, id: list1.id } - - issues = described_class.new(project, user, params).execute - - expect(issues).to eq [list1_issue3, list1_issue1, list1_issue2] - end - end - context 'with list that does not belong to the board' do it 'raises an error' do list = create(:list) diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb index 5b71762e2b3..e0bbdba531b 100644 --- a/spec/services/boards/issues/move_service_spec.rb +++ b/spec/services/boards/issues/move_service_spec.rb @@ -94,9 +94,9 @@ describe Boards::Issues::MoveService, services: true do end it 'sorts issues' do - issue.move_between(issue1, issue2) + issue.move_between!(issue1, issue2) - params.merge! move_after_iid: issue1.iid, move_before_iid: issue2.iid + params.merge! move_after_iid: issue.iid, move_before_iid: issue2.iid described_class.new(project, user, params).execute(issue1) |