From ac868482a7780a332045ef270a409ff70bcf8efd Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 1 Feb 2017 12:41:01 -0600 Subject: [PATCH] Allow issues in boards to be ordered --- .../boards/components/board_list.js.es6 | 5 +- .../javascripts/boards/models/list.js.es6 | 25 +++- .../boards/services/board_service.js.es6 | 6 +- .../boards/stores/boards_store.js.es6 | 3 + .../projects/boards/issues_controller.rb | 2 +- app/models/concerns/relative_positioning.rb | 132 ++++++++++++++++++ app/models/issue.rb | 1 + app/services/boards/issues/list_service.rb | 7 +- app/services/boards/issues/move_service.rb | 29 +++- app/services/issuable_base_service.rb | 2 +- app/services/issues/update_service.rb | 21 +++ ...1221752_add_relative_position_to_issues.rb | 31 ++++ db/schema.rb | 37 +++-- 13 files changed, 270 insertions(+), 31 deletions(-) create mode 100644 app/models/concerns/relative_positioning.rb create mode 100644 db/migrate/20170131221752_add_relative_position_to_issues.rb diff --git a/app/assets/javascripts/boards/components/board_list.js.es6 b/app/assets/javascripts/boards/components/board_list.js.es6 index 60b0a30af3f..d9a42a23beb 100644 --- a/app/assets/javascripts/boards/components/board_list.js.es6 +++ b/app/assets/javascripts/boards/components/board_list.js.es6 @@ -86,7 +86,7 @@ require('./board_new_issue'); const options = gl.issueBoards.getBoardSortableDefaultOptions({ scroll: document.querySelectorAll('.boards-list')[0], group: 'issues', - sort: false, + sort: true, disabled: this.disabled, filter: '.board-list-count, .is-disabled', onStart: (e) => { @@ -105,6 +105,9 @@ require('./board_new_issue'); e.item.remove(); }); }, + onUpdate: (e) => { + gl.issueBoards.BoardsStore.moveIssueInList(this.list, Store.moving.issue, e.oldIndex, e.newIndex); + }, }); this.sortable = Sortable.create(this.$refs.list, options); diff --git a/app/assets/javascripts/boards/models/list.js.es6 b/app/assets/javascripts/boards/models/list.js.es6 index 5152be56b66..4f7745e6e8d 100644 --- a/app/assets/javascripts/boards/models/list.js.es6 +++ b/app/assets/javascripts/boards/models/list.js.es6 @@ -111,8 +111,16 @@ class List { addIssue (issue, listFrom, newIndex) { if (!this.findIssue(issue.id)) { + let moveBeforeIid = null; + let moveAfterIid = null; + if (newIndex !== undefined) { this.issues.splice(newIndex, 0, issue); + + const issueBefore = this.issues[newIndex - 1]; + if (issueBefore) moveAfterIid = issueBefore.id; + const issueAfter = this.issues[newIndex + 1]; + if (issueAfter) moveBeforeIid = issueAfter.id; } else { this.issues.push(issue); } @@ -123,7 +131,7 @@ class List { if (listFrom) { this.issuesSize += 1; - gl.boardService.moveIssue(issue.id, listFrom.id, this.id) + gl.boardService.moveIssue(issue.id, listFrom.id, this.id, moveAfterIid, moveBeforeIid) .then(() => { listFrom.getIssues(false); }); @@ -131,6 +139,21 @@ class List { } } + moveIssue (issue, oldIndex, newIndex) { + let moveBeforeIid = null; + let moveAfterIid = null; + + this.issues.splice(oldIndex, 1); + this.issues.splice(newIndex, 0, issue); + + const issueBefore = this.issues[newIndex - 1]; + if (issueBefore) moveAfterIid = issueBefore.id; + const issueAfter = this.issues[newIndex + 1]; + if (issueAfter) moveBeforeIid = issueAfter.id; + + gl.boardService.moveIssue(issue.id, null, null, moveAfterIid, moveBeforeIid); + } + findIssue (id) { return this.issues.filter(issue => issue.id === id)[0]; } diff --git a/app/assets/javascripts/boards/services/board_service.js.es6 b/app/assets/javascripts/boards/services/board_service.js.es6 index 065e90518df..1561f27de7f 100644 --- a/app/assets/javascripts/boards/services/board_service.js.es6 +++ b/app/assets/javascripts/boards/services/board_service.js.es6 @@ -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_after_iid = null, move_before_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.es6 b/app/assets/javascripts/boards/stores/boards_store.js.es6 index 50842ecbaaa..c8b76f839a5 100644 --- a/app/assets/javascripts/boards/stores/boards_store.js.es6 +++ b/app/assets/javascripts/boards/stores/boards_store.js.es6 @@ -106,6 +106,9 @@ listFrom.removeIssue(issue); } }, + moveIssueInList (list, issue, oldIndex, newIndex) { + list.moveIssue(issue, oldIndex, newIndex); + }, findList (key, val, type = 'label') { return this.state.lists.filter((list) => { const byType = type ? list['type'] === type : true; diff --git a/app/controllers/projects/boards/issues_controller.rb b/app/controllers/projects/boards/issues_controller.rb index 61fef4dc133..25e50995764 100644 --- a/app/controllers/projects/boards/issues_controller.rb +++ b/app/controllers/projects/boards/issues_controller.rb @@ -63,7 +63,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 diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb new file mode 100644 index 00000000000..b9af6350c07 --- /dev/null +++ b/app/models/concerns/relative_positioning.rb @@ -0,0 +1,132 @@ +module RelativePositioning + extend ActiveSupport::Concern + + MIN_POSITION = Float::MIN + MAX_POSITION = Float::MAX + + included do + after_save :save_positionable_neighbours + end + + def min_relative_position + self.class.minimum(:relative_position) + end + + def max_relative_position + self.class.maximum(:relative_position) + end + + def prev_relative_position + prev_pos = nil + + if self.relative_position + prev_pos = self.class. + 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. + where('relative_position > ?', self.relative_position). + minimum(:relative_position) + end + + next_pos || MAX_POSITION + end + + def move_between(before, after) + return move_nowhere unless before || after + return move_after(before) if before && !after + return move_before(after) if after && !before + + pos_before = before.relative_position + pos_after = after.relative_position + + if pos_before && pos_after + if pos_before == pos_after + pos = pos_before + + self.relative_position = pos + before.move_before(self) + after.move_after(self) + @positionable_neighbours = [before, after] + else + self.relative_position = position_between(pos_before, pos_after) + end + elsif pos_before + self.move_after(before) + after.move_after(self) + @positionable_neighbours = [after] + elsif pos_after + self.move_before(after) + before.move_before(self) + @positionable_neighbours = [before] + else + move_to_end + before.move_before(self) + after.move_after(self) + @positionable_neighbours = [before, after] + end + end + + def move_before(after) + pos_after = after.relative_position + if pos_after + self.relative_position = position_between(after.prev_relative_position, pos_after) + else + move_to_end + after.move_after(self) + @positionable_neighbours = [after] + end + end + + def move_after(before) + pos_before = before.relative_position + if pos_before + self.relative_position = position_between(pos_before, before.next_relative_position) + else + move_to_end + before.move_before(self) + @positionable_neighbours = [before] + end + end + + def move_nowhere + self.relative_position = nil + end + + def move_to_front + self.relative_position = position_between(MIN_POSITION, min_relative_position || MAX_POSITION) + end + + def move_to_end + self.relative_position = position_between(max_relative_position || MIN_POSITION, MAX_POSITION) + end + + def move_between!(*args) + move_between(*args) && save! + end + + private + + def position_between(pos_before, pos_after) + pos_before, pos_after = [pos_before, pos_after].sort + + pos_before + (pos_after - pos_before) / 2 + end + + def save_positionable_neighbours + return unless @positionable_neighbours + + @positionable_neighbours.each(&:save) + @positionable_neighbours = nil + + true + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index d8826b65fcc..f3c8f49cc66 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..7c0df55e9b6 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,13 @@ module Boards Array(label_ids).compact 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 + [move_after_iid, move_before_iid] + end end end end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 5f3ced49665..2256d7db008 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -212,7 +212,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? && update_issuable(issuable, params) + if (issuable.changed? || params.present?) && update_issuable(issuable, params) # We do not touch as it will affect a update on updated_at field ActiveRecord::Base.no_touching do handle_common_system_notes(issuable, old_labels: old_labels) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 78cbf94ec69..ba06f208dbc 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -1,6 +1,7 @@ module Issues class UpdateService < Issues::BaseService def execute(issue) + handle_move_between_iids(issue) update(issue) end @@ -47,6 +48,26 @@ module Issues Issues::CloseService end + def handle_move_between_iids(issue) + if move_between_iids = params.delete(:move_between_iids) + before_iid, after_iid = move_between_iids + + issue_before = nil + if before_iid + issue_before = issue.project.issues.find_by(iid: before_iid) + issue_before = nil unless can?(current_user, :update_issue, issue_before) + end + + issue_after = nil + if after_iid + issue_after = issue.project.issues.find_by(iid: after_iid) + issue_after = nil unless can?(current_user, :update_issue, issue_after) + end + + issue.move_between(issue_before, issue_after) + end + end + private def create_confidentiality_note(issue) 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..41e17cf13ba --- /dev/null +++ b/db/migrate/20170131221752_add_relative_position_to_issues.rb @@ -0,0 +1,31 @@ +# 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 change + add_column :issues, :relative_position, :float + + add_index :issues, :relative_position + end +end diff --git a/db/schema.rb b/db/schema.rb index 52672406ec6..545dd4ad7c0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -87,9 +87,9 @@ ActiveRecord::Schema.define(version: 20170214111112) do t.boolean "send_user_confirmation_email", default: false t.integer "container_registry_token_expire_delay", default: 5 t.text "after_sign_up_text" - t.boolean "user_default_external", default: false, null: false t.string "repository_storages", default: "default" t.string "enabled_git_access_protocol" + t.boolean "user_default_external", default: false, null: false t.boolean "domain_blacklist_enabled", default: false t.text "domain_blacklist" t.boolean "koding_enabled" @@ -402,22 +402,22 @@ ActiveRecord::Schema.define(version: 20170214111112) do add_index "deploy_keys_projects", ["project_id"], name: "index_deploy_keys_projects_on_project_id", using: :btree create_table "deployments", force: :cascade do |t| - t.integer "iid", null: false - t.integer "project_id", null: false - t.integer "environment_id", null: false - t.string "ref", null: false - t.boolean "tag", null: false - t.string "sha", null: false + t.integer "iid" + t.integer "project_id" + t.integer "environment_id" + t.string "ref" + t.boolean "tag" + t.string "sha" t.integer "user_id" - t.integer "deployable_id" - t.string "deployable_type" + t.integer "deployable_id", null: false + t.string "deployable_type", null: false t.datetime "created_at" t.datetime "updated_at" t.string "on_stop" end add_index "deployments", ["project_id", "environment_id", "iid"], name: "index_deployments_on_project_id_and_environment_id_and_iid", using: :btree - add_index "deployments", ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", unique: true, using: :btree + add_index "deployments", ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", using: :btree create_table "emails", force: :cascade do |t| t.integer "user_id", null: false @@ -514,6 +514,7 @@ ActiveRecord::Schema.define(version: 20170214111112) do t.text "title_html" t.text "description_html" t.integer "time_estimate" + t.float "relative_position" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -525,6 +526,7 @@ ActiveRecord::Schema.define(version: 20170214111112) 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"} @@ -692,8 +694,8 @@ ActiveRecord::Schema.define(version: 20170214111112) do t.integer "merge_user_id" t.string "merge_commit_sha" t.datetime "deleted_at" - t.string "in_progress_merge_commit_sha" t.integer "lock_version" + t.string "in_progress_merge_commit_sha" t.text "title_html" t.text "description_html" t.integer "time_estimate" @@ -770,6 +772,16 @@ ActiveRecord::Schema.define(version: 20170214111112) do add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"} add_index "namespaces", ["type"], name: "index_namespaces_on_type", using: :btree + create_table "note_templates", force: :cascade do |t| + t.integer "user_id" + t.string "title" + t.text "note" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "note_templates", ["user_id"], name: "index_note_templates_on_user_id", using: :btree + create_table "notes", force: :cascade do |t| t.text "note" t.string "noteable_type" @@ -785,6 +797,7 @@ ActiveRecord::Schema.define(version: 20170214111112) do t.text "st_diff" t.integer "updated_by_id" t.string "type" + t.string "system_type" t.text "position" t.text "original_position" t.datetime "resolved_at" @@ -978,7 +991,7 @@ ActiveRecord::Schema.define(version: 20170214111112) do t.boolean "has_external_wiki" t.boolean "lfs_enabled" t.text "description_html" - t.boolean "only_allow_merge_if_all_discussions_are_resolved" + t.boolean "only_allow_merge_if_all_discussions_are_resolved", default: false, null: false end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree