Permalink
Browse files

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.
  • Loading branch information...
1 parent 7dcfdb0 commit bbe3879eaac0c8b00195550ae792c57c9c6b8cc4 @stojg stojg committed May 10, 2012
Showing with 62 additions and 22 deletions.
  1. +39 −22 security/Member.php
  2. +23 −0 tests/security/MemberTest.php
View
@@ -640,8 +640,8 @@ function onBeforeWrite() {
&& 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 @@ public static function map($filter = "", $sort = "", $blank="") {
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();
}
@@ -564,6 +564,29 @@ function testOnChangeGroups() {
'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.

0 comments on commit bbe3879

Please sign in to comment.