From c0fc531999689a6d472612586c44693cadfc96dd Mon Sep 17 00:00:00 2001 From: Eulyeon Ko Date: Thu, 9 Dec 2021 19:52:00 +0900 Subject: [PATCH 1/6] Support iteration search by cadence title - Update the iterations GraphQL API to support iterations search using the attributes title or cadence title. Changelog: added EE: true --- app/models/concerns/timebox.rb | 11 --- app/models/milestone.rb | 11 +++ doc/api/graphql/reference/index.md | 27 ++++++- ee/app/finders/iterations_finder.rb | 26 +++++-- .../graphql/resolvers/iterations_resolver.rb | 42 +++++++++- .../types/iteration_searchable_field_enum.rb | 12 +++ ee/app/graphql/types/iteration_sort_enum.rb | 10 +++ ee/app/models/ee/iteration.rb | 19 +++++ ee/app/models/iterations/cadence.rb | 4 +- ee/lib/api/iterations.rb | 3 +- ee/spec/finders/iterations_finder_spec.rb | 63 +++++++++++++-- .../resolvers/iterations_resolver_spec.rb | 59 ++++++++++++-- ee/spec/models/ee/iteration_spec.rb | 76 +++++++++++++++++++ .../api/graphql/iterations/iterations_spec.rb | 41 ++++++++-- 14 files changed, 362 insertions(+), 42 deletions(-) create mode 100644 ee/app/graphql/types/iteration_searchable_field_enum.rb create mode 100644 ee/app/graphql/types/iteration_sort_enum.rb diff --git a/app/models/concerns/timebox.rb b/app/models/concerns/timebox.rb index 3fe9d7f4d7124e..c8ca71d03da8a3 100644 --- a/app/models/concerns/timebox.rb +++ b/app/models/concerns/timebox.rb @@ -125,17 +125,6 @@ def search(query) fuzzy_search(query, [:title, :description]) end - # Searches for timeboxes with a matching title. - # - # This method uses ILIKE on PostgreSQL - # - # query - The search query as a String - # - # Returns an ActiveRecord::Relation. - def search_title(query) - fuzzy_search(query, [:title]) - end - def filter_by_state(timeboxes, state) case state when 'closed' then timeboxes.closed diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 868bee9961b05a..2c95cc2672cce1 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -52,6 +52,17 @@ class Predefined state :active end + # Searches for timeboxes with a matching title. + # + # This method uses ILIKE on PostgreSQL + # + # query - The search query as a String + # + # Returns an ActiveRecord::Relation. + def self.search_title(query) + fuzzy_search(query, [:title]) + end + def self.min_chars_for_partial_matching 2 end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 1b1adff7993a9a..775bdb32a8386d 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -11062,12 +11062,15 @@ four standard [pagination arguments](#connection-pagination-arguments): | `endDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.end. | | `id` | [`ID`](#id) | Global ID of the Iteration to look up. | | `iid` | [`ID`](#id) | Internal ID of the Iteration to look up. | +| `in` | [`[IterationSearchableField!]`](#iterationsearchablefield) | Fields in which the fuzzy-search should be performed with the query given in the argument `search`. Defaults to `[title]`. | | `includeAncestors` | [`Boolean`](#boolean) | Whether to include ancestor iterations. Defaults to true. | | `iterationCadenceIds` | [`[IterationsCadenceID!]`](#iterationscadenceid) | Global iteration cadence IDs by which to look up the iterations. | +| `search` | [`String`](#string) | Query used for fuzzy-searching in the fields selected in the argument `in`. | +| `sort` | [`IterationSort`](#iterationsort) | List iterations by sort order. If unspecified, an arbitrary order (subject to change) is used. | | `startDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.start. | | `state` | [`IterationState`](#iterationstate) | Filter iterations by state. | | `timeframe` | [`Timeframe`](#timeframe) | List items overlapping the given timeframe. | -| `title` | [`String`](#string) | Fuzzy search by title. | +| `title` | [`String`](#string) | Fuzzy search by title. To be deprecated in 15.4. Please use `search` field. | ##### `Group.label` @@ -13759,12 +13762,15 @@ four standard [pagination arguments](#connection-pagination-arguments): | `endDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.end. | | `id` | [`ID`](#id) | Global ID of the Iteration to look up. | | `iid` | [`ID`](#id) | Internal ID of the Iteration to look up. | +| `in` | [`[IterationSearchableField!]`](#iterationsearchablefield) | Fields in which the fuzzy-search should be performed with the query given in the argument `search`. Defaults to `[title]`. | | `includeAncestors` | [`Boolean`](#boolean) | Whether to include ancestor iterations. Defaults to true. | | `iterationCadenceIds` | [`[IterationsCadenceID!]`](#iterationscadenceid) | Global iteration cadence IDs by which to look up the iterations. | +| `search` | [`String`](#string) | Query used for fuzzy-searching in the fields selected in the argument `in`. | +| `sort` | [`IterationSort`](#iterationsort) | List iterations by sort order. If unspecified, an arbitrary order (subject to change) is used. | | `startDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.start. | | `state` | [`IterationState`](#iterationstate) | Filter iterations by state. | | `timeframe` | [`Timeframe`](#timeframe) | List items overlapping the given timeframe. | -| `title` | [`String`](#string) | Fuzzy search by title. | +| `title` | [`String`](#string) | Fuzzy search by title. To be deprecated in 15.4. Please use `search` field. | ##### `Project.jobs` @@ -16997,6 +17003,23 @@ Issue type. | `REQUIREMENT` | Requirement issue type. | | `TEST_CASE` | Test Case issue type. | +### `IterationSearchableField` + +Fields to perform the search in. + +| Value | Description | +| ----- | ----------- | +| `CADENCE_TITLE` | Search in cadence_title field. | +| `TITLE` | Search in title field. | + +### `IterationSort` + +Iteration sort values. + +| Value | Description | +| ----- | ----------- | +| `CADENCE_AND_DUE_DATE_ASC` | Sort by cadence id and due date in ascending order. | + ### `IterationState` State of a GitLab iteration. diff --git a/ee/app/finders/iterations_finder.rb b/ee/app/finders/iterations_finder.rb index 2b98344574a1e7..7020d939809da1 100644 --- a/ee/app/finders/iterations_finder.rb +++ b/ee/app/finders/iterations_finder.rb @@ -5,14 +5,18 @@ # params - Hash # parent - The group in which to look-up iterations. # include_ancestors - whether to look-up iterations in group ancestors. -# order - Orders by field default due date asc. # title - Filter by title. +# search - Filter by fuzzy searching the given query in the selected fields. +# in - Array of searchable fields used with search param. # state - Filters by state. +# sort - Items are sorted by due_date and title with id as a tie breaker if unspecified. class IterationsFinder include FinderMethods include TimeFrameFilter + SEARCHABLE_FIELDS = %i(title cadence_title).freeze + attr_reader :params, :current_user def initialize(current_user, params = {}) @@ -30,7 +34,7 @@ def execute(skip_authorization: false) items = by_iid(items) items = by_groups(items) items = by_title(items) - items = by_search_title(items) + items = by_search(items) items = by_state(items) items = by_timeframe(items) items = by_iteration_cadences(items) @@ -72,10 +76,20 @@ def by_title(items) items.with_title(params[:title]) end - def by_search_title(items) - return items unless params[:search_title].present? + def by_search(items) + return items unless params[:search].present? && params[:in].present? + + query = params[:search] + in_title = params[:in].include?(:title) + in_cadence_title = params[:in].include?(:cadence_title) - items.search_title(params[:search_title]) + if in_title && in_cadence_title + items.search_title_or_cadence_title(query) + elsif in_title + items.search_title(query) + elsif in_cadence_title + items.search_cadence_title(query) + end end def by_state(items) @@ -96,6 +110,8 @@ def by_iteration_cadences(items) # rubocop: disable CodeReuse/ActiveRecord def order(items) + return items.sort_by_cadence_id_and_due_date_asc if params[:sort].present? && params[:sort] == :cadence_and_due_date_asc + items.reorder(:due_date).order(:title, { id: :asc }) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/ee/app/graphql/resolvers/iterations_resolver.rb b/ee/app/graphql/resolvers/iterations_resolver.rb index cf7381551173e4..a1c0ac4dd8329f 100644 --- a/ee/app/graphql/resolvers/iterations_resolver.rb +++ b/ee/app/graphql/resolvers/iterations_resolver.rb @@ -5,12 +5,22 @@ class IterationsResolver < BaseResolver include Gitlab::Graphql::Authorize::AuthorizeResource include TimeFrameArguments + DEFAULT_IN_FIELD = :title + argument :state, Types::IterationStateEnum, required: false, description: 'Filter iterations by state.' argument :title, GraphQL::Types::String, required: false, - description: 'Fuzzy search by title.' + description: 'Fuzzy search by title. To be deprecated in 15.4. Please use `search` field.' + + argument :search, GraphQL::Types::String, + required: false, + description: 'Query used for fuzzy-searching in the fields selected in the argument `in`.' + + argument :in, [Types::IterationSearchableFieldEnum], + required: false, + description: "Fields in which the fuzzy-search should be performed with the query given in the argument `search`. Defaults to `[#{DEFAULT_IN_FIELD}]`." # rubocop:disable Graphql/IDType argument :id, GraphQL::Types::ID, @@ -29,10 +39,15 @@ class IterationsResolver < BaseResolver required: false, description: 'Global iteration cadence IDs by which to look up the iterations.' + argument :sort, Types::IterationSortEnum, + required: false, + description: 'List iterations by sort order. If unspecified, an arbitrary order (subject to change) is used.' + type Types::IterationType.connection_type, null: true def resolve(**args) validate_timeframe_params!(args) + validate_search_params!(args) authorize! @@ -40,6 +55,8 @@ def resolve(**args) args[:iteration_cadence_ids] = parse_iteration_cadence_ids(args[:iteration_cadence_ids]) args[:include_ancestors] = true if args[:include_ancestors].nil? && args[:iid].nil? + handle_search_params!(args) + iterations = IterationsFinder.new(context[:current_user], iterations_finder_params(args)).execute # Necessary for scopedPath computation in IterationPresenter @@ -50,6 +67,25 @@ def resolve(**args) private + def validate_search_params!(args) + return unless args[:title].present? && (args[:search].present? || args[:in].present?) + + raise Gitlab::Graphql::Errors::ArgumentError, "'title' is deprecated in favor of 'search'. Please use 'search'." + end + + def handle_search_params!(args) + if args[:search].present? && !args[:in].present? + args[:in] = [DEFAULT_IN_FIELD] + return + end + + # Legacy support for `title` to be deprecated in 15.4 + if args[:title] + args[:search] = args[:title] + args[:in] = [DEFAULT_IN_FIELD] + end + end + def iterations_finder_params(args) { parent: parent, @@ -58,7 +94,9 @@ def iterations_finder_params(args) iid: args[:iid], iteration_cadence_ids: args[:iteration_cadence_ids], state: args[:state] || 'all', - search_title: args[:title] + search: args[:search], + in: args[:in], + sort: args[:sort] }.merge(transform_timeframe_parameters(args)) end diff --git a/ee/app/graphql/types/iteration_searchable_field_enum.rb b/ee/app/graphql/types/iteration_searchable_field_enum.rb new file mode 100644 index 00000000000000..714f2e81104ebb --- /dev/null +++ b/ee/app/graphql/types/iteration_searchable_field_enum.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Types + class IterationSearchableFieldEnum < BaseEnum + graphql_name 'IterationSearchableField' + description 'Fields to perform the search in' + + IterationsFinder::SEARCHABLE_FIELDS.each do |field| + value field.to_s.upcase, value: field, description: "Search in #{field} field." + end + end +end diff --git a/ee/app/graphql/types/iteration_sort_enum.rb b/ee/app/graphql/types/iteration_sort_enum.rb new file mode 100644 index 00000000000000..cd07cea19579c0 --- /dev/null +++ b/ee/app/graphql/types/iteration_sort_enum.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +module Types + class IterationSortEnum < BaseEnum + graphql_name 'IterationSort' + description 'Iteration sort values' + + value 'CADENCE_AND_DUE_DATE_ASC', 'Sort by cadence id and due date in ascending order.', value: :cadence_and_due_date_asc + end +end diff --git a/ee/app/models/ee/iteration.rb b/ee/app/models/ee/iteration.rb index ffa50321b23f5b..a919bd14a1c848 100644 --- a/ee/app/models/ee/iteration.rb +++ b/ee/app/models/ee/iteration.rb @@ -60,6 +60,7 @@ def self.by_id(id) after_commit :reset, on: [:update, :create], if: :saved_change_to_start_or_due_date? scope :due_date_order_asc, -> { order(:due_date) } + scope :sort_by_cadence_id_and_due_date_asc, -> { reorder(iterations_cadence_id: :asc).due_date_order_asc.order(id: :asc) } scope :upcoming, -> { with_state(:upcoming) } scope :current, -> { with_state(:current) } scope :closed, -> { with_state(:closed) } @@ -147,6 +148,24 @@ def filter_by_state(iterations, state) else raise ArgumentError, "Unknown state filter: #{state}" end end + + def search_title(query) + use_minimum_char_limit = !(query =~ / \d+ /).nil? + + fuzzy_search(query, [:title], use_minimum_char_limit: use_minimum_char_limit) + end + + def search_cadence_title(query) + cadence_ids = Iterations::Cadence.select(:id).search_title(query) + + where(iterations_cadence_id: cadence_ids) + end + + def search_title_or_cadence_title(query) + union_sql = ::Gitlab::SQL::Union.new([search_title(query), search_cadence_title(query)]).to_sql + + ::Iteration.from("(#{union_sql}) #{table_name}") + end end def display_text diff --git a/ee/app/models/iterations/cadence.rb b/ee/app/models/iterations/cadence.rb index f800441bbdba81..77bd616ca83c7b 100644 --- a/ee/app/models/iterations/cadence.rb +++ b/ee/app/models/iterations/cadence.rb @@ -37,7 +37,9 @@ class Cadence < ApplicationRecord end def self.search_title(query) - fuzzy_search(query, [:title]) + use_minimum_char_limit = !(query =~ / \d+ /).nil? + + fuzzy_search(query, [:title], use_minimum_char_limit: use_minimum_char_limit) end def next_open_iteration(date) diff --git a/ee/lib/api/iterations.rb b/ee/lib/api/iterations.rb index 15260e9ce6b692..ba79b404c0652a 100644 --- a/ee/lib/api/iterations.rb +++ b/ee/lib/api/iterations.rb @@ -27,7 +27,8 @@ def iterations_finder_params(parent) parent: parent, include_ancestors: params[:include_ancestors], state: params[:state], - search_title: params[:search] + search: params[:search], + in: params[:search] ? [:title] : [] } end end diff --git a/ee/spec/finders/iterations_finder_spec.rb b/ee/spec/finders/iterations_finder_spec.rb index a4329f221e9a09..a6bccdb29aeab6 100644 --- a/ee/spec/finders/iterations_finder_spec.rb +++ b/ee/spec/finders/iterations_finder_spec.rb @@ -10,11 +10,11 @@ let_it_be(:iteration_cadence1) { create(:iterations_cadence, group: group, active: true, duration_in_weeks: 1, title: 'one week iterations') } let_it_be(:iteration_cadence2) { create(:iterations_cadence, group: group, active: true, duration_in_weeks: 2, title: 'two week iterations') } let_it_be(:iteration_cadence3) { create(:iterations_cadence, group: root, active: true, duration_in_weeks: 3, title: 'three week iterations') } - let_it_be(:closed_iteration) { create(:closed_iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence2, group: iteration_cadence2.group, start_date: 7.days.ago, due_date: 2.days.ago) } - let_it_be(:started_group_iteration) { create(:current_iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence2, group: iteration_cadence2.group, title: 'one test', start_date: 1.day.ago, due_date: Date.today) } - let_it_be(:upcoming_group_iteration) { create(:iteration, iterations_cadence: iteration_cadence1, group: iteration_cadence1.group, start_date: 1.day.from_now, due_date: 3.days.from_now) } - let_it_be(:root_group_iteration) { create(:current_iteration, iterations_cadence: iteration_cadence3, group: iteration_cadence3.group, start_date: 1.day.ago, due_date: 2.days.from_now) } - let_it_be(:root_closed_iteration) { create(:closed_iteration, iterations_cadence: iteration_cadence3, group: iteration_cadence3.group, start_date: 1.week.ago, due_date: 2.days.ago) } + let_it_be(:closed_iteration) { create(:closed_iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence2, start_date: 7.days.ago, due_date: 2.days.ago) } + let_it_be(:started_group_iteration) { create(:current_iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence2, title: 'one test', start_date: 1.day.ago, due_date: Date.today) } + let_it_be(:upcoming_group_iteration) { create(:iteration, iterations_cadence: iteration_cadence1, title: 'Iteration 1', start_date: 1.day.from_now, due_date: 3.days.from_now) } + let_it_be(:root_group_iteration) { create(:current_iteration, iterations_cadence: iteration_cadence3, start_date: 1.day.ago, due_date: 2.days.from_now) } + let_it_be(:root_closed_iteration) { create(:closed_iteration, iterations_cadence: iteration_cadence3, start_date: 1.week.ago, due_date: 2.days.ago) } let(:parent) { project_1 } let(:params) { { parent: parent, include_ancestors: true } } @@ -132,10 +132,41 @@ expect(subject.to_a).to contain_exactly(started_group_iteration) end - it 'filters by search_title' do - params[:search_title] = 'one t' + context "with search params" do + using RSpec::Parameterized::TableSyntax - expect(subject.to_a).to contain_exactly(started_group_iteration) + shared_examples "search returns correct items" do + before do + params.merge!({ search: search, in: fields_to_search }) + end + + it { is_expected.to contain_exactly(*expected_iterations) } + end + + context 'filters by title' do + let(:all_iterations) { [closed_iteration, started_group_iteration, upcoming_group_iteration, root_group_iteration, root_closed_iteration] } + + where(:search, :fields_to_search, :expected_iterations) do + '' | [] | lazy { all_iterations } + 'iteration' | [] | lazy { all_iterations } + 'iteration' | [:title] | lazy { [upcoming_group_iteration] } + 'iteration' | [:title] | lazy { [upcoming_group_iteration] } + 'iter 1' | [:title] | lazy { [upcoming_group_iteration] } + 'iteration 1' | [:title] | lazy { [upcoming_group_iteration] } + 'iteration test' | [:title] | lazy { [] } + 'one week iter' | [:cadence_title] | lazy { [upcoming_group_iteration] } + 'iteration' | [:cadence_title] | lazy { all_iterations } + 'two week' | [:cadence_title] | lazy { [closed_iteration, started_group_iteration] } + 'iteration test' | [:cadence_title] | lazy { [] } + 'one week' | [:title, :cadence_title] | lazy { [upcoming_group_iteration] } + 'iteration' | [:title, :cadence_title] | lazy { all_iterations } + 'iteration 1' | [:title, :cadence_title] | lazy { [upcoming_group_iteration] } + end + + with_them do + it_behaves_like "search returns correct items" + end + end end it 'filters by ID' do @@ -205,6 +236,22 @@ end end end + + context 'sorting' do + it 'sorts by the default order (due_date, title, id asc) when no param is given' do + expect(subject).to eq([closed_iteration, root_closed_iteration, started_group_iteration, root_group_iteration, upcoming_group_iteration]) + end + + it 'sorts correctly when supported sorting param provided' do + params[:sort] = :cadence_and_due_date_asc + + cadence1_iterations = [upcoming_group_iteration] + cadence2_iterations = [closed_iteration, started_group_iteration] + cadence3_iterations = [root_closed_iteration, root_group_iteration] + + expect(subject).to eq([*cadence1_iterations, *cadence2_iterations, *cadence3_iterations]) + end + end end describe '#find_by' do diff --git a/ee/spec/graphql/resolvers/iterations_resolver_spec.rb b/ee/spec/graphql/resolvers/iterations_resolver_spec.rb index 532247bbc6905f..ba117dd0fe1bc5 100644 --- a/ee/spec/graphql/resolvers/iterations_resolver_spec.rb +++ b/ee/spec/graphql/resolvers/iterations_resolver_spec.rb @@ -15,7 +15,9 @@ iteration_cadence_ids: nil, parent: nil, state: nil, - search_title: nil + search: nil, + in: nil, + sort: nil } end @@ -50,6 +52,53 @@ def resolve_group_iterations(args = {}, obj = group, context = { current_user: c end context 'with parameters' do + context 'search' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:cadence1) { create(:iterations_cadence, title: 'plan cadence', group: group) } + let_it_be(:cadence2) { create(:iterations_cadence, title: 'project management', group: group) } + let_it_be(:iteration1) { create(:iteration, :with_due_date, title: "Iteration 1", iterations_cadence: cadence1, start_date: 1.week.ago)} + let_it_be(:iteration2) { create(:iteration, :with_due_date, title: "My iteration", iterations_cadence: cadence1, start_date: 2.weeks.ago)} + let_it_be(:iteration3) { create(:iteration, :with_due_date, iterations_cadence: cadence2, start_date: 1.week.from_now)} + + let(:all_iterations) { group.iterations } + + context 'with search and in parameters' do + where(:search, :fields_to_search, :expected_iterations) do + '' | [] | lazy { all_iterations } + 'iteration' | [] | lazy { cadence1.iterations } + 'iteration' | [:title] | lazy { cadence1.iterations } + 'iteration' | [:title, :cadence_title] | lazy { cadence1.iterations } + 'plan' | [] | lazy { [] } + 'plan' | [:cadence_title] | lazy { cadence1.iterations } + end + + with_them do + it "returns correct items" do + expect(resolve_group_iterations({ search: search, in: fields_to_search }).items).to contain_exactly(*expected_iterations) + end + end + end + + context "with the deprecated argument 'title' (to be deprecated in 15.4)" do + [ + { search: "foo" }, + { in: [:title] }, + { in: [:cadence_title] } + ].each do |params| + it "raises an error when 'title' is used with #{params}" do + expect do + resolve_group_iterations({ title: "foo", **params }) + end.to raise_error(Gitlab::Graphql::Errors::ArgumentError, "'title' is deprecated in favor of 'search'. Please use 'search'.") + end + end + + it "uses 'search' and 'in' arguments to search title" do + expect(resolve_group_iterations({ title: 'iteration' }).items).to contain_exactly(*cadence1.iterations) + end + end + end + it 'calls IterationsFinder with correct parameters, using timeframe' do start_date = now end_date = start_date + 1.hour @@ -58,11 +107,11 @@ def resolve_group_iterations(args = {}, obj = group, context = { current_user: c iid = 2 iteration_cadence_ids = ['5'] - params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search_title: search) + params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search: search, in: [:title]) expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original - resolve_group_iterations(timeframe: { start: start_date, end: end_date }, state: 'closed', title: search, id: 'gid://gitlab/Iteration/1', iteration_cadence_ids: ['gid://gitlab/Iterations::Cadence/5'], iid: iid) + resolve_group_iterations(timeframe: { start: start_date, end: end_date }, state: 'closed', search: search, id: 'gid://gitlab/Iteration/1', iteration_cadence_ids: ['gid://gitlab/Iterations::Cadence/5'], iid: iid) end it 'calls IterationsFinder with correct parameters, using start and end date' do @@ -73,11 +122,11 @@ def resolve_group_iterations(args = {}, obj = group, context = { current_user: c iid = 2 iteration_cadence_ids = ['5'] - params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search_title: search) + params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search: search, in: [:title]) expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original - resolve_group_iterations(start_date: start_date, end_date: end_date, state: 'closed', title: search, id: 'gid://gitlab/Iteration/1', iteration_cadence_ids: ['gid://gitlab/Iterations::Cadence/5'], iid: iid) + resolve_group_iterations(start_date: start_date, end_date: end_date, state: 'closed', search: search, id: 'gid://gitlab/Iteration/1', iteration_cadence_ids: ['gid://gitlab/Iterations::Cadence/5'], iid: iid) end it 'accepts a raw model id for backward compatibility' do diff --git a/ee/spec/models/ee/iteration_spec.rb b/ee/spec/models/ee/iteration_spec.rb index d5c3bf566811e0..59ff645248422b 100644 --- a/ee/spec/models/ee/iteration_spec.rb +++ b/ee/spec/models/ee/iteration_spec.rb @@ -538,6 +538,82 @@ end end + context 'search and sorting scopes' do + let_it_be(:group1) { create(:group) } + let_it_be(:group2) { create(:group) } + let_it_be(:subgroup) { create(:group, parent: group1) } + let_it_be(:cadence1) { create(:iterations_cadence, title: 'plan cadence', group: group1) } + let_it_be(:cadence2) { create(:iterations_cadence, title: 'project management', group: subgroup) } + let_it_be(:cadence3) { create(:iterations_cadence, title: 'cadence', group: group2) } + let_it_be(:iteration1) { create(:iteration, :with_due_date, title: "Iteration 1", iterations_cadence: cadence1, start_date: 1.week.ago)} + let_it_be(:iteration2) { create(:iteration, :with_due_date, title: "My iteration", iterations_cadence: cadence1, start_date: 2.weeks.ago)} + let_it_be(:iteration3) { create(:iteration, :with_due_date, title: "Iteration 2", iterations_cadence: cadence2, start_date: 1.week.from_now)} + let_it_be(:iteration4) { create(:iteration, :with_due_date, iterations_cadence: cadence3, start_date: Date.today)} + + shared_examples "search returns correct records" do + it { is_expected.to contain_exactly(*expected_iterations) } + end + + describe '.search_title' do + where(:query, :expected_iterations) do + 'iter 1' | lazy { [iteration1] } + 'iteration' | lazy { [iteration1, iteration2, iteration3] } + 'iteration 1' | lazy { [iteration1] } + 'my iteration 1' | lazy { [] } + end + + with_them do + subject { described_class.search_title(query) } + + it_behaves_like "search returns correct records" + end + end + + describe '.search_cadence_title' do + where(:query, :expected_iterations) do + 'plan' | lazy { [iteration1, iteration2] } + 'plan cadence' | lazy { [iteration1, iteration2] } + 'project cadence' | lazy { [] } + 'cadence' | lazy { [iteration1, iteration2, iteration4] } + end + + with_them do + subject { described_class.search_cadence_title(query) } + + it_behaves_like "search returns correct records" + end + end + + describe '.search_title_or_cadence_title' do + where(:query, :expected_iterations) do + # The smae test cases used for .search_title + 'iter 1' | lazy { [iteration1] } + 'iteration' | lazy { [iteration1, iteration2, iteration3] } + 'iteration 1' | lazy { [iteration1] } + 'my iteration 1' | lazy { [] } + # The same test cases used for .search_cadence_title + 'plan' | lazy { [iteration1, iteration2] } + 'plan cadence' | lazy { [iteration1, iteration2] } + 'project cadence' | lazy { [] } + 'cadence' | lazy { [iteration1, iteration2, iteration4] } + # At least one of cadence title or iteration title should contain all of the terms + 'plan iteration' | lazy { [] } + end + + with_them do + subject { described_class.search_title_or_cadence_title(query) } + + it_behaves_like "search returns correct records" + end + end + + describe '.sort_by_cadence_id_and_due_date_asc' do + subject { described_class.all.sort_by_cadence_id_and_due_date_asc } + + it { is_expected.to eq([iteration2, iteration1, iteration3, iteration4]) } + end + end + context 'time scopes' do let_it_be(:group) { create(:group) } let_it_be(:iteration_cadence) { create(:iterations_cadence, group: group) } diff --git a/ee/spec/requests/api/graphql/iterations/iterations_spec.rb b/ee/spec/requests/api/graphql/iterations/iterations_spec.rb index 48e92a916ca932..50a910ab9fc5c0 100644 --- a/ee/spec/requests/api/graphql/iterations/iterations_spec.rb +++ b/ee/spec/requests/api/graphql/iterations/iterations_spec.rb @@ -11,9 +11,9 @@ let_it_be(:iteration_cadence1) { create(:iterations_cadence, group: group, active: true, duration_in_weeks: 1, title: 'one week iterations') } let_it_be(:iteration_cadence2) { create(:iterations_cadence, group: group, active: true, duration_in_weeks: 2, title: 'two week iterations') } - let_it_be(:current_group_iteration) { create(:iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence1, group: iteration_cadence1.group, title: 'one test', start_date: 1.day.ago, due_date: 1.week.from_now) } - let_it_be(:upcoming_group_iteration) { create(:iteration, iterations_cadence: iteration_cadence2, group: iteration_cadence2.group, start_date: 1.day.from_now, due_date: 2.days.from_now) } - let_it_be(:closed_group_iteration) { create(:iteration, :skip_project_validation, iterations_cadence: iteration_cadence1, group: iteration_cadence1.group, start_date: 3.weeks.ago, due_date: 1.week.ago) } + let_it_be(:current_group_iteration) { create(:iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence1, title: 'one test', start_date: 1.day.ago, due_date: 1.week.from_now) } + let_it_be(:upcoming_group_iteration) { create(:iteration, iterations_cadence: iteration_cadence2, start_date: 1.day.from_now, due_date: 2.days.from_now) } + let_it_be(:closed_group_iteration) { create(:iteration, :skip_project_validation, iterations_cadence: iteration_cadence1, start_date: 3.weeks.ago, due_date: 1.week.ago) } before do group.add_maintainer(user) @@ -47,6 +47,28 @@ describe 'query for iterations by cadence' do context 'with multiple cadences' do + context 'searching by cadence title or iteration title and sorting by cadence and due date ASC' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:past_iteration1) { create(:iteration, :with_due_date, iterations_cadence: iteration_cadence2, start_date: 4.weeks.ago) } + let_it_be(:past_iteration2) { create(:iteration, :with_due_date, iterations_cadence: iteration_cadence2, start_date: 2.weeks.ago) } + + where(:search, :ordered_expected_iterations) do + 'two' | lazy { [past_iteration1, past_iteration2, upcoming_group_iteration] } + 'iteration' | lazy { [closed_group_iteration, current_group_iteration, past_iteration1, past_iteration2, upcoming_group_iteration] } + end + + with_them do + let(:field_queries) { "search: \"#{search}\", in: [TITLE, CADENCE_TITLE], sort: CADENCE_AND_DUE_DATE_ASC" } + + it 'correctly returns ordered items' do + post_graphql(iterations_query(group, field_queries), current_user: user) + + expect(actual_iterations).to eq(expected_iterations(ordered_expected_iterations)) + end + end + end + it 'returns iterations' do post_graphql(iteration_cadence_query(group, [iteration_cadence1.to_global_id, iteration_cadence2.to_global_id]), current_user: user) @@ -103,11 +125,16 @@ def iterations_query(group, field_queries) QUERY end - def expect_iterations_response(*iterations) - actual_iterations = graphql_data['group']['iterations']['nodes'].map { |iteration| iteration['id'] } - expected_iterations = iterations.map { |iteration| iteration.to_global_id.to_s } + def actual_iterations + graphql_data['group']['iterations']['nodes'].map { |iteration| iteration['id'] } + end - expect(actual_iterations).to contain_exactly(*expected_iterations) + def expected_iterations(iterations) + iterations.map { |iteration| iteration.to_global_id.to_s } + end + + def expect_iterations_response(*iterations) + expect(actual_iterations).to contain_exactly(*expected_iterations(iterations)) expect(graphql_errors).to be_nil end end -- GitLab From 6fca128708cb07ad0bdcead64abdf008d055771a Mon Sep 17 00:00:00 2001 From: Eulyeon Ko Date: Fri, 17 Dec 2021 16:22:55 +0900 Subject: [PATCH 2/6] Remove hardcoded enum values from specs --- ee/spec/workers/iterations_update_status_worker_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/workers/iterations_update_status_worker_spec.rb b/ee/spec/workers/iterations_update_status_worker_spec.rb index 6f67e2ce692404..c2f7fee5a738eb 100644 --- a/ee/spec/workers/iterations_update_status_worker_spec.rb +++ b/ee/spec/workers/iterations_update_status_worker_spec.rb @@ -12,8 +12,8 @@ describe '#perform' do before do - current_iteration1.update_column(:state_enum, 2) - closed_iteration1.update_column(:state_enum, 1) + current_iteration1.update_column(:state_enum, Iteration::STATE_ENUM_MAP[:current]) + closed_iteration1.update_column(:state_enum, Iteration::STATE_ENUM_MAP[:upcoming]) end it 'schedules an issues roll-over job' do -- GitLab From c6cfc9d114bdb9acbb6c853a13c1e2820d501470 Mon Sep 17 00:00:00 2001 From: Eulyeon Ko Date: Mon, 24 Jan 2022 20:46:21 +0900 Subject: [PATCH 3/6] Apply backend reviewer suggestions --- ee/app/models/ee/iteration.rb | 10 ++-- ee/app/models/iterations/cadence.rb | 8 ++-- ee/lib/api/iterations.rb | 15 +++++- .../resolvers/iterations_resolver_spec.rb | 20 ++++---- ee/spec/models/ee/iteration_spec.rb | 46 +++++++++---------- 5 files changed, 58 insertions(+), 41 deletions(-) diff --git a/ee/app/models/ee/iteration.rb b/ee/app/models/ee/iteration.rb index a919bd14a1c848..3713e5ab8680c3 100644 --- a/ee/app/models/ee/iteration.rb +++ b/ee/app/models/ee/iteration.rb @@ -150,9 +150,7 @@ def filter_by_state(iterations, state) end def search_title(query) - use_minimum_char_limit = !(query =~ / \d+ /).nil? - - fuzzy_search(query, [:title], use_minimum_char_limit: use_minimum_char_limit) + fuzzy_search(query, [:title], use_minimum_char_limit: contains_digits?(query)) end def search_cadence_title(query) @@ -166,6 +164,12 @@ def search_title_or_cadence_title(query) ::Iteration.from("(#{union_sql}) #{table_name}") end + + private + + def contains_digits?(query) + !(query =~ / \d+ /).nil? + end end def display_text diff --git a/ee/app/models/iterations/cadence.rb b/ee/app/models/iterations/cadence.rb index 77bd616ca83c7b..695d8b0cccc1af 100644 --- a/ee/app/models/iterations/cadence.rb +++ b/ee/app/models/iterations/cadence.rb @@ -37,9 +37,7 @@ class Cadence < ApplicationRecord end def self.search_title(query) - use_minimum_char_limit = !(query =~ / \d+ /).nil? - - fuzzy_search(query, [:title], use_minimum_char_limit: use_minimum_char_limit) + fuzzy_search(query, [:title], use_minimum_char_limit: contains_digit?(query)) end def next_open_iteration(date) @@ -78,5 +76,9 @@ def update_iteration_sequences WHERE t.id=sprints.id AND (sprints.sequence IS DISTINCT FROM t.row_number) SQL end + + def self.contains_digit?(query) + !(query =~ / \d+ /).nil? + end end end diff --git a/ee/lib/api/iterations.rb b/ee/lib/api/iterations.rb index ba79b404c0652a..5ca9e8f2f091f0 100644 --- a/ee/lib/api/iterations.rb +++ b/ee/lib/api/iterations.rb @@ -23,12 +23,23 @@ def list_iterations_for(parent) end def iterations_finder_params(parent) - { + finder_params = { parent: parent, include_ancestors: params[:include_ancestors], state: params[:state], + search: nil, + in: nil + } + + finder_params.merge!(search_params) if params[:search] + + finder_params + end + + def search_params + { search: params[:search], - in: params[:search] ? [:title] : [] + in: [:title] } end end diff --git a/ee/spec/graphql/resolvers/iterations_resolver_spec.rb b/ee/spec/graphql/resolvers/iterations_resolver_spec.rb index ba117dd0fe1bc5..1414b95ea25070 100644 --- a/ee/spec/graphql/resolvers/iterations_resolver_spec.rb +++ b/ee/spec/graphql/resolvers/iterations_resolver_spec.rb @@ -55,22 +55,22 @@ def resolve_group_iterations(args = {}, obj = group, context = { current_user: c context 'search' do using RSpec::Parameterized::TableSyntax - let_it_be(:cadence1) { create(:iterations_cadence, title: 'plan cadence', group: group) } - let_it_be(:cadence2) { create(:iterations_cadence, title: 'project management', group: group) } - let_it_be(:iteration1) { create(:iteration, :with_due_date, title: "Iteration 1", iterations_cadence: cadence1, start_date: 1.week.ago)} - let_it_be(:iteration2) { create(:iteration, :with_due_date, title: "My iteration", iterations_cadence: cadence1, start_date: 2.weeks.ago)} - let_it_be(:iteration3) { create(:iteration, :with_due_date, iterations_cadence: cadence2, start_date: 1.week.from_now)} + let_it_be(:plan_cadence) { create(:iterations_cadence, title: 'plan cadence', group: group) } + let_it_be(:product_cadence) { create(:iterations_cadence, title: 'product management', group: group) } + let_it_be(:plan_iteration1) { create(:iteration, :with_due_date, title: "Iteration 1", iterations_cadence: plan_cadence, start_date: 1.week.ago)} + let_it_be(:plan_iteration2) { create(:iteration, :with_due_date, title: "My iteration", iterations_cadence: plan_cadence, start_date: 2.weeks.ago)} + let_it_be(:product_iteration) { create(:iteration, :with_due_date, iterations_cadence: product_cadence, start_date: 1.week.from_now)} let(:all_iterations) { group.iterations } context 'with search and in parameters' do where(:search, :fields_to_search, :expected_iterations) do '' | [] | lazy { all_iterations } - 'iteration' | [] | lazy { cadence1.iterations } - 'iteration' | [:title] | lazy { cadence1.iterations } - 'iteration' | [:title, :cadence_title] | lazy { cadence1.iterations } + 'iteration' | [] | lazy { plan_cadence.iterations } + 'iteration' | [:title] | lazy { plan_cadence.iterations } + 'iteration' | [:title, :cadence_title] | lazy { plan_cadence.iterations } 'plan' | [] | lazy { [] } - 'plan' | [:cadence_title] | lazy { cadence1.iterations } + 'plan' | [:cadence_title] | lazy { plan_cadence.iterations } end with_them do @@ -94,7 +94,7 @@ def resolve_group_iterations(args = {}, obj = group, context = { current_user: c end it "uses 'search' and 'in' arguments to search title" do - expect(resolve_group_iterations({ title: 'iteration' }).items).to contain_exactly(*cadence1.iterations) + expect(resolve_group_iterations({ title: 'iteration' }).items).to contain_exactly(*plan_cadence.iterations) end end end diff --git a/ee/spec/models/ee/iteration_spec.rb b/ee/spec/models/ee/iteration_spec.rb index 59ff645248422b..6d4df67fe7a71a 100644 --- a/ee/spec/models/ee/iteration_spec.rb +++ b/ee/spec/models/ee/iteration_spec.rb @@ -542,13 +542,13 @@ let_it_be(:group1) { create(:group) } let_it_be(:group2) { create(:group) } let_it_be(:subgroup) { create(:group, parent: group1) } - let_it_be(:cadence1) { create(:iterations_cadence, title: 'plan cadence', group: group1) } - let_it_be(:cadence2) { create(:iterations_cadence, title: 'project management', group: subgroup) } - let_it_be(:cadence3) { create(:iterations_cadence, title: 'cadence', group: group2) } - let_it_be(:iteration1) { create(:iteration, :with_due_date, title: "Iteration 1", iterations_cadence: cadence1, start_date: 1.week.ago)} - let_it_be(:iteration2) { create(:iteration, :with_due_date, title: "My iteration", iterations_cadence: cadence1, start_date: 2.weeks.ago)} - let_it_be(:iteration3) { create(:iteration, :with_due_date, title: "Iteration 2", iterations_cadence: cadence2, start_date: 1.week.from_now)} - let_it_be(:iteration4) { create(:iteration, :with_due_date, iterations_cadence: cadence3, start_date: Date.today)} + let_it_be(:plan_cadence) { create(:iterations_cadence, title: 'plan cadence', group: group1) } + let_it_be(:product_cadence) { create(:iterations_cadence, title: 'product management', group: subgroup) } + let_it_be(:cadence) { create(:iterations_cadence, title: 'cadence', group: group2) } + let_it_be(:plan_iteration1) { create(:iteration, :with_due_date, title: "Iteration 1", iterations_cadence: plan_cadence, start_date: 1.week.ago)} + let_it_be(:plan_iteration2) { create(:iteration, :with_due_date, title: "My iteration", iterations_cadence: plan_cadence, start_date: 2.weeks.ago)} + let_it_be(:product_iteration) { create(:iteration, :with_due_date, title: "Iteration 2", iterations_cadence: product_cadence, start_date: 1.week.from_now)} + let_it_be(:cadence_iteration) { create(:iteration, :with_due_date, iterations_cadence: cadence, start_date: Date.today)} shared_examples "search returns correct records" do it { is_expected.to contain_exactly(*expected_iterations) } @@ -556,9 +556,9 @@ describe '.search_title' do where(:query, :expected_iterations) do - 'iter 1' | lazy { [iteration1] } - 'iteration' | lazy { [iteration1, iteration2, iteration3] } - 'iteration 1' | lazy { [iteration1] } + 'iter 1' | lazy { [plan_iteration1] } + 'iteration' | lazy { [plan_iteration1, plan_iteration2, product_iteration] } + 'iteration 1' | lazy { [plan_iteration1] } 'my iteration 1' | lazy { [] } end @@ -571,10 +571,10 @@ describe '.search_cadence_title' do where(:query, :expected_iterations) do - 'plan' | lazy { [iteration1, iteration2] } - 'plan cadence' | lazy { [iteration1, iteration2] } - 'project cadence' | lazy { [] } - 'cadence' | lazy { [iteration1, iteration2, iteration4] } + 'plan' | lazy { [plan_iteration1, plan_iteration2] } + 'plan cadence' | lazy { [plan_iteration1, plan_iteration2] } + 'product cadence' | lazy { [] } + 'cadence' | lazy { [plan_iteration1, plan_iteration2, cadence_iteration] } end with_them do @@ -586,16 +586,16 @@ describe '.search_title_or_cadence_title' do where(:query, :expected_iterations) do - # The smae test cases used for .search_title - 'iter 1' | lazy { [iteration1] } - 'iteration' | lazy { [iteration1, iteration2, iteration3] } - 'iteration 1' | lazy { [iteration1] } + # The same test cases used for .search_title + 'iter 1' | lazy { [plan_iteration1] } + 'iteration' | lazy { [plan_iteration1, plan_iteration2, product_iteration] } + 'iteration 1' | lazy { [plan_iteration1] } 'my iteration 1' | lazy { [] } # The same test cases used for .search_cadence_title - 'plan' | lazy { [iteration1, iteration2] } - 'plan cadence' | lazy { [iteration1, iteration2] } - 'project cadence' | lazy { [] } - 'cadence' | lazy { [iteration1, iteration2, iteration4] } + 'plan' | lazy { [plan_iteration1, plan_iteration2] } + 'plan cadence' | lazy { [plan_iteration1, plan_iteration2] } + 'product cadence' | lazy { [] } + 'cadence' | lazy { [plan_iteration1, plan_iteration2, cadence_iteration] } # At least one of cadence title or iteration title should contain all of the terms 'plan iteration' | lazy { [] } end @@ -610,7 +610,7 @@ describe '.sort_by_cadence_id_and_due_date_asc' do subject { described_class.all.sort_by_cadence_id_and_due_date_asc } - it { is_expected.to eq([iteration2, iteration1, iteration3, iteration4]) } + it { is_expected.to eq([plan_iteration2, plan_iteration1, product_iteration, cadence_iteration]) } end end -- GitLab From ae4a430055a08e14bdbfbd71328e2ff42742392d Mon Sep 17 00:00:00 2001 From: Albert Salim Date: Tue, 25 Jan 2022 01:33:23 +0000 Subject: [PATCH 4/6] Apply 1 suggestion(s) to 1 file(s) --- ee/app/models/ee/iteration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/iteration.rb b/ee/app/models/ee/iteration.rb index 3713e5ab8680c3..28900b786da0a1 100644 --- a/ee/app/models/ee/iteration.rb +++ b/ee/app/models/ee/iteration.rb @@ -154,7 +154,7 @@ def search_title(query) end def search_cadence_title(query) - cadence_ids = Iterations::Cadence.select(:id).search_title(query) + cadence_ids = Iterations::Cadence.search_title(query).pluck(:id) where(iterations_cadence_id: cadence_ids) end -- GitLab From 7760023cb1239d21d1846fc4641184c9a8582e92 Mon Sep 17 00:00:00 2001 From: Eulyeon Ko Date: Tue, 25 Jan 2022 13:41:12 +0900 Subject: [PATCH 5/6] Use deprecated for title argument --- doc/api/graphql/reference/index.md | 4 ++-- ee/app/graphql/resolvers/iterations_resolver.rb | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 775bdb32a8386d..bb338e78a37449 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -11070,7 +11070,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | `startDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.start. | | `state` | [`IterationState`](#iterationstate) | Filter iterations by state. | | `timeframe` | [`Timeframe`](#timeframe) | List items overlapping the given timeframe. | -| `title` | [`String`](#string) | Fuzzy search by title. To be deprecated in 15.4. Please use `search` field. | +| `title` **{warning-solid}** | [`String`](#string) | **Deprecated** in 15.4. The argument will be removed in 15.4. Please use `search` and `in` fields instead. | ##### `Group.label` @@ -13770,7 +13770,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | `startDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.start. | | `state` | [`IterationState`](#iterationstate) | Filter iterations by state. | | `timeframe` | [`Timeframe`](#timeframe) | List items overlapping the given timeframe. | -| `title` | [`String`](#string) | Fuzzy search by title. To be deprecated in 15.4. Please use `search` field. | +| `title` **{warning-solid}** | [`String`](#string) | **Deprecated** in 15.4. The argument will be removed in 15.4. Please use `search` and `in` fields instead. | ##### `Project.jobs` diff --git a/ee/app/graphql/resolvers/iterations_resolver.rb b/ee/app/graphql/resolvers/iterations_resolver.rb index a1c0ac4dd8329f..c066c80e30255c 100644 --- a/ee/app/graphql/resolvers/iterations_resolver.rb +++ b/ee/app/graphql/resolvers/iterations_resolver.rb @@ -12,7 +12,8 @@ class IterationsResolver < BaseResolver description: 'Filter iterations by state.' argument :title, GraphQL::Types::String, required: false, - description: 'Fuzzy search by title. To be deprecated in 15.4. Please use `search` field.' + description: 'Fuzzy search by title.', + deprecated: { reason: 'The argument will be removed in 15.4. Please use `search` and `in` fields instead', milestone: '15.4' } argument :search, GraphQL::Types::String, required: false, -- GitLab From 4884d8e2d81d9b90acdfe8210c860843d9bd4976 Mon Sep 17 00:00:00 2001 From: Eulyeon Ko Date: Tue, 25 Jan 2022 22:37:06 +0900 Subject: [PATCH 6/6] Apply backend maintainer suggestions --- .../graphql/resolvers/iterations_resolver.rb | 20 +++++++++---------- ee/app/models/ee/iteration.rb | 2 +- ee/app/models/iterations/cadence.rb | 16 +++++++++------ ee/lib/api/iterations.rb | 2 +- .../resolvers/iterations_resolver_spec.rb | 7 +++++++ 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/ee/app/graphql/resolvers/iterations_resolver.rb b/ee/app/graphql/resolvers/iterations_resolver.rb index c066c80e30255c..0b099770f325e2 100644 --- a/ee/app/graphql/resolvers/iterations_resolver.rb +++ b/ee/app/graphql/resolvers/iterations_resolver.rb @@ -69,22 +69,20 @@ def resolve(**args) private def validate_search_params!(args) - return unless args[:title].present? && (args[:search].present? || args[:in].present?) + if args[:title].present? && (args[:search].present? || args[:in].present?) + raise Gitlab::Graphql::Errors::ArgumentError, "'title' is deprecated in favor of 'search'. Please use 'search'." + end - raise Gitlab::Graphql::Errors::ArgumentError, "'title' is deprecated in favor of 'search'. Please use 'search'." + if !args[:search].present? && args[:in].present? + raise Gitlab::Graphql::Errors::ArgumentError, "'search' must be specified when using 'in' argument." + end end def handle_search_params!(args) - if args[:search].present? && !args[:in].present? - args[:in] = [DEFAULT_IN_FIELD] - return - end + return unless args[:search] || args[:title] - # Legacy support for `title` to be deprecated in 15.4 - if args[:title] - args[:search] = args[:title] - args[:in] = [DEFAULT_IN_FIELD] - end + args[:in] = [DEFAULT_IN_FIELD] if args[:in].nil? || args[:in].empty? + args[:search] = args[:title] if args[:title] end def iterations_finder_params(args) diff --git a/ee/app/models/ee/iteration.rb b/ee/app/models/ee/iteration.rb index 28900b786da0a1..b8d2a66880c7df 100644 --- a/ee/app/models/ee/iteration.rb +++ b/ee/app/models/ee/iteration.rb @@ -150,7 +150,7 @@ def filter_by_state(iterations, state) end def search_title(query) - fuzzy_search(query, [:title], use_minimum_char_limit: contains_digits?(query)) + fuzzy_search(query, [::Resolvers::IterationsResolver::DEFAULT_IN_FIELD], use_minimum_char_limit: contains_digits?(query)) end def search_cadence_title(query) diff --git a/ee/app/models/iterations/cadence.rb b/ee/app/models/iterations/cadence.rb index 695d8b0cccc1af..72aebc96d7687b 100644 --- a/ee/app/models/iterations/cadence.rb +++ b/ee/app/models/iterations/cadence.rb @@ -36,8 +36,16 @@ class Cadence < ApplicationRecord .where("DATE ((COALESCE(iterations_cadences.last_run_date, DATE('01-01-1970')) + iterations_cadences.duration_in_weeks * INTERVAL '1 week')) <= CURRENT_DATE") end - def self.search_title(query) - fuzzy_search(query, [:title], use_minimum_char_limit: contains_digit?(query)) + class << self + def search_title(query) + fuzzy_search(query, [::Resolvers::IterationsResolver::DEFAULT_IN_FIELD], use_minimum_char_limit: contains_digit?(query)) + end + + private + + def contains_digit?(query) + !(query =~ / \d+ /).nil? + end end def next_open_iteration(date) @@ -76,9 +84,5 @@ def update_iteration_sequences WHERE t.id=sprints.id AND (sprints.sequence IS DISTINCT FROM t.row_number) SQL end - - def self.contains_digit?(query) - !(query =~ / \d+ /).nil? - end end end diff --git a/ee/lib/api/iterations.rb b/ee/lib/api/iterations.rb index 5ca9e8f2f091f0..dbc0023b622955 100644 --- a/ee/lib/api/iterations.rb +++ b/ee/lib/api/iterations.rb @@ -39,7 +39,7 @@ def iterations_finder_params(parent) def search_params { search: params[:search], - in: [:title] + in: [::Resolvers::IterationsResolver::DEFAULT_IN_FIELD] } end end diff --git a/ee/spec/graphql/resolvers/iterations_resolver_spec.rb b/ee/spec/graphql/resolvers/iterations_resolver_spec.rb index 1414b95ea25070..c1628619d9cc46 100644 --- a/ee/spec/graphql/resolvers/iterations_resolver_spec.rb +++ b/ee/spec/graphql/resolvers/iterations_resolver_spec.rb @@ -66,6 +66,7 @@ def resolve_group_iterations(args = {}, obj = group, context = { current_user: c context 'with search and in parameters' do where(:search, :fields_to_search, :expected_iterations) do '' | [] | lazy { all_iterations } + 'iteration' | nil | lazy { plan_cadence.iterations } 'iteration' | [] | lazy { plan_cadence.iterations } 'iteration' | [:title] | lazy { plan_cadence.iterations } 'iteration' | [:title, :cadence_title] | lazy { plan_cadence.iterations } @@ -93,6 +94,12 @@ def resolve_group_iterations(args = {}, obj = group, context = { current_user: c end end + it "raises an error when 'in' is specified but 'search' is not" do + expect do + resolve_group_iterations({ in: [:title] }) + end.to raise_error(Gitlab::Graphql::Errors::ArgumentError, "'search' must be specified when using 'in' argument.") + end + it "uses 'search' and 'in' arguments to search title" do expect(resolve_group_iterations({ title: 'iteration' }).items).to contain_exactly(*plan_cadence.iterations) end -- GitLab