diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-03-08 01:03:31 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-03-08 01:03:31 +0000 |
commit | 2995f48ef3158252446c5e03c138feb6c1889941 (patch) | |
tree | f78e13064b0939477d41f5b147a9dfe0b6b34880 | |
parent | 7f2819b778b055278a7fafe9c782d12d09dbd2ea (diff) | |
parent | 8a199f324b74f38629978b585fc71e51040f852a (diff) | |
download | gitlab-ce-2995f48ef3158252446c5e03c138feb6c1889941.tar.gz |
Merge branch 'orderable-issues' into 'master'
Allow issues in boards to be ordered
Closes #21264
See merge request !8916
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 |