summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-03-08 01:03:31 +0000
committerDouwe Maan <douwe@gitlab.com>2017-03-08 01:03:31 +0000
commit2995f48ef3158252446c5e03c138feb6c1889941 (patch)
treef78e13064b0939477d41f5b147a9dfe0b6b34880
parent7f2819b778b055278a7fafe9c782d12d09dbd2ea (diff)
parent8a199f324b74f38629978b585fc71e51040f852a (diff)
downloadgitlab-ce-2995f48ef3158252446c5e03c138feb6c1889941.tar.gz
Merge branch 'orderable-issues' into 'master'
Allow issues in boards to be ordered Closes #21264 See merge request !8916
-rw-r--r--app/assets/javascripts/boards/components/board_list.js14
-rw-r--r--app/assets/javascripts/boards/models/issue.js5
-rw-r--r--app/assets/javascripts/boards/models/list.js25
-rw-r--r--app/assets/javascripts/boards/services/board_service.js6
-rw-r--r--app/assets/javascripts/boards/stores/boards_store.js6
-rw-r--r--app/assets/javascripts/test_utils/simulate_drag.js29
-rw-r--r--app/controllers/projects/boards/issues_controller.rb11
-rw-r--r--app/models/concerns/relative_positioning.rb101
-rw-r--r--app/models/issue.rb1
-rw-r--r--app/services/boards/issues/list_service.rb7
-rw-r--r--app/services/boards/issues/move_service.rb28
-rw-r--r--app/services/issuable_base_service.rb2
-rw-r--r--app/services/issues/create_service.rb1
-rw-r--r--app/services/issues/update_service.rb20
-rw-r--r--app/views/projects/boards/components/_board_list.html.haml5
-rw-r--r--db/migrate/20170131221752_add_relative_position_to_issues.rb37
-rw-r--r--db/schema.rb2
-rw-r--r--spec/controllers/projects/boards/issues_controller_spec.rb1
-rw-r--r--spec/features/boards/boards_spec.rb18
-rw-r--r--spec/features/boards/issue_ordering_spec.rb166
-rw-r--r--spec/features/boards/sidebar_spec.rb4
-rw-r--r--spec/fixtures/api/schemas/issue.json1
-rw-r--r--spec/javascripts/boards/boards_store_spec.js75
-rw-r--r--spec/javascripts/boards/issue_spec.js16
-rw-r--r--spec/javascripts/boards/list_spec.js3
-rw-r--r--spec/lib/gitlab/import_export/safe_model_attributes.yml1
-rw-r--r--spec/models/concerns/relative_positioning_spec.rb104
-rw-r--r--spec/services/boards/issues/list_service_spec.rb26
-rw-r--r--spec/services/boards/issues/move_service_spec.rb18
-rw-r--r--spec/services/issues/update_service_spec.rb16
30 files changed, 672 insertions, 77 deletions
diff --git a/app/assets/javascripts/boards/components/board_list.js b/app/assets/javascripts/boards/components/board_list.js
index 2d52e96e7fb..1330d4ae840 100644
--- a/app/assets/javascripts/boards/components/board_list.js
+++ b/app/assets/javascripts/boards/components/board_list.js
@@ -56,11 +56,6 @@ import boardCard from './board_card';
});
}
},
- computed: {
- orderedIssues () {
- return _.sortBy(this.issues, 'priority');
- },
- },
methods: {
listHeight () {
return this.$refs.list.getBoundingClientRect().height;
@@ -92,9 +87,9 @@ import boardCard from './board_card';
const options = gl.issueBoards.getBoardSortableDefaultOptions({
scroll: document.querySelectorAll('.boards-list')[0],
group: 'issues',
- sort: false,
disabled: this.disabled,
filter: '.board-list-count, .is-disabled',
+ dataIdAttr: 'data-issue-id',
onStart: (e) => {
const card = this.$refs.issue[e.oldIndex];
@@ -111,6 +106,13 @@ import boardCard from './board_card';
e.item.remove();
});
},
+ onUpdate: (e) => {
+ const sortedArray = this.sortable.toArray().filter(id => id !== '-1');
+ gl.issueBoards.BoardsStore.moveIssueInList(this.list, Store.moving.issue, e.oldIndex, e.newIndex, sortedArray);
+ },
+ onMove(e) {
+ return !e.related.classList.contains('board-list-count');
+ }
});
this.sortable = Sortable.create(this.$refs.list, options);
diff --git a/app/assets/javascripts/boards/models/issue.js b/app/assets/javascripts/boards/models/issue.js
index 2d0a295ae4d..ca5e6fa7e9d 100644
--- a/app/assets/javascripts/boards/models/issue.js
+++ b/app/assets/javascripts/boards/models/issue.js
@@ -15,6 +15,7 @@ class ListIssue {
this.labels = [];
this.selected = false;
this.assignee = false;
+ this.position = obj.relative_position || Infinity;
if (obj.assignee) {
this.assignee = new ListUser(obj.assignee);
@@ -27,10 +28,6 @@ class ListIssue {
obj.labels.forEach((label) => {
this.labels.push(new ListLabel(label));
});
-
- this.priority = this.labels.reduce((max, label) => {
- return (label.priority < max) ? label.priority : max;
- }, Infinity);
}
addLabel (label) {
diff --git a/app/assets/javascripts/boards/models/list.js b/app/assets/javascripts/boards/models/list.js
index 8158ed4ec2c..f237567208c 100644
--- a/app/assets/javascripts/boards/models/list.js
+++ b/app/assets/javascripts/boards/models/list.js
@@ -110,9 +110,20 @@ class List {
}
addIssue (issue, listFrom, newIndex) {
+ let moveBeforeIid = null;
+ let moveAfterIid = null;
+
if (!this.findIssue(issue.id)) {
if (newIndex !== undefined) {
this.issues.splice(newIndex, 0, issue);
+
+ if (this.issues[newIndex - 1]) {
+ moveBeforeIid = this.issues[newIndex - 1].id;
+ }
+
+ if (this.issues[newIndex + 1]) {
+ moveAfterIid = this.issues[newIndex + 1].id;
+ }
} else {
this.issues.push(issue);
}
@@ -123,13 +134,21 @@ class List {
if (listFrom) {
this.issuesSize += 1;
- this.updateIssueLabel(issue, listFrom);
+
+ this.updateIssueLabel(issue, listFrom, moveBeforeIid, moveAfterIid);
}
}
}
- updateIssueLabel(issue, listFrom) {
- gl.boardService.moveIssue(issue.id, listFrom.id, this.id)
+ moveIssue (issue, oldIndex, newIndex, moveBeforeIid, moveAfterIid) {
+ this.issues.splice(oldIndex, 1);
+ this.issues.splice(newIndex, 0, issue);
+
+ gl.boardService.moveIssue(issue.id, null, null, moveBeforeIid, moveAfterIid);
+ }
+
+ updateIssueLabel(issue, listFrom, moveBeforeIid, moveAfterIid) {
+ gl.boardService.moveIssue(issue.id, listFrom.id, this.id, moveBeforeIid, moveAfterIid)
.then(() => {
listFrom.getIssues(false);
});
diff --git a/app/assets/javascripts/boards/services/board_service.js b/app/assets/javascripts/boards/services/board_service.js
index 065e90518df..e54102814d6 100644
--- a/app/assets/javascripts/boards/services/board_service.js
+++ b/app/assets/javascripts/boards/services/board_service.js
@@ -64,10 +64,12 @@ class BoardService {
return this.issues.get(data);
}
- moveIssue (id, from_list_id, to_list_id) {
+ moveIssue (id, from_list_id = null, to_list_id = null, move_before_iid = null, move_after_iid = null) {
return this.issue.update({ id }, {
from_list_id,
- to_list_id
+ to_list_id,
+ move_before_iid,
+ move_after_iid,
});
}
diff --git a/app/assets/javascripts/boards/stores/boards_store.js b/app/assets/javascripts/boards/stores/boards_store.js
index 56436c8fdc7..3866c6bbfc6 100644
--- a/app/assets/javascripts/boards/stores/boards_store.js
+++ b/app/assets/javascripts/boards/stores/boards_store.js
@@ -109,6 +109,12 @@
listFrom.removeIssue(issue);
}
},
+ moveIssueInList (list, issue, oldIndex, newIndex, idArray) {
+ const beforeId = parseInt(idArray[newIndex - 1], 10) || null;
+ const afterId = parseInt(idArray[newIndex + 1], 10) || null;
+
+ list.moveIssue(issue, oldIndex, newIndex, beforeId, afterId);
+ },
findList (key, val, type = 'label') {
return this.state.lists.filter((list) => {
const byType = type ? list['type'] === type : true;
diff --git a/app/assets/javascripts/test_utils/simulate_drag.js b/app/assets/javascripts/test_utils/simulate_drag.js
index 7dba5840c8a..d48f2404fa5 100644
--- a/app/assets/javascripts/test_utils/simulate_drag.js
+++ b/app/assets/javascripts/test_utils/simulate_drag.js
@@ -43,7 +43,14 @@
return event;
}
- function getTraget(target) {
+ function isLast(target) {
+ var el = typeof target.el === 'string' ? document.getElementById(target.el.substr(1)) : target.el;
+ var children = el.children;
+
+ return children.length - 1 === target.index;
+ }
+
+ function getTarget(target) {
var el = typeof target.el === 'string' ? document.getElementById(target.el.substr(1)) : target.el;
var children = el.children;
@@ -75,12 +82,22 @@
function simulateDrag(options, callback) {
options.to.el = options.to.el || options.from.el;
- var fromEl = getTraget(options.from);
- var toEl = getTraget(options.to);
+ var fromEl = getTarget(options.from);
+ var toEl = getTarget(options.to);
+ var firstEl = getTarget({
+ el: options.to.el,
+ index: 'first'
+ });
+ var lastEl = getTarget({
+ el: options.to.el,
+ index: 'last'
+ });
var scrollable = options.scrollable;
var fromRect = getRect(fromEl);
var toRect = getRect(toEl);
+ var firstRect = getRect(firstEl);
+ var lastRect = getRect(lastEl);
var startTime = new Date().getTime();
var duration = options.duration || 1000;
@@ -88,6 +105,12 @@
options.ontap && options.ontap();
window.SIMULATE_DRAG_ACTIVE = 1;
+ if (options.to.index === 0) {
+ toRect.cy = firstRect.y;
+ } else if (isLast(options.to)) {
+ toRect.cy = lastRect.y + lastRect.h + 50;
+ }
+
var dragInterval = setInterval(function loop() {
var progress = (new Date().getTime() - startTime) / duration;
var x = (fromRect.cx + (toRect.cx - fromRect.cx) * progress) - scrollable.scrollLeft;
diff --git a/app/controllers/projects/boards/issues_controller.rb b/app/controllers/projects/boards/issues_controller.rb
index 61fef4dc133..28c9646910d 100644
--- a/app/controllers/projects/boards/issues_controller.rb
+++ b/app/controllers/projects/boards/issues_controller.rb
@@ -8,6 +8,7 @@ module Projects
def index
issues = ::Boards::Issues::ListService.new(project, current_user, filter_params).execute
issues = issues.page(params[:page]).per(params[:per] || 20)
+ make_sure_position_is_set(issues)
render json: {
issues: serialize_as_json(issues),
@@ -38,6 +39,12 @@ module Projects
private
+ def make_sure_position_is_set(issues)
+ issues.each do |issue|
+ issue.move_to_end && issue.save unless issue.relative_position
+ end
+ end
+
def issue
@issue ||=
IssuesFinder.new(current_user, project_id: project.id)
@@ -63,7 +70,7 @@ module Projects
end
def move_params
- params.permit(:board_id, :id, :from_list_id, :to_list_id)
+ params.permit(:board_id, :id, :from_list_id, :to_list_id, :move_before_iid, :move_after_iid)
end
def issue_params
@@ -73,7 +80,7 @@ module Projects
def serialize_as_json(resource)
resource.as_json(
labels: true,
- only: [:id, :iid, :title, :confidential, :due_date],
+ only: [:id, :iid, :title, :confidential, :due_date, :relative_position],
include: {
assignee: { only: [:id, :name, :username], methods: [:avatar_url] },
milestone: { only: [:id, :title] }
diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
new file mode 100644
index 00000000000..603f2dd7e5d
--- /dev/null
+++ b/app/models/concerns/relative_positioning.rb
@@ -0,0 +1,101 @@
+module RelativePositioning
+ extend ActiveSupport::Concern
+
+ MIN_POSITION = 0
+ MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
+
+ included do
+ after_save :save_positionable_neighbours
+ end
+
+ def min_relative_position
+ self.class.in_projects(project.id).minimum(:relative_position)
+ end
+
+ def max_relative_position
+ self.class.in_projects(project.id).maximum(:relative_position)
+ end
+
+ def prev_relative_position
+ prev_pos = nil
+
+ if self.relative_position
+ prev_pos = self.class.
+ in_projects(project.id).
+ where('relative_position < ?', self.relative_position).
+ maximum(:relative_position)
+ end
+
+ prev_pos || MIN_POSITION
+ end
+
+ def next_relative_position
+ next_pos = nil
+
+ if self.relative_position
+ next_pos = self.class.
+ in_projects(project.id).
+ where('relative_position > ?', self.relative_position).
+ minimum(:relative_position)
+ end
+
+ next_pos || MAX_POSITION
+ end
+
+ def move_between(before, after)
+ return move_after(before) unless after
+ return move_before(after) unless before
+
+ pos_before = before.relative_position
+ pos_after = after.relative_position
+
+ if pos_after && (pos_before == pos_after)
+ self.relative_position = pos_before
+ before.move_before(self)
+ after.move_after(self)
+
+ @positionable_neighbours = [before, after]
+ else
+ self.relative_position = position_between(pos_before, pos_after)
+ end
+ end
+
+ def move_before(after)
+ self.relative_position = position_between(after.prev_relative_position, after.relative_position)
+ end
+
+ def move_after(before)
+ self.relative_position = position_between(before.relative_position, before.next_relative_position)
+ end
+
+ def move_to_end
+ self.relative_position = position_between(max_relative_position, MAX_POSITION)
+ end
+
+ 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
+
+ pos_before, pos_after = [pos_before, pos_after].sort
+
+ rand(pos_before.next..pos_after.pred)
+ end
+
+ def save_positionable_neighbours
+ return unless @positionable_neighbours
+
+ status = @positionable_neighbours.all?(&:save)
+ @positionable_neighbours = nil
+
+ status
+ end
+end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index de90f19f854..0f7a26ee3e1 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -7,6 +7,7 @@ class Issue < ActiveRecord::Base
include Sortable
include Spammable
include FasterCacheKeys
+ include RelativePositioning
DueDateStruct = Struct.new(:title, :name).freeze
NoDueDate = DueDateStruct.new('No Due Date', '0').freeze
diff --git a/app/services/boards/issues/list_service.rb b/app/services/boards/issues/list_service.rb
index 8a94c54b6ab..185838764c1 100644
--- a/app/services/boards/issues/list_service.rb
+++ b/app/services/boards/issues/list_service.rb
@@ -5,7 +5,7 @@ module Boards
issues = IssuesFinder.new(current_user, filter_params).execute
issues = without_board_labels(issues) unless movable_list?
issues = with_list_label(issues) if movable_list?
- issues
+ issues.reorder(Gitlab::Database.nulls_last_order('relative_position', 'ASC'))
end
private
@@ -26,7 +26,6 @@ module Boards
def filter_params
set_default_scope
- set_default_sort
set_project
set_state
@@ -37,10 +36,6 @@ module Boards
params[:scope] = 'all'
end
- def set_default_sort
- params[:sort] = 'priority'
- end
-
def set_project
params[:project_id] = project.id
end
diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb
index 96554a92a02..2a9981ab884 100644
--- a/app/services/boards/issues/move_service.rb
+++ b/app/services/boards/issues/move_service.rb
@@ -3,7 +3,7 @@ module Boards
class MoveService < BaseService
def execute(issue)
return false unless can?(current_user, :update_issue, issue)
- return false unless valid_move?
+ return false if issue_params.empty?
update_service.execute(issue)
end
@@ -14,7 +14,7 @@ module Boards
@board ||= project.boards.find(params[:board_id])
end
- def valid_move?
+ def move_between_lists?
moving_from_list.present? && moving_to_list.present? &&
moving_from_list != moving_to_list
end
@@ -32,11 +32,19 @@ module Boards
end
def issue_params
- {
- add_label_ids: add_label_ids,
- remove_label_ids: remove_label_ids,
- state_event: issue_state
- }
+ attrs = {}
+
+ if move_between_lists?
+ attrs.merge!(
+ add_label_ids: add_label_ids,
+ remove_label_ids: remove_label_ids,
+ state_event: issue_state,
+ )
+ end
+
+ attrs[:move_between_iids] = move_between_iids if move_between_iids
+
+ attrs
end
def issue_state
@@ -58,6 +66,12 @@ module Boards
Array(label_ids).compact
end
+
+ def move_between_iids
+ return unless params[:move_after_iid] || params[:move_before_iid]
+
+ [params[:move_after_iid], params[:move_before_iid]]
+ end
end
end
end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index b618c3e038e..b071a398481 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -211,7 +211,7 @@ class IssuableBaseService < BaseService
label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids)
- if params.present?
+ if issuable.changed? || params.present?
issuable.assign_attributes(params.merge(updated_by: current_user))
before_update(issuable)
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 366b3572738..85b6eb3fe3d 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -13,6 +13,7 @@ module Issues
def before_create(issue)
spam_check(issue, current_user)
+ issue.move_to_end
end
def after_create(issuable)
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 22e32b13259..a444c78b609 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -3,8 +3,8 @@ module Issues
include SpamCheckService
def execute(issue)
+ handle_move_between_iids(issue)
filter_spam_check_params
-
update(issue)
end
@@ -37,11 +37,13 @@ module Issues
end
added_labels = issue.labels - old_labels
+
if added_labels.present?
notification_service.relabeled_issue(issue, added_labels, current_user)
end
added_mentions = issue.mentioned_users - old_mentioned_users
+
if added_mentions.present?
notification_service.new_mentions_in_issue(issue, added_mentions, current_user)
end
@@ -55,8 +57,24 @@ module Issues
Issues::CloseService
end
+ def handle_move_between_iids(issue)
+ return unless params[:move_between_iids]
+
+ after_iid, before_iid = params.delete(:move_between_iids)
+
+ 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.move_between(issue_before, issue_after)
+ end
+
private
+ def get_issue_if_allowed(project, iid)
+ issue = project.issues.find_by(iid: iid)
+ issue if can?(current_user, :update_issue, issue)
+ end
+
def create_confidentiality_note(issue)
SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user)
end
diff --git a/app/views/projects/boards/components/_board_list.html.haml b/app/views/projects/boards/components/_board_list.html.haml
index 0993e880da9..4a4dd84d5d2 100644
--- a/app/views/projects/boards/components/_board_list.html.haml
+++ b/app/views/projects/boards/components/_board_list.html.haml
@@ -8,7 +8,7 @@
"v-show" => "!loading",
":data-board" => "list.id",
":class" => '{ "is-smaller": showIssueForm }' }
- %board-card{ "v-for" => "(issue, index) in orderedIssues",
+ %board-card{ "v-for" => "(issue, index) in issues",
"ref" => "issue",
":index" => "index",
":list" => "list",
@@ -17,7 +17,8 @@
":root-path" => "rootPath",
":disabled" => "disabled",
":key" => "issue.id" }
- %li.board-list-count.text-center{ "v-if" => "showCount" }
+ %li.board-list-count.text-center{ "v-if" => "showCount",
+ "data-issue-id" => "-1" }
= icon("spinner spin", "v-show" => "list.loadingMore" )
%span{ "v-if" => "list.issues.length === list.issuesSize" }
Showing all issues
diff --git a/db/migrate/20170131221752_add_relative_position_to_issues.rb b/db/migrate/20170131221752_add_relative_position_to_issues.rb
new file mode 100644
index 00000000000..1baad0893e3
--- /dev/null
+++ b/db/migrate/20170131221752_add_relative_position_to_issues.rb
@@ -0,0 +1,37 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRelativePositionToIssues < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ # Set this constant to true if this migration requires downtime.
+ DOWNTIME = false
+
+ # When a migration requires downtime you **must** uncomment the following
+ # constant and define a short and easy to understand explanation as to why the
+ # migration requires downtime.
+ # DOWNTIME_REASON = ''
+
+ # When using the methods "add_concurrent_index" or "add_column_with_default"
+ # you must disable the use of transactions as these methods can not run in an
+ # existing transaction. When using "add_concurrent_index" make sure that this
+ # method is the _only_ method called in the migration, any other changes
+ # should go in a separate migration. This ensures that upon failure _only_ the
+ # index creation fails and can be retried or reverted easily.
+ #
+ # To disable transactions uncomment the following line and remove these
+ # comments:
+ disable_ddl_transaction!
+
+ def up
+ add_column :issues, :relative_position, :integer
+
+ add_concurrent_index :issues, :relative_position
+ end
+
+ def down
+ remove_column :issues, :relative_position
+
+ remove_index :issues, :relative_position if index_exists? :issues, :relative_position
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 21ab9bf9eab..3ec5461f600 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -530,6 +530,7 @@ ActiveRecord::Schema.define(version: 20170306170512) do
t.text "title_html"
t.text "description_html"
t.integer "time_estimate"
+ t.integer "relative_position"
end
add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -541,6 +542,7 @@ ActiveRecord::Schema.define(version: 20170306170512) do
add_index "issues", ["due_date"], name: "index_issues_on_due_date", using: :btree
add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree
add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree
+ add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree
add_index "issues", ["state"], name: "index_issues_on_state", using: :btree
add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}
diff --git a/spec/controllers/projects/boards/issues_controller_spec.rb b/spec/controllers/projects/boards/issues_controller_spec.rb
index 3d0533cb516..15667e8d4b1 100644
--- a/spec/controllers/projects/boards/issues_controller_spec.rb
+++ b/spec/controllers/projects/boards/issues_controller_spec.rb
@@ -43,6 +43,7 @@ describe Projects::Boards::IssuesController do
expect(response).to match_response_schema('issues')
expect(parsed_response.length).to eq 2
+ expect(development.issues.map(&:relative_position)).not_to include(nil)
end
end
diff --git a/spec/features/boards/boards_spec.rb b/spec/features/boards/boards_spec.rb
index e247bfa2980..ecc356f2505 100644
--- a/spec/features/boards/boards_spec.rb
+++ b/spec/features/boards/boards_spec.rb
@@ -71,16 +71,16 @@ describe 'Issue Boards', feature: true, js: true do
let!(:list1) { create(:list, board: board, label: planning, position: 0) }
let!(:list2) { create(:list, board: board, label: development, position: 1) }
- let!(:confidential_issue) { create(:labeled_issue, :confidential, project: project, author: user, labels: [planning]) }
- let!(:issue1) { create(:labeled_issue, project: project, assignee: user, labels: [planning]) }
- let!(:issue2) { create(:labeled_issue, project: project, author: user2, labels: [planning]) }
- let!(:issue3) { create(:labeled_issue, project: project, labels: [planning]) }
- let!(:issue4) { create(:labeled_issue, project: project, labels: [planning]) }
- let!(:issue5) { create(:labeled_issue, project: project, labels: [planning], milestone: milestone) }
- let!(:issue6) { create(:labeled_issue, project: project, labels: [planning, development]) }
- let!(:issue7) { create(:labeled_issue, project: project, labels: [development]) }
+ let!(:confidential_issue) { create(:labeled_issue, :confidential, project: project, author: user, labels: [planning], relative_position: 9) }
+ let!(:issue1) { create(:labeled_issue, project: project, assignee: user, labels: [planning], relative_position: 8) }
+ let!(:issue2) { create(:labeled_issue, project: project, author: user2, labels: [planning], relative_position: 7) }
+ let!(:issue3) { create(:labeled_issue, project: project, labels: [planning], relative_position: 6) }
+ let!(:issue4) { create(:labeled_issue, project: project, labels: [planning], relative_position: 5) }
+ let!(:issue5) { create(:labeled_issue, project: project, labels: [planning], milestone: milestone, relative_position: 4) }
+ let!(:issue6) { create(:labeled_issue, project: project, labels: [planning, development], relative_position: 3) }
+ let!(:issue7) { create(:labeled_issue, project: project, labels: [development], relative_position: 2) }
let!(:issue8) { create(:closed_issue, project: project) }
- let!(:issue9) { create(:labeled_issue, project: project, labels: [planning, testing, bug, accepting]) }
+ let!(:issue9) { create(:labeled_issue, project: project, labels: [planning, testing, bug, accepting], relative_position: 1) }
before do
visit namespace_project_board_path(project.namespace, project, board)
diff --git a/spec/features/boards/issue_ordering_spec.rb b/spec/features/boards/issue_ordering_spec.rb
new file mode 100644
index 00000000000..c50155a6d14
--- /dev/null
+++ b/spec/features/boards/issue_ordering_spec.rb
@@ -0,0 +1,166 @@
+require 'rails_helper'
+
+describe 'Issue Boards', :feature, :js do
+ include WaitForVueResource
+ include DragTo
+
+ let(:project) { create(:empty_project, :public) }
+ let(:board) { create(:board, project: project) }
+ let(:user) { create(:user) }
+ let(:label) { create(:label, project: project) }
+ let!(:list1) { create(:list, board: board, label: label, position: 0) }
+ let!(:issue1) { create(:labeled_issue, project: project, title: 'testing 1', labels: [label], relative_position: 3) }
+ let!(:issue2) { create(:labeled_issue, project: project, title: 'testing 2', labels: [label], relative_position: 2) }
+ let!(:issue3) { create(:labeled_issue, project: project, title: 'testing 3', labels: [label], relative_position: 1) }
+
+ before do
+ project.team << [user, :master]
+
+ login_as(user)
+ end
+
+ context 'un-ordered issues' do
+ let!(:issue4) { create(:labeled_issue, project: project, labels: [label]) }
+
+ before do
+ visit namespace_project_board_path(project.namespace, project, board)
+ wait_for_vue_resource
+
+ expect(page).to have_selector('.board', count: 2)
+ end
+
+ it 'has un-ordered issue as last issue' do
+ page.within(first('.board')) do
+ expect(all('.card').last).to have_content(issue4.title)
+ end
+ end
+
+ it 'moves un-ordered issue to top of list' do
+ drag(from_index: 3, to_index: 0)
+
+ page.within(first('.board')) do
+ expect(first('.card')).to have_content(issue4.title)
+ end
+ end
+ end
+
+ context 'ordering in list' do
+ before do
+ visit namespace_project_board_path(project.namespace, project, board)
+ wait_for_vue_resource
+
+ expect(page).to have_selector('.board', count: 2)
+ end
+
+ it 'moves from middle to top' do
+ drag(from_index: 1, to_index: 0)
+
+ wait_for_vue_resource
+
+ expect(first('.card')).to have_content(issue2.title)
+ end
+
+ it 'moves from middle to bottom' do
+ drag(from_index: 1, to_index: 2)
+
+ wait_for_vue_resource
+
+ expect(all('.card').last).to have_content(issue2.title)
+ end
+
+ it 'moves from top to bottom' do
+ drag(from_index: 0, to_index: 2)
+
+ wait_for_vue_resource
+
+ expect(all('.card').last).to have_content(issue3.title)
+ end
+
+ it 'moves from bottom to top' do
+ drag(from_index: 2, to_index: 0)
+
+ wait_for_vue_resource
+
+ expect(first('.card')).to have_content(issue1.title)
+ end
+
+ it 'moves from top to middle' do
+ drag(from_index: 0, to_index: 1)
+
+ wait_for_vue_resource
+
+ expect(first('.card')).to have_content(issue2.title)
+ end
+
+ it 'moves from bottom to middle' do
+ drag(from_index: 2, to_index: 1)
+
+ wait_for_vue_resource
+
+ expect(all('.card').last).to have_content(issue2.title)
+ end
+ end
+
+ context 'ordering when changing list' do
+ let(:label2) { create(:label, project: project) }
+ let!(:list2) { create(:list, board: board, label: label2, position: 1) }
+ let!(:issue4) { create(:labeled_issue, project: project, title: 'testing 1', labels: [label2], relative_position: 3.0) }
+ let!(:issue5) { create(:labeled_issue, project: project, title: 'testing 2', labels: [label2], relative_position: 2.0) }
+ let!(:issue6) { create(:labeled_issue, project: project, title: 'testing 3', labels: [label2], relative_position: 1.0) }
+
+ before do
+ visit namespace_project_board_path(project.namespace, project, board)
+ wait_for_vue_resource
+
+ expect(page).to have_selector('.board', count: 3)
+ end
+
+ it 'moves to top of another list' do
+ drag(list_from_index: 0, list_to_index: 1)
+
+ wait_for_vue_resource
+
+ expect(first('.board')).to have_selector('.card', count: 2)
+ expect(all('.board')[1]).to have_selector('.card', count: 4)
+
+ page.within(all('.board')[1]) do
+ expect(first('.card')).to have_content(issue3.title)
+ end
+ end
+
+ it 'moves to bottom of another list' do
+ drag(list_from_index: 0, list_to_index: 1, to_index: 2)
+
+ wait_for_vue_resource
+
+ expect(first('.board')).to have_selector('.card', count: 2)
+ expect(all('.board')[1]).to have_selector('.card', count: 4)
+
+ page.within(all('.board')[1]) do
+ expect(all('.card').last).to have_content(issue3.title)
+ end
+ end
+
+ it 'moves to index of another list' do
+ drag(list_from_index: 0, list_to_index: 1, to_index: 1)
+
+ wait_for_vue_resource
+
+ expect(first('.board')).to have_selector('.card', count: 2)
+ expect(all('.board')[1]).to have_selector('.card', count: 4)
+
+ page.within(all('.board')[1]) do
+ expect(all('.card')[1]).to have_content(issue3.title)
+ end
+ end
+ end
+
+ def drag(selector: '.board-list', list_from_index: 0, from_index: 0, to_index: 0, list_to_index: 0)
+ drag_to(selector: selector,
+ scrollable: '#board-app',
+ list_from_index: list_from_index,
+ from_index: from_index,
+ to_index: to_index,
+ list_to_index: list_to_index)
+ end
+end
diff --git a/spec/features/boards/sidebar_spec.rb b/spec/features/boards/sidebar_spec.rb
index 59e87b3f69c..3332e07ec31 100644
--- a/spec/features/boards/sidebar_spec.rb
+++ b/spec/features/boards/sidebar_spec.rb
@@ -11,8 +11,8 @@ describe 'Issue Boards', feature: true, js: true do
let!(:bug) { create(:label, project: project, name: 'Bug') }
let!(:regression) { create(:label, project: project, name: 'Regression') }
let!(:stretch) { create(:label, project: project, name: 'Stretch') }
- let!(:issue1) { create(:labeled_issue, project: project, assignee: user, milestone: milestone, labels: [development]) }
- let!(:issue2) { create(:labeled_issue, project: project, labels: [development, stretch]) }
+ let!(:issue1) { create(:labeled_issue, project: project, assignee: user, milestone: milestone, labels: [development], relative_position: 2) }
+ let!(:issue2) { create(:labeled_issue, project: project, labels: [development, stretch], relative_position: 1) }
let(:board) { create(:board, project: project) }
let!(:list) { create(:list, board: board, label: development, position: 0) }
let(:card) { first('.board').first('.card') }
diff --git a/spec/fixtures/api/schemas/issue.json b/spec/fixtures/api/schemas/issue.json
index 8e19cee5440..21c078e0f44 100644
--- a/spec/fixtures/api/schemas/issue.json
+++ b/spec/fixtures/api/schemas/issue.json
@@ -11,6 +11,7 @@
"title": { "type": "string" },
"confidential": { "type": "boolean" },
"due_date": { "type": ["date", "null"] },
+ "relative_position": { "type": "integer" },
"labels": {
"type": "array",
"items": {
diff --git a/spec/javascripts/boards/boards_store_spec.js b/spec/javascripts/boards/boards_store_spec.js
index 9dd741a680b..49a2ca4a78f 100644
--- a/spec/javascripts/boards/boards_store_spec.js
+++ b/spec/javascripts/boards/boards_store_spec.js
@@ -5,6 +5,7 @@
/* global Cookies */
/* global listObj */
/* global listObjDuplicate */
+/* global ListIssue */
require('~/lib/utils/url_utility');
require('~/boards/models/issue');
@@ -14,6 +15,7 @@ require('~/boards/models/user');
require('~/boards/services/board_service');
require('~/boards/stores/boards_store');
require('./mock_data');
+require('es6-promise').polyfill();
describe('Store', () => {
beforeEach(() => {
@@ -21,6 +23,10 @@ describe('Store', () => {
gl.boardService = new BoardService('/test/issue-boards/board', '', '1');
gl.issueBoards.BoardsStore.create();
+ spyOn(gl.boardService, 'moveIssue').and.callFake(() => new Promise((resolve) => {
+ resolve();
+ }));
+
Cookies.set('issue_board_welcome_hidden', 'false', {
expires: 365 * 10,
path: ''
@@ -154,5 +160,74 @@ describe('Store', () => {
done();
}, 0);
});
+
+ it('moves issue to top of another list', (done) => {
+ const listOne = gl.issueBoards.BoardsStore.addList(listObj);
+ const listTwo = gl.issueBoards.BoardsStore.addList(listObjDuplicate);
+
+ expect(gl.issueBoards.BoardsStore.state.lists.length).toBe(2);
+
+ setTimeout(() => {
+ listOne.issues[0].id = 2;
+
+ expect(listOne.issues.length).toBe(1);
+ expect(listTwo.issues.length).toBe(1);
+
+ gl.issueBoards.BoardsStore.moveIssueToList(listOne, listTwo, listOne.findIssue(2), 0);
+
+ expect(listOne.issues.length).toBe(0);
+ expect(listTwo.issues.length).toBe(2);
+ expect(listTwo.issues[0].id).toBe(2);
+ expect(gl.boardService.moveIssue).toHaveBeenCalledWith(2, listOne.id, listTwo.id, null, 1);
+
+ done();
+ }, 0);
+ });
+
+ it('moves issue to bottom of another list', (done) => {
+ const listOne = gl.issueBoards.BoardsStore.addList(listObj);
+ const listTwo = gl.issueBoards.BoardsStore.addList(listObjDuplicate);
+
+ expect(gl.issueBoards.BoardsStore.state.lists.length).toBe(2);
+
+ setTimeout(() => {
+ listOne.issues[0].id = 2;
+
+ expect(listOne.issues.length).toBe(1);
+ expect(listTwo.issues.length).toBe(1);
+
+ gl.issueBoards.BoardsStore.moveIssueToList(listOne, listTwo, listOne.findIssue(2), 1);
+
+ expect(listOne.issues.length).toBe(0);
+ expect(listTwo.issues.length).toBe(2);
+ expect(listTwo.issues[1].id).toBe(2);
+ expect(gl.boardService.moveIssue).toHaveBeenCalledWith(2, listOne.id, listTwo.id, 1, null);
+
+ done();
+ }, 0);
+ });
+
+ it('moves issue in list', (done) => {
+ const issue = new ListIssue({
+ title: 'Testing',
+ iid: 2,
+ confidential: false,
+ labels: []
+ });
+ const list = gl.issueBoards.BoardsStore.addList(listObj);
+
+ setTimeout(() => {
+ list.addIssue(issue);
+
+ expect(list.issues.length).toBe(2);
+
+ gl.issueBoards.BoardsStore.moveIssueInList(list, issue, 0, 1, [1, 2]);
+
+ expect(list.issues[0].id).toBe(2);
+ expect(gl.boardService.moveIssue).toHaveBeenCalledWith(2, null, null, 1, null);
+
+ done();
+ });
+ });
});
});
diff --git a/spec/javascripts/boards/issue_spec.js b/spec/javascripts/boards/issue_spec.js
index aab4d9c501e..c96dfe94a4a 100644
--- a/spec/javascripts/boards/issue_spec.js
+++ b/spec/javascripts/boards/issue_spec.js
@@ -79,4 +79,20 @@ describe('Issue model', () => {
issue.removeLabels([issue.labels[0], issue.labels[1]]);
expect(issue.labels.length).toBe(0);
});
+
+ it('sets position to infinity if no position is stored', () => {
+ expect(issue.position).toBe(Infinity);
+ });
+
+ it('sets position', () => {
+ const relativePositionIssue = new ListIssue({
+ title: 'Testing',
+ iid: 1,
+ confidential: false,
+ relative_position: 1,
+ labels: []
+ });
+
+ expect(relativePositionIssue.position).toBe(1);
+ });
});
diff --git a/spec/javascripts/boards/list_spec.js b/spec/javascripts/boards/list_spec.js
index c8a18af7198..d49d3af33d9 100644
--- a/spec/javascripts/boards/list_spec.js
+++ b/spec/javascripts/boards/list_spec.js
@@ -103,6 +103,7 @@ describe('List model', () => {
listDup.updateIssueLabel(list, issue);
- expect(gl.boardService.moveIssue).toHaveBeenCalledWith(issue.id, list.id, listDup.id);
+ expect(gl.boardService.moveIssue)
+ .toHaveBeenCalledWith(issue.id, list.id, listDup.id, undefined, undefined);
});
});
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 3bd1f335a89..c718e792461 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/models/concerns/relative_positioning_spec.rb b/spec/models/concerns/relative_positioning_spec.rb
new file mode 100644
index 00000000000..69906382545
--- /dev/null
+++ b/spec/models/concerns/relative_positioning_spec.rb
@@ -0,0 +1,104 @@
+require 'spec_helper'
+
+describe Issue, 'RelativePositioning' do
+ let(:project) { create(:empty_project) }
+ let(:issue) { create(:issue, project: project) }
+ let(:issue1) { create(:issue, project: project) }
+ let(:new_issue) { create(:issue, project: project) }
+
+ before do
+ [issue, issue1].each do |issue|
+ issue.move_to_end && issue.save
+ end
+ end
+
+ describe '#min_relative_position' do
+ it 'returns maximum position' do
+ expect(issue.min_relative_position).to eq issue.relative_position
+ 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 minimum position if there is no issue above' do
+ expect(issue.prev_relative_position).to eq RelativePositioning::MIN_POSITION
+ 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 next position if there is no issue below' do
+ expect(issue1.next_relative_position).to eq RelativePositioning::MAX_POSITION
+ 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 '#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
+ end
+end
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 77f75167b3d..727ea04ea5c 100644
--- a/spec/services/boards/issues/move_service_spec.rb
+++ b/spec/services/boards/issues/move_service_spec.rb
@@ -78,8 +78,10 @@ describe Boards::Issues::MoveService, services: true do
end
context 'when moving to same list' do
- let(:issue) { create(:labeled_issue, project: project, labels: [bug, development]) }
- let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
+ let(:issue) { create(:labeled_issue, project: project, labels: [bug, development]) }
+ let(:issue1) { create(:labeled_issue, project: project, labels: [bug, development]) }
+ let(:issue2) { create(:labeled_issue, project: project, labels: [bug, development]) }
+ let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
it 'returns false' do
expect(described_class.new(project, user, params).execute(issue)).to eq false
@@ -90,6 +92,18 @@ describe Boards::Issues::MoveService, services: true do
expect(issue.reload.labels).to contain_exactly(bug, development)
end
+
+ it 'sorts issues' do
+ [issue, issue1, issue2].each do |issue|
+ issue.move_to_end && issue.save!
+ end
+
+ params.merge!(move_after_iid: issue1.iid, move_before_iid: issue2.iid)
+
+ described_class.new(project, user, params).execute(issue)
+
+ expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+ end
end
end
end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index d83b09fd32c..fa472f3e2c3 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -58,6 +58,22 @@ describe Issues::UpdateService, services: true do
expect(issue.due_date).to eq Date.tomorrow
end
+ it 'sorts issues as specified by parameters' do
+ issue1 = create(:issue, project: project, assignee_id: user3.id)
+ issue2 = create(:issue, project: project, assignee_id: user3.id)
+
+ [issue, issue1, issue2].each do |issue|
+ issue.move_to_end
+ issue.save
+ end
+
+ opts[:move_between_iids] = [issue1.iid, issue2.iid]
+
+ update_issue(opts)
+
+ expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+ end
+
context 'when current user cannot admin issues in the project' do
let(:guest) { create(:user) }
before do