Handle invitation flow

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.
   #

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)

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."

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

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

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