From 5815c5b48ac03dbd89a239e87c0f49216a428563 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 30 Jul 2018 13:44:41 +0000 Subject: [PATCH] [Backport] View summed weights of issues in board column --- .../javascripts/boards/components/board.js | 13 +++++++++ app/assets/javascripts/boards/models/list.js | 2 ++ .../javascripts/boards/stores/boards_store.js | 10 +++++-- .../stylesheets/framework/typography.scss | 2 ++ app/assets/stylesheets/pages/boards.scss | 2 +- .../pages/issues/issue_count_badge.scss | 26 +++--------------- app/controllers/boards/issues_controller.rb | 17 +++++++----- app/services/boards/issues/list_service.rb | 27 ++++++++++++++++--- .../shared/boards/components/_board.html.haml | 24 ++++++++++------- .../boards/issues_controller_spec.rb | 4 +-- spec/javascripts/boards/boards_store_spec.js | 22 +++++++++++++++ .../services/boards/issues_list_service.rb | 10 +++++++ 12 files changed, 113 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/boards/components/board.js b/app/assets/javascripts/boards/components/board.js index a2355d7fd5c..9ad451fa375 100644 --- a/app/assets/javascripts/boards/components/board.js +++ b/app/assets/javascripts/boards/components/board.js @@ -2,6 +2,9 @@ import Sortable from 'sortablejs'; import Vue from 'vue'; +import { n__ } from '~/locale'; +import Icon from '~/vue_shared/components/icon.vue'; +import Tooltip from '~/vue_shared/directives/tooltip'; import AccessorUtilities from '../../lib/utils/accessor'; import boardList from './board_list.vue'; import BoardBlankState from './board_blank_state.vue'; @@ -17,6 +20,10 @@ gl.issueBoards.Board = Vue.extend({ boardList, 'board-delete': gl.issueBoards.BoardDelete, BoardBlankState, + Icon, + }, + directives: { + Tooltip, }, props: { list: { @@ -46,6 +53,12 @@ gl.issueBoards.Board = Vue.extend({ filter: Store.filter, }; }, + computed: { + counterTooltip() { + const { issuesSize } = this.list; + return `${n__('%d issue', '%d issues', issuesSize)}`; + }, + }, watch: { filter: { handler() { diff --git a/app/assets/javascripts/boards/models/list.js b/app/assets/javascripts/boards/models/list.js index 4f05a0e4282..050cbd8db48 100644 --- a/app/assets/javascripts/boards/models/list.js +++ b/app/assets/javascripts/boards/models/list.js @@ -136,6 +136,8 @@ class List { } this.createIssues(data.issues); + + return data; }); } diff --git a/app/assets/javascripts/boards/stores/boards_store.js b/app/assets/javascripts/boards/stores/boards_store.js index 333338489bc..76467564608 100644 --- a/app/assets/javascripts/boards/stores/boards_store.js +++ b/app/assets/javascripts/boards/stores/boards_store.js @@ -125,11 +125,17 @@ gl.issueBoards.BoardsStore = { } else if (listTo.type === 'backlog' && listFrom.type === 'assignee') { issue.removeAssignee(listFrom.assignee); listFrom.removeIssue(issue); - } else if ((listTo.type !== 'label' && listFrom.type === 'assignee') || - (listTo.type !== 'assignee' && listFrom.type === 'label')) { + } else if (this.shouldRemoveIssue(listFrom, listTo)) { listFrom.removeIssue(issue); } }, + shouldRemoveIssue(listFrom, listTo) { + return ( + (listTo.type !== 'label' && listFrom.type === 'assignee') || + (listTo.type !== 'assignee' && listFrom.type === 'label') || + (listFrom.type === 'backlog') + ); + }, moveIssueInList (list, issue, oldIndex, newIndex, idArray) { const beforeId = parseInt(idArray[newIndex - 1], 10) || null; const afterId = parseInt(idArray[newIndex + 1], 10) || null; diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index a2789021ab4..473ca408c04 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -444,3 +444,5 @@ textarea { color: $placeholder-text-color; } } + +.lh-100 { line-height: 1; } diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss index 7347da2ae61..a68b47b1d02 100644 --- a/app/assets/stylesheets/pages/boards.scss +++ b/app/assets/stylesheets/pages/boards.scss @@ -205,7 +205,7 @@ .board-title { margin: 0; - padding: 12px $gl-padding; + padding: $gl-padding-8 $gl-padding; font-size: 1em; border-bottom: 1px solid $border-color; display: flex; diff --git a/app/assets/stylesheets/pages/issues/issue_count_badge.scss b/app/assets/stylesheets/pages/issues/issue_count_badge.scss index ccb62bfed18..4fba89e956b 100644 --- a/app/assets/stylesheets/pages/issues/issue_count_badge.scss +++ b/app/assets/stylesheets/pages/issues/issue_count_badge.scss @@ -1,29 +1,11 @@ .issue-count-badge { display: inline-flex; - align-items: stretch; - height: 24px; + border-radius: $border-radius-base; + border: 1px solid $border-color; + padding: 5px $gl-padding-8; } .issue-count-badge-count { - display: flex; + display: inline-flex; align-items: center; - padding-right: 10px; - padding-left: 10px; - border: 1px solid $border-color; - border-radius: $border-radius-base; - line-height: 1; - - &.has-btn { - border-right: 0; - border-top-right-radius: 0; - border-bottom-right-radius: 0; - } -} - -.issue-count-badge-add-button { - display: flex; - align-items: center; - border: 1px solid $border-color; - border-radius: 0 $border-radius-base $border-radius-base 0; - line-height: 1; } diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index 09e143c23e8..7dd19f87ef5 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/app/controllers/boards/issues_controller.rb @@ -12,8 +12,9 @@ module Boards skip_before_action :authenticate_user!, only: [:index] def index - issues = Boards::Issues::ListService.new(board_parent, current_user, filter_params).execute - issues = issues.page(params[:page]).per(params[:per] || 20) + list_service = Boards::Issues::ListService.new(board_parent, current_user, filter_params) + issues = list_service.execute + issues = issues.page(params[:page]).per(params[:per] || 20).without_count make_sure_position_is_set(issues) if Gitlab::Database.read_write? issues = issues.preload(:project, :milestone, @@ -22,10 +23,7 @@ module Boards notes: [:award_emoji, :author] ) - render json: { - issues: serialize_as_json(issues), - size: issues.total_count - } + render_issues(issues, list_service.metadata) end def create @@ -51,6 +49,13 @@ module Boards private + def render_issues(issues, metadata) + data = { issues: serialize_as_json(issues) } + data.merge!(metadata) + + render json: data + end + def make_sure_position_is_set(issues) issues.each do |issue| issue.move_to_end && issue.save unless issue.relative_position diff --git a/app/services/boards/issues/list_service.rb b/app/services/boards/issues/list_service.rb index 50c11be0d15..0db1418b37a 100644 --- a/app/services/boards/issues/list_service.rb +++ b/app/services/boards/issues/list_service.rb @@ -3,14 +3,35 @@ module Boards module Issues class ListService < Boards::BaseService + include Gitlab::Utils::StrongMemoize + def execute - issues = IssuesFinder.new(current_user, filter_params).execute - issues = filter(issues) - issues.order_by_position_and_priority + fetch_issues.order_by_position_and_priority + end + + def metadata + keys = metadata_fields.keys + columns = metadata_fields.values_at(*keys).join(', ') + results = Issue.where(id: fetch_issues.select('issues.id')).pluck(columns) + + Hash[keys.zip(results.flatten)] end private + def metadata_fields + { size: 'COUNT(*)' } + end + + # We memoize the query here since the finder methods we use are quite complex. This does not memoize the result of the query. + def fetch_issues + strong_memoize(:fetch_issues) do + issues = IssuesFinder.new(current_user, filter_params).execute + + filter(issues).reorder(nil) + end + end + def filter(issues) issues = without_board_labels(issues) unless list&.movable? || list&.closed? issues = with_list_label(issues) if list&.label? diff --git a/app/views/shared/boards/components/_board.html.haml b/app/views/shared/boards/components/_board.html.haml index 03e008f5fa0..b35877e5518 100644 --- a/app/views/shared/boards/components/_board.html.haml +++ b/app/views/shared/boards/components/_board.html.haml @@ -32,17 +32,21 @@ "v-if" => "!list.preset && list.id" } %button.board-delete.has-tooltip.float-right{ type: "button", title: _("Delete list"), "aria-label" => _("Delete list"), data: { placement: "bottom" }, "@click.stop" => "deleteBoard" } = icon("trash") - .issue-count-badge.clearfix{ "v-if" => 'list.type !== "blank" && list.type !== "promotion"' } - %span.issue-count-badge-count.float-left{ ":class" => '{ "has-btn": list.type !== "closed" && !disabled }' } + .issue-count-badge.text-secondary{ "v-if" => 'list.type !== "blank" && list.type !== "promotion"', ":title": "counterTooltip", "v-tooltip": true, data: { placement: "top" } } + %span.issue-count-badge-count + %icon.mr-1{ name: "issues" } {{ list.issuesSize }} - - if can?(current_user, :admin_list, current_board_parent) - %button.issue-count-badge-add-button.btn.btn-sm.btn-default.has-tooltip.js-no-trigger-collapse{ type: "button", - "@click" => "showNewIssueForm", - "v-if" => 'list.type !== "closed"', - "aria-label" => _("New issue"), - "title" => _("New issue"), - data: { placement: "top", container: "body" } } - = icon("plus", class: "js-no-trigger-collapse") + = render_if_exists "shared/boards/components/list_weight" + + - if can?(current_user, :admin_list, current_board_parent) + %button.issue-count-badge-add-button.btn.btn-sm.btn-default.ml-1.has-tooltip.js-no-trigger-collapse{ type: "button", + "@click" => "showNewIssueForm", + "v-if" => 'list.type !== "closed"', + "aria-label" => _("New issue"), + "title" => _("New issue"), + data: { placement: "top", container: "body" } } + = icon("plus", class: "js-no-trigger-collapse") + %board-list{ "v-if" => 'list.type !== "blank" && list.type !== "promotion"', ":list" => "list", ":issues" => "list.issues", diff --git a/spec/controllers/boards/issues_controller_spec.rb b/spec/controllers/boards/issues_controller_spec.rb index ce7762691c9..d98e6ff0df8 100644 --- a/spec/controllers/boards/issues_controller_spec.rb +++ b/spec/controllers/boards/issues_controller_spec.rb @@ -42,7 +42,7 @@ describe Boards::IssuesController do parsed_response = JSON.parse(response.body) expect(response).to match_response_schema('issues') - expect(parsed_response.length).to eq 2 + expect(parsed_response['issues'].length).to eq 2 expect(development.issues.map(&:relative_position)).not_to include(nil) end @@ -80,7 +80,7 @@ describe Boards::IssuesController do parsed_response = JSON.parse(response.body) expect(response).to match_response_schema('issues') - expect(parsed_response.length).to eq 2 + expect(parsed_response['issues'].length).to eq 2 end end diff --git a/spec/javascripts/boards/boards_store_spec.js b/spec/javascripts/boards/boards_store_spec.js index f7af099b3bf..1ee6f4cf680 100644 --- a/spec/javascripts/boards/boards_store_spec.js +++ b/spec/javascripts/boards/boards_store_spec.js @@ -161,6 +161,28 @@ describe('Store', () => { }, 0); }); + it('moves an issue from backlog to a list', (done) => { + const backlog = gl.issueBoards.BoardsStore.addList({ + ...listObj, + list_type: 'backlog', + }); + const listTwo = gl.issueBoards.BoardsStore.addList(listObjDuplicate); + + expect(gl.issueBoards.BoardsStore.state.lists.length).toBe(2); + + setTimeout(() => { + expect(backlog.issues.length).toBe(1); + expect(listTwo.issues.length).toBe(1); + + gl.issueBoards.BoardsStore.moveIssueToList(backlog, listTwo, backlog.findIssue(1)); + + expect(backlog.issues.length).toBe(0); + expect(listTwo.issues.length).toBe(1); + + 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); diff --git a/spec/support/shared_examples/services/boards/issues_list_service.rb b/spec/support/shared_examples/services/boards/issues_list_service.rb index 3e744323cea..8b879cef084 100644 --- a/spec/support/shared_examples/services/boards/issues_list_service.rb +++ b/spec/support/shared_examples/services/boards/issues_list_service.rb @@ -7,6 +7,16 @@ shared_examples 'issues list service' do described_class.new(parent, user, params).execute end + context '#metadata' do + it 'returns issues count for list' do + params = { board_id: board.id, id: list1.id } + + metadata = described_class.new(parent, user, params).metadata + + expect(metadata[:size]).to eq(3) + end + end + context 'issues are ordered by priority' do it 'returns opened issues when list_id is missing' do params = { board_id: board.id }