Reviews code after first discussion

8 files changed, 47 insertions, 39 deletions

diff --git a/app/controllers/concerns/accepts_pending_invitations.rb b/app/controllers/concerns/accepts_pending_invitations.rb
index 4de1a39772a..b062f930c7e 100644
--- a/app/controllers/concerns/accepts_pending_invitations.rb
+++ b/app/controllers/concerns/accepts_pending_invitations.rb
@@ -1,14 +1,15 @@
 module AcceptsPendingInvitations
   extend ActiveSupport::Concern
 
-  def accept_pending_invitations_for(resource)
-    resource.accept_pending_invitations.each do |accepted_invite_token|
-      clear_stored_location_for(resource) if accept_invite_path(id: accepted_invite_token)
-    end
+  def accept_pending_invitations
+    return unless resource.active_for_authentication?
+
+    clear_stored_location_for(resource) if Member.accept_pending_invitations(resource).any?
   end
 
   def clear_stored_location_for(resource)
-    # by calling stored_location_for devise removes the stored location from the session
-    stored_location_for(resource)
+    session_key = stored_location_key_for(resource)
+
+    session.delete(session_key)
   end
 end
diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index 14608614a51..7bc46a6ccc0 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -13,7 +13,7 @@ class ConfirmationsController < Devise::ConfirmationsController
   end
 
   def after_confirmation_path_for(resource_name, resource)
-    accept_pending_invitations_for(resource)
+    accept_pending_invitations
 
     # incoming resource can either be a :user or an :email
     if signed_in?(:user)
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index 62833977674..f5a222b3a48 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -17,7 +17,7 @@ class RegistrationsController < Devise::RegistrationsController
     end
 
     if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha
-      accept_pending_invitations_for(resource)
+      accept_pending_invitations
       super
     else
       flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
diff --git a/app/models/member.rb b/app/models/member.rb
index 68572f2e33a..b3ce7a5597f 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -192,6 +192,14 @@ class Member < ActiveRecord::Base
       Gitlab::Access.sym_options
     end
 
+    def accept_pending_invitations(user)
+      members = where(invite_email: user.email).invite
+
+      members.select do |member|
+        member.accept_invite!(user)
+      end
+    end
+
     private
 
     def parse_users_list(source, list)
diff --git a/app/models/user.rb b/app/models/user.rb
index 79bd75bbecd..dfef065f094 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -461,16 +461,6 @@ class User < ActiveRecord::Base
     skip_reconfirmation! if bool
   end
 
-  def accept_pending_invitations
-    accepted_invite_tokens = []
-    Member.where(invite_email: email).invite.each do |member|
-      accepted_invite_tokens << member.invite_token
-      member.accept_invite!(self)
-    end
-
-    accepted_invite_tokens
-  end
-
   def generate_reset_token
     @reset_token, enc = Devise.token_generator.generate(self.class, :reset_password_token)
 
diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb
index 1db0c659150..4d1c1e9c00a 100644
--- a/spec/features/invites_spec.rb
+++ b/spec/features/invites_spec.rb
@@ -15,9 +15,9 @@ describe 'Invites' do
   end
 
   def confirm_email_and_sign_in(new_user)
-    User.find_by_email(new_user.email).confirm
+    new_user_token = User.find_by_email(new_user.email).confirmation_token
 
-    visit new_user_session_path
+    visit user_confirmation_path(confirmation_token: new_user_token)
     fill_in_sign_in_form(new_user)
   end
 
@@ -122,8 +122,8 @@ describe 'Invites' do
     let!(:project_invite) { create(:project_member, :invited, project: project, invite_email: invite_email) }
 
     before do
-      visit invite_path(group_invite.raw_invite_token)
       stub_application_setting(send_user_confirmation_email: send_email_confirmation)
+      visit invite_path(group_invite.raw_invite_token)
     end
 
     context 'email confirmation disabled' do
@@ -132,6 +132,7 @@ describe 'Invites' do
       it 'signs up and redirects to the dashboard page with all the projects/groups invitations automatically accepted' do
         fill_in_sign_up_form(new_user)
 
+        expect(current_path).to eq(dashboard_projects_path)
         expect(page).to have_content(project.full_name)
         visit group_path(group)
         expect(page).to have_content(group.full_name)
@@ -160,6 +161,14 @@ describe 'Invites' do
         expect(page).to have_content(group.full_name)
       end
 
+      it "doesn't accept invitations until the user confirm his email" do
+        fill_in_sign_up_form(new_user)
+        sign_in(owner)
+
+        visit project_project_members_path(project)
+        expect(page).to have_content 'Invited'
+      end
+
       context 'the user sign-up using a different email address' do
         let(:invite_email) { build_stubbed(:user).email }
 
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index c64cdf8f812..1e8e0786f1a 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -422,6 +422,24 @@ describe Member do
     end
   end
 
+  describe '.accept_pending_invitations' do
+    let(:user) { create(:user, email: 'user@email.com') }
+    let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) }
+    let!(:group_member_invite) { create(:group_member, :invited, invite_email: user.email) }
+    let!(:external_project_member_invite) { create(:project_member, :invited, invite_email: 'external@email.com') }
+    let!(:external_group_member_invite) { create(:group_member, :invited, invite_email: 'external@email.com') }
+
+    it 'accepts all the user pending invitations and returns the accepted_members' do
+      accepted_members = described_class.accept_pending_invitations(user)
+
+      expect(accepted_members).to match_array([project_member_invite, group_member_invite])
+      expect(group_member_invite.reload).not_to be_invite
+      expect(project_member_invite.reload).not_to be_invite
+      expect(external_project_member_invite.reload).to be_invite
+      expect(external_group_member_invite.reload).to be_invite
+    end
+  end
+
   describe '#accept_request' do
     let(:member) { create(:project_member, requested_at: Time.now.utc) }
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a0b4de315ac..ad094b3ed48 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -2132,24 +2132,6 @@ describe User do
     end
   end
 
-  describe '#accept_pending_invitations' do
-    let(:user) { create(:user, email: 'user@email.com') }
-    let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) }
-    let!(:group_member_invite) { create(:group_member, :invited, invite_email: user.email) }
-    let!(:external_project_member_invite) { create(:project_member, :invited, invite_email: 'external@email.com') }
-    let!(:external_group_member_invite) { create(:group_member, :invited, invite_email: 'external@email.com') }
-
-    it 'accepts all the user members pending invitations and returns the accepted invite_tokens' do
-      accepted_invite_tokens = user.accept_pending_invitations
-
-      expect(accepted_invite_tokens).to match_array([project_member_invite.invite_token, group_member_invite.invite_token])
-      expect(group_member_invite.reload).not_to be_invite
-      expect(project_member_invite.reload).not_to be_invite
-      expect(external_project_member_invite.reload).to be_invite
-      expect(external_group_member_invite.reload).to be_invite
-    end
-  end
-
   describe '#update_two_factor_requirement' do
     let(:user) { create :user }