do away with user-merge warnings when unnecessary

refs #CNVS-1959

remove session interaction, everything is
now tracked with parameters.  Also
removed the "confirm_merge" action which
does not appear to be used.

Test Plan:
1) login as an admin
2) try to merge two users together
3) as long as the users being merged are 
  not the same user, you should not get a warning
  about how the user cannot be merged with itself.
  (naturally, if you DID try to merge a user 
   into itself, you absolutely should receive
   that warning)

Change-Id: I98fdb925b3f6d14b4de98737f9af8b6093312847
Reviewed-on: https://gerrit.instructure.com/16692
Reviewed-by: Brian Palmer <brianp@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Clare Hetherington <clare@instructure.com>

Author: evizitei

app/controllers/users_controller.rb

@@ -85,9 +85,9 @@ class UsersController < ApplicationController
   include LinkedIn
   include DeliciousDiigo
   include SearchHelper
-  before_filter :require_user, :only => [:grades, :confirm_merge, :merge, :kaltura_session, :ignore_item, :ignore_stream_item, :close_notification, :mark_avatar_image, :user_dashboard, :toggle_dashboard, :masquerade, :external_tool]
+  before_filter :require_user, :only => [:grades, :merge, :kaltura_session, :ignore_item, :ignore_stream_item, :close_notification, :mark_avatar_image, :user_dashboard, :toggle_dashboard, :masquerade, :external_tool]
   before_filter :require_registered_user, :only => [:delete_user_service, :create_user_service]
-  before_filter :reject_student_view_student, :only => [:delete_user_service, :create_user_service, :confirm_merge, :merge, :user_dashboard, :masquerade]
+  before_filter :reject_student_view_student, :only => [:delete_user_service, :create_user_service, :merge, :user_dashboard, :masquerade]
   before_filter :require_self_registration, :only => [:new, :create]
 
   def grades
@@ -932,7 +932,7 @@ def media_download
   end
 
   def merge
-    @user_about_to_go_away = User.find_by_id(session[:merge_user_id]) if session[:merge_user_id].present?
+    @user_about_to_go_away = User.find(params[:user_id])
 
     if params[:new_user_id] && @true_user = User.find_by_id(params[:new_user_id])
       if @true_user.grants_right?(@current_user, session, :manage_logins) && @user_about_to_go_away.grants_right?(@current_user, session, :manage_logins)
@@ -947,11 +947,11 @@ def merge
     if @user_about_to_go_away && @user_that_will_still_be_around
       @user_about_to_go_away.move_to_user(@user_that_will_still_be_around)
       @user_that_will_still_be_around.touch
-      session.delete(:merge_user_id)
       flash[:notice] = t('user_merge_success', "User merge succeeded! %{first_user} and %{second_user} are now one and the same.", :first_user => @user_that_will_still_be_around.name, :second_user => @user_about_to_go_away.name)
     else
       flash[:error] = t('user_merge_fail', "User merge failed. Please make sure you have proper permission and try again.")
     end
+
     if @user_that_will_still_be_around == @current_user
       redirect_to user_profile_url(@current_user)
     elsif @user_that_will_still_be_around
@@ -963,47 +963,38 @@ def merge
 
   def admin_merge
     @user = User.find(params[:user_id])
-    pending_user_id = params[:pending_user_id] || session[:pending_user_id]
-    pending_other_error = get_pending_user_and_error(pending_user_id, params[:pending_user_id])
+    pending_other_error = get_pending_user_and_error(params[:pending_user_id])
     @other_user = User.find_by_id(params[:new_user_id]) if params[:new_user_id].present?
     if authorized_action(@user, @current_user, :manage_logins)
       flash[:error] = pending_other_error if pending_other_error.present?
+
       if @user && (params[:clear] || !@pending_other_user)
-        session[:pending_user_id] = @user.id
         @pending_other_user = nil
       end
-      if @other_user && @other_user.grants_right?(@current_user, session, :manage_logins)
-        session[:merge_user_id] = @user.id
-        session.delete(:pending_user_id)
-      else
+
+      unless @other_user && @other_user.grants_right?(@current_user, session, :manage_logins)
         @other_user = nil
       end
