From 11794feed149ca29ab9cbb27b72ea9835fa87dca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Tue, 18 Oct 2016 14:47:06 +0200 Subject: [PATCH] Handle invitation flow --- app/controllers/concerns/user_invitation.rb | 30 +++-- app/services/users/create_user_service.rb | 44 +++++++- config/locales/en.yml | 1 + lib/api/v3/users/create_user.rb | 1 - lib/api/v3/users/user_representer.rb | 9 +- .../api/v3/user/create_user_resource_spec.rb | 103 +++++++++++++++--- 6 files changed, 154 insertions(+), 34 deletions(-) diff --git a/app/controllers/concerns/user_invitation.rb b/app/controllers/concerns/user_invitation.rb index 96fddba233..99c5114c39 100644 --- a/app/controllers/concerns/user_invitation.rb +++ b/app/controllers/concerns/user_invitation.rb @@ -43,9 +43,6 @@ module UserInvitation ## # Creates an invited user with the given email address. - # If no first and last is given it will default to 'OpenProject User' - # for the first name and 'To-be' for the last name. - # The default login is the email address. # # @param email E-Mail address the invitation is sent to. # @param login User's login (optional) @@ -58,19 +55,34 @@ module UserInvitation # on the returned user will yield `false`. Check for validation errors # in that case. def invite_new_user(email:, login: nil, first_name: nil, last_name: nil) - placeholder = placeholder_name(email) - - user = User.new login: login || email, - mail: email, - firstname: first_name || placeholder.first, - lastname: last_name || placeholder.last, + user = User.new mail: email, + login: login, + firstname: first_name, + lastname: last_name, status: Principal::STATUSES[:invited] + assign_user_attributes(user) + yield user if block_given? invite_user! user end + ## + # For the given user with at least the mail attribute set, + # derives login and first name + # + # If no first and last is given it will default to 'OpenProject User' + # for the first name and 'To-be' for the last name. + # The default login is the email address. + def assign_user_attributes(user) + placeholder = placeholder_name(user.mail) + + user.login = user.login.presence || user.mail + user.firstname = user.firstname.presence || placeholder.first + user.lastname = user.lastname.presence || placeholder.last + end + ## # Sends a new invitation to the user with a new token. # diff --git a/app/services/users/create_user_service.rb b/app/services/users/create_user_service.rb index a7d5599a62..4ffbcbe9ab 100644 --- a/app/services/users/create_user_service.rb +++ b/app/services/users/create_user_service.rb @@ -28,6 +28,7 @@ #++ require 'work_packages/create_contract' +require 'concerns/user_invitation' module Users class CreateUserService @@ -52,11 +53,46 @@ module Users def create(new_user) initialize_contract(new_user) - result, errors = validate_and_save(new_user) + unless new_user.invited? + _, errors = validate_and_save(new_user) + return build_result(new_user, errors) + end + + # As we're basing on the user's mail, this parameter is required + # before we're able to validate the contract or user + if new_user.mail.blank? + contract.errors.add :mail, :blank + build_result(new_user, contract.errors) + else + create_invited(new_user) + end + end + + def build_result(result, errors) + success = result.is_a?(User) && errors.empty? + ServiceResult.new(success: success, errors: errors, result: result) + end + + ## + # User creation flow for users that are invited. + # Re-uses UserInvitation and thus avoids +validate_and_save+ + def create_invited(new_user) + # Assign values other than mail to new_user + ::UserInvitation.assign_user_attributes new_user + + # Check contract validity before moving to UserInvitation + if !contract.validate + build_result(new_user, contract.errors) + end + + invite_user! new_user + end + + def invite_user!(new_user) + invited = ::UserInvitation.invite_user! new_user + new_user.errors.add :base, I18n.t(:error_can_not_invite_user) unless invited.is_a? User - ServiceResult.new(success: result, - errors: errors, - result: new_user) + build_result(invited, new_user.errors) end def initialize_contract(new_user) diff --git a/config/locales/en.yml b/config/locales/en.yml index 9c280da01c..20de80e865 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -779,6 +779,7 @@ en: error_can_not_delete_custom_field: "Unable to delete custom field" error_can_not_delete_type: "This type contains work packages and cannot be deleted." error_can_not_delete_standard_type: "Standard types cannot be deleted." + error_can_not_invite_user: "Failed to send invitation to user." error_can_not_remove_role: "This role is in use and cannot be deleted." error_can_not_reopen_work_package_on_closed_version: "A work package assigned to a closed version cannot be reopened" error_check_user_and_role: "Please choose a user and a role." diff --git a/lib/api/v3/users/create_user.rb b/lib/api/v3/users/create_user.rb index 321f2806a0..a4ba8c6484 100644 --- a/lib/api/v3/users/create_user.rb +++ b/lib/api/v3/users/create_user.rb @@ -33,7 +33,6 @@ module API module V3 module Users module CreateUser - ## # Call the user create service for the current request # and return the service result API representation diff --git a/lib/api/v3/users/user_representer.rb b/lib/api/v3/users/user_representer.rb index baad6f8bec..e12f99c6fd 100644 --- a/lib/api/v3/users/user_representer.rb +++ b/lib/api/v3/users/user_representer.rb @@ -97,9 +97,9 @@ module API render_nil: true, # FIXME: remove the "is_a?" as soon as we have a dedicated group representer getter: ->(*) { - if self.is_a?(User) && !pref.hide_mail - mail - end + if is_a?(User) && !pref.hide_mail + mail + end } property :avatar, getter: -> (*) { avatar_url(represented) }, @@ -123,10 +123,9 @@ module API getter: -> (*) { nil }, render_nil: false, setter: -> (value, *) { - self.password = self.password_confirmation = value + self.password = self.password_confirmation = value } - def _type 'User' end diff --git a/spec/requests/api/v3/user/create_user_resource_spec.rb b/spec/requests/api/v3/user/create_user_resource_spec.rb index 4ac2b421f4..2d0baf2a13 100644 --- a/spec/requests/api/v3/user/create_user_resource_spec.rb +++ b/spec/requests/api/v3/user/create_user_resource_spec.rb @@ -39,22 +39,41 @@ describe ::API::V3::Users::UsersAPI do before do login_as(user) + end + + def send_request post path, parameters.to_json, 'CONTENT_TYPE' => 'application/json' end subject(:response) { last_response } - let(:errors) { - parse_json(subject.body)['_embedded']['errors'] - } + let(:errors) { parse_json(subject.body)['_embedded']['errors'] } + + shared_context 'represents the created user' do |expected_attributes| + it 'returns the represented user' do + send_request + + expect(subject.body).to have_json_type(Object).at_path('_links') + expect(subject.body) + .to be_json_eql('User'.to_json) + .at_path('_type') + + parameters.merge!(expected_attributes) if expected_attributes + + user = User.find_by!(login: parameters.fetch(:login, parameters[:email])) + expect(user.firstname).to eq(parameters[:firstName]) + expect(user.lastname).to eq(parameters[:lastName]) + expect(user.mail).to eq(parameters[:email]) + end + end describe 'empty request body' do - it 'should return 422' do + it 'returns an erroneous response' do + send_request + expect(response.status).to eq(422) - end - it 'has 5 validation errors' do expect(errors.count).to eq(5) - expect(errors.collect{ |el| el['_embedded']['details']['attribute']}) + expect(errors.collect { |el| el['_embedded']['details']['attribute'] }) .to contain_exactly('password', 'login', 'firstname', 'lastname', 'email') expect(subject.body) @@ -72,13 +91,15 @@ describe ::API::V3::Users::UsersAPI do firstName: 'Foo', lastName: 'Bar', email: 'foobar@example.org', - password: password, + password: password } } describe 'active status' do let(:status) { 'active' } it 'returns the represented user' do + send_request + expect(subject.body).not_to have_json_path("_embedded/errors") expect(subject.body).to have_json_type(Object).at_path('_links') expect(subject.body) @@ -86,22 +107,74 @@ describe ::API::V3::Users::UsersAPI do .at_path('_type') end - it 'creates the user' do - user = User.find_by!(login: 'myusername') - expect(user.firstname).to eq('Foo') - expect(user.lastname).to eq('Bar') - expect(user.mail).to eq('foobar@example.org') - end + it_behaves_like 'represents the created user' context 'empty password' do let(:password) { '' } it 'marks the password missing and too short' do + send_request + expect(errors.count).to eq(2) - expect(errors.collect{ |el| el['_embedded']['details']['attribute']}) + expect(errors.collect { |el| el['_embedded']['details']['attribute'] }) .to match_array %w(password password) end end end + + describe 'invited status' do + let(:status) { 'invited' } + let(:invitation_request) { + { + status: status, + email: 'foo@example.org' + } + } + + describe 'invitation successful' do + before do + expect(OpenProject::Notifications).to receive(:send) do |event, _| + expect(event).to eq 'user_invited' + end + end + + context 'only mail set' do + let(:parameters) { invitation_request } + + it_behaves_like 'represents the created user', + { firstName: 'foo', lastName: '@example.org' } + + it 'sets the other attributes' do + send_request + + user = User.find_by!(login: 'foo@example.org') + expect(user.firstname).to eq('foo') + expect(user.lastname).to eq('@example.org') + expect(user.mail).to eq('foo@example.org') + end + end + + context 'mail and name set' do + let(:parameters) { invitation_request.merge(firstName: 'First', lastName: 'Last') } + + it_behaves_like 'represents the created user' + end + end + + context 'missing email' do + let(:parameters) { { status: status } } + + it 'marks the mail as missing' do + send_request + + expect(subject.body) + .to be_json_eql('urn:openproject-org:api:v3:errors:PropertyConstraintViolation'.to_json) + .at_path('errorIdentifier') + expect(subject.body) + .to be_json_eql('email'.to_json) + .at_path('_embedded/details/attribute') + end + end + end end end