fixes #10482 - get external user group members only once during refresh

theforeman · May 19, 2015 · 0fd7412 · 0fd7412
1 parent df8887a
commit 0fd7412
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 10 deletions.
diff --git a/app/models/external_usergroup.rb b/app/models/external_usergroup.rb
@@ -14,14 +14,16 @@ class ExternalUsergroup < ActiveRecord::Base
   def refresh
     return false unless auth_source.respond_to?(:users_in_group)
 
-    current_users  = usergroup.users.map(&:login)
-    all_users      = usergroup.external_usergroups.map(&:users).flatten.uniq
+    current_users = usergroup.users.map(&:login)
+    my_users = users
+    all_other_users = (usergroup.external_usergroups - [self]).map(&:users)
+    all_users = (all_other_users + my_users).flatten.uniq
 
     # We need to make sure when we refresh a external_usergroup
     # other external_usergroup users remain in. Otherwise refreshing
     # a external user group with no users in will empty the user group.
     old_users = current_users - all_users
-    new_users = users - current_users
+    new_users = my_users - current_users
 
     usergroup.remove_users(old_users)
     usergroup.add_users(new_users)

diff --git a/test/functional/api/v2/external_usergroups_controller_test.rb b/test/functional/api/v2/external_usergroups_controller_test.rb
@@ -33,7 +33,7 @@ class Api::V2::ExternalUsergroupsControllerTest < ActionController::TestCase
   end
 
   test 'refresh external user group' do
-    ExternalUsergroup.any_instance.expects(:users).returns([]).twice
+    ExternalUsergroup.any_instance.expects(:users).returns([])
     put :refresh, { :usergroup_id => @external_usergroup.usergroup_id,
                     :id           => @external_usergroup.id }
     assert_response :success

diff --git a/test/unit/usergroup_test.rb b/test/unit/usergroup_test.rb
@@ -211,31 +211,31 @@ def populate_usergroups
     end
 
     test "delete user if not in LDAP directory" do
-      LdapFluff.any_instance.stubs(:valid_group?).at_least_once.with('aname').returns(false)
+      LdapFluff.any_instance.stubs(:valid_group?).with('aname').returns(false)
       @usergroup.users << users(:one)
       @usergroup.save
 
-      AuthSourceLdap.any_instance.expects(:users_in_group).at_least_once.with('aname').returns([])
+      AuthSourceLdap.any_instance.expects(:users_in_group).with('aname').returns([])
       @usergroup.external_usergroups.select { |eu| eu.name == 'aname'}.first.refresh
 
       refute_includes @usergroup.users, users(:one)
     end
 
     test "add user if in LDAP directory" do
-      LdapFluff.any_instance.stubs(:valid_group?).at_least_once.with('aname').returns(true)
+      LdapFluff.any_instance.stubs(:valid_group?).with('aname').returns(true)
       @usergroup.save
 
-      AuthSourceLdap.any_instance.expects(:users_in_group).at_least_once.with('aname').returns([users(:one).login])
+      AuthSourceLdap.any_instance.expects(:users_in_group).with('aname').returns([users(:one).login])
       @usergroup.external_usergroups.select { |eu| eu.name == 'aname'}.first.refresh
       assert_includes @usergroup.users, users(:one)
     end
 
     test "keep user if in LDAP directory" do
-      LdapFluff.any_instance.stubs(:valid_group?).at_least_once.with('aname').returns(true)
+      LdapFluff.any_instance.stubs(:valid_group?).with('aname').returns(true)
       @usergroup.users << users(:one)
       @usergroup.save
 
-      AuthSourceLdap.any_instance.expects(:users_in_group).at_least_once.with('aname').returns([users(:one).login])
+      AuthSourceLdap.any_instance.expects(:users_in_group).with('aname').returns([users(:one).login])
       @usergroup.external_usergroups.select { |eu| eu.name == 'aname'}.first.refresh
       assert_includes @usergroup.users, users(:one)
     end