diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-10-26 10:42:41 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-10-26 10:42:41 +0000 |
commit | 8325a1fc9f0b2c9cea9073d4377f2b3b35771e8b (patch) | |
tree | 9ea5433076dd1576d867babbe33e7f648a087b58 | |
parent | c10c553e1db344f0cd65454373c63af0a5cb58f0 (diff) | |
parent | e0f0c29b0cfe3c0c97191eeb96eae1299f3983d1 (diff) | |
download | gitlab-ce-8325a1fc9f0b2c9cea9073d4377f2b3b35771e8b.tar.gz |
Merge branch '52384-api-filter-assignee-none-any' into 'master'
Resolve "Filter by `None`/`Any` for assignee_id in issues/mrs API"
Closes #52384
See merge request gitlab-org/gitlab-ce!22598
-rw-r--r-- | app/finders/issuable_finder.rb | 35 | ||||
-rw-r--r-- | app/finders/issues_finder.rb | 8 | ||||
-rw-r--r-- | changelogs/unreleased/52384-api-filter-assignee-none-any.yml | 5 | ||||
-rw-r--r-- | doc/api/issues.md | 6 | ||||
-rw-r--r-- | doc/api/merge_requests.md | 6 | ||||
-rw-r--r-- | lib/api/helpers/custom_validators.rb | 13 | ||||
-rw-r--r-- | lib/api/issues.rb | 3 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 3 | ||||
-rw-r--r-- | spec/finders/issues_finder_spec.rb | 30 | ||||
-rw-r--r-- | spec/lib/api/helpers/custom_validators_spec.rb | 64 | ||||
-rw-r--r-- | spec/requests/api/issues_spec.rb | 18 | ||||
-rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 17 |
12 files changed, 183 insertions, 25 deletions
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 8abfe0c4c17..eb3d2498830 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -14,7 +14,7 @@ # project_id: integer # milestone_title: string # author_id: integer -# assignee_id: integer +# assignee_id: integer or 'None' or 'Any' # search: string # label_name: string # sort: string @@ -34,6 +34,11 @@ class IssuableFinder requires_cross_project_access unless: -> { project? } + # This is used as a common filter for None / Any + FILTER_NONE = 'none'.freeze + FILTER_ANY = 'any'.freeze + + # This is accepted as a deprecated filter and is also used in unassigning users NONE = '0'.freeze attr_accessor :current_user, :params @@ -236,16 +241,20 @@ class IssuableFinder # rubocop: enable CodeReuse/ActiveRecord def assignee_id? - params[:assignee_id].present? && params[:assignee_id].to_s != NONE + params[:assignee_id].present? end def assignee_username? - params[:assignee_username].present? && params[:assignee_username].to_s != NONE + params[:assignee_username].present? end - def no_assignee? + def filter_by_no_assignee? # Assignee_id takes precedence over assignee_username - params[:assignee_id].to_s == NONE || params[:assignee_username].to_s == NONE + [NONE, FILTER_NONE].include?(params[:assignee_id].to_s.downcase) || params[:assignee_username].to_s == NONE + end + + def filter_by_any_assignee? + params[:assignee_id].to_s.downcase == FILTER_ANY end # rubocop: disable CodeReuse/ActiveRecord @@ -399,15 +408,17 @@ class IssuableFinder # rubocop: disable CodeReuse/ActiveRecord def by_assignee(items) - if assignee - items = items.where(assignee_id: assignee.id) - elsif no_assignee? - items = items.where(assignee_id: nil) + if filter_by_no_assignee? + items.where(assignee_id: nil) + elsif filter_by_any_assignee? + items.where('assignee_id IS NOT NULL') + elsif assignee + items.where(assignee_id: assignee.id) elsif assignee_id? || assignee_username? # assignee not found - items = items.none + items.none + else + items end - - items end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index abdc47b9866..cee57a83df4 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -137,10 +137,12 @@ class IssuesFinder < IssuableFinder # rubocop: disable CodeReuse/ActiveRecord def by_assignee(items) - if assignee - items.assigned_to(assignee) - elsif no_assignee? + if filter_by_no_assignee? items.unassigned + elsif filter_by_any_assignee? + items.assigned + elsif assignee + items.assigned_to(assignee) elsif assignee_id? || assignee_username? # assignee not found items.none else diff --git a/changelogs/unreleased/52384-api-filter-assignee-none-any.yml b/changelogs/unreleased/52384-api-filter-assignee-none-any.yml new file mode 100644 index 00000000000..9acec04d946 --- /dev/null +++ b/changelogs/unreleased/52384-api-filter-assignee-none-any.yml @@ -0,0 +1,5 @@ +--- +title: Add None/Any option for assignee_id in Issues and Merge Requests API +merge_request: 22598 +author: Heinrich Lee Yu +type: added diff --git a/doc/api/issues.md b/doc/api/issues.md index cc1d6834a20..57e861bc62e 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -40,7 +40,7 @@ GET /issues?my_reaction_emoji=star | `milestone` | string | no | The milestone title. `No+Milestone` lists all issues with no milestone. `Any+Milestone` lists all issues that have an assigned milestone | | `scope` | string | no | Return issues for the given scope: `created_by_me`, `assigned_to_me` or `all`. Defaults to `created_by_me`<br> For versions before 11.0, use the now deprecated `created-by-me` or `assigned-to-me` scopes instead.<br> _([Introduced][ce-13004] in GitLab 9.5. [Changed to snake_case][ce-18935] in GitLab 11.0)_ | | `author_id` | integer | no | Return issues created by the given user `id`. Combine with `scope=all` or `scope=assigned_to_me`. _([Introduced][ce-13004] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Return issues assigned to the given user `id`. `None` returns unassigned issues. `Any` returns issues with an assignee. _([Introduced][ce-13004] in GitLab 9.5)_ | | `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `iids[]` | Array[integer] | no | Return only the issues having the given `iid` | | `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | @@ -154,7 +154,7 @@ GET /groups/:id/issues?my_reaction_emoji=star | `milestone` | string | no | The milestone title. `No+Milestone` lists all issues with no milestone | | `scope` | string | no | Return issues for the given scope: `created_by_me`, `assigned_to_me` or `all`.<br> For versions before 11.0, use the now deprecated `created-by-me` or `assigned-to-me` scopes instead.<br> _([Introduced][ce-13004] in GitLab 9.5. [Changed to snake_case][ce-18935] in GitLab 11.0)_ | | `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Return issues assigned to the given user `id`. `None` returns unassigned issues. `Any` returns issues with an assignee. _([Introduced][ce-13004] in GitLab 9.5)_ | | `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | | `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | @@ -268,7 +268,7 @@ GET /projects/:id/issues?my_reaction_emoji=star | `milestone` | string | no | The milestone title. `No+Milestone` lists all issues with no milestone | | `scope` | string | no | Return issues for the given scope: `created_by_me`, `assigned_to_me` or `all`.<br> For versions before 11.0, use the now deprecated `created-by-me` or `assigned-to-me` scopes instead.<br> _([Introduced][ce-13004] in GitLab 9.5. [Changed to snake_case][ce-18935] in GitLab 11.0)_ | | `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Return issues assigned to the given user `id`. `None` returns unassigned issues. `Any` returns issues with an assignee. _([Introduced][ce-13004] in GitLab 9.5)_ | | `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | | `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 862ee398a84..0291b7e00c2 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -42,7 +42,7 @@ Parameters: | `updated_before` | datetime | no | Return merge requests updated on or before the given time | | `scope` | string | no | Return merge requests for the given scope: `created_by_me`, `assigned_to_me` or `all`. Defaults to `created_by_me`<br> For versions before 11.0, use the now deprecated `created-by-me` or `assigned-to-me` scopes instead. | | `author_id` | integer | no | Returns merge requests created by the given user `id`. Combine with `scope=all` or `scope=assigned_to_me` | -| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` | +| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id`. `None` returns unassigned merge requests. `Any` returns merge requests with an assignee. | | `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `source_branch` | string | no | Return merge requests with the given source branch | | `target_branch` | string | no | Return merge requests with the given target branch | @@ -166,7 +166,7 @@ Parameters: | `updated_before` | datetime | no | Return merge requests updated on or before the given time | | `scope` | string | no | Return merge requests for the given scope: `created_by_me`, `assigned_to_me` or `all`.<br> For versions before 11.0, use the now deprecated `created-by-me` or `assigned-to-me` scopes instead.<br> _([Introduced][ce-13060] in GitLab 9.5. [Changed to snake_case][ce-18935] in GitLab 11.0)_ | | `author_id` | integer | no | Returns merge requests created by the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id`. `None` returns unassigned merge requests. `Any` returns merge requests with an assignee. _([Introduced][ce-13060] in GitLab 9.5)_ | | `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `source_branch` | string | no | Return merge requests with the given source branch | | `target_branch` | string | no | Return merge requests with the given target branch | @@ -279,7 +279,7 @@ Parameters: | `updated_before` | datetime | no | Return merge requests updated on or before the given time | | `scope` | string | no | Return merge requests for the given scope: `created_by_me`, `assigned_to_me` or `all`.<br> | | `author_id` | integer | no | Returns merge requests created by the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id`. `None` returns unassigned merge requests. `Any` returns merge requests with an assignee. _([Introduced][ce-13060] in GitLab 9.5)_ | | `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `source_branch` | string | no | Return merge requests with the given source branch | | `target_branch` | string | no | Return merge requests with the given target branch | diff --git a/lib/api/helpers/custom_validators.rb b/lib/api/helpers/custom_validators.rb index 23b1cd1ad45..1058f4e8a5e 100644 --- a/lib/api/helpers/custom_validators.rb +++ b/lib/api/helpers/custom_validators.rb @@ -10,8 +10,21 @@ module API raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:absence) end end + + class IntegerNoneAny < Grape::Validations::Base + def validate_param!(attr_name, params) + value = params[attr_name] + + return if value.is_a?(Integer) || + [IssuableFinder::FILTER_NONE, IssuableFinder::FILTER_ANY].include?(value.to_s.downcase) + + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], + message: "should be an integer, 'None' or 'Any'" + end + end end end end Grape::Validations.register_validator(:absence, ::API::Helpers::CustomValidators::Absence) +Grape::Validations.register_validator(:integer_none_any, ::API::Helpers::CustomValidators::IntegerNoneAny) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 25d78053c88..405fc30a2ed 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -40,7 +40,8 @@ module API optional :updated_after, type: DateTime, desc: 'Return issues updated after the specified time' optional :updated_before, type: DateTime, desc: 'Return issues updated before the specified time' optional :author_id, type: Integer, desc: 'Return issues which are authored by the user with the given ID' - optional :assignee_id, type: Integer, desc: 'Return issues which are assigned to the user with the given ID' + optional :assignee_id, types: [Integer, String], integer_none_any: true, + desc: 'Return issues which are assigned to the user with the given ID' optional :scope, type: String, values: %w[created-by-me assigned-to-me created_by_me assigned_to_me all], desc: 'Return issues for the given scope: `created_by_me`, `assigned_to_me` or `all`' optional :my_reaction_emoji, type: String, desc: 'Return issues reacted by the authenticated user by the given emoji' diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 440d94ae186..a617efaaa4c 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -89,7 +89,8 @@ module API optional :updated_before, type: DateTime, desc: 'Return merge requests updated before the specified time' optional :view, type: String, values: %w[simple], desc: 'If simple, returns the `iid`, URL, title, description, and basic state of merge request' optional :author_id, type: Integer, desc: 'Return merge requests which are authored by the user with the given ID' - optional :assignee_id, type: Integer, desc: 'Return merge requests which are assigned to the user with the given ID' + optional :assignee_id, types: [Integer, String], integer_none_any: true, + desc: 'Return merge requests which are assigned to the user with the given ID' optional :scope, type: String, values: %w[created-by-me assigned-to-me created_by_me assigned_to_me all], desc: 'Return merge requests for the given scope: `created_by_me`, `assigned_to_me` or `all`' optional :my_reaction_emoji, type: String, desc: 'Return issues reacted by the authenticated user by the given emoji' diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 0689c843104..2f164ffa8b0 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -57,11 +57,37 @@ describe IssuesFinder do end context 'filtering by no assignee' do - let(:params) { { assignee_id: 0 } } + let(:params) { { assignee_id: 'None' } } - it 'returns issues not assign to any assignee' do + it 'returns issues not assigned to any assignee' do expect(issues).to contain_exactly(issue4) end + + it 'returns issues not assigned to any assignee' do + params[:assignee_id] = 0 + + expect(issues).to contain_exactly(issue4) + end + + it 'returns issues not assigned to any assignee' do + params[:assignee_id] = 'none' + + expect(issues).to contain_exactly(issue4) + end + end + + context 'filtering by any assignee' do + let(:params) { { assignee_id: 'Any' } } + + it 'returns issues assigned to any assignee' do + expect(issues).to contain_exactly(issue1, issue2, issue3) + end + + it 'returns issues assigned to any assignee' do + params[:assignee_id] = 'any' + + expect(issues).to contain_exactly(issue1, issue2, issue3) + end end context 'filtering by group_id' do diff --git a/spec/lib/api/helpers/custom_validators_spec.rb b/spec/lib/api/helpers/custom_validators_spec.rb new file mode 100644 index 00000000000..41e6fb47b11 --- /dev/null +++ b/spec/lib/api/helpers/custom_validators_spec.rb @@ -0,0 +1,64 @@ +require 'spec_helper' + +describe API::Helpers::CustomValidators do + let(:scope) do + Struct.new(:opts) do + def full_name(attr_name) + attr_name + end + end + end + + describe API::Helpers::CustomValidators::Absence do + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'empty param' do + it 'does not raise a validation error' do + expect_no_validation_error({}) + end + end + + context 'invalid parameters' do + it 'should raise a validation error' do + expect_validation_error({ 'test' => 'some_value' }) + end + end + end + + describe API::Helpers::CustomValidators::IntegerNoneAny do + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'valid parameters' do + it 'does not raise a validation error' do + expect_no_validation_error({ 'test' => 2 }) + expect_no_validation_error({ 'test' => 100 }) + expect_no_validation_error({ 'test' => 'None' }) + expect_no_validation_error({ 'test' => 'Any' }) + expect_no_validation_error({ 'test' => 'none' }) + expect_no_validation_error({ 'test' => 'any' }) + end + end + + context 'invalid parameters' do + it 'should raise a validation error' do + expect_validation_error({ 'test' => 'some_other_string' }) + end + end + end + + def expect_no_validation_error(params) + expect { validate_test_param!(params) }.not_to raise_error + end + + def expect_validation_error(params) + expect { validate_test_param!(params) }.to raise_error(Grape::Exceptions::Validation) + end + + def validate_test_param!(params) + subject.validate_param!('test', params) + end +end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 9f6cf12f9a7..9cda39a569b 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -178,6 +178,24 @@ describe API::Issues do expect(first_issue['id']).to eq(issue2.id) end + it 'returns issues with no assignee' do + issue2 = create(:issue, author: user2, project: project) + + get api('/issues', user), assignee_id: 'None', scope: 'all' + + expect_paginated_array_response(size: 1) + expect(first_issue['id']).to eq(issue2.id) + end + + it 'returns issues with any assignee' do + # This issue without assignee should not be returned + create(:issue, author: user2, project: project) + + get api('/issues', user), assignee_id: 'Any', scope: 'all' + + expect_paginated_array_response(size: 3) + end + it 'returns issues reacted by the authenticated user by the given emoji' do issue2 = create(:issue, project: project, author: user, assignees: [user]) award_emoji = create(:award_emoji, awardable: issue2, user: user2, name: 'star') diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 07d19e3ad29..e4e0ca285e0 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -143,6 +143,23 @@ describe API::MergeRequests do expect_response_ordered_exactly(merge_request3) end + it 'returns an array of merge requests with no assignee' do + merge_request3 = create(:merge_request, :simple, author: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + + get api('/merge_requests', user), assignee_id: 'None', scope: :all + + expect_response_ordered_exactly(merge_request3) + end + + it 'returns an array of merge requests with any assignee' do + # This MR with no assignee should not be returned + create(:merge_request, :simple, author: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + + get api('/merge_requests', user), assignee_id: 'Any', scope: :all + + expect_response_contain_exactly(merge_request, merge_request2, merge_request_closed, merge_request_merged, merge_request_locked) + end + it 'returns an array of merge requests assigned to me' do merge_request3 = create(:merge_request, :simple, author: user, assignee: user2, source_project: project2, target_project: project2, source_branch: 'other-branch') |