+
       render :action => 'admin_merge'
     end
   end
 
-  def get_pending_user_and_error(pending_user_id, entered_user_id)
+  def get_pending_user_and_error(pending_user_id)
     pending_other_error = nil
-    @pending_other_user = api_find_all(User, [pending_user_id]).first if pending_user_id.present?
-    @pending_other_user = nil unless @pending_other_user.try(:grants_right?, @current_user, session, :manage_logins)
-    if @pending_other_user == @user
-      @pending_other_user = nil
-      pending_other_error = t('cant_self_merge', "You can't merge an account with itself.")
-    elsif @pending_other_user.blank? && entered_user_id.present? && pending_other_error.blank?
-      pending_other_error = t('user_not_found', "No active user with that ID was found.")
-    end
-    return pending_other_error
-  end
 
-  def confirm_merge
-    @user = User.find_by_id(session[:merge_user_id]) if session[:merge_user_id].present?
-    if @user && @user != @current_user
-      render :action => 'confirm_merge'
-    else
-      session[:merge_user_id] = @current_user.id
-      store_location(user_confirm_merge_url(@current_user.id))
-      render :action => 'merge'
+    if pending_user_id.present?
+      @pending_other_user = api_find_all(User, [pending_user_id]).first
+      @pending_other_user = nil unless @pending_other_user.try(:grants_right?, @current_user, session, :manage_logins)
+      if @pending_other_user == @user
+        @pending_other_user = nil
+        pending_other_error = t('cant_self_merge', "You can't merge an account with itself.")
+      elsif @pending_other_user.blank? && pending_other_error.blank?
+        pending_other_error = t('user_not_found', "No active user with that ID was found.")
+      end
     end
+
+    pending_other_error
   end
 
   def assignments_needing_grading

app/views/users/admin_merge.html.erb

