From 9b3e66a506160246bd081f04bd78f143faa48a8f Mon Sep 17 00:00:00 2001 From: Jacopo Date: Mon, 6 May 2019 22:37:18 +0200 Subject: Adds email types for api/users/:id/emails By using `types[]` parameter the user can obtain the `primary`, `secondary` or both types of emails. The `primary` is user.email and `secondary` are user.emails. --- app/finders/user_emails_finder.rb | 41 +++++++++++++++++++++ app/models/concerns/null_sorting.rb | 9 +++++ app/models/email.rb | 4 +++ app/models/user.rb | 1 + ...s-id-emails-is-inconsistent-w-documentation.yml | 5 +++ doc/api/users.md | 1 + lib/api/entities.rb | 3 +- lib/api/users.rb | 9 ++--- spec/finders/user_emails_finder_spec.rb | 42 ++++++++++++++++++++++ spec/requests/api/users_spec.rb | 27 ++++++++++++-- 10 files changed, 134 insertions(+), 8 deletions(-) create mode 100644 app/finders/user_emails_finder.rb create mode 100644 app/models/concerns/null_sorting.rb create mode 100644 changelogs/unreleased/53618-admin-api-get-users-id-emails-is-inconsistent-w-documentation.yml create mode 100644 spec/finders/user_emails_finder_spec.rb diff --git a/app/finders/user_emails_finder.rb b/app/finders/user_emails_finder.rb new file mode 100644 index 00000000000..4f0215a3707 --- /dev/null +++ b/app/finders/user_emails_finder.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +# Finds the user emails. +# +# Arguments: +# user - which user use +# types: Array - email types (primary/secondary) the primary is user.email, +# the secondary are user.emails. +# +# Returns an Email(id, email attributes only) ActiveRecord::Relation with the id set to nil +# for the primary email. +# +# Using pluck in this finder is not supported because we are using select NULL as id and pluck +# overrides the select statement. +class UserEmailsFinder < UnionFinder + attr_reader :user, :params + + def initialize(user, params = {}) + @user = user + @params = params + end + + def execute + return Email.none unless params[:types].present? + + find_union(all_emails_by_types, Email).order_id_asc_nulls_first + end + + private + + # rubocop: disable CodeReuse/ActiveRecord + def all_emails_by_types + emails = [] + + emails << user.class.where(id: user).select('NULL as id, email') if params[:types].include?('primary') + emails << user.emails.select('id, email') if params[:types].include?('secondary') + + emails + end + # rubocop: enable CodeReuse/ActiveRecord +end diff --git a/app/models/concerns/null_sorting.rb b/app/models/concerns/null_sorting.rb new file mode 100644 index 00000000000..294e6b7d073 --- /dev/null +++ b/app/models/concerns/null_sorting.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module NullSorting + extend ActiveSupport::Concern + + included do + scope :order_id_asc_nulls_first, -> { order(Gitlab::Database.nulls_first_order('id', 'ASC')) } + end +end diff --git a/app/models/email.rb b/app/models/email.rb index 0ddaa049c3b..10424128943 100644 --- a/app/models/email.rb +++ b/app/models/email.rb @@ -1,8 +1,12 @@ # frozen_string_literal: true class Email < ApplicationRecord + EMAIL_TYPES = %w[primary secondary].freeze + include Sortable + include NullSorting include Gitlab::SQL::Pattern + include FromUnion belongs_to :user diff --git a/app/models/user.rb b/app/models/user.rb index 60f69659a6b..b82bffd02ac 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -11,6 +11,7 @@ class User < ApplicationRecord include Avatarable include Referable include Sortable + include NullSorting include CaseSensitivity include TokenAuthenticatable include IgnorableColumn diff --git a/changelogs/unreleased/53618-admin-api-get-users-id-emails-is-inconsistent-w-documentation.yml b/changelogs/unreleased/53618-admin-api-get-users-id-emails-is-inconsistent-w-documentation.yml new file mode 100644 index 00000000000..c62b52967f7 --- /dev/null +++ b/changelogs/unreleased/53618-admin-api-get-users-id-emails-is-inconsistent-w-documentation.yml @@ -0,0 +1,5 @@ +--- +title: Adds email types for api/users/:id/emails +merge_request: 27904 +author: Jacopo Beschi @jacopo-beschi +type: changed diff --git a/doc/api/users.md b/doc/api/users.md index d3e67d3d510..0b051c13b5e 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -918,6 +918,7 @@ GET /users/:id/emails Parameters: - `id` (required) - id of specified user +- `types` (optional) - the email types: `primary` or `secondary` (default) ## Single email diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 625fada4f08..442d3cd456b 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -99,7 +99,8 @@ module API end class Email < Grape::Entity - expose :id, :email + expose :id, unless: ->(email) { email.id.nil? } + expose :email end class Hook < Grape::Entity diff --git a/lib/api/users.rb b/lib/api/users.rb index 2f23e33bd4a..821f13dfd45 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -418,17 +418,18 @@ module API end params do requires :id, type: Integer, desc: 'The ID of the user' + optional :types, type: Array, values: Email::EMAIL_TYPES, desc: "Type of the email: #{Email::EMAIL_TYPES.join(',')}", default: 'secondary' use :pagination end - # rubocop: disable CodeReuse/ActiveRecord get ':id/emails' do authenticated_as_admin! - user = User.find_by(id: params[:id]) + user = User.find_by(id: params[:id]) # rubocop: disable CodeReuse/ActiveRecord not_found!('User') unless user - present paginate(user.emails), with: Entities::Email + emails = UserEmailsFinder.new(user, types: params[:types]).execute + + present paginate(emails), with: Entities::Email end - # rubocop: enable CodeReuse/ActiveRecord desc 'Delete an email address of a specified user. Available only for admins.' do success Entities::Email diff --git a/spec/finders/user_emails_finder_spec.rb b/spec/finders/user_emails_finder_spec.rb new file mode 100644 index 00000000000..b9f512b3a6b --- /dev/null +++ b/spec/finders/user_emails_finder_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe UserEmailsFinder do + describe '#execute' do + let(:user) { create(:user) } + let!(:email1) { create(:email, user: user) } + let!(:email2) { create(:email, user: user) } + + def slice_email_attributes(emails) + emails.map { |email| email.slice('id', 'email') }.map(&:symbolize_keys) + end + + it 'returns the primary email with nil id when types is primary' do + emails = described_class.new(user.reload, types: %w[primary]).execute + + expect(slice_email_attributes(emails)).to eq([{ id: nil, email: user.email }]) + end + + it 'returns the secondary emails when types is secondary' do + emails = described_class.new(user.reload, types: %w[secondary]).execute + + expect(slice_email_attributes(emails)).to eq([{ id: email1.id, email: email1.email }, + { id: email2.id, email: email2.email }]) + end + + it 'returns both primary and secondary emails when type is primary,secondary sorted by id asc null first' do + emails = described_class.new(user.reload, types: %w[primary secondary]).execute + + expect(slice_email_attributes(emails)).to eq([{ id: nil, email: user.email }, + { id: email1.id, email: email1.email }, + { id: email2.id, email: email2.email }]) + end + + it 'returns an empty relation when types is empty' do + emails = described_class.new(user.reload, types: []).execute + + expect(emails).to be_empty + end + end +end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index b84202364e1..0cea02fcceb 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -1146,18 +1146,39 @@ describe API::Users do expect(json_response['message']).to eq('404 User Not Found') end - it 'returns array of emails' do - user.emails << email - user.save + it 'returns array of secondary emails' do + email get api("/users/#{user.id}/emails", admin) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array + expect(json_response.length).to eq(1) expect(json_response.first['email']).to eq(email.email) end + it 'returns only the primary email when types[]=primary' do + email + + get api("/users/#{user.id}/emails", admin), params: { types: ['primary'] } + + expect(response).to have_gitlab_http_status(200) + expect(json_response.length).to eq(1) + expect(json_response.first).to eq({ 'email' => user.email }) + end + + it 'returns both primary and secondary emails when types[]=primary,secondary' do + email + + get api("/users/#{user.id}/emails", admin), params: { types: %w[primary secondary] } + + expect(response).to have_gitlab_http_status(200) + expect(json_response.length).to eq(2) + expect(json_response.first).to eq({ 'email' => user.email }) + expect(json_response.second).to eq({ 'id' => email.id, 'email' => email.email }) + end + it "returns a 404 for invalid ID" do get api("/users/ASDF/emails", admin) -- cgit v1.2.3