BUGFIX: Member::mapInGroups() throws SQL error

Renamed the Member::mapInGroups() to Member::map_in_groups() since it's a static method and throws deprecation message if using the old variant.
Rewrote the mapInGroups to use a more ORMy way of fetching Members for a set of groups and included a test for.

security/Member.php

@@ -640,8 +640,8 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			&& Member::$notify_password_change
 		) {
 			$e = Member_ChangePasswordEmail::create();
-			$e->populateTemplate($this);
-			$e->setTo($this->Email);
+			$e->populateTemplate($this);
+			$e->setTo($this->Email);
 			$e->send();
 		}
 
@@ -965,38 +965,55 @@ class Member extends DataObject implements TemplateGlobalProvider {
 		return $map;
 	}
 
-
 	/**
 	 * Get a member SQLMap of members in specific groups
-	 *
-	 * @param mixed $groups Optional groups to include in the map. If NULL is
-	 *                      passed, all groups are returned, i.e.
-	 *                      {@link map()} will be called.
+	 * 
+	 * If no $groups is passed, all members will be returned
+	 * 
+	 * @param mixed $groups - takes a SS_List, an array or a single Group.ID
 	 * @return SQLMap Returns an SQLMap that returns all Member data.
 	 * @see map()
-	 *
-	 * @todo Improve documentation of this function! (Markus)
 	 */
 	public static function mapInGroups($groups = null) {
-		if(!$groups)
-			return Member::map();
-
+		Deprecation::notice('3.0', 'Use Member::map_in_groups() instead');
+		return self::map_in_groups();
+	}
+	
+	/**
+	 * Get a member SQLMap of members in specific groups
+	 * 
+	 * If no $groups is passed, all members will be returned
+	 * 
+	 * @param mixed $groups - takes a SS_List, an array or a single Group.ID
+	 * @return SQLMap Returns an SQLMap that returns all Member data.
+	 * @see map()
+	 */
+	public static function map_in_groups($groups = null) {
 		$groupIDList = array();
-
-		if(is_a($groups, 'SS_List')) {
-			foreach( $groups as $group )
+		
+		if($groups instanceof SS_List) {
+			foreach( $groups as $group ) {
 				$groupIDList[] = $group->ID;
+			}
 		} elseif(is_array($groups)) {
 			$groupIDList = $groups;
-		} else {
+		} elseif($groups) {
 			$groupIDList[] = $groups;
 		}
-
-		if(empty($groupIDList))
-			return Member::map();
-
-		return DataList::create("Member")->where("\"GroupID\" IN (" . implode( ',', $groupIDList ) . ")")
-			->sort("\"Surname\", \"FirstName\"")->map();
+		
+		// No groups, return all Members
+		if(!$groupIDList) {
+			return Member::get()->sort(array('Surname'=>'ASC', 'FirstName'=>'ASC'))->map();
+		}
+		
+		$membersList = new ArrayList();
+		// This is a bit ineffective, but follow the ORM style
+		foreach(Group::get()->byIDs($groupIDList) as $group) {
+			$membersList->merge($group->Members());
+		}
+		
+		$membersList->removeDuplicates('ID');
+		return $membersList->map();
 	}
 
 

tests/security/MemberTest.php

@@ -564,6 +564,29 @@ class MemberTest extends FunctionalTest {
 			'Adding new admin group relation is allowed for admin members'
 		);
 	}
+	
+	/**
+	 * Test that all members are returned
+	 */
+	function testMap_in_groupsReturnsAll() {
+		$members = Member::map_in_groups();
+		$this->assertEquals(13, count($members), 'There are 12 members in the mock plus a fake admin');
+	}
+	
+	/**
+	 * Test that only admin members are returned 
+	 */
+	function testMap_in_groupsReturnsAdmins() {
+		$adminID = $this->objFromFixture('Group', 'admingroup')->ID;
+		$members = Member::map_in_groups($adminID);
+		
+		$admin = $this->objFromFixture('Member', 'admin');
+		$otherAdmin = $this->objFromFixture('Member', 'other-admin');
+		
+		$this->assertTrue(in_array($admin->getTitle(), $members), $admin->getTitle().' should be in the returned list.');
+		$this->assertTrue(in_array($otherAdmin->getTitle(), $members), $otherAdmin->getTitle().' should be in the returned list.');
+		$this->assertEquals(2, count($members), 'There should be 2 members from the admin group');
+	}
 
 	/**
 	 * Add the given array of member extensions as class names.