@@ -65,9 +65,7 @@
       </span>
     <% else %>
       <p><%= t('merge_user_initial_instructions', "You've selected to merge the user, %{user_name} (%{user_email}) with another account.
-      You can search for the user you'd like to merge with this user using the form below, or 
-      just navigate to that user's page and click \"Merge User\" again to merge someone else with this
-      user.", :user_name => @user.name, :user_email => @user.email) %></p>
+      You can search for the user you'd like to merge with this user using the form below.", :user_name => @user.name, :user_email => @user.email) %></p>
       <table>
         <tr>
           <td style="vertical-align: top; padding: 10px 30px 10px;">

app/views/users/confirm_merge.html.erb

@@ -529,7 +529,6 @@ def add_zip_file_imports(context)
     user.assignments_needing_grading 'assignments_needing_grading', :controller => 'users', :action => 'assignments_needing_grading'
     user.assignments_needing_submitting 'assignments_needing_submitting', :controller => 'users', :action => 'assignments_needing_submitting'
     user.admin_merge 'admin_merge', :controller => 'users', :action => 'admin_merge', :conditions => {:method => :get}
-    user.confirm_merge 'merge', :controller => 'users', :action => 'confirm_merge', :conditions => {:method => :get}
     user.merge 'merge', :controller => 'users', :action => 'merge', :conditions => {:method => :post}
     user.grades 'grades', :controller => 'users', :action => 'grades'
     user.resources :user_notes

config/routes.rb

@@ -372,32 +372,49 @@
     end
 
     context 'account admin creating users' do
-      it "should create a pre_registered user (in the correct account)" do
-        account = Account.create!
-        user_with_pseudonym(:account => account)
-        account.add_user(@user)
-        user_session(@user, @pseudonym)
-        post 'create', :format => 'json', :account_id => account.id, :pseudonym => { :unique_id => 'jacob@instructure.com', :sis_user_id => 'testsisid' }, :user => { :name => 'Jacob Fugal' }
-        response.should be_success
-        p = Pseudonym.find_by_unique_id('jacob@instructure.com')
-        p.account_id.should == account.id
-        p.should be_active
-        p.sis_user_id.should == 'testsisid'
-        p.user.should be_pre_registered
-      end
 
-      it "should create users with non-email pseudonyms" do
-        account = Account.create!
-        user_with_pseudonym(:account => account)
-        account.add_user(@user)
-        user_session(@user, @pseudonym)
-        post 'create', :format => 'json', :account_id => account.id, :pseudonym => { :unique_id => 'jacob', :sis_user_id => 'testsisid' }, :user => { :name => 'Jacob Fugal' }
-        response.should be_success
-        p = Pseudonym.find_by_unique_id('jacob')
-        p.account_id.should == account.id
-        p.should be_active
-        p.sis_user_id.should == 'testsisid'
-        p.user.should be_pre_registered
+      describe 'successfully' do
+        let!(:account) { Account.create! }
+
+        before do
+          user_with_pseudonym(:account => account)
+          account.add_user(@user)
+          user_session(@user, @pseudonym)
+        end
+
+        it "should create a pre_registered user (in the correct account)" do
+          post 'create', :format => 'json', :account_id => account.id, :pseudonym => { :unique_id => 'jacob@instructure.com', :sis_user_id => 'testsisid' }, :user => { :name => 'Jacob Fugal' }
+          response.should be_success
+          p = Pseudonym.find_by_unique_id('jacob@instructure.com')
+          p.account_id.should == account.id
+          p.should be_active
+          p.sis_user_id.should == 'testsisid'
+          p.user.should be_pre_registered
+        end
+
+        it "should create users with non-email pseudonyms" do
+          post 'create', :format => 'json', :account_id => account.id, :pseudonym => { :unique_id => 'jacob', :sis_user_id => 'testsisid' }, :user => { :name => 'Jacob Fugal' }
+          response.should be_success
+          p = Pseudonym.find_by_unique_id('jacob')
+          p.account_id.should == account.id
+          p.should be_active
+          p.sis_user_id.should == 'testsisid'
+          p.user.should be_pre_registered
+        end
+
+
+        it "should not require acceptance of the terms or birthdate" do
+          post 'create', :account_id => account.id, :pseudonym => { :unique_id => 'jacob@instructure.com' }, :user => { :name => 'Jacob Fugal' }
+          response.should be_success
+        end
+
+        it "should allow setting a password" do
+          post 'create', :account_id => account.id, :pseudonym => { :unique_id => 'jacob@instructure.com', :password => 'asdfasdf', :password_confirmation => 'asdfasdf' }, :user => { :name => 'Jacob Fugal' }
+          u = User.find_by_name 'Jacob Fugal'
+          u.should be_present
+          u.pseudonym.should_not be_password_auto_generated
+        end
+
       end
 
       it "should not allow an admin to set the sis id when creating a user if they don't have privileges to manage sis" do
@@ -447,36 +464,6 @@
         p = Pseudonym.find_by_unique_id('jacob@instructure.com')
         Message.find(:first, :conditions => { :communication_channel_id => p.user.email_channel.id, :notification_id => notification.id }).should be_nil
       end
-
-      it "should not require acceptance of the terms" do
-        account = Account.create!
-        user_with_pseudonym(:account => account)
-        account.add_user(@user)
-        user_session(@user, @pseudonym)
-        post 'create', :account_id => account.id, :pseudonym => { :unique_id => 'jacob@instructure.com' }, :user => { :name => 'Jacob Fugal' }
-        response.should be_success
-      end
-
-      it "should not require the birthdate" do
-        account = Account.create!
-        user_with_pseudonym(:account => account)
-        account.add_user(@user)
-        user_session(@user, @pseudonym)
-        post 'create', :account_id => account.id, :pseudonym => { :unique_id => 'jacob@instructure.com' }, :user => { :name => 'Jacob Fugal' }
-        response.should be_success
-      end
-
-      it "should allow setting a password" do
-        account = Account.create!
-        user_with_pseudonym(:account => account)
-        account.add_user(@user)
-        user_session(@user, @pseudonym)
-        post 'create', :account_id => account.id, :pseudonym => { :unique_id => 'jacob@instructure.com', :password => 'lolwtf', :password_confirmation => 'lolwtf' }, :user => { :name => 'Jacob Fugal' }
-        response.should be_success
-        u = User.find_by_name 'Jacob Fugal'
-        u.should be_present
-        u.pseudonym.should_not be_password_auto_generated
-      end
     end
   end
 
@@ -643,11 +630,32 @@
   end
 
   describe "GET 'admin_merge'" do
-    it "should not allow you to view any user by id" do
+    let(:account) { Account.create! }
+
+    before do
       account_admin_user
       user_session(@admin)
-      user_with_pseudonym(:account => Account.create!)
+    end
+
+    describe 'as site admin' do
+      before { Account.site_admin.add_user(@admin) }
+
+      it 'warns about merging a user with itself' do
+        user = User.create!
+        get 'admin_merge', :user_id => user.id, :pending_user_id => user.id
+        flash[:error].should == 'You can\'t merge an account with itself.'
+      end
 
+      it 'does not issue warning if the users are different' do
+        user = User.create!
+        other_user = User.create!
+        get 'admin_merge', :user_id => user.id, :pending_user_id => other_user.id
+        flash[:error].should be_nil
+      end
+    end
+
+    it "should not allow you to view any user by id" do
+      user_with_pseudonym(:account => account)
       get 'admin_merge', :user_id => @admin.id, :pending_user_id => @user.id
       response.should be_success
       assigns[:pending_other_user].should be_nil

spec/controllers/users_controller_spec.rb

@@ -253,21 +253,15 @@ def disable_avatars!
       response.should be_success
       assigns['pending_other_user'].should == @admin
       assigns['other_user'].should be_nil
-      session[:pending_user_id].should be_nil
-      session[:merge_user_id].should be_nil
 
       get user_admin_merge_url(@user, :new_user_id => @admin.id)
       response.should be_success
       assigns['pending_other_user'].should be_nil
       assigns['other_user'].should == @admin
-      session[:pending_user_id].should be_nil
-      session[:merge_user_id].should == @user.id
 
       post user_merge_url(@user, :new_user_id => @admin.id)
       response.should redirect_to(user_profile_url(@admin))
 
-      session[:pending_user_id].should be_nil
-      session[:merge_user_id].should be_nil
       @user.reload.should be_deleted
       @admin.reload.should be_registered
       @admin.pseudonyms.count.should == 2

spec/integration/users_controller_spec.rb

@@ -79,8 +79,10 @@
   end
 
   context "admin merge" do
-    def setup_user_merge(users)
-      2.times { |i| get "/users/#{users[i].id}/admin_merge" }
+    def setup_user_merge(from_user, into_user)
+      get "/users/#{from_user.id}/admin_merge"
+      f('#manual_user_id').send_keys(into_user.id)
+      expect_new_page_load { f('button[type="submit"]').click }
     end
 
     def reload_users(users)
@@ -113,19 +115,17 @@ def validate_login_info(user_id)
       @users = [@student_1, @student_2]
     end
 
-    it "should merge user a with user b with navigate to another user function" do
-      setup_user_merge(@users)
+    it "should merge user a with user b" do
+      setup_user_merge(@student_2, @student_1)
       submit_merge
       reload_users(@users)
       @student_1.workflow_state.should == 'registered'
       @student_2.workflow_state.should == 'deleted'
       validate_login_info(@student_1_id)
     end
 
-    it "should merge user b with user a with enter user id function" do
-      get "/users/#{@student_1.id}/admin_merge"
-      f('#manual_user_id').send_keys(@student_2.id)
-      expect_new_page_load { f('button[type="submit"]').click }
+    it "should merge user b with user a" do
+      setup_user_merge(@student_1, @student_2)
       submit_merge
       reload_users(@users)
       @student_1.workflow_state.should == 'deleted'
@@ -134,7 +134,7 @@ def validate_login_info(user_id)
     end
 
     it "should validate switching the users to merge" do
-      setup_user_merge(@users)
+      setup_user_merge(@student_2, @student_1)
       user_names = ff('.result td')
       user_names[0].should include_text(@student_2.name)
       user_names[1].should include_text(@student_1.name)
@@ -151,7 +151,7 @@ def validate_login_info(user_id)
     end
 
     it "should cancel a merge and validate both users still exist" do
-      setup_user_merge(@users)
+      setup_user_merge(@student_2, @student_1)
       expect_new_page_load { f('#prepare_to_merge').click }
       expect_new_page_load { f('.button-secondary').click }
       f('#courses_menu_item').should be_displayed