From d01a93e0f0ae3c5344b47ec3674bdfd7be309556 Mon Sep 17 00:00:00 2001 From: Sylvain Date: Wed, 12 Dec 2018 17:24:31 +0100 Subject: [PATCH] [ongoing] members tests --- app/controllers/api/members_controller.rb | 166 ++++++++++-------- .../users/omniauth_callbacks_controller.rb | 19 +- .../members_service.rb} | 4 +- test/integration/members/as_admin_test.rb | 39 ++++ 4 files changed, 143 insertions(+), 85 deletions(-) rename app/{processors/members_processor.rb => services/members_service.rb} (97%) create mode 100644 test/integration/members/as_admin_test.rb diff --git a/app/controllers/api/members_controller.rb b/app/controllers/api/members_controller.rb index 89c1c47bf..d647ff902 100644 --- a/app/controllers/api/members_controller.rb +++ b/app/controllers/api/members_controller.rb @@ -13,7 +13,7 @@ class API::MembersController < API::ApiController # remove unmerged profiles from list @members = @query.to_a - @members.delete_if { |u| u.need_completion? } + @members.delete_if(&:need_completion?) end def last_subscribed @@ -21,7 +21,7 @@ class API::MembersController < API::ApiController # remove unmerged profiles from list @members = @query.to_a - @members.delete_if { |u| u.need_completion? } + @members.delete_if(&:need_completion?) @requested_attributes = ['profile'] render :index @@ -63,17 +63,17 @@ class API::MembersController < API::ApiController def update authorize @member - @flow_worker = MembersProcessor.new(@member) + members_service = MembersService.new(@member) - if user_params[:group_id] and @member.group_id != user_params[:group_id].to_i and @member.subscribed_plan != nil + if user_params[:group_id] && @member.group_id != user_params[:group_id].to_i && !@member.subscribed_plan.nil? # here a group change is requested but unprocessable, handle the exception @member.errors[:group_id] = t('members.unable_to_change_the_group_while_a_subscription_is_running') render json: @member.errors, status: :unprocessable_entity else # otherwise, run the user update - if @flow_worker.update(user_params) + if members_service.update(user_params) # Update password without logging out - sign_in(@member, :bypass => true) unless current_user.id != params[:id].to_i + sign_in(@member, bypass: true) unless current_user.id != params[:id].to_i render :show, status: :ok, location: member_path(@member) else render json: @member.errors, status: :unprocessable_entity @@ -92,16 +92,18 @@ class API::MembersController < API::ApiController def export_subscriptions authorize :export - export = Export.where({category:'users', export_type: 'subscriptions'}).where('created_at > ?', Subscription.maximum('updated_at')).last + export = Export.where(category:'users', export_type: 'subscriptions').where('created_at > ?', Subscription.maximum('updated_at')).last if export.nil? || !FileTest.exist?(export.file) - @export = Export.new({category:'users', export_type: 'subscriptions', user: current_user}) + @export = Export.new(category:'users', export_type: 'subscriptions', user: current_user) if @export.save - render json: {export_id: @export.id}, status: :ok + render json: { export_id: @export.id }, status: :ok else render json: @export.errors, status: :unprocessable_entity end else - send_file File.join(Rails.root, export.file), :type => 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', :disposition => 'attachment' + send_file File.join(Rails.root, export.file), + type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + disposition: 'attachment' end end @@ -109,32 +111,40 @@ class API::MembersController < API::ApiController def export_reservations authorize :export - export = Export.where({category:'users', export_type: 'reservations'}).where('created_at > ?', Reservation.maximum('updated_at')).last + export = Export.where(category:'users', export_type: 'reservations') + .where('created_at > ?', Reservation.maximum('updated_at')) + .last if export.nil? || !FileTest.exist?(export.file) - @export = Export.new({category:'users', export_type: 'reservations', user: current_user}) + @export = Export.new(category: 'users', export_type: 'reservations', user: current_user) if @export.save - render json: {export_id: @export.id}, status: :ok + render json: { export_id: @export.id }, status: :ok else render json: @export.errors, status: :unprocessable_entity end else - send_file File.join(Rails.root, export.file), :type => 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', :disposition => 'attachment' + send_file File.join(Rails.root, export.file), + type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + disposition: 'attachment' end end def export_members authorize :export - export = Export.where({category:'users', export_type: 'members'}).where('created_at > ?', User.with_role(:member).maximum('updated_at')).last + export = Export.where(category:'users', export_type: 'members') + .where('created_at > ?', User.with_role(:member).maximum('updated_at')) + .last if export.nil? || !FileTest.exist?(export.file) - @export = Export.new({category:'users', export_type: 'members', user: current_user}) + @export = Export.new(category:'users', export_type: 'members', user: current_user) if @export.save - render json: {export_id: @export.id}, status: :ok + render json: { export_id: @export.id }, status: :ok else render json: @export.errors, status: :unprocessable_entity end else - send_file File.join(Rails.root, export.file), :type => 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', :disposition => 'attachment' + send_file File.join(Rails.root, export.file), + type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + disposition: 'attachment' end end @@ -147,21 +157,21 @@ class API::MembersController < API::ApiController @account = User.find_by(auth_token: token) if @account - @flow_worker = MembersProcessor.new(@account) + members_service = MembersService.new(@account) begin - if @flow_worker.merge_from_sso(@member) + if members_service.merge_from_sso(@member) @member = @account # finally, log on the real account - sign_in(@member, :bypass => true) + sign_in(@member, bypass: true) render :show, status: :ok, location: member_path(@member) else render json: @member.errors, status: :unprocessable_entity end rescue DuplicateIndexError => error - render json: {error: t('members.please_input_the_authentication_code_sent_to_the_address', EMAIL: error.message)}, status: :unprocessable_entity + render json: { error: t('members.please_input_the_authentication_code_sent_to_the_address', EMAIL: error.message) }, status: :unprocessable_entity end else - render json: {error: t('members.your_authentication_code_is_not_valid')}, status: :unprocessable_entity + render json: { error: t('members.your_authentication_code_is_not_valid') }, status: :unprocessable_entity end end @@ -170,41 +180,38 @@ class API::MembersController < API::ApiController p = params.require(:query).permit(:search, :order_by, :page, :size) - unless p[:page].is_a? Integer - render json: {error: 'page must be an integer'}, status: :unprocessable_entity - end - unless p[:size].is_a? Integer - render json: {error: 'size must be an integer'}, status: :unprocessable_entity - end + render json: {error: 'page must be an integer'}, status: :unprocessable_entity unless p[:page].is_a? Integer + + render json: {error: 'size must be an integer'}, status: :unprocessable_entity unless p[:size].is_a? Integer direction = (p[:order_by][0] == '-' ? 'DESC' : 'ASC') order_key = (p[:order_by][0] == '-' ? p[:order_by][1, p[:order_by].size] : p[:order_by]) - case order_key - when 'last_name' - order_key = 'profiles.last_name' - when 'first_name' - order_key = 'profiles.first_name' - when 'email' - order_key = 'users.email' - when 'phone' - order_key = 'profiles.phone' - when 'group' - order_key = 'groups.name' - when 'plan' - order_key = 'plans.base_name' - else - order_key = 'users.id' - end + order_key = case order_key + when 'last_name' + 'profiles.last_name' + when 'first_name' + 'profiles.first_name' + when 'email' + 'users.email' + when 'phone' + 'profiles.phone' + when 'group' + 'groups.name' + when 'plan' + 'plans.base_name' + else + 'users.id' + end @query = User.includes(:profile, :group, :subscriptions) - .joins(:profile, :group, :roles, 'LEFT JOIN "subscriptions" ON "subscriptions"."user_id" = "users"."id" LEFT JOIN "plans" ON "plans"."id" = "subscriptions"."plan_id"') - .where("users.is_active = 'true' AND roles.name = 'member'") - .order("#{order_key} #{direction}") - .page(p[:page]) - .per(p[:size]) + .joins(:profile, :group, :roles, 'LEFT JOIN "subscriptions" ON "subscriptions"."user_id" = "users"."id" LEFT JOIN "plans" ON "plans"."id" = "subscriptions"."plan_id"') + .where("users.is_active = 'true' AND roles.name = 'member'") + .order("#{order_key} #{direction}") + .page(p[:page]) + .per(p[:size]) # ILIKE => PostgreSQL case-insensitive LIKE @query = @query.where('profiles.first_name ILIKE :search OR profiles.last_name ILIKE :search OR profiles.phone ILIKE :search OR email ILIKE :search OR groups.name ILIKE :search OR plans.base_name ILIKE :search', search: "%#{p[:search]}%") if p[:search].size > 0 @@ -215,9 +222,9 @@ class API::MembersController < API::ApiController def search @members = User.includes(:profile) - .joins(:profile, :roles, 'LEFT JOIN "subscriptions" ON "subscriptions"."user_id" = "users"."id"') - .where("users.is_active = 'true' AND roles.name = 'member'") - .where("lower(f_unaccent(profiles.first_name)) ~ regexp_replace(:search, E'\\\\s+', '|') OR lower(f_unaccent(profiles.last_name)) ~ regexp_replace(:search, E'\\\\s+', '|')", search: params[:query].downcase) + .joins(:profile, :roles, 'LEFT JOIN "subscriptions" ON "subscriptions"."user_id" = "users"."id"') + .where("users.is_active = 'true' AND roles.name = 'member'") + .where("lower(f_unaccent(profiles.first_name)) ~ regexp_replace(:search, E'\\\\s+', '|') OR lower(f_unaccent(profiles.last_name)) ~ regexp_replace(:search, E'\\\\s+', '|')", search: params[:query].downcase) if current_user.is_member? # non-admin can only retrieve users with "public profiles" @@ -241,26 +248,37 @@ class API::MembersController < API::ApiController end private - def set_member - @member = User.find(params[:id]) - end - - def user_params - if current_user.id == params[:id].to_i - params.require(:user).permit(:username, :email, :password, :password_confirmation, :group_id, :is_allow_contact, :is_allow_newsletter, - profile_attributes: [:id, :first_name, :last_name, :gender, :birthday, :phone, :interest, :software_mastered, :website, :job, - :facebook, :twitter, :google_plus, :viadeo, :linkedin, :instagram, :youtube, :vimeo, :dailymotion, :github, :echosciences, :pinterest, :lastfm, :flickr, - user_avatar_attributes: [:id, :attachment, :_destroy], address_attributes: [:id, :address], - organization_attributes: [:id, :name, address_attributes: [:id, :address]]]) - - elsif current_user.is_admin? - params.require(:user).permit(:username, :email, :password, :password_confirmation, :invoicing_disabled, :is_allow_contact, :is_allow_newsletter, - :group_id, training_ids: [], tag_ids: [], - profile_attributes: [:id, :first_name, :last_name, :gender, :birthday, :phone, :interest, :software_mastered, :website, :job, - :facebook, :twitter, :google_plus, :viadeo, :linkedin, :instagram, :youtube, :vimeo, :dailymotion, :github, :echosciences, :pinterest, :lastfm, :flickr, - user_avatar_attributes: [:id, :attachment, :_destroy], address_attributes: [:id, :address], - organization_attributes: [:id, :name, address_attributes: [:id, :address]]]) - - end + + def set_member + @member = User.find(params[:id]) + end + + def user_params + if current_user.id == params[:id].to_i + params.require(:user).permit(:username, :email, :password, :password_confirmation, :group_id, :is_allow_contact, + :is_allow_newsletter, + profile_attributes: [:id, :first_name, :last_name, :gender, :birthday, :phone, :interest, + :software_mastered, :website, :job, :facebook, :twitter, + :google_plus, :viadeo, :linkedin, :instagram, :youtube, :vimeo, + :dailymotion, :github, :echosciences, :pinterest, :lastfm, :flickr, + user_avatar_attributes: %i[id attachment destroy], + address_attributes: %i[id address], + organization_attributes: [:id, :name, + address_attributes: %i[id address]]]) + + elsif current_user.is_admin? + params.require(:user).permit(:username, :email, :password, :password_confirmation, :invoicing_disabled, + :is_allow_contact, :is_allow_newsletter, :group_id, + training_ids: [], tag_ids: [], + profile_attributes: [:id, :first_name, :last_name, :gender, :birthday, :phone, :interest, + :software_mastered, :website, :job, :facebook, :twitter, + :google_plus, :viadeo, :linkedin, :instagram, :youtube, :vimeo, + :dailymotion, :github, :echosciences, :pinterest, :lastfm, :flickr, + user_avatar_attributes: %i[id attachment destroy], + address_attributes: %i[id address], + organization_attributes: [:id, :name, + address_attributes: %i[id address]]]) + end + end end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index f11fe55c9..9853b857b 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -9,7 +9,7 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController if @user.id.nil? # => new user (ie. not updating existing) # If the username is mapped, we just check its uniqueness as it would break the postgresql - # unique contraint otherwise. If the name is not unique, another unique is generated + # unique constraint otherwise. If the name is not unique, another unique is generated if active_provider.sso_fields.include?('user.username') @user.username = generate_unique_username(@user.username) end @@ -36,8 +36,8 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController # We BYPASS THE VALIDATION because, in case of a new user, we want to save him anyway, we'll ask him later to complete his profile (on first login). # In case of an existing user, we trust the SSO validation as we want the SSO to have authority on users management and policy. - @user.save(:validate => false) - sign_in_and_redirect @user, :event => :authentication #this will throw if @user is not activated + @user.save(validate: false) + sign_in_and_redirect @user, event: :authentication # this will throw if @user is not activated else @user = User.find_by(auth_token: request.env['omniauth.params']['auth_token']) @@ -46,7 +46,7 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController begin @user.link_with_omniauth_provider(request.env['omniauth.auth']) - sign_in_and_redirect @user, :event => :authentication + sign_in_and_redirect @user, event: :authentication rescue DuplicateIndexError redirect_to root_url, alert: t('omniauth.this_account_is_already_linked_to_an_user_of_the_platform', NAME: active_provider.name) end @@ -55,19 +55,20 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController end private + def username_exists?(username, exclude_id = nil) if exclude_id.nil? - User.where(username: username).size > 0 + User.where(username: username).size.positive? else - User.where(username: username).where.not(id: exclude_id).size > 0 + User.where(username: username).where.not(id: exclude_id).size.positive? end end def email_exists?(email, exclude_id = nil) if exclude_id.nil? - User.where(email: email).size > 0 + User.where(email: email).size.positive? else - User.where(email: email).where.not(id: exclude_id).size > 0 + User.where(email: email).where.not(id: exclude_id).size.positive? end end @@ -80,4 +81,4 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController end generated end -end \ No newline at end of file +end diff --git a/app/processors/members_processor.rb b/app/services/members_service.rb similarity index 97% rename from app/processors/members_processor.rb rename to app/services/members_service.rb index ee9ddc6e5..d9c173079 100644 --- a/app/processors/members_processor.rb +++ b/app/services/members_service.rb @@ -1,5 +1,5 @@ -class MembersProcessor +class MembersService attr_accessor :member @@ -42,4 +42,4 @@ class MembersProcessor attached_object: self.member end -end \ No newline at end of file +end diff --git a/test/integration/members/as_admin_test.rb b/test/integration/members/as_admin_test.rb new file mode 100644 index 000000000..dc4e16861 --- /dev/null +++ b/test/integration/members/as_admin_test.rb @@ -0,0 +1,39 @@ +class MemebersTest < ActionDispatch::IntegrationTest + + # Called before every test method runs. Can be used + # to set up fixture information. + def setup + @admin = User.find_by(username: 'admin') + login_as(@admin, scope: :user) + end + + test 'admin creates member' do + + group_id = Group.first.id + email = 'robert.dubois@gmail.com' + + VCR.use_cassette('members_admin_create_success') do + post members_path, { user: { + username: 'bob', + email: email, + group_id: group_id, + profile_attributes: { + gender: true, + last_name: 'Dubois', + first_name: 'Robert', + birthday: '2018-02-08', + phone: '0485232145' + } + } }.to_json, default_headers + end + + # Check response format & status + assert_equal 201, response.status, response.body + assert_equal Mime::JSON, response.content_type + + # Check the user was subscribed + user = json_response(response.body) + assert_equal email, user[:email], "user's mail does not match" + + end +end