From 6df9844ba3ef28524cb024e68419d0dd2be1e9fe Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Mon, 14 Jan 2019 21:49:09 +0900 Subject: [PATCH 01/10] Add target branch filter to merge requests search bar --- .../filtered_search_dropdown.js | 5 ++- .../filtered_search_dropdown_manager.js | 7 +++- .../filtered_search_manager.js | 19 ++++------ .../filtered_search_token_keys.js | 5 +++ .../filtered_search_visual_tokens.js | 28 ++++++++++++-- app/controllers/dashboard_controller.rb | 6 +++ app/controllers/groups_controller.rb | 6 +++ .../projects/merge_requests_controller.rb | 8 +++- app/finders/merge_requests_finder.rb | 15 +++++++- .../shared/issuable/_search_bar.html.haml | 5 +++ ...filter-merge-requests-by-target-branch.yml | 5 +++ config/routes/dashboard.rb | 4 ++ config/routes/group.rb | 4 ++ config/routes/project.rb | 4 ++ spec/controllers/dashboard_controller_spec.rb | 30 +++++++++++++++ spec/controllers/groups_controller_spec.rb | 30 +++++++++++++++ .../merge_requests_controller_spec.rb | 37 +++++++++++++++++++ spec/finders/merge_requests_finder_spec.rb | 18 ++++++++- 18 files changed, 216 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/filter-merge-requests-by-target-branch.yml diff --git a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js index 146d3ba963c686..c90245849ad44e 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js +++ b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js @@ -37,7 +37,10 @@ export default class FilteredSearchDropdown { if (!dataValueSet) { const value = getValueFunction(selected); - FilteredSearchDropdownManager.addWordToInput(this.filter, value, true); + const tokenValueClass = selected.parentNode.getAttribute('data-token-value-class') || ''; + FilteredSearchDropdownManager.addWordToInput(this.filter, value, true, { + tokenValueClass, + }); } this.resetFilters(); diff --git a/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js b/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js index cb0a84b490be50..cbfd01bf6d2d4d 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js +++ b/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js @@ -57,11 +57,16 @@ export default class FilteredSearchDropdownManager { } static addWordToInput(tokenName, tokenValue = '', clicked = false, options = {}) { - const { uppercaseTokenName = false, capitalizeTokenValue = false } = options; + const { + uppercaseTokenName = false, + capitalizeTokenValue = false, + tokenValueClass = '', + } = options; const input = FilteredSearchContainer.container.querySelector('.filtered-search'); FilteredSearchVisualTokens.addFilterVisualToken(tokenName, tokenValue, { uppercaseTokenName, capitalizeTokenValue, + tokenValueClass, }); input.value = ''; diff --git a/app/assets/javascripts/filtered_search/filtered_search_manager.js b/app/assets/javascripts/filtered_search/filtered_search_manager.js index a54b88b78df410..cb5e228b645f3c 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_manager.js +++ b/app/assets/javascripts/filtered_search/filtered_search_manager.js @@ -437,6 +437,7 @@ export default class FilteredSearchManager { FilteredSearchVisualTokens.addFilterVisualToken(t.key, `${t.symbol}${t.value}`, { uppercaseTokenName: this.filteredSearchTokenKeys.shouldUppercaseTokenName(t.key), capitalizeTokenValue: this.filteredSearchTokenKeys.shouldCapitalizeTokenValue(t.key), + tokenValueClass: this.filteredSearchTokenKeys.getTokenValueClass(t.key), }); }); @@ -456,6 +457,7 @@ export default class FilteredSearchManager { FilteredSearchVisualTokens.addFilterVisualToken(tokenKey, null, { uppercaseTokenName: this.filteredSearchTokenKeys.shouldUppercaseTokenName(tokenKey), capitalizeTokenValue: this.filteredSearchTokenKeys.shouldCapitalizeTokenValue(tokenKey), + tokenValueClass: this.filteredSearchTokenKeys.getTokenValueClass(tokenKey), }); input.value = input.value.replace(`${tokenKey}:`, ''); } @@ -467,6 +469,7 @@ export default class FilteredSearchManager { const tokenKey = FilteredSearchVisualTokens.getLastTokenPartial(); FilteredSearchVisualTokens.addFilterVisualToken(searchToken, null, { capitalizeTokenValue: this.filteredSearchTokenKeys.shouldCapitalizeTokenValue(tokenKey), + tokenValueClass: this.filteredSearchTokenKeys.getTokenValueClass(tokenKey), }); // Trim the last space as seen in the if statement above @@ -527,14 +530,7 @@ export default class FilteredSearchManager { const match = this.filteredSearchTokenKeys.searchByKeyParam(keyParam); if (match) { - // Use lastIndexOf because the token key is allowed to contain underscore - // e.g. 'my_reaction' is the token key of 'my_reaction_emoji' - const lastIndexOf = keyParam.lastIndexOf('_'); - let sanitizedKey = lastIndexOf !== -1 ? keyParam.slice(0, lastIndexOf) : keyParam; - // Replace underscore with hyphen in the sanitizedkey. - // e.g. 'my_reaction' => 'my-reaction' - sanitizedKey = sanitizedKey.replace('_', '-'); - const { symbol } = match; + const { key, symbol } = match; let quotationsToUse = ''; if (sanitizedValue.indexOf(' ') !== -1) { @@ -543,15 +539,16 @@ export default class FilteredSearchManager { } hasFilteredSearch = true; - const canEdit = this.canEdit && this.canEdit(sanitizedKey, sanitizedValue); - const { uppercaseTokenName, capitalizeTokenValue } = match; + const canEdit = this.canEdit && this.canEdit(key, sanitizedValue); + const { uppercaseTokenName, capitalizeTokenValue, tokenValueClass } = match; FilteredSearchVisualTokens.addFilterVisualToken( - sanitizedKey, + key, `${symbol}${quotationsToUse}${sanitizedValue}${quotationsToUse}`, { canEdit, uppercaseTokenName, capitalizeTokenValue, + tokenValueClass, }, ); } else if (!match && keyParam === 'assignee_id') { diff --git a/app/assets/javascripts/filtered_search/filtered_search_token_keys.js b/app/assets/javascripts/filtered_search/filtered_search_token_keys.js index 11ed85504ec662..562f40787d7a53 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_token_keys.js +++ b/app/assets/javascripts/filtered_search/filtered_search_token_keys.js @@ -33,6 +33,11 @@ export default class FilteredSearchTokenKeys { return token && token.capitalizeTokenValue; } + getTokenValueClass(tokenKey) { + const token = this.searchByKey(tokenKey.toLowerCase()); + return token && token.tokenValueClass; + } + searchByKey(key) { return this.tokenKeys.find(tokenKey => tokenKey.key === key) || null; } diff --git a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js index 7746908714e2ab..f201e693de9f86 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js +++ b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js @@ -42,13 +42,19 @@ export default class FilteredSearchVisualTokens { } static createVisualTokenElementHTML(options = {}) { - const { canEdit = true, uppercaseTokenName = false, capitalizeTokenValue = false } = options; + const { + canEdit = true, + uppercaseTokenName = false, + capitalizeTokenValue = false, + tokenValueClass = '', + } = options; return `
-
+
@@ -69,7 +75,13 @@ export default class FilteredSearchVisualTokens { } static addVisualTokenElement(name, value, options = {}) { - const { isSearchTerm = false, canEdit, uppercaseTokenName, capitalizeTokenValue } = options; + const { + isSearchTerm = false, + canEdit, + uppercaseTokenName, + capitalizeTokenValue, + tokenValueClass, + } = options; const li = document.createElement('li'); li.classList.add('js-visual-token'); li.classList.add(isSearchTerm ? 'filtered-search-term' : 'filtered-search-token'); @@ -79,6 +91,7 @@ export default class FilteredSearchVisualTokens { canEdit, uppercaseTokenName, capitalizeTokenValue, + tokenValueClass, }); FilteredSearchVisualTokens.renderVisualTokenValue(li, name, value); } else { @@ -108,7 +121,12 @@ export default class FilteredSearchVisualTokens { static addFilterVisualToken( tokenName, tokenValue, - { canEdit, uppercaseTokenName = false, capitalizeTokenValue = false } = {}, + { + canEdit, + uppercaseTokenName = false, + capitalizeTokenValue = false, + tokenValueClass = '', + } = {}, ) { const { lastVisualToken, @@ -121,6 +139,7 @@ export default class FilteredSearchVisualTokens { canEdit, uppercaseTokenName, capitalizeTokenValue, + tokenValueClass, }); } else { const previousTokenName = lastVisualToken.querySelector('.name').innerText; @@ -132,6 +151,7 @@ export default class FilteredSearchVisualTokens { canEdit, uppercaseTokenName, capitalizeTokenValue, + tokenValueClass, }); } } diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 75329b05a6f8fb..8a3efd1f33e924 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -24,6 +24,12 @@ def activity end end + def merge_requests_target_branches + target_branches = MergeRequestsFinder.new(current_user, non_archived: true).only_target_branches + + render json: target_branches.map { |target_branch| { title: target_branch } } + end + protected def load_events diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index afe279b137febd..f483a75cf6eb01 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -102,6 +102,12 @@ def destroy redirect_to root_path, status: 302, alert: "Group '#{@group.name}' was scheduled for deletion." end + def merge_requests_target_branches + target_branches = MergeRequestsFinder.new(current_user, group_id: @group.id, non_archived: true).only_target_branches + + render json: target_branches.map { |target_branch| { title: target_branch } } + end + # rubocop: disable CodeReuse/ActiveRecord def transfer parent_group = Group.find_by(id: params[:new_parent_group_id]) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 4487749968b7ea..5877d4abaad16e 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -9,7 +9,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo include IssuableCollections include RecordUserLastActivity - skip_before_action :merge_request, only: [:index, :bulk_update] + skip_before_action :merge_request, only: [:index, :bulk_update, :target_branches] before_action :whitelist_query_limiting, only: [:assign_related_issues, :update] before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :set_issuables_index, only: [:index] @@ -230,6 +230,12 @@ def discussions super end + def target_branches + target_branches = MergeRequestsFinder.new(@current_user, project_id: @project.id).only_target_branches + + render json: target_branches.map { |target_branch| { title: target_branch } } + end + protected alias_method :subscribable_resource, :merge_request diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index 3fc4a281fb6bce..31fa30d3a4921d 100644 --- a/app/finders/merge_requests_finder.rb +++ b/app/finders/merge_requests_finder.rb @@ -28,8 +28,10 @@ # updated_before: datetime # class MergeRequestsFinder < IssuableFinder + TARGET_BRANCH_MAX = 100 + def self.scalar_params - @scalar_params ||= super + [:wip] + @scalar_params ||= super + [:wip, :target_branch] end def klass @@ -43,6 +45,17 @@ def filter_items(_items) by_target_branch(items) end + # rubocop: disable CodeReuse/ActiveRecord + def only_target_branches + execute + .group(:target_branch) + .select(:target_branch) + .reorder('MAX(merge_requests.updated_at) DESC') + .limit(TARGET_BRANCH_MAX) + .pluck(:target_branch) + end + # rubocop: enable CodeReuse/ActiveRecord + private def by_commit(items) diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index f43be304e6b0cc..755f1d8c3be467 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -137,6 +137,11 @@ %li.filter-dropdown-item{ data: { value: 'no', capitalize: true } } %button.btn.btn-link{ type: 'button' } = _('No') + #js-dropdown-target-branch.filtered-search-input-dropdown-menu.dropdown-menu + %ul.filter-dropdown.ref-name{ data: { dynamic: true, dropdown: true, token_value_class: 'ref-name' } } + %li.filter-dropdown-item + %button.btn.btn-link.js-data-value + {{title}} = render_if_exists 'shared/issuable/filter_weight', type: type diff --git a/changelogs/unreleased/filter-merge-requests-by-target-branch.yml b/changelogs/unreleased/filter-merge-requests-by-target-branch.yml new file mode 100644 index 00000000000000..d0aba631c96a58 --- /dev/null +++ b/changelogs/unreleased/filter-merge-requests-by-target-branch.yml @@ -0,0 +1,5 @@ +--- +title: Add target branch filter to merge requests search bar +merge_request: 24380 +author: Hiroyuki Sato +type: added diff --git a/config/routes/dashboard.rb b/config/routes/dashboard.rb index f1e8c2b9d8251f..16c148e2198713 100644 --- a/config/routes/dashboard.rb +++ b/config/routes/dashboard.rb @@ -4,6 +4,10 @@ get :merge_requests get :activity + scope constraints: { format: 'json' }, as: :json do + get 'merge_requests/target_branches', to: 'dashboard#merge_requests_target_branches' + end + scope module: :dashboard do resources :milestones, only: [:index, :show] do member do diff --git a/config/routes/group.rb b/config/routes/group.rb index b3015529c6ecd9..dcbe5bb9a99816 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -19,6 +19,10 @@ # TODO: Remove as part of refactor in https://gitlab.com/gitlab-org/gitlab-ce/issues/49693 get 'shared', action: :show, as: :group_shared get 'archived', action: :show, as: :group_archived + + scope constraints: { format: 'json' }, as: :json do + get 'merge_requests/target_branches', to: 'groups#merge_requests_target_branches' + end end get '/', action: :show, as: :group_canonical diff --git a/config/routes/project.rb b/config/routes/project.rb index b7393b80af44a2..0dd8df22e0731c 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -155,6 +155,10 @@ collection do get :diff_for_path post :bulk_update + + scope constraints: { format: 'json' }, as: :json do + get :target_branches + end end ## EE-specific diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index c857a78d5e8c09..2bb5b74af5a970 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -19,6 +19,36 @@ it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests it_behaves_like 'issuables requiring filter', :merge_requests end + + describe 'GET merge_requests_target_branches' do + subject { get :merge_requests_target_branches, format: :json } + + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'merged-target') } + + context 'with authorized user' do + before do + sign_in(user) + end + + it 'returns target branches' do + subject + + expect(json_response).to match_array([{ 'title' => 'merged-target' }]) + end + end + + context 'with unauthorized user' do + before do + sign_out(:user) + end + + it 'does not return any target branches' do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end end it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 792acfa101d83a..c3bb43dce9d34a 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -562,6 +562,36 @@ def group_moved_message(redirect_route, group) end end + describe 'GET merge_requests_target_branches' do + subject { get :merge_requests_target_branches, params: { id: group.to_param }, format: :json } + + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'merged-target') } + + context 'with unauthorized user' do + before do + sign_out(:user) + end + + it 'does not return any target branches' do + subject + + expect(json_response).to be_empty + end + end + + context 'with authorized user' do + before do + sign_in(user) + end + + it 'returns target branches' do + subject + + expect(json_response).to match_array([{ 'title' => 'merged-target' }]) + end + end + end + describe 'PUT transfer', :postgresql do before do sign_in(user) diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 79f97aa4170c78..9de3683d8e52cf 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -681,6 +681,43 @@ def go(format: 'html') end end + describe 'GET target_branches' do + subject do + get :target_branches, + params: { + namespace_id: project.namespace.to_param, + project_id: project + }, + format: :json + end + + let!(:merge_request1) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'merged-target') } + + context 'with unauthorized user' do + before do + sign_out(:user) + end + + it 'returns unauthorized http status' do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'with authorized user' do + before do + sign_in(user) + end + + it 'returns target branches' do + subject + + expect(json_response).to match_array([{ 'title' => 'merged-target' }]) + end + end + end + describe 'POST remove_wip' do before do merge_request.title = merge_request.wip_title diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 59cc8b2654d595..28301257e1efcc 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -36,7 +36,7 @@ def create_project_without_n_plus_1(*args) let(:project5) { create_project_without_n_plus_1(group: subgroup) } let(:project6) { create_project_without_n_plus_1(group: subgroup) } - let!(:merge_request1) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1) } + let!(:merge_request1) { create(:merge_request, author: user, source_project: project2, target_project: project1, target_branch: 'merged-target') } let!(:merge_request2) { create(:merge_request, :conflict, author: user, source_project: project2, target_project: project1, state: 'closed') } let!(:merge_request3) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project2, state: 'locked', title: 'thing WIP thing') } let!(:merge_request4) { create(:merge_request, :simple, author: user, source_project: project3, target_project: project3, title: 'WIP thing') } @@ -285,6 +285,22 @@ def create_project_without_n_plus_1(*args) expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request) end end + + describe '#only_target_branches' do + it 'returns target branches sort by recently updated at' do + merge_request1.update_columns(updated_at: 1.day.since) + merge_request2.update_columns(updated_at: 2.days.since) + merge_request3.update_columns(updated_at: 3.days.since) + target_branches = described_class.new(user).only_target_branches + + expect(target_branches).to eq(%w[master feature merged-target]) + + stub_const('MergeRequestsFinder::TARGET_BRANCH_MAX', 1) + target_branches = described_class.new(user).only_target_branches + + expect(target_branches).to eq(%w[master]) + end + end end describe '#row_count', :request_store do -- GitLab From 04e891471edea7088fae64205d080f91b5033d37 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Mon, 21 Jan 2019 22:24:18 +0900 Subject: [PATCH 02/10] Refactor backend Conflicts: config/routes.rb --- app/controllers/autocomplete_controller.rb | 9 ++++- app/controllers/dashboard_controller.rb | 6 --- app/controllers/groups_controller.rb | 6 --- .../projects/merge_requests_controller.rb | 8 +--- app/finders/merge_requests_finder.rb | 13 ------- app/models/merge_request.rb | 16 ++++++++ config/routes.rb | 1 + config/routes/dashboard.rb | 4 -- config/routes/group.rb | 4 -- config/routes/project.rb | 4 -- .../autocomplete_controller_spec.rb | 31 ++++++++++++++++ spec/controllers/dashboard_controller_spec.rb | 30 --------------- spec/controllers/groups_controller_spec.rb | 30 --------------- .../merge_requests_controller_spec.rb | 37 ------------------- spec/finders/merge_requests_finder_spec.rb | 16 -------- spec/models/merge_request_spec.rb | 19 ++++++++++ 16 files changed, 76 insertions(+), 158 deletions(-) diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 1d33f175a0e610..d72c054ee8d750 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AutocompleteController < ApplicationController - skip_before_action :authenticate_user!, only: [:users, :award_emojis] + skip_before_action :authenticate_user!, only: [:users, :award_emojis, :merge_request_target_branches] def users project = Autocomplete::ProjectFinder @@ -38,6 +38,13 @@ def projects def award_emojis render json: AwardedEmojiFinder.new(current_user).execute end + + def merge_request_target_branches + merge_requests = MergeRequestsFinder.new(current_user, params).execute + target_branches = merge_requests.recently_target_branches + + render json: target_branches.map { |target_branch| { title: target_branch } } + end end AutocompleteController.prepend(EE::AutocompleteController) diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 8a3efd1f33e924..75329b05a6f8fb 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -24,12 +24,6 @@ def activity end end - def merge_requests_target_branches - target_branches = MergeRequestsFinder.new(current_user, non_archived: true).only_target_branches - - render json: target_branches.map { |target_branch| { title: target_branch } } - end - protected def load_events diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index f483a75cf6eb01..afe279b137febd 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -102,12 +102,6 @@ def destroy redirect_to root_path, status: 302, alert: "Group '#{@group.name}' was scheduled for deletion." end - def merge_requests_target_branches - target_branches = MergeRequestsFinder.new(current_user, group_id: @group.id, non_archived: true).only_target_branches - - render json: target_branches.map { |target_branch| { title: target_branch } } - end - # rubocop: disable CodeReuse/ActiveRecord def transfer parent_group = Group.find_by(id: params[:new_parent_group_id]) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 5877d4abaad16e..4487749968b7ea 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -9,7 +9,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo include IssuableCollections include RecordUserLastActivity - skip_before_action :merge_request, only: [:index, :bulk_update, :target_branches] + skip_before_action :merge_request, only: [:index, :bulk_update] before_action :whitelist_query_limiting, only: [:assign_related_issues, :update] before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :set_issuables_index, only: [:index] @@ -230,12 +230,6 @@ def discussions super end - def target_branches - target_branches = MergeRequestsFinder.new(@current_user, project_id: @project.id).only_target_branches - - render json: target_branches.map { |target_branch| { title: target_branch } } - end - protected alias_method :subscribable_resource, :merge_request diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index 31fa30d3a4921d..84c2ae42bcd329 100644 --- a/app/finders/merge_requests_finder.rb +++ b/app/finders/merge_requests_finder.rb @@ -28,8 +28,6 @@ # updated_before: datetime # class MergeRequestsFinder < IssuableFinder - TARGET_BRANCH_MAX = 100 - def self.scalar_params @scalar_params ||= super + [:wip, :target_branch] end @@ -45,17 +43,6 @@ def filter_items(_items) by_target_branch(items) end - # rubocop: disable CodeReuse/ActiveRecord - def only_target_branches - execute - .group(:target_branch) - .select(:target_branch) - .reorder('MAX(merge_requests.updated_at) DESC') - .limit(TARGET_BRANCH_MAX) - .pluck(:target_branch) - end - # rubocop: enable CodeReuse/ActiveRecord - private def by_commit(items) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e4e6fa45067013..0dc8ae711598bb 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -205,6 +205,22 @@ def self.reference_prefix '!' end + # Returns the top 100 target branches + # + # The returned value is a Array containing branch names + # sort by updated_at of merge request: + # + # ['master', 'develop', 'production'] + # + # limit - The maximum number of target branch to return. + def self.recently_target_branches(limit: 100) + group(:target_branch) + .select(:target_branch) + .reorder('MAX(merge_requests.updated_at) DESC') + .limit(limit) + .pluck(:target_branch) + end + def rebase_in_progress? strong_memoize(:rebase_in_progress) do # The source project can be deleted diff --git a/config/routes.rb b/config/routes.rb index 5c53f9a3872585..d47ec7c864bc3c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -47,6 +47,7 @@ get '/autocomplete/projects' => 'autocomplete#projects' get '/autocomplete/award_emojis' => 'autocomplete#award_emojis' get '/autocomplete/project_groups' => 'autocomplete#project_groups' + get '/autocomplete/merge_request_target_branches' => 'autocomplete#merge_request_target_branches' # Search get 'search' => 'search#show' diff --git a/config/routes/dashboard.rb b/config/routes/dashboard.rb index 16c148e2198713..f1e8c2b9d8251f 100644 --- a/config/routes/dashboard.rb +++ b/config/routes/dashboard.rb @@ -4,10 +4,6 @@ get :merge_requests get :activity - scope constraints: { format: 'json' }, as: :json do - get 'merge_requests/target_branches', to: 'dashboard#merge_requests_target_branches' - end - scope module: :dashboard do resources :milestones, only: [:index, :show] do member do diff --git a/config/routes/group.rb b/config/routes/group.rb index dcbe5bb9a99816..b3015529c6ecd9 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -19,10 +19,6 @@ # TODO: Remove as part of refactor in https://gitlab.com/gitlab-org/gitlab-ce/issues/49693 get 'shared', action: :show, as: :group_shared get 'archived', action: :show, as: :group_archived - - scope constraints: { format: 'json' }, as: :json do - get 'merge_requests/target_branches', to: 'groups#merge_requests_target_branches' - end end get '/', action: :show, as: :group_canonical diff --git a/config/routes/project.rb b/config/routes/project.rb index 0dd8df22e0731c..b7393b80af44a2 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -155,10 +155,6 @@ collection do get :diff_for_path post :bulk_update - - scope constraints: { format: 'json' }, as: :json do - get :target_branches - end end ## EE-specific diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index 89b43928cb645a..93317f3dddbb25 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -409,5 +409,36 @@ expect(json_response[3]).to match('name' => 'thumbsdown') end end + + context 'Get merge_request_target_branches' do + let(:user2) { create(:user) } + let!(:merge_request1) { create(:merge_request, source_project: project, target_branch: 'feature') } + + context 'unauthorized user' do + it 'returns empty json' do + get :merge_request_target_branches + + expect(json_response).to be_empty + end + end + + context 'sign in as user without any accesible merge requests' do + it 'returns empty json' do + sign_in(user2) + get :merge_request_target_branches + + expect(json_response).to be_empty + end + end + + context 'sign in as user with a accesible merge request' do + it 'returns json' do + sign_in(user) + get :merge_request_target_branches + + expect(json_response).to contain_exactly({ 'title' => 'feature' }) + end + end + end end end diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index 2bb5b74af5a970..c857a78d5e8c09 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -19,36 +19,6 @@ it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests it_behaves_like 'issuables requiring filter', :merge_requests end - - describe 'GET merge_requests_target_branches' do - subject { get :merge_requests_target_branches, format: :json } - - let!(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'merged-target') } - - context 'with authorized user' do - before do - sign_in(user) - end - - it 'returns target branches' do - subject - - expect(json_response).to match_array([{ 'title' => 'merged-target' }]) - end - end - - context 'with unauthorized user' do - before do - sign_out(:user) - end - - it 'does not return any target branches' do - subject - - expect(response).to have_gitlab_http_status(:unauthorized) - end - end - end end it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index c3bb43dce9d34a..792acfa101d83a 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -562,36 +562,6 @@ def group_moved_message(redirect_route, group) end end - describe 'GET merge_requests_target_branches' do - subject { get :merge_requests_target_branches, params: { id: group.to_param }, format: :json } - - let!(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'merged-target') } - - context 'with unauthorized user' do - before do - sign_out(:user) - end - - it 'does not return any target branches' do - subject - - expect(json_response).to be_empty - end - end - - context 'with authorized user' do - before do - sign_in(user) - end - - it 'returns target branches' do - subject - - expect(json_response).to match_array([{ 'title' => 'merged-target' }]) - end - end - end - describe 'PUT transfer', :postgresql do before do sign_in(user) diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 9de3683d8e52cf..79f97aa4170c78 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -681,43 +681,6 @@ def go(format: 'html') end end - describe 'GET target_branches' do - subject do - get :target_branches, - params: { - namespace_id: project.namespace.to_param, - project_id: project - }, - format: :json - end - - let!(:merge_request1) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'merged-target') } - - context 'with unauthorized user' do - before do - sign_out(:user) - end - - it 'returns unauthorized http status' do - subject - - expect(response).to have_gitlab_http_status(:unauthorized) - end - end - - context 'with authorized user' do - before do - sign_in(user) - end - - it 'returns target branches' do - subject - - expect(json_response).to match_array([{ 'title' => 'merged-target' }]) - end - end - end - describe 'POST remove_wip' do before do merge_request.title = merge_request.wip_title diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 28301257e1efcc..ef7088f1d771a7 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -285,22 +285,6 @@ def create_project_without_n_plus_1(*args) expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request) end end - - describe '#only_target_branches' do - it 'returns target branches sort by recently updated at' do - merge_request1.update_columns(updated_at: 1.day.since) - merge_request2.update_columns(updated_at: 2.days.since) - merge_request3.update_columns(updated_at: 3.days.since) - target_branches = described_class.new(user).only_target_branches - - expect(target_branches).to eq(%w[master feature merged-target]) - - stub_const('MergeRequestsFinder::TARGET_BRANCH_MAX', 1) - target_branches = described_class.new(user).only_target_branches - - expect(target_branches).to eq(%w[master]) - end - end end describe '#row_count', :request_store do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 85120a96b30fa6..36d648d5940f4c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -270,6 +270,25 @@ def create_merge_request_with_diffs(source_branch, diffs: 2) end end + describe '.recently_target_branches' do + let(:project) { create(:project) } + let!(:merge_request1) { create(:merge_request, :opened, source_project: project, target_branch: 'feature') } + let!(:merge_request2) { create(:merge_request, :closed, source_project: project, target_branch: 'merge-test') } + let!(:merge_request3) { create(:merge_request, :opened, source_project: project, target_branch: 'fix') } + let!(:merge_request4) { create(:merge_request, :closed, source_project: project, target_branch: 'feature') } + + before do + merge_request1.update_columns(updated_at: 1.day.since) + merge_request2.update_columns(updated_at: 2.days.since) + merge_request3.update_columns(updated_at: 3.days.since) + merge_request4.update_columns(updated_at: 4.days.since) + end + + it 'returns target branches sort by updated at desc' do + expect(described_class.recently_target_branches).to match_array(['feature', 'merge-test', 'fix']) + end + end + describe '#target_branch_sha' do let(:project) { create(:project, :repository) } -- GitLab From 4688f0273ccba40ba1f187dba05f07c4e2909b2a Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Mon, 21 Jan 2019 22:28:47 +0900 Subject: [PATCH 03/10] Add getTargetBranchesEndpoint method --- .../available_dropdown_mappings.js | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/app/assets/javascripts/filtered_search/available_dropdown_mappings.js b/app/assets/javascripts/filtered_search/available_dropdown_mappings.js index e2f9c03ee650ec..be867a3838d02d 100644 --- a/app/assets/javascripts/filtered_search/available_dropdown_mappings.js +++ b/app/assets/javascripts/filtered_search/available_dropdown_mappings.js @@ -5,6 +5,7 @@ import DropdownEmoji from './dropdown_emoji'; import NullDropdown from './null_dropdown'; import DropdownAjaxFilter from './dropdown_ajax_filter'; import DropdownUtils from './dropdown_utils'; +import { mergeUrlParams } from '../lib/utils/url_utility'; export default class AvailableDropdownMappings { constructor(container, baseEndpoint, groupsOnly, includeAncestorGroups, includeDescendantGroups) { @@ -13,6 +14,7 @@ export default class AvailableDropdownMappings { this.groupsOnly = groupsOnly; this.includeAncestorGroups = includeAncestorGroups; this.includeDescendantGroups = includeDescendantGroups; + this.filteredSearchInput = this.container.querySelector('.filtered-search'); } getAllowedMappings(supportedTokens) { @@ -102,6 +104,15 @@ export default class AvailableDropdownMappings { }, element: this.container.querySelector('#js-dropdown-runner-tag'), }, + 'target-branch': { + reference: null, + gl: DropdownNonUser, + extraArguments: { + endpoint: this.getMergeRequestTargetBranchesEndpoint(), + symbol: '', + }, + element: this.container.querySelector('#js-dropdown-target-branch'), + }, }; } @@ -130,4 +141,24 @@ export default class AvailableDropdownMappings { getRunnerTagsEndpoint() { return `${this.baseEndpoint}/admin/runners/tag_list.json`; } + + getMergeRequestTargetBranchesEndpoint() { + const endpoint = `${gon.relative_url_root || + ''}/autocomplete/merge_request_target_branches.json`; + + const params = { + group_id: this.getGroupId(), + project_id: this.getProjectId(), + }; + + return mergeUrlParams(params, endpoint); + } + + getGroupId() { + return this.filteredSearchInput.getAttribute('data-group-id') || ''; + } + + getProjectId() { + return this.filteredSearchInput.getAttribute('data-project-id') || ''; + } } -- GitLab From b0384d320841619b3ffdde963c598ae968506454 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Mon, 21 Jan 2019 22:29:23 +0900 Subject: [PATCH 04/10] Dynamically adding a class based on the token name --- .../add_extra_tokens_for_merge_requests.js | 12 +++++++++ .../filtered_search_dropdown.js | 5 +--- .../filtered_search_dropdown_manager.js | 7 +---- .../filtered_search_manager.js | 6 +---- .../filtered_search_token_keys.js | 5 ---- .../filtered_search_visual_tokens.js | 26 ++++++------------- app/assets/stylesheets/framework/filters.scss | 7 +++++ .../shared/issuable/_search_bar.html.haml | 4 +-- .../filtered_search_visual_tokens_spec.js | 4 +++ .../helpers/filtered_search_spec_helper.js | 2 +- 10 files changed, 37 insertions(+), 41 deletions(-) diff --git a/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js b/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js index 54ea936252e4e7..0b24d9fc920ce6 100644 --- a/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js +++ b/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js @@ -13,4 +13,16 @@ export default IssuableTokenKeys => { IssuableTokenKeys.tokenKeys.push(wipToken); IssuableTokenKeys.tokenKeysWithAlternative.push(wipToken); + + const targetBranchToken = { + key: 'target-branch', + type: 'string', + param: '', + symbol: '', + icon: 'arrow-right', + tag: 'branch', + }; + + IssuableTokenKeys.tokenKeys.push(targetBranchToken); + IssuableTokenKeys.tokenKeysWithAlternative.push(targetBranchToken); }; diff --git a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js index c90245849ad44e..146d3ba963c686 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js +++ b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js @@ -37,10 +37,7 @@ export default class FilteredSearchDropdown { if (!dataValueSet) { const value = getValueFunction(selected); - const tokenValueClass = selected.parentNode.getAttribute('data-token-value-class') || ''; - FilteredSearchDropdownManager.addWordToInput(this.filter, value, true, { - tokenValueClass, - }); + FilteredSearchDropdownManager.addWordToInput(this.filter, value, true); } this.resetFilters(); diff --git a/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js b/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js index cbfd01bf6d2d4d..cb0a84b490be50 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js +++ b/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js @@ -57,16 +57,11 @@ export default class FilteredSearchDropdownManager { } static addWordToInput(tokenName, tokenValue = '', clicked = false, options = {}) { - const { - uppercaseTokenName = false, - capitalizeTokenValue = false, - tokenValueClass = '', - } = options; + const { uppercaseTokenName = false, capitalizeTokenValue = false } = options; const input = FilteredSearchContainer.container.querySelector('.filtered-search'); FilteredSearchVisualTokens.addFilterVisualToken(tokenName, tokenValue, { uppercaseTokenName, capitalizeTokenValue, - tokenValueClass, }); input.value = ''; diff --git a/app/assets/javascripts/filtered_search/filtered_search_manager.js b/app/assets/javascripts/filtered_search/filtered_search_manager.js index cb5e228b645f3c..e23ad4b3651ef6 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_manager.js +++ b/app/assets/javascripts/filtered_search/filtered_search_manager.js @@ -437,7 +437,6 @@ export default class FilteredSearchManager { FilteredSearchVisualTokens.addFilterVisualToken(t.key, `${t.symbol}${t.value}`, { uppercaseTokenName: this.filteredSearchTokenKeys.shouldUppercaseTokenName(t.key), capitalizeTokenValue: this.filteredSearchTokenKeys.shouldCapitalizeTokenValue(t.key), - tokenValueClass: this.filteredSearchTokenKeys.getTokenValueClass(t.key), }); }); @@ -457,7 +456,6 @@ export default class FilteredSearchManager { FilteredSearchVisualTokens.addFilterVisualToken(tokenKey, null, { uppercaseTokenName: this.filteredSearchTokenKeys.shouldUppercaseTokenName(tokenKey), capitalizeTokenValue: this.filteredSearchTokenKeys.shouldCapitalizeTokenValue(tokenKey), - tokenValueClass: this.filteredSearchTokenKeys.getTokenValueClass(tokenKey), }); input.value = input.value.replace(`${tokenKey}:`, ''); } @@ -469,7 +467,6 @@ export default class FilteredSearchManager { const tokenKey = FilteredSearchVisualTokens.getLastTokenPartial(); FilteredSearchVisualTokens.addFilterVisualToken(searchToken, null, { capitalizeTokenValue: this.filteredSearchTokenKeys.shouldCapitalizeTokenValue(tokenKey), - tokenValueClass: this.filteredSearchTokenKeys.getTokenValueClass(tokenKey), }); // Trim the last space as seen in the if statement above @@ -540,7 +537,7 @@ export default class FilteredSearchManager { hasFilteredSearch = true; const canEdit = this.canEdit && this.canEdit(key, sanitizedValue); - const { uppercaseTokenName, capitalizeTokenValue, tokenValueClass } = match; + const { uppercaseTokenName, capitalizeTokenValue } = match; FilteredSearchVisualTokens.addFilterVisualToken( key, `${symbol}${quotationsToUse}${sanitizedValue}${quotationsToUse}`, @@ -548,7 +545,6 @@ export default class FilteredSearchManager { canEdit, uppercaseTokenName, capitalizeTokenValue, - tokenValueClass, }, ); } else if (!match && keyParam === 'assignee_id') { diff --git a/app/assets/javascripts/filtered_search/filtered_search_token_keys.js b/app/assets/javascripts/filtered_search/filtered_search_token_keys.js index 562f40787d7a53..11ed85504ec662 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_token_keys.js +++ b/app/assets/javascripts/filtered_search/filtered_search_token_keys.js @@ -33,11 +33,6 @@ export default class FilteredSearchTokenKeys { return token && token.capitalizeTokenValue; } - getTokenValueClass(tokenKey) { - const token = this.searchByKey(tokenKey.toLowerCase()); - return token && token.tokenValueClass; - } - searchByKey(key) { return this.tokenKeys.find(tokenKey => tokenKey.key === key) || null; } diff --git a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js index f201e693de9f86..315cd6f64da2a6 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js +++ b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js @@ -42,19 +42,13 @@ export default class FilteredSearchVisualTokens { } static createVisualTokenElementHTML(options = {}) { - const { - canEdit = true, - uppercaseTokenName = false, - capitalizeTokenValue = false, - tokenValueClass = '', - } = options; + const { canEdit = true, uppercaseTokenName = false, capitalizeTokenValue = false } = options; return `
-
+
@@ -80,18 +74,21 @@ export default class FilteredSearchVisualTokens { canEdit, uppercaseTokenName, capitalizeTokenValue, - tokenValueClass, + tokenClass = `search-token-${name.toLowerCase()}`, } = options; const li = document.createElement('li'); li.classList.add('js-visual-token'); li.classList.add(isSearchTerm ? 'filtered-search-term' : 'filtered-search-token'); + if (!isSearchTerm) { + li.classList.add(tokenClass); + } + if (value) { li.innerHTML = FilteredSearchVisualTokens.createVisualTokenElementHTML({ canEdit, uppercaseTokenName, capitalizeTokenValue, - tokenValueClass, }); FilteredSearchVisualTokens.renderVisualTokenValue(li, name, value); } else { @@ -121,12 +118,7 @@ export default class FilteredSearchVisualTokens { static addFilterVisualToken( tokenName, tokenValue, - { - canEdit, - uppercaseTokenName = false, - capitalizeTokenValue = false, - tokenValueClass = '', - } = {}, + { canEdit, uppercaseTokenName = false, capitalizeTokenValue = false } = {}, ) { const { lastVisualToken, @@ -139,7 +131,6 @@ export default class FilteredSearchVisualTokens { canEdit, uppercaseTokenName, capitalizeTokenValue, - tokenValueClass, }); } else { const previousTokenName = lastVisualToken.querySelector('.name').innerText; @@ -151,7 +142,6 @@ export default class FilteredSearchVisualTokens { canEdit, uppercaseTokenName, capitalizeTokenValue, - tokenValueClass, }); } } diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index e5b529ae11d50d..f2a1bd4541de34 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -412,3 +412,10 @@ padding: 8px 16px; text-align: center; } + +.search-token-target-branch { + .js-data-value, + .value { + font-family: $monospace-font; + } +} diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index 755f1d8c3be467..eeeb765195ca26 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -137,8 +137,8 @@ %li.filter-dropdown-item{ data: { value: 'no', capitalize: true } } %button.btn.btn-link{ type: 'button' } = _('No') - #js-dropdown-target-branch.filtered-search-input-dropdown-menu.dropdown-menu - %ul.filter-dropdown.ref-name{ data: { dynamic: true, dropdown: true, token_value_class: 'ref-name' } } + #js-dropdown-target-branch.filtered-search-input-dropdown-menu.dropdown-menu.search-token-target-branch + %ul.filter-dropdown{ data: { dynamic: true, dropdown: true } } %li.filter-dropdown-item %button.btn.btn-link.js-data-value {{title}} diff --git a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js index f3dc35552d5d9e..a72ea6ab5476e0 100644 --- a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js +++ b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js @@ -293,6 +293,7 @@ describe('Filtered Search Visual Tokens', () => { subject.addVisualTokenElement('milestone'); const token = tokensContainer.querySelector('.js-visual-token'); + expect(token.classList.contains('search-token-milestone')).toEqual(true); expect(token.classList.contains('filtered-search-token')).toEqual(true); expect(token.querySelector('.name').innerText).toEqual('milestone'); expect(token.querySelector('.value')).toEqual(null); @@ -302,6 +303,7 @@ describe('Filtered Search Visual Tokens', () => { subject.addVisualTokenElement('label', 'Frontend'); const token = tokensContainer.querySelector('.js-visual-token'); + expect(token.classList.contains('search-token-label')).toEqual(true); expect(token.classList.contains('filtered-search-token')).toEqual(true); expect(token.querySelector('.name').innerText).toEqual('label'); expect(token.querySelector('.value').innerText).toEqual('Frontend'); @@ -317,10 +319,12 @@ describe('Filtered Search Visual Tokens', () => { const labelToken = tokens[0]; const assigneeToken = tokens[1]; + expect(labelToken.classList.contains('search-token-label')).toEqual(true); expect(labelToken.classList.contains('filtered-search-token')).toEqual(true); expect(labelToken.querySelector('.name').innerText).toEqual('label'); expect(labelToken.querySelector('.value').innerText).toEqual('Frontend'); + expect(assigneeToken.classList.contains('search-token-assignee')).toEqual(true); expect(assigneeToken.classList.contains('filtered-search-token')).toEqual(true); expect(assigneeToken.querySelector('.name').innerText).toEqual('assignee'); expect(assigneeToken.querySelector('.value').innerText).toEqual('@root'); diff --git a/spec/javascripts/helpers/filtered_search_spec_helper.js b/spec/javascripts/helpers/filtered_search_spec_helper.js index 8933dd5def406f..fd06bb1f3244d3 100644 --- a/spec/javascripts/helpers/filtered_search_spec_helper.js +++ b/spec/javascripts/helpers/filtered_search_spec_helper.js @@ -5,7 +5,7 @@ export default class FilteredSearchSpecHelper { static createFilterVisualToken(name, value, isSelected = false) { const li = document.createElement('li'); - li.classList.add('js-visual-token', 'filtered-search-token'); + li.classList.add('js-visual-token', 'filtered-search-token', `search-token-${name}`); li.innerHTML = `
-- GitLab From 8383af25d5c9a6965a85077e90f001726795187f Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Thu, 24 Jan 2019 15:16:50 +0900 Subject: [PATCH 05/10] Use a proper method name --- app/controllers/autocomplete_controller.rb | 2 +- app/models/merge_request.rb | 2 +- spec/models/merge_request_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index d72c054ee8d750..79673a5aea3309 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -41,7 +41,7 @@ def award_emojis def merge_request_target_branches merge_requests = MergeRequestsFinder.new(current_user, params).execute - target_branches = merge_requests.recently_target_branches + target_branches = merge_requests.recent_target_branches render json: target_branches.map { |target_branch| { title: target_branch } } end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0dc8ae711598bb..1bfe2b0c93f042 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -213,7 +213,7 @@ def self.reference_prefix # ['master', 'develop', 'production'] # # limit - The maximum number of target branch to return. - def self.recently_target_branches(limit: 100) + def self.recent_target_branches(limit: 100) group(:target_branch) .select(:target_branch) .reorder('MAX(merge_requests.updated_at) DESC') diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 36d648d5940f4c..5e0be894218580 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -270,7 +270,7 @@ def create_merge_request_with_diffs(source_branch, diffs: 2) end end - describe '.recently_target_branches' do + describe '.recent_target_branches' do let(:project) { create(:project) } let!(:merge_request1) { create(:merge_request, :opened, source_project: project, target_branch: 'feature') } let!(:merge_request2) { create(:merge_request, :closed, source_project: project, target_branch: 'merge-test') } @@ -285,7 +285,7 @@ def create_merge_request_with_diffs(source_branch, diffs: 2) end it 'returns target branches sort by updated at desc' do - expect(described_class.recently_target_branches).to match_array(['feature', 'merge-test', 'fix']) + expect(described_class.recent_target_branches).to match_array(['feature', 'merge-test', 'fix']) end end -- GitLab From b3e3923a9eba92f302a1d6109877412f69e1613a Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 15 Feb 2019 18:29:32 +0900 Subject: [PATCH 06/10] Don't style classes with a js- prefix --- app/assets/stylesheets/framework/filters.scss | 1 - app/views/shared/issuable/_search_bar.html.haml | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index f2a1bd4541de34..6e24d367fec418 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -414,7 +414,6 @@ } .search-token-target-branch { - .js-data-value, .value { font-family: $monospace-font; } diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index eeeb765195ca26..2bcfcb6fa7c918 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -137,10 +137,10 @@ %li.filter-dropdown-item{ data: { value: 'no', capitalize: true } } %button.btn.btn-link{ type: 'button' } = _('No') - #js-dropdown-target-branch.filtered-search-input-dropdown-menu.dropdown-menu.search-token-target-branch + #js-dropdown-target-branch.filtered-search-input-dropdown-menu.dropdown-menu %ul.filter-dropdown{ data: { dynamic: true, dropdown: true } } %li.filter-dropdown-item - %button.btn.btn-link.js-data-value + %button.btn.btn-link.js-data-value.monospace {{title}} = render_if_exists 'shared/issuable/filter_weight', type: type -- GitLab From 2cf882427ceaa660e56778a763050e52beb51b10 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Thu, 28 Feb 2019 19:52:35 +0900 Subject: [PATCH 07/10] Fix the style of search token target branch --- app/assets/stylesheets/framework/filters.scss | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index 6e24d367fec418..9387bcb14694a7 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -121,8 +121,7 @@ .remove-token { display: inline-block; - padding-left: 4px; - padding-right: 0; + padding: 2px 0 2px 4px; .fa-close { color: $gl-text-color-secondary; @@ -416,5 +415,7 @@ .search-token-target-branch { .value { font-family: $monospace-font; + font-size: 13px; + line-height: 21px; } } -- GitLab From ce139cfe2742f2af32e3c2adf2bf0bb73cddd794 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 1 Mar 2019 19:57:18 +0900 Subject: [PATCH 08/10] Add spec --- .../user_filters_by_target_branch_spec.rb | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 spec/features/merge_requests/user_filters_by_target_branch_spec.rb diff --git a/spec/features/merge_requests/user_filters_by_target_branch_spec.rb b/spec/features/merge_requests/user_filters_by_target_branch_spec.rb new file mode 100644 index 00000000000000..ffbdacc68f6a77 --- /dev/null +++ b/spec/features/merge_requests/user_filters_by_target_branch_spec.rb @@ -0,0 +1,45 @@ +require 'rails_helper' + +describe 'Merge Requests > User filters by target branch', :js do + include FilteredSearchHelpers + + let!(:project) { create(:project, :public, :repository) } + let!(:user) { project.creator } + let!(:mr1) { create(:merge_request, source_project: project, target_project: project, source_branch: 'feature', target_branch: 'master') } + let!(:mr2) { create(:merge_request, source_project: project, target_project: project, source_branch: 'feature', target_branch: 'merged-target') } + + before do + sign_in(user) + visit project_merge_requests_path(project) + end + + context 'filtering by target-branch:master' do + it 'applies the filter' do + input_filtered_search('target-branch:master') + + expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) + expect(page).to have_content mr1.title + expect(page).not_to have_content mr2.title + end + end + + context 'filtering by target-branch:merged-target' do + it 'applies the filter' do + input_filtered_search('target-branch:merged-target') + + expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) + expect(page).not_to have_content mr1.title + expect(page).to have_content mr2.title + end + end + + context 'filtering by target-branch:feature' do + it 'applies the filter' do + input_filtered_search('target-branch:feature') + + expect(page).to have_issuable_counts(open: 0, closed: 0, all: 0) + expect(page).not_to have_content mr1.title + expect(page).not_to have_content mr2.title + end + end +end -- GitLab From cd57b406e4346d49b651a7a038276539121743ef Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Wed, 6 Mar 2019 14:36:38 +0900 Subject: [PATCH 09/10] Use flexbox to dynamically center the value and remove token --- app/assets/stylesheets/framework/filters.scss | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index 9387bcb14694a7..fac852c8cd6894 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -108,6 +108,8 @@ } .value-container { + display: flex; + align-items: center; background-color: $white-normal; color: $filter-value-text-color; border-radius: 0 2px 2px 0; @@ -121,7 +123,8 @@ .remove-token { display: inline-block; - padding: 2px 0 2px 4px; + padding-left: 8px; + padding-right: 0px; .fa-close { color: $gl-text-color-secondary; @@ -416,6 +419,5 @@ .value { font-family: $monospace-font; font-size: 13px; - line-height: 21px; } } -- GitLab From 23fbdb877a6a3507fbf94f6bd5c3e5de102b508b Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Wed, 6 Mar 2019 14:20:38 +0100 Subject: [PATCH 10/10] Fix stylelint issue. --- app/assets/stylesheets/framework/filters.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index fac852c8cd6894..5bcfd5d13226aa 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -124,7 +124,7 @@ .remove-token { display: inline-block; padding-left: 8px; - padding-right: 0px; + padding-right: 0; .fa-close { color: $gl-text-color-secondary; -- GitLab